Re: [PATCH] staging: iio: ad5933: finalized device-tree support
On lis 23, 2018 21:51, Marcelo Schmitt wrote: > Added a of_device_id struct variable and subsequent call to > MODULE_DEVICE_TABLE macro to complete device-tree support for this > driver. > > Signed-off-by: Marcelo Schmitt > --- > drivers/staging/iio/impedance-analyzer/ad5933.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c > b/drivers/staging/iio/impedance-analyzer/ad5933.c > index edb8b540bbf1..19e8b6d2865c 100644 > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c > @@ -771,9 +771,18 @@ static const struct i2c_device_id ad5933_id[] = { > > MODULE_DEVICE_TABLE(i2c, ad5933_id); > > +static const struct of_device_id ad5933_of_match[] = { > + { .compatible = "analog-devices,ad5933" }, > + { .compatible = "analog-devices,ad5934" }, Why "analog-devices", rather than "adi"? My understanding is that analog devices have "adi" as a manufacture part in compatible. > + { }, > +}; > + > +MODULE_DEVICE_TABLE(of, ad5933_of_match); > + > static struct i2c_driver ad5933_driver = { > .driver = { > .name = "ad5933", > + .of_match_table = ad5933_of_match, > }, > .probe = ad5933_probe, > .remove = ad5933_remove, -- Slawomir Stepien
Re: [PATCH] staging: iio: ad5933: finalized device-tree support
On lis 23, 2018 21:51, Marcelo Schmitt wrote: > Added a of_device_id struct variable and subsequent call to > MODULE_DEVICE_TABLE macro to complete device-tree support for this > driver. > > Signed-off-by: Marcelo Schmitt > --- > drivers/staging/iio/impedance-analyzer/ad5933.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c > b/drivers/staging/iio/impedance-analyzer/ad5933.c > index edb8b540bbf1..19e8b6d2865c 100644 > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c > @@ -771,9 +771,18 @@ static const struct i2c_device_id ad5933_id[] = { > > MODULE_DEVICE_TABLE(i2c, ad5933_id); > > +static const struct of_device_id ad5933_of_match[] = { > + { .compatible = "analog-devices,ad5933" }, > + { .compatible = "analog-devices,ad5934" }, Why "analog-devices", rather than "adi"? My understanding is that analog devices have "adi" as a manufacture part in compatible. > + { }, > +}; > + > +MODULE_DEVICE_TABLE(of, ad5933_of_match); > + > static struct i2c_driver ad5933_driver = { > .driver = { > .name = "ad5933", > + .of_match_table = ad5933_of_match, > }, > .probe = ad5933_probe, > .remove = ad5933_remove, -- Slawomir Stepien
Re: [PATCH v5 2/2] staging: iio: ad2s1210: Add device tree support.
Hi On paź 26, 2018 18:55, Nishad Kamdar wrote: > Add device tree table for matching vendor ID > and support for retrieving platform data > from device tree. So maybe you should make 2 commits? > Signed-off-by: Nishad Kamdar > --- > drivers/staging/iio/resolver/ad2s1210.c | 43 - > 1 file changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c > b/drivers/staging/iio/resolver/ad2s1210.c > index 93c3c70ce62e..9fd5461c4ed0 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -669,6 +670,27 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state > *st) > return 0; > } > > +#ifdef CONFIG_OF > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev) > +{ > + struct device_node *np = dev->of_node; > + struct ad2s1210_platform_data *pdata; > + > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return NULL; > + > + pdata->gpioin = of_property_read_bool(np, "adi,gpioin"); Why here you are adding "adi", but you are not adding it to the gpios in prev commit? I've also seen this: https://patchwork.kernel.org/patch/10355839/. However I do not understand why adding vendor id to props is needed... > + > + return pdata; > +} > +#else > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev) > +{ > + return NULL; > +} > +#endif > + > static int ad2s1210_probe(struct spi_device *spi) > { > struct iio_dev *indio_dev; > @@ -682,7 +704,19 @@ static int ad2s1210_probe(struct spi_device *spi) > if (!indio_dev) > return -ENOMEM; > st = iio_priv(indio_dev); > - st->pdata = spi->dev.platform_data; > + if (spi->dev.of_node) { > + st->pdata = ad2s1210_parse_dt(>dev); > + if (!st->pdata) > + return -EINVAL; > + } else { > + st->pdata = spi->dev.platform_data; > + } > + > + if (!st->pdata) { > + dev_err(>dev, "ad2s1210: no platform data supplied\n"); > + return -EINVAL; > + } > + Why not just use only device-tree here? The ad2s1210_platform_data has now only one entry... In that case remember to add "depends on OF" in Kconfig. > ret = ad2s1210_setup_gpios(st); > if (ret < 0) > return ret; > @@ -724,6 +758,12 @@ static int ad2s1210_remove(struct spi_device *spi) > return 0; > } > > +static const struct of_device_id ad2s1210_of_match[] = { > + { .compatible = "adi,ad2s1210", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ad2s1210_of_match); > + > static const struct spi_device_id ad2s1210_id[] = { > { "ad2s1210" }, > {} > @@ -733,6 +773,7 @@ MODULE_DEVICE_TABLE(spi, ad2s1210_id); > static struct spi_driver ad2s1210_driver = { > .driver = { > .name = DRV_NAME, > + .of_match_table = of_match_ptr(ad2s1210_of_match), > }, > .probe = ad2s1210_probe, > .remove = ad2s1210_remove, -- Slawomir Stepien
Re: [PATCH v5 2/2] staging: iio: ad2s1210: Add device tree support.
Hi On paź 26, 2018 18:55, Nishad Kamdar wrote: > Add device tree table for matching vendor ID > and support for retrieving platform data > from device tree. So maybe you should make 2 commits? > Signed-off-by: Nishad Kamdar > --- > drivers/staging/iio/resolver/ad2s1210.c | 43 - > 1 file changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c > b/drivers/staging/iio/resolver/ad2s1210.c > index 93c3c70ce62e..9fd5461c4ed0 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -669,6 +670,27 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state > *st) > return 0; > } > > +#ifdef CONFIG_OF > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev) > +{ > + struct device_node *np = dev->of_node; > + struct ad2s1210_platform_data *pdata; > + > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return NULL; > + > + pdata->gpioin = of_property_read_bool(np, "adi,gpioin"); Why here you are adding "adi", but you are not adding it to the gpios in prev commit? I've also seen this: https://patchwork.kernel.org/patch/10355839/. However I do not understand why adding vendor id to props is needed... > + > + return pdata; > +} > +#else > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev) > +{ > + return NULL; > +} > +#endif > + > static int ad2s1210_probe(struct spi_device *spi) > { > struct iio_dev *indio_dev; > @@ -682,7 +704,19 @@ static int ad2s1210_probe(struct spi_device *spi) > if (!indio_dev) > return -ENOMEM; > st = iio_priv(indio_dev); > - st->pdata = spi->dev.platform_data; > + if (spi->dev.of_node) { > + st->pdata = ad2s1210_parse_dt(>dev); > + if (!st->pdata) > + return -EINVAL; > + } else { > + st->pdata = spi->dev.platform_data; > + } > + > + if (!st->pdata) { > + dev_err(>dev, "ad2s1210: no platform data supplied\n"); > + return -EINVAL; > + } > + Why not just use only device-tree here? The ad2s1210_platform_data has now only one entry... In that case remember to add "depends on OF" in Kconfig. > ret = ad2s1210_setup_gpios(st); > if (ret < 0) > return ret; > @@ -724,6 +758,12 @@ static int ad2s1210_remove(struct spi_device *spi) > return 0; > } > > +static const struct of_device_id ad2s1210_of_match[] = { > + { .compatible = "adi,ad2s1210", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ad2s1210_of_match); > + > static const struct spi_device_id ad2s1210_id[] = { > { "ad2s1210" }, > {} > @@ -733,6 +773,7 @@ MODULE_DEVICE_TABLE(spi, ad2s1210_id); > static struct spi_driver ad2s1210_driver = { > .driver = { > .name = DRV_NAME, > + .of_match_table = of_match_ptr(ad2s1210_of_match), > }, > .probe = ad2s1210_probe, > .remove = ad2s1210_remove, -- Slawomir Stepien
Re: [PATCH v4] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On paź 24, 2018 20:20, Nishad Kamdar wrote: > Use the gpiod interface instead of the deprecated old non-descriptor > interface. > > Signed-off-by: Nishad Kamdar > --- > Changes in v4: > - Add spaces after { and before } in gpios[] >initialization. > - Check the correct pointer for error. > - Align the dev_err msg to existing format in the code. > Changes in v3: > - Use a pointer to pointer for gpio_desc in >struct ad2s1210_gpio as it will be used to >modify a pointer. > - Use dot notation to initialize the structure. > - Use a pointer variable to avoid writing gpios[i]. > Changes in v2: > - Use the spi_device struct embedded in st instead >of passing it as an argument to ad2s1210_setup_gpios(). > - Use an array of structs to reduce redundant code in >in ad2s1210_setup_gpios(). > - Remove ad2s1210_free_gpios() as devm API is being used. > --- > drivers/staging/iio/resolver/ad2s1210.c | 92 ++--- > drivers/staging/iio/resolver/ad2s1210.h | 3 - > 2 files changed, 50 insertions(+), 45 deletions(-) Looks good to me. Reviewed-by: Slawomir Stepien -- Slawomir Stepien
Re: [PATCH v4] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On paź 24, 2018 20:20, Nishad Kamdar wrote: > Use the gpiod interface instead of the deprecated old non-descriptor > interface. > > Signed-off-by: Nishad Kamdar > --- > Changes in v4: > - Add spaces after { and before } in gpios[] >initialization. > - Check the correct pointer for error. > - Align the dev_err msg to existing format in the code. > Changes in v3: > - Use a pointer to pointer for gpio_desc in >struct ad2s1210_gpio as it will be used to >modify a pointer. > - Use dot notation to initialize the structure. > - Use a pointer variable to avoid writing gpios[i]. > Changes in v2: > - Use the spi_device struct embedded in st instead >of passing it as an argument to ad2s1210_setup_gpios(). > - Use an array of structs to reduce redundant code in >in ad2s1210_setup_gpios(). > - Remove ad2s1210_free_gpios() as devm API is being used. > --- > drivers/staging/iio/resolver/ad2s1210.c | 92 ++--- > drivers/staging/iio/resolver/ad2s1210.h | 3 - > 2 files changed, 50 insertions(+), 45 deletions(-) Looks good to me. Reviewed-by: Slawomir Stepien -- Slawomir Stepien
Re: [PATCH v3] staging: iio: ad2s1210: Switch to the gpio descriptor interface
e, 0); > /* delay (6 * tck + 20) nano seconds */ > udelay(1); > > @@ -512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > } > > error_ret: > - gpio_set_value(st->pdata->sample, 1); > + gpiod_set_value(st->sample, 1); > /* delay (2 * tck + 20) nano seconds */ > udelay(1); > mutex_unlock(>lock); > @@ -630,30 +641,32 @@ static const struct iio_info ad2s1210_info = { > > static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > + int ret, i; > + struct spi_device *spi = st->sdev; > + struct ad2s1210_gpio *pin; > + unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW; > + > + struct ad2s1210_gpio gpios[] = { > + {.ptr = >sample, .name = "sample", .flags = GPIOD_IN}, > + {.ptr = >a0, .name = "a0", .flags = flags}, > + {.ptr = >a1, .name = "a1", .flags = flags}, > + {.ptr = >res0, .name = "res0", .flags = flags}, > + {.ptr = >res1, .name = "res1", .flags = flags}, > }; I think that you should add spaces after { and before }. It's the file style from what I see. > > - return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > -} > - > -static void ad2s1210_free_gpios(struct ad2s1210_state *st) > -{ > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > + for (i = 0; i < ARRAY_SIZE(gpios); i++) { > + pin = [i]; > + *pin->ptr = devm_gpiod_get(>dev, pin->name, > +pin->flags); This can now fit into one line. > + if (IS_ERR(pin->ptr)) { Here you are checking the wrong pointer. Also notice the line below. > + ret = PTR_ERR(pin->ptr); > + dev_err(>dev, "Failed to request %s GPIO: %d\n", > + pin->name, ret); Please notice how other dev_err looks like in this code. Align the message to the existing format. > + return ret; > + } > + } > > - gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + return 0; > } > > static int ad2s1210_probe(struct spi_device *spi) > @@ -692,7 +705,7 @@ static int ad2s1210_probe(struct spi_device *spi) > > ret = iio_device_register(indio_dev); > if (ret) > - goto error_free_gpios; > + return ret; > > st->fclkin = spi->max_speed_hz; > spi->mode = SPI_MODE_3; > @@ -700,10 +713,6 @@ static int ad2s1210_probe(struct spi_device *spi) > ad2s1210_initial(st); > > return 0; > - > -error_free_gpios: > - ad2s1210_free_gpios(st); > - return ret; > } > > static int ad2s1210_remove(struct spi_device *spi) > @@ -711,7 +720,6 @@ static int ad2s1210_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > iio_device_unregister(indio_dev); > - ad2s1210_free_gpios(iio_priv(indio_dev)); > > return 0; > } > diff --git a/drivers/staging/iio/resolver/ad2s1210.h > b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..63d479b20a6c 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -12,9 +12,6 @@ > #define _AD2S1210_H > > struct ad2s1210_platform_data { > - unsigned intsample; > - unsigned inta[2]; > - unsigned intres[2]; > boolgpioin; > }; > #endif /* _AD2S1210_H */ -- Slawomir Stepien
Re: [PATCH v3] staging: iio: ad2s1210: Switch to the gpio descriptor interface
e, 0); > /* delay (6 * tck + 20) nano seconds */ > udelay(1); > > @@ -512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > } > > error_ret: > - gpio_set_value(st->pdata->sample, 1); > + gpiod_set_value(st->sample, 1); > /* delay (2 * tck + 20) nano seconds */ > udelay(1); > mutex_unlock(>lock); > @@ -630,30 +641,32 @@ static const struct iio_info ad2s1210_info = { > > static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > + int ret, i; > + struct spi_device *spi = st->sdev; > + struct ad2s1210_gpio *pin; > + unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW; > + > + struct ad2s1210_gpio gpios[] = { > + {.ptr = >sample, .name = "sample", .flags = GPIOD_IN}, > + {.ptr = >a0, .name = "a0", .flags = flags}, > + {.ptr = >a1, .name = "a1", .flags = flags}, > + {.ptr = >res0, .name = "res0", .flags = flags}, > + {.ptr = >res1, .name = "res1", .flags = flags}, > }; I think that you should add spaces after { and before }. It's the file style from what I see. > > - return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > -} > - > -static void ad2s1210_free_gpios(struct ad2s1210_state *st) > -{ > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > + for (i = 0; i < ARRAY_SIZE(gpios); i++) { > + pin = [i]; > + *pin->ptr = devm_gpiod_get(>dev, pin->name, > +pin->flags); This can now fit into one line. > + if (IS_ERR(pin->ptr)) { Here you are checking the wrong pointer. Also notice the line below. > + ret = PTR_ERR(pin->ptr); > + dev_err(>dev, "Failed to request %s GPIO: %d\n", > + pin->name, ret); Please notice how other dev_err looks like in this code. Align the message to the existing format. > + return ret; > + } > + } > > - gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + return 0; > } > > static int ad2s1210_probe(struct spi_device *spi) > @@ -692,7 +705,7 @@ static int ad2s1210_probe(struct spi_device *spi) > > ret = iio_device_register(indio_dev); > if (ret) > - goto error_free_gpios; > + return ret; > > st->fclkin = spi->max_speed_hz; > spi->mode = SPI_MODE_3; > @@ -700,10 +713,6 @@ static int ad2s1210_probe(struct spi_device *spi) > ad2s1210_initial(st); > > return 0; > - > -error_free_gpios: > - ad2s1210_free_gpios(st); > - return ret; > } > > static int ad2s1210_remove(struct spi_device *spi) > @@ -711,7 +720,6 @@ static int ad2s1210_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > iio_device_unregister(indio_dev); > - ad2s1210_free_gpios(iio_priv(indio_dev)); > > return 0; > } > diff --git a/drivers/staging/iio/resolver/ad2s1210.h > b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..63d479b20a6c 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -12,9 +12,6 @@ > #define _AD2S1210_H > > struct ad2s1210_platform_data { > - unsigned intsample; > - unsigned inta[2]; > - unsigned intres[2]; > boolgpioin; > }; > #endif /* _AD2S1210_H */ -- Slawomir Stepien
Re: [PATCH v2] staging: iio: ad2s1210: Switch to the gpio descriptor interface
d2s1210_read_raw(struct iio_dev *indio_dev, > } > > error_ret: > - gpio_set_value(st->pdata->sample, 1); > + gpiod_set_value(st->sample, 1); > /* delay (2 * tck + 20) nano seconds */ > udelay(1); > mutex_unlock(>lock); > @@ -630,30 +641,30 @@ static const struct iio_info ad2s1210_info = { > > static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > + int ret = 0, i = 0; You do not need to init that i with 0. Also the ret do not have to be 0. If you return just 0 at the on this function. > + struct spi_device *spi = st->sdev; > + unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW; > + > + struct ad2s1210_gpio gpios[] = { > + {st->sample, "sample", GPIOD_IN}, > + {st->a0, "a0", flags}, > + {st->a1, "a1", flags}, > + {st->res0, "res0", flags}, > + {st->res1, "res1", flags}, To change a pointer, you need to pass an address of that pointer. I think you should you . notation when initializing the struct. > }; > > - return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > -} > - > -static void ad2s1210_free_gpios(struct ad2s1210_state *st) > -{ > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > + for (i = 0; i < ARRAY_SIZE(gpios); i++) { I do not know if that is a common practise, but you can set a pointer to gpio[i] as the first instruction in this for, so you would not have to write this whole gpio[i]: pin = [i] and then: pin->ptr, etc. > + gpios[i].ptr = devm_gpiod_get(>dev, gpios[i].name, > + gpios[i].flags); > + if (IS_ERR(gpios[i].ptr)) { > + ret = PTR_ERR(gpios[i].ptr); > + dev_err(>dev, "Failed to request %s GPIO: %d\n", > + gpios[i].name, ret); > + return ret; > + } > + } > > - gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + return ret; > } > > static int ad2s1210_probe(struct spi_device *spi) > @@ -692,18 +703,13 @@ static int ad2s1210_probe(struct spi_device *spi) > > ret = iio_device_register(indio_dev); > if (ret) > - goto error_free_gpios; > - Removing a empty line here, that is here to make it more readable. > + return ret; > st->fclkin = spi->max_speed_hz; > spi->mode = SPI_MODE_3; > spi_setup(spi); > ad2s1210_initial(st); > > return 0; > - > -error_free_gpios: > - ad2s1210_free_gpios(st); > - return ret; > } > > static int ad2s1210_remove(struct spi_device *spi) > @@ -711,7 +717,6 @@ static int ad2s1210_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > iio_device_unregister(indio_dev); > - ad2s1210_free_gpios(iio_priv(indio_dev)); > > return 0; > } > diff --git a/drivers/staging/iio/resolver/ad2s1210.h > b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..63d479b20a6c 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -12,9 +12,6 @@ > #define _AD2S1210_H > > struct ad2s1210_platform_data { > - unsigned intsample; > - unsigned inta[2]; > - unsigned intres[2]; > boolgpioin; > }; > #endif /* _AD2S1210_H */ -- Slawomir Stepien
Re: [PATCH v2] staging: iio: ad2s1210: Switch to the gpio descriptor interface
d2s1210_read_raw(struct iio_dev *indio_dev, > } > > error_ret: > - gpio_set_value(st->pdata->sample, 1); > + gpiod_set_value(st->sample, 1); > /* delay (2 * tck + 20) nano seconds */ > udelay(1); > mutex_unlock(>lock); > @@ -630,30 +641,30 @@ static const struct iio_info ad2s1210_info = { > > static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > + int ret = 0, i = 0; You do not need to init that i with 0. Also the ret do not have to be 0. If you return just 0 at the on this function. > + struct spi_device *spi = st->sdev; > + unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW; > + > + struct ad2s1210_gpio gpios[] = { > + {st->sample, "sample", GPIOD_IN}, > + {st->a0, "a0", flags}, > + {st->a1, "a1", flags}, > + {st->res0, "res0", flags}, > + {st->res1, "res1", flags}, To change a pointer, you need to pass an address of that pointer. I think you should you . notation when initializing the struct. > }; > > - return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > -} > - > -static void ad2s1210_free_gpios(struct ad2s1210_state *st) > -{ > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > + for (i = 0; i < ARRAY_SIZE(gpios); i++) { I do not know if that is a common practise, but you can set a pointer to gpio[i] as the first instruction in this for, so you would not have to write this whole gpio[i]: pin = [i] and then: pin->ptr, etc. > + gpios[i].ptr = devm_gpiod_get(>dev, gpios[i].name, > + gpios[i].flags); > + if (IS_ERR(gpios[i].ptr)) { > + ret = PTR_ERR(gpios[i].ptr); > + dev_err(>dev, "Failed to request %s GPIO: %d\n", > + gpios[i].name, ret); > + return ret; > + } > + } > > - gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + return ret; > } > > static int ad2s1210_probe(struct spi_device *spi) > @@ -692,18 +703,13 @@ static int ad2s1210_probe(struct spi_device *spi) > > ret = iio_device_register(indio_dev); > if (ret) > - goto error_free_gpios; > - Removing a empty line here, that is here to make it more readable. > + return ret; > st->fclkin = spi->max_speed_hz; > spi->mode = SPI_MODE_3; > spi_setup(spi); > ad2s1210_initial(st); > > return 0; > - > -error_free_gpios: > - ad2s1210_free_gpios(st); > - return ret; > } > > static int ad2s1210_remove(struct spi_device *spi) > @@ -711,7 +717,6 @@ static int ad2s1210_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > iio_device_unregister(indio_dev); > - ad2s1210_free_gpios(iio_priv(indio_dev)); > > return 0; > } > diff --git a/drivers/staging/iio/resolver/ad2s1210.h > b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..63d479b20a6c 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -12,9 +12,6 @@ > #define _AD2S1210_H > > struct ad2s1210_platform_data { > - unsigned intsample; > - unsigned inta[2]; > - unsigned intres[2]; > boolgpioin; > }; > #endif /* _AD2S1210_H */ -- Slawomir Stepien
Re: [PATCH] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On paź 21, 2018 10:31, Slawomir Stepien wrote: > On paź 21, 2018 11:49, Nishad Kamdar wrote: > > -static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > > +static int ad2s1210_setup_gpios(struct spi_device *spi, > > + struct ad2s1210_state *st) > > This change is not needed. The st has the spi_device inside. Use > container_of() > macro here. Wait...what? I mean use direct access using st->sdev. -- Slawomir Stepien
Re: [PATCH] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On paź 21, 2018 10:31, Slawomir Stepien wrote: > On paź 21, 2018 11:49, Nishad Kamdar wrote: > > -static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > > +static int ad2s1210_setup_gpios(struct spi_device *spi, > > + struct ad2s1210_state *st) > > This change is not needed. The st has the spi_device inside. Use > container_of() > macro here. Wait...what? I mean use direct access using st->sdev. -- Slawomir Stepien
Re: [PATCH] staging: iio: ad2s1210: Switch to the gpio descriptor interface
->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > + int ret = 0; > + unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW; > + > + st->sample = devm_gpiod_get(>dev, "sample", GPIOD_IN); > + if (IS_ERR(st->sample)) { > + ret = PTR_ERR(st->sample); > + dev_err(>dev, "Failed to request sample GPIO: %d\n", > + ret); > + return ret; > + } > + st->a0 = devm_gpiod_get(>dev, "a0", flags); > + if (IS_ERR(st->a0)) { > + ret = PTR_ERR(st->a0); > + dev_err(>dev, "Failed to request a0 GPIO: %d\n", > + ret); > + return ret; > + } > + st->a1 = devm_gpiod_get(>dev, "a1", flags); > + if (IS_ERR(st->a1)) { > + ret = PTR_ERR(st->a1); > + dev_err(>dev, "Failed to request a1 GPIO: %d\n", > + ret); > + return ret; > + } > + st->res0 = devm_gpiod_get(>dev, "res0", flags); > + if (IS_ERR(st->res0)) { > + ret = PTR_ERR(st->res0); > + dev_err(>dev, "Failed to request res0 GPIO: %d\n", > + ret); > + return ret; > + } > + st->res1 = devm_gpiod_get(>dev, "res1", flags); > + if (IS_ERR(st->res1)) { > + ret = PTR_ERR(st->res1); > + dev_err(>dev, "Failed to request res1 GPIO: %d\n", > + ret); > + return ret; > + } To reduce the redundant code here, create an array of structs. The struct will have a pointer to gpio_desc, name of the gpio and the gpios' flags. Then you can use for loop to get all the needed gpios: for (l = 0; l < ARRAY_SIZE(str); l++) { str[l].ptr = devm_gpiod_get(>dev, str[l].name, str[l].flags); if (IS_ERR(str[l].res1)) { ... } } > - return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + return ret; > } > > -static void ad2s1210_free_gpios(struct ad2s1210_state *st) > +static void ad2s1210_free_gpios(struct spi_device *spi, > + struct ad2s1210_state *st) > { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > - > - gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + devm_gpiod_put(>dev, st->sample); > + devm_gpiod_put(>dev, st->a0); > + devm_gpiod_put(>dev, st->a1); > + devm_gpiod_put(>dev, st->res0); > + devm_gpiod_put(>dev, st->res1); > } This whole function ad2s1210_free_gpios is not needed anymore because you are using the devm API (it's called from probe and remove, so you are safe). > static int ad2s1210_probe(struct spi_device *spi) > @@ -670,7 +702,7 @@ static int ad2s1210_probe(struct spi_device *spi) > return -ENOMEM; > st = iio_priv(indio_dev); > st->pdata = spi->dev.platform_data; > - ret = ad2s1210_setup_gpios(st); > + ret = ad2s1210_setup_gpios(spi, st); > if (ret < 0) > return ret; > > @@ -702,7 +734,7 @@ static int ad2s1210_probe(struct spi_device *spi) > return 0; > > error_free_gpios: > - ad2s1210_free_gpios(st); > + ad2s1210_free_gpios(spi, st); > return ret; > } > > @@ -711,7 +743,7 @@ static int ad2s1210_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > iio_device_unregister(indio_dev); > - ad2s1210_free_gpios(iio_priv(indio_dev)); > + ad2s1210_free_gpios(spi, iio_priv(indio_dev)); > > return 0; > } > diff --git a/drivers/staging/iio/resolver/ad2s1210.h > b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..63d479b20a6c 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -12,9 +12,6 @@ > #define _AD2S1210_H > > struct ad2s1210_platform_data { > - unsigned intsample; > - unsigned inta[2]; > - unsigned intres[2]; > boolgpioin; > }; > #endif /* _AD2S1210_H */ -- Slawomir Stepien
Re: [PATCH] staging: iio: ad2s1210: Switch to the gpio descriptor interface
->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > + int ret = 0; > + unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW; > + > + st->sample = devm_gpiod_get(>dev, "sample", GPIOD_IN); > + if (IS_ERR(st->sample)) { > + ret = PTR_ERR(st->sample); > + dev_err(>dev, "Failed to request sample GPIO: %d\n", > + ret); > + return ret; > + } > + st->a0 = devm_gpiod_get(>dev, "a0", flags); > + if (IS_ERR(st->a0)) { > + ret = PTR_ERR(st->a0); > + dev_err(>dev, "Failed to request a0 GPIO: %d\n", > + ret); > + return ret; > + } > + st->a1 = devm_gpiod_get(>dev, "a1", flags); > + if (IS_ERR(st->a1)) { > + ret = PTR_ERR(st->a1); > + dev_err(>dev, "Failed to request a1 GPIO: %d\n", > + ret); > + return ret; > + } > + st->res0 = devm_gpiod_get(>dev, "res0", flags); > + if (IS_ERR(st->res0)) { > + ret = PTR_ERR(st->res0); > + dev_err(>dev, "Failed to request res0 GPIO: %d\n", > + ret); > + return ret; > + } > + st->res1 = devm_gpiod_get(>dev, "res1", flags); > + if (IS_ERR(st->res1)) { > + ret = PTR_ERR(st->res1); > + dev_err(>dev, "Failed to request res1 GPIO: %d\n", > + ret); > + return ret; > + } To reduce the redundant code here, create an array of structs. The struct will have a pointer to gpio_desc, name of the gpio and the gpios' flags. Then you can use for loop to get all the needed gpios: for (l = 0; l < ARRAY_SIZE(str); l++) { str[l].ptr = devm_gpiod_get(>dev, str[l].name, str[l].flags); if (IS_ERR(str[l].res1)) { ... } } > - return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + return ret; > } > > -static void ad2s1210_free_gpios(struct ad2s1210_state *st) > +static void ad2s1210_free_gpios(struct spi_device *spi, > + struct ad2s1210_state *st) > { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > - > - gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + devm_gpiod_put(>dev, st->sample); > + devm_gpiod_put(>dev, st->a0); > + devm_gpiod_put(>dev, st->a1); > + devm_gpiod_put(>dev, st->res0); > + devm_gpiod_put(>dev, st->res1); > } This whole function ad2s1210_free_gpios is not needed anymore because you are using the devm API (it's called from probe and remove, so you are safe). > static int ad2s1210_probe(struct spi_device *spi) > @@ -670,7 +702,7 @@ static int ad2s1210_probe(struct spi_device *spi) > return -ENOMEM; > st = iio_priv(indio_dev); > st->pdata = spi->dev.platform_data; > - ret = ad2s1210_setup_gpios(st); > + ret = ad2s1210_setup_gpios(spi, st); > if (ret < 0) > return ret; > > @@ -702,7 +734,7 @@ static int ad2s1210_probe(struct spi_device *spi) > return 0; > > error_free_gpios: > - ad2s1210_free_gpios(st); > + ad2s1210_free_gpios(spi, st); > return ret; > } > > @@ -711,7 +743,7 @@ static int ad2s1210_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > iio_device_unregister(indio_dev); > - ad2s1210_free_gpios(iio_priv(indio_dev)); > + ad2s1210_free_gpios(spi, iio_priv(indio_dev)); > > return 0; > } > diff --git a/drivers/staging/iio/resolver/ad2s1210.h > b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..63d479b20a6c 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -12,9 +12,6 @@ > #define _AD2S1210_H > > struct ad2s1210_platform_data { > - unsigned intsample; > - unsigned inta[2]; > - unsigned intres[2]; > boolgpioin; > }; > #endif /* _AD2S1210_H */ -- Slawomir Stepien
Re: [PATCH] staging: iio: adc: Add comments to prevent checks corrections
On paź 15, 2018 18:25, Gabriel Capella wrote: > This patch adds 3 comments in 2 different files. > These comments warn to don't correct the checks of type: > "CHECK: spaces preferred around that '-'" > > Signed-off-by: Gabriel Capella > --- > drivers/staging/iio/adc/ad7192.c | 1 + > drivers/staging/iio/adc/ad7280a.c | 2 ++ > 2 files changed, 3 insertions(+) I have this simpler solution...let's focus our efforts on moving the two drivers out of staging. This will prevent the CHK to appear: Cut from checkpatch.pl: if ($realfile =~ m@^(?:drivers/net/|net/|drivers/staging/)@) { $check = 1; Existing driver out of staging, with this "problem": $ ./scripts/checkpatch.pl --types SPACING --file drivers/iio/adc/ad7793.c total: 0 errors, 0 warnings, 827 lines checked drivers/iio/adc/ad7793.c has no obvious style problems and is ready for submission. NOTE: Used message types: SPACING In my opinion it would stop this incorrect patches. I have also this changes to checkpatch.pl: https://github.com/s-stepien/linux-1/commit/c976a31b392393fb417f2d9e2ec9639bc226ad0b and usage: https://github.com/s-stepien/linux-1/commit/1adc0428b496f44f6a931637084bb619ddd9992d but I'm not that sure it is the best way to go. What do you all think? -- Slawomir Stepien
Re: [PATCH] staging: iio: adc: Add comments to prevent checks corrections
On paź 15, 2018 18:25, Gabriel Capella wrote: > This patch adds 3 comments in 2 different files. > These comments warn to don't correct the checks of type: > "CHECK: spaces preferred around that '-'" > > Signed-off-by: Gabriel Capella > --- > drivers/staging/iio/adc/ad7192.c | 1 + > drivers/staging/iio/adc/ad7280a.c | 2 ++ > 2 files changed, 3 insertions(+) I have this simpler solution...let's focus our efforts on moving the two drivers out of staging. This will prevent the CHK to appear: Cut from checkpatch.pl: if ($realfile =~ m@^(?:drivers/net/|net/|drivers/staging/)@) { $check = 1; Existing driver out of staging, with this "problem": $ ./scripts/checkpatch.pl --types SPACING --file drivers/iio/adc/ad7793.c total: 0 errors, 0 warnings, 827 lines checked drivers/iio/adc/ad7793.c has no obvious style problems and is ready for submission. NOTE: Used message types: SPACING In my opinion it would stop this incorrect patches. I have also this changes to checkpatch.pl: https://github.com/s-stepien/linux-1/commit/c976a31b392393fb417f2d9e2ec9639bc226ad0b and usage: https://github.com/s-stepien/linux-1/commit/1adc0428b496f44f6a931637084bb619ddd9992d but I'm not that sure it is the best way to go. What do you all think? -- Slawomir Stepien
Re: [PATCH] staging: iio: adc: ad7280a: fix 2 checks
On paź 11, 2018 10:32, Dan Carpenter wrote: > On Sat, Oct 06, 2018 at 10:25:42PM +0200, Slawomir Stepien wrote: > > On paź 06, 2018 13:27, Gabriel Capella wrote: > > > This patch does not change the logic, it only > > > corrects the checkpatch checks. > > > > > > The patch fixes 2 checks of type: > > > "CHECK: spaces preferred around that '-'" > > > > I've made the same mistake few days ago. This change is incorrect. > > Please see: https://lore.kernel.org/patchwork/patch/635994/. > > > > You could add a comment. /* do not put spaces around the hyphen. > #checkpatch.pl */ > > These are the only places which use this style and a lot of people > bump into it. Gabriel go ahead if you want to. If not then I would be happy to prepare this patch. -- Slawomir Stepien
Re: [PATCH] staging: iio: adc: ad7280a: fix 2 checks
On paź 11, 2018 10:32, Dan Carpenter wrote: > On Sat, Oct 06, 2018 at 10:25:42PM +0200, Slawomir Stepien wrote: > > On paź 06, 2018 13:27, Gabriel Capella wrote: > > > This patch does not change the logic, it only > > > corrects the checkpatch checks. > > > > > > The patch fixes 2 checks of type: > > > "CHECK: spaces preferred around that '-'" > > > > I've made the same mistake few days ago. This change is incorrect. > > Please see: https://lore.kernel.org/patchwork/patch/635994/. > > > > You could add a comment. /* do not put spaces around the hyphen. > #checkpatch.pl */ > > These are the only places which use this style and a lot of people > bump into it. Gabriel go ahead if you want to. If not then I would be happy to prepare this patch. -- Slawomir Stepien
Re: [PATCH] staging: iio: adc: ad7280a: fix 2 checks
On paź 06, 2018 13:27, Gabriel Capella wrote: > This patch does not change the logic, it only > corrects the checkpatch checks. > > The patch fixes 2 checks of type: > "CHECK: spaces preferred around that '-'" I've made the same mistake few days ago. This change is incorrect. Please see: https://lore.kernel.org/patchwork/patch/635994/. > Signed-off-by: Gabriel Capella > --- > drivers/staging/iio/adc/ad7280a.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7280a.c > b/drivers/staging/iio/adc/ad7280a.c > index 58420dcb406d..a4b4f8678c56 100644 > --- a/drivers/staging/iio/adc/ad7280a.c > +++ b/drivers/staging/iio/adc/ad7280a.c > @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void > *private) > } > > static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value, > - in_voltage-voltage_thresh_low_value, > + in_voltage - voltage_thresh_low_value, > 0644, > ad7280_read_channel_config, > ad7280_write_channel_config, > AD7280A_CELL_UNDERVOLTAGE); > > static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value, > - in_voltage-voltage_thresh_high_value, > + in_voltage - voltage_thresh_high_value, > 0644, > ad7280_read_channel_config, > ad7280_write_channel_config, -- Slawomir Stepien
Re: [PATCH] staging: iio: adc: ad7280a: fix 2 checks
On paź 06, 2018 13:27, Gabriel Capella wrote: > This patch does not change the logic, it only > corrects the checkpatch checks. > > The patch fixes 2 checks of type: > "CHECK: spaces preferred around that '-'" I've made the same mistake few days ago. This change is incorrect. Please see: https://lore.kernel.org/patchwork/patch/635994/. > Signed-off-by: Gabriel Capella > --- > drivers/staging/iio/adc/ad7280a.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7280a.c > b/drivers/staging/iio/adc/ad7280a.c > index 58420dcb406d..a4b4f8678c56 100644 > --- a/drivers/staging/iio/adc/ad7280a.c > +++ b/drivers/staging/iio/adc/ad7280a.c > @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void > *private) > } > > static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value, > - in_voltage-voltage_thresh_low_value, > + in_voltage - voltage_thresh_low_value, > 0644, > ad7280_read_channel_config, > ad7280_write_channel_config, > AD7280A_CELL_UNDERVOLTAGE); > > static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value, > - in_voltage-voltage_thresh_high_value, > + in_voltage - voltage_thresh_high_value, > 0644, > ad7280_read_channel_config, > ad7280_write_channel_config, -- Slawomir Stepien
Re: [PATCH v3 1/8] iio:core: add a callback to allow drivers to provide _available attributes
On Nov 07, 2016 12:57, Peter Rosin wrote: > On 2016-11-07 12:37, Daniel Baluta wrote: > > On Mon, Oct 24, 2016 at 1:39 AM, Peter Rosin <p...@axentia.se> wrote: > >> From: Jonathan Cameron <ji...@kernel.org> > >> > >> A large number of attributes can only take a limited range of values. > >> Currently in IIO this is handled by directly registering additional > >> *_available attributes thus providing this information to userspace. > >> > >> It is desirable to provide this information via the core for much the same > >> reason this was done for the actual channel information attributes in the > >> first place. If it isn't there, then it can only really be accessed from > >> userspace. Other in kernel IIO consumers have no access to what valid > >> parameters are. > >> > >> Two forms are currently supported: > >> * list of values in one particular IIO_VAL_* format. > >> e.g. 1.30 1.50 1.73 > >> * range specification with a step size: > >> e.g. [1.00 0.50 2.50] > >> equivalent to 1.00 1.500 2.00 2.50 > > > > Is there any driver using this format? :) > > Yes, soon. Hopefully. See patch 3/8 > iio: mcp4531: provide range of available raw values > https://patchwork.kernel.org/patch/9391283/ I would also like to add this to mcp4131.c and ds1803.c. -- Slawomir Stepien
Re: [PATCH v3 1/8] iio:core: add a callback to allow drivers to provide _available attributes
On Nov 07, 2016 12:57, Peter Rosin wrote: > On 2016-11-07 12:37, Daniel Baluta wrote: > > On Mon, Oct 24, 2016 at 1:39 AM, Peter Rosin wrote: > >> From: Jonathan Cameron > >> > >> A large number of attributes can only take a limited range of values. > >> Currently in IIO this is handled by directly registering additional > >> *_available attributes thus providing this information to userspace. > >> > >> It is desirable to provide this information via the core for much the same > >> reason this was done for the actual channel information attributes in the > >> first place. If it isn't there, then it can only really be accessed from > >> userspace. Other in kernel IIO consumers have no access to what valid > >> parameters are. > >> > >> Two forms are currently supported: > >> * list of values in one particular IIO_VAL_* format. > >> e.g. 1.30 1.50 1.73 > >> * range specification with a step size: > >> e.g. [1.00 0.50 2.50] > >> equivalent to 1.00 1.500 2.00 2.50 > > > > Is there any driver using this format? :) > > Yes, soon. Hopefully. See patch 3/8 > iio: mcp4531: provide range of available raw values > https://patchwork.kernel.org/patch/9391283/ I would also like to add this to mcp4131.c and ds1803.c. -- Slawomir Stepien
Re: [PATCH] Input: gpio-keys - use module_platform_driver macro
On Oct 27, 2016 09:48, Dmitry Torokhov wrote: > Hi Slawomir, Hi Dmitry, > > -late_initcall(gpio_keys_init); > ^ > > Because of this we can't switch to module_platform_driver(). The > late_initcall was requirement of one of platforms because of the probe > ordering issues. It may be resolved now with deferred probing, but you'd > need to confirm with all the users. I was unaware of that. Let me look in to that more deeply. At the moment I do not have the knowledge what will be to correct solution for that. > > -module_exit(gpio_keys_exit); > > +module_platform_driver(gpio_keys_device_driver); > > > > MODULE_LICENSE("GPL"); > > MODULE_AUTHOR("Phil Blundell <p...@handhelds.org>"); > > Thanks. Thank you. -- Slawomir Stepien
Re: [PATCH] Input: gpio-keys - use module_platform_driver macro
On Oct 27, 2016 09:48, Dmitry Torokhov wrote: > Hi Slawomir, Hi Dmitry, > > -late_initcall(gpio_keys_init); > ^ > > Because of this we can't switch to module_platform_driver(). The > late_initcall was requirement of one of platforms because of the probe > ordering issues. It may be resolved now with deferred probing, but you'd > need to confirm with all the users. I was unaware of that. Let me look in to that more deeply. At the moment I do not have the knowledge what will be to correct solution for that. > > -module_exit(gpio_keys_exit); > > +module_platform_driver(gpio_keys_device_driver); > > > > MODULE_LICENSE("GPL"); > > MODULE_AUTHOR("Phil Blundell "); > > Thanks. Thank you. -- Slawomir Stepien
[PATCH] Input: gpio-keys - use module_platform_driver macro
The gpio_keys_init() and gpio_keys_exit() are not doing anything more then just register and unregister. Replace these functions with module_platform_driver. Signed-off-by: Slawomir Stepien <s...@poczta.fm> --- drivers/input/keyboard/gpio_keys.c | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 2909365..e54b586 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -877,18 +877,7 @@ static struct platform_driver gpio_keys_device_driver = { } }; -static int __init gpio_keys_init(void) -{ - return platform_driver_register(_keys_device_driver); -} - -static void __exit gpio_keys_exit(void) -{ - platform_driver_unregister(_keys_device_driver); -} - -late_initcall(gpio_keys_init); -module_exit(gpio_keys_exit); +module_platform_driver(gpio_keys_device_driver); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Phil Blundell <p...@handhelds.org>"); -- 2.10.0
[PATCH] Input: gpio-keys - use module_platform_driver macro
The gpio_keys_init() and gpio_keys_exit() are not doing anything more then just register and unregister. Replace these functions with module_platform_driver. Signed-off-by: Slawomir Stepien --- drivers/input/keyboard/gpio_keys.c | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 2909365..e54b586 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -877,18 +877,7 @@ static struct platform_driver gpio_keys_device_driver = { } }; -static int __init gpio_keys_init(void) -{ - return platform_driver_register(_keys_device_driver); -} - -static void __exit gpio_keys_exit(void) -{ - platform_driver_unregister(_keys_device_driver); -} - -late_initcall(gpio_keys_init); -module_exit(gpio_keys_exit); +module_platform_driver(gpio_keys_device_driver); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Phil Blundell "); -- 2.10.0
[PATCH v3] iio: potentiometer: add driver for Maxim Integrated DS1803
The following functions are supported: - write, read potentiometer value - potentiometer scale Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf Signed-off-by: Slawomir Stepien <s...@poczta.fm> --- Changes since v2: - Use array of u8 as a result type for i2c_master_recv() call Changes since v1: - Removed unnecessary include file - Use i2c_master_recv() in place of i2c_smbus_read_word_swapped() - On raw write make sure val2 is zero - Use ARRAY_SIZE when possible .../bindings/iio/potentiometer/ds1803.txt | 21 +++ drivers/iio/potentiometer/Kconfig | 10 ++ drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/ds1803.c | 173 + 4 files changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt create mode 100644 drivers/iio/potentiometer/ds1803.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt new file mode 100644 index 000..df77bf5 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt @@ -0,0 +1,21 @@ +* Maxim Integrated DS1803 digital potentiometer driver + +The node for this driver must be a child node of a I2C controller, hence +all mandatory properties for your controller must be specified. See directory: + +Documentation/devicetree/bindings/i2c + +for more details. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "maxim,ds1803-010", + "maxim,ds1803-050", + "maxim,ds1803-100" + +Example: +ds1803: ds1803@1 { + reg = <0x28>; + compatible = "maxim,ds1803-010"; +}; diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig index 7ea069b..6acb238 100644 --- a/drivers/iio/potentiometer/Kconfig +++ b/drivers/iio/potentiometer/Kconfig @@ -5,6 +5,16 @@ menu "Digital potentiometers" +config DS1803 + tristate "Maxim Integrated DS1803 Digital Potentiometer driver" + depends on I2C + help + Say yes here to build support for the Maxim Integrated DS1803 + digital potentiomenter chip. + + To compile this driver as a module, choose M here: the + module will be called ds1803. + config MCP4131 tristate "Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer driver" depends on SPI diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile index 91a80f8..6007faa 100644 --- a/drivers/iio/potentiometer/Makefile +++ b/drivers/iio/potentiometer/Makefile @@ -3,6 +3,7 @@ # # When adding new entries keep the list in alphabetical order +obj-$(CONFIG_DS1803) += ds1803.o obj-$(CONFIG_MCP4131) += mcp4131.o obj-$(CONFIG_MCP4531) += mcp4531.o obj-$(CONFIG_TPL0102) += tpl0102.o diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c new file mode 100644 index 000..fb9e2a3 --- /dev/null +++ b/drivers/iio/potentiometer/ds1803.c @@ -0,0 +1,173 @@ +/* + * Maxim Integrated DS1803 digital potentiometer driver + * Copyright (c) 2016 Slawomir Stepien + * + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf + * + * DEVID #Wipers #Positions Resistor Opts (kOhm)i2c address + * ds1803 2 256 10, 50, 100 0101xxx + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include + +#define DS1803_MAX_POS 255 +#define DS1803_WRITE(chan) (0xa8 | ((chan) + 1)) + +enum ds1803_type { + DS1803_010, + DS1803_050, + DS1803_100, +}; + +struct ds1803_cfg { + int kohms; +}; + +static const struct ds1803_cfg ds1803_cfg[] = { + [DS1803_010] = { .kohms = 10, }, + [DS1803_050] = { .kohms = 50, }, + [DS1803_100] = { .kohms = 100, }, +}; + +struct ds1803_data { + struct i2c_client *client; + 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), \ +} + +static const struct iio_chan_spec ds1803_channels[] = { + DS1803_CHANNEL(0), + DS1803_CHANNEL(1), +}; + +static
[PATCH v3] iio: potentiometer: add driver for Maxim Integrated DS1803
The following functions are supported: - write, read potentiometer value - potentiometer scale Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf Signed-off-by: Slawomir Stepien --- Changes since v2: - Use array of u8 as a result type for i2c_master_recv() call Changes since v1: - Removed unnecessary include file - Use i2c_master_recv() in place of i2c_smbus_read_word_swapped() - On raw write make sure val2 is zero - Use ARRAY_SIZE when possible .../bindings/iio/potentiometer/ds1803.txt | 21 +++ drivers/iio/potentiometer/Kconfig | 10 ++ drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/ds1803.c | 173 + 4 files changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt create mode 100644 drivers/iio/potentiometer/ds1803.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt new file mode 100644 index 000..df77bf5 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt @@ -0,0 +1,21 @@ +* Maxim Integrated DS1803 digital potentiometer driver + +The node for this driver must be a child node of a I2C controller, hence +all mandatory properties for your controller must be specified. See directory: + +Documentation/devicetree/bindings/i2c + +for more details. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "maxim,ds1803-010", + "maxim,ds1803-050", + "maxim,ds1803-100" + +Example: +ds1803: ds1803@1 { + reg = <0x28>; + compatible = "maxim,ds1803-010"; +}; diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig index 7ea069b..6acb238 100644 --- a/drivers/iio/potentiometer/Kconfig +++ b/drivers/iio/potentiometer/Kconfig @@ -5,6 +5,16 @@ menu "Digital potentiometers" +config DS1803 + tristate "Maxim Integrated DS1803 Digital Potentiometer driver" + depends on I2C + help + Say yes here to build support for the Maxim Integrated DS1803 + digital potentiomenter chip. + + To compile this driver as a module, choose M here: the + module will be called ds1803. + config MCP4131 tristate "Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer driver" depends on SPI diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile index 91a80f8..6007faa 100644 --- a/drivers/iio/potentiometer/Makefile +++ b/drivers/iio/potentiometer/Makefile @@ -3,6 +3,7 @@ # # When adding new entries keep the list in alphabetical order +obj-$(CONFIG_DS1803) += ds1803.o obj-$(CONFIG_MCP4131) += mcp4131.o obj-$(CONFIG_MCP4531) += mcp4531.o obj-$(CONFIG_TPL0102) += tpl0102.o diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c new file mode 100644 index 000..fb9e2a3 --- /dev/null +++ b/drivers/iio/potentiometer/ds1803.c @@ -0,0 +1,173 @@ +/* + * Maxim Integrated DS1803 digital potentiometer driver + * Copyright (c) 2016 Slawomir Stepien + * + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf + * + * DEVID #Wipers #Positions Resistor Opts (kOhm)i2c address + * ds1803 2 256 10, 50, 100 0101xxx + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include + +#define DS1803_MAX_POS 255 +#define DS1803_WRITE(chan) (0xa8 | ((chan) + 1)) + +enum ds1803_type { + DS1803_010, + DS1803_050, + DS1803_100, +}; + +struct ds1803_cfg { + int kohms; +}; + +static const struct ds1803_cfg ds1803_cfg[] = { + [DS1803_010] = { .kohms = 10, }, + [DS1803_050] = { .kohms = 50, }, + [DS1803_100] = { .kohms = 100, }, +}; + +struct ds1803_data { + struct i2c_client *client; + 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), \ +} + +static const struct iio_chan_spec ds1803_channels[] = { + DS1803_CHANNEL(0), + DS1803_CHANNEL(1), +}; + +static int ds1803_read_raw(struct iio_
Re: [PATCH v2] iio: potentiometer: add driver for Maxim Integrated DS1803
On Apr 08, 2016 23:27, Slawomir Stepien wrote: > +static int ds1803_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ds1803_data *data = iio_priv(indio_dev); > + int pot = chan->channel; > + int ret; > + u16 result; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = i2c_master_recv(data->client, (char *), > + indio_dev->num_channels); > + if (ret < 0) > + return ret; > + > + /* Get bits for given pot */ > + *val = (pot == 0 ? result & 0xff : result >> 8); > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + *val = 1000 * data->cfg->kohms; > + *val2 = DS1803_MAX_POS; > + return IIO_VAL_FRACTIONAL; > + } > + > + return -EINVAL; > +} Or maybe this is more clean? static int ds1803_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) { struct ds1803_data *data = iio_priv(indio_dev); int pot = chan->channel; int ret; unsigned char result[indio_dev->num_channels]; 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; case IIO_CHAN_INFO_SCALE: *val = 1000 * data->cfg->kohms; *val2 = DS1803_MAX_POS; return IIO_VAL_FRACTIONAL; } return -EINVAL; } What do you think? -- Slawomir Stepien
Re: [PATCH v2] iio: potentiometer: add driver for Maxim Integrated DS1803
On Apr 08, 2016 23:27, Slawomir Stepien wrote: > +static int ds1803_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ds1803_data *data = iio_priv(indio_dev); > + int pot = chan->channel; > + int ret; > + u16 result; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = i2c_master_recv(data->client, (char *), > + indio_dev->num_channels); > + if (ret < 0) > + return ret; > + > + /* Get bits for given pot */ > + *val = (pot == 0 ? result & 0xff : result >> 8); > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + *val = 1000 * data->cfg->kohms; > + *val2 = DS1803_MAX_POS; > + return IIO_VAL_FRACTIONAL; > + } > + > + return -EINVAL; > +} Or maybe this is more clean? static int ds1803_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) { struct ds1803_data *data = iio_priv(indio_dev); int pot = chan->channel; int ret; unsigned char result[indio_dev->num_channels]; 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; case IIO_CHAN_INFO_SCALE: *val = 1000 * data->cfg->kohms; *val2 = DS1803_MAX_POS; return IIO_VAL_FRACTIONAL; } return -EINVAL; } What do you think? -- Slawomir Stepien
[PATCH v2] iio: potentiometer: add driver for Maxim Integrated DS1803
The following functions are supported: - write, read potentiometer value - potentiometer scale Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf Signed-off-by: Slawomir Stepien <s...@poczta.fm> --- Changes since v1: - Removed unnecessary include file - Use i2c_master_recv() in place of i2c_smbus_read_word_swapped() - On raw write make sure val2 is zero - Use ARRAY_SIZE when possible .../bindings/iio/potentiometer/ds1803.txt | 21 +++ drivers/iio/potentiometer/Kconfig | 10 ++ drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/ds1803.c | 174 + 4 files changed, 206 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt create mode 100644 drivers/iio/potentiometer/ds1803.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt new file mode 100644 index 000..df77bf5 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt @@ -0,0 +1,21 @@ +* Maxim Integrated DS1803 digital potentiometer driver + +The node for this driver must be a child node of a I2C controller, hence +all mandatory properties for your controller must be specified. See directory: + +Documentation/devicetree/bindings/i2c + +for more details. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "maxim,ds1803-010", + "maxim,ds1803-050", + "maxim,ds1803-100" + +Example: +ds1803: ds1803@1 { + reg = <0x28>; + compatible = "maxim,ds1803-010"; +}; diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig index 7ea069b..6acb238 100644 --- a/drivers/iio/potentiometer/Kconfig +++ b/drivers/iio/potentiometer/Kconfig @@ -5,6 +5,16 @@ menu "Digital potentiometers" +config DS1803 + tristate "Maxim Integrated DS1803 Digital Potentiometer driver" + depends on I2C + help + Say yes here to build support for the Maxim Integrated DS1803 + digital potentiomenter chip. + + To compile this driver as a module, choose M here: the + module will be called ds1803. + config MCP4131 tristate "Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer driver" depends on SPI diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile index 91a80f8..6007faa 100644 --- a/drivers/iio/potentiometer/Makefile +++ b/drivers/iio/potentiometer/Makefile @@ -3,6 +3,7 @@ # # When adding new entries keep the list in alphabetical order +obj-$(CONFIG_DS1803) += ds1803.o obj-$(CONFIG_MCP4131) += mcp4131.o obj-$(CONFIG_MCP4531) += mcp4531.o obj-$(CONFIG_TPL0102) += tpl0102.o diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c new file mode 100644 index 000..cfa4cc1 --- /dev/null +++ b/drivers/iio/potentiometer/ds1803.c @@ -0,0 +1,174 @@ +/* + * Maxim Integrated DS1803 digital potentiometer driver + * Copyright (c) 2016 Slawomir Stepien + * + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf + * + * DEVID #Wipers #Positions Resistor Opts (kOhm)i2c address + * ds1803 2 256 10, 50, 100 0101xxx + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include + +#define DS1803_MAX_POS 255 +#define DS1803_WRITE(chan) (0xa8 | ((chan) + 1)) + +enum ds1803_type { + DS1803_010, + DS1803_050, + DS1803_100, +}; + +struct ds1803_cfg { + int kohms; +}; + +static const struct ds1803_cfg ds1803_cfg[] = { + [DS1803_010] = { .kohms = 10, }, + [DS1803_050] = { .kohms = 50, }, + [DS1803_100] = { .kohms = 100, }, +}; + +struct ds1803_data { + struct i2c_client *client; + 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), \ +} + +static const struct iio_chan_spec ds1803_channels[] = { + DS1803_CHANNEL(0), + DS1803_CHANNEL(1), +}; + +static int ds1803_read_raw(struct iio_dev *indio_dev, + struct iio
[PATCH v2] iio: potentiometer: add driver for Maxim Integrated DS1803
The following functions are supported: - write, read potentiometer value - potentiometer scale Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf Signed-off-by: Slawomir Stepien --- Changes since v1: - Removed unnecessary include file - Use i2c_master_recv() in place of i2c_smbus_read_word_swapped() - On raw write make sure val2 is zero - Use ARRAY_SIZE when possible .../bindings/iio/potentiometer/ds1803.txt | 21 +++ drivers/iio/potentiometer/Kconfig | 10 ++ drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/ds1803.c | 174 + 4 files changed, 206 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt create mode 100644 drivers/iio/potentiometer/ds1803.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt new file mode 100644 index 000..df77bf5 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt @@ -0,0 +1,21 @@ +* Maxim Integrated DS1803 digital potentiometer driver + +The node for this driver must be a child node of a I2C controller, hence +all mandatory properties for your controller must be specified. See directory: + +Documentation/devicetree/bindings/i2c + +for more details. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "maxim,ds1803-010", + "maxim,ds1803-050", + "maxim,ds1803-100" + +Example: +ds1803: ds1803@1 { + reg = <0x28>; + compatible = "maxim,ds1803-010"; +}; diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig index 7ea069b..6acb238 100644 --- a/drivers/iio/potentiometer/Kconfig +++ b/drivers/iio/potentiometer/Kconfig @@ -5,6 +5,16 @@ menu "Digital potentiometers" +config DS1803 + tristate "Maxim Integrated DS1803 Digital Potentiometer driver" + depends on I2C + help + Say yes here to build support for the Maxim Integrated DS1803 + digital potentiomenter chip. + + To compile this driver as a module, choose M here: the + module will be called ds1803. + config MCP4131 tristate "Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer driver" depends on SPI diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile index 91a80f8..6007faa 100644 --- a/drivers/iio/potentiometer/Makefile +++ b/drivers/iio/potentiometer/Makefile @@ -3,6 +3,7 @@ # # When adding new entries keep the list in alphabetical order +obj-$(CONFIG_DS1803) += ds1803.o obj-$(CONFIG_MCP4131) += mcp4131.o obj-$(CONFIG_MCP4531) += mcp4531.o obj-$(CONFIG_TPL0102) += tpl0102.o diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c new file mode 100644 index 000..cfa4cc1 --- /dev/null +++ b/drivers/iio/potentiometer/ds1803.c @@ -0,0 +1,174 @@ +/* + * Maxim Integrated DS1803 digital potentiometer driver + * Copyright (c) 2016 Slawomir Stepien + * + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf + * + * DEVID #Wipers #Positions Resistor Opts (kOhm)i2c address + * ds1803 2 256 10, 50, 100 0101xxx + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include + +#define DS1803_MAX_POS 255 +#define DS1803_WRITE(chan) (0xa8 | ((chan) + 1)) + +enum ds1803_type { + DS1803_010, + DS1803_050, + DS1803_100, +}; + +struct ds1803_cfg { + int kohms; +}; + +static const struct ds1803_cfg ds1803_cfg[] = { + [DS1803_010] = { .kohms = 10, }, + [DS1803_050] = { .kohms = 50, }, + [DS1803_100] = { .kohms = 100, }, +}; + +struct ds1803_data { + struct i2c_client *client; + 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), \ +} + +static const struct iio_chan_spec ds1803_channels[] = { + DS1803_CHANNEL(0), + DS1803_CHANNEL(1), +}; + +static int ds1803_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, +
Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1803
On Apr 08, 2016 18:00, Peter Meerwald-Stadler wrote: > > > > maybe a #define to explain the name of register 0 > > > > Well this zero is not register nor any kind of command that DS1803 needs to > > send > > back pots values. > > DS1803 only requires the standard R/W bit to be set as read. > > I used _word_ function series to get back a word (16 bits) as this poti > > returns > > 16 bits. > > > > How should I named it? > > > > #define NON_COMMAND 0 > > ? > > maybe i2c_transfer() is more appropriate then > > u8 databuf[2]; > struct i2c_msg msgs[2] = { > { .addr = client->addr, .len = sizeof(databuf), .buf = > databuf, > .flags = I2C_M_RD } }; > ret = i2c_transfer(client->adapter, msgs, 2); > > this does not send any data to the chip but only reads two bytes Good point. But maybe the better solution would be to use i2c_master_recv? Anyhow I will check both options ;) > > Or should I use different function? (2x i2c_smbus_read_byte?) > > > > The i2c_smbus_read_byte() function also used 0 as command > > for its transfers... > > > > > +static int ds1803_write_raw(struct iio_dev *indio_dev, > > > > +struct iio_chan_spec const *chan, > > > > +int val, int val2, long mask) > > > > +{ > > > > + struct ds1803_data *data = iio_priv(indio_dev); > > > > + int pot = chan->channel; > > > > + > > > > + switch (mask) { > > > > + case IIO_CHAN_INFO_RAW: > > > > + if (val > DS1803_MAX_POS || val < 0) > > > > > > check that val2 is 0 or use .write_raw_get_fmt > > > > At this point I do not know why should I do it, but I will look into that. > > write_raw expects a VAL_INT_PLUS_MICROS per default, passed > in val and val2 > > you can change that with .write_raw_get_fmt (e.g. to expect VAL_INT), > or just make sure that the micros are 0 Thank you for the explanation. -- Slawomir Stepien
Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1803
On Apr 08, 2016 18:00, Peter Meerwald-Stadler wrote: > > > > maybe a #define to explain the name of register 0 > > > > Well this zero is not register nor any kind of command that DS1803 needs to > > send > > back pots values. > > DS1803 only requires the standard R/W bit to be set as read. > > I used _word_ function series to get back a word (16 bits) as this poti > > returns > > 16 bits. > > > > How should I named it? > > > > #define NON_COMMAND 0 > > ? > > maybe i2c_transfer() is more appropriate then > > u8 databuf[2]; > struct i2c_msg msgs[2] = { > { .addr = client->addr, .len = sizeof(databuf), .buf = > databuf, > .flags = I2C_M_RD } }; > ret = i2c_transfer(client->adapter, msgs, 2); > > this does not send any data to the chip but only reads two bytes Good point. But maybe the better solution would be to use i2c_master_recv? Anyhow I will check both options ;) > > Or should I use different function? (2x i2c_smbus_read_byte?) > > > > The i2c_smbus_read_byte() function also used 0 as command > > for its transfers... > > > > > +static int ds1803_write_raw(struct iio_dev *indio_dev, > > > > +struct iio_chan_spec const *chan, > > > > +int val, int val2, long mask) > > > > +{ > > > > + struct ds1803_data *data = iio_priv(indio_dev); > > > > + int pot = chan->channel; > > > > + > > > > + switch (mask) { > > > > + case IIO_CHAN_INFO_RAW: > > > > + if (val > DS1803_MAX_POS || val < 0) > > > > > > check that val2 is 0 or use .write_raw_get_fmt > > > > At this point I do not know why should I do it, but I will look into that. > > write_raw expects a VAL_INT_PLUS_MICROS per default, passed > in val and val2 > > you can change that with .write_raw_get_fmt (e.g. to expect VAL_INT), > or just make sure that the micros are 0 Thank you for the explanation. -- Slawomir Stepien
Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1803
On Apr 07, 2016 21:36, Peter Meerwald-Stadler wrote: > > > The following functions are supported: > > - write, read potentiometer value > > - potentiometer scale > > minor comments below, nice driver Thank you for your comments! > > +#include > > for what is cache.h needed? It is not needed here. I will delete it in v2. > > +static int ds1803_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct ds1803_data *data = iio_priv(indio_dev); > > + int pot = chan->channel; > > + s32 ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + ret = i2c_smbus_read_word_swapped(data->client, 0); > > maybe a #define to explain the name of register 0 Well this zero is not register nor any kind of command that DS1803 needs to send back pots values. DS1803 only requires the standard R/W bit to be set as read. I used _word_ function series to get back a word (16 bits) as this poti returns 16 bits. How should I named it? #define NON_COMMAND 0 ? Or should I use different function? (2x i2c_smbus_read_byte?) The i2c_smbus_read_byte() function also used 0 as command for its transfers... > > + if (ret < 0) > > + return ret; > > + > > + /* Get bits for given pot */ > > + *val = (pot == 0 ? ret >> 8 : ret & 255); > > often 0xff is used to mask a byte OK. > > + return IIO_VAL_INT; > > + > > + case IIO_CHAN_INFO_SCALE: > > + *val = 1000 * data->cfg->kohms; > > + *val2 = DS1803_MAX_POS; > > + return IIO_VAL_FRACTIONAL; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int ds1803_write_raw(struct iio_dev *indio_dev, > > +struct iio_chan_spec const *chan, > > +int val, int val2, long mask) > > +{ > > + struct ds1803_data *data = iio_priv(indio_dev); > > + int pot = chan->channel; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (val > DS1803_MAX_POS || val < 0) > > check that val2 is 0 or use .write_raw_get_fmt At this point I do not know why should I do it, but I will look into that. > > + indio_dev->dev.parent = dev; > > + indio_dev->info = _info; > > + indio_dev->channels = ds1803_channels; > > + indio_dev->num_channels = DS1803_NUM_WIPERS; > > ARRAY_SIZE(ds1803_channels) Good point. -- Slawomir Stepien
Re: [PATCH] iio: potentiometer: add driver for Maxim Integrated DS1803
On Apr 07, 2016 21:36, Peter Meerwald-Stadler wrote: > > > The following functions are supported: > > - write, read potentiometer value > > - potentiometer scale > > minor comments below, nice driver Thank you for your comments! > > +#include > > for what is cache.h needed? It is not needed here. I will delete it in v2. > > +static int ds1803_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct ds1803_data *data = iio_priv(indio_dev); > > + int pot = chan->channel; > > + s32 ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + ret = i2c_smbus_read_word_swapped(data->client, 0); > > maybe a #define to explain the name of register 0 Well this zero is not register nor any kind of command that DS1803 needs to send back pots values. DS1803 only requires the standard R/W bit to be set as read. I used _word_ function series to get back a word (16 bits) as this poti returns 16 bits. How should I named it? #define NON_COMMAND 0 ? Or should I use different function? (2x i2c_smbus_read_byte?) The i2c_smbus_read_byte() function also used 0 as command for its transfers... > > + if (ret < 0) > > + return ret; > > + > > + /* Get bits for given pot */ > > + *val = (pot == 0 ? ret >> 8 : ret & 255); > > often 0xff is used to mask a byte OK. > > + return IIO_VAL_INT; > > + > > + case IIO_CHAN_INFO_SCALE: > > + *val = 1000 * data->cfg->kohms; > > + *val2 = DS1803_MAX_POS; > > + return IIO_VAL_FRACTIONAL; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int ds1803_write_raw(struct iio_dev *indio_dev, > > +struct iio_chan_spec const *chan, > > +int val, int val2, long mask) > > +{ > > + struct ds1803_data *data = iio_priv(indio_dev); > > + int pot = chan->channel; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (val > DS1803_MAX_POS || val < 0) > > check that val2 is 0 or use .write_raw_get_fmt At this point I do not know why should I do it, but I will look into that. > > + indio_dev->dev.parent = dev; > > + indio_dev->info = _info; > > + indio_dev->channels = ds1803_channels; > > + indio_dev->num_channels = DS1803_NUM_WIPERS; > > ARRAY_SIZE(ds1803_channels) Good point. -- Slawomir Stepien
[PATCH] iio: potentiometer: add driver for Maxim Integrated DS1803
The following functions are supported: - write, read potentiometer value - potentiometer scale Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf Signed-off-by: Slawomir Stepien <s...@poczta.fm> --- .../bindings/iio/potentiometer/ds1803.txt | 21 +++ drivers/iio/potentiometer/Kconfig | 10 ++ drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/ds1803.c | 178 + 4 files changed, 210 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt create mode 100644 drivers/iio/potentiometer/ds1803.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt new file mode 100644 index 000..df77bf5 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt @@ -0,0 +1,21 @@ +* Maxim Integrated DS1803 digital potentiometer driver + +The node for this driver must be a child node of a I2C controller, hence +all mandatory properties for your controller must be specified. See directory: + +Documentation/devicetree/bindings/i2c + +for more details. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "maxim,ds1803-010", + "maxim,ds1803-050", + "maxim,ds1803-100" + +Example: +ds1803: ds1803@1 { + reg = <0x28>; + compatible = "maxim,ds1803-010"; +}; diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig index 7ea069b..6acb238 100644 --- a/drivers/iio/potentiometer/Kconfig +++ b/drivers/iio/potentiometer/Kconfig @@ -5,6 +5,16 @@ menu "Digital potentiometers" +config DS1803 + tristate "Maxim Integrated DS1803 Digital Potentiometer driver" + depends on I2C + help + Say yes here to build support for the Maxim Integrated DS1803 + digital potentiomenter chip. + + To compile this driver as a module, choose M here: the + module will be called ds1803. + config MCP4131 tristate "Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer driver" depends on SPI diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile index 91a80f8..6007faa 100644 --- a/drivers/iio/potentiometer/Makefile +++ b/drivers/iio/potentiometer/Makefile @@ -3,6 +3,7 @@ # # When adding new entries keep the list in alphabetical order +obj-$(CONFIG_DS1803) += ds1803.o obj-$(CONFIG_MCP4131) += mcp4131.o obj-$(CONFIG_MCP4531) += mcp4531.o obj-$(CONFIG_TPL0102) += tpl0102.o diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c new file mode 100644 index 000..b5e5b1c --- /dev/null +++ b/drivers/iio/potentiometer/ds1803.c @@ -0,0 +1,178 @@ +/* + * Maxim Integrated DS1803 digital potentiometer driver + * Copyright (c) 2016 Slawomir Stepien + * + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf + * + * DEVID #Wipers #Positions Resistor Opts (kOhm)i2c address + * ds1803 2 256 10, 50, 100 0101xxx + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include +#include + +#define DS1803_NUM_WIPERS 2 +#define DS1803_MAX_POS 255 +#define DS1803_WRITE(n)(0xA8 | ((n) + 1)) + +enum ds1803_type { + DS1803_010, + DS1803_050, + DS1803_100, +}; + +struct ds1803_cfg { + int kohms; +}; + +static const struct ds1803_cfg ds1803_cfg[] = { + [DS1803_010] = { .kohms = 10, }, + [DS1803_050] = { .kohms = 50, }, + [DS1803_100] = { .kohms = 100, }, +}; + +struct ds1803_data { + struct i2c_client *client; + 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), \ +} + +static const struct iio_chan_spec ds1803_channels[] = { + DS1803_CHANNEL(0), + DS1803_CHANNEL(1), +}; + +static int ds1803_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct ds1803_data *data = iio_priv(in
[PATCH] iio: potentiometer: add driver for Maxim Integrated DS1803
The following functions are supported: - write, read potentiometer value - potentiometer scale Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf Signed-off-by: Slawomir Stepien --- .../bindings/iio/potentiometer/ds1803.txt | 21 +++ drivers/iio/potentiometer/Kconfig | 10 ++ drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/ds1803.c | 178 + 4 files changed, 210 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt create mode 100644 drivers/iio/potentiometer/ds1803.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt new file mode 100644 index 000..df77bf5 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/ds1803.txt @@ -0,0 +1,21 @@ +* Maxim Integrated DS1803 digital potentiometer driver + +The node for this driver must be a child node of a I2C controller, hence +all mandatory properties for your controller must be specified. See directory: + +Documentation/devicetree/bindings/i2c + +for more details. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "maxim,ds1803-010", + "maxim,ds1803-050", + "maxim,ds1803-100" + +Example: +ds1803: ds1803@1 { + reg = <0x28>; + compatible = "maxim,ds1803-010"; +}; diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig index 7ea069b..6acb238 100644 --- a/drivers/iio/potentiometer/Kconfig +++ b/drivers/iio/potentiometer/Kconfig @@ -5,6 +5,16 @@ menu "Digital potentiometers" +config DS1803 + tristate "Maxim Integrated DS1803 Digital Potentiometer driver" + depends on I2C + help + Say yes here to build support for the Maxim Integrated DS1803 + digital potentiomenter chip. + + To compile this driver as a module, choose M here: the + module will be called ds1803. + config MCP4131 tristate "Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer driver" depends on SPI diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile index 91a80f8..6007faa 100644 --- a/drivers/iio/potentiometer/Makefile +++ b/drivers/iio/potentiometer/Makefile @@ -3,6 +3,7 @@ # # When adding new entries keep the list in alphabetical order +obj-$(CONFIG_DS1803) += ds1803.o obj-$(CONFIG_MCP4131) += mcp4131.o obj-$(CONFIG_MCP4531) += mcp4531.o obj-$(CONFIG_TPL0102) += tpl0102.o diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c new file mode 100644 index 000..b5e5b1c --- /dev/null +++ b/drivers/iio/potentiometer/ds1803.c @@ -0,0 +1,178 @@ +/* + * Maxim Integrated DS1803 digital potentiometer driver + * Copyright (c) 2016 Slawomir Stepien + * + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf + * + * DEVID #Wipers #Positions Resistor Opts (kOhm)i2c address + * ds1803 2 256 10, 50, 100 0101xxx + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include +#include + +#define DS1803_NUM_WIPERS 2 +#define DS1803_MAX_POS 255 +#define DS1803_WRITE(n)(0xA8 | ((n) + 1)) + +enum ds1803_type { + DS1803_010, + DS1803_050, + DS1803_100, +}; + +struct ds1803_cfg { + int kohms; +}; + +static const struct ds1803_cfg ds1803_cfg[] = { + [DS1803_010] = { .kohms = 10, }, + [DS1803_050] = { .kohms = 50, }, + [DS1803_100] = { .kohms = 100, }, +}; + +struct ds1803_data { + struct i2c_client *client; + 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), \ +} + +static const struct iio_chan_spec ds1803_channels[] = { + DS1803_CHANNEL(0), + DS1803_CHANNEL(1), +}; + +static int ds1803_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct ds1803_data *data = iio_priv(indio_dev); + int
Re: [PATCH v5] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 28, 2016 15:58, Jonathan Cameron wrote: > On 23/03/16 08:57, Slawomir Stepien wrote: > > The following functionalities are supported: > > - write, read from volatile memory > > > > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf > > > > Signed-off-by: Slawomir Stepien <s...@poczta.fm> > One process comment below... And git actually copes fine with what you > did (I wondered which way it would go until I tried it :) I did try it on my tree with git and it works so I decided to do it like that. I'll drop the additional --- next time. > Applied to the togreg branch of iio.git - initially pushed out as > testing for the autobuilders to play pingpong with it. > > A very nice, clean driver. Thanks for your hard work on this. > > Note it is in a branch I happy to rebase for the next week, so if Joachim > in particular would like to add a reviewed by tag, I'd be delighted to append > it. Often reviewers don't get enough credit in my opinion! Thank you all for the support and really good comments on this patch. I've learned a lot! > > --- > > Changes since v4: > > - Added necessary include files and sorted all includes > > - Added missing mutex unlock when there is CMDERR > > - mcp4131_write_raw spi_write/mutex_unlock/return refactor > > > > Changes since v3: > > - Added 'potentiometer' to subject > > - Replaced mcp4131_exec with spi_write and mcp4131_read > > - Narrowed mutex locking and unlocking places > > > > Changes since v2: > > - Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro > > - Replaced the rx and tx SPI buffers with one buf that is > > cacheline_aligned > > - Changed mutex locking position > > - Replaced the devid with pointer to model configuration > > - Removed positive ("Registered" & "Unregistered") dev_info prints > > - Removed the mcp4131_remove callback > > > > Changes since v1: > > - One line summary changed > > - Fixed module name and OF compatible > > - Based code on Peter Rosin's code from mcp4531.c > > - No additional sysfs attributes > > - Deleted not used devid struct memeber > > - Changed mcp4131_exec function arguments: > > - write value is now u16 > > - no need to pass return buffer array - rx array from mcp4131_data is used > > - Added missing new line characters to dev_info > Funnily enough, this is the only thing I can find wrong in this patch. > Should not introduce an additional --- as you have done here. > The version stuff should simply have a blank line between it and the stats ;) How about the MAINTAINER file? I wasn't sure if I should add myself as the M of this driver... -- Slawomir Stepien
Re: [PATCH v5] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 28, 2016 15:58, Jonathan Cameron wrote: > On 23/03/16 08:57, Slawomir Stepien wrote: > > The following functionalities are supported: > > - write, read from volatile memory > > > > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf > > > > Signed-off-by: Slawomir Stepien > One process comment below... And git actually copes fine with what you > did (I wondered which way it would go until I tried it :) I did try it on my tree with git and it works so I decided to do it like that. I'll drop the additional --- next time. > Applied to the togreg branch of iio.git - initially pushed out as > testing for the autobuilders to play pingpong with it. > > A very nice, clean driver. Thanks for your hard work on this. > > Note it is in a branch I happy to rebase for the next week, so if Joachim > in particular would like to add a reviewed by tag, I'd be delighted to append > it. Often reviewers don't get enough credit in my opinion! Thank you all for the support and really good comments on this patch. I've learned a lot! > > --- > > Changes since v4: > > - Added necessary include files and sorted all includes > > - Added missing mutex unlock when there is CMDERR > > - mcp4131_write_raw spi_write/mutex_unlock/return refactor > > > > Changes since v3: > > - Added 'potentiometer' to subject > > - Replaced mcp4131_exec with spi_write and mcp4131_read > > - Narrowed mutex locking and unlocking places > > > > Changes since v2: > > - Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro > > - Replaced the rx and tx SPI buffers with one buf that is > > cacheline_aligned > > - Changed mutex locking position > > - Replaced the devid with pointer to model configuration > > - Removed positive ("Registered" & "Unregistered") dev_info prints > > - Removed the mcp4131_remove callback > > > > Changes since v1: > > - One line summary changed > > - Fixed module name and OF compatible > > - Based code on Peter Rosin's code from mcp4531.c > > - No additional sysfs attributes > > - Deleted not used devid struct memeber > > - Changed mcp4131_exec function arguments: > > - write value is now u16 > > - no need to pass return buffer array - rx array from mcp4131_data is used > > - Added missing new line characters to dev_info > Funnily enough, this is the only thing I can find wrong in this patch. > Should not introduce an additional --- as you have done here. > The version stuff should simply have a blank line between it and the stats ;) How about the MAINTAINER file? I wasn't sure if I should add myself as the M of this driver... -- Slawomir Stepien
[PATCH v5] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
The following functionalities are supported: - write, read from volatile memory Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf Signed-off-by: Slawomir Stepien <s...@poczta.fm> --- Changes since v4: - Added necessary include files and sorted all includes - Added missing mutex unlock when there is CMDERR - mcp4131_write_raw spi_write/mutex_unlock/return refactor Changes since v3: - Added 'potentiometer' to subject - Replaced mcp4131_exec with spi_write and mcp4131_read - Narrowed mutex locking and unlocking places Changes since v2: - Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro - Replaced the rx and tx SPI buffers with one buf that is cacheline_aligned - Changed mutex locking position - Replaced the devid with pointer to model configuration - Removed positive ("Registered" & "Unregistered") dev_info prints - Removed the mcp4131_remove callback Changes since v1: - One line summary changed - Fixed module name and OF compatible - Based code on Peter Rosin's code from mcp4531.c - No additional sysfs attributes - Deleted not used devid struct memeber - Changed mcp4131_exec function arguments: - write value is now u16 - no need to pass return buffer array - rx array from mcp4131_data is used - Added missing new line characters to dev_info --- .../bindings/iio/potentiometer/mcp4131.txt | 84 drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp4131.c| 494 + 4 files changed, 597 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt create mode 100644 drivers/iio/potentiometer/mcp4131.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt new file mode 100644 index 000..3ccba16 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt @@ -0,0 +1,84 @@ +* Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer + driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4131-502" + "microchip,mcp4131-103" + "microchip,mcp4131-503" + "microchip,mcp4131-104" + "microchip,mcp4132-502" + "microchip,mcp4132-103" + "microchip,mcp4132-503" + "microchip,mcp4132-104" + "microchip,mcp4141-502" + "microchip,mcp4141-103" + "microchip,mcp4141-503" + "microchip,mcp4141-104" + "microchip,mcp4142-502" + "microchip,mcp4142-103" + "microchip,mcp4142-503" + "microchip,mcp4142-104" + "microchip,mcp4151-502" + "microchip,mcp4151-103" + "microchip,mcp4151-503" + "microchip,mcp4151-104" + "microchip,mcp4152-502" + "microchip,mcp4152-103" + "microchip,mcp4152-503" + "microchip,mcp4152-104" + "microchip,mcp4161-502" + "microchip,mcp4161-103" + "microchip,mcp4161-503" + "microchip,mcp4161-104" + "microchip,mcp4162-502" + "microchip,mcp4162-103" + "microchip,mcp4162-503" + "microchip,mcp4162-104" + "microchip,mcp4231-502" + "microchip,mcp4231-103" + "microchip,mcp4231-503" + "microchip,mcp4231-104" + "microchip,mcp4232-502" + "microchip,mcp4232-103" + "microchip,mcp4232-503" + "microchip,mcp4232-104" + "microchip,mcp4241-502" + "microchip,mcp4241-103" + "microchip,mcp4241-503" +
[PATCH v5] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
The following functionalities are supported: - write, read from volatile memory Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf Signed-off-by: Slawomir Stepien --- Changes since v4: - Added necessary include files and sorted all includes - Added missing mutex unlock when there is CMDERR - mcp4131_write_raw spi_write/mutex_unlock/return refactor Changes since v3: - Added 'potentiometer' to subject - Replaced mcp4131_exec with spi_write and mcp4131_read - Narrowed mutex locking and unlocking places Changes since v2: - Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro - Replaced the rx and tx SPI buffers with one buf that is cacheline_aligned - Changed mutex locking position - Replaced the devid with pointer to model configuration - Removed positive ("Registered" & "Unregistered") dev_info prints - Removed the mcp4131_remove callback Changes since v1: - One line summary changed - Fixed module name and OF compatible - Based code on Peter Rosin's code from mcp4531.c - No additional sysfs attributes - Deleted not used devid struct memeber - Changed mcp4131_exec function arguments: - write value is now u16 - no need to pass return buffer array - rx array from mcp4131_data is used - Added missing new line characters to dev_info --- .../bindings/iio/potentiometer/mcp4131.txt | 84 drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp4131.c| 494 + 4 files changed, 597 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt create mode 100644 drivers/iio/potentiometer/mcp4131.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt new file mode 100644 index 000..3ccba16 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt @@ -0,0 +1,84 @@ +* Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer + driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4131-502" + "microchip,mcp4131-103" + "microchip,mcp4131-503" + "microchip,mcp4131-104" + "microchip,mcp4132-502" + "microchip,mcp4132-103" + "microchip,mcp4132-503" + "microchip,mcp4132-104" + "microchip,mcp4141-502" + "microchip,mcp4141-103" + "microchip,mcp4141-503" + "microchip,mcp4141-104" + "microchip,mcp4142-502" + "microchip,mcp4142-103" + "microchip,mcp4142-503" + "microchip,mcp4142-104" + "microchip,mcp4151-502" + "microchip,mcp4151-103" + "microchip,mcp4151-503" + "microchip,mcp4151-104" + "microchip,mcp4152-502" + "microchip,mcp4152-103" + "microchip,mcp4152-503" + "microchip,mcp4152-104" + "microchip,mcp4161-502" + "microchip,mcp4161-103" + "microchip,mcp4161-503" + "microchip,mcp4161-104" + "microchip,mcp4162-502" + "microchip,mcp4162-103" + "microchip,mcp4162-503" + "microchip,mcp4162-104" + "microchip,mcp4231-502" + "microchip,mcp4231-103" + "microchip,mcp4231-503" + "microchip,mcp4231-104" + "microchip,mcp4232-502" + "microchip,mcp4232-103" + "microchip,mcp4232-503" + "microchip,mcp4232-104" + "microchip,mcp4241-502" + "microchip,mcp4241-103" + "microchip,mcp4241-503" +
Re: [PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 22, 2016 17:10, Joachim Eastwood wrote: > Hi Slawomir, > > On 22 March 2016 at 16:44, Slawomir Stepien <s...@poczta.fm> wrote: > > The following functionalities are supported: > > - write, read from volatile memory > > > > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf > > > > Signed-off-by: Slawomir Stepien <s...@poczta.fm> > > --- > > > +#include > > +#include > > +#include > > + > > +#include > > Give that you use that you have a some OF stuff in your driver you > should also include . Same goes for . > I am sure this builds fine without those includes, but you should > explicitly include stuff that you use. Oh yes that's true. > While you're at it you could also put the includes in alphabetic order. OK > > +static int mcp4131_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + int err; > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + int address = chan->channel; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + mutex_lock(>lock); > > + > > + data->buf[0] = (address << MCP4131_WIPER_SHIFT) | > > MCP4131_READ; > > + data->buf[1] = 0; > > + > > + err = mcp4131_read(data->spi, data->buf, 2); > > + if (err) { > > + mutex_unlock(>lock); > > + return err; > > + } > > + > > + /* Error, bad address/command combination */ > > + if (!MCP4131_CMDERR(data->buf)) > > + return -EIO; > > Missing mutex unlock here. Oh ;) I'll fix that. > > + > > + *val = MCP4131_RAW(data->buf); > > + mutex_unlock(>lock); > > + > > + return IIO_VAL_INT; > > + > > + case IIO_CHAN_INFO_SCALE: > > + *val = 1000 * data->cfg->kohms; > > + *val2 = data->cfg->max_pos; > > + return IIO_VAL_FRACTIONAL; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int mcp4131_write_raw(struct iio_dev *indio_dev, > > +struct iio_chan_spec const *chan, > > +int val, int val2, long mask) > > +{ > > + int err; > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + int address = chan->channel << MCP4131_WIPER_SHIFT; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (val > data->cfg->max_pos || val < 0) > > + return -EINVAL; > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + mutex_lock(>lock); > > + > > + data->buf[0] = address << MCP4131_WIPER_SHIFT; > > + data->buf[0] |= MCP4131_WRITE | (val >> 8); > > + data->buf[1] = val & 0xFF; /* 8 bits here */ > > + > > + err = spi_write(data->spi, data->buf, 2); > > + if (err) { > > + mutex_unlock(>lock); > > + return err; > > + } > > + mutex_unlock(>lock); > > + > > + return 0; > > This last part could be written as: > err = spi_write(data->spi, data->buf, 2); > mutex_unlock(>lock); > > return err; OK > Other than the things I noted driver looks good to me. > > > regards, > Joachim Eastwood -- Slawomir Stepien
Re: [PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 22, 2016 17:10, Joachim Eastwood wrote: > Hi Slawomir, > > On 22 March 2016 at 16:44, Slawomir Stepien wrote: > > The following functionalities are supported: > > - write, read from volatile memory > > > > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf > > > > Signed-off-by: Slawomir Stepien > > --- > > > +#include > > +#include > > +#include > > + > > +#include > > Give that you use that you have a some OF stuff in your driver you > should also include . Same goes for . > I am sure this builds fine without those includes, but you should > explicitly include stuff that you use. Oh yes that's true. > While you're at it you could also put the includes in alphabetic order. OK > > +static int mcp4131_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + int err; > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + int address = chan->channel; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + mutex_lock(>lock); > > + > > + data->buf[0] = (address << MCP4131_WIPER_SHIFT) | > > MCP4131_READ; > > + data->buf[1] = 0; > > + > > + err = mcp4131_read(data->spi, data->buf, 2); > > + if (err) { > > + mutex_unlock(>lock); > > + return err; > > + } > > + > > + /* Error, bad address/command combination */ > > + if (!MCP4131_CMDERR(data->buf)) > > + return -EIO; > > Missing mutex unlock here. Oh ;) I'll fix that. > > + > > + *val = MCP4131_RAW(data->buf); > > + mutex_unlock(>lock); > > + > > + return IIO_VAL_INT; > > + > > + case IIO_CHAN_INFO_SCALE: > > + *val = 1000 * data->cfg->kohms; > > + *val2 = data->cfg->max_pos; > > + return IIO_VAL_FRACTIONAL; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int mcp4131_write_raw(struct iio_dev *indio_dev, > > +struct iio_chan_spec const *chan, > > +int val, int val2, long mask) > > +{ > > + int err; > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + int address = chan->channel << MCP4131_WIPER_SHIFT; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (val > data->cfg->max_pos || val < 0) > > + return -EINVAL; > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + mutex_lock(>lock); > > + > > + data->buf[0] = address << MCP4131_WIPER_SHIFT; > > + data->buf[0] |= MCP4131_WRITE | (val >> 8); > > + data->buf[1] = val & 0xFF; /* 8 bits here */ > > + > > + err = spi_write(data->spi, data->buf, 2); > > + if (err) { > > + mutex_unlock(>lock); > > + return err; > > + } > > + mutex_unlock(>lock); > > + > > + return 0; > > This last part could be written as: > err = spi_write(data->spi, data->buf, 2); > mutex_unlock(>lock); > > return err; OK > Other than the things I noted driver looks good to me. > > > regards, > Joachim Eastwood -- Slawomir Stepien
[PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
The following functionalities are supported: - write, read from volatile memory Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf Signed-off-by: Slawomir Stepien <s...@poczta.fm> --- Changes since v3: - Added 'potentiometer' to subject - Replaced mcp4131_exec with spi_write and mcp4131_read - Narrowed mutex locking and unlocking places Changes since v2: - Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro - Replaced the rx and tx SPI buffers with one buf that is cacheline_aligned - Changed mutex locking position - Replaced the devid with pointer to model configuration - Removed positive ("Registered" & "Unregistered") dev_info prints - Removed the mcp4131_remove callback Changes since v1: - One line summary changed - Fixed module name and OF compatible - Based code on Peter Rosin's code from mcp4531.c - No additional sysfs attributes - Deleted not used devid struct memeber - Changed mcp4131_exec function arguments: - write value is now u16 - no need to pass return buffer array - rx array from mcp4131_data is used - Added missing new line characters to dev_info --- .../bindings/iio/potentiometer/mcp4131.txt | 84 drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp4131.c| 492 + 4 files changed, 595 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt create mode 100644 drivers/iio/potentiometer/mcp4131.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt new file mode 100644 index 000..3ccba16 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt @@ -0,0 +1,84 @@ +* Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer + driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4131-502" + "microchip,mcp4131-103" + "microchip,mcp4131-503" + "microchip,mcp4131-104" + "microchip,mcp4132-502" + "microchip,mcp4132-103" + "microchip,mcp4132-503" + "microchip,mcp4132-104" + "microchip,mcp4141-502" + "microchip,mcp4141-103" + "microchip,mcp4141-503" + "microchip,mcp4141-104" + "microchip,mcp4142-502" + "microchip,mcp4142-103" + "microchip,mcp4142-503" + "microchip,mcp4142-104" + "microchip,mcp4151-502" + "microchip,mcp4151-103" + "microchip,mcp4151-503" + "microchip,mcp4151-104" + "microchip,mcp4152-502" + "microchip,mcp4152-103" + "microchip,mcp4152-503" + "microchip,mcp4152-104" + "microchip,mcp4161-502" + "microchip,mcp4161-103" + "microchip,mcp4161-503" + "microchip,mcp4161-104" + "microchip,mcp4162-502" + "microchip,mcp4162-103" + "microchip,mcp4162-503" + "microchip,mcp4162-104" + "microchip,mcp4231-502" + "microchip,mcp4231-103" + "microchip,mcp4231-503" + "microchip,mcp4231-104" + "microchip,mcp4232-502" + "microchip,mcp4232-103" + "microchip,mcp4232-503" + "microchip,mcp4232-104" + "microchip,mcp4241-502" + "microchip,mcp4241-103" + "microchip,mcp4241-503" + "microchip,mcp4241-104" + "microchip,mcp4242-502" + "microchip,mcp4242-103" + "microc
[PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
The following functionalities are supported: - write, read from volatile memory Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf Signed-off-by: Slawomir Stepien --- Changes since v3: - Added 'potentiometer' to subject - Replaced mcp4131_exec with spi_write and mcp4131_read - Narrowed mutex locking and unlocking places Changes since v2: - Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro - Replaced the rx and tx SPI buffers with one buf that is cacheline_aligned - Changed mutex locking position - Replaced the devid with pointer to model configuration - Removed positive ("Registered" & "Unregistered") dev_info prints - Removed the mcp4131_remove callback Changes since v1: - One line summary changed - Fixed module name and OF compatible - Based code on Peter Rosin's code from mcp4531.c - No additional sysfs attributes - Deleted not used devid struct memeber - Changed mcp4131_exec function arguments: - write value is now u16 - no need to pass return buffer array - rx array from mcp4131_data is used - Added missing new line characters to dev_info --- .../bindings/iio/potentiometer/mcp4131.txt | 84 drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp4131.c| 492 + 4 files changed, 595 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt create mode 100644 drivers/iio/potentiometer/mcp4131.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt new file mode 100644 index 000..3ccba16 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt @@ -0,0 +1,84 @@ +* Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer + driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4131-502" + "microchip,mcp4131-103" + "microchip,mcp4131-503" + "microchip,mcp4131-104" + "microchip,mcp4132-502" + "microchip,mcp4132-103" + "microchip,mcp4132-503" + "microchip,mcp4132-104" + "microchip,mcp4141-502" + "microchip,mcp4141-103" + "microchip,mcp4141-503" + "microchip,mcp4141-104" + "microchip,mcp4142-502" + "microchip,mcp4142-103" + "microchip,mcp4142-503" + "microchip,mcp4142-104" + "microchip,mcp4151-502" + "microchip,mcp4151-103" + "microchip,mcp4151-503" + "microchip,mcp4151-104" + "microchip,mcp4152-502" + "microchip,mcp4152-103" + "microchip,mcp4152-503" + "microchip,mcp4152-104" + "microchip,mcp4161-502" + "microchip,mcp4161-103" + "microchip,mcp4161-503" + "microchip,mcp4161-104" + "microchip,mcp4162-502" + "microchip,mcp4162-103" + "microchip,mcp4162-503" + "microchip,mcp4162-104" + "microchip,mcp4231-502" + "microchip,mcp4231-103" + "microchip,mcp4231-503" + "microchip,mcp4231-104" + "microchip,mcp4232-502" + "microchip,mcp4232-103" + "microchip,mcp4232-503" + "microchip,mcp4232-104" + "microchip,mcp4241-502" + "microchip,mcp4241-103" + "microchip,mcp4241-503" + "microchip,mcp4241-104" + "microchip,mcp4242-502" + "microchip,mcp4242-103" + "microchip,mc
Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 20, 2016 19:15, Joachim Eastwood wrote: > Hi Jonathan, > > On 20 March 2016 at 18:25, Jonathan Cameron <ji...@kernel.org> wrote: > > On 20/03/16 16:12, Joachim Eastwood wrote: > >>> +static int mcp4131_exec(struct mcp4131_data *data, > >>> + u8 addr, u8 cmd, > >>> + u16 val) > >>> +{ > >>> + int err; > >>> + struct spi_device *spi = data->spi; > >>> + > >>> + data->xfer.tx_buf = data->buf; > >>> + data->xfer.rx_buf = data->buf; > >>> + > >>> + switch (cmd) { > >>> + case MCP4131_READ: > >>> + data->xfer.len = 2; /* Two bytes transfer for this > >>> command */ > >>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | > >>> MCP4131_READ; > >>> + data->buf[1] = 0; > >>> + break; > >>> + > >>> + case MCP4131_WRITE: > >>> + data->xfer.len = 2; > >>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | > >>> + MCP4131_WRITE | (val >> 8); > >>> + data->buf[1] = val & 0xFF; /* 8 bits here */ > >>> + break; > >>> + > >>> + default: > >>> + return -EINVAL; > >>> + } > >>> + > >>> + dev_dbg(>dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n", > >>> + data->buf[0], data->buf[1]); > >>> + > >>> + spi_message_init(>msg); > >>> + spi_message_add_tail(>xfer, >msg); > >>> + > >>> + err = spi_sync(spi, >msg); > >>> + if (err) { > >>> + dev_err(>dev, "spi_sync(): %d\n", err); > >>> + return err; > >>> + } > >> > >> Isn't this init, add, sync sequence basically open coding of what > >> spi_write/spi_read does? > >> If you could use those you could also get rid transfer/message structs > >> in priv data. > > I initially wrote the same comment, then realised it's more nuanced than > > that. Whilst this initially looks like an w8r8 type cycle it's actually > > something like w4r12 in the read case anyway. The write case could indeed > > be done with spi_write. > > Indeed. I didn't notice that for the read case. > > The read case could almost be copy of spi_read, though. One would only > need to add ".tx_buf = buf" when setting up the transfer struct, I > think. Having it in its a own function with a comment would make it > easier to spot the difference. Just to see if I get it. For write case I should use the spi_write as it is: case MCP4131_WRITE: spi_write(...); For read case I should create new function (e.g. mcp4131_read) that will look like spi_read but with additional tx_buf content so I can read the data on miso? case MCP4131_READ: mcp4131_read(...) Keep the needed buffers (transfer/message) local. -- Slawomir Stepien
Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 20, 2016 19:15, Joachim Eastwood wrote: > Hi Jonathan, > > On 20 March 2016 at 18:25, Jonathan Cameron wrote: > > On 20/03/16 16:12, Joachim Eastwood wrote: > >>> +static int mcp4131_exec(struct mcp4131_data *data, > >>> + u8 addr, u8 cmd, > >>> + u16 val) > >>> +{ > >>> + int err; > >>> + struct spi_device *spi = data->spi; > >>> + > >>> + data->xfer.tx_buf = data->buf; > >>> + data->xfer.rx_buf = data->buf; > >>> + > >>> + switch (cmd) { > >>> + case MCP4131_READ: > >>> + data->xfer.len = 2; /* Two bytes transfer for this > >>> command */ > >>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | > >>> MCP4131_READ; > >>> + data->buf[1] = 0; > >>> + break; > >>> + > >>> + case MCP4131_WRITE: > >>> + data->xfer.len = 2; > >>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | > >>> + MCP4131_WRITE | (val >> 8); > >>> + data->buf[1] = val & 0xFF; /* 8 bits here */ > >>> + break; > >>> + > >>> + default: > >>> + return -EINVAL; > >>> + } > >>> + > >>> + dev_dbg(>dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n", > >>> + data->buf[0], data->buf[1]); > >>> + > >>> + spi_message_init(>msg); > >>> + spi_message_add_tail(>xfer, >msg); > >>> + > >>> + err = spi_sync(spi, >msg); > >>> + if (err) { > >>> + dev_err(>dev, "spi_sync(): %d\n", err); > >>> + return err; > >>> + } > >> > >> Isn't this init, add, sync sequence basically open coding of what > >> spi_write/spi_read does? > >> If you could use those you could also get rid transfer/message structs > >> in priv data. > > I initially wrote the same comment, then realised it's more nuanced than > > that. Whilst this initially looks like an w8r8 type cycle it's actually > > something like w4r12 in the read case anyway. The write case could indeed > > be done with spi_write. > > Indeed. I didn't notice that for the read case. > > The read case could almost be copy of spi_read, though. One would only > need to add ".tx_buf = buf" when setting up the transfer struct, I > think. Having it in its a own function with a comment would make it > easier to spot the difference. Just to see if I get it. For write case I should use the spi_write as it is: case MCP4131_WRITE: spi_write(...); For read case I should create new function (e.g. mcp4131_read) that will look like spi_read but with additional tx_buf content so I can read the data on miso? case MCP4131_READ: mcp4131_read(...) Keep the needed buffers (transfer/message) local. -- Slawomir Stepien
Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 20, 2016 17:12, Joachim Eastwood wrote: > Hi Slawomir, > > On 20 March 2016 at 15:30, Slawomir Stepien <s...@poczta.fm> wrote: > > The following functionalities are supported: > > - write, read from volatile memory > > I think it would be useful if you could put 'potentiometer' either in > the subject and/or commit text so it is more obvious what this driver > is for. Ok > > + spi_message_init(>msg); > > + spi_message_add_tail(>xfer, >msg); > > + > > + err = spi_sync(spi, >msg); > > + if (err) { > > + dev_err(>dev, "spi_sync(): %d\n", err); > > + return err; > > + } > > Isn't this init, add, sync sequence basically open coding of what > spi_write/spi_read does? > If you could use those you could also get rid transfer/message structs > in priv data. > Also it these any reason why the data buffer can just be a local > variable in mcp4131_read_raw/mcp4131_write_raw? > If it could be I think it should be possible to move the lock into the > mcp4131_exec function. Ok I'll try that. > > + case IIO_CHAN_INFO_SCALE: > > + *val = 1000 * data->cfg->kohms; > > + *val2 = data->cfg->max_pos; > > + mutex_unlock(>lock); > > Is locking really necessary for IIO_CHAN_INFO_SCALE? > Isn't all data->cfg stuff constant? Yes you're right here. Didn't see it like that. > > +static int mcp4131_write_raw(struct iio_dev *indio_dev, > > +struct iio_chan_spec const *chan, > > +int val, int val2, long mask) > > +{ > > + int err; > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + int address = chan->channel << MCP4131_WIPER_SHIFT; > > + > > + mutex_lock(>lock); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (val > data->cfg->max_pos || val < 0) { > > + mutex_unlock(>lock); > > + return -EINVAL; > > + } > > + break; > > + default: > > + mutex_unlock(>lock); > > + return -EINVAL; > > + } > > + > > + err = mcp4131_exec(data, address, MCP4131_WRITE, val); > > + mutex_unlock(>lock); > > While this is not a huge function it is usually good practice to keep > the locking scope as small as possible. > > So wouldn't this be sufficient here. > mutex_lock(>lock); > err = mcp4131_exec(data, address, MCP4131_WRITE, val); > mutex_unlock(>lock); > > Of course if you are able move the lock into mcp4131_exec this will go away. I'll see if it's possible to move the whole locking into this function. Thank you for comments. > regards, > Joachim Eastwood -- Slawomir Stepien
Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 20, 2016 17:12, Joachim Eastwood wrote: > Hi Slawomir, > > On 20 March 2016 at 15:30, Slawomir Stepien wrote: > > The following functionalities are supported: > > - write, read from volatile memory > > I think it would be useful if you could put 'potentiometer' either in > the subject and/or commit text so it is more obvious what this driver > is for. Ok > > + spi_message_init(>msg); > > + spi_message_add_tail(>xfer, >msg); > > + > > + err = spi_sync(spi, >msg); > > + if (err) { > > + dev_err(>dev, "spi_sync(): %d\n", err); > > + return err; > > + } > > Isn't this init, add, sync sequence basically open coding of what > spi_write/spi_read does? > If you could use those you could also get rid transfer/message structs > in priv data. > Also it these any reason why the data buffer can just be a local > variable in mcp4131_read_raw/mcp4131_write_raw? > If it could be I think it should be possible to move the lock into the > mcp4131_exec function. Ok I'll try that. > > + case IIO_CHAN_INFO_SCALE: > > + *val = 1000 * data->cfg->kohms; > > + *val2 = data->cfg->max_pos; > > + mutex_unlock(>lock); > > Is locking really necessary for IIO_CHAN_INFO_SCALE? > Isn't all data->cfg stuff constant? Yes you're right here. Didn't see it like that. > > +static int mcp4131_write_raw(struct iio_dev *indio_dev, > > +struct iio_chan_spec const *chan, > > +int val, int val2, long mask) > > +{ > > + int err; > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + int address = chan->channel << MCP4131_WIPER_SHIFT; > > + > > + mutex_lock(>lock); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (val > data->cfg->max_pos || val < 0) { > > + mutex_unlock(>lock); > > + return -EINVAL; > > + } > > + break; > > + default: > > + mutex_unlock(>lock); > > + return -EINVAL; > > + } > > + > > + err = mcp4131_exec(data, address, MCP4131_WRITE, val); > > + mutex_unlock(>lock); > > While this is not a huge function it is usually good practice to keep > the locking scope as small as possible. > > So wouldn't this be sufficient here. > mutex_lock(>lock); > err = mcp4131_exec(data, address, MCP4131_WRITE, val); > mutex_unlock(>lock); > > Of course if you are able move the lock into mcp4131_exec this will go away. I'll see if it's possible to move the whole locking into this function. Thank you for comments. > regards, > Joachim Eastwood -- Slawomir Stepien
[PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
The following functionalities are supported: - write, read from volatile memory Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf Signed-off-by: Slawomir Stepien <s...@poczta.fm> --- Changes since v2: - Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro - Replaced the rx and tx SPI buffers with one buf that is cacheline_aligned - Changed mutex locking position - Replaced the devid with pointer to model configuration - Removed positive ("Registered" & "Unregistered") dev_info prints - Removed the mcp4131_remove callback Changes since v1: - One line summary changed - Fixed module name and OF compatible - Based code on Peter Rosin's code from mcp4531.c - No additional sysfs attributes - Deleted not used devid struct memeber - Changed mcp4131_exec function arguments: - write value is now u16 - no need to pass return buffer array - rx array from mcp4131_data is used - Added missing new line characters to dev_info --- .../bindings/iio/potentiometer/mcp4131.txt | 84 drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp4131.c| 520 + 4 files changed, 623 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt create mode 100644 drivers/iio/potentiometer/mcp4131.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt new file mode 100644 index 000..3ccba16 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt @@ -0,0 +1,84 @@ +* Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer + driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4131-502" + "microchip,mcp4131-103" + "microchip,mcp4131-503" + "microchip,mcp4131-104" + "microchip,mcp4132-502" + "microchip,mcp4132-103" + "microchip,mcp4132-503" + "microchip,mcp4132-104" + "microchip,mcp4141-502" + "microchip,mcp4141-103" + "microchip,mcp4141-503" + "microchip,mcp4141-104" + "microchip,mcp4142-502" + "microchip,mcp4142-103" + "microchip,mcp4142-503" + "microchip,mcp4142-104" + "microchip,mcp4151-502" + "microchip,mcp4151-103" + "microchip,mcp4151-503" + "microchip,mcp4151-104" + "microchip,mcp4152-502" + "microchip,mcp4152-103" + "microchip,mcp4152-503" + "microchip,mcp4152-104" + "microchip,mcp4161-502" + "microchip,mcp4161-103" + "microchip,mcp4161-503" + "microchip,mcp4161-104" + "microchip,mcp4162-502" + "microchip,mcp4162-103" + "microchip,mcp4162-503" + "microchip,mcp4162-104" + "microchip,mcp4231-502" + "microchip,mcp4231-103" + "microchip,mcp4231-503" + "microchip,mcp4231-104" + "microchip,mcp4232-502" + "microchip,mcp4232-103" + "microchip,mcp4232-503" + "microchip,mcp4232-104" + "microchip,mcp4241-502" + "microchip,mcp4241-103" + "microchip,mcp4241-503" + "microchip,mcp4241-104" + "microchip,mcp4242-502" + "microchip,mcp4242-103" + "microchip,mcp4242-503" + "microchip,mcp4242-104" + "microchip,mcp4251-502" +
[PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
The following functionalities are supported: - write, read from volatile memory Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf Signed-off-by: Slawomir Stepien --- Changes since v2: - Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro - Replaced the rx and tx SPI buffers with one buf that is cacheline_aligned - Changed mutex locking position - Replaced the devid with pointer to model configuration - Removed positive ("Registered" & "Unregistered") dev_info prints - Removed the mcp4131_remove callback Changes since v1: - One line summary changed - Fixed module name and OF compatible - Based code on Peter Rosin's code from mcp4531.c - No additional sysfs attributes - Deleted not used devid struct memeber - Changed mcp4131_exec function arguments: - write value is now u16 - no need to pass return buffer array - rx array from mcp4131_data is used - Added missing new line characters to dev_info --- .../bindings/iio/potentiometer/mcp4131.txt | 84 drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp4131.c| 520 + 4 files changed, 623 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt create mode 100644 drivers/iio/potentiometer/mcp4131.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt new file mode 100644 index 000..3ccba16 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt @@ -0,0 +1,84 @@ +* Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer + driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4131-502" + "microchip,mcp4131-103" + "microchip,mcp4131-503" + "microchip,mcp4131-104" + "microchip,mcp4132-502" + "microchip,mcp4132-103" + "microchip,mcp4132-503" + "microchip,mcp4132-104" + "microchip,mcp4141-502" + "microchip,mcp4141-103" + "microchip,mcp4141-503" + "microchip,mcp4141-104" + "microchip,mcp4142-502" + "microchip,mcp4142-103" + "microchip,mcp4142-503" + "microchip,mcp4142-104" + "microchip,mcp4151-502" + "microchip,mcp4151-103" + "microchip,mcp4151-503" + "microchip,mcp4151-104" + "microchip,mcp4152-502" + "microchip,mcp4152-103" + "microchip,mcp4152-503" + "microchip,mcp4152-104" + "microchip,mcp4161-502" + "microchip,mcp4161-103" + "microchip,mcp4161-503" + "microchip,mcp4161-104" + "microchip,mcp4162-502" + "microchip,mcp4162-103" + "microchip,mcp4162-503" + "microchip,mcp4162-104" + "microchip,mcp4231-502" + "microchip,mcp4231-103" + "microchip,mcp4231-503" + "microchip,mcp4231-104" + "microchip,mcp4232-502" + "microchip,mcp4232-103" + "microchip,mcp4232-503" + "microchip,mcp4232-104" + "microchip,mcp4241-502" + "microchip,mcp4241-103" + "microchip,mcp4241-503" + "microchip,mcp4241-104" + "microchip,mcp4242-502" + "microchip,mcp4242-103" + "microchip,mcp4242-503" + "microchip,mcp4242-104" + "microchip,mcp4251-502" +
Re: [PATCH v2] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 20, 2016 10:25, Jonathan Cameron wrote: > >> +struct mcp4131_data { > >> + struct spi_device *spi; > > This is only used to lookup elements of your cfg array, I'd just have > a pointer to the relevant element of that array in here instead. > > struct mcp4131_cfg *cfg; > > and in probe do > data->cfg = _cfg[id]; Great idea. I'll use it in v3. > >> + unsigned long devid; > >> + struct mutex lock; > >> + u8 tx[2], rx[2]; > > > > alignment requirements for SPI transfer? > By which he means put them at the end of this structure and > mark the with __cacheline_aligned. It's not technically about alignment > but rather about ensuring nothing else is in the cacheline which will on some > spi devices be scrubbed when a transaction occurs. Thank you for this explanation. I'll move it at the and mark it with the attribute. > >> + data->rx[0] = 0; > >> + data->rx[1] = 0; > > > > initialization needed? > > > > setup of data->xfer + data->tx is done outside the lock, this seems wrong > agreed. I'll lock the mutex just before switching the mask in _read_raw and in write_raw like this: mutex_lock(>lock); switch(mask) { case IIO_CHAN_INFO_RAW: (...) } mutex_unlock(>lock); > Now I'd change the way you are doing this slightly so that you have > data->cfg pointing to mcp4131[data->devid]. Moves the 'what part am I?' > question to a single place in the probe function giving slightly cleaner code. > >> + *val = 1000 * mcp4131_cfg[data->devid].kohms; > >> + *val2 = mcp4131_cfg[data->devid].max_pos; > >> + return IIO_VAL_FRACTIONAL; Something like this: *val = 1000 * data->cfg->kohms; *val2 = data->cfg->max_pos; mutex_unlock(>lock); return IIO_VAL_FRACTIONAL; ? > >> + dev_info(>dev, "Registered %s\n", indio_dev->name); > > > > I'd rather drop this message > Agreed, adds noise and it's easy to check if the register succeeded anyway > by just looking to see if the device is there in sysfs. OK > >> +static int mcp4131_remove(struct spi_device *spi) > >> +{ > >> + struct iio_dev *indio_dev = spi_get_drvdata(spi); > >> + struct mcp4131_data *data = iio_priv(indio_dev); > >> + > >> + mutex_destroy(>lock); > > > > no need to call > Hmm. This is an oddity, the mutex_destroy exists to aid in debugging locking > issues by explicity marking the mutex as do not use - iff the mutex > debugging is enabled. In this case the storage is promptly deleted anyway > so any attempt to use the mutex would result in a null pointer dereference > anyway. Hence probably not worth having it here. OK. Thank your for all the explanations. This helps a lot. -- Slawomir Stepien
Re: [PATCH v2] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 20, 2016 10:25, Jonathan Cameron wrote: > >> +struct mcp4131_data { > >> + struct spi_device *spi; > > This is only used to lookup elements of your cfg array, I'd just have > a pointer to the relevant element of that array in here instead. > > struct mcp4131_cfg *cfg; > > and in probe do > data->cfg = _cfg[id]; Great idea. I'll use it in v3. > >> + unsigned long devid; > >> + struct mutex lock; > >> + u8 tx[2], rx[2]; > > > > alignment requirements for SPI transfer? > By which he means put them at the end of this structure and > mark the with __cacheline_aligned. It's not technically about alignment > but rather about ensuring nothing else is in the cacheline which will on some > spi devices be scrubbed when a transaction occurs. Thank you for this explanation. I'll move it at the and mark it with the attribute. > >> + data->rx[0] = 0; > >> + data->rx[1] = 0; > > > > initialization needed? > > > > setup of data->xfer + data->tx is done outside the lock, this seems wrong > agreed. I'll lock the mutex just before switching the mask in _read_raw and in write_raw like this: mutex_lock(>lock); switch(mask) { case IIO_CHAN_INFO_RAW: (...) } mutex_unlock(>lock); > Now I'd change the way you are doing this slightly so that you have > data->cfg pointing to mcp4131[data->devid]. Moves the 'what part am I?' > question to a single place in the probe function giving slightly cleaner code. > >> + *val = 1000 * mcp4131_cfg[data->devid].kohms; > >> + *val2 = mcp4131_cfg[data->devid].max_pos; > >> + return IIO_VAL_FRACTIONAL; Something like this: *val = 1000 * data->cfg->kohms; *val2 = data->cfg->max_pos; mutex_unlock(>lock); return IIO_VAL_FRACTIONAL; ? > >> + dev_info(>dev, "Registered %s\n", indio_dev->name); > > > > I'd rather drop this message > Agreed, adds noise and it's easy to check if the register succeeded anyway > by just looking to see if the device is there in sysfs. OK > >> +static int mcp4131_remove(struct spi_device *spi) > >> +{ > >> + struct iio_dev *indio_dev = spi_get_drvdata(spi); > >> + struct mcp4131_data *data = iio_priv(indio_dev); > >> + > >> + mutex_destroy(>lock); > > > > no need to call > Hmm. This is an oddity, the mutex_destroy exists to aid in debugging locking > issues by explicity marking the mutex as do not use - iff the mutex > debugging is enabled. In this case the storage is promptly deleted anyway > so any attempt to use the mutex would result in a null pointer dereference > anyway. Hence probably not worth having it here. OK. Thank your for all the explanations. This helps a lot. -- Slawomir Stepien
Re: [PATCH v2] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 19, 2016 14:48, Peter Meerwald-Stadler wrote: > > +#define MCP4131_WIPER_SHIFT(4) > > () not needed OK > > +struct mcp4131_data { > > + struct spi_device *spi; > > + unsigned long devid; > > + struct mutex lock; > > + u8 tx[2], rx[2]; > > alignment requirements for SPI transfer? Do you mean the cacheline_aligned attribute? I did not add it because I'm not quite sure why it's needed there. Will have to find it out... Could you point me some materials where it's explained? I think I can drop two separated buffers in favor of one buffer (e.g. buf[2]). I saw drivers doing that. Do you think that's a good idea? > > + data->rx[0] = 0; > > + data->rx[1] = 0; > > initialization needed? No. You're right. > setup of data->xfer + data->tx is done outside the lock, this seems wrong True. Will fix it in v3. > > + dev_info(>dev, "Registered %s\n", indio_dev->name); > > I'd rather drop this message OK. Will leave only the dev_info for errors. > > +static int mcp4131_remove(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + > > + mutex_destroy(>lock); > > no need to call > > > + devm_iio_device_unregister(>dev, indio_dev); > > don't call this explicitly, it is done automatically after _remove That's why it's called managed (devm_*)? > > + > > + dev_info(>dev, "Unregistered %s\n", indio_dev->name); > > don't > > I think the entire _remove can be removed OK. Thank you for the review. I'm learning a lot! -- Slawomir Stepien
Re: [PATCH v2] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On Mar 19, 2016 14:48, Peter Meerwald-Stadler wrote: > > +#define MCP4131_WIPER_SHIFT(4) > > () not needed OK > > +struct mcp4131_data { > > + struct spi_device *spi; > > + unsigned long devid; > > + struct mutex lock; > > + u8 tx[2], rx[2]; > > alignment requirements for SPI transfer? Do you mean the cacheline_aligned attribute? I did not add it because I'm not quite sure why it's needed there. Will have to find it out... Could you point me some materials where it's explained? I think I can drop two separated buffers in favor of one buffer (e.g. buf[2]). I saw drivers doing that. Do you think that's a good idea? > > + data->rx[0] = 0; > > + data->rx[1] = 0; > > initialization needed? No. You're right. > setup of data->xfer + data->tx is done outside the lock, this seems wrong True. Will fix it in v3. > > + dev_info(>dev, "Registered %s\n", indio_dev->name); > > I'd rather drop this message OK. Will leave only the dev_info for errors. > > +static int mcp4131_remove(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + > > + mutex_destroy(>lock); > > no need to call > > > + devm_iio_device_unregister(>dev, indio_dev); > > don't call this explicitly, it is done automatically after _remove That's why it's called managed (devm_*)? > > + > > + dev_info(>dev, "Unregistered %s\n", indio_dev->name); > > don't > > I think the entire _remove can be removed OK. Thank you for the review. I'm learning a lot! -- Slawomir Stepien
[PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
The following functionalities are supported: - write, read from volatile and non volatile memory - increase and decrease commands - read from status register - write and read to tcon register Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059b.pdf Signed-off-by: Slawomir Stepien <s...@poczta.fm> --- .../bindings/iio/potentiometer/mcp41xx.txt | 51 ++ Documentation/iio/mcp41xx.txt | 86 +++ drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp41xx.c| 709 + 5 files changed, 865 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt create mode 100644 Documentation/iio/mcp41xx.txt create mode 100644 drivers/iio/potentiometer/mcp41xx.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt new file mode 100644 index 000..6031142 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt @@ -0,0 +1,51 @@ +* Microchip MCP414X/416X/424X/426X Digital Potentiometer driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4113x-502" + "microchip,mcp4113x-103" + "microchip,mcp4113x-503" + "microchip,mcp4113x-104" + "microchip,mcp4114x-502" + "microchip,mcp4114x-103" + "microchip,mcp4114x-503" + "microchip,mcp4114x-104" + "microchip,mcp4115x-502" + "microchip,mcp4115x-103" + "microchip,mcp4115x-503" + "microchip,mcp4115x-104" + "microchip,mcp4116x-502" + "microchip,mcp4116x-103" + "microchip,mcp4116x-503" + "microchip,mcp4116x-104" + "microchip,mcp4123x-502" + "microchip,mcp4123x-103" + "microchip,mcp4123x-503" + "microchip,mcp4123x-104" + "microchip,mcp4124x-502" + "microchip,mcp4124x-103" + "microchip,mcp4124x-503" + "microchip,mcp4124x-104" + "microchip,mcp4125x-502" + "microchip,mcp4125x-103" + "microchip,mcp4125x-503" + "microchip,mcp4125x-104" + "microchip,mcp4126x-502" + "microchip,mcp4126x-103" + "microchip,mcp4126x-503" + "microchip,mcp4126x-104" + +Example: +mcp41xx: mcp41xx@0 { + compatible = "mcp416x-502"; + reg = <0>; + spi-max-frequency = <50>; +}; diff --git a/Documentation/iio/mcp41xx.txt b/Documentation/iio/mcp41xx.txt new file mode 100644 index 000..57dc889 --- /dev/null +++ b/Documentation/iio/mcp41xx.txt @@ -0,0 +1,86 @@ +Industrial I/O driver for MCP414X/416X/424X/426X Digital POTs +- + +Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059a.pdf + +sysfs interface +--- +N is the wiper number (0 = first wiper, 1 = second wiper) + +File: /sys/bus/iio/devices/../decr_wiperN +Mode: Write +Description: + Write anything to this file to decrease wiper N position by one step. +Example: + echo > decr_wiper0 + + +File: /sys/bus/iio/devices/../incr_wiperN +Mode: Write +Description: + Write anything to this file to increase wiper N position by one step. +Example: + echo > incr_wiper0 + + +File: /sys/bus/iio/devices/../memory_map +Mode: Read/Write +Description: + Read the whole memory or write value to given memory address. + Read outputs two columns. First column is address and second column is + value at that address. + For write use hex values with leading 0x. First hex is address second + hex is the value. +Example: + cat memory_map + echo "0x00 0x80" > memory_map + + +File: /sys/bus/iio/devices/../nv_wiperN +Mode: Read/Write +Description: +
[PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
The following functionalities are supported: - write, read from volatile and non volatile memory - increase and decrease commands - read from status register - write and read to tcon register Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059b.pdf Signed-off-by: Slawomir Stepien --- .../bindings/iio/potentiometer/mcp41xx.txt | 51 ++ Documentation/iio/mcp41xx.txt | 86 +++ drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp41xx.c| 709 + 5 files changed, 865 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt create mode 100644 Documentation/iio/mcp41xx.txt create mode 100644 drivers/iio/potentiometer/mcp41xx.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt new file mode 100644 index 000..6031142 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt @@ -0,0 +1,51 @@ +* Microchip MCP414X/416X/424X/426X Digital Potentiometer driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4113x-502" + "microchip,mcp4113x-103" + "microchip,mcp4113x-503" + "microchip,mcp4113x-104" + "microchip,mcp4114x-502" + "microchip,mcp4114x-103" + "microchip,mcp4114x-503" + "microchip,mcp4114x-104" + "microchip,mcp4115x-502" + "microchip,mcp4115x-103" + "microchip,mcp4115x-503" + "microchip,mcp4115x-104" + "microchip,mcp4116x-502" + "microchip,mcp4116x-103" + "microchip,mcp4116x-503" + "microchip,mcp4116x-104" + "microchip,mcp4123x-502" + "microchip,mcp4123x-103" + "microchip,mcp4123x-503" + "microchip,mcp4123x-104" + "microchip,mcp4124x-502" + "microchip,mcp4124x-103" + "microchip,mcp4124x-503" + "microchip,mcp4124x-104" + "microchip,mcp4125x-502" + "microchip,mcp4125x-103" + "microchip,mcp4125x-503" + "microchip,mcp4125x-104" + "microchip,mcp4126x-502" + "microchip,mcp4126x-103" + "microchip,mcp4126x-503" + "microchip,mcp4126x-104" + +Example: +mcp41xx: mcp41xx@0 { + compatible = "mcp416x-502"; + reg = <0>; + spi-max-frequency = <50>; +}; diff --git a/Documentation/iio/mcp41xx.txt b/Documentation/iio/mcp41xx.txt new file mode 100644 index 000..57dc889 --- /dev/null +++ b/Documentation/iio/mcp41xx.txt @@ -0,0 +1,86 @@ +Industrial I/O driver for MCP414X/416X/424X/426X Digital POTs +- + +Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059a.pdf + +sysfs interface +--- +N is the wiper number (0 = first wiper, 1 = second wiper) + +File: /sys/bus/iio/devices/../decr_wiperN +Mode: Write +Description: + Write anything to this file to decrease wiper N position by one step. +Example: + echo > decr_wiper0 + + +File: /sys/bus/iio/devices/../incr_wiperN +Mode: Write +Description: + Write anything to this file to increase wiper N position by one step. +Example: + echo > incr_wiper0 + + +File: /sys/bus/iio/devices/../memory_map +Mode: Read/Write +Description: + Read the whole memory or write value to given memory address. + Read outputs two columns. First column is address and second column is + value at that address. + For write use hex values with leading 0x. First hex is address second + hex is the value. +Example: + cat memory_map + echo "0x00 0x80" > memory_map + + +File: /sys/bus/iio/devices/../nv_wiperN +Mode: Read/Write +Description: + Read or write to n
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote: > On Wed, 16 Mar 2016, Slawomir Stepien wrote: > > > The following functionalities are supported: > > - write, read from volatile and non volatile memory > > - increase and decrease commands > > - read from status register > > - write and read to tcon register > > > > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059b.pdf Thank you for all your comments. > the driver naming is a mess: the subject says MCP414X, the file name is > mcp41xx, the DT compatible string says mcp4113x -- this does not match OK. I will change that to mcp414x in version 2. > plenty of the private API, some of which seems to be debug only? > what is really needed to interact with a poti? I wanted to export both the non volatile and volatile memory addresses for wiper position access. That is bare minimum for the poti to operate. But I also wanted to export additional features of this chip. That is way there is increase and decrease API, and STATUS and TCON register access. The memory_map API is a way to access all the not used by chip memory addresses. This API I think could be deleted. But I still think that some people might find it useful. > comments below > > > +File: /sys/bus/iio/devices/../out_resistanceN_raw > > this is already described in sysfs-bus-iio Should I leave it and add reference to sysfs-bus-iio or delete it completely? > > +struct mcp41xx_cfg { > > + unsigned long int devid; > > devid is not set/used That's true. Will fix it in v2. > > +static int mcp41xx_exec(struct mcp41xx_data *data, > > + u8 addr, u8 cmd, > > + int *trans, size_t n) > > should trans really be int, not u8? There is a need for 9 bit value write/read. I used int just for my convenience. However there is one piece of code missing now I see. I should check the value of tans[0] to see if it's > 256 or lower then 0. Will add it in v2. Using u8 will force additional bit operations. I could try using u16 to still have the option of parsing user input as 9 bit value (eg. 256 position). > > +{ > > + int err; > > + struct spi_device *spi = data->spi; > > + > > + /* Two bytes are needed for the response */ > > + if (n != 2) > > + return -EINVAL; > > why pass n if it is always 2? Will fix in v2. > > + err = devm_iio_device_register(>dev, indio_dev); > > + if (err) { > > + dev_info(>dev, "Unable to register %s", indio_dev->name); > > \n missing > > > + return err; > > + } > > + > > + dev_info(>dev, "Registered %s", indio_dev->name); > > \n > > > + > > + return 0; > > +} > > + > > +static int mcp41xx_remove(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > > + struct mcp41xx_data *data = iio_priv(indio_dev); > > + > > + mutex_destroy(>lock); > > + devm_iio_device_unregister(>dev, indio_dev); > > + kfree(mcp41xx_attributes); > > + > > + dev_info(>dev, "Unregistered %s", indio_dev->name); Also \n is missing here. Will fix in v2. -- Slawomir Stepien
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote: > On Wed, 16 Mar 2016, Slawomir Stepien wrote: > > > The following functionalities are supported: > > - write, read from volatile and non volatile memory > > - increase and decrease commands > > - read from status register > > - write and read to tcon register > > > > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059b.pdf Thank you for all your comments. > the driver naming is a mess: the subject says MCP414X, the file name is > mcp41xx, the DT compatible string says mcp4113x -- this does not match OK. I will change that to mcp414x in version 2. > plenty of the private API, some of which seems to be debug only? > what is really needed to interact with a poti? I wanted to export both the non volatile and volatile memory addresses for wiper position access. That is bare minimum for the poti to operate. But I also wanted to export additional features of this chip. That is way there is increase and decrease API, and STATUS and TCON register access. The memory_map API is a way to access all the not used by chip memory addresses. This API I think could be deleted. But I still think that some people might find it useful. > comments below > > > +File: /sys/bus/iio/devices/../out_resistanceN_raw > > this is already described in sysfs-bus-iio Should I leave it and add reference to sysfs-bus-iio or delete it completely? > > +struct mcp41xx_cfg { > > + unsigned long int devid; > > devid is not set/used That's true. Will fix it in v2. > > +static int mcp41xx_exec(struct mcp41xx_data *data, > > + u8 addr, u8 cmd, > > + int *trans, size_t n) > > should trans really be int, not u8? There is a need for 9 bit value write/read. I used int just for my convenience. However there is one piece of code missing now I see. I should check the value of tans[0] to see if it's > 256 or lower then 0. Will add it in v2. Using u8 will force additional bit operations. I could try using u16 to still have the option of parsing user input as 9 bit value (eg. 256 position). > > +{ > > + int err; > > + struct spi_device *spi = data->spi; > > + > > + /* Two bytes are needed for the response */ > > + if (n != 2) > > + return -EINVAL; > > why pass n if it is always 2? Will fix in v2. > > + err = devm_iio_device_register(>dev, indio_dev); > > + if (err) { > > + dev_info(>dev, "Unable to register %s", indio_dev->name); > > \n missing > > > + return err; > > + } > > + > > + dev_info(>dev, "Registered %s", indio_dev->name); > > \n > > > + > > + return 0; > > +} > > + > > +static int mcp41xx_remove(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > > + struct mcp41xx_data *data = iio_priv(indio_dev); > > + > > + mutex_destroy(>lock); > > + devm_iio_device_unregister(>dev, indio_dev); > > + kfree(mcp41xx_attributes); > > + > > + dev_info(>dev, "Unregistered %s", indio_dev->name); Also \n is missing here. Will fix in v2. -- Slawomir Stepien
[PATCH v2] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
The following functionalities are supported: - write, read from volatile memory Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf Signed-off-by: Slawomir Stepien <s...@poczta.fm> --- This is v2 of patch: "iio: add driver for Microchip MCP414X/416X/424X/426X" Changes since v1: - One line summary changed - Fixed module name and OF compatible - Based code on Peter Rosin's code from mcp4531.c - No additional sysfs attributes - Deleted not used devid struct memeber - Changed mcp4131_exec function arguments: - write value is now u16 - no need to pass return buffer array - rx array from mcp4131_data is used - Added missing new line characters to dev_info --- .../bindings/iio/potentiometer/mcp4131.txt | 84 drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp4131.c| 523 + 4 files changed, 626 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt create mode 100644 drivers/iio/potentiometer/mcp4131.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt new file mode 100644 index 000..3ccba16 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt @@ -0,0 +1,84 @@ +* Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer + driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4131-502" + "microchip,mcp4131-103" + "microchip,mcp4131-503" + "microchip,mcp4131-104" + "microchip,mcp4132-502" + "microchip,mcp4132-103" + "microchip,mcp4132-503" + "microchip,mcp4132-104" + "microchip,mcp4141-502" + "microchip,mcp4141-103" + "microchip,mcp4141-503" + "microchip,mcp4141-104" + "microchip,mcp4142-502" + "microchip,mcp4142-103" + "microchip,mcp4142-503" + "microchip,mcp4142-104" + "microchip,mcp4151-502" + "microchip,mcp4151-103" + "microchip,mcp4151-503" + "microchip,mcp4151-104" + "microchip,mcp4152-502" + "microchip,mcp4152-103" + "microchip,mcp4152-503" + "microchip,mcp4152-104" + "microchip,mcp4161-502" + "microchip,mcp4161-103" + "microchip,mcp4161-503" + "microchip,mcp4161-104" + "microchip,mcp4162-502" + "microchip,mcp4162-103" + "microchip,mcp4162-503" + "microchip,mcp4162-104" + "microchip,mcp4231-502" + "microchip,mcp4231-103" + "microchip,mcp4231-503" + "microchip,mcp4231-104" + "microchip,mcp4232-502" + "microchip,mcp4232-103" + "microchip,mcp4232-503" + "microchip,mcp4232-104" + "microchip,mcp4241-502" + "microchip,mcp4241-103" + "microchip,mcp4241-503" + "microchip,mcp4241-104" + "microchip,mcp4242-502" + "microchip,mcp4242-103" + "microchip,mcp4242-503" + "microchip,mcp4242-104" + "microchip,mcp4251-502" + "microchip,mcp4251-103" + "microchip,mcp4251-503" + "microchip,mcp4251-104" + "microchip,mcp4252-502" + "microchip,mcp4252-103" +
[PATCH v2] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
The following functionalities are supported: - write, read from volatile memory Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf Signed-off-by: Slawomir Stepien --- This is v2 of patch: "iio: add driver for Microchip MCP414X/416X/424X/426X" Changes since v1: - One line summary changed - Fixed module name and OF compatible - Based code on Peter Rosin's code from mcp4531.c - No additional sysfs attributes - Deleted not used devid struct memeber - Changed mcp4131_exec function arguments: - write value is now u16 - no need to pass return buffer array - rx array from mcp4131_data is used - Added missing new line characters to dev_info --- .../bindings/iio/potentiometer/mcp4131.txt | 84 drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp4131.c| 523 + 4 files changed, 626 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt create mode 100644 drivers/iio/potentiometer/mcp4131.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt new file mode 100644 index 000..3ccba16 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt @@ -0,0 +1,84 @@ +* Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer + driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4131-502" + "microchip,mcp4131-103" + "microchip,mcp4131-503" + "microchip,mcp4131-104" + "microchip,mcp4132-502" + "microchip,mcp4132-103" + "microchip,mcp4132-503" + "microchip,mcp4132-104" + "microchip,mcp4141-502" + "microchip,mcp4141-103" + "microchip,mcp4141-503" + "microchip,mcp4141-104" + "microchip,mcp4142-502" + "microchip,mcp4142-103" + "microchip,mcp4142-503" + "microchip,mcp4142-104" + "microchip,mcp4151-502" + "microchip,mcp4151-103" + "microchip,mcp4151-503" + "microchip,mcp4151-104" + "microchip,mcp4152-502" + "microchip,mcp4152-103" + "microchip,mcp4152-503" + "microchip,mcp4152-104" + "microchip,mcp4161-502" + "microchip,mcp4161-103" + "microchip,mcp4161-503" + "microchip,mcp4161-104" + "microchip,mcp4162-502" + "microchip,mcp4162-103" + "microchip,mcp4162-503" + "microchip,mcp4162-104" + "microchip,mcp4231-502" + "microchip,mcp4231-103" + "microchip,mcp4231-503" + "microchip,mcp4231-104" + "microchip,mcp4232-502" + "microchip,mcp4232-103" + "microchip,mcp4232-503" + "microchip,mcp4232-104" + "microchip,mcp4241-502" + "microchip,mcp4241-103" + "microchip,mcp4241-503" + "microchip,mcp4241-104" + "microchip,mcp4242-502" + "microchip,mcp4242-103" + "microchip,mcp4242-503" + "microchip,mcp4242-104" + "microchip,mcp4251-502" + "microchip,mcp4251-103" + "microchip,mcp4251-503" + "microchip,mcp4251-104" + "microchip,mcp4252-502" + "microchip,mcp4252-103" + "micr
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On Mar 16, 2016 20:28, Daniel Baluta wrote: > On Wed, Mar 16, 2016 at 6:25 PM, Slawomir Stepien <s...@poczta.fm> wrote: > > On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote: > >> On Wed, 16 Mar 2016, Slawomir Stepien wrote: > >> > >> > The following functionalities are supported: > >> > - write, read from volatile and non volatile memory > >> > - increase and decrease commands > >> > - read from status register > >> > - write and read to tcon register > >> > > >> > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059b.pdf > > > > Thank you for all your comments. > > > >> the driver naming is a mess: the subject says MCP414X, the file name is > >> mcp41xx, the DT compatible string says mcp4113x -- this does not match > > > > OK. I will change that to mcp414x in version 2. > > Filename shouldn't be generic (e.g ending with xx). It should be a > specific chip name > (a good candidate is the first in alphabetical order), because there > could be chips falling > in the same name category but with a different driver. OK got it. Please wait for the version 2. -- Slawomir Stepien
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On Mar 16, 2016 20:28, Daniel Baluta wrote: > On Wed, Mar 16, 2016 at 6:25 PM, Slawomir Stepien wrote: > > On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote: > >> On Wed, 16 Mar 2016, Slawomir Stepien wrote: > >> > >> > The following functionalities are supported: > >> > - write, read from volatile and non volatile memory > >> > - increase and decrease commands > >> > - read from status register > >> > - write and read to tcon register > >> > > >> > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059b.pdf > > > > Thank you for all your comments. > > > >> the driver naming is a mess: the subject says MCP414X, the file name is > >> mcp41xx, the DT compatible string says mcp4113x -- this does not match > > > > OK. I will change that to mcp414x in version 2. > > Filename shouldn't be generic (e.g ending with xx). It should be a > specific chip name > (a good candidate is the first in alphabetical order), because there > could be chips falling > in the same name category but with a different driver. OK got it. Please wait for the version 2. -- Slawomir Stepien
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On Mar 16, 2016 19:24, Lars-Peter Clausen wrote: > On 03/16/2016 05:25 PM, Slawomir Stepien wrote: > > On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote: > [...] > >> plenty of the private API, some of which seems to be debug only? > >> what is really needed to interact with a poti? > > > > I wanted to export both the non volatile and volatile memory addresses for > > wiper > > position access. That is bare minimum for the poti to operate. > > > > But I also wanted to export additional features of this chip. That is way > > there > > is increase and decrease API, and STATUS and TCON register access. > > > > The important part about a framework and the associated device drivers > is to expose the features of a device using a standardized interface so > you can write generic applications/libraries and share infrastructure. > If an application requires device specific knowledge to access the > features of a device you may as well write a userspace driver using i2cdev. > > So when you are introducing new ABI it should at least follow the > standard naming scheme. And also try to think whether this is a feature > that is present in other similar devices and come up with a device > independent way to expose this functionality. > > Let's start with the simple stuff, I don't really see the advantage of > having separate inc/dec controls. This can be handled through the > standard raw attribute. If the newly written value is one off from the > previous one use inc/dec otherwise write it directly. And even then it > might make sense to just ignore that and always write the raw value. I've got your point. The version 2 will use only the IIO facilities. Then I will try to build more based on that. > > The memory_map API is a way to access all the not used by chip memory > > addresses. > > This API I think could be deleted. But I still think that some people might > > find > > it useful. > > This sounds more like it should maybe be exposed as a standard EEPROM > device. Not quite familiar with this, but will look into that. Once more thank you for comments. -- Slawomir Stepien
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On Mar 16, 2016 19:24, Lars-Peter Clausen wrote: > On 03/16/2016 05:25 PM, Slawomir Stepien wrote: > > On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote: > [...] > >> plenty of the private API, some of which seems to be debug only? > >> what is really needed to interact with a poti? > > > > I wanted to export both the non volatile and volatile memory addresses for > > wiper > > position access. That is bare minimum for the poti to operate. > > > > But I also wanted to export additional features of this chip. That is way > > there > > is increase and decrease API, and STATUS and TCON register access. > > > > The important part about a framework and the associated device drivers > is to expose the features of a device using a standardized interface so > you can write generic applications/libraries and share infrastructure. > If an application requires device specific knowledge to access the > features of a device you may as well write a userspace driver using i2cdev. > > So when you are introducing new ABI it should at least follow the > standard naming scheme. And also try to think whether this is a feature > that is present in other similar devices and come up with a device > independent way to expose this functionality. > > Let's start with the simple stuff, I don't really see the advantage of > having separate inc/dec controls. This can be handled through the > standard raw attribute. If the newly written value is one off from the > previous one use inc/dec otherwise write it directly. And even then it > might make sense to just ignore that and always write the raw value. I've got your point. The version 2 will use only the IIO facilities. Then I will try to build more based on that. > > The memory_map API is a way to access all the not used by chip memory > > addresses. > > This API I think could be deleted. But I still think that some people might > > find > > it useful. > > This sounds more like it should maybe be exposed as a standard EEPROM > device. Not quite familiar with this, but will look into that. Once more thank you for comments. -- Slawomir Stepien