| Message ID | 1617881896-3164-2-git-send-email-yangyicong@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Simplify codes with devm_add_action_or_reset | expand |
On Thu, Apr 8, 2021 at 2:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote: > > Use devm_add_action_or_reset() instead of devres_alloc() and > devres_add(), which works the same. This will simplify the > code. There is no functional changes. > Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com> > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/iio/adc/adi-axi-adc.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c > index 9109da2..575a63f 100644 > --- a/drivers/iio/adc/adi-axi-adc.c > +++ b/drivers/iio/adc/adi-axi-adc.c > @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv) > kfree(cl); > } > > -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res) > +static void devm_adi_axi_adc_conv_release(void *conv) > { > - adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res); > + adi_axi_adc_conv_unregister(conv); > } > > struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev, > size_t sizeof_priv) > { > - struct adi_axi_adc_conv **ptr, *conv; > - > - ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr), > - GFP_KERNEL); > - if (!ptr) > - return ERR_PTR(-ENOMEM); > + struct adi_axi_adc_conv *conv; > + int ret; > > conv = adi_axi_adc_conv_register(dev, sizeof_priv); > - if (IS_ERR(conv)) { > - devres_free(ptr); > + if (IS_ERR(conv)) > return ERR_CAST(conv); > - } > > - *ptr = conv; > - devres_add(dev, ptr); > + ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release, > + conv); > + if (ret) > + return ERR_PTR(ret); > > return conv; > } > -- > 2.8.1 >
On Thu, Apr 8, 2021 at 2:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote: > > Use devm_add_action_or_reset() instead of devres_alloc() and > devres_add(), which works the same. This will simplify the > code. There is no functional changes. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/iio/adc/adi-axi-adc.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c > index 9109da2..575a63f 100644 > --- a/drivers/iio/adc/adi-axi-adc.c > +++ b/drivers/iio/adc/adi-axi-adc.c > @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv) > kfree(cl); > } > > -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res) > +static void devm_adi_axi_adc_conv_release(void *conv) > { > - adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res); On a second pass, I'm wondering if this requires a cast like static void devm_adi_axi_adc_conv_release(void *data) { struct adi_axi_adc_conv *conv = data; If the compiler doesn't complain, that I'm fine > + adi_axi_adc_conv_unregister(conv); > } > > struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev, > size_t sizeof_priv) > { > - struct adi_axi_adc_conv **ptr, *conv; > - > - ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr), > - GFP_KERNEL); > - if (!ptr) > - return ERR_PTR(-ENOMEM); > + struct adi_axi_adc_conv *conv; > + int ret; > > conv = adi_axi_adc_conv_register(dev, sizeof_priv); > - if (IS_ERR(conv)) { > - devres_free(ptr); > + if (IS_ERR(conv)) > return ERR_CAST(conv); > - } > > - *ptr = conv; > - devres_add(dev, ptr); > + ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release, > + conv); > + if (ret) > + return ERR_PTR(ret); > > return conv; > } > -- > 2.8.1 >
On 2021/4/8 21:04, Alexandru Ardelean wrote: > On Thu, Apr 8, 2021 at 2:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote: >> >> Use devm_add_action_or_reset() instead of devres_alloc() and >> devres_add(), which works the same. This will simplify the >> code. There is no functional changes. >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> drivers/iio/adc/adi-axi-adc.c | 22 +++++++++------------- >> 1 file changed, 9 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c >> index 9109da2..575a63f 100644 >> --- a/drivers/iio/adc/adi-axi-adc.c >> +++ b/drivers/iio/adc/adi-axi-adc.c >> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv) >> kfree(cl); >> } >> >> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res) >> +static void devm_adi_axi_adc_conv_release(void *conv) >> { >> - adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res); > > On a second pass, I'm wondering if this requires a cast like > > static void devm_adi_axi_adc_conv_release(void *data) > { > struct adi_axi_adc_conv *conv = data; > > If the compiler doesn't complain, that I'm fine > no warn noticed when make W=1. >> + adi_axi_adc_conv_unregister(conv); >> } >> >> struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev, >> size_t sizeof_priv) >> { >> - struct adi_axi_adc_conv **ptr, *conv; >> - >> - ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr), >> - GFP_KERNEL); >> - if (!ptr) >> - return ERR_PTR(-ENOMEM); >> + struct adi_axi_adc_conv *conv; >> + int ret; >> >> conv = adi_axi_adc_conv_register(dev, sizeof_priv); >> - if (IS_ERR(conv)) { >> - devres_free(ptr); >> + if (IS_ERR(conv)) >> return ERR_CAST(conv); >> - } >> >> - *ptr = conv; >> - devres_add(dev, ptr); >> + ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release, >> + conv); >> + if (ret) >> + return ERR_PTR(ret); >> >> return conv; >> } >> -- >> 2.8.1 >> > > . >
On Thu, 8 Apr 2021 19:38:10 +0800 Yicong Yang <yangyicong@hisilicon.com> wrote: > Use devm_add_action_or_reset() instead of devres_alloc() and > devres_add(), which works the same. This will simplify the > code. There is no functional changes. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/iio/adc/adi-axi-adc.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c > index 9109da2..575a63f 100644 > --- a/drivers/iio/adc/adi-axi-adc.c > +++ b/drivers/iio/adc/adi-axi-adc.c > @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv) > kfree(cl); > } > > -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res) > +static void devm_adi_axi_adc_conv_release(void *conv) > { > - adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res); > + adi_axi_adc_conv_unregister(conv); > } > > struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev, > size_t sizeof_priv) > { > - struct adi_axi_adc_conv **ptr, *conv; > - > - ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr), > - GFP_KERNEL); > - if (!ptr) > - return ERR_PTR(-ENOMEM); > + struct adi_axi_adc_conv *conv; > + int ret; > > conv = adi_axi_adc_conv_register(dev, sizeof_priv); > - if (IS_ERR(conv)) { > - devres_free(ptr); > + if (IS_ERR(conv)) > return ERR_CAST(conv); Is that ERR_CAST() needed here? conv is already of the right type so we don't need to cast it to a void * and back gain. Obviously was there before an not needed either, but might as well tidy it up whilst we are here! Thanks, Jonathan > - } > > - *ptr = conv; > - devres_add(dev, ptr); > + ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release, > + conv); > + if (ret) > + return ERR_PTR(ret); > > return conv; > }
On 2021/4/11 22:12, Jonathan Cameron wrote: > On Thu, 8 Apr 2021 19:38:10 +0800 > Yicong Yang <yangyicong@hisilicon.com> wrote: > >> Use devm_add_action_or_reset() instead of devres_alloc() and >> devres_add(), which works the same. This will simplify the >> code. There is no functional changes. >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> drivers/iio/adc/adi-axi-adc.c | 22 +++++++++------------- >> 1 file changed, 9 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c >> index 9109da2..575a63f 100644 >> --- a/drivers/iio/adc/adi-axi-adc.c >> +++ b/drivers/iio/adc/adi-axi-adc.c >> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv) >> kfree(cl); >> } >> >> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res) >> +static void devm_adi_axi_adc_conv_release(void *conv) >> { >> - adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res); >> + adi_axi_adc_conv_unregister(conv); >> } >> >> struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev, >> size_t sizeof_priv) >> { >> - struct adi_axi_adc_conv **ptr, *conv; >> - >> - ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr), >> - GFP_KERNEL); >> - if (!ptr) >> - return ERR_PTR(-ENOMEM); >> + struct adi_axi_adc_conv *conv; >> + int ret; >> >> conv = adi_axi_adc_conv_register(dev, sizeof_priv); >> - if (IS_ERR(conv)) { >> - devres_free(ptr); >> + if (IS_ERR(conv)) >> return ERR_CAST(conv); > > Is that ERR_CAST() needed here? conv is already of the > right type so we don't need to cast it to a void * and back gain. > Obviously was there before an not needed either, but might as well > tidy it up whilst we are here! sure. thanks for the hint. I didn't notice this. will drop the ERR_CAST. thanks! > > Thanks, > > Jonathan > > > >> - } >> >> - *ptr = conv; >> - devres_add(dev, ptr); >> + ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release, >> + conv); >> + if (ret) >> + return ERR_PTR(ret); >> >> return conv; >> } > > > . >
On Mon, 12 Apr 2021 17:10:17 +0800 Yicong Yang <yangyicong@hisilicon.com> wrote: > On 2021/4/11 22:12, Jonathan Cameron wrote: > > On Thu, 8 Apr 2021 19:38:10 +0800 > > Yicong Yang <yangyicong@hisilicon.com> wrote: > > > >> Use devm_add_action_or_reset() instead of devres_alloc() and > >> devres_add(), which works the same. This will simplify the > >> code. There is no functional changes. > >> > >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > >> --- > >> drivers/iio/adc/adi-axi-adc.c | 22 +++++++++------------- > >> 1 file changed, 9 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c > >> index 9109da2..575a63f 100644 > >> --- a/drivers/iio/adc/adi-axi-adc.c > >> +++ b/drivers/iio/adc/adi-axi-adc.c > >> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv) > >> kfree(cl); > >> } > >> > >> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res) > >> +static void devm_adi_axi_adc_conv_release(void *conv) > >> { > >> - adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res); > >> + adi_axi_adc_conv_unregister(conv); > >> } > >> > >> struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev, > >> size_t sizeof_priv) > >> { > >> - struct adi_axi_adc_conv **ptr, *conv; > >> - > >> - ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr), > >> - GFP_KERNEL); > >> - if (!ptr) > >> - return ERR_PTR(-ENOMEM); > >> + struct adi_axi_adc_conv *conv; > >> + int ret; > >> > >> conv = adi_axi_adc_conv_register(dev, sizeof_priv); > >> - if (IS_ERR(conv)) { > >> - devres_free(ptr); > >> + if (IS_ERR(conv)) > >> return ERR_CAST(conv); > > > > Is that ERR_CAST() needed here? conv is already of the > > right type so we don't need to cast it to a void * and back gain. > > Obviously was there before an not needed either, but might as well > > tidy it up whilst we are here! > > sure. thanks for the hint. I didn't notice this. will drop the ERR_CAST. > > thanks! Rather than waste your time doing a v2 for such a trivial change, I've applied the patch and tweaked that whilst doing so. Applied to the togreg branch of iio.git and pushed out as testing to let 0-day poke at it etc. Thanks, Jonathan > > > > > Thanks, > > > > Jonathan > > > > > > > >> - } > >> > >> - *ptr = conv; > >> - devres_add(dev, ptr); > >> + ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release, > >> + conv); > >> + if (ret) > >> + return ERR_PTR(ret); > >> > >> return conv; > >> } > > > > > > . > > >
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c index 9109da2..575a63f 100644 --- a/drivers/iio/adc/adi-axi-adc.c +++ b/drivers/iio/adc/adi-axi-adc.c @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv) kfree(cl); } -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res) +static void devm_adi_axi_adc_conv_release(void *conv) { - adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res); + adi_axi_adc_conv_unregister(conv); } struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev, size_t sizeof_priv) { - struct adi_axi_adc_conv **ptr, *conv; - - ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr), - GFP_KERNEL); - if (!ptr) - return ERR_PTR(-ENOMEM); + struct adi_axi_adc_conv *conv; + int ret; conv = adi_axi_adc_conv_register(dev, sizeof_priv); - if (IS_ERR(conv)) { - devres_free(ptr); + if (IS_ERR(conv)) return ERR_CAST(conv); - } - *ptr = conv; - devres_add(dev, ptr); + ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release, + conv); + if (ret) + return ERR_PTR(ret); return conv; }
Use devm_add_action_or_reset() instead of devres_alloc() and devres_add(), which works the same. This will simplify the code. There is no functional changes. Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- drivers/iio/adc/adi-axi-adc.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)