Re: [PATCHv2] iio: adc: add driver for MCP3204/08 12-bit ADC

2013-05-03 Thread Lars-Peter Clausen 
On 05/03/2013 12:43 AM, Oskar Andero wrote:
> This adds support for Microchip's 12 bit AD converters MCP3204 and
> MCP3208. These chips communicates over SPI and supports single-ended
> and pseudo-differential configurations.
> 
> Cc: Jonathan Cameron 
> Cc: Lars-Peter Clausen 
> Signed-off-by: Oskar Andero 

Looks good, except some issues with probe error path handling

> ---
>  drivers/iio/adc/Kconfig   |  10 ++
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/mcp320x.c | 255 
> ++
>  3 files changed, 266 insertions(+)
>  create mode 100644 drivers/iio/adc/mcp320x.c
> 
[...]
> +static int mcp320x_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
[...]
> + case IIO_CHAN_INFO_SCALE:
> + /* Digital output code = (4096 * Vin) / Vref */
> + ret = regulator_get_voltage(adc->reg);
> + if (ret < 0)
> + goto out;
> +
> + *val = ret / 1000;
> + *val2 = 4096;
> + ret = IIO_VAL_FRACTIONAL;

You can use IIO_VAL_FRACTIONAL_LOG2 here, it is slightly more efficient and
take the log2 of the divisor for val2, e.g. 12 in this case.

> + break;
> +
> + default:
> + break;
> + }
> +
> +out:
> + mutex_unlock(>lock);
> +
> + return ret;
> +}
> +
[...]
> +static int mcp320x_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct mcp320x *adc;
> + const struct mcp3208_chip_info *chip_info;
> + int ret;
> +
> + indio_dev = iio_device_alloc(sizeof(*adc));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + adc = iio_priv(indio_dev);
> + adc->spi = spi;
> +
> + indio_dev->dev.parent = >dev;
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = _info;
> +
> + chip_info = _chip_infos[spi_get_device_id(spi)->driver_data];
> + indio_dev->channels = chip_info->channels;
> + indio_dev->num_channels = chip_info->num_channels;
> +
> + adc->transfer[0].tx_buf = >tx_buf;
> + adc->transfer[0].len = sizeof(adc->tx_buf);
> + adc->transfer[1].rx_buf = adc->rx_buf;
> + adc->transfer[1].len = sizeof(adc->rx_buf);
> +
> + spi_message_init_with_transfers(>msg, adc->transfer,
> + ARRAY_SIZE(adc->transfer));
> +
> + adc->reg = regulator_get(>dev, "vref");
> + if (IS_ERR(adc->reg))

You need to free the iio device in this case

> + return PTR_ERR(adc->reg);
> +
> + ret = regulator_enable(adc->reg);
> + if (ret < 0)
> + goto reg_free;
> +
> + mutex_init(>lock);
> +
> + ret = iio_device_register(indio_dev);
> + if (ret < 0)
> + goto iio_free;
> +
> + return 0;
> +
> +iio_free:
> + iio_device_free(indio_dev);
> +reg_free:
> + regulator_put(adc->reg);
> +

I think the order go reversed here, e.g. if you jump to reg_free you don't free
the IIO device. And you should also disable the regulator again in case
iio_device_register fails.

> + return ret;
> +}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2] iio: adc: add driver for MCP3204/08 12-bit ADC

2013-05-03 Thread Lars-Peter Clausen 
On 05/03/2013 12:43 AM, Oskar Andero wrote:
 This adds support for Microchip's 12 bit AD converters MCP3204 and
 MCP3208. These chips communicates over SPI and supports single-ended
 and pseudo-differential configurations.
 
 Cc: Jonathan Cameron ji...@cam.ac.uk
 Cc: Lars-Peter Clausen l...@metafoo.de
 Signed-off-by: Oskar Andero oskar.and...@gmail.com

Looks good, except some issues with probe error path handling

 ---
  drivers/iio/adc/Kconfig   |  10 ++
  drivers/iio/adc/Makefile  |   1 +
  drivers/iio/adc/mcp320x.c | 255 
 ++
  3 files changed, 266 insertions(+)
  create mode 100644 drivers/iio/adc/mcp320x.c
 
