Re: [PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz <quentin.schulz@free-electrons.com>]

[-- Attachment #1.1: Type: text/plain, Size: 11828 bytes --]

On 18/07/2016 14:57, Maxime Ripard wrote:
> On Fri, Jul 15, 2016 at 11:59:12AM +0200, 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 driver probes on three different platform_device_id to take into
>> account slight differences between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> ---
>>
>> 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           |  12 ++
>>  drivers/iio/adc/Makefile          |   1 +
>>  drivers/iio/adc/sunxi-gpadc-iio.c | 417 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 430 insertions(+)
>>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 25378c5..184856f 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -338,6 +338,18 @@ config NAU7802
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called nau7802.
>>  
>> +config SUNXI_ADC
> 
> We try to avoid the SUNXI prefix usually, otherwise this driver will
> have a generic name (or at least is implicitly saying that it supports
> all the sunxi SoCs), while it supports only a subset of those SoCs.
> 

ACK. Will be replaced by SUN4I_GPADC.

>> +	tristate "ADC driver for sunxi platforms"
> 
> And you should also mention which ADC is supported, since we usually
> have several of them.
> 
> Something like "Support for the Allwinner SoCs GPADC"
> 

ACK.

>> +	depends on IIO
>> +	depends on MFD_SUNXI_ADC
> 
> The order of your patches is quite weird. You depend on an option that
> is not present yet?
> 

ACK. Will modify the order of patches to reflect the real order.

>> +	help
>> +	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
>> +	  ADC. 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
> 
> Your indentation is weird here, and the wrapping is likely to be wrong
> too.
> 

ACK.

[...]
>> @@ -0,0 +1,417 @@
>> +/* ADC driver for sunxi platforms
>> + *
>> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
>> + *
>> + * 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.
> 
> Your wrapping is wrong.
> 

ACK.

>> + */
>> +
>> +#include <linux/completion.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/driver.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/mfd/sunxi-gpadc-mfd.h>
>> +
>> +#define SUNXI_GPADC_TP_CTRL0			0x00
>> +#define SUNXI_GPADC_TP_CTRL1			0x04
>> +#define SUNXI_GPADC_TP_CTRL2			0x08
>> +#define SUNXI_GPADC_TP_CTRL3			0x0c
>> +#define SUNXI_GPADC_TP_TPR			0x18
>> +#define SUNXI_GPADC_TP_CDAT			0x1c
>> +#define SUNXI_GPADC_TEMP_DATA			0x20
>> +#define SUNXI_GPADC_TP_DATA			0x24
>> +
>> +/* TP_CTRL0 bits */
>> +#define SUNXI_GPADC_ADC_FIRST_DLY(x)		((x) << 24) /* 8 bits */
>> +#define SUNXI_GPADC_ADC_FIRST_DLY_MODE		BIT(23)
>> +#define SUNXI_GPADC_ADC_CLK_SELECT		BIT(22)
>> +#define SUNXI_GPADC_ADC_CLK_DIVIDER(x)		((x) << 20) /* 2 bits */
>> +#define SUNXI_GPADC_FS_DIV(x)			((x) << 16) /* 4 bits */
>> +#define SUNXI_GPADC_T_ACQ(x)			((x) << 0)  /* 16 bits */
> 
> We usually prefer to have the bits defined directly after the
> registers, and prefixed with the name of the register they belong to.
> 
> Something like SUNXI_GPADC_TP_CTRL_T_ACQ in this case
> 

This modification induces the name of the bits to be really long:
SUNXI_GPADC_TP_CTRL1_SUN6I_TOUCH_PAN_CALI_EN for example. ACK anyway.

>> +
>> +/* TP_CTRL1 bits */
>> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x)	((x) << 12) /* 8 bits */
>> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN	BIT(9)
>> +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN		BIT(6)
>> +#define SUNXI_GPADC_TP_DUAL_EN			BIT(5)
>> +#define SUNXI_GPADC_TP_MODE_EN			BIT(4)
>> +#define SUNXI_GPADC_TP_ADC_SELECT		BIT(3)
>> +#define SUNXI_GPADC_ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */
> 
> Usually the comments are on the line above. However, if you really
> want to enforce something, you should rather mask the
> value. Otherwise, that comment is pretty useless.
> 

Do you mean something like that:
#define SUNXI_GPADC_ADC_CHAN_SELECT(x)		(GENMASK(2,0) & x) ?

