RE: [PATCH 2/3] iio: adc: add new lp8788 adc driver
> This is mostly fine though things have gotten a little confused > wrt to the handling iio_priv in the probe and remove so that > needs cleaning up. A few other minor bits inline. > > Thanks, > > Jonathan Thanks a lot for detailed review. Patch v2 has been sent. Title: [PATCH v2] iio: adc: add new lp8788 adc driver Thank you. Best Regards, Milo -- 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: [PATCH 2/3] iio: adc: add new lp8788 adc driver
This is mostly fine though things have gotten a little confused wrt to the handling iio_priv in the probe and remove so that needs cleaning up. A few other minor bits inline. Thanks, Jonathan Thanks a lot for detailed review. Patch v2 has been sent. Title: [PATCH v2] iio: adc: add new lp8788 adc driver Thank you. Best Regards, Milo -- 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: [PATCH 2/3] iio: adc: add new lp8788 adc driver
On 08/09/2012 09:22 AM, Kim, Milo wrote:
> TI LP8788 has ADC function.
> The result of LP878 ADC is used in the LP8788 power supply driver.
> (such like getting the battery voltage, temperature and etc)
>
Hi,
This is mostly fine though things have gotten a little confused
wrt to the handling iio_priv in the probe and remove so that
needs cleaning up. A few other minor bits inline.
Thanks,
Jonathan
> Signed-off-by: Milo(Woogyom) Kim
> ---
> drivers/iio/adc/Kconfig |6 +
> drivers/iio/adc/Makefile |1 +
> drivers/iio/adc/lp8788_adc.c | 240
> ++
> 3 files changed, 247 insertions(+), 0 deletions(-)
> create mode 100644 drivers/iio/adc/lp8788_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8a78b4f..30c06ed 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -22,4 +22,10 @@ config AT91_ADC
> help
> Say yes here to build support for Atmel AT91 ADC.
>
> +config LP8788_ADC
> + bool "LP8788 ADC driver"
> + depends on MFD_LP8788
> + help
> + Say yes here to build support for TI LP8788 ADC.
> +
> endmenu
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 52eec25..72f9a76 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -4,3 +4,4 @@
>
> obj-$(CONFIG_AD7266) += ad7266.o
> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> +obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> diff --git a/drivers/iio/adc/lp8788_adc.c b/drivers/iio/adc/lp8788_adc.c
> new file mode 100644
> index 000..30767d5
> --- /dev/null
> +++ b/drivers/iio/adc/lp8788_adc.c
> @@ -0,0 +1,240 @@
> +/*
> + * TI LP8788 MFD - ADC driver
> + *
> + * Copyright 2012 Texas Instruments
> + *
> + * Author: Milo(Woogyom) Kim
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* register address */
> +#define LP8788_ADC_CONF 0x60
> +#define LP8788_ADC_RAW 0x61
> +#define LP8788_ADC_DONE 0x63
> +
> +#define START_ADC_CHANNELLPADC_VBATT_5P5
> +#define END_ADC_CHANNEL LPADC_MAX
> +#define ADC_CONV_START 1
> +#define ADC_CONV_DELAY_US100
> +
> +struct lp8788_adc {
> + struct lp8788 *lp;
As explained below I would drop this pointer.
The iio_priv with the suggested changes just stores a
pointer to lp.
> + struct iio_dev *indio_dev;
> +};
> +
> +static const int adc_const[LPADC_MAX] = {
> + [LPADC_VBATT_5P5] = 1343,
> + [LPADC_VIN_CHG] = 3052,
> + [LPADC_IBATT] = 610,
> + [LPADC_IC_TEMP] = 610,
> + [LPADC_VBATT_6P0] = 1465,
> + [LPADC_VBATT_5P0] = 1221,
> + [LPADC_ADC1] = 610,
> + [LPADC_ADC2] = 610,
> + [LPADC_VDD] = 1025,
> + [LPADC_VCOIN] = 757,
> + [LPADC_VDD_LDO] = 610,
> + [LPADC_ADC3] = 610,
> + [LPADC_ADC4] = 610,
> +};
> +
> +static inline unsigned int _get_adc_micro_unit(enum lp8788_adc_id id,
> + unsigned int adc_result)
> +{
> + return adc_const[id] * ((adc_result * 1000 + 500) / 1000);
> +}
You haven't marked the channels as processed (e.g. converted into correct units)
so I would do these as chan_info elements (scale and offset)
> +
> +static int lp8788_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct lp8788_adc *adc = iio_priv(indio_dev);
> + int retry = 5;
> + unsigned int msb, lsb;
> + u8 data, rawdata[2], shift;
> + int size = ARRAY_SIZE(rawdata);
only used in one place, so do it inline.
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + data = (chan->channel << 1) | ADC_CONV_START;
> + if (lp8788_write_byte(adc->lp, LP8788_ADC_CONF, data))
> + goto err;
> +
> + /* retry until adc conversion is done */
> + data = 0;
> + while (retry--) {
> + udelay(ADC_CONV_DELAY_US);
> +
> + if (lp8788_read_byte(adc->lp, LP8788_ADC_DONE, ))
> + goto err;
> +
> + /* conversion done */
> + if (data)
> + break;
> + }
> +
> + if (lp8788_read_multi_bytes(adc->lp, LP8788_ADC_RAW, rawdata, size))
> + goto err;
> +
This is a little odd. You establish the shift from the channel type, but
then have the masks hard coded. I'd just hard code the lot to
make it slightly easier to read
> + shift = chan->scan_type.shift;
> + msb = (rawdata[0] << shift) & 0x0ff0;
> + lsb = (rawdata[1] >> shift) & 0x000f;
> + *val =
Re: [PATCH 2/3] iio: adc: add new lp8788 adc driver
On 08/09/2012 09:22 AM, Kim, Milo wrote:
TI LP8788 has ADC function.
The result of LP878 ADC is used in the LP8788 power supply driver.
(such like getting the battery voltage, temperature and etc)
Hi,
This is mostly fine though things have gotten a little confused
wrt to the handling iio_priv in the probe and remove so that
needs cleaning up. A few other minor bits inline.
Thanks,
Jonathan
Signed-off-by: Milo(Woogyom) Kim milo@ti.com
---
drivers/iio/adc/Kconfig |6 +
drivers/iio/adc/Makefile |1 +
drivers/iio/adc/lp8788_adc.c | 240
++
3 files changed, 247 insertions(+), 0 deletions(-)
create mode 100644 drivers/iio/adc/lp8788_adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8a78b4f..30c06ed 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -22,4 +22,10 @@ config AT91_ADC
help
Say yes here to build support for Atmel AT91 ADC.
+config LP8788_ADC
+ bool LP8788 ADC driver
+ depends on MFD_LP8788
+ help
+ Say yes here to build support for TI LP8788 ADC.
+
endmenu
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 52eec25..72f9a76 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_AD7266) += ad7266.o
obj-$(CONFIG_AT91_ADC) += at91_adc.o
+obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
diff --git a/drivers/iio/adc/lp8788_adc.c b/drivers/iio/adc/lp8788_adc.c
new file mode 100644
index 000..30767d5
--- /dev/null
+++ b/drivers/iio/adc/lp8788_adc.c
@@ -0,0 +1,240 @@
+/*
+ * TI LP8788 MFD - ADC driver
+ *
+ * Copyright 2012 Texas Instruments
+ *
+ * Author: Milo(Woogyom) Kim milo@ti.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/slab.h
+#include linux/err.h
+#include linux/platform_device.h
+#include linux/delay.h
+#include linux/iio/iio.h
+#include linux/iio/driver.h
+#include linux/mfd/lp8788.h
+
+/* register address */
+#define LP8788_ADC_CONF 0x60
+#define LP8788_ADC_RAW 0x61
+#define LP8788_ADC_DONE 0x63
+
+#define START_ADC_CHANNELLPADC_VBATT_5P5
+#define END_ADC_CHANNEL LPADC_MAX
+#define ADC_CONV_START 1
+#define ADC_CONV_DELAY_US100
+
+struct lp8788_adc {
+ struct lp8788 *lp;
As explained below I would drop this pointer.
The iio_priv with the suggested changes just stores a
pointer to lp.
+ struct iio_dev *indio_dev;
+};
+
+static const int adc_const[LPADC_MAX] = {
+ [LPADC_VBATT_5P5] = 1343,
+ [LPADC_VIN_CHG] = 3052,
+ [LPADC_IBATT] = 610,
+ [LPADC_IC_TEMP] = 610,
+ [LPADC_VBATT_6P0] = 1465,
+ [LPADC_VBATT_5P0] = 1221,
+ [LPADC_ADC1] = 610,
+ [LPADC_ADC2] = 610,
+ [LPADC_VDD] = 1025,
+ [LPADC_VCOIN] = 757,
+ [LPADC_VDD_LDO] = 610,
+ [LPADC_ADC3] = 610,
+ [LPADC_ADC4] = 610,
+};
+
+static inline unsigned int _get_adc_micro_unit(enum lp8788_adc_id id,
+ unsigned int adc_result)
+{
+ return adc_const[id] * ((adc_result * 1000 + 500) / 1000);
+}
You haven't marked the channels as processed (e.g. converted into correct units)
so I would do these as chan_info elements (scale and offset)
+
+static int lp8788_adc_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct lp8788_adc *adc = iio_priv(indio_dev);
+ int retry = 5;
+ unsigned int msb, lsb;
+ u8 data, rawdata[2], shift;
+ int size = ARRAY_SIZE(rawdata);
only used in one place, so do it inline.
+
+ if (mask != IIO_CHAN_INFO_RAW)
+ return -EINVAL;
+
+ data = (chan-channel 1) | ADC_CONV_START;
+ if (lp8788_write_byte(adc-lp, LP8788_ADC_CONF, data))
+ goto err;
+
+ /* retry until adc conversion is done */
+ data = 0;
+ while (retry--) {
+ udelay(ADC_CONV_DELAY_US);
+
+ if (lp8788_read_byte(adc-lp, LP8788_ADC_DONE, data))
+ goto err;
+
+ /* conversion done */
+ if (data)
+ break;
+ }
+
+ if (lp8788_read_multi_bytes(adc-lp, LP8788_ADC_RAW, rawdata, size))
+ goto err;
+
This is a little odd. You establish the shift from the channel type, but
then have the masks hard coded. I'd just hard code the lot to
make it slightly easier to read
+ shift = chan-scan_type.shift;
+ msb = (rawdata[0] shift) 0x0ff0;
+ lsb = (rawdata[1] shift) 0x000f;
+
