Re: [PATCH v3] iio: max5487: Add support for Maxim digital potentiometers

2016-05-18 Thread Matt Ranostay
Slight nitpick below.

On Tue, May 17, 2016 at 6:11 AM, Cristina Moraru
 wrote:
> Add implementation for Maxim MAX5487, MAX5488, MAX5489
> digital potentiometers.
>
> Datasheet:
> http://datasheets.maximintegrated.com/en/ds/MAX5487-MAX5489.pdf
>
> Signed-off-by: Cristina Moraru 
> CC: Daniel Baluta 
> ---
> Changes since v2:
> Change switch statement in max5487_write_raw
> to if statement for consistency
> Add blank line before return statement
> Eliminate regmap support and use spi_write
> Use iio_device_register instead of devm_ version
> Changes since v1:
> Fix spacing
> Eliminate max5487_cfg struct
> Add kohms as driver data
>
>  drivers/iio/potentiometer/Kconfig   |  11 +++
>  drivers/iio/potentiometer/Makefile  |   1 +
>  drivers/iio/potentiometer/max5487.c | 162 
> 
>  3 files changed, 174 insertions(+)
>  create mode 100644 drivers/iio/potentiometer/max5487.c
>
> diff --git a/drivers/iio/potentiometer/Kconfig 
> b/drivers/iio/potentiometer/Kconfig
> index 6acb238..bb77b50 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -15,6 +15,17 @@ config DS1803
>   To compile this driver as a module, choose M here: the
>   module will be called ds1803.
>
> +config MAX5487
> +tristate "Maxim MAX5487/MAX5488/MAX5489 Digital Potentiometer driver"
> +depends on SPI
> +help
> +  Say yes here to build support for the Maxim
> +  MAX5487, MAX5488, MAX5489 digital potentiomenter
> +  chips.
> +
> +  To compile this driver as a module, choose M here: the
> +  module will be called max5487.
> +
>  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 6007faa..8adb58f 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -4,6 +4,7 @@
>
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_DS1803) += ds1803.o
> +obj-$(CONFIG_MAX5487) += max5487.o
>  obj-$(CONFIG_MCP4131) += mcp4131.o
>  obj-$(CONFIG_MCP4531) += mcp4531.o
>  obj-$(CONFIG_TPL0102) += tpl0102.o
> diff --git a/drivers/iio/potentiometer/max5487.c 
> b/drivers/iio/potentiometer/max5487.c
> new file mode 100644
> index 000..d5c9089
> --- /dev/null
> +++ b/drivers/iio/potentiometer/max5487.c
> @@ -0,0 +1,162 @@
> +/*
> + * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
> + *
> + * Copyright (C) Cristina-Gabriela Moraru 

You'll want to put a year for the copyright.

> + *
> + * 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 
> +
> +#define MAX5487_WRITE_WIPER_A  (0x01 << 8)
> +#define MAX5487_WRITE_WIPER_B  (0x02 << 8)
> +
> +/* copy both wiper regs to NV regs */
> +#define MAX5487_COPY_AB_TO_NV  (0x23 << 8)
> +/* copy both NV regs to wiper regs */
> +#define MAX5487_COPY_NV_TO_AB  (0x33 << 8)
> +
> +#define MAX5487_MAX_POS255
> +#define MAX5487_REG_SIZE   16
> +
> +struct max5487_data {
> +   struct spi_device *spi;
> +   int kohms;
> +};
> +
> +#define MAX5487_CHANNEL(ch, addr) {\
> +   .type = IIO_RESISTANCE, \
> +   .indexed = 1,   \
> +   .output = 1,\
> +   .channel = ch,  \
> +   .address = addr,\
> +   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   \
> +   .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> +}
> +
> +static const struct iio_chan_spec max5487_channels[] = {
> +   MAX5487_CHANNEL(0, MAX5487_WRITE_WIPER_A),
> +   MAX5487_CHANNEL(1, MAX5487_WRITE_WIPER_B),
> +};
> +
> +static int max5487_write_cmd(struct spi_device *spi, int cmd)
> +{
> +   return spi_write(spi, (const void *) &cmd, MAX5487_REG_SIZE);
> +}
> +
> +static int max5487_read_raw(struct iio_dev *indio_dev,
> +   struct iio_chan_spec const *chan,
> +   int *val, int *val2, long mask)
> +{
> +   struct max5487_data *data = iio_priv(indio_dev);
> +
> +   if (mask != IIO_CHAN_INFO_SCALE)
> +   return -EINVAL;
> +
> +   *val = 1000 * data->kohms;
> +   *val2 = MAX5487_MAX_POS;
> +
> +   return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int max5487_write_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int 

Re: [PATCH v3] iio: max5487: Add support for Maxim digital potentiometers

2016-05-17 Thread Peter Meerwald-Stadler

> Add implementation for Maxim MAX5487, MAX5488, MAX5489
> digital potentiometers.

comments below
 
> Datasheet:
> http://datasheets.maximintegrated.com/en/ds/MAX5487-MAX5489.pdf
> 
> Signed-off-by: Cristina Moraru 
> CC: Daniel Baluta 
> ---
> Changes since v2:
> Change switch statement in max5487_write_raw
> to if statement for consistency
> Add blank line before return statement
> Eliminate regmap support and use spi_write
> Use iio_device_register instead of devm_ version
> Changes since v1:
> Fix spacing
> Eliminate max5487_cfg struct
> Add kohms as driver data
> 
>  drivers/iio/potentiometer/Kconfig   |  11 +++
>  drivers/iio/potentiometer/Makefile  |   1 +
>  drivers/iio/potentiometer/max5487.c | 162 
> 
>  3 files changed, 174 insertions(+)
>  create mode 100644 drivers/iio/potentiometer/max5487.c
> 
> diff --git a/drivers/iio/potentiometer/Kconfig 
> b/drivers/iio/potentiometer/Kconfig
> index 6acb238..bb77b50 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -15,6 +15,17 @@ config DS1803
> To compile this driver as a module, choose M here: the
> module will be called ds1803.
>  
> +config MAX5487
> +tristate "Maxim MAX5487/MAX5488/MAX5489 Digital Potentiometer driver"
> +depends on SPI
> +help
> +  Say yes here to build support for the Maxim
> +  MAX5487, MAX5488, MAX5489 digital potentiomenter

spelling: potentiometer

> +  chips.
> +
> +  To compile this driver as a module, choose M here: the
> +  module will be called max5487.
> +
>  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 6007faa..8adb58f 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -4,6 +4,7 @@
>  
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_DS1803) += ds1803.o
> +obj-$(CONFIG_MAX5487) += max5487.o
>  obj-$(CONFIG_MCP4131) += mcp4131.o
>  obj-$(CONFIG_MCP4531) += mcp4531.o
>  obj-$(CONFIG_TPL0102) += tpl0102.o
> diff --git a/drivers/iio/potentiometer/max5487.c 
> b/drivers/iio/potentiometer/max5487.c
> new file mode 100644
> index 000..d5c9089
> --- /dev/null
> +++ b/drivers/iio/potentiometer/max5487.c
> @@ -0,0 +1,162 @@
> +/*
> + * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
> + *
> + * Copyright (C) Cristina-Gabriela Moraru 
> + *
> + * 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 
> +
> +#define MAX5487_WRITE_WIPER_A(0x01 << 8)
> +#define MAX5487_WRITE_WIPER_B(0x02 << 8)
> +
> +/* copy both wiper regs to NV regs */
> +#define MAX5487_COPY_AB_TO_NV(0x23 << 8)
> +/* copy both NV regs to wiper regs */
> +#define MAX5487_COPY_NV_TO_AB(0x33 << 8)
> +
> +#define MAX5487_MAX_POS  255
> +#define MAX5487_REG_SIZE 16
> +
> +struct max5487_data {
> + struct spi_device *spi;
> + int kohms;
> +};
> +
> +#define MAX5487_CHANNEL(ch, addr) {  \
> + .type = IIO_RESISTANCE, \
> + .indexed = 1,   \
> + .output = 1,\
> + .channel = ch,  \
> + .address = addr,\
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> +}
> +
> +static const struct iio_chan_spec max5487_channels[] = {
> + MAX5487_CHANNEL(0, MAX5487_WRITE_WIPER_A),
> + MAX5487_CHANNEL(1, MAX5487_WRITE_WIPER_B),
> +};
> +
> +static int max5487_write_cmd(struct spi_device *spi, int cmd)
> +{
> + return spi_write(spi, (const void *) &cmd, MAX5487_REG_SIZE);

_REG_SIZE is 16, sizeof(cmd) is likely 4 or 8?!
maybe cmd should be u16?
maybe REG_SIZE should indicate bytes, not bits?

> +}
> +
> +static int max5487_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct max5487_data *data = iio_priv(indio_dev);
> +
> + if (mask != IIO_CHAN_INFO_SCALE)
> + return -EINVAL;
> +
> + *val = 1000 * data->kohms;
> + *val2 = MAX5487_MAX_POS;
> +
> + return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int max5487_write_raw(struct iio_dev *indio_dev,
> +  struct iio_chan_spec const *chan,
> +  

[PATCH v3] iio: max5487: Add support for Maxim digital potentiometers

2016-05-17 Thread Cristina Moraru
Add implementation for Maxim MAX5487, MAX5488, MAX5489
digital potentiometers.

Datasheet:
http://datasheets.maximintegrated.com/en/ds/MAX5487-MAX5489.pdf

Signed-off-by: Cristina Moraru 
CC: Daniel Baluta 
---
Changes since v2:
Change switch statement in max5487_write_raw
to if statement for consistency
Add blank line before return statement
Eliminate regmap support and use spi_write
Use iio_device_register instead of devm_ version
Changes since v1:
Fix spacing
Eliminate max5487_cfg struct
Add kohms as driver data

 drivers/iio/potentiometer/Kconfig   |  11 +++
 drivers/iio/potentiometer/Makefile  |   1 +
 drivers/iio/potentiometer/max5487.c | 162 
 3 files changed, 174 insertions(+)
 create mode 100644 drivers/iio/potentiometer/max5487.c

diff --git a/drivers/iio/potentiometer/Kconfig 
b/drivers/iio/potentiometer/Kconfig
index 6acb238..bb77b50 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -15,6 +15,17 @@ config DS1803
  To compile this driver as a module, choose M here: the
  module will be called ds1803.
 
+config MAX5487
+tristate "Maxim MAX5487/MAX5488/MAX5489 Digital Potentiometer driver"
+depends on SPI
+help
+  Say yes here to build support for the Maxim
+  MAX5487, MAX5488, MAX5489 digital potentiomenter
+  chips.
+
+  To compile this driver as a module, choose M here: the
+  module will be called max5487.
+
 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 6007faa..8adb58f 100644
--- a/drivers/iio/potentiometer/Makefile
+++ b/drivers/iio/potentiometer/Makefile
@@ -4,6 +4,7 @@
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_DS1803) += ds1803.o
+obj-$(CONFIG_MAX5487) += max5487.o
 obj-$(CONFIG_MCP4131) += mcp4131.o
 obj-$(CONFIG_MCP4531) += mcp4531.o
 obj-$(CONFIG_TPL0102) += tpl0102.o
