Re: [PATCH] mfd: Add support for TI LMP92001

On Tue, Aug 22, 2017 at 9:03 PM, Jonathan Cameron <
Jonathan.Cameron@huawei.com> wrote:

On Tue, 22 Aug 2017 13:26:11 +0700
[off-list ref] wrote:

quoted

From: Abhisit Sangjan <redacted>

TI LMP92001 Analog System Monitor and Controller

...

quoted

+
+What:           /sys/bus/iio/devices/iio:deviceX/outx
+Date:                August 2016
+KernelVersion:  4.1.15
+Contact:        Abhisit Sangjan [off-list ref]
+Description:
+             The pin output mode for DAC.
+                Can be either:
+                - "hiz" = High impedance state.
+                - "dac" = DAC output.
+             - "0" = Drive it to low.
+             - "1" = Drive it to high.

Look at the existing ABI for powerdown modes.  Covers this.

Could you point to where is powerdown modes?

quoted

+
+What:           /sys/bus/iio/devices/iio:deviceX/vref
+Date:                August 2016
+KernelVersion:  4.1.15
+Contact:        Abhisit Sangjan [off-list ref]
+Description:
+             This is voltage referance source for DACs.
+                Can be either:
+                - "external"
+                - "internal"

Normal assumption is that if an external reference is provided by
the hardware (specified in DT of similar) then we want to use it,
if not fall back to internal.

i.e. I don't think we have ever exposed this to userspace.

The real purpose is for debug on run-time, such as cat to know what is
running mode now.

quoted

+
+What:           /sys/devices/socX/soc/XXXXXXX

.aips-bus/XXXXXXX.i2c/i2c-X/X-00XX/lmp92001-adc.X.auto/iio:deviceX/en

Why the insanely long path rather than
/sys/bus/iio/device/iio:deviceX/en ?

quoted

+Date:                August 2016
+KernelVersion:  4.1.15
+Contact:        Abhisit Sangjan [off-list ref]
+Description:
+             This is ADC Conversion Enable for each channel.
+                Can be either:
+                - "enable"
+                - "disable"

Current form isn't per channel. Should be covered by buffered mode
scan_elements or the driver should be able to figure it out based
on what channel is being read.  Would not expect to see individual
channel enables for an ADC.

I am agree with you and I have read buffered mode and do not understand. I
thinks, I should omit first.

quoted

+
+What:           /sys/devices/socX/soc/XXXXXXX

.aips-bus/XXXXXXX.i2c/i2c-X/X-00XX/lmp92001-adc.X.auto/iio:deviceX/mode

Again, short path please.

quoted

+Date:                August 2016
+KernelVersion:  4.1.15
+Contact:        Abhisit Sangjan [off-list ref]
+Description:
+             This is conversion mode for all of ADCs.
+                Can be either:
+                - "continuous" = Continuously conversion all the time.
+                - "single-shot" = Once time conversion and stop.

Which of these makes sense is usually possible to figure out from the way
the
driver is being used.  This isn't something we want userspace to have to
specify.

I am agree with you. Choose from device tree and giving to userspace for
debug purpose (On time changing is optional), like cat to see what is
running mode now.

quoted

+
+What:           /sys/devices/socX/soc/XXXXXXX

.aips-bus/XXXXXXX.i2c/i2c-X/X-00XX/lmp92001-adc.X.auto/iio:deviceX/vref

quoted

+Date:                August 2016
+KernelVersion:  4.1.15
+Contact:        Abhisit Sangjan [off-list ref]
+Description:
+             This is voltage referance source for ADCs.
+                Can be either:
+                - "external"
+                - "internal"

Again, tends to be a hardware platform defined thing rather than
something it makes sense to expose to userspace.

The real purpose is for debug on run-time, such as cat to know what is
running mode now.

quoted

+ - ti,lmp92001-dac-outx:
+        Cy = 0, 1 = will force associated OUTx outputs to VDD
+        Cy = 0, 0 = will force associated OUTx outputs to GND
+ - ti,lmp92001-dac-gang: What group of Cy is governed to.
+        -----------------------------------------
+        |  Cy   | CDAC:GANG = 0 | CDAC:GANG = 1 |
+        -----------------------------------------
+        |  C1   |   OUT[1:4]    |    OUT[1:3]   |
+        -----------------------------------------
+        |  C2   |   OUT[5:6]    |    OUT[4:6]   |
+        -----------------------------------------
+        |  C3   |   OUT[7:8]    |    OUT[7:9]   |
+        -----------------------------------------
+        |  C4   |   OUT[9:12]   |    OUT[10:12] |
+        -----------------------------------------