>> +
>> +/* TP_CTRL1 bits for sun6i SOCs */
>> +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN	BIT(7)
>> +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN		BIT(6)
>> +#define SUNXI_GPADC_SUN6I_TP_MODE_EN		BIT(5)
>> +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT		BIT(4)
> 
> Shouldn't that go in either a common define or the touchscreen driver?
> 

Then shouldn't I put all defines in a common header? (sunxi-gpadc-mfd.h)

[...]
>> +/* TP_TPR bits */
>> +#define SUNXI_GPADC_TEMP_ENABLE(x)		((x) << 16)
>> +/* t = x * 256 * 16 / clkin */
> 
> That comment would be better next to the code that does that
> computation.
> 

ACK.

[...]
>> +	reinit_completion(&info->completion);
>> +	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
>> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
>> +			     SUNXI_GPADC_SUN6I_TP_MODE_EN |
>> +			     SUNXI_GPADC_SUN6I_TP_ADC_SELECT |
>> +			     SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(channel));
>> +	else
>> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
>> +			     SUNXI_GPADC_TP_MODE_EN |
>> +			     SUNXI_GPADC_TP_ADC_SELECT |
>> +			     SUNXI_GPADC_ADC_CHAN_SELECT(channel));
> 
> Using a function pointer that would compute this, or some fields in a
> structure to store this would be better.
> 

ACK.

[...]
>> +	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>> +		*val = info->temp_data * 133 - 257000;
>> +	else if (info->flags & SUNXI_GPADC_ARCH_SUN5I)
>> +		*val = info->temp_data * 100 - 144700;
>> +	else if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
>> +		*val = info->temp_data * 167 - 271000;
> 
> Ditto, having functions to comptue this and just store the function
> pointer would be better.
> 

As Jonathan suggests, we should better go with separate read_raws
(IIO_CHAN_RAW returns info->temp_data, INFO_CHAN_SCALE and
INFO_CHAN_OFFSET return a different value depending on the
architecture). So this would split the above code in separate functions
as you wanted.

[...]
>> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no TEMP_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_temp_data_irq_handler, 0,
>> +					   "temp_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request TEMP_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		goto err;
>> +	}
>> +
>> +	info->temp_data_irq = irq;
>> +	disable_irq(irq);
>> +
>> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no FIFO_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_fifo_data_irq_handler,
>> +					   0, "fifo_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		goto err;
>> +	}
>> +
>> +	info->fifo_data_irq = irq;
>> +	disable_irq(irq);
> 
> request_irq starts with irq enabled, which means that you can have an
> interrupt showing up between your call to request_irq and the
> disable_irq. 
> 
> How would that work?
> 

Same as what I answered in Jonathan's mail:

"Once the interrupt is activated, the IP performs continuous conversions
(temp_data_irq only periodically). I want these interrupts to be enabled
only when I read the sysfs file or we get useless interrupts.
In the current state of this driver's irq handlers, I only set values in
structures and all the needed structures are already initialized before
requesting irqs. So it does not look like a race. I can prevent races in
future versions by adding an atomic flag if wanted."

>> +
>> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to register iio map array\n");
>> +		goto err;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "could not register the device\n");
>> +		iio_map_array_unregister(indio_dev);
> 
> That should go in a separate label.
> 

ACK.

>> +		goto err;
>> +	}
>> +
>> +	return 0;
>> +
>> +err:
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
> 
> Why is that needed?
> 

This disables ADC and Temperature on the hardware side of the IP.
(mainly a shortcut to SUNXI_GPADC_TP_MODE_EN (or its architecture
variant) and SUNXI_GPADC_TEMP_ENABLE set to 0.

>> +	return ret;
>> +}
>> +
>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>> +{
>> +	struct sunxi_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);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct platform_device_id sunxi_gpadc_id[] = {
>> +	{ "sun4i-a10-gpadc-iio", SUNXI_GPADC_ARCH_SUN4I },
>> +	{ "sun5i-a13-gpadc-iio", SUNXI_GPADC_ARCH_SUN5I },
>> +	{ "sun6i-a31-gpadc-iio", SUNXI_GPADC_ARCH_SUN6I },
>> +	{ /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver sunxi_gpadc_driver = {
>> +	.driver = {
>> +		.name = "sunxi-gpadc-iio",
>> +	},
>> +	.id_table = sunxi_gpadc_id,
>> +	.probe = sunxi_gpadc_probe,
>> +	.remove = sunxi_gpadc_remove,
>> +};
> 
> Having some runtime_pm support for this would be great too.
> 

Basically disabling the ADC and interrupts (as in the remove) in
_suspend and _idle and reenabling everything in "before _suspend"-state
in _resume I guess?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]