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
