In reply to one of Andy's questions. > On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@xxxxxxxxxx> > wrote: > > On Mon, 22 Aug 2022 15:51:05 +0300 > > Ciprian Regus <ciprian.regus@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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()? The BIT(ddata->chip_info->resolution + 1) subtraction part is already doing the sign extension, since that bit (which is the most significant one) will be 0 if the result is < 0, and 1 otherwise. Regards, Ciprian > > 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
