diff mbox series

[5/7] iio: core: simplify some devm functions

Message ID 1617881896-3164-6-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/industrialio-core.c | 43 +++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 27 deletions(-)

Comments

Alexandru Ardelean April 8, 2021, 1:09 p.m. UTC | #1
On Thu, Apr 8, 2021 at 2:41 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/industrialio-core.c | 43 +++++++++++++++--------------------------
>  1 file changed, 16 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 7db761a..2dfbed3 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1627,9 +1627,9 @@ void iio_device_free(struct iio_dev *dev)
>  }
>  EXPORT_SYMBOL(iio_device_free);
>
> -static void devm_iio_device_release(struct device *dev, void *res)
> +static void devm_iio_device_release(void *iio_dev)
>  {
> -       iio_device_free(*(struct iio_dev **)res);
> +       iio_device_free(iio_dev);
>  }
>
>  /**
> @@ -1645,20 +1645,17 @@ static void devm_iio_device_release(struct device *dev, void *res)
>   */
>  struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)
>  {
> -       struct iio_dev **ptr, *iio_dev;
> -
> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
> -                          GFP_KERNEL);
> -       if (!ptr)
> -               return NULL;
> +       struct iio_dev *iio_dev;
> +       int ret;
>
>         iio_dev = iio_device_alloc(parent, sizeof_priv);
> -       if (iio_dev) {
> -               *ptr = iio_dev;
> -               devres_add(parent, ptr);
> -       } else {
> -               devres_free(ptr);
> -       }
> +       if (!iio_dev)
> +               return iio_dev;

This is correct.
But the preference is usually:
        if (!iio_dev)
            return NULL;

> +
> +       ret = devm_add_action_or_reset(parent, devm_iio_device_release,
> +                                      iio_dev);
> +       if (ret)
> +               return ERR_PTR(ret);
>
>         return iio_dev;
>  }
> @@ -1889,29 +1886,21 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  }
>  EXPORT_SYMBOL(iio_device_unregister);
>
> -static void devm_iio_device_unreg(struct device *dev, void *res)
> +static void devm_iio_device_unreg(void *indio_dev)
>  {
> -       iio_device_unregister(*(struct iio_dev **)res);
> +       iio_device_unregister(indio_dev);
>  }
>
>  int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>                                struct module *this_mod)
>  {
> -       struct iio_dev **ptr;
>         int ret;
>
> -       ptr = devres_alloc(devm_iio_device_unreg, sizeof(*ptr), GFP_KERNEL);
> -       if (!ptr)
> -               return -ENOMEM;
> -
> -       *ptr = indio_dev;
>         ret = __iio_device_register(indio_dev, this_mod);
> -       if (!ret)
> -               devres_add(dev, ptr);
> -       else
> -               devres_free(ptr);
> +       if (ret)
> +               return ret;
>
> -       return ret;
> +       return devm_add_action_or_reset(dev, devm_iio_device_unreg, indio_dev);
>  }
>  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>
> --
> 2.8.1
>
Yicong Yang April 9, 2021, 7:21 a.m. UTC | #2
On 2021/4/8 21:09, Alexandru Ardelean wrote:
> On Thu, Apr 8, 2021 at 2:41 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/industrialio-core.c | 43 +++++++++++++++--------------------------
>>  1 file changed, 16 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index 7db761a..2dfbed3 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -1627,9 +1627,9 @@ void iio_device_free(struct iio_dev *dev)
>>  }
>>  EXPORT_SYMBOL(iio_device_free);
>>
>> -static void devm_iio_device_release(struct device *dev, void *res)
>> +static void devm_iio_device_release(void *iio_dev)
>>  {
>> -       iio_device_free(*(struct iio_dev **)res);
>> +       iio_device_free(iio_dev);
>>  }
>>
>>  /**
>> @@ -1645,20 +1645,17 @@ static void devm_iio_device_release(struct device *dev, void *res)
>>   */
>>  struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)
>>  {
>> -       struct iio_dev **ptr, *iio_dev;
>> -
>> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
>> -                          GFP_KERNEL);
>> -       if (!ptr)
>> -               return NULL;
>> +       struct iio_dev *iio_dev;
>> +       int ret;
>>
>>         iio_dev = iio_device_alloc(parent, sizeof_priv);
>> -       if (iio_dev) {
>> -               *ptr = iio_dev;
>> -               devres_add(parent, ptr);
>> -       } else {
>> -               devres_free(ptr);
>> -       }
>> +       if (!iio_dev)
>> +               return iio_dev;
> 
> This is correct.
> But the preference is usually:
>         if (!iio_dev)
>             return NULL;
> 

