linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iio: max5487: Add support for Maxim digital potentiometers
@ 2016-03-24 11:21 Cristina Moraru
  2016-03-24 11:42 ` Daniel Baluta
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Cristina Moraru @ 2016-03-24 11:21 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw, peda, linux-kernel, linux-iio,
	daniel.baluta, octavian.purdila
  Cc: Cristina Moraru

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

Signed-off-by: Cristina Moraru <cristina.moraru09@gmail.com>
CC: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/potentiometer/Kconfig   |  11 +++
 drivers/iio/potentiometer/Makefile  |   1 +
 drivers/iio/potentiometer/max5487.c | 185 ++++++++++++++++++++++++++++++++++++
 3 files changed, 197 insertions(+)
 create mode 100644 drivers/iio/potentiometer/max5487.c

diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index ffc735c..3046c79 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -5,6 +5,17 @@
 
 menu "Digital potentiometers"
 
+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 MCP4531
 	tristate "Microchip MCP45xx/MCP46xx Digital Potentiometer driver"
 	depends on I2C
diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
index b563b49..dcc791a 100644
--- a/drivers/iio/potentiometer/Makefile
+++ b/drivers/iio/potentiometer/Makefile
@@ -3,5 +3,6 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_MAX5487) += max5487.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 0000000..69db979
--- /dev/null
+++ b/drivers/iio/potentiometer/max5487.c
@@ -0,0 +1,185 @@
+/*
+ * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
+ *
+ * Copyright (C) Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>
+ *
+ * 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 <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/acpi.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/sysfs.h>
+#include <linux/iio/iio.h>
+
+#define MAX5487_DRV_NAME "max5487"
+
+#define MAX5487_WRITE_WIPER_A	0x01
+#define MAX5487_WRITE_WIPER_B	0x02
+
+/* copy both wiper regs to NV regs */
+#define MAX5487_COPY_AB_TO_NV	0x23
+/* copy both NV regs to wiper regs */
+#define MAX5487_COPY_NV_TO_AB	0x33
+
+enum {
+	MAX5487,
+	MAX5488,
+	MAX5489,
+};
+
+struct max5487_cfg {
+	int wipers;
+	int max_pos;
+	int kohms;
+};
+
+static const struct max5487_cfg max5487_cfg[] = {
+	[MAX5487] = { .wipers = 2, .max_pos = 256, .kohms =  10,},
+	[MAX5488] = { .wipers = 2, .max_pos = 256, .kohms =  50,},
+	[MAX5489] = { .wipers = 2, .max_pos = 256, .kohms =  100,}
+};
+
+struct max5487_data {
+	struct regmap *regmap;
+	int chip_id;
+};
+
+#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_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 * max5487_cfg[data->chip_id].kohms;
+	*val2 = max5487_cfg[data->chip_id].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);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val < 0 || val >= max5487_cfg[data->chip_id].max_pos)
+			return -EINVAL;
+		return regmap_write(data->regmap, chan->address, val);
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
+static const struct iio_info max5487_info = {
+	.read_raw = &max5487_read_raw,
+	.write_raw = &max5487_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static const struct regmap_config max5487_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = MAX5487_COPY_NV_TO_AB,
+};
+
+static int max5487_spi_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct max5487_data *data;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	dev_set_drvdata(&spi->dev, indio_dev);
+	data = iio_priv(indio_dev);
+
+	data->regmap = devm_regmap_init_spi(spi, &max5487_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	data->chip_id = id->driver_data;
+
+	indio_dev->info = &max5487_info;
+	indio_dev->name = id->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = max5487_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
+
+	/* restore both wiper regs from NV regs */
+	ret = regmap_write(data->regmap, MAX5487_COPY_NV_TO_AB, 0);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static int max5487_spi_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
+	struct max5487_data *data = iio_priv(indio_dev);
+
+	/* save both wiper regs to NV regs */
+	return regmap_write(data->regmap, MAX5487_COPY_AB_TO_NV, 0);
+}
+
+static const struct spi_device_id max5487_id[] = {
+	{ "MAX5487", MAX5487 },
+	{ "MAX5488", MAX5488 },
+	{ "MAX5489", MAX5489 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, max5487_id);
+
+static const struct acpi_device_id max5487_acpi_match[] = {
+	{ "MAX5487", MAX5487 },
+	{ "MAX5488", MAX5488 },
+	{ "MAX5489", MAX5489 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
+
+static struct spi_driver max5487_driver = {
+	.driver = {
+		.name = MAX5487_DRV_NAME,
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(max5487_acpi_match),
+	},
+	.id_table = max5487_id,
+	.probe = max5487_spi_probe,
+	.remove = max5487_spi_remove
+};
+module_spi_driver(max5487_driver);
+
+MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
+MODULE_DESCRIPTION("max5487 SPI driver");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] iio: max5487: Add support for Maxim digital potentiometers
  2016-03-24 11:21 [PATCH] iio: max5487: Add support for Maxim digital potentiometers Cristina Moraru
@ 2016-03-24 11:42 ` Daniel Baluta
  2016-03-24 21:46 ` Peter Rosin
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Daniel Baluta @ 2016-03-24 11:42 UTC (permalink / raw)
  To: Cristina Moraru
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Peter Rosin, Linux Kernel Mailing List,
	linux-iio, Daniel Baluta, octavian.purdila

On Thu, Mar 24, 2016 at 1:21 PM, Cristina Moraru
<cristina.moraru09@gmail.com> 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 <cristina.moraru09@gmail.com>
> CC: Daniel Baluta <daniel.baluta@intel.com>

Tested-by: Daniel Baluta <daniel.baluta@intel.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH] iio: max5487: Add support for Maxim digital potentiometers
  2016-03-24 11:21 [PATCH] iio: max5487: Add support for Maxim digital potentiometers Cristina Moraru
  2016-03-24 11:42 ` Daniel Baluta
