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