since it returned iio_dev when failure before, so i just keep as is

>> +
>> +       ret = devm_add_action_or_reset(parent, devm_iio_device_release,
>> +                                      iio_dev);
>> +       if (ret)
>> +               return ERR_PTR(ret);
>>
>>         return iio_dev;
>>  }
>> @@ -1889,29 +1886,21 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>>  }
>>  EXPORT_SYMBOL(iio_device_unregister);
>>
>> -static void devm_iio_device_unreg(struct device *dev, void *res)
>> +static void devm_iio_device_unreg(void *indio_dev)
>>  {
>> -       iio_device_unregister(*(struct iio_dev **)res);
>> +       iio_device_unregister(indio_dev);
>>  }
>>
>>  int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>>                                struct module *this_mod)
>>  {
>> -       struct iio_dev **ptr;
>>         int ret;
>>
>> -       ptr = devres_alloc(devm_iio_device_unreg, sizeof(*ptr), GFP_KERNEL);
>> -       if (!ptr)
>> -               return -ENOMEM;
>> -
>> -       *ptr = indio_dev;
>>         ret = __iio_device_register(indio_dev, this_mod);
>> -       if (!ret)
>> -               devres_add(dev, ptr);
>> -       else
>> -               devres_free(ptr);
>> +       if (ret)
>> +               return ret;
>>
>> -       return ret;
>> +       return devm_add_action_or_reset(dev, devm_iio_device_unreg, indio_dev);
>>  }
>>  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>>
>> --
>> 2.8.1
>>
> 
> .
>
Andy Shevchenko April 9, 2021, 9:19 a.m. UTC | #3
On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:
> On 2021/4/8 21:09, Alexandru Ardelean wrote:
> > On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:

...

> >> -       if (!ptr)
> >> -               return NULL;

> >> +       if (!iio_dev)
> >> +               return iio_dev;
> >
> > This is correct.
> > But the preference is usually:
> >         if (!iio_dev)
> >             return NULL;
> >
>
> since it returned iio_dev when failure before, so i just keep as is

Actually Alexandru has a good point. Compare the pieces I left above.
Yicong Yang April 9, 2021, 9:41 a.m. UTC | #4
On 2021/4/9 17:19, Andy Shevchenko wrote:
> On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:
>> On 2021/4/8 21:09, Alexandru Ardelean wrote:
>>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
> 
> ...
> 
>>>> -       if (!ptr)
>>>> -               return NULL;
> 
>>>> +       if (!iio_dev)
>>>> +               return iio_dev;
>>>
>>> This is correct.
>>> But the preference is usually:
>>>         if (!iio_dev)
>>>             return NULL;
>>>
>>
>> since it returned iio_dev when failure before, so i just keep as is
> 
> Actually Alexandru has a good point. Compare the pieces I left above.
> 

a little different from my perspective when I did this.

previously:

-       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
-                          GFP_KERNEL);
-       if (!ptr)
-               return NULL;

if devres_alloc() failure we return NULL.


        iio_dev = iio_device_alloc(parent, sizeof_priv);
-       if (iio_dev) {
-               *ptr = iio_dev;
-               devres_add(parent, ptr);
-       } else {
-               devres_free(ptr);
-       }

        return iio_dev;