@ 2016-03-24 21:46 ` Peter Rosin
  2016-04-01  8:28   ` Daniel Baluta
  2016-03-25 10:20 ` Peter Rosin
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Peter Rosin @ 2016-03-24 21:46 UTC (permalink / raw)
  To: Cristina Moraru, jic23, knaack.h, lars, pmeerw, linux-kernel,
	linux-iio, daniel.baluta, octavian.purdila

Hi Cristina,

Some comments inline...

Cheers,
Peter

Cristina Moraru wrote:
> Add implementation for Maxim MAX5487, MAX5488, MAX5489
> digital potentiometers.
> 
> Signed-off-by: Cristina Moraru <cristina.moraru09@gmail.com>
> CC: Daniel Baluta <daniel.baluta@intel.com>
> ---
>  drivers/iio/potentiometer/Kconfig   |  11 +++
>  drivers/iio/potentiometer/Makefile  |   1 +
>  drivers/iio/potentiometer/max5487.c | 185 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 197 insertions(+)
>  create mode 100644 drivers/iio/potentiometer/max5487.c
> 
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index ffc735c..3046c79 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -5,6 +5,17 @@
>  
>  menu "Digital potentiometers"
>  
> +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.

The whitespace on this line is different.

> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called max5487.
> +
>  config MCP4531
>  	tristate "Microchip MCP45xx/MCP46xx Digital Potentiometer driver"
>  	depends on I2C
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index b563b49..dcc791a 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -3,5 +3,6 @@
>  #
>  
>  # When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_MAX5487) += max5487.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 0000000..69db979
> --- /dev/null
> +++ b/drivers/iio/potentiometer/max5487.c
> @@ -0,0 +1,185 @@
> +/*
> + * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
> + *
> + * Copyright (C) Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>
> + *
> + * 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 <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/acpi.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/iio.h>
> +
> +#define MAX5487_DRV_NAME "max5487"
> +
> +#define MAX5487_WRITE_WIPER_A	0x01
> +#define MAX5487_WRITE_WIPER_B	0x02
> +
> +/* copy both wiper regs to NV regs */
> +#define MAX5487_COPY_AB_TO_NV	0x23
> +/* copy both NV regs to wiper regs */
> +#define MAX5487_COPY_NV_TO_AB	0x33
> +
> +enum {
> +	MAX5487,
> +	MAX5488,
> +	MAX5489,
> +};
> +
> +struct max5487_cfg {
> +	int wipers;
> +	int max_pos;
> +	int kohms;
> +};
> +
> +static const struct max5487_cfg max5487_cfg[] = {
> +	[MAX5487] = { .wipers = 2, .max_pos = 256, .kohms =  10,},
> +	[MAX5488] = { .wipers = 2, .max_pos = 256, .kohms =  50,},
> +	[MAX5489] = { .wipers = 2, .max_pos = 256, .kohms =  100,}
> +};

.wipers and .max_pos need not be in max5487_cfg, they are common.
.wipers isn't even used. Which means that if you like, you can
use the ohms reading as the driver_data directly instead of going
via the MAX548x enumeration, see below. Or is there some reason
not doing so?

> +
> +struct max5487_data {
> +	struct regmap *regmap;
> +	int chip_id;

I.e., change chip_id to 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_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 * max5487_cfg[data->chip_id].kohms;

Use data->kohms here.

> +	*val2 = max5487_cfg[data->chip_id].max_pos;

Hardcode this to 256 (using a define).

> +	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);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val < 0 || val >= max5487_cfg[data->chip_id].max_pos)

Hardcode to 256.

> +			return -EINVAL;
> +		return regmap_write(data->regmap, chan->address, val);
> +	default:
> +		return -EINVAL;
> +	}
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info max5487_info = {
> +	.read_raw = &max5487_read_raw,
> +	.write_raw = &max5487_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static const struct regmap_config max5487_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = MAX5487_COPY_NV_TO_AB,
> +};
> +
> +static int max5487_spi_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max5487_data *data;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&spi->dev, indio_dev);
> +	data = iio_priv(indio_dev);
> +
> +	data->regmap = devm_regmap_init_spi(spi, &max5487_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	data->chip_id = id->driver_data;

Use data->kohms = id->driver_data

> +
> +	indio_dev->info = &max5487_info;
> +	indio_dev->name = id->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = max5487_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
> +
> +	/* restore both wiper regs from NV regs */
> +	ret = regmap_write(data->regmap, MAX5487_COPY_NV_TO_AB, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static int max5487_spi_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> +	struct max5487_data *data = iio_priv(indio_dev);
> +
> +	/* save both wiper regs to NV regs */
> +	return regmap_write(data->regmap, MAX5487_COPY_AB_TO_NV, 0);
> +}
> +
> +static const struct spi_device_id max5487_id[] = {
> +	{ "MAX5487", MAX5487 },
> +	{ "MAX5488", MAX5488 },
> +	{ "MAX5489", MAX5489 },

Use 10, 50, and 100 instead of the MAX548x enums.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, max5487_id);
> +
> +static const struct acpi_device_id max5487_acpi_match[] = {
> +	{ "MAX5487", MAX5487 },
> +	{ "MAX5488", MAX5488 },
> +	{ "MAX5489", MAX5489 },

Dito.

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
> +
> +static struct spi_driver max5487_driver = {
> +	.driver = {
> +		.name = MAX5487_DRV_NAME,
> +		.owner = THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(max5487_acpi_match),
> +	},
> +	.id_table = max5487_id,
> +	.probe = max5487_spi_probe,
> +	.remove = max5487_spi_remove
> +};
> +module_spi_driver(max5487_driver);
> +
> +MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
> +MODULE_DESCRIPTION("max5487 SPI driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.5.0
> 
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH] iio: max5487: Add support for Maxim digital potentiometers
  2016-03-24 11:21 [PATCH] iio: max5487: Add support for Maxim digital potentiometers Cristina Moraru
  2016-03-24 11:42 ` Daniel Baluta
  2016-03-24 21:46 ` Peter Rosin
@ 2016-03-25 10:20 ` Peter Rosin
  2016-04-01  8:33   ` Daniel Baluta
  2016-04-09  8:24 ` [PATCH v2] " Cristina Moraru
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Peter Rosin @ 2016-03-25 10:20 UTC (permalink / raw)
  To: Cristina Moraru, jic23, knaack.h, lars, pmeerw, linux-kernel,
	linux-iio, daniel.baluta, octavian.purdila

Hi again,

Cristina Moraru wrote:
> Add implementation for Maxim MAX5487, MAX5488, MAX5489
> digital potentiometers.
> 
> Signed-off-by: Cristina Moraru <cristina.moraru09@gmail.com>
> CC: Daniel Baluta <daniel.baluta@intel.com>

