Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/09/2016 16:35, Jonathan Cameron wrote:
> On 01/09/16 15:05, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree. This registers
>> the driver in the thermal framework.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences (registers bit and temperature computation)
>> between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz 
> One utterly trivial point about unrolling code ordering inline.
> 
> Other than the bit about patch 1 I'm basically happy with this..

ACK. Will revert this patch in v5. Thanks.

> However I would like some input (i.e. an Ack) from thermal given this
> sets up a thermal zone.
> 
> Zhang or Eduardo, could you take a quick look at this and confirm you
> are happy with it?
> 
> Thanks,
> 
> Jonathan
[...]
>> +
>> +err_map:
>> +iio_map_array_unregister(indio_dev);
>> +
>> +err_fifo_irq:
>> +/* Disable FIFO_DATA_PENDING interrupt on hardware side. */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>> +   0);
>> +
>> +err_temp_irq:
>> +/* Disable TEMP_DATA_PENDING interrupt on hardware side. */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN,
>> +   0);
>> +
>> +err:
>> +pm_runtime_put(>dev);
>> +pm_runtime_disable(>dev);
>> +
>> +return ret;
>> +}
>> +
>> +static int sun4i_gpadc_remove(struct platform_device *pdev)
>> +{
>> +struct sun4i_gpadc_dev *info;
>> +struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> +info = iio_priv(indio_dev);
>> +iio_device_unregister(indio_dev);
>> +iio_map_array_unregister(indio_dev);
>> +pm_runtime_put(>dev);
>> +pm_runtime_disable(>dev);
> Its really minor but in the interests of 'obviously correct' making
> review easy I'd rather everything in the remove was in the reverse order
> of probe (and hence the same as the error path in probe for most of it).
> 
> That would put the pm_runtime stuff last I think..
> 
> If you weren't rerolling anyway over patch 1 I'd probably have just let
> this go, but might as well make this trivial change as well.
> 

I'm going with the following order:
pm_runtime_put
pm_runtime_disable
regmap_update_bits
iio_map_array_unregister
iio_device_unregister

Is that okay? (I don't really know which one of iio_map_array_unregister
or iio_device_unregister to put first, if it matters in any way).

Thanks!
Quentin
> 
>> +/*
>> + * Disable TEMP_DATA_PENDING and FIFO_DATA_PENDING interrupts on
>> + * hardware side.
>> + */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN |
>> +SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>> +   0);
>> +
>> +return 0;
>> +}
>> +
>> +static const struct platform_device_id sun4i_gpadc_id[] = {
>> +{ "sun4i-a10-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ "sun5i-a13-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ "sun6i-a31-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver sun4i_gpadc_driver = {
>> +.driver = {
>> +.name = "sun4i-gpadc-iio",
>> +.pm = _gpadc_pm_ops,
>> +},
>> +.id_table = sun4i_gpadc_id,
>> +.probe = sun4i_gpadc_probe,
>> +.remove = sun4i_gpadc_remove,
>> +};
>> +
>> +module_platform_driver(sun4i_gpadc_driver);
>> +
>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>> +MODULE_AUTHOR("Quentin Schulz ");
>> +MODULE_LICENSE("GPL v2");
>>
> 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/09/2016 16:35, Jonathan Cameron wrote:
> On 01/09/16 15:05, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree. This registers
>> the driver in the thermal framework.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences (registers bit and temperature computation)
>> between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz 
> One utterly trivial point about unrolling code ordering inline.
> 
> Other than the bit about patch 1 I'm basically happy with this..

ACK. Will revert this patch in v5. Thanks.

> However I would like some input (i.e. an Ack) from thermal given this
> sets up a thermal zone.
> 
> Zhang or Eduardo, could you take a quick look at this and confirm you
> are happy with it?
> 
> Thanks,
> 
> Jonathan
[...]
>> +
>> +err_map:
>> +iio_map_array_unregister(indio_dev);
>> +
>> +err_fifo_irq:
>> +/* Disable FIFO_DATA_PENDING interrupt on hardware side. */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>> +   0);
>> +
>> +err_temp_irq:
>> +/* Disable TEMP_DATA_PENDING interrupt on hardware side. */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN,
>> +   0);
>> +
>> +err:
>> +pm_runtime_put(>dev);
>> +pm_runtime_disable(>dev);
>> +
>> +return ret;
>> +}
>> +
>> +static int sun4i_gpadc_remove(struct platform_device *pdev)
>> +{
>> +struct sun4i_gpadc_dev *info;
>> +struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> +info = iio_priv(indio_dev);
>> +iio_device_unregister(indio_dev);
>> +iio_map_array_unregister(indio_dev);
>> +pm_runtime_put(>dev);
>> +pm_runtime_disable(>dev);
> Its really minor but in the interests of 'obviously correct' making
> review easy I'd rather everything in the remove was in the reverse order
> of probe (and hence the same as the error path in probe for most of it).
> 
> That would put the pm_runtime stuff last I think..
> 
> If you weren't rerolling anyway over patch 1 I'd probably have just let
> this go, but might as well make this trivial change as well.
> 

I'm going with the following order:
pm_runtime_put
pm_runtime_disable
regmap_update_bits
iio_map_array_unregister
iio_device_unregister

Is that okay? (I don't really know which one of iio_map_array_unregister
or iio_device_unregister to put first, if it matters in any way).

Thanks!
Quentin
> 
>> +/*
>> + * Disable TEMP_DATA_PENDING and FIFO_DATA_PENDING interrupts on
>> + * hardware side.
>> + */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN |
>> +SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>> +   0);
>> +
>> +return 0;
>> +}
>> +
>> +static const struct platform_device_id sun4i_gpadc_id[] = {
>> +{ "sun4i-a10-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ "sun5i-a13-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ "sun6i-a31-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver sun4i_gpadc_driver = {
>> +.driver = {
>> +.name = "sun4i-gpadc-iio",
>> +.pm = _gpadc_pm_ops,
>> +},
>> +.id_table = sun4i_gpadc_id,
>> +.probe = sun4i_gpadc_probe,
>> +.remove = sun4i_gpadc_remove,
>> +};
>> +
>> +module_platform_driver(sun4i_gpadc_driver);
>> +
>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>> +MODULE_AUTHOR("Quentin Schulz ");
>> +MODULE_LICENSE("GPL v2");
>>
> 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/09/2016 18:12, Peter Meerwald-Stadler wrote:
> 
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
> 
> nitpicking ahead
>  
[...]
>> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
>> b/drivers/iio/adc/sun4i-gpadc-iio.c
>> new file mode 100644
>> index 000..a93d36d
>> --- /dev/null
>> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>> @@ -0,0 +1,525 @@
>> +/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
>> + *
>> + * Copyright (c) 2016 Quentin Schulz 
> 
> email address is incomplete
> 

As in my other patches, thanks!

>> + *
>> + * 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.
>> + *
>> + * The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> + * controller and a thermal sensor.
>> + * The thermal sensor works only when the ADC acts as a touchscreen 
>> controller
>> + * and is configured to throw an interrupt every fixed periods of time (let 
>> say
>> + * every X seconds).
>> + * One would be tempted to disable the IP on the hardware side rather than
>> + * disabling interrupts to save some power but that reset the internal 
>> clock of
> 
> resets
> 
>> + * the IP, resulting in having to wait X seconds every time we want to read 
>> the
>> + * value of the thermal sensor.
>> + * This is also the reason of using autosuspend in pm_runtime. If there 
>> were no
> 
> there was no
> 
[...]
>> +
>> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
> 
> static instead of const?
> 

static const then?

>> +{
>> +return SUN4I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>> +}
>> +
>> +const unsigned int sun6i_gpadc_chan_select(unsigned int chan)
> 
> static instead of const?
> 

static const then?

>> +{
>> +return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>> +}
>> +
>> +struct soc_specific {
>> +const int   temp_offset;
> 
> wondering why you constify every member?
> 

Because they're supposed to be fixed values? It won't change in runtime.
Is there any reason why I shouldn't do that?

>> +const int   temp_scale;
>> +const unsigned int  tp_mode_en;
>> +const unsigned int  tp_adc_select;
>> +const unsigned int  (*adc_chan_select)(unsigned int chan);
>> +};
[...]
>> +static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int 
>> *val,
>> +unsigned int irq)
>> +{
>> +struct sun4i_gpadc_dev *info = iio_priv(indio_dev);
>> +int ret = 0;
>> +
>> +pm_runtime_get_sync(indio_dev->dev.parent);
>> +mutex_lock(>mutex);
>> +
>> +reinit_completion(>completion);
>> +
>> +regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
>> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH);
> 
> check retval? here and elsewhere for regmap_write()
> 

ACK. What should I do with the retval then?

There are some in:
- sun4i_gpadc_read called for read_raws => return which error code (or
-ret only?)?
- sun4i_gpadc_runtime_suspend => return value of ret, but that would
cancel the suspend right?
- sun4i_gpadc_runtime_resume => return value of ret, but that would
cancel resume right?
- sun4i_gpadc_probe in the error gotos => probe already failing so do we
actually care if regmap_update_bits fails? If yes, what's the expected
behaviour?
- sun4i_gpadc_remove => return value of ret, but that would mean the
remove failed right?

