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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190729120802.000025e8@huawei.com>
Date:   Mon, 29 Jul 2019 12:08:02 +0100
From:   Jonathan Cameron <jonathan.cameron@...wei.com>
To:     Brian Masney <masneyb@...tation.org>
CC:     Chuhong Yuan <hslester96@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iio: tsl2772: Use device-managed API

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

> On Mon, Jul 29, 2019 at 11:03:00AM +0800, Chuhong Yuan wrote:
> > Brian Masney <masneyb@...tation.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