Date:   Thu, 2 Mar 2023 16:23:14 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Mike Looijmans <mike.looijmans@topic.nl>
Cc:     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>,
        Lars-Peter Clausen <lars@metafoo.de>,
        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: <ZACxUpzCtlrMehrA@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>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a2ba706f-888b-0a72-03a5-cbf761dfaf19@topic.nl>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Thu, Mar 02, 2023 at 08:49:22AM +0100, Mike Looijmans wrote:
> On 01-03-2023 16:30, Andy Shevchenko wrote:
> > On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote:

...

> > > +	/* Shift result to compensate for bit resolution vs. sample rate */
> > > +	value <<= 16 - ads1100_data_bits(data);
> > > +	*val = sign_extend32(value, 15);
> > Why not simply
> > 
> > 	*val = sign_extend32(value, ads1100_data_bits(data) - 1);
> > 
> > ?
> 
> As discussed with? Jonathan Cameron, the register is right-justified and the
> number of bits depend on the data rate. Rather than having the "scale"
> change when the sample rate changes, we chose to adjust the sample result so
> it's always left-justified.

Hmm... OK, but it adds unneeded code I think.

...

> > > +	for (i = 0; i < 4; i++) {
> > > +		if (BIT(i) == gain) {
> > ffs()/__ffs() (look at the documentation for the difference and use proper one).
> 
> Thought of it, but I'd rather have it return EINVAL for attempting to set
> the analog gain to "7" (0nly 1,2,4,8 allowed).

I'm not sure what you are implying.

You have open coded something that has already to be a function which on some
architectures become a single assembly instruction.

That said, drop your for-loop if-cond and use one of the proposed directly.
Then you may compare the result to what ever you want to be a limit and return
whatever error code you want to.

...

> > > +	for (i = 0; i < size; ++i) {
> > Why pre-increment?
> 
> Spent too much time with other coding guidelines, missed this one... Will
> change.

I don't remember that's in coding guidelines, but it's standard practice in the
Linux kernel project. Yeah, we have a few hundreds of the pre-increments, but
reasons may be quite different for those.

...

> > > +	int millivolts = regulator_get_voltage(data->reg_vdd) / 1000;
> > units.h?
> 
> Should I write:
> 
> regulator_get_voltage(data->reg_vdd) / (MICROS / MILLIS);
> 
> I doubt that improves readability.

Yeah, it should be something like MICROVOLT_PER_MILLIVOLT.
But it's not defined yet.

...

> > > +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.

Ah, it's my mistake, I misread it as ->remove().

-- 
With Best Regards,
Andy Shevchenko