Date:   Thu, 2 Mar 2023 16:24:06 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Lars-Peter Clausen <lars@metafoo.de>
Cc:     Mike Looijmans <mike.looijmans@topic.nl>,
        devicetree@vger.kernel.org, linux-iio@vger.kernel.org,
        Caleb Connolly <caleb.connolly@linaro.org>,
        ChiYuan Huang <cy_huang@richtek.com>,
        ChiaEn Wu <chiaen_wu@richtek.com>,
        Cosmin Tanislav <demonsingur@gmail.com>,
        Ibrahim Tilki <Ibrahim.Tilki@analog.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Ramona Bolboaca <ramona.bolboaca@analog.com>,
        William Breathitt Gray <william.gray@linaro.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Message-ID: <ZACxhiwgR7/MYUAQ@smile.fi.intel.com>
References: <20230228063151.17598-1-mike.looijmans@topic.nl>
 <20230228063151.17598-2-mike.looijmans@topic.nl>
 <Y/9vez/fzLD5dRVF@smile.fi.intel.com>
 <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.0685d97e-4a28-499e-a9e3-3bafec126832@emailsignatures365.codetwo.com>
 <a2ba706f-888b-0a72-03a5-cbf761dfaf19@topic.nl>
 <87bc192e-45ae-9480-5e84-8fe0adfc12e7@metafoo.de>
 <b635b91e-1dcc-71ba-95d1-1f87a20dbaf2@metafoo.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <b635b91e-1dcc-71ba-95d1-1f87a20dbaf2@metafoo.de>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Thu, Mar 02, 2023 at 05:20:38AM -0800, Lars-Peter Clausen wrote:
> On 3/2/23 05:16, Lars-Peter Clausen wrote:
> > On 3/1/23 23:49, Mike Looijmans wrote:

...

> > > > > +static int ads1100_runtime_suspend(struct device *dev)
> > > > > +{
> > > > > +??? struct iio_dev *indio_dev =
> > > > > i2c_get_clientdata(to_i2c_client(dev));
> > > > > +??? struct ads1100_data *data = iio_priv(indio_dev);
> > > > > +
> > > > > +??? ads1100_set_config_bits(data, ADS1100_CFG_SC,
> > > > > ADS1100_SINGLESHOT);
> > > > > +??? regulator_disable(data->reg_vdd);
> > > > Wrong devm / non-devm ordering.
> > > 
> > > Don't understand your remark, can you explain further please?
> > > 
> > > devm / non-devm ordering would be related to the "probe" function.
> > > As far as I can tell, I'm not allocating resources after the devm
> > > calls. And the "remove" is empty.
> > 
> > Strictly speaking we need to unregister the IIO device before disabling
> > the regulator, otherwise there is a small window where the IIO device
> > still exists, but doesn't work anymore. This is a very theoretical
> > scenario though.
> > 
> > You are lucky :) There is a new function
> > `devm_regulator_get_enable()`[1], which will manage the
> > regulator_disable() for you. Using that will also reduce the boilerplate
> > in `probe()` a bit
> > 
> > [1] https://lwn.net/Articles/904383/
> > 
> Sorry, just saw that Andy's comment was on the suspend() function, not
> remove(). In that case there is of course no need for any devm things. But
> still a good idea to use `devm_regulator_get_enable()` in probe for the
> boiler plate.

Yeah, sorry, I mistakenly took it as ->remove().

-- 
With Best Regards,
Andy Shevchenko