diff mbox series

[1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register

Message ID 1617881896-3164-2-git-send-email-yangyicong@hisilicon.com (mailing list archive)
State New
Headers show
Series Simplify codes with devm_add_action_or_reset | expand

Commit Message

Yicong Yang April 8, 2021, 11:38 a.m. UTC
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(-)

Comments

Alexandru Ardelean April 8, 2021, 1 p.m. UTC | #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.
>

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
>
Alexandru Ardelean April 8, 2021, 1:04 p.m. UTC | #2
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
>
Yicong Yang April 9, 2021, 7:39 a.m. UTC | #3
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
>>
> 
> .
>
Jonathan Cameron April 11, 2021, 2:12 p.m. UTC | #4
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;
>  }
Yicong Yang April 12, 2021, 9:10 a.m. UTC | #5
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;
>>  }
> 
> 
> .
>
Jonathan Cameron April 24, 2021, 12:31 p.m. UTC | #6
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 mbox series

Patch

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;
 }