Some more comments, the mcp4531 chips have n**2 + 1 positions,
therefore .max_pos in that driver isn't the number of wiper positions, it's
the actual maximum value. So, in this driver, the corrent number for
.max_pos would be 255, otherwise the reported scale is wrong (and then
you also need to adjust the EINVAL check in max5487_write_raw to use >
instead of >=).

Further comparison with the mcp4531 driver reveals that this driver does
not support IIO_CHAN_INFO_RAW in max5487_read_raw. I assume the SPI
interface does not support reading back the current value?

Cheers,
Peter

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] iio: max5487: Add support for Maxim digital potentiometers
  2016-03-24 21:46 ` Peter Rosin
@ 2016-04-01  8:28   ` Daniel Baluta
  2016-04-03  9:25     ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: Daniel Baluta @ 2016-04-01  8:28 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Cristina Moraru, jic23, knaack.h, lars, pmeerw, linux-kernel,
	linux-iio, daniel.baluta, octavian.purdila

> .wipers and .max_pos need not be in max5487_cfg, they are common.
> .wipers isn't even used. Which means that if you like, you can
> use the ohms reading as the driver_data directly instead of going
> via the MAX548x enumeration, see below. Or is there some reason
> not doing so?

You make a good point. Anyhow we thought of a generic approach. In the future
this chip family could support more wipers with different positions.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] iio: max5487: Add support for Maxim digital potentiometers
  2016-03-25 10:20 ` Peter Rosin
@ 2016-04-01  8:33   ` Daniel Baluta
  0 siblings, 0 replies; 17+ messages in thread
From: Daniel Baluta @ 2016-04-01  8:33 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Cristina Moraru, jic23, knaack.h, lars, pmeerw, linux-kernel,
	linux-iio, daniel.baluta, octavian.purdila

On Fri, Mar 25, 2016 at 12:20 PM, Peter Rosin <peda@axentia.se> wrote:
> Hi again,
>
> Cristina Moraru wrote:
>> Add implementation for Maxim MAX5487, MAX5488, MAX5489
>> digital potentiometers.
>>
>> Signed-off-by: Cristina Moraru <cristina.moraru09@gmail.com>
>> CC: Daniel Baluta <daniel.baluta@intel.com>
>
> Some more comments, the mcp4531 chips have n**2 + 1 positions,
> therefore .max_pos in that driver isn't the number of wiper positions, it's
> the actual maximum value. So, in this driver, the corrent number for
> .max_pos would be 255, otherwise the reported scale is wrong (and then
> you also need to adjust the EINVAL check in max5487_write_raw to use >
> instead of >=).
>
> Further comparison with the mcp4531 driver reveals that this driver does
> not support IIO_CHAN_INFO_RAW in max5487_read_raw. I assume the SPI
> interface does not support reading back the current value?

Yes. The registers are write only. We could have used the caching
facility of regmap with default values.

The problem is when using the non volatile (NV) mem there is no way to know
the previous wiper position.

Daniel.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] iio: max5487: Add support for Maxim digital potentiometers
  2016-04-01  8:28   ` Daniel Baluta
@ 2016-04-03  9:25     ` Jonathan Cameron
  0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2016-04-03  9:25 UTC (permalink / raw)
  To: Daniel Baluta, Peter Rosin
  Cc: Cristina Moraru, knaack.h, lars, pmeerw, linux-kernel, linux-iio,
	octavian.purdila

On 01/04/16 09:28, Daniel Baluta wrote:
>> .wipers and .max_pos need not be in max5487_cfg, they are common.
>> .wipers isn't even used. Which means that if you like, you can
>> use the ohms reading as the driver_data directly instead of going
>> via the MAX548x enumeration, see below. Or is there some reason
>> not doing so?
> 
> You make a good point. Anyhow we thought of a generic approach. In the future
> this chip family could support more wipers with different positions.
Make it generic when you need too.  Easy enough to add later!

J
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers
  2016-03-24 11:21 [PATCH] iio: max5487: Add support for Maxim digital potentiometers Cristina Moraru
                   ` (2 preceding siblings ...)
  2016-03-25 10:20 ` Peter Rosin
@ 2016-04-09  8:24 ` Cristina Moraru
  2016-04-10 11:24   ` Jonathan Cameron
  2016-04-10 12:47   ` Joachim Eastwood
  2016-05-17 13:11 ` [PATCH v3] " Cristina Moraru
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 17+ messages in thread
From: Cristina Moraru @ 2016-04-09  8:24 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw, peda, linux-kernel, linux-iio,
	daniel.baluta, octavian.purdila
  Cc: 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 <cristina.moraru09@gmail.com>
CC: Daniel Baluta <daniel.baluta@intel.com>
---
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 | 169 ++++++++++++++++++++++++++++++++++++
 3 files changed, 181 insertions(+)
 create mode 100644 drivers/iio/potentiometer/max5487.c

diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index fd75db7..242c62d 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -5,6 +5,17 @@
 
 menu "Digital potentiometers"
 
+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 MCP4531
 	tristate "Microchip MCP45xx/MCP46xx Digital Potentiometer driver"
 	depends on I2C
diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
index 8afe492..bc2dfd8 100644
--- a/drivers/iio/potentiometer/Makefile
+++ b/drivers/iio/potentiometer/Makefile
@@ -3,4 +3,5 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_MAX5487) += max5487.o
 obj-$(CONFIG_MCP4531) += mcp4531.o
