Re: [PATCH] iio: adc: Add Renesas GyroADC driver

On 12/30/2016 08:50 PM, Peter Meerwald-Stadler wrote:

quoted

Add IIO driver for the Renesas RCar GyroADC block. This block is a
simple 4/8-channel ADC which samples 12/15/24 bits of data every
cycle from all channels.

comments below
 

quoted

Signed-off-by: Marek Vasut <redacted>
Cc: Geert Uytterhoeven <redacted>
Cc: Simon Horman <horms+renesas-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
---
 .../bindings/iio/adc/renesas,gyroadc.txt           |  38 +++
 MAINTAINERS                                        |   6 +
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/rcar_gyro_adc.c                    | 379 +++++++++++++++++++++
 5 files changed, 434 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
 create mode 100644 drivers/iio/adc/rcar_gyro_adc.c

diff --git a/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
new file mode 100644
index 0000000..3fd5f57
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt

@@ -0,0 +1,38 @@
+* Renesas RCar GyroADC device driver
+
+Required properties:
+- compatible:	Should be "renesas,rcar-gyroadc" for regular GyroADC or
+		"renesas,rcar-gyroadc-r8a7792" for GyroADC without interrupt
+		block found in R8A7792.
+- reg:		Address and length of the register set for the device
+- clocks:	References to all the clocks specified in the clock-names
+		property as specified in
+		Documentation/devicetree/bindings/clock/clock-bindings.txt.
+- clock-names:	Shall contain "fck" and "if". The "fck" are the GyroADC block

"fck" is...

quoted

+		clock, the "if" are the interface clock.

"if" is ...

quoted

+		power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;

this is just an example and not appropriate here?

Oh, copy-paste error, thanks :)

quoted

+- power-domains: Must contain a reference to the PM domain, if available.
+- renesas,gyroadc-mode:	GyroADC mode of operation, must be either of:
+			1 - MB88101A mode, 12bit sampling, 4 channels
+			2 - ADCS7476 mode, 15bit sampling, 8 channels
+			3 - MAX1162 mode,  16bit sampling, 8 channels
+- renesas,gyroadc-vref:	Array of reference voltage values for each input to
+			the GyroADC, in uV. Array must have 4 elemenets for

elements

All spelling fixed.

[...]

quoted

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 99c0514..4a4cac7 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig

@@ -408,6 +408,16 @@ config QCOM_SPMI_VADC
 	  To compile this driver as a module, choose M here: the module will
 	  be called qcom-spmi-vadc.
 
+config RCAR_GYRO_ADC
+	tristate "Renesas RCAR GyroADC driver"
+	depends on ARCH_RCAR_GEN2 || (ARM && COMPILE_TEST)
+	help
+	  Say yes here to build support for the GyroADC found in Renesas
+	  RCar Gen2 SoCs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called rcar-gyroadc.

called rcar_gyro_adc?

Why so ? The driver is really named rcar-gyroadc , I guess I should
rename either the file or the driver to keep things consistent. So
probably the file .

quoted

+
 config ROCKCHIP_SARADC
 	tristate "Rockchip SARADC driver"
 	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)

diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile

[...]

quoted

+
+#define RCAR_GYROADC_CLOCK_LENGTH		0x08
+#define RCAR_GYROADC_1_25MS_LENGTH		0x0c
+
+#define RCAR_GYROADC_REALTIME_DATA(ch)		(0x10 + ((ch) * 4))
+#define RCAR_GYROADC_100MS_ADDED_DATA(ch)	(0x30 + ((ch) * 4))
+#define RCAR_GYROADC_10MS_AVG_DATA(ch)		(0x50 + ((ch) * 4))
+
+#define RCAR_GYROADC_FIFO_STATUS		0x70
+#define RCAR_GYROADC_FIFO_STATUS_EMPTY(ch)	BIT(0 + (4 * (ch)))

FIFO_STATUS_... is not used (yet)

Is it a problem to have a complete register layout here ?

4*ch looks suspicious for ch==8??

Well yes, channel is in range 0..7 :)

quoted

+#define RCAR_GYROADC_FIFO_STATUS_FULL(ch)	BIT(1 + (4 * (ch)))
+#define RCAR_GYROADC_FIFO_STATUS_ERROR(ch)	BIT(2 + (4 * (ch)))
+
+#define RCAR_GYROADC_INTR			0x74
+#define RCAR_GYROADC_INTR_INT			BIT(0)
+
+#define RCAR_GYROADC_INTENR			0x78
+#define RCAR_GYROADC_INTENR_INTEN		BIT(0)
+
+#define RCAR_GYROADC_SAMPLE_RATE		800	/* Hz */

