Date:   Mon, 29 Jul 2019 11:03:00 +0800
From:   Chuhong Yuan <hslester96@...il.com>
To:     Brian Masney <masneyb@...tation.org>
Cc:     Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: tsl2772: Use device-managed API

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

Powered by blists - more mailing lists