Re: [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502

[Jagath Jog J <jagathjog1996@gmail.com>]

Hello Andy,

On Mon, Feb 14, 2022 at 01:32:14PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 14, 2022 at 5:36 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >
> > The DS3502 is a 7-bit, nonvolatile digital potentiometer featuring
> > an output voltage range of up to 15.5V.
> > DS3502 support is implemented into existing ds1803 driver
> 
> Be consistent here and in other commit messages with how you refer to
> the IC parts, i.e.
> DS1803. Don't forget English grammar and punctuation, i.e. missed period above.
> 

I will fix this in v3

> > Datasheet: https://datasheets.maximintegrated.com/en/ds/DS3502.pdf
> 
> >
> 
> A tag block may not have blank lines. Drop it.
> 
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> 
> ...
> 
> > -       tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
> > +       tristate "Maxim Integrated DS1803 and similar Digital Potentiometer driver"
> 
> Please, list them like other drivers do:
> 
>        tristate "Maxim Integrated DS1803/DS... Digital Potentiometer driver"
> 
> ...
> 
> > -         Say yes here to build support for the Maxim Integrated DS1803
> > -         digital potentiometer chip.
> > +         Say yes here to build support for the Maxim Integrated DS1803 and
> > +         similar digital potentiometer chip.
> 
> Same here.
> 
> ...
> 
> > - * Maxim Integrated DS1803 digital potentiometer driver
> > + * Maxim Integrated DS1803 and similar digital potentiometer driver
> 
> Same here.

Based on Jonathan suggestion for the previous patch version I used 
"and similar" wording here.

> 
> ...
> 
> > -#define DS1803_MAX_POS         255
> > -#define DS1803_WRITE(chan)     (0xa8 | ((chan) + 1))
> 
> Not sure why these were removed (or moved?)

Since max wiper position is present in avail array of ds1803_cfg structure
and that is being used for read scale so DS1803_MAX_POS is removed.

Since each wiper address of both parts is assigned to the address
member of iio_chan_spec struct so DS1803_WRITE(chan) is removed.

> 
> ...
> 
> > +static const struct ds1803_cfg ds1803_cfg[] = {
> > +       [DS1803_010] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  10,
> > +                        .channels = ds1803_channels,
> > +                        .num_channels = ARRAY_SIZE(ds1803_channels) },
> > +       [DS1803_050] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  50,
> > +                        .channels = ds1803_channels,
> > +                        .num_channels = ARRAY_SIZE(ds1803_channels) },
> > +       [DS1803_100] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms = 100,
> > +                        .channels = ds1803_channels,
> > +                        .num_channels = ARRAY_SIZE(ds1803_channels) },
> > +       [DS3502] =     { .wipers = 1, .avail = { 0, 1, 127 }, .kohms =  10,
> > +                        .channels = ds3502_channels,
> > +                        .num_channels = ARRAY_SIZE(ds3502_channels) },
> >  };
> 
> Split this on a per type basis. I believe it won't be too much work,
> also, consider adding channels as a separate preparatory patch as you
> did with avail.

Based on Jonathan suggestion for the previous patch version to avoid
having different chip type related structures so channels and num_channels
are added into ds1803_cfg structure.

Sure for channels I will split into separate patch for old part in v3.

>
> ...
> 
> > -       data->cfg = &ds1803_cfg[id->driver_data];
> > +       data->chip_type = (uintptr_t)device_get_match_data(dev);
> > +       if (data->chip_type < DS1803_010 || data->chip_type > DS3502)
> > +               data->chip_type = id->driver_data;
> 
> Split it into a separate patch and use pointer validation instead:
> 
> data->cfg = ...
> if (!data->cfg)
>   data->cfg = ...id->driver_data;
> 
> ...
> 
> > -       { .compatible = "maxim,ds1803-010", .data = &ds1803_cfg[DS1803_010] },
> > -       { .compatible = "maxim,ds1803-050", .data = &ds1803_cfg[DS1803_050] },
> > -       { .compatible = "maxim,ds1803-100", .data = &ds1803_cfg[DS1803_100] },

To get the chip specific structure I can use previous structure method for data
and validation as you shown above.
But it is necessary to get the chip_type also because of dependency in 
ds1803_raw_read().

To get the chip_type can I use 
data->chip_type = id->driver_data

> > +       { .compatible = "maxim,ds1803-010", .data = (void *)DS1803_010 },
> > +       { .compatible = "maxim,ds1803-050", .data = (void *)DS1803_050 },
> > +       { .compatible = "maxim,ds1803-100", .data = (void *)DS1803_100 },
> 
> This is not good, please use pointers as it was before.
> 
> > +       { .compatible = "maxim,ds3502",     .data = (void *)DS3502 },
> 
> Ditto. Create a new, separate structure for this type.
> 
> ...
> 
> >         { "ds1803-010", DS1803_010 },
> >         { "ds1803-050", DS1803_050 },
> >         { "ds1803-100", DS1803_100 },
> > +       { "ds3502",     DS3502     },
> 
> Too many spaces.
> Besides this, please create a new prerequisite patch to convert this
> to use pointers as above.

Sure I will split this patch in v3.
Thanks for feedback.

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Best Regards,
Jagath