[...]
 +static int mcp320x_read_raw(struct iio_dev *indio_dev,
 + struct iio_chan_spec const *channel, int *val,
 + int *val2, long mask)
 +{
[...]
 + case IIO_CHAN_INFO_SCALE:
 + /* Digital output code = (4096 * Vin) / Vref */
 + ret = regulator_get_voltage(adc-reg);
 + if (ret  0)
 + goto out;
 +
 + *val = ret / 1000;
 + *val2 = 4096;
 + ret = IIO_VAL_FRACTIONAL;

You can use IIO_VAL_FRACTIONAL_LOG2 here, it is slightly more efficient and
take the log2 of the divisor for val2, e.g. 12 in this case.

 + break;
 +
 + default:
 + break;
 + }
 +
 +out:
 + mutex_unlock(adc-lock);
 +
 + return ret;
 +}
 +
[...]
 +static int mcp320x_probe(struct spi_device *spi)
 +{
 + struct iio_dev *indio_dev;
 + struct mcp320x *adc;
 + const struct mcp3208_chip_info *chip_info;
 + int ret;
 +
 + indio_dev = iio_device_alloc(sizeof(*adc));
 + if (!indio_dev)
 + return -ENOMEM;
 +
 + adc = iio_priv(indio_dev);
 + adc-spi = spi;
 +
 + indio_dev-dev.parent = spi-dev;
 + indio_dev-name = spi_get_device_id(spi)-name;
 + indio_dev-modes = INDIO_DIRECT_MODE;
 + indio_dev-info = mcp320x_info;
 +
 + chip_info = mcp3208_chip_infos[spi_get_device_id(spi)-driver_data];
 + indio_dev-channels = chip_info-channels;
 + indio_dev-num_channels = chip_info-num_channels;
 +
 + adc-transfer[0].tx_buf = adc-tx_buf;
 + adc-transfer[0].len = sizeof(adc-tx_buf);
 + adc-transfer[1].rx_buf = adc-rx_buf;
 + adc-transfer[1].len = sizeof(adc-rx_buf);
 +
 + spi_message_init_with_transfers(adc-msg, adc-transfer,
 + ARRAY_SIZE(adc-transfer));
 +
 + adc-reg = regulator_get(spi-dev, vref);
 + if (IS_ERR(adc-reg))

You need to free the iio device in this case

 + return PTR_ERR(adc-reg);
 +
 + ret = regulator_enable(adc-reg);
 + if (ret  0)
 + goto reg_free;
 +
 + mutex_init(adc-lock);
 +
 + ret = iio_device_register(indio_dev);
 + if (ret  0)
 + goto iio_free;
 +
 + return 0;
 +
 +iio_free:
 + iio_device_free(indio_dev);
 +reg_free:
 + regulator_put(adc-reg);
 +

I think the order go reversed here, e.g. if you jump to reg_free you don't free
the IIO device. And you should also disable the regulator again in case
iio_device_register fails.

 + return ret;
 +}

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCHv2] iio: adc: add driver for MCP3204/08 12-bit ADC

2013-05-02 Thread Oskar Andero 
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.

Cc: Jonathan Cameron 
Cc: Lars-Peter Clausen 
Signed-off-by: Oskar Andero 
---
 drivers/iio/adc/Kconfig   |  10 ++
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/mcp320x.c | 255 ++
 3 files changed, 266 insertions(+)
 create mode 100644 drivers/iio/adc/mcp320x.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
  max11646, max11647) Provides direct access via sysfs and buffered
  data via the iio dev interface.
 
+config MCP320X
+   tristate "Microchip Technology MCP3204/08"
+   depends on SPI
+   help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
 config TI_ADC081C