[...]

quoted

+#define RCAR_GYROADC_CHAN(_idx, _chan_type, _realbits) {	\
+	.type			= (_chan_type),			\

_chan_type is IIO_VOLTAGE always?

Yep, fixed.

quoted

+	.indexed		= 1,				\
+	.channel		= (_idx),			\
+	.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				    BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+	.scan_index		= (_idx),			\

no buffered mode yet, so strictly no need for a scan_index and scan_type

OK

quoted

+	.scan_type	= {					\
+		.sign		= 'u',				\
+		.realbits	= (_realbits),			\
+		.storagebits	= 16,				\
+	},							\
+}
+
+static const struct iio_chan_spec rcar_gyroadc_iio_channels_1[] = {
+	RCAR_GYROADC_CHAN(0, IIO_VOLTAGE, 12),
+	RCAR_GYROADC_CHAN(1, IIO_VOLTAGE, 12),
+	RCAR_GYROADC_CHAN(2, IIO_VOLTAGE, 12),
+	RCAR_GYROADC_CHAN(3, IIO_VOLTAGE, 12),
+};
+
+static const struct iio_chan_spec rcar_gyroadc_iio_channels_2[] = {
+	RCAR_GYROADC_CHAN(0, IIO_VOLTAGE, 15),
+	RCAR_GYROADC_CHAN(1, IIO_VOLTAGE, 15),
+	RCAR_GYROADC_CHAN(2, IIO_VOLTAGE, 15),
+	RCAR_GYROADC_CHAN(3, IIO_VOLTAGE, 15),
+	RCAR_GYROADC_CHAN(4, IIO_VOLTAGE, 15),
+	RCAR_GYROADC_CHAN(5, IIO_VOLTAGE, 15),
+	RCAR_GYROADC_CHAN(6, IIO_VOLTAGE, 15),
+	RCAR_GYROADC_CHAN(7, IIO_VOLTAGE, 15),
+};
+
+/*
+ * NOTE: The data we receive in mode 3 from MAX1162 have MSByte = 0,
+ *       therefore we only use 16bit realbits here instead of 24.
+ */
+static const struct iio_chan_spec rcar_gyroadc_iio_channels_3[] = {
+	RCAR_GYROADC_CHAN(0, IIO_VOLTAGE, 16),
+	RCAR_GYROADC_CHAN(1, IIO_VOLTAGE, 16),
+	RCAR_GYROADC_CHAN(2, IIO_VOLTAGE, 16),
+	RCAR_GYROADC_CHAN(3, IIO_VOLTAGE, 16),
+	RCAR_GYROADC_CHAN(4, IIO_VOLTAGE, 16),
+	RCAR_GYROADC_CHAN(5, IIO_VOLTAGE, 16),
+	RCAR_GYROADC_CHAN(6, IIO_VOLTAGE, 16),
+	RCAR_GYROADC_CHAN(7, IIO_VOLTAGE, 16),
+};
+
+static int rcar_gyroadc_read_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int *val, int *val2, long mask)
+{
+	struct rcar_gyroadc *priv = iio_priv(indio_dev);
+	unsigned int datareg = RCAR_GYROADC_REALTIME_DATA(chan->channel);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->type != IIO_VOLTAGE)
+			return -EINVAL;
+
+		if (iio_buffer_enabled(indio_dev))
+			return -EBUSY;

use iio_device_claim_direct_mode()

Why ? Do I really need the mutex locking here ?

quoted

+
+		*val = readl(priv->regs + datareg);
+		*val &= BIT(chan->scan_type.realbits) - 1;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		*val2 = (priv->vref_uv[chan->channel] * 1000) / 0x10000;
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = RCAR_GYROADC_SAMPLE_RATE;
+		*val2 = 0;

*val2 = 0 not needed

OK

[...]

quoted

+	indio_dev->name = dev_name(dev);
+	indio_dev->dev.parent = dev;
+	indio_dev->dev.of_node = pdev->dev.of_node;
+	indio_dev->info = &rcar_gyroadc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	if (mode == 1) {

maybe do the mode differentiation only once, any with a switch?

Done

[...]

-- 
Best regards,
Marek Vasut