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