[...]
>> +static int sun4i_gpadc_read_raw(struct iio_dev *indio_dev,
>> +struct iio_chan_spec const *chan, int *val,
>> +int *val2, long mask)
>> +{
>> +int ret;
>> +
>> +switch (mask) {
>> +case IIO_CHAN_INFO_OFFSET:
>> +ret = sun4i_gpadc_temp_offset(indio_dev, val);
>> +if (ret)
>> +return ret;
>> +
>> +return IIO_VAL_INT;
>> +case IIO_CHAN_INFO_RAW:
>> +if (chan->type == IIO_VOLTAGE) {
>> +ret = sun4i_gpadc_adc_read(indio_dev, chan->channel,
>> +   val);
>> +if (ret)
>> +return ret;
> 
> could share the "if (ret) return ret;" between code path
> 

ACK.

[...]
>> +static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>> +  irq_handler_t handler, const char *devname,
>> +  unsigned int *irq, atomic_t *atomic)
>> +{
>> +int ret;
>> +struct sun4i_gpadc_mfd_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
>> +struct sun4i_gpadc_dev *info = iio_priv(dev_get_drvdata(>dev));
>> +
>> +/*
>> + * Once the interrupt is activated, the IP continuously performs
>> +   

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/09/2016 18:12, Peter Meerwald-Stadler wrote:
> 
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
> 
> nitpicking ahead
>  
[...]
>> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
>> b/drivers/iio/adc/sun4i-gpadc-iio.c
>> new file mode 100644
>> index 000..a93d36d
>> --- /dev/null
>> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>> @@ -0,0 +1,525 @@
>> +/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
>> + *
>> + * Copyright (c) 2016 Quentin Schulz 
> 
> email address is incomplete
> 

As in my other patches, thanks!

>> + *
>> + * 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.
>> + *
>> + * The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> + * controller and a thermal sensor.
>> + * The thermal sensor works only when the ADC acts as a touchscreen 
>> controller
>> + * and is configured to throw an interrupt every fixed periods of time (let 
>> say
>> + * every X seconds).
>> + * One would be tempted to disable the IP on the hardware side rather than
>> + * disabling interrupts to save some power but that reset the internal 
>> clock of
> 
> resets
> 
>> + * the IP, resulting in having to wait X seconds every time we want to read 
>> the
>> + * value of the thermal sensor.
>> + * This is also the reason of using autosuspend in pm_runtime. If there 
>> were no
> 
> there was no
> 
[...]
>> +
>> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
> 
> static instead of const?
> 

static const then?

>> +{
>> +return SUN4I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>> +}
>> +
>> +const unsigned int sun6i_gpadc_chan_select(unsigned int chan)
> 
> static instead of const?
> 

static const then?

>> +{
>> +return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>> +}
>> +
>> +struct soc_specific {
>> +const int   temp_offset;
> 
> wondering why you constify every member?
> 

Because they're supposed to be fixed values? It won't change in runtime.
Is there any reason why I shouldn't do that?

>> +const int   temp_scale;
>> +const unsigned int  tp_mode_en;
>> +const unsigned int  tp_adc_select;
>> +const unsigned int  (*adc_chan_select)(unsigned int chan);
>> +};
[...]
>> +static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int 
>> *val,
>> +unsigned int irq)
>> +{
>> +struct sun4i_gpadc_dev *info = iio_priv(indio_dev);
>> +int ret = 0;
>> +
>> +pm_runtime_get_sync(indio_dev->dev.parent);
>> +mutex_lock(>mutex);
>> +
>> +reinit_completion(>completion);
>> +
>> +regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
>> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH);
> 
> check retval? here and elsewhere for regmap_write()
> 

ACK. What should I do with the retval then?

There are some in:
- sun4i_gpadc_read called for read_raws => return which error code (or
-ret only?)?
- sun4i_gpadc_runtime_suspend => return value of ret, but that would
cancel the suspend right?
- sun4i_gpadc_runtime_resume => return value of ret, but that would
cancel resume right?
- sun4i_gpadc_probe in the error gotos => probe already failing so do we
actually care if regmap_update_bits fails? If yes, what's the expected
behaviour?
- sun4i_gpadc_remove => return value of ret, but that would mean the
remove failed right?

[...]
>> +static int sun4i_gpadc_read_raw(struct iio_dev *indio_dev,
>> +struct iio_chan_spec const *chan, int *val,
>> +int *val2, long mask)
>> +{
>> +int ret;
>> +
>> +switch (mask) {
>> +case IIO_CHAN_INFO_OFFSET:
>> +ret = sun4i_gpadc_temp_offset(indio_dev, val);
>> +if (ret)
>> +return ret;
>> +
>> +return IIO_VAL_INT;
>> +case IIO_CHAN_INFO_RAW:
>> +if (chan->type == IIO_VOLTAGE) {
>> +ret = sun4i_gpadc_adc_read(indio_dev, chan->channel,
>> +   val);
>> +if (ret)
>> +return ret;
> 
> could share the "if (ret) return ret;" between code path
> 

ACK.

[...]
>> +static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>> +  irq_handler_t handler, const char *devname,
>> +  unsigned int *irq, atomic_t *atomic)
>> +{
>> +int ret;
>> +struct sun4i_gpadc_mfd_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
>> +struct sun4i_gpadc_dev *info = iio_priv(dev_get_drvdata(>dev));
>> +
>> +/*
>> + * Once the interrupt is activated, the IP continuously performs
>> + * conversions thus throws 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 05/09/16 07:29, Quentin Schulz wrote:
> On 04/09/2016 16:35, Jonathan Cameron wrote:
>> On 01/09/16 15:05, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. This patch adds the ADC driver which is
>>> based on the MFD for the same SoCs ADC.
>>>
>>> This also registers the thermal adc channel in the iio map array so
>>> iio_hwmon could use it without modifying the Device Tree. This registers
>>> the driver in the thermal framework.
>>>
>>> This driver probes on three different platform_device_id to take into
>>> account slight differences (registers bit and temperature computation)
>>> between Allwinner SoCs ADCs.
>>>
>>> Signed-off-by: Quentin Schulz 
>> One utterly trivial point about unrolling code ordering inline.
>>
>> Other than the bit about patch 1 I'm basically happy with this..
> 
> ACK. Will revert this patch in v5. Thanks.
> 
>> However I would like some input (i.e. an Ack) from thermal given this
>> sets up a thermal zone.
>>
>> Zhang or Eduardo, could you take a quick look at this and confirm you
>> are happy with it?
>>
>> Thanks,
>>
>> Jonathan
> [...]
>>> +
>>> +err_map:
>>> +   iio_map_array_unregister(indio_dev);
>>> +
>>> +err_fifo_irq:
>>> +   /* Disable FIFO_DATA_PENDING interrupt on hardware side. */
>>> +   regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>>> +  SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>>> +  0);
>>> +
>>> +err_temp_irq:
>>> +   /* Disable TEMP_DATA_PENDING interrupt on hardware side. */
>>> +   regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>>> +  SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN,
>>> +  0);
>>> +
>>> +err:
>>> +   pm_runtime_put(>dev);
>>> +   pm_runtime_disable(>dev);
>>> +
>>> +   return ret;
>>> +}
>>> +
>>> +static int sun4i_gpadc_remove(struct platform_device *pdev)
>>> +{
>>> +   struct sun4i_gpadc_dev *info;
>>> +   struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +
>>> +   info = iio_priv(indio_dev);
>>> +   iio_device_unregister(indio_dev);
>>> +   iio_map_array_unregister(indio_dev);
>>> +   pm_runtime_put(>dev);
>>> +   pm_runtime_disable(>dev);
>> Its really minor but in the interests of 'obviously correct' making
>> review easy I'd rather everything in the remove was in the reverse order
>> of probe (and hence the same as the error path in probe for most of it).
>>
>> That would put the pm_runtime stuff last I think..
>>
>> If you weren't rerolling anyway over patch 1 I'd probably have just let
>> this go, but might as well make this trivial change as well.
>>
> 
> I'm going with the following order:
> pm_runtime_put
> pm_runtime_disable
> regmap_update_bits
> iio_map_array_unregister
> iio_device_unregister
> 
> Is that okay? (I don't really know which one of iio_map_array_unregister
> or iio_device_unregister to put first, if it matters in any way).
Unless we have a really complex race condition involving a client driver
coming up just as the provider is unregistered I doubt it matters.

At some point we should probably chase down any paths through that
with some carefully crafted tests but it's in the seriously unlikely
category!

Jonathan
> 
> Thanks!
> Quentin
>>
>>> +   /*
>>> +* Disable TEMP_DATA_PENDING and FIFO_DATA_PENDING interrupts on
>>> +* hardware side.
>>> +*/
>>> +   regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>>> +  SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN |
>>> +   SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>>> +  0);
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static const struct platform_device_id sun4i_gpadc_id[] = {
>>> +   { "sun4i-a10-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>>> +   { "sun5i-a13-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>>> +   { "sun6i-a31-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>>> +   { /* sentinel */ },
>>> +};
>>> +
>>> +static struct platform_driver sun4i_gpadc_driver = {
>>> +   .driver = {
>>> +   .name = "sun4i-gpadc-iio",
>>> +   .pm = _gpadc_pm_ops,
>>> +   },
>>> +   .id_table = sun4i_gpadc_id,
>>> +   .probe = sun4i_gpadc_probe,
>>> +   .remove = sun4i_gpadc_remove,
>>> +};
>>> +
>>> +module_platform_driver(sun4i_gpadc_driver);
>>> +
>>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>>> +MODULE_AUTHOR("Quentin Schulz ");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 05/09/16 07:29, Quentin Schulz wrote:
> On 04/09/2016 16:35, Jonathan Cameron wrote:
>> On 01/09/16 15:05, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. This patch adds the ADC driver which is
>>> based on the MFD for the same SoCs ADC.
>>>
>>> This also registers the thermal adc channel in the iio map array so
>>> iio_hwmon could use it without modifying the Device Tree. This registers
>>> the driver in the thermal framework.
>>>
>>> This driver probes on three different platform_device_id to take into
>>> account slight differences (registers bit and temperature computation)
>>> between Allwinner SoCs ADCs.
>>>
>>> Signed-off-by: Quentin Schulz 
>> One utterly trivial point about unrolling code ordering inline.
>>
>> Other than the bit about patch 1 I'm basically happy with this..
> 
> ACK. Will revert this patch in v5. Thanks.
> 
>> However I would like some input (i.e. an Ack) from thermal given this
>> sets up a thermal zone.
>>
>> Zhang or Eduardo, could you take a quick look at this and confirm you
>> are happy with it?
>>
>> Thanks,
>>
>> Jonathan
> [...]
>>> +
>>> +err_map:
>>> +   iio_map_array_unregister(indio_dev);
>>> +
>>> +err_fifo_irq:
>>> +   /* Disable FIFO_DATA_PENDING interrupt on hardware side. */
>>> +   regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>>> +  SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>>> +  0);
>>> +
>>> +err_temp_irq:
>>> +   /* Disable TEMP_DATA_PENDING interrupt on hardware side. */
>>> +   regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>>> +  SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN,
>>> +  0);
>>> +
>>> +err:
>>> +   pm_runtime_put(>dev);
>>> +   pm_runtime_disable(>dev);
>>> +
>>> +   return ret;
>>> +}
>>> +
>>> +static int sun4i_gpadc_remove(struct platform_device *pdev)
>>> +{
>>> +   struct sun4i_gpadc_dev *info;
>>> +   struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +
>>> +   info = iio_priv(indio_dev);
>>> +   iio_device_unregister(indio_dev);
>>> +   iio_map_array_unregister(indio_dev);
>>> +   pm_runtime_put(>dev);
>>> +   pm_runtime_disable(>dev);
>> Its really minor but in the interests of 'obviously correct' making
>> review easy I'd rather everything in the remove was in the reverse order
>> of probe (and hence the same as the error path in probe for most of it).
>>
>> That would put the pm_runtime stuff last I think..
>>
>> If you weren't rerolling anyway over patch 1 I'd probably have just let
>> this go, but might as well make this trivial change as well.
>>
> 
> I'm going with the following order:
> pm_runtime_put
> pm_runtime_disable
> regmap_update_bits
> iio_map_array_unregister
> iio_device_unregister
> 
> Is that okay? (I don't really know which one of iio_map_array_unregister
> or iio_device_unregister to put first, if it matters in any way).
Unless we have a really complex race condition involving a client driver
coming up just as the provider is unregistered I doubt it matters.

At some point we should probably chase down any paths through that
with some carefully crafted tests but it's in the seriously unlikely
category!

Jonathan
> 
> Thanks!
> Quentin
>>
>>> +   /*
>>> +* Disable TEMP_DATA_PENDING and FIFO_DATA_PENDING interrupts on
>>> +* hardware side.
>>> +*/
>>> +   regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>>> +  SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN |
>>> +   SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>>> +  0);
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static const struct platform_device_id sun4i_gpadc_id[] = {
>>> +   { "sun4i-a10-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>>> +   { "sun5i-a13-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>>> +   { "sun6i-a31-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>>> +   { /* sentinel */ },
>>> +};
>>> +
>>> +static struct platform_driver sun4i_gpadc_driver = {
>>> +   .driver = {
>>> +   .name = "sun4i-gpadc-iio",
>>> +   .pm = _gpadc_pm_ops,
>>> +   },
>>> +   .id_table = sun4i_gpadc_id,
>>> +   .probe = sun4i_gpadc_probe,
>>> +   .remove = sun4i_gpadc_remove,
>>> +};
>>> +
>>> +module_platform_driver(sun4i_gpadc_driver);
>>> +
>>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>>> +MODULE_AUTHOR("Quentin Schulz ");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Joshua Clayton]

On Monday, September 05, 2016 09:11:47 AM Quentin Schulz wrote:
> On 05/09/2016 09:07, Maxime Ripard wrote:
> > Hi,
> > 
> > Nitpicks ahead.
> > 
> > On Thu, Sep 01, 2016 at 04:05:05PM +0200, Quentin Schulz wrote:
> >> +  info->soc_specific = (struct soc_specific 
> >> *)platform_get_device_id(pdev)->driver_data;
> > 
> > This line is still rather long. How about calling the field "data" and
> > the structure gpadc_data?
> > 
> 
> driver_data is coming from the platform_device_id
> (http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L498)
> so can't really change that.
> I could change the structure to gpadc_data, that would save me 2
> characters, still 13 characters above the 80 characters limit however.
> 
I think Maxime meant to change member "soc_specific" to "data" of type
struct gpadc_data

> >> +
> >> +  tzd = devm_thermal_zone_of_sensor_register(pdev->dev.parent, 0, info,
> >> + _ts_tz_ops);
> > 
> > A comment on why you put the parent device structure and not the
> > device itself like you're doing on all the other calls in that probe
> > would be nice.
> > 
> 
> Indeed.
> 
> Thanks,
> Quentin
> 
> > Thanks!
> > Maxime
> > 
> 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Joshua Clayton]

