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

[Andy Shevchenko]

On Mon, Feb 14, 2022 at 5:36 AM Jagath Jog J <jagathjog1996@xxxxxxxxx> 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.

> 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@xxxxxxxxx>

...

> -       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.

...

> -#define DS1803_MAX_POS         255
> -#define DS1803_WRITE(chan)     (0xa8 | ((chan) + 1))

Not sure why these were removed (or moved?)

...

> +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.

...

> -       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] },
> +       { .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.

-- 
With Best Regards,
Andy Shevchenko