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