If this is here, then why are we exposing it to userspace?
Either this is a feature of the hardware or it's not...

This feature is hardware related for the pin.

quoted

+
+     return 0;
+}
+
+static struct platform_driver lmp92001_gpio_driver = {
+     .driver.name    = "lmp92001-gpio",
+     .probe          = lmp92001_gpio_probe,
+     .remove         = lmp92001_gpio_remove,
+};
+
+static int __init lmp92001_gpio_init(void)
+{
+     return platform_driver_register(&lmp92001_gpio_driver);
+}
+subsys_initcall(lmp92001_gpio_init);

Do we actually need to do this as a subsys_initcall rather than
relying on deferred probing?

I am referenced to exiting mfd gpio driver. Could you help me what is the
right way.

quoted

+{
+     struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
+     unsigned int code, cgen, sgen, try;
+     int ret;
+
+     mutex_lock(&lmp92001->adc_lock);
+
+     ret = regmap_read(lmp92001->regmap, LMP92001_CGEN, &cgen);
+     if (ret < 0)
+             goto exit;
+
+     /*
+      * Is not continuous conversion?
+      * Lock the HW registers (if needed).
+      * Triggering single-short conversion.
+      * Waiting for conversion successfully.
+      */
+     if (!(cgen & CGEN_STRT)) {
+             if (!(cgen & CGEN_LCK)) {
+                     ret = regmap_update_bits(lmp92001->regmap,
+                                     LMP92001_CGEN, CGEN_LCK, CGEN_LCK);
+                     if (ret < 0)
+                             goto exit;
+             }
+
+             /* Writing any value to triggered Single-Shot conversion.

*/

quoted

+             ret = regmap_write(lmp92001->regmap, LMP92001_CTRIG, 1);
+             if (ret < 0)
+                     goto exit;
+
+             /* In case of conversion is in-progress, repeat for 10

times. */

quoted

+             try = 10;
+             do {
+                     ret = regmap_read(lmp92001->regmap,
+                                             LMP92001_SGEN, &sgen);
+                     if (ret < 0)
+                             goto exit;
+             } while ((sgen & CGEN_RST) && (--try > 0));
+
+             if (!try) {
+                     ret = -ETIME;
+                     goto exit;
+             }
+     }

Move this whole block to be single shot only.  Keep continuous for
triggered buffer usage.

Does this device have a dataready interrupt? Otherwise you'll probably
want to
do buffers with out the trigger and use a thread to monitor this complete
signal at 2x the expected frequency (so we don't miss any samples).

I have no idea what is buffered mode. I have read and do not understand.

quoted

+static int lmp92001_adc_probe(struct platform_device *pdev)
+{
+     struct lmp92001 *lmp92001 = *dev_get_drvdata(pdev->dev.parent);*
+     struct iio_dev *indio_dev;
+     struct device_node *np = pdev->dev.of_node;
+     const char *conversion;
+     unsigned int cgen = 0, cad1, cad2, cad3;
+     u32 mask;
+     int ret;
+
+     indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*lmp92001));
+     if (!indio_dev)
+             return -ENOMEM;
+
+     mutex_init(&lmp92001->adc_lock);
+
+     iio_device_set_drvdata(indio_dev, lmp92001);

Why?  You can always get the lmp92001 from iio_priv(indio_dev)

I would like to take it from parent driver data that has been
allocated (*highlight
in blue*). Now I am allocate the size for zero.

quoted

+
+     indio_dev->name = pdev->name;
+     indio_dev->dev.parent = &pdev->dev;
+     indio_dev->modes = INDIO_DIRECT_MODE;
+     indio_dev->info = &lmp92001_info;
+     indio_dev->channels = lmp92001_adc_channels;
+     indio_dev->num_channels = ARRAY_SIZE(lmp92001_adc_channels);
+
+     ret = regmap_update_bits(lmp92001->regmap, LMP92001_CGEN,
+                                     CGEN_RST, CGEN_RST);
+     if (ret < 0) {
+             dev_err(&pdev->dev, "failed to self reset all

registers\n");

quoted

+             return ret;
+     }
+
+     /*
+      * Turn on all of them, if you are pretty sure they are must be
+      * real-time update or specify which channel is needed to be used

to

quoted

+      * save conversion time for a cycle.
+      */
+     ret = of_property_read_u32(np, "ti,lmp92001-adc-mask", &mask);
+     if (ret < 0) {
+             cad1 = cad2 = cad3 = 0xFF;
+             dev_info(&pdev->dev, "turn on all of channels by

default\n");

quoted

+     } else {
+             cad1 = mask & 0xFF;
+             cad2 = (mask >> 8) & 0xFF;
+             cad3 = (mask >> 16) & 0xFF;
+     }

If you need these efficiencies, use the buffered mode interface of IIO and
switch into continuous mode with just the right channels.  For single reads
use oneshot.

This will then all be handled in the update_scan_mask callback.

I do not understand buffered mode. I could not implement it, sorry.

quoted

+
+     return 0;
+}
+
+static struct platform_driver lmp92001_adc_driver = {
+     .driver.name    = "lmp92001-adc",
+     .probe          = lmp92001_adc_probe,
+     .remove         = lmp92001_adc_remove,
+};
+
+static int __init lmp92001_adc_init(void)
+{
+     return platform_driver_register(&lmp92001_adc_driver);
+}
+subsys_initcall(lmp92001_adc_init);

Why?

+

quoted

+static void __exit lmp92001_adc_exit(void)
+{
+     platform_driver_unregister(&lmp92001_adc_driver);
+}
+module_exit(lmp92001_adc_exit);

use the magic platform macros to drop this boiler plate unless
you have a good reason for the subsys_initcall.

I am follow up the exiting mfd gpio driver.

Lots of comments would be about the ABI but we've already covered that
above.

Thank you, I will do.

quoted

+
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/lmp92001/core.h>
+
+#define CREF_DEXT    (1 << 0) /* 1 - DAC external reference.
+                               * 0 - DAC internal reference.
+                               */
+#define CDAC_OFF     (1 << 0) /* 1 - Forces all outputs to high

impedance. */

quoted

+#define CDAC_OLVL    (1 << 1) /* 1 - Cy=0 will force associated OUTx

outputs

quoted

+                               *     to VDD.
+                               * 0 - Cy=0 will force associated OUTx

outputs

quoted

+                               *     to GND.
+                               */
+#define CDAC_GANG    (1 << 2) /* Controls the association of analog

output

quoted

+                               * channels OUTx with asynchronous control
+                               * inputs Cy.
+                               *
+                               *         Cy to OUTx Assignment
+                               * --------------------------------------
+                               * | Cy | CDAC:GANG = 0 | CDAC:GANG = 1 |
+                               * --------------------------------------
+                               * | C1 | OUT[1:4]      | OUT[1:3]      |
+                               * --------------------------------------
+                               * | C2 | OUT[5:6]      | OUT[4:6]      |
+                               * --------------------------------------
+                               * | C3 | OUT[7:8]      | OUT[7:9]      |
+                               * --------------------------------------
+                               * | C4 | OUT[9:12]     | OUT[10:12]    |
+                               * --------------------------------------
+                               */

 ...

quoted

+int lmp92001_write_raw(struct iio_dev *indio_dev,
+     struct iio_chan_spec const *channel,
+     int val, int val2,
+     long mask)
+{
+     struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
+     int ret;
+
+     mutex_lock(&lmp92001->dac_lock);
+
+     if (val < 0 || val > 4095) {
+             ret = -EINVAL;
+             goto exit;
+     }
+
+     switch (mask) {
+     case IIO_CHAN_INFO_RAW:
+             switch (channel->type) {
+             case IIO_VOLTAGE:

Move lock in here... Will simplify code flow by allowing direct returns.

Thank you.

quoted

+                     ret = regmap_write(lmp92001->regmap,
+                                     0x7F + channel->channel, val);
+                     if (ret < 0)
+                             goto exit;

 ...

quoted

+static int lmp92001_dac_probe(struct platform_device *pdev)
+{
+     struct lmp92001 *lmp92001 = *dev_get_drvdata(pdev->dev.parent);*
+     struct iio_dev *indio_dev;
+     struct device_node *np = pdev->dev.of_node;
+     u8 gang = 0, outx = 0, hiz = 0;
+     unsigned int cdac = 0;
+     int ret;
+
+     indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*lmp92001));
+     if (!indio_dev)
+             return -ENOMEM;
+
+     mutex_init(&lmp92001->dac_lock);
+
+     iio_device_set_drvdata(indio_dev, lmp92001);

again, why? Use iio_priv(indio_dev) where ever you have been using
drvdata.

I would like to take it from parent driver data that has been
allocated (*highlight
in blue*).

quoted

+
+     indio_dev->name = pdev->name;
+     indio_dev->modes = INDIO_DIRECT_MODE;
+     indio_dev->info = &lmp92001_info;
+     indio_dev->channels = lmp92001_dac_channels;
+     indio_dev->num_channels = ARRAY_SIZE(lmp92001_dac_channels);
+
+     of_property_read_u8(np, "ti,lmp92001-dac-hiz", &hiz);
+     cdac |= hiz;

cdac = hiz and drop the = 0 above.  Also handle errors.

Thank you.

quoted

+
+     of_property_read_u8(np, "ti,lmp92001-dac-outx", &outx);
+     cdac |= outx << 1;
+
+     of_property_read_u8(np, "ti,lmp92001-dac-gang", &gang);
+     cdac |= gang << 2;
+
+     ret = regmap_update_bits(lmp92001->regmap, LMP92001_CDAC,
+                                     CDAC_GANG | CDAC_OLVL | CDAC_OFF,

cdac);

quoted

+     if (ret < 0)
+             return ret;
+
+     platform_set_drvdata(pdev, indio_dev);
+
+     return devm_iio_device_register(&pdev->dev, indio_dev);
+}
+
+static int lmp92001_dac_remove(struct platform_device *pdev)
+{
+     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+     devm_iio_device_unregister(&pdev->dev, indio_dev);

Same as for the ADC.

quoted

+
+     return 0;
+}
+
+static struct platform_driver lmp92001_dac_driver = {
+     .driver.name    = "lmp92001-dac",
+     .probe          = lmp92001_dac_probe,
+     .remove         = lmp92001_dac_remove,
+};
+
+static int __init lmp92001_dac_init(void)
+{
+     return platform_driver_register(&lmp92001_dac_driver);
+}
+subsys_initcall(lmp92001_dac_init);

Why subsys_initcall?

I am follow the exiting mfd dac driver. What is should be function for this?

quoted

+/* TODO: To read/write block access, it may need to re-ordering

endianness! */

quoted

+static int lmp92001_reg_read(void *context, unsigned int reg, unsigned

int *val)

quoted

+{
+     struct device *dev = context;
+     struct i2c_client *i2c = to_i2c_client(dev);
+     int ret;
+
+     if (reg > 0xff)
+             return -EINVAL;
+
+     switch (reg) {
+     case LMP92001_ID ... LMP92001_CTRIG:
+     case LMP92001_CREF:
+             ret = i2c_smbus_read_byte_data(i2c, reg);
+             break;
+     case LMP92001_ADC1 ... LMP92001_LIL11:
+     case LMP92001_DAC1 ... LMP92001_DALL:
+             ret = i2c_smbus_read_word_swapped(i2c, reg);
+             break;
+     case LMP92001_BLK0 ... LMP92001_BLK5:
+             ret = i2c_smbus_read_block_data(i2c, reg,
+                                             (u8 *)((uintptr_t)*val));
+             break;
+     default:
+             return -EINVAL;
+     }
+
+     if (ret < 0)
+             return ret;
+
+     if (reg <= LMP92001_DALL)
+             *val = ret;

Cleaner to push this up into the case statements perhaps? Or at least
return ret for the BLK one up there.

Thank you.

Ran out of time towards the end of this so review was rather less detailed!

Will take a look at the next version after you've broken this up.

Jonathan

Thank you so much Jonathan for your review. I will send new version by
today.

Abhisit.