diff --git a/drivers/iio/potentiometer/max5487.c b/drivers/iio/potentiometer/max5487.c
new file mode 100644
index 0000000..fa455b8
--- /dev/null
+++ b/drivers/iio/potentiometer/max5487.c
@@ -0,0 +1,169 @@
+/*
+ * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
+ *
+ * Copyright (C) Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>
+ *
+ * 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 <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/acpi.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/sysfs.h>
+#include <linux/iio/iio.h>
+
+#define MAX5487_DRV_NAME "max5487"
+
+#define MAX5487_WRITE_WIPER_A	0x01
+#define MAX5487_WRITE_WIPER_B	0x02
+
+/* copy both wiper regs to NV regs */
+#define MAX5487_COPY_AB_TO_NV	0x23
+/* copy both NV regs to wiper regs */
+#define MAX5487_COPY_NV_TO_AB	0x33
+
+#define MAX5487_MAX_POS		255
+
+struct max5487_data {
+	struct regmap *regmap;
+	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_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);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val < 0 || val > MAX5487_MAX_POS)
+			return -EINVAL;
+		return regmap_write(data->regmap, chan->address, val);
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
+static const struct iio_info max5487_info = {
+	.read_raw = &max5487_read_raw,
+	.write_raw = &max5487_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static const struct regmap_config max5487_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = MAX5487_COPY_NV_TO_AB,
+};
+
+static int max5487_spi_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct max5487_data *data;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	dev_set_drvdata(&spi->dev, indio_dev);
+	data = iio_priv(indio_dev);
+
+	data->regmap = devm_regmap_init_spi(spi, &max5487_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	data->kohms = id->driver_data;
+
+	indio_dev->info = &max5487_info;
+	indio_dev->name = id->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = max5487_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
+
+	/* restore both wiper regs from NV regs */
+	ret = regmap_write(data->regmap, MAX5487_COPY_NV_TO_AB, 0);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static int max5487_spi_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
+	struct max5487_data *data = iio_priv(indio_dev);
+
+	/* save both wiper regs to NV regs */
+	return regmap_write(data->regmap, MAX5487_COPY_AB_TO_NV, 0);
+}
+
+static const struct spi_device_id max5487_id[] = {
+	{ "MAX5487", 10 },
+	{ "MAX5488", 50 },
+	{ "MAX5489", 100 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, max5487_id);
+
+static const struct acpi_device_id max5487_acpi_match[] = {
+	{ "MAX5487", 10 },
+	{ "MAX5488", 50 },
+	{ "MAX5489", 100 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
+
+static struct spi_driver max5487_driver = {
+	.driver = {
+		.name = MAX5487_DRV_NAME,
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(max5487_acpi_match),
+	},
+	.id_table = max5487_id,
+	.probe = max5487_spi_probe,
+	.remove = max5487_spi_remove
+};
+module_spi_driver(max5487_driver);
+
+MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
+MODULE_DESCRIPTION("max5487 SPI driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers
  2016-04-09  8:24 ` [PATCH v2] " Cristina Moraru
@ 2016-04-10 11:24   ` Jonathan Cameron
  2016-04-10 12:47   ` Joachim Eastwood
  1 sibling, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2016-04-10 11:24 UTC (permalink / raw)
  To: Cristina Moraru, knaack.h, lars, pmeerw, peda, linux-kernel,
	linux-iio, daniel.baluta, octavian.purdila

On 09/04/16 09:24, 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 <cristina.moraru09@gmail.com>
> CC: Daniel Baluta <daniel.baluta@intel.com>
Looks pretty good.  A few nitpicks + a somewhat theoretical race condition
in the remove function due to use of devm_iio_device_register.
Rule of thumb is you can't use this unless you have no remove function at all.
(less obviously than normal in this case though!)

Jonathan
> ---
> 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 | 169 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
>  create mode 100644 drivers/iio/potentiometer/max5487.c
> 
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index fd75db7..242c62d 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -5,6 +5,17 @@
>  
>  menu "Digital potentiometers"
>  
> +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 MCP4531
>  	tristate "Microchip MCP45xx/MCP46xx Digital Potentiometer driver"
>  	depends on I2C
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index 8afe492..bc2dfd8 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -3,4 +3,5 @@
>  #
>  
>  # When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_MAX5487) += max5487.o
>  obj-$(CONFIG_MCP4531) += mcp4531.o
> diff --git a/drivers/iio/potentiometer/max5487.c b/drivers/iio/potentiometer/max5487.c
> new file mode 100644
> index 0000000..fa455b8
> --- /dev/null
> +++ b/drivers/iio/potentiometer/max5487.c
> @@ -0,0 +1,169 @@
> +/*
> + * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
> + *
> + * Copyright (C) Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>
> + *
> + * 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 <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/acpi.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/iio.h>
> +
> +#define MAX5487_DRV_NAME "max5487"
This is one of my pet hates ;)  Why have this in a define at the top
of the driver if it is used in only one place - and that place is where
I would expect to look to see what that the driver is called.
Still I don't care that much if you want to leave it here.  Just being fussy.
> +
> +#define MAX5487_WRITE_WIPER_A	0x01
> +#define MAX5487_WRITE_WIPER_B	0x02
> +
> +/* copy both wiper regs to NV regs */
> +#define MAX5487_COPY_AB_TO_NV	0x23
> +/* copy both NV regs to wiper regs */
> +#define MAX5487_COPY_NV_TO_AB	0x33
> +
> +#define MAX5487_MAX_POS		255
> +
> +struct max5487_data {
> +	struct regmap *regmap;
> +	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_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;
Blank line here marginally preferred.
> +	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);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val < 0 || val > MAX5487_MAX_POS)
> +			return -EINVAL;
> +		return regmap_write(data->regmap, chan->address, val);
> +	default:
> +		return -EINVAL;
> +	}
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info max5487_info = {
> +	.read_raw = &max5487_read_raw,
> +	.write_raw = &max5487_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static const struct regmap_config max5487_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = MAX5487_COPY_NV_TO_AB,
> +};
> +
> +static int max5487_spi_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max5487_data *data;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&spi->dev, indio_dev);
> +	data = iio_priv(indio_dev);
> +
> +	data->regmap = devm_regmap_init_spi(spi, &max5487_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	data->kohms = id->driver_data;
> +
> +	indio_dev->info = &max5487_info;
> +	indio_dev->name = id->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = max5487_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
> +
> +	/* restore both wiper regs from NV regs */
> +	ret = regmap_write(data->regmap, MAX5487_COPY_NV_TO_AB, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
Hmm. Interesting case here - using this clearly doesn't result in a driver
crash, but it could in theory result in a 'race' of expectations...

As you are using the devm_ register here, the unregister will only occur
after everything in max5487_spi_remove has run. As the userspace api will
remain available until then, it is in theory possible to have the wiper
values stored to the non volatile memory and then get a sneaky change in
after that on request from userspace.  Thus userspace may think that the
value has been writen to the non volatile memory but it hasn't.

Hence, pleasee switch to the non devm_ version of iio_device_register
and call iio_device_unregister before you save the wiper regs out.
It's also more 'obviously correct' to unwind in a remove in the precise
reverse order of the probe.  Makes reviewer's job easier :)

> +}
> +
> +static int max5487_spi_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> +	struct max5487_data *data = iio_priv(indio_dev);
> +
> +	/* save both wiper regs to NV regs */
> +	return regmap_write(data->regmap, MAX5487_COPY_AB_TO_NV, 0);
> +}
> +
> +static const struct spi_device_id max5487_id[] = {
> +	{ "MAX5487", 10 },
> +	{ "MAX5488", 50 },
> +	{ "MAX5489", 100 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, max5487_id);
> +
> +static const struct acpi_device_id max5487_acpi_match[] = {
> +	{ "MAX5487", 10 },
> +	{ "MAX5488", 50 },
> +	{ "MAX5489", 100 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
> +
> +static struct spi_driver max5487_driver = {
> +	.driver = {
> +		.name = MAX5487_DRV_NAME,
> +		.owner = THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(max5487_acpi_match),
> +	},
> +	.id_table = max5487_id,
> +	.probe = max5487_spi_probe,
> +	.remove = max5487_spi_remove
> +};
> +module_spi_driver(max5487_driver);
> +
> +MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
> +MODULE_DESCRIPTION("max5487 SPI driver");
> +MODULE_LICENSE("GPL v2");
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers
  2016-04-09  8:24 ` [PATCH v2] " Cristina Moraru
  2016-04-10 11:24   ` Jonathan Cameron
@ 2016-04-10 12:47   ` Joachim Eastwood
  2016-04-10 13:10     ` Joachim Eastwood
  1 sibling, 1 reply; 17+ messages in thread
From: Joachim Eastwood @ 2016-04-10 12:47 UTC (permalink / raw)
  To: Cristina Moraru
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, peda, linux-kernel, linux-iio,
	daniel.baluta, octavian.purdila

Hi Cristina,

On 9 April 2016 at 10:24, Cristina Moraru <cristina.moraru09@gmail.com> 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 <cristina.moraru09@gmail.com>
> CC: Daniel Baluta <daniel.baluta@intel.com>
> ---
...
> +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;

Newline before return.

> +       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);
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               if (val < 0 || val > MAX5487_MAX_POS)
> +                       return -EINVAL;
> +               return regmap_write(data->regmap, chan->address, val);
> +       default:
> +               return -EINVAL;
> +       }
> +       return -EINVAL;

To be consistent with your max5487_read_raw() function you could do a:
       if (mask != IIO_CHAN_INFO_RAW)
               return -EINVAL;


> +static const struct iio_info max5487_info = {
> +       .read_raw = &max5487_read_raw,
> +       .write_raw = &max5487_write_raw,

Address operator should be unnecessary on functions.


> +       data->regmap = devm_regmap_init_spi(spi, &max5487_regmap_config);
> +       if (IS_ERR(data->regmap))
> +               return PTR_ERR(data->regmap);

Nothing wrong with using regmap here, but since you are only using
simple regmap_write()'s you might as well have used spi_write()
directly. I am not telling you to switch, but I don't see the point of
using regmap here.

Which reminds me; for regmap you need to select REGMAP_SPI in your
Kconfig entry.


regards,
Joachim Eastwood

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers
  2016-04-10 12:47   ` Joachim Eastwood
@ 2016-04-10 13:10     ` Joachim Eastwood
  0 siblings, 0 replies; 17+ messages in thread
From: Joachim Eastwood @ 2016-04-10 13:10 UTC (permalink / raw)
  To: Cristina Moraru
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, peda, linux-kernel, linux-iio,
	daniel.baluta, octavian.purdila

On 10 April 2016 at 14:47, Joachim  Eastwood <manabian@gmail.com> wrote:
> Hi Cristina,
>
> On 9 April 2016 at 10:24, Cristina Moraru <cristina.moraru09@gmail.com> 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 <cristina.moraru09@gmail.com>
>> CC: Daniel Baluta <daniel.baluta@intel.com>
>> ---
> ...
>> +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;
>
> Newline before return.
>
>> +       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);
>> +
>> +       switch (mask) {
>> +       case IIO_CHAN_INFO_RAW:
>> +               if (val < 0 || val > MAX5487_MAX_POS)
>> +                       return -EINVAL;
>> +               return regmap_write(data->regmap, chan->address, val);
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +       return -EINVAL;
>
> To be consistent with your max5487_read_raw() function you could do a:
>        if (mask != IIO_CHAN_INFO_RAW)
>                return -EINVAL;
>
>
>> +static const struct iio_info max5487_info = {
>> +       .read_raw = &max5487_read_raw,
>> +       .write_raw = &max5487_write_raw,
>
> Address operator should be unnecessary on functions.
>
>
>> +       data->regmap = devm_regmap_init_spi(spi, &max5487_regmap_config);
>> +       if (IS_ERR(data->regmap))
>> +               return PTR_ERR(data->regmap);
>
> Nothing wrong with using regmap here, but since you are only using
> simple regmap_write()'s you might as well have used spi_write()
> directly. I am not telling you to switch, but I don't see the point of
> using regmap here.

Looking again: it seem that spi.h doesn't have a function that do
write(cmd, data) which regmap does. So I guess that is one reason for
using regmap. But it wouldn't be hard to create a write(cmd,
data)-function for spi either. Just wrap spi_write() and have a local
buf var. I am a bit surprised that spi.h doesn't have such a function
as it should be quite a common pattern for spi chips.

>
> Which reminds me; for regmap you need to select REGMAP_SPI in your
> Kconfig entry.
>
>
> regards,
> Joachim Eastwood

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3] iio: max5487: Add support for Maxim digital potentiometers
  2016-03-24 11:21 [PATCH] iio: max5487: Add support for Maxim digital potentiometers Cristina Moraru
                   ` (3 preceding siblings ...)
  2016-04-09  8:24 ` [PATCH v2] " Cristina Moraru
@ 2016-05-17 13:11 ` Cristina Moraru
  2016-05-17 13:33   ` Peter Meerwald-Stadler
  2016-05-19  2:21   ` Matt Ranostay
  2016-05-18  9:15 ` [PATCH v4] " Cristina Moraru
  2016-05-19  5:55 ` [PATCH v5] " Cristina Moraru
  6 siblings, 2 replies; 17+ messages in thread
From: Cristina Moraru @ 2016-05-17 13:11 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw, sst, peda, mranostay, manabian,
	linux-kernel, linux-iio, daniel.baluta, octavian.purdila
  Cc: 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 <cristina.moraru09@gmail.com>
CC: Daniel Baluta <daniel.baluta@intel.com>
---
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 0000000..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 <cristina.moraru09@gmail.com>
+ *
+ * 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 <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/acpi.h>
+
+#include <linux/iio/sysfs.h>
+#include <linux/iio/iio.h>
+
+#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);
+}
+
+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,
+	.write_raw = max5487_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int max5487_spi_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct max5487_data *data;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	dev_set_drvdata(&spi->dev, indio_dev);
+	data = iio_priv(indio_dev);
+
+	data->spi = spi;
+	data->kohms = id->driver_data;
+
+	indio_dev->info = &max5487_info;
+	indio_dev->name = id->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = max5487_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
+
+	/* restore both wiper regs from NV regs */
+	ret = max5487_write_cmd(data->spi, MAX5487_COPY_NV_TO_AB);
+	if (ret < 0)
+		return ret;
+
+	return iio_device_register(indio_dev);
+}
+
+static int max5487_spi_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
+
+	iio_device_unregister(indio_dev);
+
+	/* save both wiper regs to NV regs */
+	return max5487_write_cmd(spi, MAX5487_COPY_AB_TO_NV);
+}
+
+static const struct spi_device_id max5487_id[] = {
+	{ "MAX5487", 10 },
+	{ "MAX5488", 50 },
+	{ "MAX5489", 100 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, max5487_id);
+
+static const struct acpi_device_id max5487_acpi_match[] = {
+	{ "MAX5487", 10 },
+	{ "MAX5488", 50 },
+	{ "MAX5489", 100 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
+
+static struct spi_driver max5487_driver = {
+	.driver = {
+		.name = "max5487",
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(max5487_acpi_match),
+	},
+	.id_table = max5487_id,
+	.probe = max5487_spi_probe,
+	.remove = max5487_spi_remove
+};
+module_spi_driver(max5487_driver);
+
+MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
+MODULE_DESCRIPTION("max5487 SPI driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] iio: max5487: Add support for Maxim digital potentiometers
  2016-05-17 13:11 ` [PATCH v3] " Cristina Moraru
@ 2016-05-17 13:33   ` Peter Meerwald-Stadler
  2016-05-19  2:21   ` Matt Ranostay
  1 sibling, 0 replies; 17+ messages in thread
From: Peter Meerwald-Stadler @ 2016-05-17 13:33 UTC (permalink / raw)
  To: Cristina Moraru
  Cc: jic23, knaack.h, lars, sst, peda, mranostay, manabian,
	linux-kernel, linux-iio, daniel.baluta, octavian.purdila


> 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 <cristina.moraru09@gmail.com>
> CC: Daniel Baluta <daniel.baluta@intel.com>
> ---
> 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 0000000..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 <cristina.moraru09@gmail.com>
> + *
> + * 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 <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/acpi.h>
> +
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/iio.h>
> +
> +#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,
> +			     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,
> +	.write_raw = max5487_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int max5487_spi_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max5487_data *data;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&spi->dev, indio_dev);
> +	data = iio_priv(indio_dev);
> +
> +	data->spi = spi;
> +	data->kohms = id->driver_data;
> +
> +	indio_dev->info = &max5487_info;
> +	indio_dev->name = id->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = max5487_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
> +
> +	/* restore both wiper regs from NV regs */
> +	ret = max5487_write_cmd(data->spi, MAX5487_COPY_NV_TO_AB);
> +	if (ret < 0)
> +		return ret;
> +
> +	return iio_device_register(indio_dev);
> +}
> +
> +static int max5487_spi_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	/* save both wiper regs to NV regs */
> +	return max5487_write_cmd(spi, MAX5487_COPY_AB_TO_NV);
> +}
> +
> +static const struct spi_device_id max5487_id[] = {
> +	{ "MAX5487", 10 },
> +	{ "MAX5488", 50 },
> +	{ "MAX5489", 100 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, max5487_id);
> +
> +static const struct acpi_device_id max5487_acpi_match[] = {
> +	{ "MAX5487", 10 },
> +	{ "MAX5488", 50 },
> +	{ "MAX5489", 100 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
> +
> +static struct spi_driver max5487_driver = {
> +	.driver = {
> +		.name = "max5487",
> +		.owner = THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(max5487_acpi_match),
> +	},
> +	.id_table = max5487_id,
> +	.probe = max5487_spi_probe,
> +	.remove = max5487_spi_remove
> +};
> +module_spi_driver(max5487_driver);
> +
> +MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
> +MODULE_DESCRIPTION("max5487 SPI driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4] iio: max5487: Add support for Maxim digital potentiometers
  2016-03-24 11:21 [PATCH] iio: max5487: Add support for Maxim digital potentiometers Cristina Moraru
                   ` (4 preceding siblings ...)
  2016-05-17 13:11 ` [PATCH v3] " Cristina Moraru
@ 2016-05-18  9:15 ` Cristina Moraru
  2016-05-19  5:55 ` [PATCH v5] " Cristina Moraru
  6 siblings, 0 replies; 17+ messages in thread
From: Cristina Moraru @ 2016-05-18  9:15 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw, sst, peda, mranostay, manabian,
	linux-kernel, linux-iio, daniel.baluta, octavian.purdila
  Cc: 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 <cristina.moraru09@gmail.com>
CC: Daniel Baluta <daniel.baluta@intel.com>
---
Changes since v3:
        Fix size issue in max5487_write_cmd
        Fix typo
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 | 161 ++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 drivers/iio/potentiometer/max5487.c

diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index 6acb238..0941c8d4 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 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 0000000..a828897
--- /dev/null
+++ b/drivers/iio/potentiometer/max5487.c
@@ -0,0 +1,161 @@
+/*
+ * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
+ *
+ * Copyright (C) Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>
+ *
+ * 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 <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/acpi.h>
+
+#include <linux/iio/sysfs.h>
+#include <linux/iio/iio.h>
+
+#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
+
+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, u16 cmd)
+{
+	return spi_write(spi, (const void *) &cmd, sizeof(u16));
+}
+
+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,
+	.write_raw = max5487_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int max5487_spi_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct max5487_data *data;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	dev_set_drvdata(&spi->dev, indio_dev);
+	data = iio_priv(indio_dev);
+
+	data->spi = spi;
+	data->kohms = id->driver_data;
+
+	indio_dev->info = &max5487_info;
+	indio_dev->name = id->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = max5487_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
+
+	/* restore both wiper regs from NV regs */
+	ret = max5487_write_cmd(data->spi, MAX5487_COPY_NV_TO_AB);
+	if (ret < 0)
+		return ret;
+
+	return iio_device_register(indio_dev);
+}
+
+static int max5487_spi_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
+
+	iio_device_unregister(indio_dev);
+
+	/* save both wiper regs to NV regs */
+	return max5487_write_cmd(spi, MAX5487_COPY_AB_TO_NV);
+}
+
+static const struct spi_device_id max5487_id[] = {
+	{ "MAX5487", 10 },
+	{ "MAX5488", 50 },
+	{ "MAX5489", 100 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, max5487_id);
+
+static const struct acpi_device_id max5487_acpi_match[] = {
+	{ "MAX5487", 10 },
+	{ "MAX5488", 50 },
+	{ "MAX5489", 100 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
+
+static struct spi_driver max5487_driver = {
+	.driver = {
+		.name = "max5487",
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(max5487_acpi_match),
+	},
+	.id_table = max5487_id,
+	.probe = max5487_spi_probe,
+	.remove = max5487_spi_remove
+};
+module_spi_driver(max5487_driver);
+
+MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
+MODULE_DESCRIPTION("max5487 SPI driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] iio: max5487: Add support for Maxim digital potentiometers
  2016-05-17 13:11 ` [PATCH v3] " Cristina Moraru
  2016-05-17 13:33   ` Peter Meerwald-Stadler
@ 2016-05-19  2:21   ` Matt Ranostay
  1 sibling, 0 replies; 17+ messages in thread
From: Matt Ranostay @ 2016-05-19  2:21 UTC (permalink / raw)
  To: Cristina Moraru
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Slawomir Stepien, peda, manabian,
	linux-kernel, linux-iio, Daniel Baluta, octavian.purdila

Slight nitpick below.

On Tue, May 17, 2016 at 6:11 AM, Cristina Moraru
<cristina.moraru09@gmail.com> 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 <cristina.moraru09@gmail.com>
> CC: Daniel Baluta <daniel.baluta@intel.com>
> ---
> 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 0000000..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 <cristina.moraru09@gmail.com>

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 <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/acpi.h>
> +
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/iio.h>
> +
> +#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);
> +}
> +
> +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,
> +       .write_raw = max5487_write_raw,
> +       .driver_module = THIS_MODULE,
> +};
> +
> +static int max5487_spi_probe(struct spi_device *spi)
> +{
> +       struct iio_dev *indio_dev;
> +       struct max5487_data *data;
> +       const struct spi_device_id *id = spi_get_device_id(spi);
> +       int ret;
> +
> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       dev_set_drvdata(&spi->dev, indio_dev);
> +       data = iio_priv(indio_dev);
> +
> +       data->spi = spi;
> +       data->kohms = id->driver_data;
> +
> +       indio_dev->info = &max5487_info;
> +       indio_dev->name = id->name;
> +       indio_dev->dev.parent = &spi->dev;
> +       indio_dev->modes = INDIO_DIRECT_MODE;
> +       indio_dev->channels = max5487_channels;
> +       indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
> +
> +       /* restore both wiper regs from NV regs */
> +       ret = max5487_write_cmd(data->spi, MAX5487_COPY_NV_TO_AB);
> +       if (ret < 0)
> +               return ret;
> +
> +       return iio_device_register(indio_dev);
> +}
> +
> +static int max5487_spi_remove(struct spi_device *spi)
> +{
> +       struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> +
> +       iio_device_unregister(indio_dev);
> +
> +       /* save both wiper regs to NV regs */
> +       return max5487_write_cmd(spi, MAX5487_COPY_AB_TO_NV);
> +}
> +
> +static const struct spi_device_id max5487_id[] = {
> +       { "MAX5487", 10 },
> +       { "MAX5488", 50 },
> +       { "MAX5489", 100 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(spi, max5487_id);
> +
> +static const struct acpi_device_id max5487_acpi_match[] = {
> +       { "MAX5487", 10 },
> +       { "MAX5488", 50 },
> +       { "MAX5489", 100 },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
> +
> +static struct spi_driver max5487_driver = {
> +       .driver = {
> +               .name = "max5487",
> +               .owner = THIS_MODULE,
> +               .acpi_match_table = ACPI_PTR(max5487_acpi_match),
> +       },
> +       .id_table = max5487_id,
> +       .probe = max5487_spi_probe,
> +       .remove = max5487_spi_remove
> +};
> +module_spi_driver(max5487_driver);
> +
> +MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
> +MODULE_DESCRIPTION("max5487 SPI driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v5] iio: max5487: Add support for Maxim digital potentiometers
  2016-03-24 11:21 [PATCH] iio: max5487: Add support for Maxim digital potentiometers Cristina Moraru
                   ` (5 preceding siblings ...)
  2016-05-18  9:15 ` [PATCH v4] " Cristina Moraru
@ 2016-05-19  5:55 ` Cristina Moraru
  2016-05-21 19:14   ` Jonathan Cameron
  6 siblings, 1 reply; 17+ messages in thread
From: Cristina Moraru @ 2016-05-19  5:55 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw, sst, peda, mranostay, manabian,
	linux-kernel, linux-iio, daniel.baluta, octavian.purdila
  Cc: 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 <cristina.moraru09@gmail.com>
CC: Daniel Baluta <daniel.baluta@intel.com>
---
Changes since v4:
	Add year for copyright
Changes since v3:
        Fix size issue in max5487_write_cmd
        Fix typo
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 | 161 ++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 drivers/iio/potentiometer/max5487.c

diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index 6acb238..0941c8d4 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 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 0000000..6c50939
--- /dev/null
+++ b/drivers/iio/potentiometer/max5487.c
@@ -0,0 +1,161 @@
+/*
+ * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
+ *
+ * Copyright (C) 2016 Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>
+ *
+ * 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 <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/acpi.h>
+
+#include <linux/iio/sysfs.h>
+#include <linux/iio/iio.h>
+
+#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
+
+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, u16 cmd)
+{
+	return spi_write(spi, (const void *) &cmd, sizeof(u16));
+}
+
+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,
+	.write_raw = max5487_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int max5487_spi_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct max5487_data *data;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	dev_set_drvdata(&spi->dev, indio_dev);
+	data = iio_priv(indio_dev);
+
+	data->spi = spi;
+	data->kohms = id->driver_data;
+
+	indio_dev->info = &max5487_info;
+	indio_dev->name = id->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = max5487_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
+
+	/* restore both wiper regs from NV regs */
+	ret = max5487_write_cmd(data->spi, MAX5487_COPY_NV_TO_AB);
+	if (ret < 0)
+		return ret;
+
+	return iio_device_register(indio_dev);
+}
+
+static int max5487_spi_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
+
+	iio_device_unregister(indio_dev);
+
+	/* save both wiper regs to NV regs */
+	return max5487_write_cmd(spi, MAX5487_COPY_AB_TO_NV);
+}
+
+static const struct spi_device_id max5487_id[] = {
+	{ "MAX5487", 10 },
+	{ "MAX5488", 50 },
+	{ "MAX5489", 100 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, max5487_id);
+
+static const struct acpi_device_id max5487_acpi_match[] = {
+	{ "MAX5487", 10 },
+	{ "MAX5488", 50 },
+	{ "MAX5489", 100 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
+
+static struct spi_driver max5487_driver = {
+	.driver = {
+		.name = "max5487",
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(max5487_acpi_match),
+	},
+	.id_table = max5487_id,
+	.probe = max5487_spi_probe,
+	.remove = max5487_spi_remove
+};
+module_spi_driver(max5487_driver);
+
+MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
+MODULE_DESCRIPTION("max5487 SPI driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] iio: max5487: Add support for Maxim digital potentiometers
  2016-05-19  5:55 ` [PATCH v5] " Cristina Moraru
@ 2016-05-21 19:14   ` Jonathan Cameron
  0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2016-05-21 19:14 UTC (permalink / raw)
  To: Cristina Moraru, knaack.h, lars, pmeerw, sst, peda, mranostay,
	manabian, linux-kernel, linux-iio, daniel.baluta,
	octavian.purdila

On 19/05/16 06:55, 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 <cristina.moraru09@gmail.com>
> CC: Daniel Baluta <daniel.baluta@intel.com>
Applied to the togreg branch of iio.git - initially pushed out as
testing.  As ever with stuff in testing I can still modify the
commit if anyone has anything to add (acks etc welcome of course!)

Jonathan
> ---
> Changes since v4:
> 	Add year for copyright
> Changes since v3:
>         Fix size issue in max5487_write_cmd
>         Fix typo
> 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 | 161 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 173 insertions(+)
>  create mode 100644 drivers/iio/potentiometer/max5487.c
> 
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 6acb238..0941c8d4 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 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 0000000..6c50939
> --- /dev/null
> +++ b/drivers/iio/potentiometer/max5487.c
> @@ -0,0 +1,161 @@
> +/*
> + * max5487.c - Support for MAX5487, MAX5488, MAX5489 digital potentiometers
> + *
> + * Copyright (C) 2016 Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>
> + *
> + * 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 <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/acpi.h>
> +
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/iio.h>
> +
> +#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
> +
> +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, u16 cmd)
> +{
> +	return spi_write(spi, (const void *) &cmd, sizeof(u16));
> +}
> +
> +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,
> +	.write_raw = max5487_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int max5487_spi_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max5487_data *data;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&spi->dev, indio_dev);
> +	data = iio_priv(indio_dev);
> +
> +	data->spi = spi;
> +	data->kohms = id->driver_data;
> +
> +	indio_dev->info = &max5487_info;
> +	indio_dev->name = id->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = max5487_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max5487_channels);
> +
> +	/* restore both wiper regs from NV regs */
> +	ret = max5487_write_cmd(data->spi, MAX5487_COPY_NV_TO_AB);
> +	if (ret < 0)
> +		return ret;
> +
> +	return iio_device_register(indio_dev);
> +}
> +
> +static int max5487_spi_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	/* save both wiper regs to NV regs */
> +	return max5487_write_cmd(spi, MAX5487_COPY_AB_TO_NV);
> +}
> +
> +static const struct spi_device_id max5487_id[] = {
> +	{ "MAX5487", 10 },
> +	{ "MAX5488", 50 },
> +	{ "MAX5489", 100 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, max5487_id);
> +
> +static const struct acpi_device_id max5487_acpi_match[] = {
> +	{ "MAX5487", 10 },
> +	{ "MAX5488", 50 },
> +	{ "MAX5489", 100 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, max5487_acpi_match);
> +
> +static struct spi_driver max5487_driver = {
> +	.driver = {
> +		.name = "max5487",
> +		.owner = THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(max5487_acpi_match),
> +	},
> +	.id_table = max5487_id,
> +	.probe = max5487_spi_probe,
> +	.remove = max5487_spi_remove
> +};
> +module_spi_driver(max5487_driver);
> +
> +MODULE_AUTHOR("Cristina-Gabriela Moraru <cristina.moraru09@gmail.com>");
> +MODULE_DESCRIPTION("max5487 SPI driver");
> +MODULE_LICENSE("GPL v2");
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2016-05-21 19:14 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-24 11:21 [PATCH] iio: max5487: Add support for Maxim digital potentiometers Cristina Moraru
2016-03-24 11:42 ` Daniel Baluta
2016-03-24 21:46 ` Peter Rosin
2016-04-01  8:28   ` Daniel Baluta
2016-04-03  9:25     ` Jonathan Cameron
2016-03-25 10:20 ` Peter Rosin
2016-04-01  8:33   ` Daniel Baluta
2016-04-09  8:24 ` [PATCH v2] " Cristina Moraru
2016-04-10 11:24   ` Jonathan Cameron
2016-04-10 12:47   ` Joachim Eastwood
2016-04-10 13:10     ` Joachim Eastwood
2016-05-17 13:11 ` [PATCH v3] " Cristina Moraru
2016-05-17 13:33   ` Peter Meerwald-Stadler
2016-05-19  2:21   ` Matt Ranostay
2016-05-18  9:15 ` [PATCH v4] " Cristina Moraru
2016-05-19  5:55 ` [PATCH v5] " Cristina Moraru
2016-05-21 19:14   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).