From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <20170822150330.00007eeb@huawei.com> References: <1503383171-15515-1-git-send-email-s.abhisit@gmail.com> <20170822150330.00007eeb@huawei.com> From: Abhisit Sangjan Date: Wed, 30 Aug 2017 14:23:08 +0700 Message-ID: Subject: Re: [PATCH] mfd: Add support for TI LMP92001 Content-Type: multipart/alternative; boundary="94eb2c1a1a425139640557f3668d" To: Jonathan Cameron Cc: Lee Jones , Peter Meerwald-Stadler , jmondi , Linus Walleij , linux-kernel@vger.kernel.org, knaack.h@gmx.de, lars@metafoo.de, fabrice.gasnier@st.com, robh@kernel.org, Akinobu Mita , marek.vasut+renesas@gmail.com, jacopo+renesas@jmondi.org, mike.looijmans@topic.nl, peda@axentia.se, =?UTF-8?Q?Jean=2DFran=C3=A7ois_Dagenais?= , linux-iio@vger.kernel.org, linux-gpio@vger.kernel.org, robh+dt@kernel.org, mark.rutland@arm.com, devicetree@vger.kernel.org List-ID: --94eb2c1a1a425139640557f3668d Content-Type: text/plain; charset="UTF-8" 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 > wrote: > > > From: Abhisit Sangjan > > > > TI LMP92001 Analog System Monitor and Controller > ... > > + > > +What: /sys/bus/iio/devices/iio:deviceX/outx > > +Date: August 2016 > > +KernelVersion: 4.1.15 > > +Contact: Abhisit Sangjan > > +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? > > > + > > +What: /sys/bus/iio/devices/iio:deviceX/vref > > +Date: August 2016 > > +KernelVersion: 4.1.15 > > +Contact: Abhisit Sangjan > > +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. > > > + > > +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 ? > > > +Date: August 2016 > > +KernelVersion: 4.1.15 > > +Contact: Abhisit Sangjan > > +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. > > > > + > > +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. > > > +Date: August 2016 > > +KernelVersion: 4.1.15 > > +Contact: Abhisit Sangjan > > +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. > > + > > +What: /sys/devices/socX/soc/XXXXXXX > .aips-bus/XXXXXXX.i2c/i2c-X/X-00XX/lmp92001-adc.X.auto/iio:deviceX/vref > > +Date: August 2016 > > +KernelVersion: 4.1.15 > > +Contact: Abhisit Sangjan > > +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. > > + - 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. > > + > > + 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. > > > +{ > > + 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. > */ > > + ret = regmap_write(lmp92001->regmap, LMP92001_CTRIG, 1); > > + if (ret < 0) > > + goto exit; > > + > > + /* In case of conversion is in-progress, repeat for 10 > times. */ > > + 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. > > > +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. > > > + > > + 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"); > > + 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 > > + * 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"); > > + } 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. > > > + > > + 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? > > + > > +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. > > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > + > > +#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. */ > > +#define CDAC_OLVL (1 << 1) /* 1 - Cy=0 will force associated OUTx > outputs > > + * to VDD. > > + * 0 - Cy=0 will force associated OUTx > outputs > > + * to GND. > > + */ > > +#define CDAC_GANG (1 << 2) /* Controls the association of analog > output > > + * 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] | > > + * -------------------------------------- > > + */ > ... > > +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. > > + ret = regmap_write(lmp92001->regmap, > > + 0x7F + channel->channel, val); > > + if (ret < 0) > > + goto exit; > ... > > +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*). > > + > > + 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. > > + > > + 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); > > + 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. > > + > > + 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? > > +/* TODO: To read/write block access, it may need to re-ordering > endianness! */ > > +static int lmp92001_reg_read(void *context, unsigned int reg, unsigned > int *val) > > +{ > > + 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. --94eb2c1a1a425139640557f3668d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Tue, Aug 22, 2017 at 9:03 PM, Jonathan C= ameron <Jonathan.Cameron@huawei.com> wrote:
On Tue, 22 Aug= 2017 13:26:11 +0700
<s.abhisit@gmai= l.com> wrote:

> From: Abhisit Sangjan <s.abhisit@gmail.com>
>
> TI LMP92001 Analog System Monitor and Controller
...
>= +
> +What:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/sys/bus/iio/devices/ii= o:deviceX/outx
> +Date:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 August 2= 016
> +KernelVersion:=C2=A0 4.1.15
> +Contact:=C2=A0 =C2=A0 =C2=A0 =C2=A0 Abhisit Sangjan <s.abhisit@gmail.com>
> +Description:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0The pin output mode f= or DAC.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Can be either= :
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "hiz&q= uot; =3D High impedance state.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "dac&q= uot; =3D DAC output.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0- "0" =3D D= rive it to low.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0- "1" =3D D= rive it to high.
Look at the existing ABI for powerdown modes.=C2=A0 Covers this.
=

Could you point to where is pow= erdown modes?
=C2=A0

> +
> +What:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/sys/bus/iio/devices/ii= o:deviceX/vref
> +Date:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 August 2= 016
> +KernelVersion:=C2=A0 4.1.15
> +Contact:=C2=A0 =C2=A0 =C2=A0 =C2=A0 Abhisit Sangjan <s.abhisit@gmail.com>
> +Description:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0This is voltage refer= ance source for DACs.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Can be either= :
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "exter= nal"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "inter= nal"

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.

<= font face=3D"monospace, monospace">The real purpose is for debug on run-tim= e, such as cat to know what is running mode now.
=C2=A0

> +
> +What:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/sys/devices/socX/soc/X= XXXXXX.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 ?

> +Date:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 August 2= 016
> +KernelVersion:=C2=A0 4.1.15
> +Contact:=C2=A0 =C2=A0 =C2=A0 =C2=A0 Abhisit Sangjan <s.abhisit@gmail.com>
> +Description:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0This is ADC Conversio= n Enable for each channel.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Can be either= :
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "enabl= e"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "disab= le"

Current form isn't per channel. Should be covered by buffered mo= de
scan_elements or the driver should be able to figure it out based
on what channel is being read.=C2=A0 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.
=C2=A0


> +
> +What:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/sys/devices/socX/soc/X= XXXXXX.aips-bus/XXXXXXX.i2c/i2c-X/X-00XX/lmp92001-adc.X.auto/iio:= deviceX/mode

Again, short path please.

> +Date:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 August 2= 016
> +KernelVersion:=C2=A0 4.1.15
> +Contact:=C2=A0 =C2=A0 =C2=A0 =C2=A0 Abhisit Sangjan <s.abhisit@gmail.com>
> +Description:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0This is conversion mo= de for all of ADCs.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Can be either= :
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "conti= nuous" =3D Continuously conversion all the time.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "singl= e-shot" =3D Once time conversion and stop.

Which of these makes sense is usually possible to figure out from th= e way the
driver is being used.=C2=A0 This isn't something we want userspace to h= ave to
specify.
I am agree with you= .=C2=A0Choose from device tree and giving to userspace for debug purpose (O= n time changing is optional), like cat to see what is running mode now.


> +
> +What:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/sys/devices/socX/soc/X= XXXXXX.aips-bus/XXXXXXX.i2c/i2c-X/X-00XX/lmp92001-adc.X.auto/iio:= deviceX/vref
> +Date:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 August 2= 016
> +KernelVersion:=C2=A0 4.1.15
> +Contact:=C2=A0 =C2=A0 =C2=A0 =C2=A0 Abhisit Sangjan <s.abhisit@gmail.com>
> +Description:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0This is voltage refer= ance source for ADCs.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Can be either= :
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "exter= nal"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 - "inter= nal"

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.


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

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 pi= n.


