Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Andy Shevchenko]

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