* [PATCH v2 0/4] iio: potentiometer: Add support for DS3502
@ 2022-02-14 3:36 Jagath Jog J
2022-02-14 3:36 ` [PATCH v2 1/4] iio: potentiometer: Alignment to match the open parenthesis Jagath Jog J
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Jagath Jog J @ 2022-02-14 3:36 UTC (permalink / raw)
To: jic23, lars, andy.shevchenko, sst, robh+dt
Cc: linux-iio, devicetree, linux-kernel
Add dt-bindings and support for Maxim DS3502 into existing ds1803 driver.
DS3502 is a 7 bit Nonvolatile Digital Potentiometer.
Changes since v1:
1. Fixes the alignment to match the open parenthesis in separate patch
2. Adding available functionality for ds1803 driver in separate patch
3. Moving maxim_potentiometer members into ds1803_cfg structure
4. Droping of the INFO_ENABLE channel type
5. Firmware entry with data is used instead of id->driver_data to
to retrieve the chip specific data.
Jagath Jog J (4):
iio: potentiometer: Alignment to match the open parenthesis
iio: potentiometer: Add available functionality
iio: potentiometer: Add support for Maxim DS3502
dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices
.../devicetree/bindings/trivial-devices.yaml | 2 +
drivers/iio/potentiometer/Kconfig | 6 +-
drivers/iio/potentiometer/ds1803.c | 141 +++++++++++++-----
3 files changed, 106 insertions(+), 43 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 1/4] iio: potentiometer: Alignment to match the open parenthesis 2022-02-14 3:36 [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 Jagath Jog J @ 2022-02-14 3:36 ` Jagath Jog J 2022-02-14 11:14 ` Andy Shevchenko 2022-02-14 3:36 ` [PATCH v2 2/4] iio: potentiometer: Add available functionality Jagath Jog J ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Jagath Jog J @ 2022-02-14 3:36 UTC (permalink / raw) To: jic23, lars, andy.shevchenko, sst, robh+dt Cc: linux-iio, devicetree, linux-kernel Fix following checkpatch.pl check by removing black space CHECK: Alignment should match open parenthesis Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> --- drivers/iio/potentiometer/ds1803.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c index 20b45407eaac..3481dd460525 100644 --- a/drivers/iio/potentiometer/ds1803.c +++ b/drivers/iio/potentiometer/ds1803.c @@ -55,8 +55,8 @@ static const struct iio_chan_spec ds1803_channels[] = { }; static int ds1803_read_raw(struct iio_dev *indio_dev, - struct iio_chan_spec const *chan, - int *val, int *val2, long mask) + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) { struct ds1803_data *data = iio_priv(indio_dev); int pot = chan->channel; @@ -66,7 +66,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_RAW: ret = i2c_master_recv(data->client, result, - indio_dev->num_channels); + indio_dev->num_channels); if (ret < 0) return ret; @@ -83,8 +83,8 @@ static int ds1803_read_raw(struct iio_dev *indio_dev, } static int ds1803_write_raw(struct iio_dev *indio_dev, - struct iio_chan_spec const *chan, - int val, int val2, long mask) + struct iio_chan_spec const *chan, + int val, int val2, long mask) { struct ds1803_data *data = iio_priv(indio_dev); int pot = chan->channel; @@ -110,7 +110,7 @@ static const struct iio_info ds1803_info = { }; static int ds1803_probe(struct i2c_client *client, - const struct i2c_device_id *id) + const struct i2c_device_id *id) { struct device *dev = &client->dev; struct ds1803_data *data; -- 2.17.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] iio: potentiometer: Alignment to match the open parenthesis 2022-02-14 3:36 ` [PATCH v2 1/4] iio: potentiometer: Alignment to match the open parenthesis Jagath Jog J @ 2022-02-14 11:14 ` Andy Shevchenko 0 siblings, 0 replies; 13+ messages in thread From: Andy Shevchenko @ 2022-02-14 11:14 UTC (permalink / raw) To: Jagath Jog J Cc: Jonathan Cameron, Lars-Peter Clausen, Slawomir Stepien, Rob Herring, linux-iio, devicetree, Linux Kernel Mailing List On Mon, Feb 14, 2022 at 5:36 AM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > Fix following checkpatch.pl check by removing black space > CHECK: Alignment should match open parenthesis I believe in some cases you may make them more compact, like join lines (it will be slightly longer, but still readable). OTOH, it depends on the style of the entire driver. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/4] iio: potentiometer: Add available functionality 2022-02-14 3:36 [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 Jagath Jog J 2022-02-14 3:36 ` [PATCH v2 1/4] iio: potentiometer: Alignment to match the open parenthesis Jagath Jog J @ 2022-02-14 3:36 ` Jagath Jog J 2022-02-14 11:22 ` Andy Shevchenko 2022-02-14 3:36 ` [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502 Jagath Jog J ` (2 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Jagath Jog J @ 2022-02-14 3:36 UTC (permalink / raw) To: jic23, lars, andy.shevchenko, sst, robh+dt Cc: linux-iio, devicetree, linux-kernel Adding available functionality for ds1803 driver which will show the minimum, step and maximum values it can accepts through sysfs entry. Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> --- drivers/iio/potentiometer/ds1803.c | 38 ++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c index 3481dd460525..bccb9ccac931 100644 --- a/drivers/iio/potentiometer/ds1803.c +++ b/drivers/iio/potentiometer/ds1803.c @@ -27,12 +27,13 @@ enum ds1803_type { struct ds1803_cfg { int kohms; + int avail[3]; }; static const struct ds1803_cfg ds1803_cfg[] = { - [DS1803_010] = { .kohms = 10, }, - [DS1803_050] = { .kohms = 50, }, - [DS1803_100] = { .kohms = 100, }, + [DS1803_010] = { .avail = { 0, 1, 255 }, .kohms = 10, }, + [DS1803_050] = { .avail = { 0, 1, 255 }, .kohms = 50, }, + [DS1803_100] = { .avail = { 0, 1, 255 }, .kohms = 100, }, }; struct ds1803_data { @@ -40,13 +41,14 @@ struct ds1803_data { const struct ds1803_cfg *cfg; }; -#define DS1803_CHANNEL(ch) { \ - .type = IIO_RESISTANCE, \ - .indexed = 1, \ - .output = 1, \ - .channel = (ch), \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ +#define DS1803_CHANNEL(ch) { \ + .type = IIO_RESISTANCE, \ + .indexed = 1, \ + .output = 1, \ + .channel = (ch), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_RAW), \ } static const struct iio_chan_spec ds1803_channels[] = { @@ -104,9 +106,25 @@ static int ds1803_write_raw(struct iio_dev *indio_dev, return i2c_smbus_write_byte_data(data->client, DS1803_WRITE(pot), val); } +static int ds1803_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, long mask) +{ + struct ds1803_data *data = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + *vals = data->cfg->avail; + *length = ARRAY_SIZE(data->cfg->avail); + *type = IIO_VAL_INT; + return IIO_AVAIL_RANGE; + } + return -EINVAL; +} + static const struct iio_info ds1803_info = { .read_raw = ds1803_read_raw, .write_raw = ds1803_write_raw, + .read_avail = ds1803_read_avail, }; static int ds1803_probe(struct i2c_client *client, -- 2.17.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] iio: potentiometer: Add available functionality 2022-02-14 3:36 ` [PATCH v2 2/4] iio: potentiometer: Add available functionality Jagath Jog J @ 2022-02-14 11:22 ` Andy Shevchenko 0 siblings, 0 replies; 13+ messages in thread From: Andy Shevchenko @ 2022-02-14 11:22 UTC (permalink / raw) To: Jagath Jog J Cc: Jonathan Cameron, Lars-Peter Clausen, Slawomir Stepien, Rob Herring, linux-iio, devicetree, Linux Kernel Mailing List On Mon, Feb 14, 2022 at 5:36 AM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > Adding available functionality for ds1803 driver which > will show the minimum, step and maximum values > it can accepts through sysfs entry. accept ... > struct ds1803_cfg { > int kohms; > + int avail[3]; > }; > > static const struct ds1803_cfg ds1803_cfg[] = { > - [DS1803_010] = { .kohms = 10, }, > - [DS1803_050] = { .kohms = 50, }, > - [DS1803_100] = { .kohms = 100, }, > + [DS1803_010] = { .avail = { 0, 1, 255 }, .kohms = 10, }, > + [DS1803_050] = { .avail = { 0, 1, 255 }, .kohms = 50, }, > + [DS1803_100] = { .avail = { 0, 1, 255 }, .kohms = 100, }, > }; Please, do it rather with additional data: static const int ds1803_range[] = { ... }; .avail = ..._range, Also, looking into the next patch, can you move the structure to the place where it's more appropriate? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502 2022-02-14 3:36 [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 Jagath Jog J 2022-02-14 3:36 ` [PATCH v2 1/4] iio: potentiometer: Alignment to match the open parenthesis Jagath Jog J 2022-02-14 3:36 ` [PATCH v2 2/4] iio: potentiometer: Add available functionality Jagath Jog J @ 2022-02-14 3:36 ` Jagath Jog J 2022-02-14 11:32 ` Andy Shevchenko 2022-02-14 3:36 ` [PATCH v2 4/4] dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices Jagath Jog J 2022-02-14 11:33 ` [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 Andy Shevchenko 4 siblings, 1 reply; 13+ messages in thread From: Jagath Jog J @ 2022-02-14 3:36 UTC (permalink / raw) To: jic23, lars, andy.shevchenko, sst, robh+dt Cc: linux-iio, devicetree, linux-kernel 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 Datasheet: https://datasheets.maximintegrated.com/en/ds/DS3502.pdf Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> --- drivers/iio/potentiometer/Kconfig | 6 +- drivers/iio/potentiometer/ds1803.c | 101 ++++++++++++++++++++--------- 2 files changed, 75 insertions(+), 32 deletions(-) diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig index 832df8da2bc6..cd9aaceb15fe 100644 --- a/drivers/iio/potentiometer/Kconfig +++ b/drivers/iio/potentiometer/Kconfig @@ -27,11 +27,11 @@ config AD5272 module will be called ad5272. config DS1803 - tristate "Maxim Integrated DS1803 Digital Potentiometer driver" + tristate "Maxim Integrated DS1803 and similar Digital Potentiometer driver" depends on I2C help - 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. To compile this driver as a module, choose M here: the module will be called ds1803. diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c index bccb9ccac931..0990536e7f36 100644 --- a/drivers/iio/potentiometer/ds1803.c +++ b/drivers/iio/potentiometer/ds1803.c @@ -1,12 +1,15 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Maxim Integrated DS1803 digital potentiometer driver + * Maxim Integrated DS1803 and similar digital potentiometer driver * Copyright (c) 2016 Slawomir Stepien + * Copyright (c) 2022 Jagath Jog J * * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS3502.pdf * * DEVID #Wipers #Positions Resistor Opts (kOhm) i2c address * ds1803 2 256 10, 50, 100 0101xxx + * ds3502 1 128 10 01010xx */ #include <linux/err.h> @@ -16,44 +19,64 @@ #include <linux/module.h> #include <linux/mod_devicetable.h> -#define DS1803_MAX_POS 255 -#define DS1803_WRITE(chan) (0xa8 | ((chan) + 1)) +#define DS1803_WIPER_0 0xA9 +#define DS1803_WIPER_1 0xAA +#define DS3502_WR_IVR 0x00 enum ds1803_type { DS1803_010, DS1803_050, DS1803_100, + DS3502, }; struct ds1803_cfg { + int wipers; int kohms; int avail[3]; -}; - -static const struct ds1803_cfg ds1803_cfg[] = { - [DS1803_010] = { .avail = { 0, 1, 255 }, .kohms = 10, }, - [DS1803_050] = { .avail = { 0, 1, 255 }, .kohms = 50, }, - [DS1803_100] = { .avail = { 0, 1, 255 }, .kohms = 100, }, + const struct iio_chan_spec *channels; + u8 num_channels; }; struct ds1803_data { struct i2c_client *client; const struct ds1803_cfg *cfg; + enum ds1803_type chip_type; }; -#define DS1803_CHANNEL(ch) { \ +#define DS1803_CHANNEL(ch, addr) { \ .type = IIO_RESISTANCE, \ .indexed = 1, \ .output = 1, \ .channel = (ch), \ + .address = (addr), \ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_RAW), \ } static const struct iio_chan_spec ds1803_channels[] = { - DS1803_CHANNEL(0), - DS1803_CHANNEL(1), + DS1803_CHANNEL(0, DS1803_WIPER_0), + DS1803_CHANNEL(1, DS1803_WIPER_1), +}; + +static const struct iio_chan_spec ds3502_channels[] = { + DS1803_CHANNEL(0, DS3502_WR_IVR), +}; + +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) }, }; static int ds1803_read_raw(struct iio_dev *indio_dev, @@ -67,17 +90,30 @@ static int ds1803_read_raw(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_RAW: - ret = i2c_master_recv(data->client, result, - indio_dev->num_channels); - if (ret < 0) - return ret; - - *val = result[pot]; - return IIO_VAL_INT; + switch (data->chip_type) { + case DS1803_010: + case DS1803_050: + case DS1803_100: + ret = i2c_master_recv(data->client, result, + indio_dev->num_channels); + if (ret < 0) + return ret; + *val = result[pot]; + return IIO_VAL_INT; + case DS3502: + ret = i2c_smbus_read_byte_data(data->client, + chan->address); + if (ret < 0) + return ret; + *val = ret; + return IIO_VAL_INT; + default: + return -EINVAL; + } case IIO_CHAN_INFO_SCALE: *val = 1000 * data->cfg->kohms; - *val2 = DS1803_MAX_POS; + *val2 = data->cfg->avail[2]; /* Max wiper position */ return IIO_VAL_FRACTIONAL; } @@ -89,21 +125,22 @@ static int ds1803_write_raw(struct iio_dev *indio_dev, int val, int val2, long mask) { struct ds1803_data *data = iio_priv(indio_dev); - int pot = chan->channel; + int max_pos = data->cfg->avail[2]; + u8 addr = chan->address; if (val2 != 0) return -EINVAL; switch (mask) { case IIO_CHAN_INFO_RAW: - if (val > DS1803_MAX_POS || val < 0) + if (val > max_pos || val < 0) return -EINVAL; break; default: return -EINVAL; } - return i2c_smbus_write_byte_data(data->client, DS1803_WRITE(pot), val); + return i2c_smbus_write_byte_data(data->client, addr, val); } static int ds1803_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, @@ -142,20 +179,24 @@ static int ds1803_probe(struct i2c_client *client, data = iio_priv(indio_dev); data->client = client; - 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; + data->cfg = &ds1803_cfg[data->chip_type]; indio_dev->info = &ds1803_info; - indio_dev->channels = ds1803_channels; - indio_dev->num_channels = ARRAY_SIZE(ds1803_channels); + indio_dev->channels = data->cfg->channels; + indio_dev->num_channels = data->cfg->num_channels; indio_dev->name = client->name; return devm_iio_device_register(dev, indio_dev); } static const struct of_device_id ds1803_dt_ids[] = { - { .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 }, + { .compatible = "maxim,ds3502", .data = (void *)DS3502 }, {} }; MODULE_DEVICE_TABLE(of, ds1803_dt_ids); @@ -164,6 +205,7 @@ static const struct i2c_device_id ds1803_id[] = { { "ds1803-010", DS1803_010 }, { "ds1803-050", DS1803_050 }, { "ds1803-100", DS1803_100 }, + { "ds3502", DS3502 }, {} }; MODULE_DEVICE_TABLE(i2c, ds1803_id); @@ -180,5 +222,6 @@ static struct i2c_driver ds1803_driver = { module_i2c_driver(ds1803_driver); MODULE_AUTHOR("Slawomir Stepien <sst@poczta.fm>"); +MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>"); MODULE_DESCRIPTION("DS1803 digital potentiometer"); MODULE_LICENSE("GPL v2"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502 2022-02-14 3:36 ` [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502 Jagath Jog J @ 2022-02-14 11:32 ` Andy Shevchenko 2022-02-14 19:52 ` Jagath Jog J 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2022-02-14 11:32 UTC (permalink / raw) To: Jagath Jog J Cc: Jonathan Cameron, Lars-Peter Clausen, Slawomir Stepien, Rob Herring, linux-iio, devicetree, Linux Kernel Mailing List 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. > 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. ... > -#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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502 2022-02-14 11:32 ` Andy Shevchenko @ 2022-02-14 19:52 ` Jagath Jog J 2022-02-18 12:24 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Jagath Jog J @ 2022-02-14 19:52 UTC (permalink / raw) To: Andy Shevchenko, jic23 Cc: Lars-Peter Clausen, Slawomir Stepien, Rob Herring, linux-iio, devicetree, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502 2022-02-14 19:52 ` Jagath Jog J @ 2022-02-18 12:24 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2022-02-18 12:24 UTC (permalink / raw) To: Jagath Jog J Cc: Andy Shevchenko, Lars-Peter Clausen, Slawomir Stepien, Rob Herring, linux-iio, devicetree, Linux Kernel Mailing List On Tue, 15 Feb 2022 01:22:54 +0530 Jagath Jog J <jagathjog1996@gmail.com> wrote: > 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. Usual thing is to use the and similar phrasing for the title if it is getting to long and have the one place that lists them all being the belp text. We need it somewhere so people can grep for it. > > > > ... > > > > > - * 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. I wasn't that clear on exactly where to do that! Sorry about that. > > > > > ... > > > > > -#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. I'm not totally sure what Andy is suggesting with his feedback here. > > > > > ... > > > > > - 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(). You could use a function pointer in the cfg structure and provide appropriate read functions for the different types so that read_raw() can call the appropriate function for the type of device. That would be tidy and avoid this need to change to an enum. Sorry for slow response btw as I didn't get in before v3! Thanks, Jonathan > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 4/4] dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices 2022-02-14 3:36 [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 Jagath Jog J ` (2 preceding siblings ...) 2022-02-14 3:36 ` [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502 Jagath Jog J @ 2022-02-14 3:36 ` Jagath Jog J 2022-02-14 11:32 ` Andy Shevchenko 2022-02-15 22:56 ` Rob Herring 2022-02-14 11:33 ` [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 Andy Shevchenko 4 siblings, 2 replies; 13+ messages in thread From: Jagath Jog J @ 2022-02-14 3:36 UTC (permalink / raw) To: jic23, lars, andy.shevchenko, sst, robh+dt Cc: linux-iio, devicetree, linux-kernel Maxim DS3502 is a 7 bit nonvolatile digital potentiometer. Add DS3502 binding into trivial-devices.yaml Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> --- Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index 091792ba993e..b6187603317a 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -157,6 +157,8 @@ properties: - maxim,ds1803-050 # 100 kOhm digital potentiometer with I2C interface - maxim,ds1803-100 + # 10 kOhm digital potentiometer with I2C interface + - maxim,ds3502 # Low-Power, 4-/12-Channel, 2-Wire Serial, 12-Bit ADCs - maxim,max1237 # Temperature Sensor, I2C interface -- 2.17.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices 2022-02-14 3:36 ` [PATCH v2 4/4] dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices Jagath Jog J @ 2022-02-14 11:32 ` Andy Shevchenko 2022-02-15 22:56 ` Rob Herring 1 sibling, 0 replies; 13+ messages in thread From: Andy Shevchenko @ 2022-02-14 11:32 UTC (permalink / raw) To: Jagath Jog J Cc: Jonathan Cameron, Lars-Peter Clausen, Slawomir Stepien, Rob Herring, linux-iio, devicetree, Linux Kernel Mailing List On Mon, Feb 14, 2022 at 5:36 AM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > Maxim DS3502 is a 7 bit nonvolatile digital potentiometer. > Add DS3502 binding into trivial-devices.yaml Missed grammar period here. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices 2022-02-14 3:36 ` [PATCH v2 4/4] dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices Jagath Jog J 2022-02-14 11:32 ` Andy Shevchenko @ 2022-02-15 22:56 ` Rob Herring 1 sibling, 0 replies; 13+ messages in thread From: Rob Herring @ 2022-02-15 22:56 UTC (permalink / raw) To: Jagath Jog J Cc: sst, linux-iio, lars, jic23, robh+dt, linux-kernel, devicetree, andy.shevchenko On Mon, 14 Feb 2022 09:06:20 +0530, Jagath Jog J wrote: > Maxim DS3502 is a 7 bit nonvolatile digital potentiometer. > Add DS3502 binding into trivial-devices.yaml > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> > --- > Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Please add Acked-by/Reviewed-by tags when posting new versions. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply. If a tag was not added on purpose, please state why and what changed. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 2022-02-14 3:36 [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 Jagath Jog J ` (3 preceding siblings ...) 2022-02-14 3:36 ` [PATCH v2 4/4] dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices Jagath Jog J @ 2022-02-14 11:33 ` Andy Shevchenko 4 siblings, 0 replies; 13+ messages in thread From: Andy Shevchenko @ 2022-02-14 11:33 UTC (permalink / raw) To: Jagath Jog J Cc: Jonathan Cameron, Lars-Peter Clausen, Slawomir Stepien, Rob Herring, linux-iio, devicetree, Linux Kernel Mailing List On Mon, Feb 14, 2022 at 5:36 AM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > Add dt-bindings and support for Maxim DS3502 into existing ds1803 driver. > DS3502 is a 7 bit Nonvolatile Digital Potentiometer. A very good start! A few minor comments to be addressed and one patch has to be split to at least three. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-02-18 12:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-14 3:36 [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 Jagath Jog J 2022-02-14 3:36 ` [PATCH v2 1/4] iio: potentiometer: Alignment to match the open parenthesis Jagath Jog J 2022-02-14 11:14 ` Andy Shevchenko 2022-02-14 3:36 ` [PATCH v2 2/4] iio: potentiometer: Add available functionality Jagath Jog J 2022-02-14 11:22 ` Andy Shevchenko 2022-02-14 3:36 ` [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502 Jagath Jog J 2022-02-14 11:32 ` Andy Shevchenko 2022-02-14 19:52 ` Jagath Jog J 2022-02-18 12:24 ` Jonathan Cameron 2022-02-14 3:36 ` [PATCH v2 4/4] dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices Jagath Jog J 2022-02-14 11:32 ` Andy Shevchenko 2022-02-15 22:56 ` Rob Herring 2022-02-14 11:33 ` [PATCH v2 0/4] iio: potentiometer: Add support for DS3502 Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).