> +
> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}
> +
> +static struct platform_driver lmp92001_gpio_driver =3D {
> +=C2=A0 =C2=A0 =C2=A0.driver.name=C2=A0 =C2=A0 =3D "lmp92001-gpio&qu= ot;,
> +=C2=A0 =C2=A0 =C2=A0.probe=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D lmp9= 2001_gpio_probe,
> +=C2=A0 =C2=A0 =C2=A0.remove=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D lmp9= 2001_gpio_remove,
> +};
> +
> +static int __init lmp92001_gpio_init(void)
> +{
> +=C2=A0 =C2=A0 =C2=A0return platform_driver_register(&lmp9200= 1_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.
=C2=A0
> +{
> +=C2=A0 =C2=A0 =C2=A0struct lmp92001 *lmp92001 =3D iio_device_get_drvd= ata(indio_dev);
> +=C2=A0 =C2=A0 =C2=A0unsigned int code, cgen, sgen, try;
> +=C2=A0 =C2=A0 =C2=A0int ret;
> +
> +=C2=A0 =C2=A0 =C2=A0mutex_lock(&lmp92001->adc_lock);
> +
> +=C2=A0 =C2=A0 =C2=A0ret =3D regmap_read(lmp92001->regmap, LMP92001= _CGEN, &cgen);
> +=C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto exit;
> +
> +=C2=A0 =C2=A0 =C2=A0/*
> +=C2=A0 =C2=A0 =C2=A0 * Is not continuous conversion?
> +=C2=A0 =C2=A0 =C2=A0 * Lock the HW registers (if needed).
> +=C2=A0 =C2=A0 =C2=A0 * Triggering single-short conversion.
> +=C2=A0 =C2=A0 =C2=A0 * Waiting for conversion successfully.
> +=C2=A0 =C2=A0 =C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0if (!(cgen & CGEN_STRT)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!(cgen & CGEN= _LCK)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0ret =3D regmap_update_bits(lmp92001->regmap,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0LMP92001_CGE= N, CGEN_LCK, CGEN_LCK);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto exit;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Writing any value = to triggered Single-Shot conversion. */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D regmap_write(= lmp92001->regmap, LMP92001_CTRIG, 1);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0goto exit;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* In case of convers= ion is in-progress, repeat for 10 times. */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0try =3D 10;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0do {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0ret =3D regmap_read(lmp92001->regmap,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0LMP92001_SGEN, &sgen);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto exit;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} while ((sgen & = CGEN_RST) && (--try > 0));
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!try) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0ret =3D -ETIME;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0goto exit;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0}
Move this whole bloc= k to be single shot only.=C2=A0 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.
=C2=A0

> +static int lmp92001_adc_probe(struct platform_device *pdev)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct lmp92001 *lmp92001 =3D dev_get_drvdata(pdev->dev.parent);
> +=C2=A0 =C2=A0 =C2=A0struct iio_dev *indio_dev;
> +=C2=A0 =C2=A0 =C2=A0struct device_node *np =3D pdev->dev.of_node;<= br> > +=C2=A0 =C2=A0 =C2=A0const char *conversion;
> +=C2=A0 =C2=A0 =C2=A0unsigned int cgen =3D 0, cad1, cad2, cad3;
> +=C2=A0 =C2=A0 =C2=A0u32 mask;
> +=C2=A0 =C2=A0 =C2=A0int ret;
> +
> +=C2=A0 =C2=A0 =C2=A0indio_dev =3D devm_iio_device_alloc(&pdev->= ;dev, sizeof(*lmp92001));
> +=C2=A0 =C2=A0 =C2=A0if (!indio_dev)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOMEM;
> +
> +=C2=A0 =C2=A0 =C2=A0mutex_init(&lmp92001->adc_lock);
> +
> +=C2=A0 =C2=A0 =C2=A0iio_device_set_drvdata(indio_dev, lmp92001);=

Why?=C2=A0 You can a= lways get the lmp92001 from iio_priv(indio_dev)

I would like to take it from parent driver data that has be= en allocated (highlight in blue). Now= I am allocate the size for zero.
=C2=A0

> +
> +=C2=A0 =C2=A0 =C2=A0indio_dev->name =3D pdev->name;
> +=C2=A0 =C2=A0 =C2=A0indio_dev->dev.parent =3D &pdev->dev; > +=C2=A0 =C2=A0 =C2=A0indio_dev->modes =3D INDIO_DIRECT_MODE;
> +=C2=A0 =C2=A0 =C2=A0indio_dev->info =3D &lmp92001_info;
> +=C2=A0 =C2=A0 =C2=A0indio_dev->channels =3D lmp92001_adc_channels;=
> +=C2=A0 =C2=A0 =C2=A0indio_dev->num_channels =3D ARRAY_SIZE(lmp9200= 1_adc_channels);
> +
> +=C2=A0 =C2=A0 =C2=A0ret =3D regmap_update_bits(lmp92001->regm= ap, LMP92001_CGEN,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0CGEN_RST, CG= EN_RST);
> +=C2=A0 =C2=A0 =C2=A0if (ret < 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_err(&pdev->= ;dev, "failed to self reset all registers\n");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0/*
> +=C2=A0 =C2=A0 =C2=A0 * Turn on all of them, if you are pretty sure th= ey are must be
> +=C2=A0 =C2=A0 =C2=A0 * real-time update or specify which channel is n= eeded to be used to
> +=C2=A0 =C2=A0 =C2=A0 * save conversion time for a cycle.
> +=C2=A0 =C2=A0 =C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0ret =3D of_property_read_u32(np, "ti,lmp9200= 1-adc-mask", &mask);
> +=C2=A0 =C2=A0 =C2=A0if (ret < 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cad1 =3D cad2 =3D cad= 3 =3D 0xFF;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_info(&pdev-&g= t;dev, "turn on all of channels by default\n");
> +=C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cad1 =3D mask & 0= xFF;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cad2 =3D (mask >&g= t; 8) & 0xFF;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cad3 =3D (mask >&g= t; 16) & 0xFF;
> +=C2=A0 =C2=A0 =C2=A0}

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

This will then all be handled in the update_scan_mask callback.
<= /blockquote>

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

= > +
> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}
> +
> +static struct platform_driver lmp92001_adc_driver =3D {
> +=C2=A0 =C2=A0 =C2=A0.driver.name=C2=A0 =C2=A0 =3D "lmp92001-adc&quo= t;,
> +=C2=A0 =C2=A0 =C2=A0.probe=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D lmp9= 2001_adc_probe,
> +=C2=A0 =C2=A0 =C2=A0.remove=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D lmp9= 2001_adc_remove,
> +};
> +
> +static int __init lmp92001_adc_init(void)
> +{
> +=C2=A0 =C2=A0 =C2=A0return platform_driver_register(&lmp9200= 1_adc_driver);
> +}
> +subsys_initcall(lmp92001_adc_init);
Why?
> +
> +static void __exit lmp92001_adc_exit(void)
> +{
> +=C2=A0 =C2=A0 =C2=A0platform_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.
<= /div>
=C2=A0
Lots of comments would be about the ABI but we'= ;ve already covered that above.
Thank you, I will do.
=C2=A0

> +
> +#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=C2=A0 =C2=A0 (1 << 0) /* 1 - DAC external ref= erence.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* 0 - DAC internal reference.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> +#define CDAC_OFF=C2=A0 =C2=A0 =C2=A0(1 << 0) /* 1 - Forces all = outputs to high impedance. */
> +#define CDAC_OLVL=C2=A0 =C2=A0 (1 << 1) /* 1 - Cy=3D0 will forc= e associated OUTx outputs
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*=C2=A0 =C2=A0 =C2=A0to VDD.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* 0 - Cy=3D0 will force associate= d OUTx outputs
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*=C2=A0 =C2=A0 =C2=A0to GND.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> +#define CDAC_GANG=C2=A0 =C2=A0 (1 << 2) /* Controls the associa= tion of analog output
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* channels OUTx with asynchronous= control
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* inputs Cy.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0Cy to OUTx Assignment
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* ------------------------------<= wbr>--------
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* | Cy | CDAC:GANG =3D 0 | CDAC:G= ANG =3D 1 |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* ------------------------------<= wbr>--------
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* | C1 | OUT[1:4]=C2=A0 =C2=A0 = =C2=A0 | OUT[1:3]=C2=A0 =C2=A0 =C2=A0 |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* ------------------------------<= wbr>--------
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* | C2 | OUT[5:6]=C2=A0 =C2=A0 = =C2=A0 | OUT[4:6]=C2=A0 =C2=A0 =C2=A0 |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* ------------------------------<= wbr>--------
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* | C3 | OUT[7:8]=C2=A0 =C2=A0 = =C2=A0 | OUT[7:9]=C2=A0 =C2=A0 =C2=A0 |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* ------------------------------<= wbr>--------
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* | C4 | OUT[9:12]=C2=A0 =C2=A0 = =C2=A0| OUT[10:12]=C2=A0 =C2=A0 |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* ------------------------------<= wbr>--------
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
=C2=A0...
=
> +int lmp92001_write_raw(struct iio_dev *indio_dev,
> +=C2=A0 =C2=A0 =C2=A0struct iio_chan_spec const *channel,
> +=C2=A0 =C2=A0 =C2=A0int val, int val2,
> +=C2=A0 =C2=A0 =C2=A0long mask)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct lmp92001 *lmp92001 =3D iio_device_get_drvd= ata(indio_dev);
> +=C2=A0 =C2=A0 =C2=A0int ret;
> +
> +=C2=A0 =C2=A0 =C2=A0mutex_lock(&lmp92001->dac_lock);
> +
> +=C2=A0 =C2=A0 =C2=A0if (val < 0 || val > 4095) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D -EINVAL;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto exit;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0switch (mask) {
> +=C2=A0 =C2=A0 =C2=A0case IIO_CHAN_INFO_RAW:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0switch (channel->t= ype) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case IIO_VOLTAGE:
Move lock in here...= Will simplify code flow by allowing direct returns.

Thank you.
=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D regmap_write(lmp92001->regmap,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x7F + chann= el->channel, val);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto exit;
=C2=A0...
> +static int lmp92001_dac_probe(struct platform_device *pdev)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct lmp92001 *lmp92001 =3D dev_get_drvdata(pdev->dev.parent);
> +=C2=A0 =C2=A0 =C2=A0struct iio_dev *indio_dev;
> +=C2=A0 =C2=A0 =C2=A0struct device_node *np =3D pdev->dev.of_node;<= br> > +=C2=A0 =C2=A0 =C2=A0u8 gang =3D 0, outx =3D 0, hiz =3D 0;
> +=C2=A0 =C2=A0 =C2=A0unsigned int cdac =3D 0;
> +=C2=A0 =C2=A0 =C2=A0int ret;
> +
> +=C2=A0 =C2=A0 =C2=A0indio_dev =3D devm_iio_device_alloc(&pdev->= ;dev, sizeof(*lmp92001));
> +=C2=A0 =C2=A0 =C2=A0if (!indio_dev)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOMEM;
> +
> +=C2=A0 =C2=A0 =C2=A0mutex_init(&lmp92001->dac_lock);
> +
> +=C2=A0 =C2=A0 =C2=A0iio_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).
=C2=A0
> +
> +=C2=A0 =C2=A0 =C2=A0indio_dev->name =3D pdev->name;
> +=C2=A0 =C2=A0 =C2=A0indio_dev->modes =3D INDIO_DIRECT_MODE;
> +=C2=A0 =C2=A0 =C2=A0indio_dev->info =3D &lmp92001_info;
> +=C2=A0 =C2=A0 =C2=A0indio_dev->channels =3D lmp92001_dac_channels;=
> +=C2=A0 =C2=A0 =C2=A0indio_dev->num_channels =3D ARRAY_SIZE(lmp9200= 1_dac_channels);
> +
> +=C2=A0 =C2=A0 =C2=A0of_property_read_u8(np, "ti,lmp92001-dac-hiz= ", &hiz);
> +=C2=A0 =C2=A0 =C2=A0cdac |=3D hiz;
cdac =3D hiz and drop the =3D 0 above.=C2=A0 Also handle errors.
=

Thank you.
=C2=A0
> +
> +=C2=A0 =C2=A0 =C2=A0of_property_read_u8(np, "ti,lmp92001-dac-out= x", &outx);
> +=C2=A0 =C2=A0 =C2=A0cdac |=3D outx << 1;
> +
> +=C2=A0 =C2=A0 =C2=A0of_property_read_u8(np, "ti,lmp92001-dac-gan= g", &gang);
> +=C2=A0 =C2=A0 =C2=A0cdac |=3D gang << 2;
> +
> +=C2=A0 =C2=A0 =C2=A0ret =3D regmap_update_bits(lmp92001->regm= ap, LMP92001_CDAC,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0CDAC_GANG | = CDAC_OLVL | CDAC_OFF, cdac);
> +=C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0platform_set_drvdata(pdev, indio_dev);
> +
> +=C2=A0 =C2=A0 =C2=A0return devm_iio_device_register(&pdev-&g= t;dev, indio_dev);
> +}
> +
> +static int lmp92001_dac_remove(struct platform_device *pdev)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct iio_dev *indio_dev =3D platform_get_drvdat= a(pdev);
> +
> +=C2=A0 =C2=A0 =C2=A0devm_iio_device_unregister(&pdev->dev= , indio_dev);
Same as for the ADC.
> +
> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}
> +
> +static struct platform_driver lmp92001_dac_driver =3D {
> +=C2=A0 =C2=A0 =C2=A0.driver.name=C2=A0 =C2=A0 =3D "lmp92001-dac&quo= t;,
> +=C2=A0 =C2=A0 =C2=A0.probe=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D lmp9= 2001_dac_probe,
> +=C2=A0 =C2=A0 =C2=A0.remove=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D lmp9= 2001_dac_remove,
> +};
> +
> +static int __init lmp92001_dac_init(void)
> +{
> +=C2=A0 =C2=A0 =C2=A0return platform_driver_register(&lmp9200= 1_dac_driver);
> +}
> +subsys_initcall(lmp92001_dac_init);
Why subsys_initcall?

