On Tue, 28 Jul 2020 06:24:03 +0000
"Berghe, Darius" <Darius.Berghe@xxxxxxxxxx> wrote:
> Hi Jonathan,
>
> Thanks for review, comments inline.
>
> > -----Original Message-----
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Sent: Monday, July 27, 2020 8:27 PM
> > To: Berghe, Darius <Darius.Berghe@xxxxxxxxxx>
> > Cc: linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; jic23@xxxxxxxxxx; robh@xxxxxxxxxx
> > Subject: Re: [PATCH v3 2/3] ltc2471: ltc2461/ltc2463 compatible strings
> >
> > [External]
> >
> > On Mon, 27 Jul 2020 16:58:33 +0300
> > Darius Berghe <darius.berghe@xxxxxxxxxx> wrote:
> >
> > > Add compatible strings for these devices in the existing ltc2471
> > > driver.
> > >
> > > Signed-off-by: Darius Berghe <darius.berghe@xxxxxxxxxx>
> >
> > Hi Darius,
> >
> > A few additional minor comments from me.
> >
> > > ---
> > > drivers/iio/adc/ltc2471.c | 16 ++++++++++++----
> > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ltc2471.c b/drivers/iio/adc/ltc2471.c
> > > index e1c4e966524d..8c57203b1fe4 100644
> > > --- a/drivers/iio/adc/ltc2471.c
> > > +++ b/drivers/iio/adc/ltc2471.c
> > > @@ -1,5 +1,7 @@
> > > /*
> > > - * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
> > > + * Driver for Linear Technology LTC2461, LTC2463, LTC2471 and LTC2473
> > > + voltage
> > > + * monitors.
> > > + * The LTC2463 is identical to the 2461, but reports a differential signal.
> > > * The LTC2473 is identical to the 2471, but reports a differential signal.
> > > *
> > > * Copyright (C) 2017 Topic Embedded Products @@ -17,8 +19,10 @@
> > > #include <linux/mod_devicetable.h>
> > >
> > > enum ltc2471_chips {
> > > + ltc2461,
> > > + ltc2463,
> > > ltc2471,
> > > - ltc2473,
> > > + ltc2473
> >
> > Why drop the comma? We've just added two new devices. Seems possible
> > there may be more in the future!
> >
>
> Ok, will leave it there in v4.
>
> > > };
> > >
> > > struct ltc2471_data {
> > > @@ -122,7 +126,7 @@ static int ltc2471_i2c_probe(struct i2c_client
> > *client,
> > > indio_dev->name = id->name;
> > > indio_dev->info = <c2471_info;
> > > indio_dev->modes = INDIO_DIRECT_MODE;
> > > - if (id->driver_data == ltc2473)
> > > + if (id->driver_data == ltc2473 || id->driver_data == ltc2463)
> > If the only use of driver_data is going to be this check, then just set it to 2473
> > for the 2463 and 2473. It's not uncommon to do this when we have a bunch
> > of devices that look the same to software.
>
> Yes the chips are similar but there is at least one feature which requires this level
> of distinguishing them: the sampling rate (60sps for ltc2461/3 and selectable 208/833sps
> for ltc2471/3). It's not used anywhere for now but I can see it being implemented
> as standard IIO dev attribute sampling frequency.
Fair enough. Perhaps a note in the patch description so no one asks the question
on the next version?
>
> >
> > > indio_dev->channels = ltc2473_channel;
> > > else
> > > indio_dev->channels = ltc2471_channel; @@ -139,6 +143,8
> > @@ static
> > > int ltc2471_i2c_probe(struct i2c_client *client, }
> > >
> > > static const struct i2c_device_id ltc2471_i2c_id[] = {
> > > + { "ltc2461", ltc2461 },
> > > + { "ltc2463", ltc2463 },
> > > { "ltc2471", ltc2471 },
> > > { "ltc2473", ltc2473 },
> > > {}
> > > @@ -146,6 +152,8 @@ static const struct i2c_device_id ltc2471_i2c_id[]
> > > = { MODULE_DEVICE_TABLE(i2c, ltc2471_i2c_id);
> > >
> > > static const struct of_device_id ltc2471_of_match[] = {
> > > + { .compatible = "adi,ltc2461" },
> > > + { .compatible = "adi,ltc2463" },
> > > { .compatible = "adi,ltc2471" },
> > > { .compatible = "adi,ltc2473" },
> > > {}
> > > @@ -163,6 +171,6 @@ static struct i2c_driver ltc2471_i2c_driver = {
> > >
> > > module_i2c_driver(ltc2471_i2c_driver);
> > >
> > > -MODULE_DESCRIPTION("LTC2471/LTC2473 ADC driver");
> > > +MODULE_DESCRIPTION("LTC2461/LTC2463/LTC2471/LTC2473 ADC
> > driver");
> > > MODULE_AUTHOR("Topic Embedded Products"); MODULE_LICENSE("GPL
> > v2");
>