On Monday, September 05, 2016 09:11:47 AM Quentin Schulz wrote:
> On 05/09/2016 09:07, Maxime Ripard wrote:
> > Hi,
> > 
> > Nitpicks ahead.
> > 
> > On Thu, Sep 01, 2016 at 04:05:05PM +0200, Quentin Schulz wrote:
> >> +  info->soc_specific = (struct soc_specific 
> >> *)platform_get_device_id(pdev)->driver_data;
> > 
> > This line is still rather long. How about calling the field "data" and
> > the structure gpadc_data?
> > 
> 
> driver_data is coming from the platform_device_id
> (http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L498)
> so can't really change that.
> I could change the structure to gpadc_data, that would save me 2
> characters, still 13 characters above the 80 characters limit however.
> 
I think Maxime meant to change member "soc_specific" to "data" of type
struct gpadc_data

> >> +
> >> +  tzd = devm_thermal_zone_of_sensor_register(pdev->dev.parent, 0, info,
> >> + _ts_tz_ops);
> > 
> > A comment on why you put the parent device structure and not the
> > device itself like you're doing on all the other calls in that probe
> > would be nice.
> > 
> 
> Indeed.
> 
> Thanks,
> Quentin
> 
> > Thanks!
> > Maxime
> > 
> 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Peter Meerwald-Stadler]

>  +{
>  +return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>  +}
>  +
>  +struct soc_specific {
>  +const int   temp_offset;
> >>>
> >>> wondering why you constify every member?
> >>>
> >>
> >> Because they're supposed to be fixed values? It won't change in runtime.
> >> Is there any reason why I shouldn't do that?
> > 
> > yes, but using the entire struct as const has the same effect;
> > constifying individual members makes more sense if there are also 
> > non-const members
> > 
> > nothing wrong, just unusual
> > 
> 
> So I would let all members non-const and then when using the struct
> soc_specific as a member in a struct or as a variable I would prefix it
> with const? That's what you mean by using the entire struct as const?

yes, exactly

thanks, p.

-- 

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

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Peter Meerwald-Stadler]