diff --git a/drivers/iio/potentiometer/max5487.c 
b/drivers/iio/potentiometer/max5487.c
new file mode 100644
index 000..d5c9089
--- /dev/null
+++ b/drivers/iio/potentiometer/max5487.c
@@ -0,0 +1,162 @@
+/*
+ * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
+ *
+ * Copyright (C) Cristina-Gabriela Moraru 
+ *
+ * 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 
+
+#define MAX5487_WRITE_WIPER_A  (0x01 << 8)
+#define MAX5487_WRITE_WIPER_B  (0x02 << 8)
+
+/* copy both wiper regs to NV regs */
+#define MAX5487_COPY_AB_TO_NV  (0x23 << 8)
+/* copy both NV regs to wiper regs */
+#define MAX5487_COPY_NV_TO_AB  (0x33 << 8)
+
+#define MAX5487_MAX_POS255
+#define MAX5487_REG_SIZE   16
+
+struct max5487_data {
+   struct spi_device *spi;
+   int kohms;
+};
+
+#define MAX5487_CHANNEL(ch, addr) {\
+   .type = IIO_RESISTANCE, \
+   .indexed = 1,   \
+   .output = 1,\
+   .channel = ch,  \
+   .address = addr,\
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   \
+   .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
+}
+
+static const struct iio_chan_spec max5487_channels[] = {
+   MAX5487_CHANNEL(0, MAX5487_WRITE_WIPER_A),
+   MAX5487_CHANNEL(1, MAX5487_WRITE_WIPER_B),
+};
+
+static int max5487_write_cmd(struct spi_device *spi, int cmd)
+{
+   return spi_write(spi, (const void *) &cmd, MAX5487_REG_SIZE);
+}
+
+static int max5487_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *chan,
+   int *val, int *val2, long mask)
+{
+   struct max5487_data *data = iio_priv(indio_dev);
+
+   if (mask != IIO_CHAN_INFO_SCALE)
+   return -EINVAL;
+
+   *val = 1000 * data->kohms;
+   *val2 = MAX5487_MAX_POS;
+
+   return IIO_VAL_FRACTIONAL;
+}
+
+static int max5487_write_raw(struct iio_dev *indio_dev,
+struct iio_chan_spec const *chan,
+int val, int val2, long mask)
+{
+   struct max5487_data *data = iio_priv(indio_dev);
+
+   if (mask != IIO_CHAN_INFO_RAW)
+   return -EINVAL;
+
+   if (val < 0 || val > MAX5487_MAX_POS)
+   return -EINVAL;
+
+   return max5487_write_cmd(data->spi, chan->address | val);
+}
+
+static const struct iio_info max5487_info = {
+   .read_raw = max5487_read_raw,
+