Brian Masney <masneyb@xxxxxxxxxxxxx> 于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@xxxxxxxxx> wrote:
> >
> > > Use devm_iio_device_register to simplify
> > > the code.
> > >
> > > Signed-off-by: Chuhong Yuan <hslester96@xxxxxxxxx>
> >
> > 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;
> > > }
> > >
