On 04/09/2016 18:12, Peter Meerwald-Stadler wrote:

quoted

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
 

[...]

quoted

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
new file mode 100644
index 0000000..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 <quentin.schulz@free-electrons>

email address is incomplete

As in my other patches, thanks!

quoted

+ *
+ * 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

quoted

+ * 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

[...]

quoted

+
+const unsigned int sun4i_gpadc_chan_select(unsigned int chan)

static instead of const?

static const then?

quoted

+{
+	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?

quoted

+{
+	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?

quoted

+	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);
+};

[...]

quoted

+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(&info->mutex);
+
+	reinit_completion(&info->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?

[...]

quoted

+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.

[...]

quoted

+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(&pdev->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