tristate "Texas Instruments ADC081C021/027"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..7dc32c3
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,255 @@
+/*
+ * Copyright (C) 2013 Oskar Andero 
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * 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 MCP_SINGLE_ENDED   (1 << 3)
+#define MCP_START_BIT  (1 << 4)
+
+enum {
+   mcp3204,
+   mcp3208,
+};
+
+struct mcp320x {
+   struct spi_device *spi;
+   struct spi_message msg;
+   struct spi_transfer transfer[2];
+
+   u8 tx_buf;
+   u8 rx_buf[2];
+
+   struct regulator *reg;
+   unsigned int ref_mv;
+
+   struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+   int ret;
+
+   adc->tx_buf = msg;
+   ret = spi_sync(adc->spi, >msg);
+   if (ret < 0)
+   return ret;
+
+   return ((adc->rx_buf[0] & 0x3f) << 6)  |
+   (adc->rx_buf[1] >> 2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *channel, int *val,
+   int *val2, long mask)
+{
+   struct mcp320x *adc = iio_priv(indio_dev);
+   int ret = -EINVAL;
+
+   mutex_lock(>lock);
+
+   switch (mask) {
+   case IIO_CHAN_INFO_RAW:
+   if (channel->differential)
+   ret = mcp320x_adc_conversion(adc,
+   MCP_START_BIT | channel->address);
+   else
+   ret = mcp320x_adc_conversion(adc,
+   MCP_START_BIT | MCP_SINGLE_ENDED |
+   channel->address);
+   if (ret < 0)
+   goto out;
+
+   *val = ret;
+   ret = IIO_VAL_INT;
+   break;
+
+   case IIO_CHAN_INFO_SCALE:
+   /* Digital output code = (4096 * Vin) / Vref */
+   ret = regulator_get_voltage(adc->reg);
+   if (ret < 0)
+   goto out;
+
+   *val = ret / 1000;
+   *val2 = 4096;
+   ret = IIO_VAL_FRACTIONAL;
+   break;
+
+   default:
+   break;
+   }
+
+out:
+   mutex_unlock(>lock);
+
+   return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num)   \
+   {   \
+   .type = IIO_VOLTAGE,\
+   .indexed = 1,   \
+   .channel = (num),   \
+   .address = (num),   \
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   \
+   .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
+   }
+
+#define MCP320X_VOLTAGE_CHANNEL_DIFF(num)

[PATCHv2] iio: adc: add driver for MCP3204/08 12-bit ADC

2013-05-02 Thread Oskar Andero 
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.

Cc: Jonathan Cameron ji...@cam.ac.uk
Cc: Lars-Peter Clausen l...@metafoo.de
Signed-off-by: Oskar Andero oskar.and...@gmail.com
---
 drivers/iio/adc/Kconfig   |  10 ++
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/mcp320x.c | 255 ++
 3 files changed, 266 insertions(+)
 create mode 100644 drivers/iio/adc/mcp320x.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
  max11646, max11647) Provides direct access via sysfs and buffered
  data via the iio dev interface.
 
+config MCP320X
+   tristate Microchip Technology MCP3204/08
+   depends on SPI
+   help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
 config TI_ADC081C
tristate Texas Instruments ADC081C021/027
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..7dc32c3
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,255 @@
+/*
+ * Copyright (C) 2013 Oskar Andero oskar.and...@gmail.com
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * 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/err.h
+#include linux/spi/spi.h
+#include linux/module.h
+#include linux/iio/iio.h
+#include linux/regulator/consumer.h
+
+#define MCP_SINGLE_ENDED   (1  3)
+#define MCP_START_BIT  (1  4)
+
+enum {
+   mcp3204,
+   mcp3208,
+};
+
+struct mcp320x {
+   struct spi_device *spi;
+   struct spi_message msg;
+   struct spi_transfer transfer[2];
+
+   u8 tx_buf;
+   u8 rx_buf[2];
+
+   struct regulator *reg;
+   unsigned int ref_mv;
+
+   struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+   int ret;
+
+   adc-tx_buf = msg;
+   ret = spi_sync(adc-spi, adc-msg);
+   if (ret  0)
+   return ret;
+
+   return ((adc-rx_buf[0]  0x3f)  6)  |
+   (adc-rx_buf[1]  2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *channel, int *val,
+   int *val2, long mask)
+{
+   struct mcp320x *adc = iio_priv(indio_dev);
+   int ret = -EINVAL;
+
+   mutex_lock(adc-lock);
+
+   switch (mask) {
+   case IIO_CHAN_INFO_RAW:
+   if (channel-differential)
+   ret = mcp320x_adc_conversion(adc,
+   MCP_START_BIT | channel-address);
+   else
+   ret = mcp320x_adc_conversion(adc,
+   MCP_START_BIT | MCP_SINGLE_ENDED |
+   channel-address);
+   if (ret  0)
+   goto out;
+
+   *val = ret;
+   ret = IIO_VAL_INT;
+   break;
+
+   case IIO_CHAN_INFO_SCALE:
+   /* Digital output code = (4096 * Vin) / Vref */
+   ret = regulator_get_voltage(adc-reg);
+   if (ret  0)
+   goto out;
+
+   *val = ret / 1000;
+   *val2 = 4096;
+   ret = IIO_VAL_FRACTIONAL;
+   break;
+
+   default:
+   break;
+   }
+
+out:
+   mutex_unlock(adc-lock);
+
+   return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num)   \
+   {   \
+   .type = IIO_VOLTAGE,\
+   .indexed = 1,   \
+   .channel = (num),   \
+   .address = (num),   \
+   .info_mask_separate =