[PATCH] iio: tsl2772: Use device-managed API

[PATCH] iio: tsl2772: Use device-managed API

[Chuhong Yuan]

Use devm_iio_device_register to simplify
the code.

Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
 drivers/iio/light/tsl2772.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
index 83cece921843..aa5891d105e8 100644
--- a/drivers/iio/light/tsl2772.c
+++ b/drivers/iio/light/tsl2772.c
@@ -1877,7 +1877,7 @@ static int tsl2772_probe(struct i2c_client *clientp,
 	if (ret < 0)
 		return ret;
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(&clientp->dev, indio_dev);
 	if (ret) {
 		tsl2772_chip_off(indio_dev);
 		dev_err(&clientp->dev,
@@ -1928,8 +1928,6 @@ static int tsl2772_remove(struct i2c_client *client)
 
 	tsl2772_chip_off(indio_dev);
 
-	iio_device_unregister(indio_dev);
-
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH] iio: tsl2772: Use device-managed API

[Jonathan Cameron]

On Fri, 26 Jul 2019 20:30:58 +0800
Chuhong Yuan <hslester96@gmail.com> wrote:

> Use devm_iio_device_register to simplify
> the code.
> 
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>

Please try to pick up on likely reviewers in your cc list.  In this case
Brian did a lot of work cleaning these drivers up so I've added him.
Not everyone keeps up with the linux-iio list for some reason ;)

Immediate thought was that you had broken the ordering but turns out
it is already buggy.

As things stand, we remove the userspace interfaces (iio_device_unregister)
after turning off the power.   This is obviously a bad idea and also
form a purely "obviously correct" stand point, we aren't doing the reverse
of probe.

So, I 'think' the right option is to reorder remove so that the power left
on until after the iio_device_unregister call. At that point, we can't
use devm_iio_device_register as we'll have the same incorrect ordering
we currently have.

Brian, you looked at this driver most recently.  Thoughts?

Thanks,

Jonathan



> ---
>  drivers/iio/light/tsl2772.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
> index 83cece921843..aa5891d105e8 100644
> --- a/drivers/iio/light/tsl2772.c
> +++ b/drivers/iio/light/tsl2772.c
> @@ -1877,7 +1877,7 @@ static int tsl2772_probe(struct i2c_client *clientp,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_iio_device_register(&clientp->dev, indio_dev);
>  	if (ret) {
>  		tsl2772_chip_off(indio_dev);
>  		dev_err(&clientp->dev,
> @@ -1928,8 +1928,6 @@ static int tsl2772_remove(struct i2c_client *client)
>  
>  	tsl2772_chip_off(indio_dev);
>  
> -	iio_device_unregister(indio_dev);
> -
>  	return 0;
>  }
>  

Re: [PATCH] iio: tsl2772: Use device-managed API

[Brian Masney]

On Sat, Jul 27, 2019 at 12:57:49PM +0100, Jonathan Cameron wrote:
> On Fri, 26 Jul 2019 20:30:58 +0800
> Chuhong Yuan <hslester96@gmail.com> wrote:
> 
> > Use devm_iio_device_register to simplify
> > the code.
> > 
> > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> 
> Please try to pick up on likely reviewers in your cc list.  In this case
> Brian did a lot of work cleaning these drivers up so I've added him.
> Not everyone keeps up with the linux-iio list for some reason ;)
> 
> Immediate thought was that you had broken the ordering but turns out
> it is already buggy.
> 
> As things stand, we remove the userspace interfaces (iio_device_unregister)
> after turning off the power.   This is obviously a bad idea and also
> form a purely "obviously correct" stand point, we aren't doing the reverse
> of probe.
> 
> So, I 'think' the right option is to reorder remove so that the power left
> on until after the iio_device_unregister call. At that point, we can't
> use devm_iio_device_register as we'll have the same incorrect ordering
> we currently have.
> 
> Brian, you looked at this driver most recently.  Thoughts?

devm_add_action() could be used in the probe function to schedule the call
to tsl2772_chip_off(). That would eliminate the need for
tsl2772_remove(). See tsl2772_disable_regulators_action() for an example in
that driver.

Chuhong: Another potential cleanup to shrink the size of this driver is
to move it over to the regulator_bulk_() API. I didn't realize that API
existed at the time I introduced the regulator support. If you're
interested in taking on that cleanup as well, I can test those changes
for you if you don't have the hardware.

Brian


> > ---
> >  drivers/iio/light/tsl2772.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
> > index 83cece921843..aa5891d105e8 100644
> > --- a/drivers/iio/light/tsl2772.c
> > +++ b/drivers/iio/light/tsl2772.c
> > @@ -1877,7 +1877,7 @@ static int tsl2772_probe(struct i2c_client *clientp,
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	ret = iio_device_register(indio_dev);
> > +	ret = devm_iio_device_register(&clientp->dev, indio_dev);
> >  	if (ret) {
> >  		tsl2772_chip_off(indio_dev);
> >  		dev_err(&clientp->dev,
> > @@ -1928,8 +1928,6 @@ static int tsl2772_remove(struct i2c_client *client)
> >  
> >  	tsl2772_chip_off(indio_dev);
> >  
> > -	iio_device_unregister(indio_dev);
> > -
> >  	return 0;
> >  }
> >  

Re: [PATCH] iio: tsl2772: Use device-managed API

[Chuhong Yuan]

Brian Masney <masneyb@onstation.org> 于2019年7月28日周日 下午4:31写道：
>
> On Sat, Jul 27, 2019 at 12:57:49PM +0100, Jonathan Cameron wrote:
> > On Fri, 26 Jul 2019 20:30:58 +0800
> > Chuhong Yuan <hslester96@gmail.com> wrote:
> >
> > > Use devm_iio_device_register to simplify
> > > the code.
> > >
> > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> >
> > Please try to pick up on likely reviewers in your cc list.  In this case
> > Brian did a lot of work cleaning these drivers up so I've added him.
> > Not everyone keeps up with the linux-iio list for some reason ;)
> >
> > Immediate thought was that you had broken the ordering but turns out
> > it is already buggy.
> >
> > As things stand, we remove the userspace interfaces (iio_device_unregister)
> > after turning off the power.   This is obviously a bad idea and also
> > form a purely "obviously correct" stand point, we aren't doing the reverse
> > of probe.
> >
> > So, I 'think' the right option is to reorder remove so that the power left
> > on until after the iio_device_unregister call. At that point, we can't
> > use devm_iio_device_register as we'll have the same incorrect ordering
> > we currently have.
> >
> > Brian, you looked at this driver most recently.  Thoughts?
>
> devm_add_action() could be used in the probe function to schedule the call
> to tsl2772_chip_off(). That would eliminate the need for
> tsl2772_remove(). See tsl2772_disable_regulators_action() for an example in
> that driver.
>

I find that we can use devm_add_action_or_reset() for
tsl2772_disable_regulators_action() to eliminate the fault handling code.

I am not sure whether devm_add_action() can be used for
tsl2772_chip_off() because it returns an integer, not void.
And the return value is used in several functions.

> Chuhong: Another potential cleanup to shrink the size of this driver is
> to move it over to the regulator_bulk_() API. I didn't realize that API
> existed at the time I introduced the regulator support. If you're
> interested in taking on that cleanup as well, I can test those changes
> for you if you don't have the hardware.
>
> Brian
>

Does that mean merging vdd_supply and vddio_supply to an array of
regulator_bulk_data and utilize regulator_bulk_() API to operate them
together?
If so, I can do that cleanup in next version.

I have an additional question that I find regulator_disable() is used in the
end of many .remove functions of drivers, which hinders us to use devm
functions.
One example is drivers/iio/light/gp2ap020a00f.c.
Is there any solution to this problem?

Regards,
Chuhong

>
> > > ---
> > >  drivers/iio/light/tsl2772.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
> > > index 83cece921843..aa5891d105e8 100644
> > > --- a/drivers/iio/light/tsl2772.c
> > > +++ b/drivers/iio/light/tsl2772.c
> > > @@ -1877,7 +1877,7 @@ static int tsl2772_probe(struct i2c_client *clientp,
> > >     if (ret < 0)
> > >             return ret;
> > >
> > > -   ret = iio_device_register(indio_dev);
> > > +   ret = devm_iio_device_register(&clientp->dev, indio_dev);
> > >     if (ret) {
> > >             tsl2772_chip_off(indio_dev);
> > >             dev_err(&clientp->dev,
> > > @@ -1928,8 +1928,6 @@ static int tsl2772_remove(struct i2c_client *client)
> > >
> > >     tsl2772_chip_off(indio_dev);
> > >
> > > -   iio_device_unregister(indio_dev);
> > > -
> > >     return 0;
> > >  }
> > >

Re: [PATCH] iio: tsl2772: Use device-managed API

[Brian Masney]

On Mon, Jul 29, 2019 at 11:03:00AM +0800, Chuhong Yuan wrote:
> Brian Masney <masneyb@onstation.org> 于2019年7月28日周日 下午4:31写道：
> > devm_add_action() could be used in the probe function to schedule the call
> > to tsl2772_chip_off(). That would eliminate the need for
> > tsl2772_remove(). See tsl2772_disable_regulators_action() for an example in
> > that driver.
> >
> 
> I find that we can use devm_add_action_or_reset() for
> tsl2772_disable_regulators_action() to eliminate the fault handling code.
> 
> I am not sure whether devm_add_action() can be used for
> tsl2772_chip_off() because it returns an integer, not void.
> And the return value is used in several functions.

I would add a wrapper function (tsl2772_chip_off_action?) with the
expected declaration that calls tsl2772_chip_off().

> > Chuhong: Another potential cleanup to shrink the size of this driver is
> > to move it over to the regulator_bulk_() API. I didn't realize that API
> > existed at the time I introduced the regulator support. If you're
> > interested in taking on that cleanup as well, I can test those changes
> > for you if you don't have the hardware.
> >
> > Brian
> >
> 
> Does that mean merging vdd_supply and vddio_supply to an array of
> regulator_bulk_data and utilize regulator_bulk_() API to operate them
> together?

Yes.

> I have an additional question that I find regulator_disable() is used in the
> end of many .remove functions of drivers, which hinders us to use devm
> functions.
> One example is drivers/iio/light/gp2ap020a00f.c.
> Is there any solution to this problem?

There are devm_regulator_*() variants of the regulator API available
that you can use. Lots of other APIs in the kernel have devm variants
to simply drivers.

Brian

Re: [PATCH] iio: tsl2772: Use device-managed API

[Jonathan Cameron]

On Mon, 29 Jul 2019 04:03:07 -0400
Brian Masney <masneyb@onstation.org> wrote:

> On Mon, Jul 29, 2019 at 11:03:00AM +0800, Chuhong Yuan wrote:
> > Brian Masney <masneyb@onstation.org> 于2019年7月28日周日 下午4:31写道：  
> > > devm_add_action() could be used in the probe function to schedule the call
> > > to tsl2772_chip_off(). That would eliminate the need for
> > > tsl2772_remove(). See tsl2772_disable_regulators_action() for an example in
> > > that driver.
> > >  
> > 
> > I find that we can use devm_add_action_or_reset() for
> > tsl2772_disable_regulators_action() to eliminate the fault handling code.
> > 
> > I am not sure whether devm_add_action() can be used for
> > tsl2772_chip_off() because it returns an integer, not void.
> > And the return value is used in several functions.  
> 
> I would add a wrapper function (tsl2772_chip_off_action?) with the
> expected declaration that calls tsl2772_chip_off().
> 
> > > Chuhong: Another potential cleanup to shrink the size of this driver is
> > > to move it over to the regulator_bulk_() API. I didn't realize that API
> > > existed at the time I introduced the regulator support. If you're
> > > interested in taking on that cleanup as well, I can test those changes
> > > for you if you don't have the hardware.
> > >
> > > Brian
> > >  
> > 
> > Does that mean merging vdd_supply and vddio_supply to an array of
> > regulator_bulk_data and utilize regulator_bulk_() API to operate them
> > together?  
> 
> Yes.
> 
> > I have an additional question that I find regulator_disable() is used in the
> > end of many .remove functions of drivers, which hinders us to use devm
> > functions.
> > One example is drivers/iio/light/gp2ap020a00f.c.
> > Is there any solution to this problem?  
> 
> There are devm_regulator_*() variants of the regulator API available
> that you can use. Lots of other APIs in the kernel have devm variants
> to simply drivers.
I don't think there are any devm_ versions of regulator disable.

IIRC the argument made when this last came up was that it was rarely correct
to be as course grained as a lot of IIO drivers are.   We should probably
do runtime pm and turn these regulators off a lot more frequently.

The reality is that it is an optimization that doesn't get done in 
IIO drivers that often as we mostly just want them to work and many
usecases aren't actually power constrained,

So we end up doing a lot of devm_add_action_or_reset to power down the
regulators.

Jonathan
> 
> Brian

Re: [PATCH] iio: tsl2772: Use device-managed API

[Chuhong Yuan]

Jonathan Cameron <jonathan.cameron@huawei.com> 于2019年7月29日周一 下午7:09写道：
>
> On Mon, 29 Jul 2019 04:03:07 -0400
> Brian Masney <masneyb@onstation.org> wrote:
>
> > On Mon, Jul 29, 2019 at 11:03:00AM +0800, Chuhong Yuan wrote:
> > > Brian Masney <masneyb@onstation.org> 于2019年7月28日周日 下午4:31写道：
> > > > devm_add_action() could be used in the probe function to schedule the call
> > > > to tsl2772_chip_off(). That would eliminate the need for
> > > > tsl2772_remove(). See tsl2772_disable_regulators_action() for an example in
> > > > that driver.
> > > >
> > >
> > > I find that we can use devm_add_action_or_reset() for
> > > tsl2772_disable_regulators_action() to eliminate the fault handling code.
> > >
> > > I am not sure whether devm_add_action() can be used for
> > > tsl2772_chip_off() because it returns an integer, not void.
> > > And the return value is used in several functions.
> >
> > I would add a wrapper function (tsl2772_chip_off_action?) with the
> > expected declaration that calls tsl2772_chip_off().
> >
> > > > Chuhong: Another potential cleanup to shrink the size of this driver is
> > > > to move it over to the regulator_bulk_() API. I didn't realize that API
> > > > existed at the time I introduced the regulator support. If you're
> > > > interested in taking on that cleanup as well, I can test those changes
> > > > for you if you don't have the hardware.
> > > >
> > > > Brian
> > > >
> > >
> > > Does that mean merging vdd_supply and vddio_supply to an array of
> > > regulator_bulk_data and utilize regulator_bulk_() API to operate them
> > > together?
> >
> > Yes.
> >
> > > I have an additional question that I find regulator_disable() is used in the
> > > end of many .remove functions of drivers, which hinders us to use devm
> > > functions.
> > > One example is drivers/iio/light/gp2ap020a00f.c.
> > > Is there any solution to this problem?
> >
> > There are devm_regulator_*() variants of the regulator API available
> > that you can use. Lots of other APIs in the kernel have devm variants
> > to simply drivers.
> I don't think there are any devm_ versions of regulator disable.
>
> IIRC the argument made when this last came up was that it was rarely correct
> to be as course grained as a lot of IIO drivers are.   We should probably
> do runtime pm and turn these regulators off a lot more frequently.
>
> The reality is that it is an optimization that doesn't get done in
> IIO drivers that often as we mostly just want them to work and many
> usecases aren't actually power constrained,
>
> So we end up doing a lot of devm_add_action_or_reset to power down the
> regulators.
>

I think I can add devm_ versions of regulator_enable and disable.

Chuhong

> Jonathan
> >
> > Brian
>
>

Re: [PATCH] iio: tsl2772: Use device-managed API

[Brian Masney]

On Mon, Jul 29, 2019 at 12:08:02PM +0100, Jonathan Cameron wrote:
> On Mon, 29 Jul 2019 04:03:07 -0400
> Brian Masney <masneyb@onstation.org> wrote:
> > There are devm_regulator_*() variants of the regulator API available
> > that you can use. Lots of other APIs in the kernel have devm variants
> > to simply drivers.
> I don't think there are any devm_ versions of regulator disable.
> 
> IIRC the argument made when this last came up was that it was rarely correct
> to be as course grained as a lot of IIO drivers are.   We should probably
> do runtime pm and turn these regulators off a lot more frequently.
> 
> The reality is that it is an optimization that doesn't get done in 
> IIO drivers that often as we mostly just want them to work and many
> usecases aren't actually power constrained,
> 
> So we end up doing a lot of devm_add_action_or_reset to power down the
> regulators.

That makes sense. I have an out-of-tree patch where I started to add
runtime pm support to the tsl2772 driver around the time I was working
on the staging cleanup. I was unsure of how to do this when the user
configures an interrupt threshold via sysfs since we don't want to
completely power off the chip in that case. At the time, I couldn't
find any other examples in IIO that showed how to do that. I should
dust off that patch and send it out as a RFC to get some feedback.

Brian