>  +{
>  +return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>  +}
>  +
>  +struct soc_specific {
>  +const int   temp_offset;
> >>>
> >>> wondering why you constify every member?
> >>>
> >>
> >> Because they're supposed to be fixed values? It won't change in runtime.
> >> Is there any reason why I shouldn't do that?
> > 
> > yes, but using the entire struct as const has the same effect;
> > constifying individual members makes more sense if there are also 
> > non-const members
> > 
> > nothing wrong, just unusual
> > 
> 
> So I would let all members non-const and then when using the struct
> soc_specific as a member in a struct or as a variable I would prefix it
> with const? That's what you mean by using the entire struct as const?

yes, exactly

thanks, p.

-- 

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

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 05/09/2016 10:07, Peter Meerwald-Stadler wrote:
> 
 The Allwinner SoCs all have an ADC that can also act as a touchscreen
 controller and a thermal sensor. This patch adds the ADC driver which is
 based on the MFD for the same SoCs ADC.
>>>
>>> nitpicking ahead
> 
>> [...]
 +
 +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
>>>
>>> static instead of const?
> 
>> static const then?
> 
> no, the const is redundant and ignored
> -Wignored-qualifiers gives a warning
> 
> just static, no const
> 
> see
> http://stackoverflow.com/questions/12052468/type-qualifiers-on-function-return-type
> 

ACK. Thanks.

 +{
 +  return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
 +}
 +
 +struct soc_specific {
 +  const int   temp_offset;
>>>
>>> wondering why you constify every member?
>>>
>>
>> Because they're supposed to be fixed values? It won't change in runtime.
>> Is there any reason why I shouldn't do that?
> 
> yes, but using the entire struct as const has the same effect;
> constifying individual members makes more sense if there are also 
> non-const members
> 
> nothing wrong, just unusual
> 

So I would let all members non-const and then when using the struct
soc_specific as a member in a struct or as a variable I would prefix it
with const? That's what you mean by using the entire struct as const?

[...]

Thanks,
Quentin

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 05/09/2016 10:07, Peter Meerwald-Stadler wrote:
> 
 The Allwinner SoCs all have an ADC that can also act as a touchscreen
 controller and a thermal sensor. This patch adds the ADC driver which is
 based on the MFD for the same SoCs ADC.
>>>
>>> nitpicking ahead
> 
>> [...]
 +
 +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
>>>
>>> static instead of const?
> 
>> static const then?
> 
> no, the const is redundant and ignored
> -Wignored-qualifiers gives a warning
> 
> just static, no const
> 
> see
> http://stackoverflow.com/questions/12052468/type-qualifiers-on-function-return-type
> 

ACK. Thanks.

 +{
 +  return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
 +}
 +
 +struct soc_specific {
 +  const int   temp_offset;
>>>
>>> wondering why you constify every member?
>>>
>>
>> Because they're supposed to be fixed values? It won't change in runtime.
>> Is there any reason why I shouldn't do that?
> 
> yes, but using the entire struct as const has the same effect;
> constifying individual members makes more sense if there are also 
> non-const members
> 
> nothing wrong, just unusual
> 

So I would let all members non-const and then when using the struct
soc_specific as a member in a struct or as a variable I would prefix it
with const? That's what you mean by using the entire struct as const?

[...]

Thanks,
Quentin

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Peter Meerwald-Stadler]

> >> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> >> controller and a thermal sensor. This patch adds the ADC driver which is
> >> based on the MFD for the same SoCs ADC.
> > 
> > nitpicking ahead

> [...]
> >> +
> >> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
> > 
> > static instead of const?

> static const then?

no, the const is redundant and ignored
-Wignored-qualifiers gives a warning

just static, no const

see
http://stackoverflow.com/questions/12052468/type-qualifiers-on-function-return-type

> >> +{
> >> +  return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
> >> +}
> >> +
> >> +struct soc_specific {
> >> +  const int   temp_offset;
> > 
> > wondering why you constify every member?
> > 
> 
> Because they're supposed to be fixed values? It won't change in runtime.
> Is there any reason why I shouldn't do that?

yes, but using the entire struct as const has the same effect;
constifying individual members makes more sense if there are also 
non-const members

nothing wrong, just unusual

> >> +  const int   temp_scale;
> >> +  const unsigned int  tp_mode_en;
> >> +  const unsigned int  tp_adc_select;
> >> +  const unsigned int  (*adc_chan_select)(unsigned int chan);
> >> +};
> [...]
> >> +static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int 
> >> *val,
> >> +  unsigned int irq)
> >> +{
> >> +  struct sun4i_gpadc_dev *info = iio_priv(indio_dev);
> >> +  int ret = 0;
> >> +
> >> +  pm_runtime_get_sync(indio_dev->dev.parent);
> >> +  mutex_lock(>mutex);
> >> +
> >> +  reinit_completion(>completion);
> >> +
> >> +  regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC,
> >> +   SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
> >> +   SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH);
> > 
> > check retval? here and elsewhere for regmap_write()
> > 
> 
> ACK. What should I do with the retval then?
> 
> There are some in:
> - sun4i_gpadc_read called for read_raws => return which error code (or
> -ret only?)?

I'd just return ret; in the other cases I think it's ok to ignore errors

> - sun4i_gpadc_runtime_suspend => return value of ret, but that would
> cancel the suspend right?
> - sun4i_gpadc_runtime_resume => return value of ret, but that would
> cancel resume right?
> - sun4i_gpadc_probe in the error gotos => probe already failing so do we
> actually care if regmap_update_bits fails? If yes, what's the expected
> behaviour?
> - sun4i_gpadc_remove => return value of ret, but that would mean the
> remove failed right?
> 
> [...]
> >> +static int sun4i_gpadc_read_raw(struct iio_dev *indio_dev,
> >> +  struct iio_chan_spec const *chan, int *val,
> >> +  int *val2, long mask)
> >> +{
> >> +  int ret;
> >> +
> >> +  switch (mask) {
> >> +  case IIO_CHAN_INFO_OFFSET:
> >> +  ret = sun4i_gpadc_temp_offset(indio_dev, val);
> >> +  if (ret)
> >> +  return ret;
> >> +
> >> +  return IIO_VAL_INT;
> >> +  case IIO_CHAN_INFO_RAW:
> >> +  if (chan->type == IIO_VOLTAGE) {
> >> +  ret = sun4i_gpadc_adc_read(indio_dev, chan->channel,
> >> + val);
> >> +  if (ret)
> >> +  return ret;
> > 
> > could share the "if (ret) return ret;" between code path
> > 
> 
> ACK.
> 
> [...]
> >> +static int sun4i_irq_init(struct platform_device *pdev, const char *name,
> >> +irq_handler_t handler, const char *devname,
> >> +unsigned int *irq, atomic_t *atomic)
> >> +{
> >> +  int ret;
> >> +  struct sun4i_gpadc_mfd_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
> >> +  struct sun4i_gpadc_dev *info = iio_priv(dev_get_drvdata(>dev));
> >> +
> >> +  /*
> >> +   * Once the interrupt is activated, the IP continuously performs
> >> +   * conversions thus throws interrupts. The interrupt is activated right
> >> +   * after being requested but we want to control when these interrupts
> >> +   * occurs thus we disable it right after being requested. However, an
> > 
> > occur
> > 
> 
> ACK for all typos.
> Thanks!
> 
> Quentin
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 

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

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Peter Meerwald-Stadler]

> >> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> >> controller and a thermal sensor. This patch adds the ADC driver which is
> >> based on the MFD for the same SoCs ADC.
> > 
> > nitpicking ahead

> [...]
> >> +
> >> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
> > 
> > static instead of const?

> static const then?

no, the const is redundant and ignored
-Wignored-qualifiers gives a warning

just static, no const

see
http://stackoverflow.com/questions/12052468/type-qualifiers-on-function-return-type

> >> +{
> >> +  return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
> >> +}
> >> +
> >> +struct soc_specific {
> >> +  const int   temp_offset;
> > 
> > wondering why you constify every member?
> > 
> 
> Because they're supposed to be fixed values? It won't change in runtime.
> Is there any reason why I shouldn't do that?

yes, but using the entire struct as const has the same effect;
constifying individual members makes more sense if there are also 
non-const members

nothing wrong, just unusual

> >> +  const int   temp_scale;
> >> +  const unsigned int  tp_mode_en;
> >> +  const unsigned int  tp_adc_select;
> >> +  const unsigned int  (*adc_chan_select)(unsigned int chan);
> >> +};
> [...]
> >> +static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int 
> >> *val,
> >> +  unsigned int irq)
> >> +{
> >> +  struct sun4i_gpadc_dev *info = iio_priv(indio_dev);
> >> +  int ret = 0;
> >> +
> >> +  pm_runtime_get_sync(indio_dev->dev.parent);
> >> +  mutex_lock(>mutex);
> >> +
> >> +  reinit_completion(>completion);
> >> +
> >> +  regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC,
> >> +   SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
> >> +   SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH);
> > 
> > check retval? here and elsewhere for regmap_write()
> > 
> 
> ACK. What should I do with the retval then?
> 
> There are some in:
> - sun4i_gpadc_read called for read_raws => return which error code (or
> -ret only?)?

I'd just return ret; in the other cases I think it's ok to ignore errors

> - sun4i_gpadc_runtime_suspend => return value of ret, but that would
> cancel the suspend right?
> - sun4i_gpadc_runtime_resume => return value of ret, but that would
> cancel resume right?
> - sun4i_gpadc_probe in the error gotos => probe already failing so do we
> actually care if regmap_update_bits fails? If yes, what's the expected
> behaviour?
> - sun4i_gpadc_remove => return value of ret, but that would mean the
> remove failed right?
> 
> [...]
> >> +static int sun4i_gpadc_read_raw(struct iio_dev *indio_dev,
> >> +  struct iio_chan_spec const *chan, int *val,
> >> +  int *val2, long mask)
> >> +{
> >> +  int ret;
> >> +
> >> +  switch (mask) {
> >> +  case IIO_CHAN_INFO_OFFSET:
> >> +  ret = sun4i_gpadc_temp_offset(indio_dev, val);
> >> +  if (ret)
> >> +  return ret;
> >> +
> >> +  return IIO_VAL_INT;
> >> +  case IIO_CHAN_INFO_RAW:
> >> +  if (chan->type == IIO_VOLTAGE) {
> >> +  ret = sun4i_gpadc_adc_read(indio_dev, chan->channel,
> >> + val);
> >> +  if (ret)
> >> +  return ret;
> > 
> > could share the "if (ret) return ret;" between code path
> > 
> 
> ACK.
> 
> [...]
> >> +static int sun4i_irq_init(struct platform_device *pdev, const char *name,
> >> +irq_handler_t handler, const char *devname,
> >> +unsigned int *irq, atomic_t *atomic)
> >> +{
> >> +  int ret;
> >> +  struct sun4i_gpadc_mfd_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
> >> +  struct sun4i_gpadc_dev *info = iio_priv(dev_get_drvdata(>dev));
> >> +
> >> +  /*
> >> +   * Once the interrupt is activated, the IP continuously performs
> >> +   * conversions thus throws interrupts. The interrupt is activated right
> >> +   * after being requested but we want to control when these interrupts
> >> +   * occurs thus we disable it right after being requested. However, an
> > 
> > occur
> > 
> 
> ACK for all typos.
> Thanks!
> 
> Quentin
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 

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

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 05/09/2016 09:07, Maxime Ripard wrote:
> Hi,
> 
> Nitpicks ahead.
> 
> On Thu, Sep 01, 2016 at 04:05:05PM +0200, Quentin Schulz wrote:
>> +info->soc_specific = (struct soc_specific 
>> *)platform_get_device_id(pdev)->driver_data;
> 
> This line is still rather long. How about calling the field "data" and
> the structure gpadc_data?
> 

driver_data is coming from the platform_device_id
(http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L498)
so can't really change that.
I could change the structure to gpadc_data, that would save me 2
characters, still 13 characters above the 80 characters limit however.

>> +
>> +tzd = devm_thermal_zone_of_sensor_register(pdev->dev.parent, 0, info,
>> +   _ts_tz_ops);
> 
> A comment on why you put the parent device structure and not the
> device itself like you're doing on all the other calls in that probe
> would be nice.
> 

Indeed.

Thanks,
Quentin

> Thanks!
> Maxime
> 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 05/09/2016 09:07, Maxime Ripard wrote:
> Hi,
> 
> Nitpicks ahead.
> 
> On Thu, Sep 01, 2016 at 04:05:05PM +0200, Quentin Schulz wrote:
>> +info->soc_specific = (struct soc_specific 
>> *)platform_get_device_id(pdev)->driver_data;
> 
> This line is still rather long. How about calling the field "data" and
> the structure gpadc_data?
> 

driver_data is coming from the platform_device_id
(http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L498)
so can't really change that.
I could change the structure to gpadc_data, that would save me 2
characters, still 13 characters above the 80 characters limit however.

>> +
>> +tzd = devm_thermal_zone_of_sensor_register(pdev->dev.parent, 0, info,
>> +   _ts_tz_ops);
> 
> A comment on why you put the parent device structure and not the
> device itself like you're doing on all the other calls in that probe
> would be nice.
> 

Indeed.

Thanks,
Quentin

> Thanks!
> Maxime
> 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Maxime Ripard]

Hi,

Nitpicks ahead.

On Thu, Sep 01, 2016 at 04:05:05PM +0200, Quentin Schulz wrote:
> + info->soc_specific = (struct soc_specific 
> *)platform_get_device_id(pdev)->driver_data;

This line is still rather long. How about calling the field "data" and
the structure gpadc_data?

> +
> + tzd = devm_thermal_zone_of_sensor_register(pdev->dev.parent, 0, info,
> +_ts_tz_ops);

A comment on why you put the parent device structure and not the
device itself like you're doing on all the other calls in that probe
would be nice.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Maxime Ripard]

Hi,

Nitpicks ahead.

On Thu, Sep 01, 2016 at 04:05:05PM +0200, Quentin Schulz wrote:
> + info->soc_specific = (struct soc_specific 
> *)platform_get_device_id(pdev)->driver_data;

This line is still rather long. How about calling the field "data" and
the structure gpadc_data?

> +
> + tzd = devm_thermal_zone_of_sensor_register(pdev->dev.parent, 0, info,
> +_ts_tz_ops);

A comment on why you put the parent device structure and not the
device itself like you're doing on all the other calls in that probe
would be nice.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/09/2016 18:12, Peter Meerwald-Stadler wrote:
> 
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
> 
> nitpicking ahead
>  
[...]
>> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
>> b/drivers/iio/adc/sun4i-gpadc-iio.c
>> new file mode 100644
>> index 000..a93d36d
>> --- /dev/null
>> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>> @@ -0,0 +1,525 @@
>> +/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
>> + *
>> + * Copyright (c) 2016 Quentin Schulz 
> 
> email address is incomplete
> 

As in my other patches, thanks!

>> + *
>> + * 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.
>> + *
>> + * The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> + * controller and a thermal sensor.
>> + * The thermal sensor works only when the ADC acts as a touchscreen 
>> controller
>> + * and is configured to throw an interrupt every fixed periods of time (let 
>> say
>> + * every X seconds).
>> + * One would be tempted to disable the IP on the hardware side rather than
>> + * disabling interrupts to save some power but that reset the internal 
>> clock of
> 
> resets
> 
>> + * the IP, resulting in having to wait X seconds every time we want to read 
>> the
>> + * value of the thermal sensor.
>> + * This is also the reason of using autosuspend in pm_runtime. If there 
>> were no
> 
> there was no
> 
[...]
>> +
>> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
> 
> static instead of const?
> 

static const then?

>> +{
>> +return SUN4I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>> +}
>> +
>> +const unsigned int sun6i_gpadc_chan_select(unsigned int chan)
> 
> static instead of const?
> 

static const then?

>> +{
>> +return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>> +}
>> +
>> +struct soc_specific {
>> +const int   temp_offset;
> 
> wondering why you constify every member?
> 

Because they're supposed to be fixed values? It won't change in runtime.
Is there any reason why I shouldn't do that?

>> +const int   temp_scale;
>> +const unsigned int  tp_mode_en;
>> +const unsigned int  tp_adc_select;
>> +const unsigned int  (*adc_chan_select)(unsigned int chan);
>> +};
[...]
>> +static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int 
>> *val,
>> +unsigned int irq)
>> +{
>> +struct sun4i_gpadc_dev *info = iio_priv(indio_dev);
>> +int ret = 0;
>> +
>> +pm_runtime_get_sync(indio_dev->dev.parent);
>> +mutex_lock(>mutex);
>> +
>> +reinit_completion(>completion);
>> +
>> +regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
>> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH);
> 
> check retval? here and elsewhere for regmap_write()
> 

ACK. What should I do with the retval then?

There are some in:
- sun4i_gpadc_read called for read_raws => return which error code (or
-ret only?)?
- sun4i_gpadc_runtime_suspend => return value of ret, but that would
cancel the suspend right?
- sun4i_gpadc_runtime_resume => return value of ret, but that would
cancel resume right?
- sun4i_gpadc_probe in the error gotos => probe already failing so do we
actually care if regmap_update_bits fails? If yes, what's the expected
behaviour?
- sun4i_gpadc_remove => return value of ret, but that would mean the
remove failed right?

[...]
>> +static int sun4i_gpadc_read_raw(struct iio_dev *indio_dev,
>> +struct iio_chan_spec const *chan, int *val,
>> +int *val2, long mask)
>> +{
>> +int ret;
>> +
>> +switch (mask) {
>> +case IIO_CHAN_INFO_OFFSET:
>> +ret = sun4i_gpadc_temp_offset(indio_dev, val);
>> +if (ret)
>> +return ret;
>> +
>> +return IIO_VAL_INT;
>> +case IIO_CHAN_INFO_RAW:
>> +if (chan->type == IIO_VOLTAGE) {
>> +ret = sun4i_gpadc_adc_read(indio_dev, chan->channel,
>> +   val);
>> +if (ret)
>> +return ret;
> 
> could share the "if (ret) return ret;" between code path
> 

ACK.

[...]
>> +static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>> +  irq_handler_t handler, const char *devname,
>> +  unsigned int *irq, atomic_t *atomic)
>> +{
>> +int ret;
>> +struct sun4i_gpadc_mfd_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
>> +struct sun4i_gpadc_dev *info = iio_priv(dev_get_drvdata(>dev));
>> +
>> +/*
>> + * Once the interrupt is activated, the IP continuously performs
>> +   

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/09/2016 18:12, Peter Meerwald-Stadler wrote:
> 
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
> 
> nitpicking ahead
>  
[...]
>> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
>> b/drivers/iio/adc/sun4i-gpadc-iio.c
>> new file mode 100644
>> index 000..a93d36d
>> --- /dev/null
>> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>> @@ -0,0 +1,525 @@
>> +/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
>> + *
>> + * Copyright (c) 2016 Quentin Schulz 
> 
> email address is incomplete
> 

As in my other patches, thanks!

>> + *
>> + * 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.
>> + *
>> + * The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> + * controller and a thermal sensor.
>> + * The thermal sensor works only when the ADC acts as a touchscreen 
>> controller
>> + * and is configured to throw an interrupt every fixed periods of time (let 
>> say
>> + * every X seconds).
>> + * One would be tempted to disable the IP on the hardware side rather than
>> + * disabling interrupts to save some power but that reset the internal 
>> clock of
> 
> resets
> 
>> + * the IP, resulting in having to wait X seconds every time we want to read 
>> the
>> + * value of the thermal sensor.
>> + * This is also the reason of using autosuspend in pm_runtime. If there 
>> were no
> 
> there was no
> 
[...]
>> +
>> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
> 
> static instead of const?
> 

static const then?

>> +{
>> +return SUN4I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>> +}
>> +
>> +const unsigned int sun6i_gpadc_chan_select(unsigned int chan)
> 
> static instead of const?
> 

static const then?

>> +{
>> +return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
>> +}
>> +
>> +struct soc_specific {
>> +const int   temp_offset;
> 
> wondering why you constify every member?
> 

Because they're supposed to be fixed values? It won't change in runtime.
Is there any reason why I shouldn't do that?

>> +const int   temp_scale;
>> +const unsigned int  tp_mode_en;
>> +const unsigned int  tp_adc_select;
>> +const unsigned int  (*adc_chan_select)(unsigned int chan);
>> +};
[...]
>> +static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int 
>> *val,
>> +unsigned int irq)
>> +{
>> +struct sun4i_gpadc_dev *info = iio_priv(indio_dev);
>> +int ret = 0;
>> +
>> +pm_runtime_get_sync(indio_dev->dev.parent);
>> +mutex_lock(>mutex);
>> +
>> +reinit_completion(>completion);
>> +
>> +regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
>> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH);
> 
> check retval? here and elsewhere for regmap_write()
> 

ACK. What should I do with the retval then?

There are some in:
- sun4i_gpadc_read called for read_raws => return which error code (or
-ret only?)?
- sun4i_gpadc_runtime_suspend => return value of ret, but that would
cancel the suspend right?
- sun4i_gpadc_runtime_resume => return value of ret, but that would
cancel resume right?
- sun4i_gpadc_probe in the error gotos => probe already failing so do we
actually care if regmap_update_bits fails? If yes, what's the expected
behaviour?
- sun4i_gpadc_remove => return value of ret, but that would mean the
remove failed right?

[...]
>> +static int sun4i_gpadc_read_raw(struct iio_dev *indio_dev,
>> +struct iio_chan_spec const *chan, int *val,
>> +int *val2, long mask)
>> +{
>> +int ret;
>> +
>> +switch (mask) {
>> +case IIO_CHAN_INFO_OFFSET:
>> +ret = sun4i_gpadc_temp_offset(indio_dev, val);
>> +if (ret)
>> +return ret;
>> +
>> +return IIO_VAL_INT;
>> +case IIO_CHAN_INFO_RAW:
>> +if (chan->type == IIO_VOLTAGE) {
>> +ret = sun4i_gpadc_adc_read(indio_dev, chan->channel,
>> +   val);
>> +if (ret)
>> +return ret;
> 
> could share the "if (ret) return ret;" between code path
> 

ACK.

[...]
>> +static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>> +  irq_handler_t handler, const char *devname,
>> +  unsigned int *irq, atomic_t *atomic)
>> +{
>> +int ret;
>> +struct sun4i_gpadc_mfd_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
>> +struct sun4i_gpadc_dev *info = iio_priv(dev_get_drvdata(>dev));
>> +
>> +/*
>> + * Once the interrupt is activated, the IP continuously performs
>> + * conversions thus throws 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/09/2016 16:35, Jonathan Cameron wrote:
> On 01/09/16 15:05, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree. This registers
>> the driver in the thermal framework.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences (registers bit and temperature computation)
>> between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz 
> One utterly trivial point about unrolling code ordering inline.
> 
> Other than the bit about patch 1 I'm basically happy with this..

ACK. Will revert this patch in v5. Thanks.

> However I would like some input (i.e. an Ack) from thermal given this
> sets up a thermal zone.
> 
> Zhang or Eduardo, could you take a quick look at this and confirm you
> are happy with it?
> 
> Thanks,
> 
> Jonathan
[...]
>> +
>> +err_map:
>> +iio_map_array_unregister(indio_dev);
>> +
>> +err_fifo_irq:
>> +/* Disable FIFO_DATA_PENDING interrupt on hardware side. */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>> +   0);
>> +
>> +err_temp_irq:
>> +/* Disable TEMP_DATA_PENDING interrupt on hardware side. */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN,
>> +   0);
>> +
>> +err:
>> +pm_runtime_put(>dev);
>> +pm_runtime_disable(>dev);
>> +
>> +return ret;
>> +}
>> +
>> +static int sun4i_gpadc_remove(struct platform_device *pdev)
>> +{
>> +struct sun4i_gpadc_dev *info;
>> +struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> +info = iio_priv(indio_dev);
>> +iio_device_unregister(indio_dev);
>> +iio_map_array_unregister(indio_dev);
>> +pm_runtime_put(>dev);
>> +pm_runtime_disable(>dev);
> Its really minor but in the interests of 'obviously correct' making
> review easy I'd rather everything in the remove was in the reverse order
> of probe (and hence the same as the error path in probe for most of it).
> 
> That would put the pm_runtime stuff last I think..
> 
> If you weren't rerolling anyway over patch 1 I'd probably have just let
> this go, but might as well make this trivial change as well.
> 

I'm going with the following order:
pm_runtime_put
pm_runtime_disable
regmap_update_bits
iio_map_array_unregister
iio_device_unregister

Is that okay? (I don't really know which one of iio_map_array_unregister
or iio_device_unregister to put first, if it matters in any way).

Thanks!
Quentin
> 
>> +/*
>> + * Disable TEMP_DATA_PENDING and FIFO_DATA_PENDING interrupts on
>> + * hardware side.
>> + */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN |
>> +SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>> +   0);
>> +
>> +return 0;
>> +}
>> +
>> +static const struct platform_device_id sun4i_gpadc_id[] = {
>> +{ "sun4i-a10-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ "sun5i-a13-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ "sun6i-a31-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver sun4i_gpadc_driver = {
>> +.driver = {
>> +.name = "sun4i-gpadc-iio",
>> +.pm = _gpadc_pm_ops,
>> +},
>> +.id_table = sun4i_gpadc_id,
>> +.probe = sun4i_gpadc_probe,
>> +.remove = sun4i_gpadc_remove,
>> +};
>> +
>> +module_platform_driver(sun4i_gpadc_driver);
>> +
>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>> +MODULE_AUTHOR("Quentin Schulz ");
>> +MODULE_LICENSE("GPL v2");
>>
> 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/09/2016 16:35, Jonathan Cameron wrote:
> On 01/09/16 15:05, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree. This registers
>> the driver in the thermal framework.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences (registers bit and temperature computation)
>> between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz 
> One utterly trivial point about unrolling code ordering inline.
> 
> Other than the bit about patch 1 I'm basically happy with this..

ACK. Will revert this patch in v5. Thanks.

> However I would like some input (i.e. an Ack) from thermal given this
> sets up a thermal zone.
> 
> Zhang or Eduardo, could you take a quick look at this and confirm you
> are happy with it?
> 
> Thanks,
> 
> Jonathan
[...]
>> +
>> +err_map:
>> +iio_map_array_unregister(indio_dev);
>> +
>> +err_fifo_irq:
>> +/* Disable FIFO_DATA_PENDING interrupt on hardware side. */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>> +   0);
>> +
>> +err_temp_irq:
>> +/* Disable TEMP_DATA_PENDING interrupt on hardware side. */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN,
>> +   0);
>> +
>> +err:
>> +pm_runtime_put(>dev);
>> +pm_runtime_disable(>dev);
>> +
>> +return ret;
>> +}
>> +
>> +static int sun4i_gpadc_remove(struct platform_device *pdev)
>> +{
>> +struct sun4i_gpadc_dev *info;
>> +struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> +info = iio_priv(indio_dev);
>> +iio_device_unregister(indio_dev);
>> +iio_map_array_unregister(indio_dev);
>> +pm_runtime_put(>dev);
>> +pm_runtime_disable(>dev);
> Its really minor but in the interests of 'obviously correct' making
> review easy I'd rather everything in the remove was in the reverse order
> of probe (and hence the same as the error path in probe for most of it).
> 
> That would put the pm_runtime stuff last I think..
> 
> If you weren't rerolling anyway over patch 1 I'd probably have just let
> this go, but might as well make this trivial change as well.
> 

I'm going with the following order:
pm_runtime_put
pm_runtime_disable
regmap_update_bits
iio_map_array_unregister
iio_device_unregister

Is that okay? (I don't really know which one of iio_map_array_unregister
or iio_device_unregister to put first, if it matters in any way).

Thanks!
Quentin
> 
>> +/*
>> + * Disable TEMP_DATA_PENDING and FIFO_DATA_PENDING interrupts on
>> + * hardware side.
>> + */
>> +regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>> +   SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN |
>> +SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>> +   0);
>> +
>> +return 0;
>> +}
>> +
>> +static const struct platform_device_id sun4i_gpadc_id[] = {
>> +{ "sun4i-a10-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ "sun5i-a13-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ "sun6i-a31-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>> +{ /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver sun4i_gpadc_driver = {
>> +.driver = {
>> +.name = "sun4i-gpadc-iio",
>> +.pm = _gpadc_pm_ops,
>> +},
>> +.id_table = sun4i_gpadc_id,
>> +.probe = sun4i_gpadc_probe,
>> +.remove = sun4i_gpadc_remove,
>> +};
>> +
>> +module_platform_driver(sun4i_gpadc_driver);
>> +
>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>> +MODULE_AUTHOR("Quentin Schulz ");
>> +MODULE_LICENSE("GPL v2");
>>
> 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Peter Meerwald-Stadler]

> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.

nitpicking ahead
 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree. This registers
> the driver in the thermal framework.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences (registers bit and temperature computation)
> between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz 
> ---
> 
> v4:
>  - rename files and variables from sunxi* to sun4i*,
>  - shorten sunxi_gpadc_soc_specific structure to soc_specific,
>  - factorize sysfs ADC and temp read_raws,
>  - use cached values when read_raw times out (except before a first value
>is gotten),
>  - remove mutex locks and unlocks from runtime_pm functions,
>  - factorize irq initializations,
>  - initialize temp_data and fifo_data values to -1 (error value),
>  - "impersonate" MFD to register in thermal framework,
>  - deactivate hardware interrupts one by one when probe fails or when
>removing driver instead of blindly deactivating all hardware interrupts,
>  - selects THERMAL_OF in Kconfig,
> 
> v3:
>  - correct wrapping,
>  - add comment about thermal sensor inner working,
>  - move defines in mfd header,
>  - use structure to define SoC specific registers or behaviour,
>  - attach this structure to the device according to of_device_id of the
>platform device,
>  - use new mutex instead of iio_dev mutex,
>  - use atomic flags to avoid race between request_irq and disable_irq in
>probe,
>  - switch from processed value to raw, offset and scale values for
>temperature ADC channel,
>  - remove faulty sentinel in iio_chan_spec array,
>  - add pm_runtime support,
>  - register thermal sensor in thermal framework (forgotten since the
>beginning whereas it is present in current sun4i-ts driver),
>  - remove useless ret variables to store return value of regmap_reads,
>  - move comments on thermal sensor acquisition period in code instead of
>header,
>  - adding goto label to unregister iio_map_array when failing to register
>iio_dev,
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig   |  13 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/sun4i-gpadc-iio.c | 525 
> ++
>  3 files changed, 539 insertions(+)
>  create mode 100644 drivers/iio/adc/sun4i-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..ea36a4f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,19 @@ config ROCKCHIP_SARADC
> To compile this driver as a module, choose M here: the
> module will be called rockchip_saradc.
>  
> +config SUN4I_GPADC
> + tristate "Support for the Allwinner SoCs GPADC"
> + depends on IIO
> + depends on MFD_SUN4I_GPADC
> + select THERMAL_OF
> + help
> +   Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +   GPADC. This ADC provides 4 channels which can be used as an ADC or as
> +   a touchscreen input and one channel for thermal sensor.
> +
> +   To compile this driver as a module, choose M here: the module will be
> +   called sun4i-gpadc-iio.
> +
>  config TI_ADC081C
>   tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>   depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..204372d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>  obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>  obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> +obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
> b/drivers/iio/adc/sun4i-gpadc-iio.c
> new file mode 100644
> index 000..a93d36d
> --- /dev/null
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -0,0 +1,525 @@
> +/* ADC driver for sunxi platforms' 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Peter Meerwald-Stadler]

> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.

nitpicking ahead
 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree. This registers
> the driver in the thermal framework.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences (registers bit and temperature computation)
> between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz 
> ---
> 
> v4:
>  - rename files and variables from sunxi* to sun4i*,
>  - shorten sunxi_gpadc_soc_specific structure to soc_specific,
>  - factorize sysfs ADC and temp read_raws,
>  - use cached values when read_raw times out (except before a first value
>is gotten),
>  - remove mutex locks and unlocks from runtime_pm functions,
>  - factorize irq initializations,
>  - initialize temp_data and fifo_data values to -1 (error value),
>  - "impersonate" MFD to register in thermal framework,
>  - deactivate hardware interrupts one by one when probe fails or when
>removing driver instead of blindly deactivating all hardware interrupts,
>  - selects THERMAL_OF in Kconfig,
> 
> v3:
>  - correct wrapping,
>  - add comment about thermal sensor inner working,
>  - move defines in mfd header,
>  - use structure to define SoC specific registers or behaviour,
>  - attach this structure to the device according to of_device_id of the
>platform device,
>  - use new mutex instead of iio_dev mutex,
>  - use atomic flags to avoid race between request_irq and disable_irq in
>probe,
>  - switch from processed value to raw, offset and scale values for
>temperature ADC channel,
>  - remove faulty sentinel in iio_chan_spec array,
>  - add pm_runtime support,
>  - register thermal sensor in thermal framework (forgotten since the
>beginning whereas it is present in current sun4i-ts driver),
>  - remove useless ret variables to store return value of regmap_reads,
>  - move comments on thermal sensor acquisition period in code instead of
>header,
>  - adding goto label to unregister iio_map_array when failing to register
>iio_dev,
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig   |  13 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/sun4i-gpadc-iio.c | 525 
> ++
>  3 files changed, 539 insertions(+)
>  create mode 100644 drivers/iio/adc/sun4i-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..ea36a4f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,19 @@ config ROCKCHIP_SARADC
> To compile this driver as a module, choose M here: the
> module will be called rockchip_saradc.
>  
> +config SUN4I_GPADC
> + tristate "Support for the Allwinner SoCs GPADC"
> + depends on IIO
> + depends on MFD_SUN4I_GPADC
> + select THERMAL_OF
> + help
> +   Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +   GPADC. This ADC provides 4 channels which can be used as an ADC or as
> +   a touchscreen input and one channel for thermal sensor.
> +
> +   To compile this driver as a module, choose M here: the module will be
> +   called sun4i-gpadc-iio.
> +
>  config TI_ADC081C
>   tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>   depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..204372d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>  obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>  obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> +obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
> b/drivers/iio/adc/sun4i-gpadc-iio.c
> new file mode 100644
> index 000..a93d36d
> --- /dev/null
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -0,0 +1,525 @@
> +/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
> + *
> + 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 01/09/16 15:05, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.
> 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree. This registers
> the driver in the thermal framework.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences (registers bit and temperature computation)
> between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz 
One utterly trivial point about unrolling code ordering inline.

Other than the bit about patch 1 I'm basically happy with this..
However I would like some input (i.e. an Ack) from thermal given this
sets up a thermal zone.

Zhang or Eduardo, could you take a quick look at this and confirm you
are happy with it?

Thanks,

Jonathan
> ---
> 
> v4:
>  - rename files and variables from sunxi* to sun4i*,
>  - shorten sunxi_gpadc_soc_specific structure to soc_specific,
>  - factorize sysfs ADC and temp read_raws,
>  - use cached values when read_raw times out (except before a first value
>is gotten),
>  - remove mutex locks and unlocks from runtime_pm functions,
>  - factorize irq initializations,
>  - initialize temp_data and fifo_data values to -1 (error value),
>  - "impersonate" MFD to register in thermal framework,
>  - deactivate hardware interrupts one by one when probe fails or when
>removing driver instead of blindly deactivating all hardware interrupts,
>  - selects THERMAL_OF in Kconfig,
> 
> v3:
>  - correct wrapping,
>  - add comment about thermal sensor inner working,
>  - move defines in mfd header,
>  - use structure to define SoC specific registers or behaviour,
>  - attach this structure to the device according to of_device_id of the
>platform device,
>  - use new mutex instead of iio_dev mutex,
>  - use atomic flags to avoid race between request_irq and disable_irq in
>probe,
>  - switch from processed value to raw, offset and scale values for
>temperature ADC channel,
>  - remove faulty sentinel in iio_chan_spec array,
>  - add pm_runtime support,
>  - register thermal sensor in thermal framework (forgotten since the
>beginning whereas it is present in current sun4i-ts driver),
>  - remove useless ret variables to store return value of regmap_reads,
>  - move comments on thermal sensor acquisition period in code instead of
>header,
>  - adding goto label to unregister iio_map_array when failing to register
>iio_dev,
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig   |  13 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/sun4i-gpadc-iio.c | 525 
> ++
>  3 files changed, 539 insertions(+)
>  create mode 100644 drivers/iio/adc/sun4i-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..ea36a4f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,19 @@ config ROCKCHIP_SARADC
> To compile this driver as a module, choose M here: the
> module will be called rockchip_saradc.
>  
> +config SUN4I_GPADC
> + tristate "Support for the Allwinner SoCs GPADC"
> + depends on IIO
> + depends on MFD_SUN4I_GPADC
> + select THERMAL_OF
> + help
> +   Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +   GPADC. This ADC provides 4 channels which can be used as an ADC or as
> +   a touchscreen input and one channel for thermal sensor.
> +
> +   To compile this driver as a module, choose M here: the module will be
> +   called sun4i-gpadc-iio.
> +
>  config TI_ADC081C
>   tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>   depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..204372d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>  obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>  obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> +obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
>  obj-$(CONFIG_TI_ADC081C) += 

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 01/09/16 15:05, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.
> 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree. This registers
> the driver in the thermal framework.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences (registers bit and temperature computation)
> between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz 
One utterly trivial point about unrolling code ordering inline.

Other than the bit about patch 1 I'm basically happy with this..
However I would like some input (i.e. an Ack) from thermal given this
sets up a thermal zone.

Zhang or Eduardo, could you take a quick look at this and confirm you
are happy with it?

Thanks,

Jonathan
> ---
> 
> v4:
>  - rename files and variables from sunxi* to sun4i*,
>  - shorten sunxi_gpadc_soc_specific structure to soc_specific,
>  - factorize sysfs ADC and temp read_raws,
>  - use cached values when read_raw times out (except before a first value
>is gotten),
>  - remove mutex locks and unlocks from runtime_pm functions,
>  - factorize irq initializations,
>  - initialize temp_data and fifo_data values to -1 (error value),
>  - "impersonate" MFD to register in thermal framework,
>  - deactivate hardware interrupts one by one when probe fails or when
>removing driver instead of blindly deactivating all hardware interrupts,
>  - selects THERMAL_OF in Kconfig,
> 
> v3:
>  - correct wrapping,
>  - add comment about thermal sensor inner working,
>  - move defines in mfd header,
>  - use structure to define SoC specific registers or behaviour,
>  - attach this structure to the device according to of_device_id of the
>platform device,
>  - use new mutex instead of iio_dev mutex,
>  - use atomic flags to avoid race between request_irq and disable_irq in
>probe,
>  - switch from processed value to raw, offset and scale values for
>temperature ADC channel,
>  - remove faulty sentinel in iio_chan_spec array,
>  - add pm_runtime support,
>  - register thermal sensor in thermal framework (forgotten since the
>beginning whereas it is present in current sun4i-ts driver),
>  - remove useless ret variables to store return value of regmap_reads,
>  - move comments on thermal sensor acquisition period in code instead of
>header,
>  - adding goto label to unregister iio_map_array when failing to register
>iio_dev,
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig   |  13 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/sun4i-gpadc-iio.c | 525 
> ++
>  3 files changed, 539 insertions(+)
>  create mode 100644 drivers/iio/adc/sun4i-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..ea36a4f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,19 @@ config ROCKCHIP_SARADC
> To compile this driver as a module, choose M here: the
> module will be called rockchip_saradc.
>  
> +config SUN4I_GPADC
> + tristate "Support for the Allwinner SoCs GPADC"
> + depends on IIO
> + depends on MFD_SUN4I_GPADC
> + select THERMAL_OF
> + help
> +   Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +   GPADC. This ADC provides 4 channels which can be used as an ADC or as
> +   a touchscreen input and one channel for thermal sensor.
> +
> +   To compile this driver as a module, choose M here: the module will be
> +   called sun4i-gpadc-iio.
> +
>  config TI_ADC081C
>   tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>   depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..204372d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>  obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>  obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> +obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  

[PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. This patch adds the ADC driver which is
based on the MFD for the same SoCs ADC.

This also registers the thermal adc channel in the iio map array so
iio_hwmon could use it without modifying the Device Tree. This registers
the driver in the thermal framework.

This driver probes on three different platform_device_id to take into
account slight differences (registers bit and temperature computation)
between Allwinner SoCs ADCs.

Signed-off-by: Quentin Schulz 
---

v4:
 - rename files and variables from sunxi* to sun4i*,
 - shorten sunxi_gpadc_soc_specific structure to soc_specific,
 - factorize sysfs ADC and temp read_raws,
 - use cached values when read_raw times out (except before a first value
   is gotten),
 - remove mutex locks and unlocks from runtime_pm functions,
 - factorize irq initializations,
 - initialize temp_data and fifo_data values to -1 (error value),
 - "impersonate" MFD to register in thermal framework,
 - deactivate hardware interrupts one by one when probe fails or when
   removing driver instead of blindly deactivating all hardware interrupts,
 - selects THERMAL_OF in Kconfig,

v3:
 - correct wrapping,
 - add comment about thermal sensor inner working,
 - move defines in mfd header,
 - use structure to define SoC specific registers or behaviour,
 - attach this structure to the device according to of_device_id of the
   platform device,
 - use new mutex instead of iio_dev mutex,
 - use atomic flags to avoid race between request_irq and disable_irq in
   probe,
 - switch from processed value to raw, offset and scale values for
   temperature ADC channel,
 - remove faulty sentinel in iio_chan_spec array,
 - add pm_runtime support,
 - register thermal sensor in thermal framework (forgotten since the
   beginning whereas it is present in current sun4i-ts driver),
 - remove useless ret variables to store return value of regmap_reads,
 - move comments on thermal sensor acquisition period in code instead of
   header,
 - adding goto label to unregister iio_map_array when failing to register
   iio_dev,

v2:
 - add SUNXI_GPADC_ prefixes for defines,
 - correct typo in Kconfig,
 - reorder alphabetically includes, makefile,
 - add license header,
 - fix architecture variations not being handled in interrupt handlers or
   read raw functions,
 - fix unability to return negative values from thermal sensor,
 - add gotos to reduce code repetition,
 - fix irq variable being unsigned int instead of int,
 - remove useless dev_err and dev_info,
 - deactivate all interrupts if probe fails,
 - fix iio_device_register on NULL variable,
 - deactivate ADC in the IP when probe fails or when removing driver,

 drivers/iio/adc/Kconfig   |  13 +
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/sun4i-gpadc-iio.c | 525 ++
 3 files changed, 539 insertions(+)
 create mode 100644 drivers/iio/adc/sun4i-gpadc-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 25378c5..ea36a4f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -384,6 +384,19 @@ config ROCKCHIP_SARADC
  To compile this driver as a module, choose M here: the
  module will be called rockchip_saradc.
 
+config SUN4I_GPADC
+   tristate "Support for the Allwinner SoCs GPADC"
+   depends on IIO
+   depends on MFD_SUN4I_GPADC
+   select THERMAL_OF
+   help
+ Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
+ GPADC. This ADC provides 4 channels which can be used as an ADC or as
+ a touchscreen input and one channel for thermal sensor.
+
+ To compile this driver as a module, choose M here: the module will be
+ called sun4i-gpadc-iio.
+
 config TI_ADC081C
tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 38638d4..204372d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
 obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
 obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
+obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
b/drivers/iio/adc/sun4i-gpadc-iio.c
new file mode 100644
index 000..a93d36d
--- /dev/null
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -0,0 +1,525 @@
+/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
+ *
+ * Copyright (c) 2016 Quentin Schulz 
+ *
+ * This program is free software; you can redistribute it and/or modify it 
under
+ * the terms of the GNU General Public 

[PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. This patch adds the ADC driver which is
based on the MFD for the same SoCs ADC.

This also registers the thermal adc channel in the iio map array so
iio_hwmon could use it without modifying the Device Tree. This registers
the driver in the thermal framework.

This driver probes on three different platform_device_id to take into
account slight differences (registers bit and temperature computation)
between Allwinner SoCs ADCs.

Signed-off-by: Quentin Schulz 
---

v4:
 - rename files and variables from sunxi* to sun4i*,
 - shorten sunxi_gpadc_soc_specific structure to soc_specific,
 - factorize sysfs ADC and temp read_raws,
 - use cached values when read_raw times out (except before a first value
   is gotten),
 - remove mutex locks and unlocks from runtime_pm functions,
 - factorize irq initializations,
 - initialize temp_data and fifo_data values to -1 (error value),
 - "impersonate" MFD to register in thermal framework,
 - deactivate hardware interrupts one by one when probe fails or when
   removing driver instead of blindly deactivating all hardware interrupts,
 - selects THERMAL_OF in Kconfig,

v3:
 - correct wrapping,
 - add comment about thermal sensor inner working,
 - move defines in mfd header,
 - use structure to define SoC specific registers or behaviour,
 - attach this structure to the device according to of_device_id of the
   platform device,
 - use new mutex instead of iio_dev mutex,
 - use atomic flags to avoid race between request_irq and disable_irq in
   probe,
 - switch from processed value to raw, offset and scale values for
   temperature ADC channel,
 - remove faulty sentinel in iio_chan_spec array,
 - add pm_runtime support,
 - register thermal sensor in thermal framework (forgotten since the
   beginning whereas it is present in current sun4i-ts driver),
 - remove useless ret variables to store return value of regmap_reads,
 - move comments on thermal sensor acquisition period in code instead of
   header,
 - adding goto label to unregister iio_map_array when failing to register
   iio_dev,

v2:
 - add SUNXI_GPADC_ prefixes for defines,
 - correct typo in Kconfig,
 - reorder alphabetically includes, makefile,
 - add license header,
 - fix architecture variations not being handled in interrupt handlers or
   read raw functions,
 - fix unability to return negative values from thermal sensor,
 - add gotos to reduce code repetition,
 - fix irq variable being unsigned int instead of int,
 - remove useless dev_err and dev_info,
 - deactivate all interrupts if probe fails,
 - fix iio_device_register on NULL variable,
 - deactivate ADC in the IP when probe fails or when removing driver,

 drivers/iio/adc/Kconfig   |  13 +
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/sun4i-gpadc-iio.c | 525 ++
 3 files changed, 539 insertions(+)
 create mode 100644 drivers/iio/adc/sun4i-gpadc-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 25378c5..ea36a4f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -384,6 +384,19 @@ config ROCKCHIP_SARADC
  To compile this driver as a module, choose M here: the
  module will be called rockchip_saradc.
 
+config SUN4I_GPADC
+   tristate "Support for the Allwinner SoCs GPADC"
+   depends on IIO
+   depends on MFD_SUN4I_GPADC
+   select THERMAL_OF
+   help
+ Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
+ GPADC. This ADC provides 4 channels which can be used as an ADC or as
+ a touchscreen input and one channel for thermal sensor.
+
+ To compile this driver as a module, choose M here: the module will be
+ called sun4i-gpadc-iio.
+
 config TI_ADC081C
tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 38638d4..204372d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
 obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
 obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
+obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c 
b/drivers/iio/adc/sun4i-gpadc-iio.c
new file mode 100644
index 000..a93d36d
--- /dev/null
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -0,0 +1,525 @@
+/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
+ *
+ * Copyright (c) 2016 Quentin Schulz 
+ *
+ * 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