I am fol= low the exiting mfd dac driver. What is should be function for this?
<= div>

> +/* TODO: To read/write block access, it may need to re-ordering endia= nness! */
> +static int lmp92001_reg_read(void *context, unsigned int reg, unsigne= d int *val)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct device *dev =3D context;
> +=C2=A0 =C2=A0 =C2=A0struct i2c_client *i2c =3D to_i2c_client(dev); > +=C2=A0 =C2=A0 =C2=A0int ret;
> +
> +=C2=A0 =C2=A0 =C2=A0if (reg > 0xff)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
> +
> +=C2=A0 =C2=A0 =C2=A0switch (reg) {
> +=C2=A0 =C2=A0 =C2=A0case LMP92001_ID ... LMP92001_CTRIG:
> +=C2=A0 =C2=A0 =C2=A0case LMP92001_CREF:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D i2c_smbus_rea= d_byte_data(i2c, reg);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +=C2=A0 =C2=A0 =C2=A0case LMP92001_ADC1 ... LMP92001_LIL11:
> +=C2=A0 =C2=A0 =C2=A0case LMP92001_DAC1 ... LMP92001_DALL:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D i2c_smbus_rea= d_word_swapped(i2c, reg);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +=C2=A0 =C2=A0 =C2=A0case LMP92001_BLK0 ... LMP92001_BLK5:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D i2c_smbus_rea= d_block_data(i2c, reg,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0(u8 *)((uintptr_t)*val));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +=C2=A0 =C2=A0 =C2=A0default:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0if (reg <=3D LMP92001_DALL)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*val =3D ret;
Cleaner to push this= up into the case statements perhaps? Or at least
return ret for the BLK one up there.

=
Thank you.
=C2=A0

Ran out of time towa= rds 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 revi= ew. I will send new version by today.

<= /div>
Abhisit.
--94eb2c1a1a425139640557f3668d--