Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 23 Aug 2022 18:21:56 +0300 | Subject | Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support |
| |
On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 22 Aug 2022 15:51:05 +0300 > Ciprian Regus <ciprian.regus@analog.com> wrote:
In reply to Jonathan's comments to answer his question and add more comments from me.
...
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf > > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
Tag block mustn't have the blank line(s).
...
> > #include <linux/iio/iio.h> > > #include <linux/iio/driver.h> > > #include <linux/module.h> > > +#include <linux/property.h> > why?
device_get_match_data() requires it.
But why not sort them?
> > #include <linux/mod_devicetable.h>
...
> > - *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17); > > Old code here is less than ideal, should be reading into 3 bytes then using > the be24 accesors. Please fix whilst here. You will need multiple paths here > depending on size. > > > + *val = (be32_to_cpu(st->buf) >> st->sub_lsb) - > > + BIT(ddata->chip_info->resolution + 1);
Shouldn't this use some kind of sign_extend()?
Also with a temporary variable for chip info this line can be single.
struct ... *ci = ddata->chip_info;
...BIT(ci->resolution + 1)
...
> > + u32 resolution;
Keep this in a way that the "longer lines go first".
...
> > + resolution = st->common_ddata.chip_info->resolution; > > + st->sub_lsb = 31 - (resolution + 1); > > + st->recv_size = resolution / BITS_PER_BYTE + 1;
BITS_TO_BYTES()
...
> > static const struct i2c_device_id ltc2497_id[] = { > > - { "ltc2497", 0 }, > > + { "ltc2497", TYPE_LTC2497 }, > > + { "ltc2499", TYPE_LTC2499 },
Use pointers here like you have done for the OF table.
> > { } > > };
...
> > +enum chip_type { > > + TYPE_LTC2496, > > + TYPE_LTC2497, > > + TYPE_LTC2499
Keep trailing comma.
> > +};
-- With Best Regards, Andy Shevchenko
| |