if iio_device_alloc() failed, we return what it returns.

now we remove devres_alloc() and stay iio_device_alloc(), so I just
keep to return iio_dev.

but actually iio_device_alloc() will return NULL at failure. :)
so return NULL here is definitely correct. i'll change to that if
it's recommended.

thanks,
Yicong
Andy Shevchenko April 9, 2021, 9:55 a.m. UTC | #5
On Fri, Apr 9, 2021 at 12:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
> On 2021/4/9 17:19, Andy Shevchenko wrote:
> > On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:
> >> On 2021/4/8 21:09, Alexandru Ardelean wrote:
> >>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:

...

> > Actually Alexandru has a good point. Compare the pieces I left above.
> >
>
> a little different from my perspective when I did this.
>
> previously:
>
> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
> -                          GFP_KERNEL);
> -       if (!ptr)
> -               return NULL;
>
> if devres_alloc() failure we return NULL.
>
>
>         iio_dev = iio_device_alloc(parent, sizeof_priv);
> -       if (iio_dev) {
> -               *ptr = iio_dev;
> -               devres_add(parent, ptr);
> -       } else {
> -               devres_free(ptr);
> -       }
>
>         return iio_dev;
>
> if iio_device_alloc() failed, we return what it returns.
>
> now we remove devres_alloc() and stay iio_device_alloc(), so I just
> keep to return iio_dev.
>
> but actually iio_device_alloc() will return NULL at failure. :)
> so return NULL here is definitely correct. i'll change to that if
> it's recommended.

Confusing here (in your case) is that:

if (!FOO)
 return BAR;

...

return BAR;

I.e. twice returning BAR for different occasions. This makes
additional cognitive work and decrease readability / maintainability
of the code.

I.o.w., yes, it's preferred.
Yicong Yang April 9, 2021, 1:05 p.m. UTC | #6
On 2021/4/9 17:55, Andy Shevchenko wrote:
> On Fri, Apr 9, 2021 at 12:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>> On 2021/4/9 17:19, Andy Shevchenko wrote:
>>> On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:
>>>> On 2021/4/8 21:09, Alexandru Ardelean wrote:
>>>>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
> 
> ...
> 
>>> Actually Alexandru has a good point. Compare the pieces I left above.
>>>
>>
>> a little different from my perspective when I did this.
>>
>> previously:
>>
>> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
>> -                          GFP_KERNEL);
>> -       if (!ptr)
>> -               return NULL;
>>
>> if devres_alloc() failure we return NULL.
>>
>>
>>         iio_dev = iio_device_alloc(parent, sizeof_priv);
>> -       if (iio_dev) {
>> -               *ptr = iio_dev;
>> -               devres_add(parent, ptr);
>> -       } else {
>> -               devres_free(ptr);
>> -       }
>>
>>         return iio_dev;
>>
>> if iio_device_alloc() failed, we return what it returns.
>>
>> now we remove devres_alloc() and stay iio_device_alloc(), so I just
>> keep to return iio_dev.
>>
>> but actually iio_device_alloc() will return NULL at failure. :)
>> so return NULL here is definitely correct. i'll change to that if
>> it's recommended.
> 
> Confusing here (in your case) is that:
> 
> if (!FOO)
>  return BAR;
> 
> ...
> 
> return BAR;
> 
> I.e. twice returning BAR for different occasions. This makes
> additional cognitive work and decrease readability / maintainability
> of the code.
> 
> I.o.w., yes, it's preferred.
> 

got it. will return NULL and will check if other patches in this series
met the same condition.

thanks.
Jonathan Cameron April 24, 2021, 2:04 p.m. UTC | #7
On Fri, 9 Apr 2021 21:05:49 +0800
Yicong Yang <yangyicong@hisilicon.com> wrote:

> On 2021/4/9 17:55, Andy Shevchenko wrote:
> > On Fri, Apr 9, 2021 at 12:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote:  
> >> On 2021/4/9 17:19, Andy Shevchenko wrote:  
> >>> On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote:  
> >>>> On 2021/4/8 21:09, Alexandru Ardelean wrote:  
> >>>>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote:  
> > 
> > ...
> >   
> >>> Actually Alexandru has a good point. Compare the pieces I left above.
> >>>  
> >>
> >> a little different from my perspective when I did this.
> >>
> >> previously:
> >>
> >> -       ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
> >> -                          GFP_KERNEL);
> >> -       if (!ptr)
> >> -               return NULL;
> >>
> >> if devres_alloc() failure we return NULL.
> >>
> >>
> >>         iio_dev = iio_device_alloc(parent, sizeof_priv);
> >> -       if (iio_dev) {
> >> -               *ptr = iio_dev;
> >> -               devres_add(parent, ptr);
> >> -       } else {
> >> -               devres_free(ptr);
> >> -       }
> >>
> >>         return iio_dev;
> >>
> >> if iio_device_alloc() failed, we return what it returns.
> >>
> >> now we remove devres_alloc() and stay iio_device_alloc(), so I just
> >> keep to return iio_dev.
> >>
> >> but actually iio_device_alloc() will return NULL at failure. :)
> >> so return NULL here is definitely correct. i'll change to that if
> >> it's recommended.  
> > 
> > Confusing here (in your case) is that:
> > 
> > if (!FOO)
> >  return BAR;
> > 
> > ...
> > 
> > return BAR;
> > 
> > I.e. twice returning BAR for different occasions. This makes
> > additional cognitive work and decrease readability / maintainability
> > of the code.
> > 
> > I.o.w., yes, it's preferred.
> >   
> 
> got it. will return NULL and will check if other patches in this series
> met the same condition.
> 
> thanks.
> 
Rather than go around a gain, I fixed this up as discussed above and applied
to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan
diff mbox series

Patch

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 7db761a..2dfbed3 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1627,9 +1627,9 @@  void iio_device_free(struct iio_dev *dev)
 }
 EXPORT_SYMBOL(iio_device_free);
 
-static void devm_iio_device_release(struct device *dev, void *res)
+static void devm_iio_device_release(void *iio_dev)
 {
-	iio_device_free(*(struct iio_dev **)res);
+	iio_device_free(iio_dev);
 }
 
 /**
@@ -1645,20 +1645,17 @@  static void devm_iio_device_release(struct device *dev, void *res)
  */
 struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)
 {
-	struct iio_dev **ptr, *iio_dev;
-
-	ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return NULL;
+	struct iio_dev *iio_dev;
+	int ret;
 
 	iio_dev = iio_device_alloc(parent, sizeof_priv);
-	if (iio_dev) {
-		*ptr = iio_dev;
-		devres_add(parent, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	if (!iio_dev)
+		return iio_dev;
+
+	ret = devm_add_action_or_reset(parent, devm_iio_device_release,
+				       iio_dev);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return iio_dev;
 }
@@ -1889,29 +1886,21 @@  void iio_device_unregister(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL(iio_device_unregister);
 
-static void devm_iio_device_unreg(struct device *dev, void *res)
+static void devm_iio_device_unreg(void *indio_dev)
 {
-	iio_device_unregister(*(struct iio_dev **)res);
+	iio_device_unregister(indio_dev);
 }
 
 int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
 			       struct module *this_mod)
 {
-	struct iio_dev **ptr;
 	int ret;
 
-	ptr = devres_alloc(devm_iio_device_unreg, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return -ENOMEM;
-
-	*ptr = indio_dev;
 	ret = __iio_device_register(indio_dev, this_mod);
-	if (!ret)
-		devres_add(dev, ptr);
-	else
-		devres_free(ptr);
+	if (ret)
+		return ret;
 
-	return ret;
+	return devm_add_action_or_reset(dev, devm_iio_device_unreg, indio_dev);
 }
 EXPORT_SYMBOL_GPL(__devm_iio_device_register);