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

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

[Marek Vasut]

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.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Simon Horman <horms+renesas@verge.net.au>
---
V2: - Spelling fixes
    - Rename the driver source file to rcar-gyroadc
    - Rework the channel sample width handling
    - Use iio_device_claim_mode_direct()
    - Rename "renesas,rcar-gyroadc" to "renesas,r8a7791-gyroadc" and
      rename "renesas,rcar-gyroadc-r8a7792" to "renesas,r8a7792-gyroadc"
      to match the new naming scheme (WARNING: scif uses the old one!)
    - Switch to using regulators for channel voltage reference, add new
      properties renesas,gyroadc-vref-chN-supply for N in 0..8
    - Handle vref regulators as optional to, make channels without
      vref regulator return EINVAL on read.
    - Fix module license to GPL
    - Drop interrupt.h include
    - Rename clk to iclk
    - Rename RCar to R-Car
    - Rework the invalid mode handling
    - Don't print error message on EPROBE_DEFER
    - Drop fclk handling, use runtime PM for that instead
---
 .../bindings/iio/adc/renesas,gyroadc.txt           |  52 +++
 MAINTAINERS                                        |   6 +
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/rcar-gyroadc.c                     | 455 +++++++++++++++++++++
 5 files changed, 524 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
 create mode 100644 drivers/iio/adc/rcar-gyroadc.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 000000000000..18f5164aefcc
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
@@ -0,0 +1,52 @@
+* Renesas RCar GyroADC device driver
+
+Required properties:
+- compatible:	Should be "renesas,r8a7791-gyroadc" for regular GyroADC or
+		"renesas,r8a7792-gyroadc" for a 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" is the GyroADC block
+		clock, the "if" is the interface clock.
+- 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
+
+Optional properties:
+- renesas,gyroadc-vref-ch0-supply: Phandle to channel 0 voltage reference regulator.
+- renesas,gyroadc-vref-ch1-supply: Phandle to channel 1 voltage reference regulator.
+- renesas,gyroadc-vref-ch2-supply: Phandle to channel 2 voltage reference regulator.
+- renesas,gyroadc-vref-ch3-supply: Phandle to channel 3 voltage reference regulator.
+- renesas,gyroadc-vref-ch4-supply: Phandle to channel 4 voltage reference regulator.
+- renesas,gyroadc-vref-ch5-supply: Phandle to channel 5 voltage reference regulator.
+- renesas,gyroadc-vref-ch6-supply: Phandle to channel 6 voltage reference regulator.
+- renesas,gyroadc-vref-ch7-supply: Phandle to channel 7 voltage reference regulator.
+
+Example:
+	vref_max1162: regulator-vref-max1162 {
+		compatible = "regulator-fixed";
+
+		regulator-name = "MAX1162 Vref";
+		regulator-min-microvolt = <4096000>;
+		regulator-max-microvolt = <4096000>;
+	};
+
+	&adc {
+		compatible = "renesas,r8a7791-gyroadc";
+		reg = <0 0xe6e54000 0 64>;
+		clocks = <&mstp9_clks R8A7791_CLK_GYROADC>, <&clk_65m>;
+		clock-names = "fck", "if";
+		power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
+
+		pinctrl-0 = <&adc_pins>;
+		pinctrl-names = "default";
+
+		renesas,gyroadc-vref-ch0-supply = <&vref_max1162>;
+		renesas,gyroadc-vref-ch1-supply = <&vref_max1162>;
+		renesas,gyroadc-mode = <3>;
+		status = "okay";
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 162d904d5cc3..751e760b751b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10271,6 +10271,12 @@ L:	linux-renesas-soc@vger.kernel.org
 F:	drivers/net/ethernet/renesas/
 F:	include/linux/sh_eth.h
 
+RENESAS RCAR GYROADC DRIVER
+M:	Marek Vasut <marek.vasut@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	drivers/iio/adc/rcar_gyro_adc.c
+
 RENESAS USB2 PHY DRIVER
 M:	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
 L:	linux-renesas-soc@vger.kernel.org
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 99c051490eff..4a4cac7d4e3d 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.
+
 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
index 7a40c04c311f..13db7c2bffc8 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_NAU7802) += nau7802.o
 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_RCAR_GYRO_ADC) += rcar-gyroadc.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
 obj-$(CONFIG_STX104) += stx104.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
new file mode 100644
index 000000000000..a995b81512a7
--- /dev/null
+++ b/drivers/iio/adc/rcar-gyroadc.c
@@ -0,0 +1,455 @@
+/*
+ * Renesas R-Car GyroADC driver
+ *
+ * Copyright 2016 Marek Vasut <marek.vasut@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of_platform.h>
+#include <linux/err.h>
+#include <linux/pm_runtime.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+
+/* GyroADC registers. */
+#define RCAR_GYROADC_MODE_SELECT		0x00
+#define RCAR_GYROADC_MODE_SELECT_1_MB88101A	0x0
+#define RCAR_GYROADC_MODE_SELECT_2_ADCS7476	0x1
+#define RCAR_GYROADC_MODE_SELECT_3_MAX1162	0x3
+
+#define RCAR_GYROADC_START_STOP			0x04
+#define RCAR_GYROADC_START_STOP_START		BIT(0)
+
+#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)))
+#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 */
+
+enum rcar_gyroadc_model {
+	RCAR_GYROADC_MODEL_DEFAULT,
+	RCAR_GYROADC_MODEL_R8A7792,
+};
+
+struct rcar_gyroadc {
+	struct device			*dev;
+	void __iomem			*regs;
+	struct clk			*iclk;
+	struct regulator		*vref[8];
+	unsigned int			num_channels;
+	enum rcar_gyroadc_model		model;
+	unsigned int			mode;
+	unsigned int			sample_width;
+	u32				buffer[8];
+};
+
+static void rcar_gyroadc_hw_init(struct rcar_gyroadc *priv)
+{
+	unsigned long clk_mhz = clk_get_rate(priv->iclk) / 1000000;
+
+	/* Stop the GyroADC. */
+	writel(0, priv->regs + RCAR_GYROADC_START_STOP);
+
+	/* Disable IRQ, except on V2H. */
+	if (priv->model != RCAR_GYROADC_MODEL_R8A7792)
+		writel(0, priv->regs + RCAR_GYROADC_INTENR);
+
+	/* Set mode and timing. */
+	writel(priv->mode, priv->regs + RCAR_GYROADC_MODE_SELECT);
+
+	if (priv->mode == RCAR_GYROADC_MODE_SELECT_1_MB88101A)
+		writel(clk_mhz * 10, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
+	else if (priv->mode == RCAR_GYROADC_MODE_SELECT_2_ADCS7476)
+		writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
+	else if (priv->mode == RCAR_GYROADC_MODE_SELECT_3_MAX1162)
+		writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
+	writel(clk_mhz * 1250, priv->regs + RCAR_GYROADC_1_25MS_LENGTH);
+
+	/*
+	 * We can possibly turn the sampling on/off on-demand to reduce power
+	 * consumption, but for the sake of quick availability of samples, we
+	 * don't do it now.
+	 */
+	writel(RCAR_GYROADC_START_STOP_START,
+	       priv->regs + RCAR_GYROADC_START_STOP);
+
+	/* Wait for the first conversion to complete. */
+	udelay(1250);
+}
+
+#define RCAR_GYROADC_CHAN(_idx) {				\
+	.type			= IIO_VOLTAGE,			\
+	.indexed		= 1,				\
+	.channel		= (_idx),			\
+	.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |	\
+				  BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+}
+
+static const struct iio_chan_spec rcar_gyroadc_iio_channels_1[] = {
+	RCAR_GYROADC_CHAN(0),
+	RCAR_GYROADC_CHAN(1),
+	RCAR_GYROADC_CHAN(2),
+	RCAR_GYROADC_CHAN(3),
+};
+
+static const struct iio_chan_spec rcar_gyroadc_iio_channels_2[] = {
+	RCAR_GYROADC_CHAN(0),
+	RCAR_GYROADC_CHAN(1),
+	RCAR_GYROADC_CHAN(2),
+	RCAR_GYROADC_CHAN(3),
+	RCAR_GYROADC_CHAN(4),
+	RCAR_GYROADC_CHAN(5),
+	RCAR_GYROADC_CHAN(6),
+	RCAR_GYROADC_CHAN(7),
+};
+
+/*
+ * 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),
+	RCAR_GYROADC_CHAN(1),
+	RCAR_GYROADC_CHAN(2),
+	RCAR_GYROADC_CHAN(3),
+	RCAR_GYROADC_CHAN(4),
+	RCAR_GYROADC_CHAN(5),
+	RCAR_GYROADC_CHAN(6),
+	RCAR_GYROADC_CHAN(7),
+};
+
+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);
+	struct regulator *consumer = priv->vref[chan->channel];
+	unsigned int datareg = RCAR_GYROADC_REALTIME_DATA(chan->channel);
+	unsigned int vref;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->type != IIO_VOLTAGE)
+			return -EINVAL;
+
+		/* Channel not connected. */
+		if (!consumer)
+			return -EINVAL;
+
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		*val = readl(priv->regs + datareg);
+		*val &= BIT(priv->sample_width) - 1;
+
+		iio_device_release_direct_mode(indio_dev);
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		/* Channel not connected. */
+		if (!consumer)
+			return -EINVAL;
+
+		vref = regulator_get_voltage(consumer);
+		*val = 0;
+		*val2 = (vref * 1000) / 0x10000;
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = RCAR_GYROADC_SAMPLE_RATE;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int rcar_gyroadc_reg_access(struct iio_dev *indio_dev,
+				   unsigned int reg, unsigned int writeval,
+				   unsigned int *readval)
+{
+	struct rcar_gyroadc *priv = iio_priv(indio_dev);
+	unsigned int maxreg = RCAR_GYROADC_INTENR;
+
+	if (readval == NULL)
+		return -EINVAL;
+
+	if (reg % 4)
+		return -EINVAL;
+
+	/* Handle the V2H case with missing interrupt block. */
+	if (priv->model == RCAR_GYROADC_MODEL_R8A7792)
+		maxreg = RCAR_GYROADC_FIFO_STATUS;
+
+	if (reg > maxreg)
+		return -EINVAL;
+
+	*readval = readl(priv->regs + reg);
+
+	return 0;
+}
+
+static const struct iio_info rcar_gyroadc_iio_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= rcar_gyroadc_read_raw,
+	.debugfs_reg_access	= rcar_gyroadc_reg_access,
+};
+
+static const struct of_device_id rcar_gyroadc_match[] = {
+	{
+		/* R-Car Gen2 compatible GyroADC */
+		.compatible	= "renesas,r8a7791-gyroadc",
+		.data		= (void *)RCAR_GYROADC_MODEL_DEFAULT,
+	}, {
+		/* R-Car V2H specialty without interrupt registers. */
+		.compatible	= "renesas,r8a7792-gyroadc",
+		.data		= (void *)RCAR_GYROADC_MODEL_R8A7792,
+	}, {
+		/* sentinel */
+	}
+};
+
+MODULE_DEVICE_TABLE(of, rcar_gyroadc_match);
+
+static int rcar_gyroadc_init_mode(struct iio_dev *indio_dev)
+{
+	struct rcar_gyroadc *priv = iio_priv(indio_dev);
+	struct device *dev = priv->dev;
+	int ret, mode;
+
+	ret = of_property_read_u32(dev->of_node, "renesas,gyroadc-mode", &mode);
+	if (ret) {
+		dev_err(dev, "Failed to get GyroADC mode (ret=%i)\n", ret);
+		return ret;
+	} else if (mode == 1) {
+		priv->num_channels = 4;
+		priv->mode = RCAR_GYROADC_MODE_SELECT_1_MB88101A;
+		priv->sample_width = 12;
+		indio_dev->channels = rcar_gyroadc_iio_channels_1;
+		indio_dev->num_channels =
+			ARRAY_SIZE(rcar_gyroadc_iio_channels_1);
+	} else if (mode == 2) {
+		priv->num_channels = 8;
+		priv->mode = RCAR_GYROADC_MODE_SELECT_2_ADCS7476;
+		priv->sample_width = 15;
+		indio_dev->channels = rcar_gyroadc_iio_channels_2;
+		indio_dev->num_channels =
+			ARRAY_SIZE(rcar_gyroadc_iio_channels_2);
+	} else if (mode == 3) {
+		priv->num_channels = 8;
+		priv->mode = RCAR_GYROADC_MODE_SELECT_3_MAX1162;
+		priv->sample_width = 16;
+		indio_dev->channels = rcar_gyroadc_iio_channels_3;
+		indio_dev->num_channels =
+			ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
+	} else {
+		dev_err(dev, "Invalid GyroADC mode (mode=%i)\n", mode);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void rcar_gyroadc_deinit_supplies(struct iio_dev *indio_dev)
+{
+	struct rcar_gyroadc *priv = iio_priv(indio_dev);
+	int i;
+
+	for (i = 0; i < priv->num_channels; i++) {
+		if (!priv->vref[i])
+			continue;
+
+		regulator_disable(priv->vref[i]);
+	}
+}
+
+static int rcar_gyroadc_init_supplies(struct iio_dev *indio_dev)
+{
+	struct rcar_gyroadc *priv = iio_priv(indio_dev);
+	struct device *dev = priv->dev;
+	struct regulator *vref;
+	char name[25];
+	int i, ret;
+
+	for (i = 0; i < priv->num_channels; i++) {
+		snprintf(name, sizeof(name), "renesas,gyroadc-vref-ch%i", i);
+
+		vref = devm_regulator_get_optional(dev, name);
+		if (IS_ERR(vref)) {
+			/*
+			 * Regulator is not present, which means the channel
+			 * supply is not connected.
+			 */
+			dev_dbg(dev, "Regulator %s not connected\n", name);
+			continue;
+		}
+
+		priv->vref[i] = vref;
+	}
+
+	for (i = 0; i < priv->num_channels; i++) {
+		if (!priv->vref[i])
+			continue;
+
+		ret = regulator_enable(priv->vref[i]);
+		if (ret) {
+			dev_err(dev, "Failed to enable regulator %i (ret=%i)\n",
+				i, ret);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	rcar_gyroadc_deinit_supplies(indio_dev);
+	return ret;
+}
+
+static int rcar_gyroadc_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *of_id =
+		of_match_device(rcar_gyroadc_match, &pdev->dev);
+	struct device *dev = &pdev->dev;
+	struct rcar_gyroadc *priv;
+	struct iio_dev *indio_dev;
+	struct resource *mem;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
+	if (!indio_dev) {
+		dev_err(dev, "Failed to allocate IIO device.\n");
+		return -ENOMEM;
+	}
+
+	priv = iio_priv(indio_dev);
+	priv->dev = dev;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->regs = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	priv->iclk = devm_clk_get(dev, "if");
+	if (IS_ERR(priv->iclk)) {
+		ret = PTR_ERR(priv->iclk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get IF clock (ret=%i)\n", ret);
+		return ret;
+	}
+
+	ret = rcar_gyroadc_init_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = rcar_gyroadc_init_supplies(indio_dev);
+	if (ret)
+		return ret;
+
+	priv->model = (enum rcar_gyroadc_model)of_id->data;
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	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;
+
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
+	ret = clk_prepare_enable(priv->iclk);
+	if (ret) {
+		dev_err(dev, "Could not prepare or enable the IF clock.\n");
+		goto error_clk_if_enable;
+	}
+
+	rcar_gyroadc_hw_init(priv);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(dev, "Couldn't register IIO device.\n");
+		goto error_iio_device_register;
+	}
+
+	return 0;
+
+error_iio_device_register:
+	clk_disable_unprepare(priv->iclk);
+error_clk_if_enable:
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+	rcar_gyroadc_deinit_supplies(indio_dev);
+	return ret;
+}
+
+static int rcar_gyroadc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct rcar_gyroadc *priv = iio_priv(indio_dev);
+	struct device *dev = priv->dev;
+
+	/* Stop sampling */
+	writel(0, priv->regs + RCAR_GYROADC_START_STOP);
+
+	iio_device_unregister(indio_dev);
+	clk_disable_unprepare(priv->iclk);
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+	rcar_gyroadc_deinit_supplies(indio_dev);
+
+	return 0;
+}
+
+static struct platform_driver rcar_gyroadc_driver = {
+	.probe          = rcar_gyroadc_probe,
+	.remove         = rcar_gyroadc_remove,
+	.driver         = {
+		.name		= "rcar-gyroadc",
+		.of_match_table	= rcar_gyroadc_match,
+	},
+};
+
+module_platform_driver(rcar_gyroadc_driver);
+
+MODULE_AUTHOR("Marek Vasut <marek.vasut@gmail.com>");
+MODULE_DESCRIPTION("Renesas RCAR GyroADC driver");
+MODULE_LICENSE("GPL");
-- 
2.11.0

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

[Geert Uytterhoeven]

CC linux-renesas-soc

Hi Marek,

On Mon, Jan 9, 2017 at 1:03 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> 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.
>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> ---
> V2: - Spelling fixes

Thanks for the update!

>     - Rename the driver source file to rcar-gyroadc
>     - Rework the channel sample width handling
>     - Use iio_device_claim_mode_direct()
>     - Rename "renesas,rcar-gyroadc" to "renesas,r8a7791-gyroadc" and
>       rename "renesas,rcar-gyroadc-r8a7792" to "renesas,r8a7792-gyroadc"
>       to match the new naming scheme (WARNING: scif uses the old one!)
>     - Switch to using regulators for channel voltage reference, add new
>       properties renesas,gyroadc-vref-chN-supply for N in 0..8

0..7

>     - Handle vref regulators as optional to, make channels without
>       vref regulator return EINVAL on read.
>     - Fix module license to GPL
>     - Drop interrupt.h include
>     - Rename clk to iclk
>     - Rename RCar to R-Car
>     - Rework the invalid mode handling
>     - Don't print error message on EPROBE_DEFER
>     - Drop fclk handling, use runtime PM for that instead
> ---
>  .../bindings/iio/adc/renesas,gyroadc.txt           |  52 +++
>  MAINTAINERS                                        |   6 +
>  drivers/iio/adc/Kconfig                            |  10 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/rcar-gyroadc.c                     | 455 +++++++++++++++++++++
>  5 files changed, 524 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>  create mode 100644 drivers/iio/adc/rcar-gyroadc.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 000000000000..18f5164aefcc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
> @@ -0,0 +1,52 @@
> +* Renesas RCar GyroADC device driver
> +
> +Required properties:
> +- compatible:  Should be "renesas,r8a7791-gyroadc" for regular GyroADC or
> +               "renesas,r8a7792-gyroadc" for a GyroADC without interrupt
> +               block found in R8A7792.

I would have kept "renesas,rcar-gyroadc", too.

Upon closer look, GyroADC in r8a7792 aka R-Car V2H has builtin interrupt
functionality, (SPI IRQ 18), while other variants use the interrupt
functionality of the Speed-pulse I/F (SPI IRQ 236), which is not present
on V2H.

Hence I think we can distinguish between the two variants by looking at
the presence of an "interrupts" property, if we make that mandatory on
V2H. If/when the need arises later, non-V2H variants will need a phandle
to the Speed-pulse I/F to use its interrupt.

Then the driver can just match on "renesas,rcar-gyroadc", instead of any
other single version (all R-Car Gen1, 2, and 3 SoCs) in existence.

> +- 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" is the GyroADC block
> +               clock, the "if" is the interface clock.
> +- 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

While these mode numbers are documented in the datasheet as such, I'm
wondering whether we should use the actual values to be written to the
MODE_SEL bitfield instead:
  00 = Mode 1
  01 = Mode 2
  10 = Reserved
  11 = Mode 3

That would cater for future versions being extended to 4 modes, renaming
"Mode 3" to "Mode 4" in the process...

Thoughts?

> +Optional properties:
> +- renesas,gyroadc-vref-ch0-supply: Phandle to channel 0 voltage reference regulator.
> +- renesas,gyroadc-vref-ch1-supply: Phandle to channel 1 voltage reference regulator.
> +- renesas,gyroadc-vref-ch2-supply: Phandle to channel 2 voltage reference regulator.
> +- renesas,gyroadc-vref-ch3-supply: Phandle to channel 3 voltage reference regulator.
> +- renesas,gyroadc-vref-ch4-supply: Phandle to channel 4 voltage reference regulator.
> +- renesas,gyroadc-vref-ch5-supply: Phandle to channel 5 voltage reference regulator.
> +- renesas,gyroadc-vref-ch6-supply: Phandle to channel 6 voltage reference regulator.
> +- renesas,gyroadc-vref-ch7-supply: Phandle to channel 7 voltage reference regulator.

Why not an array of phandles? That would simplify parsing.

Also, "vref-supply" seems a fairly standard property name already in wide use.

> +
> +Example:
> +       vref_max1162: regulator-vref-max1162 {
> +               compatible = "regulator-fixed";
> +
> +               regulator-name = "MAX1162 Vref";
> +               regulator-min-microvolt = <4096000>;
> +               regulator-max-microvolt = <4096000>;
> +       };
> +
> +       &adc {
> +               compatible = "renesas,r8a7791-gyroadc";
> +               reg = <0 0xe6e54000 0 64>;
> +               clocks = <&mstp9_clks R8A7791_CLK_GYROADC>, <&clk_65m>;
> +               clock-names = "fck", "if";
> +               power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
> +
> +               pinctrl-0 = <&adc_pins>;
> +               pinctrl-names = "default";
> +
> +               renesas,gyroadc-vref-ch0-supply = <&vref_max1162>;
> +               renesas,gyroadc-vref-ch1-supply = <&vref_max1162>;
> +               renesas,gyroadc-mode = <3>;
> +               status = "okay";
> +       };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 162d904d5cc3..751e760b751b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10271,6 +10271,12 @@ L:     linux-renesas-soc@vger.kernel.org
>  F:     drivers/net/ethernet/renesas/
>  F:     include/linux/sh_eth.h
>
> +RENESAS RCAR GYROADC DRIVER

R-CAR

> +M:     Marek Vasut <marek.vasut@gmail.com>
> +L:     linux-iio@vger.kernel.org
> +S:     Supported
> +F:     drivers/iio/adc/rcar_gyro_adc.c
> +
>  RENESAS USB2 PHY DRIVER
>  M:     Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>  L:     linux-renesas-soc@vger.kernel.org
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 99c051490eff..4a4cac7d4e3d 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"

R-Car

> +       depends on ARCH_RCAR_GEN2 || (ARM && COMPILE_TEST)

This may be extended to R-Car Gen2 and Gen3 later.
Note that the former provides ARCH_RCAR_GEN2, but the latter just needs
ARCH_RENESAS

> +       help
> +         Say yes here to build support for the GyroADC found in Renesas
> +         RCar Gen2 SoCs.

R-Car

> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called rcar-gyroadc.
> +
>  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
> index 7a40c04c311f..13db7c2bffc8 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_NAU7802) += nau7802.o
>  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_RCAR_GYRO_ADC) += rcar-gyroadc.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>  obj-$(CONFIG_STX104) += stx104.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
> new file mode 100644
> index 000000000000..a995b81512a7
> --- /dev/null
> +++ b/drivers/iio/adc/rcar-gyroadc.c
> @@ -0,0 +1,455 @@
> +/*
> + * Renesas R-Car GyroADC driver
> + *
> + * Copyright 2016 Marek Vasut <marek.vasut@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_platform.h>
> +#include <linux/err.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +
> +/* GyroADC registers. */
> +#define RCAR_GYROADC_MODE_SELECT               0x00
> +#define RCAR_GYROADC_MODE_SELECT_1_MB88101A    0x0
> +#define RCAR_GYROADC_MODE_SELECT_2_ADCS7476    0x1
> +#define RCAR_GYROADC_MODE_SELECT_3_MAX1162     0x3
> +
> +#define RCAR_GYROADC_START_STOP                        0x04
> +#define RCAR_GYROADC_START_STOP_START          BIT(0)
> +
> +#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)))
> +#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 */
> +
> +enum rcar_gyroadc_model {
> +       RCAR_GYROADC_MODEL_DEFAULT,
> +       RCAR_GYROADC_MODEL_R8A7792,
> +};
> +
> +struct rcar_gyroadc {
> +       struct device                   *dev;
> +       void __iomem                    *regs;
> +       struct clk                      *iclk;
> +       struct regulator                *vref[8];
> +       unsigned int                    num_channels;
> +       enum rcar_gyroadc_model         model;
> +       unsigned int                    mode;
> +       unsigned int                    sample_width;
> +       u32                             buffer[8];
> +};
> +
> +static void rcar_gyroadc_hw_init(struct rcar_gyroadc *priv)
> +{
> +       unsigned long clk_mhz = clk_get_rate(priv->iclk) / 1000000;
> +
> +       /* Stop the GyroADC. */
> +       writel(0, priv->regs + RCAR_GYROADC_START_STOP);
> +
> +       /* Disable IRQ, except on V2H. */
> +       if (priv->model != RCAR_GYROADC_MODEL_R8A7792)

Actually it's r8a7792 (V2H) which has the RCAR_GYROADC_INTENR register,
so the test is reversed.

> +               writel(0, priv->regs + RCAR_GYROADC_INTENR);
> +
> +       /* Set mode and timing. */
> +       writel(priv->mode, priv->regs + RCAR_GYROADC_MODE_SELECT);
> +
> +       if (priv->mode == RCAR_GYROADC_MODE_SELECT_1_MB88101A)
> +               writel(clk_mhz * 10, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
> +       else if (priv->mode == RCAR_GYROADC_MODE_SELECT_2_ADCS7476)
> +               writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
> +       else if (priv->mode == RCAR_GYROADC_MODE_SELECT_3_MAX1162)
> +               writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);

switch(priv->mode)?

> +       writel(clk_mhz * 1250, priv->regs + RCAR_GYROADC_1_25MS_LENGTH);
> +
> +       /*
> +        * We can possibly turn the sampling on/off on-demand to reduce power
> +        * consumption, but for the sake of quick availability of samples, we
> +        * don't do it now.
> +        */
> +       writel(RCAR_GYROADC_START_STOP_START,
> +              priv->regs + RCAR_GYROADC_START_STOP);
> +
> +       /* Wait for the first conversion to complete. */
> +       udelay(1250);
> +}
> +
> +#define RCAR_GYROADC_CHAN(_idx) {                              \
> +       .type                   = IIO_VOLTAGE,                  \
> +       .indexed                = 1,                            \
> +       .channel                = (_idx),                       \
> +       .info_mask_separate     = BIT(IIO_CHAN_INFO_RAW) |      \
> +                                 BIT(IIO_CHAN_INFO_SCALE),     \
> +       .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +}
> +
> +static const struct iio_chan_spec rcar_gyroadc_iio_channels_1[] = {
> +       RCAR_GYROADC_CHAN(0),
> +       RCAR_GYROADC_CHAN(1),
> +       RCAR_GYROADC_CHAN(2),
> +       RCAR_GYROADC_CHAN(3),
> +};
> +
> +static const struct iio_chan_spec rcar_gyroadc_iio_channels_2[] = {
> +       RCAR_GYROADC_CHAN(0),
> +       RCAR_GYROADC_CHAN(1),
> +       RCAR_GYROADC_CHAN(2),
> +       RCAR_GYROADC_CHAN(3),
> +       RCAR_GYROADC_CHAN(4),
> +       RCAR_GYROADC_CHAN(5),
> +       RCAR_GYROADC_CHAN(6),
> +       RCAR_GYROADC_CHAN(7),
> +};
> +
> +/*
> + * 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),
> +       RCAR_GYROADC_CHAN(1),
> +       RCAR_GYROADC_CHAN(2),
> +       RCAR_GYROADC_CHAN(3),
> +       RCAR_GYROADC_CHAN(4),
> +       RCAR_GYROADC_CHAN(5),
> +       RCAR_GYROADC_CHAN(6),
> +       RCAR_GYROADC_CHAN(7),
> +};
> +
> +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);
> +       struct regulator *consumer = priv->vref[chan->channel];
> +       unsigned int datareg = RCAR_GYROADC_REALTIME_DATA(chan->channel);
> +       unsigned int vref;
> +       int ret;
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               if (chan->type != IIO_VOLTAGE)
> +                       return -EINVAL;
> +
> +               /* Channel not connected. */
> +               if (!consumer)
> +                       return -EINVAL;
> +
> +               ret = iio_device_claim_direct_mode(indio_dev);
> +               if (ret)
> +                       return ret;
> +
> +               *val = readl(priv->regs + datareg);
> +               *val &= BIT(priv->sample_width) - 1;
> +
> +               iio_device_release_direct_mode(indio_dev);
> +
> +               return IIO_VAL_INT;
> +       case IIO_CHAN_INFO_SCALE:
> +               /* Channel not connected. */
> +               if (!consumer)
> +                       return -EINVAL;
> +
> +               vref = regulator_get_voltage(consumer);
> +               *val = 0;
> +               *val2 = (vref * 1000) / 0x10000;

DIV_ROUND_CLOSEST()?

> +               return IIO_VAL_INT_PLUS_NANO;
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               *val = RCAR_GYROADC_SAMPLE_RATE;
> +               return IIO_VAL_INT;
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +static int rcar_gyroadc_reg_access(struct iio_dev *indio_dev,
> +                                  unsigned int reg, unsigned int writeval,
> +                                  unsigned int *readval)
> +{
> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
> +       unsigned int maxreg = RCAR_GYROADC_INTENR;
> +
> +       if (readval == NULL)
> +               return -EINVAL;
> +
> +       if (reg % 4)
> +               return -EINVAL;
> +
> +       /* Handle the V2H case with missing interrupt block. */
> +       if (priv->model == RCAR_GYROADC_MODEL_R8A7792)

Inverted check, r8a7792 has more registers.

> +               maxreg = RCAR_GYROADC_FIFO_STATUS;
> +
> +       if (reg > maxreg)
> +               return -EINVAL;
> +
> +       *readval = readl(priv->regs + reg);
> +
> +       return 0;
> +}
> +
> +static const struct iio_info rcar_gyroadc_iio_info = {
> +       .driver_module          = THIS_MODULE,
> +       .read_raw               = rcar_gyroadc_read_raw,
> +       .debugfs_reg_access     = rcar_gyroadc_reg_access,
> +};
> +
> +static const struct of_device_id rcar_gyroadc_match[] = {
> +       {
> +               /* R-Car Gen2 compatible GyroADC */
> +               .compatible     = "renesas,r8a7791-gyroadc",
> +               .data           = (void *)RCAR_GYROADC_MODEL_DEFAULT,
> +       }, {
> +               /* R-Car V2H specialty without interrupt registers. */

with interrupt registers.

> +               .compatible     = "renesas,r8a7792-gyroadc",
> +               .data           = (void *)RCAR_GYROADC_MODEL_R8A7792,
> +       }, {
> +               /* sentinel */
> +       }
> +};
> +
> +MODULE_DEVICE_TABLE(of, rcar_gyroadc_match);
> +
> +static int rcar_gyroadc_init_mode(struct iio_dev *indio_dev)
> +{
> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
> +       struct device *dev = priv->dev;
> +       int ret, mode;
> +
> +       ret = of_property_read_u32(dev->of_node, "renesas,gyroadc-mode", &mode);
> +       if (ret) {
> +               dev_err(dev, "Failed to get GyroADC mode (ret=%i)\n", ret);
> +               return ret;
> +       } else if (mode == 1) {

switch(mode)

> +               priv->num_channels = 4;
> +               priv->mode = RCAR_GYROADC_MODE_SELECT_1_MB88101A;
> +               priv->sample_width = 12;
> +               indio_dev->channels = rcar_gyroadc_iio_channels_1;
> +               indio_dev->num_channels =
> +                       ARRAY_SIZE(rcar_gyroadc_iio_channels_1);
> +       } else if (mode == 2) {
> +               priv->num_channels = 8;
> +               priv->mode = RCAR_GYROADC_MODE_SELECT_2_ADCS7476;
> +               priv->sample_width = 15;
> +               indio_dev->channels = rcar_gyroadc_iio_channels_2;
> +               indio_dev->num_channels =
> +                       ARRAY_SIZE(rcar_gyroadc_iio_channels_2);
> +       } else if (mode == 3) {
> +               priv->num_channels = 8;
> +               priv->mode = RCAR_GYROADC_MODE_SELECT_3_MAX1162;
> +               priv->sample_width = 16;
> +               indio_dev->channels = rcar_gyroadc_iio_channels_3;
> +               indio_dev->num_channels =
> +                       ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
> +       } else {
> +               dev_err(dev, "Invalid GyroADC mode (mode=%i)\n", mode);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static void rcar_gyroadc_deinit_supplies(struct iio_dev *indio_dev)
> +{
> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
> +       int i;

unsigned int

> +
> +       for (i = 0; i < priv->num_channels; i++) {
> +               if (!priv->vref[i])
> +                       continue;
> +
> +               regulator_disable(priv->vref[i]);
> +       }
> +}
> +
> +static int rcar_gyroadc_init_supplies(struct iio_dev *indio_dev)
> +{
> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
> +       struct device *dev = priv->dev;
> +       struct regulator *vref;
> +       char name[25];
> +       int i, ret;

unsigned int

> +
> +       for (i = 0; i < priv->num_channels; i++) {
> +               snprintf(name, sizeof(name), "renesas,gyroadc-vref-ch%i", i);
> +
> +               vref = devm_regulator_get_optional(dev, name);
> +               if (IS_ERR(vref)) {
> +                       /*
> +                        * Regulator is not present, which means the channel
> +                        * supply is not connected.
> +                        */
> +                       dev_dbg(dev, "Regulator %s not connected\n", name);
> +                       continue;
> +               }
> +
> +               priv->vref[i] = vref;
> +       }
> +
> +       for (i = 0; i < priv->num_channels; i++) {
> +               if (!priv->vref[i])
> +                       continue;
> +
> +               ret = regulator_enable(priv->vref[i]);
> +               if (ret) {
> +                       dev_err(dev, "Failed to enable regulator %i (ret=%i)\n",
> +                               i, ret);
> +                       goto err;
> +               }
> +       }
> +
> +       return 0;
> +
> +err:
> +       rcar_gyroadc_deinit_supplies(indio_dev);
> +       return ret;
> +}
> +
> +static int rcar_gyroadc_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *of_id =
> +               of_match_device(rcar_gyroadc_match, &pdev->dev);
> +       struct device *dev = &pdev->dev;
> +       struct rcar_gyroadc *priv;
> +       struct iio_dev *indio_dev;
> +       struct resource *mem;
> +       int ret;
> +
> +       indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> +       if (!indio_dev) {
> +               dev_err(dev, "Failed to allocate IIO device.\n");
> +               return -ENOMEM;
> +       }
> +
> +       priv = iio_priv(indio_dev);
> +       priv->dev = dev;
> +
> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       priv->regs = devm_ioremap_resource(dev, mem);
> +       if (IS_ERR(priv->regs))
> +               return PTR_ERR(priv->regs);
> +
> +       priv->iclk = devm_clk_get(dev, "if");
> +       if (IS_ERR(priv->iclk)) {
> +               ret = PTR_ERR(priv->iclk);
> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(dev, "Failed to get IF clock (ret=%i)\n", ret);
> +               return ret;
> +       }
> +
> +       ret = rcar_gyroadc_init_mode(indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = rcar_gyroadc_init_supplies(indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       priv->model = (enum rcar_gyroadc_model)of_id->data;
> +
> +       platform_set_drvdata(pdev, indio_dev);
> +
> +       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;
> +
> +       pm_runtime_enable(dev);
> +       pm_runtime_get_sync(dev);
> +
> +       ret = clk_prepare_enable(priv->iclk);
> +       if (ret) {
> +               dev_err(dev, "Could not prepare or enable the IF clock.\n");
> +               goto error_clk_if_enable;
> +       }
> +
> +       rcar_gyroadc_hw_init(priv);
> +
> +       ret = iio_device_register(indio_dev);
> +       if (ret) {
> +               dev_err(dev, "Couldn't register IIO device.\n");
> +               goto error_iio_device_register;
> +       }
> +
> +       return 0;
> +
> +error_iio_device_register:
> +       clk_disable_unprepare(priv->iclk);
> +error_clk_if_enable:
> +       pm_runtime_put(dev);
> +       pm_runtime_disable(dev);
> +       rcar_gyroadc_deinit_supplies(indio_dev);
> +       return ret;
> +}
> +
> +static int rcar_gyroadc_remove(struct platform_device *pdev)
> +{
> +       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
> +       struct device *dev = priv->dev;
> +
> +       /* Stop sampling */
> +       writel(0, priv->regs + RCAR_GYROADC_START_STOP);
> +
> +       iio_device_unregister(indio_dev);
> +       clk_disable_unprepare(priv->iclk);
> +       pm_runtime_put(dev);
> +       pm_runtime_disable(dev);
> +       rcar_gyroadc_deinit_supplies(indio_dev);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver rcar_gyroadc_driver = {
> +       .probe          = rcar_gyroadc_probe,
> +       .remove         = rcar_gyroadc_remove,
> +       .driver         = {
> +               .name           = "rcar-gyroadc",
> +               .of_match_table = rcar_gyroadc_match,
> +       },
> +};
> +
> +module_platform_driver(rcar_gyroadc_driver);
> +
> +MODULE_AUTHOR("Marek Vasut <marek.vasut@gmail.com>");
> +MODULE_DESCRIPTION("Renesas RCAR GyroADC driver");

R-Car

> +MODULE_LICENSE("GPL");
> --
> 2.11.0

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marek Vasut]

On 01/09/2017 01:13 PM, Geert Uytterhoeven wrote:
> CC linux-renesas-soc
> 
> Hi Marek,

Hi!

> On Mon, Jan 9, 2017 at 1:03 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> 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.
>>
>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Simon Horman <horms+renesas@verge.net.au>
>> ---
>> V2: - Spelling fixes
> 
> Thanks for the update!
> 
>>     - Rename the driver source file to rcar-gyroadc
>>     - Rework the channel sample width handling
>>     - Use iio_device_claim_mode_direct()
>>     - Rename "renesas,rcar-gyroadc" to "renesas,r8a7791-gyroadc" and
>>       rename "renesas,rcar-gyroadc-r8a7792" to "renesas,r8a7792-gyroadc"
>>       to match the new naming scheme (WARNING: scif uses the old one!)
>>     - Switch to using regulators for channel voltage reference, add new
>>       properties renesas,gyroadc-vref-chN-supply for N in 0..8
> 
> 0..7

Fixed

>>     - Handle vref regulators as optional to, make channels without
>>       vref regulator return EINVAL on read.
>>     - Fix module license to GPL
>>     - Drop interrupt.h include
>>     - Rename clk to iclk
>>     - Rename RCar to R-Car
>>     - Rework the invalid mode handling
>>     - Don't print error message on EPROBE_DEFER
>>     - Drop fclk handling, use runtime PM for that instead
>> ---
>>  .../bindings/iio/adc/renesas,gyroadc.txt           |  52 +++
>>  MAINTAINERS                                        |   6 +
>>  drivers/iio/adc/Kconfig                            |  10 +
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/rcar-gyroadc.c                     | 455 +++++++++++++++++++++
>>  5 files changed, 524 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>>  create mode 100644 drivers/iio/adc/rcar-gyroadc.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 000000000000..18f5164aefcc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>> @@ -0,0 +1,52 @@
>> +* Renesas RCar GyroADC device driver
>> +
>> +Required properties:
>> +- compatible:  Should be "renesas,r8a7791-gyroadc" for regular GyroADC or
>> +               "renesas,r8a7792-gyroadc" for a GyroADC without interrupt
>> +               block found in R8A7792.
> 
> I would have kept "renesas,rcar-gyroadc", too.

Do we have some sort of standard practice here ? Ie. NXP SoCs use the
oldest compatible SoC in the DT compat string (so the driver can bind
to that) and another DT compat string with that particular SoC name
encoded in it (so it's possible to discern it in the future in the
driver, if there is some problem).

> Upon closer look, GyroADC in r8a7792 aka R-Car V2H has builtin interrupt
> functionality, (SPI IRQ 18), while other variants use the interrupt
> functionality of the Speed-pulse I/F (SPI IRQ 236), which is not present
> on V2H.
> 
> Hence I think we can distinguish between the two variants by looking at
> the presence of an "interrupts" property, if we make that mandatory on
> V2H. If/when the need arises later, non-V2H variants will need a phandle
> to the Speed-pulse I/F to use its interrupt.

Well, the interrupt is pretty much useless (it generates 100ms pulses),
all we want to do it set the register to 0x0 to make sure the block
doesn't generate any. Do we want to add interrupts property for just
this purpose ?

> Then the driver can just match on "renesas,rcar-gyroadc", instead of any
> other single version (all R-Car Gen1, 2, and 3 SoCs) in existence.
> 
>> +- 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" is the GyroADC block
>> +               clock, the "if" is the interface clock.
>> +- 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
> 
> While these mode numbers are documented in the datasheet as such, I'm
> wondering whether we should use the actual values to be written to the
> MODE_SEL bitfield instead:
>   00 = Mode 1
>   01 = Mode 2
>   10 = Reserved
>   11 = Mode 3
> 
> That would cater for future versions being extended to 4 modes, renaming
> "Mode 3" to "Mode 4" in the process...
> 
> Thoughts?

I don't care either way, IMO it's a matter of taste :)

>> +Optional properties:
>> +- renesas,gyroadc-vref-ch0-supply: Phandle to channel 0 voltage reference regulator.
>> +- renesas,gyroadc-vref-ch1-supply: Phandle to channel 1 voltage reference regulator.
>> +- renesas,gyroadc-vref-ch2-supply: Phandle to channel 2 voltage reference regulator.
>> +- renesas,gyroadc-vref-ch3-supply: Phandle to channel 3 voltage reference regulator.
>> +- renesas,gyroadc-vref-ch4-supply: Phandle to channel 4 voltage reference regulator.
>> +- renesas,gyroadc-vref-ch5-supply: Phandle to channel 5 voltage reference regulator.
>> +- renesas,gyroadc-vref-ch6-supply: Phandle to channel 6 voltage reference regulator.
>> +- renesas,gyroadc-vref-ch7-supply: Phandle to channel 7 voltage reference regulator.
> 
> Why not an array of phandles? That would simplify parsing.

Because if you connect ADC only to ie. channel 0 and 3 , specifying that
in an array would be a hassle .

> Also, "vref-supply" seems a fairly standard property name already in wide use.

The above would thus make this vref-chX-supply ?

>> +
>> +Example:
>> +       vref_max1162: regulator-vref-max1162 {
>> +               compatible = "regulator-fixed";
>> +
>> +               regulator-name = "MAX1162 Vref";
>> +               regulator-min-microvolt = <4096000>;
>> +               regulator-max-microvolt = <4096000>;
>> +       };
>> +
>> +       &adc {
>> +               compatible = "renesas,r8a7791-gyroadc";
>> +               reg = <0 0xe6e54000 0 64>;
>> +               clocks = <&mstp9_clks R8A7791_CLK_GYROADC>, <&clk_65m>;
>> +               clock-names = "fck", "if";
>> +               power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
>> +
>> +               pinctrl-0 = <&adc_pins>;
>> +               pinctrl-names = "default";
>> +
>> +               renesas,gyroadc-vref-ch0-supply = <&vref_max1162>;
>> +               renesas,gyroadc-vref-ch1-supply = <&vref_max1162>;
>> +               renesas,gyroadc-mode = <3>;
>> +               status = "okay";
>> +       };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 162d904d5cc3..751e760b751b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -10271,6 +10271,12 @@ L:     linux-renesas-soc@vger.kernel.org
>>  F:     drivers/net/ethernet/renesas/
>>  F:     include/linux/sh_eth.h
>>
>> +RENESAS RCAR GYROADC DRIVER
> 
> R-CAR

Fixed

>> +M:     Marek Vasut <marek.vasut@gmail.com>
>> +L:     linux-iio@vger.kernel.org
>> +S:     Supported
>> +F:     drivers/iio/adc/rcar_gyro_adc.c
>> +
>>  RENESAS USB2 PHY DRIVER
>>  M:     Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>  L:     linux-renesas-soc@vger.kernel.org
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 99c051490eff..4a4cac7d4e3d 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"
> 
> R-Car

Fixed

>> +       depends on ARCH_RCAR_GEN2 || (ARM && COMPILE_TEST)
> 
> This may be extended to R-Car Gen2 and Gen3 later.
> Note that the former provides ARCH_RCAR_GEN2, but the latter just needs
> ARCH_RENESAS

I hope so! But Gen2 is what I tested it on thus far.

>> +       help
>> +         Say yes here to build support for the GyroADC found in Renesas
>> +         RCar Gen2 SoCs.
> 
> R-Car

Fixed

>> +
>> +         To compile this driver as a module, choose M here: the
>> +         module will be called rcar-gyroadc.
>> +
>>  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
>> index 7a40c04c311f..13db7c2bffc8 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -39,6 +39,7 @@ obj-$(CONFIG_NAU7802) += nau7802.o
>>  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_RCAR_GYRO_ADC) += rcar-gyroadc.o
>>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>>  obj-$(CONFIG_STX104) += stx104.o
>>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>> diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
>> new file mode 100644
>> index 000000000000..a995b81512a7
>> --- /dev/null
>> +++ b/drivers/iio/adc/rcar-gyroadc.c
>> @@ -0,0 +1,455 @@
>> +/*
>> + * Renesas R-Car GyroADC driver
>> + *
>> + * Copyright 2016 Marek Vasut <marek.vasut@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/delay.h>
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/io.h>
>> +#include <linux/clk.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/err.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/trigger.h>
>> +
>> +/* GyroADC registers. */
>> +#define RCAR_GYROADC_MODE_SELECT               0x00
>> +#define RCAR_GYROADC_MODE_SELECT_1_MB88101A    0x0
>> +#define RCAR_GYROADC_MODE_SELECT_2_ADCS7476    0x1
>> +#define RCAR_GYROADC_MODE_SELECT_3_MAX1162     0x3
>> +
>> +#define RCAR_GYROADC_START_STOP                        0x04
>> +#define RCAR_GYROADC_START_STOP_START          BIT(0)
>> +
>> +#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)))
>> +#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 */
>> +
>> +enum rcar_gyroadc_model {
>> +       RCAR_GYROADC_MODEL_DEFAULT,
>> +       RCAR_GYROADC_MODEL_R8A7792,
>> +};
>> +
>> +struct rcar_gyroadc {
>> +       struct device                   *dev;
>> +       void __iomem                    *regs;
>> +       struct clk                      *iclk;
>> +       struct regulator                *vref[8];
>> +       unsigned int                    num_channels;
>> +       enum rcar_gyroadc_model         model;
>> +       unsigned int                    mode;
>> +       unsigned int                    sample_width;
>> +       u32                             buffer[8];
>> +};
>> +
>> +static void rcar_gyroadc_hw_init(struct rcar_gyroadc *priv)
>> +{
>> +       unsigned long clk_mhz = clk_get_rate(priv->iclk) / 1000000;
>> +
>> +       /* Stop the GyroADC. */
>> +       writel(0, priv->regs + RCAR_GYROADC_START_STOP);
>> +
>> +       /* Disable IRQ, except on V2H. */
>> +       if (priv->model != RCAR_GYROADC_MODEL_R8A7792)
> 
> Actually it's r8a7792 (V2H) which has the RCAR_GYROADC_INTENR register,
> so the test is reversed.

Nice catch, fixed.

>> +               writel(0, priv->regs + RCAR_GYROADC_INTENR);
>> +
>> +       /* Set mode and timing. */
>> +       writel(priv->mode, priv->regs + RCAR_GYROADC_MODE_SELECT);
>> +
>> +       if (priv->mode == RCAR_GYROADC_MODE_SELECT_1_MB88101A)
>> +               writel(clk_mhz * 10, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
>> +       else if (priv->mode == RCAR_GYROADC_MODE_SELECT_2_ADCS7476)
>> +               writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
>> +       else if (priv->mode == RCAR_GYROADC_MODE_SELECT_3_MAX1162)
>> +               writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
> 
> switch(priv->mode)?

That's actually make the code longer.

>> +       writel(clk_mhz * 1250, priv->regs + RCAR_GYROADC_1_25MS_LENGTH);
>> +
>> +       /*
>> +        * We can possibly turn the sampling on/off on-demand to reduce power
>> +        * consumption, but for the sake of quick availability of samples, we
>> +        * don't do it now.
>> +        */
>> +       writel(RCAR_GYROADC_START_STOP_START,
>> +              priv->regs + RCAR_GYROADC_START_STOP);
>> +
>> +       /* Wait for the first conversion to complete. */
>> +       udelay(1250);
>> +}
>> +
>> +#define RCAR_GYROADC_CHAN(_idx) {                              \
>> +       .type                   = IIO_VOLTAGE,                  \
>> +       .indexed                = 1,                            \
>> +       .channel                = (_idx),                       \
>> +       .info_mask_separate     = BIT(IIO_CHAN_INFO_RAW) |      \
>> +                                 BIT(IIO_CHAN_INFO_SCALE),     \
>> +       .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>> +}
>> +
>> +static const struct iio_chan_spec rcar_gyroadc_iio_channels_1[] = {
>> +       RCAR_GYROADC_CHAN(0),
>> +       RCAR_GYROADC_CHAN(1),
>> +       RCAR_GYROADC_CHAN(2),
>> +       RCAR_GYROADC_CHAN(3),
>> +};
>> +
>> +static const struct iio_chan_spec rcar_gyroadc_iio_channels_2[] = {
>> +       RCAR_GYROADC_CHAN(0),
>> +       RCAR_GYROADC_CHAN(1),
>> +       RCAR_GYROADC_CHAN(2),
>> +       RCAR_GYROADC_CHAN(3),
>> +       RCAR_GYROADC_CHAN(4),
>> +       RCAR_GYROADC_CHAN(5),
>> +       RCAR_GYROADC_CHAN(6),
>> +       RCAR_GYROADC_CHAN(7),
>> +};
>> +
>> +/*
>> + * 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),
>> +       RCAR_GYROADC_CHAN(1),
>> +       RCAR_GYROADC_CHAN(2),
>> +       RCAR_GYROADC_CHAN(3),
>> +       RCAR_GYROADC_CHAN(4),
>> +       RCAR_GYROADC_CHAN(5),
>> +       RCAR_GYROADC_CHAN(6),
>> +       RCAR_GYROADC_CHAN(7),
>> +};
>> +
>> +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);
>> +       struct regulator *consumer = priv->vref[chan->channel];
>> +       unsigned int datareg = RCAR_GYROADC_REALTIME_DATA(chan->channel);
>> +       unsigned int vref;
>> +       int ret;
>> +
>> +       switch (mask) {
>> +       case IIO_CHAN_INFO_RAW:
>> +               if (chan->type != IIO_VOLTAGE)
>> +                       return -EINVAL;
>> +
>> +               /* Channel not connected. */
>> +               if (!consumer)
>> +                       return -EINVAL;
>> +
>> +               ret = iio_device_claim_direct_mode(indio_dev);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               *val = readl(priv->regs + datareg);
>> +               *val &= BIT(priv->sample_width) - 1;
>> +
>> +               iio_device_release_direct_mode(indio_dev);
>> +
>> +               return IIO_VAL_INT;
>> +       case IIO_CHAN_INFO_SCALE:
>> +               /* Channel not connected. */
>> +               if (!consumer)
>> +                       return -EINVAL;
>> +
>> +               vref = regulator_get_voltage(consumer);
>> +               *val = 0;
>> +               *val2 = (vref * 1000) / 0x10000;
> 
> DIV_ROUND_CLOSEST()?

Why exactly ?

>> +               return IIO_VAL_INT_PLUS_NANO;
>> +       case IIO_CHAN_INFO_SAMP_FREQ:
>> +               *val = RCAR_GYROADC_SAMPLE_RATE;
>> +               return IIO_VAL_INT;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +}
>> +
>> +static int rcar_gyroadc_reg_access(struct iio_dev *indio_dev,
>> +                                  unsigned int reg, unsigned int writeval,
>> +                                  unsigned int *readval)
>> +{
>> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
>> +       unsigned int maxreg = RCAR_GYROADC_INTENR;
>> +
>> +       if (readval == NULL)
>> +               return -EINVAL;
>> +
>> +       if (reg % 4)
>> +               return -EINVAL;
>> +
>> +       /* Handle the V2H case with missing interrupt block. */
>> +       if (priv->model == RCAR_GYROADC_MODEL_R8A7792)
> 
> Inverted check, r8a7792 has more registers.

Fixed

>> +               maxreg = RCAR_GYROADC_FIFO_STATUS;
>> +
>> +       if (reg > maxreg)
>> +               return -EINVAL;
>> +
>> +       *readval = readl(priv->regs + reg);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct iio_info rcar_gyroadc_iio_info = {
>> +       .driver_module          = THIS_MODULE,
>> +       .read_raw               = rcar_gyroadc_read_raw,
>> +       .debugfs_reg_access     = rcar_gyroadc_reg_access,
>> +};
>> +
>> +static const struct of_device_id rcar_gyroadc_match[] = {
>> +       {
>> +               /* R-Car Gen2 compatible GyroADC */
>> +               .compatible     = "renesas,r8a7791-gyroadc",
>> +               .data           = (void *)RCAR_GYROADC_MODEL_DEFAULT,
>> +       }, {
>> +               /* R-Car V2H specialty without interrupt registers. */
> 
> with interrupt registers.

Fixed

>> +               .compatible     = "renesas,r8a7792-gyroadc",
>> +               .data           = (void *)RCAR_GYROADC_MODEL_R8A7792,
>> +       }, {
>> +               /* sentinel */
>> +       }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, rcar_gyroadc_match);
>> +
>> +static int rcar_gyroadc_init_mode(struct iio_dev *indio_dev)
>> +{
>> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
>> +       struct device *dev = priv->dev;
>> +       int ret, mode;
>> +
>> +       ret = of_property_read_u32(dev->of_node, "renesas,gyroadc-mode", &mode);
>> +       if (ret) {
>> +               dev_err(dev, "Failed to get GyroADC mode (ret=%i)\n", ret);
>> +               return ret;
>> +       } else if (mode == 1) {
> 
> switch(mode)

Oh well, fixed :)

>> +               priv->num_channels = 4;
>> +               priv->mode = RCAR_GYROADC_MODE_SELECT_1_MB88101A;
>> +               priv->sample_width = 12;
>> +               indio_dev->channels = rcar_gyroadc_iio_channels_1;
>> +               indio_dev->num_channels =
>> +                       ARRAY_SIZE(rcar_gyroadc_iio_channels_1);
>> +       } else if (mode == 2) {
>> +               priv->num_channels = 8;
>> +               priv->mode = RCAR_GYROADC_MODE_SELECT_2_ADCS7476;
>> +               priv->sample_width = 15;
>> +               indio_dev->channels = rcar_gyroadc_iio_channels_2;
>> +               indio_dev->num_channels =
>> +                       ARRAY_SIZE(rcar_gyroadc_iio_channels_2);
>> +       } else if (mode == 3) {
>> +               priv->num_channels = 8;
>> +               priv->mode = RCAR_GYROADC_MODE_SELECT_3_MAX1162;
>> +               priv->sample_width = 16;
>> +               indio_dev->channels = rcar_gyroadc_iio_channels_3;
>> +               indio_dev->num_channels =
>> +                       ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
>> +       } else {
>> +               dev_err(dev, "Invalid GyroADC mode (mode=%i)\n", mode);
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void rcar_gyroadc_deinit_supplies(struct iio_dev *indio_dev)
>> +{
>> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
>> +       int i;
> 
> unsigned int

Fixed

>> +
>> +       for (i = 0; i < priv->num_channels; i++) {
>> +               if (!priv->vref[i])
>> +                       continue;
>> +
>> +               regulator_disable(priv->vref[i]);
>> +       }
>> +}
>> +
>> +static int rcar_gyroadc_init_supplies(struct iio_dev *indio_dev)
>> +{
>> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
>> +       struct device *dev = priv->dev;
>> +       struct regulator *vref;
>> +       char name[25];
>> +       int i, ret;
> 
> unsigned int

Only for the i, fixed.

>> +
>> +       for (i = 0; i < priv->num_channels; i++) {
>> +               snprintf(name, sizeof(name), "renesas,gyroadc-vref-ch%i", i);
>> +
>> +               vref = devm_regulator_get_optional(dev, name);
>> +               if (IS_ERR(vref)) {
>> +                       /*
>> +                        * Regulator is not present, which means the channel
>> +                        * supply is not connected.
>> +                        */
>> +                       dev_dbg(dev, "Regulator %s not connected\n", name);
>> +                       continue;
>> +               }
>> +
>> +               priv->vref[i] = vref;
>> +       }
>> +
>> +       for (i = 0; i < priv->num_channels; i++) {
>> +               if (!priv->vref[i])
>> +                       continue;
>> +
>> +               ret = regulator_enable(priv->vref[i]);
>> +               if (ret) {
>> +                       dev_err(dev, "Failed to enable regulator %i (ret=%i)\n",
>> +                               i, ret);
>> +                       goto err;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +
>> +err:
>> +       rcar_gyroadc_deinit_supplies(indio_dev);
>> +       return ret;
>> +}
>> +
>> +static int rcar_gyroadc_probe(struct platform_device *pdev)
>> +{
>> +       const struct of_device_id *of_id =
>> +               of_match_device(rcar_gyroadc_match, &pdev->dev);
>> +       struct device *dev = &pdev->dev;
>> +       struct rcar_gyroadc *priv;
>> +       struct iio_dev *indio_dev;
>> +       struct resource *mem;
>> +       int ret;
>> +
>> +       indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
>> +       if (!indio_dev) {
>> +               dev_err(dev, "Failed to allocate IIO device.\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       priv = iio_priv(indio_dev);
>> +       priv->dev = dev;
>> +
>> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       priv->regs = devm_ioremap_resource(dev, mem);
>> +       if (IS_ERR(priv->regs))
>> +               return PTR_ERR(priv->regs);
>> +
>> +       priv->iclk = devm_clk_get(dev, "if");
>> +       if (IS_ERR(priv->iclk)) {
>> +               ret = PTR_ERR(priv->iclk);
>> +               if (ret != -EPROBE_DEFER)
>> +                       dev_err(dev, "Failed to get IF clock (ret=%i)\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       ret = rcar_gyroadc_init_mode(indio_dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = rcar_gyroadc_init_supplies(indio_dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       priv->model = (enum rcar_gyroadc_model)of_id->data;
>> +
>> +       platform_set_drvdata(pdev, indio_dev);
>> +
>> +       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;
>> +
>> +       pm_runtime_enable(dev);
>> +       pm_runtime_get_sync(dev);
>> +
>> +       ret = clk_prepare_enable(priv->iclk);
>> +       if (ret) {
>> +               dev_err(dev, "Could not prepare or enable the IF clock.\n");
>> +               goto error_clk_if_enable;
>> +       }
>> +
>> +       rcar_gyroadc_hw_init(priv);
>> +
>> +       ret = iio_device_register(indio_dev);
>> +       if (ret) {
>> +               dev_err(dev, "Couldn't register IIO device.\n");
>> +               goto error_iio_device_register;
>> +       }
>> +
>> +       return 0;
>> +
>> +error_iio_device_register:
>> +       clk_disable_unprepare(priv->iclk);
>> +error_clk_if_enable:
>> +       pm_runtime_put(dev);
>> +       pm_runtime_disable(dev);
>> +       rcar_gyroadc_deinit_supplies(indio_dev);
>> +       return ret;
>> +}
>> +
>> +static int rcar_gyroadc_remove(struct platform_device *pdev)
>> +{
>> +       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +       struct rcar_gyroadc *priv = iio_priv(indio_dev);
>> +       struct device *dev = priv->dev;
>> +
>> +       /* Stop sampling */
>> +       writel(0, priv->regs + RCAR_GYROADC_START_STOP);
>> +
>> +       iio_device_unregister(indio_dev);
>> +       clk_disable_unprepare(priv->iclk);
>> +       pm_runtime_put(dev);
>> +       pm_runtime_disable(dev);
>> +       rcar_gyroadc_deinit_supplies(indio_dev);
>> +
>> +       return 0;
>> +}
>> +
>> +static struct platform_driver rcar_gyroadc_driver = {
>> +       .probe          = rcar_gyroadc_probe,
>> +       .remove         = rcar_gyroadc_remove,
>> +       .driver         = {
>> +               .name           = "rcar-gyroadc",
>> +               .of_match_table = rcar_gyroadc_match,
>> +       },
>> +};
>> +
>> +module_platform_driver(rcar_gyroadc_driver);
>> +
>> +MODULE_AUTHOR("Marek Vasut <marek.vasut@gmail.com>");
>> +MODULE_DESCRIPTION("Renesas RCAR GyroADC driver");
> 
> R-Car

Fixed

>> +MODULE_LICENSE("GPL");
>> --
>> 2.11.0
> 
> Gr{oetje,eeting}s,

Thanks!

-- 
Best regards,
Marek Vasut

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

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jan 9, 2017 at 3:16 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/09/2017 01:13 PM, Geert Uytterhoeven wrote:
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>>> @@ -0,0 +1,52 @@
>>> +* Renesas RCar GyroADC device driver
>>> +
>>> +Required properties:
>>> +- compatible:  Should be "renesas,r8a7791-gyroadc" for regular GyroADC or
>>> +               "renesas,r8a7792-gyroadc" for a GyroADC without interrupt
>>> +               block found in R8A7792.
>>
>> I would have kept "renesas,rcar-gyroadc", too.
>
> Do we have some sort of standard practice here ? Ie. NXP SoCs use the
> oldest compatible SoC in the DT compat string (so the driver can bind
> to that) and another DT compat string with that particular SoC name
> encoded in it (so it's possible to discern it in the future in the
> driver, if there is some problem).

On Renesas SoCs, we usually define family- and SoC-specific compatible
values, and match on the family-specific values if possible.

>> Upon closer look, GyroADC in r8a7792 aka R-Car V2H has builtin interrupt
>> functionality, (SPI IRQ 18), while other variants use the interrupt
>> functionality of the Speed-pulse I/F (SPI IRQ 236), which is not present
>> on V2H.
>>
>> Hence I think we can distinguish between the two variants by looking at
>> the presence of an "interrupts" property, if we make that mandatory on
>> V2H. If/when the need arises later, non-V2H variants will need a phandle
>> to the Speed-pulse I/F to use its interrupt.
>
> Well, the interrupt is pretty much useless (it generates 100ms pulses),
> all we want to do it set the register to 0x0 to make sure the block
> doesn't generate any. Do we want to add interrupts property for just
> this purpose ?

The alternative is to let the driver match against all of
"renesas,r8a77<n>-gyroadc" for n=78,79,90..96 (and more to come).

>> Then the driver can just match on "renesas,rcar-gyroadc", instead of any
>> other single version (all R-Car Gen1, 2, and 3 SoCs) in existence.

>>> +Optional properties:
>>> +- renesas,gyroadc-vref-ch0-supply: Phandle to channel 0 voltage reference regulator.
>>> +- renesas,gyroadc-vref-ch1-supply: Phandle to channel 1 voltage reference regulator.
>>> +- renesas,gyroadc-vref-ch2-supply: Phandle to channel 2 voltage reference regulator.
>>> +- renesas,gyroadc-vref-ch3-supply: Phandle to channel 3 voltage reference regulator.
>>> +- renesas,gyroadc-vref-ch4-supply: Phandle to channel 4 voltage reference regulator.
>>> +- renesas,gyroadc-vref-ch5-supply: Phandle to channel 5 voltage reference regulator.
>>> +- renesas,gyroadc-vref-ch6-supply: Phandle to channel 6 voltage reference regulator.
>>> +- renesas,gyroadc-vref-ch7-supply: Phandle to channel 7 voltage reference regulator.
>>
>> Why not an array of phandles? That would simplify parsing.
>
> Because if you connect ADC only to ie. channel 0 and 3 , specifying that
> in an array would be a hassle .

IIRC, you can have zeroes ("NULL pointers") in phandle arrays.

>> Also, "vref-supply" seems a fairly standard property name already in wide use.
>
> The above would thus make this vref-chX-supply ?

vref-supply = <&vref1 0 0 &vref3>;

>>> +
>>> +static void rcar_gyroadc_hw_init(struct rcar_gyroadc *priv)
>>> +{

>>> +       if (priv->mode == RCAR_GYROADC_MODE_SELECT_1_MB88101A)
>>> +               writel(clk_mhz * 10, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
>>> +       else if (priv->mode == RCAR_GYROADC_MODE_SELECT_2_ADCS7476)
>>> +               writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
>>> +       else if (priv->mode == RCAR_GYROADC_MODE_SELECT_3_MAX1162)
>>> +               writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
>>
>> switch(priv->mode)?
>
> That's actually make the code longer.

Oh, you looked at the compiler output :-)

Having a switch means you can combine the last two cases.

Actually, as mode is always a valid value here, you can just
have if (<mode1>) { ... } else { ...}.

>>> +static int rcar_gyroadc_read_raw(struct iio_dev *indio_dev,
>>> +                                struct iio_chan_spec const *chan,
>>> +                                int *val, int *val2, long mask)
>>> +{

>>> +               *val2 = (vref * 1000) / 0x10000;
>>
>> DIV_ROUND_CLOSEST()?
>
> Why exactly ?

Because the end result is closer to the actual value.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marek Vasut]

On 01/09/2017 03:32 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi!

> On Mon, Jan 9, 2017 at 3:16 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/09/2017 01:13 PM, Geert Uytterhoeven wrote:
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>>>> @@ -0,0 +1,52 @@
>>>> +* Renesas RCar GyroADC device driver
>>>> +
>>>> +Required properties:
>>>> +- compatible:  Should be "renesas,r8a7791-gyroadc" for regular GyroADC or
>>>> +               "renesas,r8a7792-gyroadc" for a GyroADC without interrupt
>>>> +               block found in R8A7792.
>>>
>>> I would have kept "renesas,rcar-gyroadc", too.
>>
>> Do we have some sort of standard practice here ? Ie. NXP SoCs use the
>> oldest compatible SoC in the DT compat string (so the driver can bind
>> to that) and another DT compat string with that particular SoC name
>> encoded in it (so it's possible to discern it in the future in the
>> driver, if there is some problem).
> 
> On Renesas SoCs, we usually define family- and SoC-specific compatible
> values, and match on the family-specific values if possible.
> 
>>> Upon closer look, GyroADC in r8a7792 aka R-Car V2H has builtin interrupt
>>> functionality, (SPI IRQ 18), while other variants use the interrupt
>>> functionality of the Speed-pulse I/F (SPI IRQ 236), which is not present
>>> on V2H.
>>>
>>> Hence I think we can distinguish between the two variants by looking at
>>> the presence of an "interrupts" property, if we make that mandatory on
>>> V2H. If/when the need arises later, non-V2H variants will need a phandle
>>> to the Speed-pulse I/F to use its interrupt.
>>
>> Well, the interrupt is pretty much useless (it generates 100ms pulses),
>> all we want to do it set the register to 0x0 to make sure the block
>> doesn't generate any. Do we want to add interrupts property for just
>> this purpose ?
> 
> The alternative is to let the driver match against all of
> "renesas,r8a77<n>-gyroadc" for n=78,79,90..96 (and more to come).

I think this would be slightly better, since I can precisely support
only the SoC which I have and which I can test. Then again, there's
already "renesas,rcar-gen2-jpu" for example, which looks appealing to
use, but I think it's not a good idea as there might be some bug found
in the hardware later on and the DT wouldn't allow us to discern that
block if it only has a "renesas,rcar-gyroadc" in it.

I think using
compatible = "renesas,r8a77<n>-gyroadc", "renesas,rcar-gyroadc";
would work.

The driver could match on "renesas,rcar-gyroadc" and if a bug is ever
found, the driver can match on "renesas,r8a77<n>-gyroadc"
instead and handle the bug.

>>> Then the driver can just match on "renesas,rcar-gyroadc", instead of any
>>> other single version (all R-Car Gen1, 2, and 3 SoCs) in existence.
> 
>>>> +Optional properties:
>>>> +- renesas,gyroadc-vref-ch0-supply: Phandle to channel 0 voltage reference regulator.
>>>> +- renesas,gyroadc-vref-ch1-supply: Phandle to channel 1 voltage reference regulator.
>>>> +- renesas,gyroadc-vref-ch2-supply: Phandle to channel 2 voltage reference regulator.
>>>> +- renesas,gyroadc-vref-ch3-supply: Phandle to channel 3 voltage reference regulator.
>>>> +- renesas,gyroadc-vref-ch4-supply: Phandle to channel 4 voltage reference regulator.
>>>> +- renesas,gyroadc-vref-ch5-supply: Phandle to channel 5 voltage reference regulator.
>>>> +- renesas,gyroadc-vref-ch6-supply: Phandle to channel 6 voltage reference regulator.
>>>> +- renesas,gyroadc-vref-ch7-supply: Phandle to channel 7 voltage reference regulator.
>>>
>>> Why not an array of phandles? That would simplify parsing.
>>
>> Because if you connect ADC only to ie. channel 0 and 3 , specifying that
>> in an array would be a hassle .
> 
> IIRC, you can have zeroes ("NULL pointers") in phandle arrays.
> 
>>> Also, "vref-supply" seems a fairly standard property name already in wide use.
>>
>> The above would thus make this vref-chX-supply ?
> 
> vref-supply = <&vref1 0 0 &vref3>;

Not sure how I feel about this one, also it's the first time I saw such
method of describing regulator set.

>>>> +
>>>> +static void rcar_gyroadc_hw_init(struct rcar_gyroadc *priv)
>>>> +{
> 
>>>> +       if (priv->mode == RCAR_GYROADC_MODE_SELECT_1_MB88101A)
>>>> +               writel(clk_mhz * 10, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
>>>> +       else if (priv->mode == RCAR_GYROADC_MODE_SELECT_2_ADCS7476)
>>>> +               writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
>>>> +       else if (priv->mode == RCAR_GYROADC_MODE_SELECT_3_MAX1162)
>>>> +               writel(clk_mhz * 5, priv->regs + RCAR_GYROADC_CLOCK_LENGTH);
>>>
>>> switch(priv->mode)?
>>
>> That's actually make the code longer.
> 
> Oh, you looked at the compiler output :-)
> 
> Having a switch means you can combine the last two cases.
> 
> Actually, as mode is always a valid value here, you can just
> have if (<mode1>) { ... } else { ...}.

In fact, you can do
clk_mul = (priv->mode == ...) ? 10 : 5;
...
writel(clk_mhz * clk_mul, ....);

Done.

>>>> +static int rcar_gyroadc_read_raw(struct iio_dev *indio_dev,
>>>> +                                struct iio_chan_spec const *chan,
>>>> +                                int *val, int *val2, long mask)
>>>> +{
> 
>>>> +               *val2 = (vref * 1000) / 0x10000;
>>>
>>> DIV_ROUND_CLOSEST()?
>>
>> Why exactly ?
> 
> Because the end result is closer to the actual value.

Fixed.

-- 
Best regards,
Marek Vasut

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

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jan 9, 2017 at 6:30 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On Mon, Jan 9, 2017 at 3:16 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 01/09/2017 01:13 PM, Geert Uytterhoeven wrote:
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>>>>> @@ -0,0 +1,52 @@
>>>>> +* Renesas RCar GyroADC device driver
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible:  Should be "renesas,r8a7791-gyroadc" for regular GyroADC or
>>>>> +               "renesas,r8a7792-gyroadc" for a GyroADC without interrupt
>>>>> +               block found in R8A7792.
>>>>
>>>> I would have kept "renesas,rcar-gyroadc", too.
>>>
>>> Do we have some sort of standard practice here ? Ie. NXP SoCs use the
>>> oldest compatible SoC in the DT compat string (so the driver can bind
>>> to that) and another DT compat string with that particular SoC name
>>> encoded in it (so it's possible to discern it in the future in the
>>> driver, if there is some problem).
>>
>> On Renesas SoCs, we usually define family- and SoC-specific compatible
>> values, and match on the family-specific values if possible.
>>
>>>> Upon closer look, GyroADC in r8a7792 aka R-Car V2H has builtin interrupt
>>>> functionality, (SPI IRQ 18), while other variants use the interrupt
>>>> functionality of the Speed-pulse I/F (SPI IRQ 236), which is not present
>>>> on V2H.
>>>>
>>>> Hence I think we can distinguish between the two variants by looking at
>>>> the presence of an "interrupts" property, if we make that mandatory on
>>>> V2H. If/when the need arises later, non-V2H variants will need a phandle
>>>> to the Speed-pulse I/F to use its interrupt.
>>>
>>> Well, the interrupt is pretty much useless (it generates 100ms pulses),
>>> all we want to do it set the register to 0x0 to make sure the block
>>> doesn't generate any. Do we want to add interrupts property for just
>>> this purpose ?
>>
>> The alternative is to let the driver match against all of
>> "renesas,r8a77<n>-gyroadc" for n=78,79,90..96 (and more to come).
>
> I think this would be slightly better, since I can precisely support
> only the SoC which I have and which I can test. Then again, there's
> already "renesas,rcar-gen2-jpu" for example, which looks appealing to
> use, but I think it's not a good idea as there might be some bug found

The JPU bindings also mandate specifying e.g. "renesas,jpu-r8a7791".

> in the hardware later on and the DT wouldn't allow us to discern that
> block if it only has a "renesas,rcar-gyroadc" in it.
>
> I think using
> compatible = "renesas,r8a77<n>-gyroadc", "renesas,rcar-gyroadc";
> would work.

Yes, that's exactly what I meant.

> The driver could match on "renesas,rcar-gyroadc" and if a bug is ever
> found, the driver can match on "renesas,r8a77<n>-gyroadc"
> instead and handle the bug.

The driver still needs to know if it's running on V2H or not, because of
the interrupt registers.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marek Vasut]

On 01/09/2017 07:30 PM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Mon, Jan 9, 2017 at 6:30 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On Mon, Jan 9, 2017 at 3:16 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 01/09/2017 01:13 PM, Geert Uytterhoeven wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>>>>>> @@ -0,0 +1,52 @@
>>>>>> +* Renesas RCar GyroADC device driver
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible:  Should be "renesas,r8a7791-gyroadc" for regular GyroADC or
>>>>>> +               "renesas,r8a7792-gyroadc" for a GyroADC without interrupt
>>>>>> +               block found in R8A7792.
>>>>>
>>>>> I would have kept "renesas,rcar-gyroadc", too.
>>>>
>>>> Do we have some sort of standard practice here ? Ie. NXP SoCs use the
>>>> oldest compatible SoC in the DT compat string (so the driver can bind
>>>> to that) and another DT compat string with that particular SoC name
>>>> encoded in it (so it's possible to discern it in the future in the
>>>> driver, if there is some problem).
>>>
>>> On Renesas SoCs, we usually define family- and SoC-specific compatible
>>> values, and match on the family-specific values if possible.
>>>
>>>>> Upon closer look, GyroADC in r8a7792 aka R-Car V2H has builtin interrupt
>>>>> functionality, (SPI IRQ 18), while other variants use the interrupt
>>>>> functionality of the Speed-pulse I/F (SPI IRQ 236), which is not present
>>>>> on V2H.
>>>>>
>>>>> Hence I think we can distinguish between the two variants by looking at
>>>>> the presence of an "interrupts" property, if we make that mandatory on
>>>>> V2H. If/when the need arises later, non-V2H variants will need a phandle
>>>>> to the Speed-pulse I/F to use its interrupt.
>>>>
>>>> Well, the interrupt is pretty much useless (it generates 100ms pulses),
>>>> all we want to do it set the register to 0x0 to make sure the block
>>>> doesn't generate any. Do we want to add interrupts property for just
>>>> this purpose ?
>>>
>>> The alternative is to let the driver match against all of
>>> "renesas,r8a77<n>-gyroadc" for n=78,79,90..96 (and more to come).
>>
>> I think this would be slightly better, since I can precisely support
>> only the SoC which I have and which I can test. Then again, there's
>> already "renesas,rcar-gen2-jpu" for example, which looks appealing to
>> use, but I think it's not a good idea as there might be some bug found
> 
> The JPU bindings also mandate specifying e.g. "renesas,jpu-r8a7791".
> 
>> in the hardware later on and the DT wouldn't allow us to discern that
>> block if it only has a "renesas,rcar-gyroadc" in it.
>>
>> I think using
>> compatible = "renesas,r8a77<n>-gyroadc", "renesas,rcar-gyroadc";
>> would work.
> 
> Yes, that's exactly what I meant.

Great :-)

>> The driver could match on "renesas,rcar-gyroadc" and if a bug is ever
>> found, the driver can match on "renesas,r8a77<n>-gyroadc"
>> instead and handle the bug.
> 
> The driver still needs to know if it's running on V2H or not, because of
> the interrupt registers.

Exactly, which is why "renesas,r8a7792-gyroadc" compatible is handled
and treated specially.

-- 
Best regards,
Marek Vasut

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

[Lars-Peter Clausen]

On 01/09/2017 01:03 AM, Marek Vasut wrote:
[...]
> +- 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

So is this a ADC, or is this just a specialized SPI controller that
interfaces to an external ADC?

- Lars

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

[Marek Vasut]

On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
> On 01/09/2017 01:03 AM, Marek Vasut wrote:
> [...]
>> +- 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
> 
> So is this a ADC, or is this just a specialized SPI controller that
> interfaces to an external ADC?

It's a special SPI controller, except one cannot access the SPI bus
directly. It sends out clock and reads in the data from the ADC .

-- 
Best regards,
Marek Vasut

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

[Lars-Peter Clausen]

On 01/09/2017 02:47 PM, Marek Vasut wrote:
> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>> [...]
>>> +- 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
>>
>> So is this a ADC, or is this just a specialized SPI controller that
>> interfaces to an external ADC?
> 
> It's a special SPI controller, except one cannot access the SPI bus
> directly. It sends out clock and reads in the data from the ADC .

OK, thanks for the clarification. The commit message does not mention this
at the moment and makes it sound like this is a built-in ADC.

Also the renesas,gyroadc-mode property is more of a driver configuration
setting rather than a description of the hardware, at least it is not very
DT-ish.

I'd go with something like


	&adc {
		compatible = "renesas,r8a7791-gyroadc";
		...

		adc@0 {
			reg = <0>;
			compatible = "maxim,max1162";

			ref-supply = <&vref_max1162>;
		};
		
		... repeat for each front-end ADC.

	};

That's much more idiomatic which makes it e.g. easier to understand for
someone who does not know the specifics of the binding.

It also makes it more extensible and portable in terms of the same binding
for similar devices.

Internally in the driver you can still use your mode variable initializing
it based on the compatible string of the subnode.

- Lars

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

[Geert Uytterhoeven]

Hi Lars,

On Mon, Jan 9, 2017 at 9:07 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>> [...]
>>>> +- 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
>>>
>>> So is this a ADC, or is this just a specialized SPI controller that
>>> interfaces to an external ADC?
>>
>> It's a special SPI controller, except one cannot access the SPI bus
>> directly. It sends out clock and reads in the data from the ADC .
>
> OK, thanks for the clarification. The commit message does not mention this
> at the moment and makes it sound like this is a built-in ADC.
>
> Also the renesas,gyroadc-mode property is more of a driver configuration
> setting rather than a description of the hardware, at least it is not very
> DT-ish.
>
> I'd go with something like
>
>
>         &adc {
>                 compatible = "renesas,r8a7791-gyroadc";
>                 ...
>
>                 adc@0 {
>                         reg = <0>;
>                         compatible = "maxim,max1162";
>
>                         ref-supply = <&vref_max1162>;

vref-supply?

>                 };
>
>                 ... repeat for each front-end ADC.
>
>         };
>
> That's much more idiomatic which makes it e.g. easier to understand for
> someone who does not know the specifics of the binding.
>
> It also makes it more extensible and portable in terms of the same binding
> for similar devices.
>
> Internally in the driver you can still use your mode variable initializing
> it based on the compatible string of the subnode.

Thanks, that sounds like a great idea!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Lars-Peter Clausen]

On 01/09/2017 09:25 PM, Geert Uytterhoeven wrote:
> Hi Lars,
> 
> On Mon, Jan 9, 2017 at 9:07 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>> [...]
>>>>> +- 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
>>>>
>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>> interfaces to an external ADC?
>>>
>>> It's a special SPI controller, except one cannot access the SPI bus
>>> directly. It sends out clock and reads in the data from the ADC .
>>
>> OK, thanks for the clarification. The commit message does not mention this
>> at the moment and makes it sound like this is a built-in ADC.
>>
>> Also the renesas,gyroadc-mode property is more of a driver configuration
>> setting rather than a description of the hardware, at least it is not very
>> DT-ish.
>>
>> I'd go with something like
>>
>>
>>         &adc {
>>                 compatible = "renesas,r8a7791-gyroadc";
>>                 ...
>>
>>                 adc@0 {
>>                         reg = <0>;
>>                         compatible = "maxim,max1162";
>>
>>                         ref-supply = <&vref_max1162>;
> 
> vref-supply?

Well, the pin is called just ref in the datasheet.

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

[Geert Uytterhoeven]

Hi Lars,

On Mon, Jan 9, 2017 at 9:27 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 01/09/2017 09:25 PM, Geert Uytterhoeven wrote:
>> On Mon, Jan 9, 2017 at 9:07 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>> [...]
>>>>>> +- 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
>>>>>
>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>> interfaces to an external ADC?
>>>>
>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>> directly. It sends out clock and reads in the data from the ADC .
>>>
>>> OK, thanks for the clarification. The commit message does not mention this
>>> at the moment and makes it sound like this is a built-in ADC.
>>>
>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>> setting rather than a description of the hardware, at least it is not very
>>> DT-ish.
>>>
>>> I'd go with something like
>>>
>>>
>>>         &adc {
>>>                 compatible = "renesas,r8a7791-gyroadc";
>>>                 ...
>>>
>>>                 adc@0 {
>>>                         reg = <0>;
>>>                         compatible = "maxim,max1162";
>>>
>>>                         ref-supply = <&vref_max1162>;
>>
>> vref-supply?
>
> Well, the pin is called just ref in the datasheet.

But the signal on the pin is called Vref.

There are also no pre-existing users of a "ref-supply" property, there
are plenty of "vref-supply'.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Lars-Peter Clausen]

On 01/09/2017 09:41 PM, Geert Uytterhoeven wrote:
> Hi Lars,
> 
> On Mon, Jan 9, 2017 at 9:27 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 01/09/2017 09:25 PM, Geert Uytterhoeven wrote:
>>> On Mon, Jan 9, 2017 at 9:07 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>>> [...]
>>>>>>> +- 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
>>>>>>
>>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>>> interfaces to an external ADC?
>>>>>
>>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>>> directly. It sends out clock and reads in the data from the ADC .
>>>>
>>>> OK, thanks for the clarification. The commit message does not mention this
>>>> at the moment and makes it sound like this is a built-in ADC.
>>>>
>>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>>> setting rather than a description of the hardware, at least it is not very
>>>> DT-ish.
>>>>
>>>> I'd go with something like
>>>>
>>>>
>>>>         &adc {
>>>>                 compatible = "renesas,r8a7791-gyroadc";
>>>>                 ...
>>>>
>>>>                 adc@0 {
>>>>                         reg = <0>;
>>>>                         compatible = "maxim,max1162";
>>>>
>>>>                         ref-supply = <&vref_max1162>;
>>>
>>> vref-supply?
>>
>> Well, the pin is called just ref in the datasheet.
> 
> But the signal on the pin is called Vref.
> 
> There are also no pre-existing users of a "ref-supply" property, there
> are plenty of "vref-supply'.

It does not really matter, I only had a quick glance at the datasheet and
went with what I saw.

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

[Marek Vasut]

On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>> [...]
>>>> +- 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
>>>
>>> So is this a ADC, or is this just a specialized SPI controller that
>>> interfaces to an external ADC?
>>
>> It's a special SPI controller, except one cannot access the SPI bus
>> directly. It sends out clock and reads in the data from the ADC .
> 
> OK, thanks for the clarification. The commit message does not mention this
> at the moment and makes it sound like this is a built-in ADC.
> 
> Also the renesas,gyroadc-mode property is more of a driver configuration
> setting rather than a description of the hardware, at least it is not very
> DT-ish.
> 
> I'd go with something like
> 
> 
> 	&adc {
> 		compatible = "renesas,r8a7791-gyroadc";
> 		...
> 
> 		adc@0 {
> 			reg = <0>;
> 			compatible = "maxim,max1162";

But then the max1162 ADC driver* will try to bind to this, right ?
And since the MAX1162 is an SPI device, it will fail to work as the
ADC driver does not provide any sort of SPI interface. Or is that
actually OK ?

* I dunno if that made it mainline yet, but there's a driver for
  compatible ADC, https://patchwork.kernel.org/patch/9472237/

> 			ref-supply = <&vref_max1162>;
> 		};
> 		
> 		... repeat for each front-end ADC.
> 
> 	};
> 
> That's much more idiomatic which makes it e.g. easier to understand for
> someone who does not know the specifics of the binding.
> 
> It also makes it more extensible and portable in terms of the same binding
> for similar devices.
> 
> Internally in the driver you can still use your mode variable initializing
> it based on the compatible string of the subnode.
> 
> - Lars
> 


-- 
Best regards,
Marek Vasut

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

[Lars-Peter Clausen]

On 01/09/2017 09:56 PM, Marek Vasut wrote:
> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>> [...]
>>>>> +- 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
>>>>
>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>> interfaces to an external ADC?
>>>
>>> It's a special SPI controller, except one cannot access the SPI bus
>>> directly. It sends out clock and reads in the data from the ADC .
>>
>> OK, thanks for the clarification. The commit message does not mention this
>> at the moment and makes it sound like this is a built-in ADC.
>>
>> Also the renesas,gyroadc-mode property is more of a driver configuration
>> setting rather than a description of the hardware, at least it is not very
>> DT-ish.
>>
>> I'd go with something like
>>
>>
>> 	&adc {
>> 		compatible = "renesas,r8a7791-gyroadc";
>> 		...
>>
>> 		adc@0 {
>> 			reg = <0>;
>> 			compatible = "maxim,max1162";
> 
> But then the max1162 ADC driver* will try to bind to this, right ?
> And since the MAX1162 is an SPI device, it will fail to work as the
> ADC driver does not provide any sort of SPI interface. Or is that
> actually OK ?

Only if you call of_register_spi_devices().

We for example have devices that can either work in SPI or I2C mode and both
use the same compatible string. Depending on whether the device is a subnode
of a SPI or I2S controller it is registered as a SPI or I2C device.

This situation is not that different.

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

[Lars-Peter Clausen]

On 01/09/2017 10:02 PM, Lars-Peter Clausen wrote:
> On 01/09/2017 09:56 PM, Marek Vasut wrote:
>> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>> [...]
>>>>>> +- 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
>>>>>
>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>> interfaces to an external ADC?
>>>>
>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>> directly. It sends out clock and reads in the data from the ADC .
>>>
>>> OK, thanks for the clarification. The commit message does not mention this
>>> at the moment and makes it sound like this is a built-in ADC.
>>>
>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>> setting rather than a description of the hardware, at least it is not very
>>> DT-ish.
>>>
>>> I'd go with something like
>>>
>>>
>>> 	&adc {
>>> 		compatible = "renesas,r8a7791-gyroadc";
>>> 		...
>>>
>>> 		adc@0 {
>>> 			reg = <0>;
>>> 			compatible = "maxim,max1162";
>>
>> But then the max1162 ADC driver* will try to bind to this, right ?
>> And since the MAX1162 is an SPI device, it will fail to work as the
>> ADC driver does not provide any sort of SPI interface. Or is that
>> actually OK ?
> 
> Only if you call of_register_spi_devices().
> 
> We for example have devices that can either work in SPI or I2C mode and both
> use the same compatible string. Depending on whether the device is a subnode
> of a SPI or I2S controller it is registered as a SPI or I2C device.

I2C controller

> 
> This situation is not that different.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Marek Vasut]

On 01/09/2017 10:02 PM, Lars-Peter Clausen wrote:
> On 01/09/2017 09:56 PM, Marek Vasut wrote:
>> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>> [...]
>>>>>> +- 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
>>>>>
>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>> interfaces to an external ADC?
>>>>
>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>> directly. It sends out clock and reads in the data from the ADC .
>>>
>>> OK, thanks for the clarification. The commit message does not mention this
>>> at the moment and makes it sound like this is a built-in ADC.
>>>
>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>> setting rather than a description of the hardware, at least it is not very
>>> DT-ish.
>>>
>>> I'd go with something like
>>>
>>>
>>> 	&adc {
>>> 		compatible = "renesas,r8a7791-gyroadc";
>>> 		...
>>>
>>> 		adc@0 {
>>> 			reg = <0>;
>>> 			compatible = "maxim,max1162";
>>
>> But then the max1162 ADC driver* will try to bind to this, right ?
>> And since the MAX1162 is an SPI device, it will fail to work as the
>> ADC driver does not provide any sort of SPI interface. Or is that
>> actually OK ?
> 
> Only if you call of_register_spi_devices().
> 
> We for example have devices that can either work in SPI or I2C mode and both
> use the same compatible string. Depending on whether the device is a subnode
> of a SPI or I2S controller it is registered as a SPI or I2C device.
> 
> This situation is not that different.

So in this case, the gyroadc driver would instead iterate over the
subnodes, check the compatible string and configure the channel mode
according to the subnode compatible string?

But then, the gyroadc only has one configuration register which applies
to all channels, which selects how many bits to read from the connected
SPI-ADC device(s) (sample width). This is not per-channel configurable.
The driver would then have to check if all the compatible properties in
the ADC subnodes select the same ADC or at least ADC which has the same
sample width. This looks like a duplication to me. Thoughts ?

-- 
Best regards,
Marek Vasut

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

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jan 9, 2017 at 10:55 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/09/2017 10:02 PM, Lars-Peter Clausen wrote:
>> On 01/09/2017 09:56 PM, Marek Vasut wrote:
>>> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>>> [...]
>>>>>>> +- 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
>>>>>>
>>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>>> interfaces to an external ADC?
>>>>>
>>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>>> directly. It sends out clock and reads in the data from the ADC .
>>>>
>>>> OK, thanks for the clarification. The commit message does not mention this
>>>> at the moment and makes it sound like this is a built-in ADC.
>>>>
>>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>>> setting rather than a description of the hardware, at least it is not very
>>>> DT-ish.
>>>>
>>>> I'd go with something like
>>>>
>>>>
>>>>     &adc {
>>>>             compatible = "renesas,r8a7791-gyroadc";
>>>>             ...
>>>>
>>>>             adc@0 {
>>>>                     reg = <0>;
>>>>                     compatible = "maxim,max1162";
>>>
>>> But then the max1162 ADC driver* will try to bind to this, right ?
>>> And since the MAX1162 is an SPI device, it will fail to work as the
>>> ADC driver does not provide any sort of SPI interface. Or is that
>>> actually OK ?
>>
>> Only if you call of_register_spi_devices().
>>
>> We for example have devices that can either work in SPI or I2C mode and both
>> use the same compatible string. Depending on whether the device is a subnode
>> of a SPI or I2S controller it is registered as a SPI or I2C device.
>>
>> This situation is not that different.
>
> So in this case, the gyroadc driver would instead iterate over the
> subnodes, check the compatible string and configure the channel mode
> according to the subnode compatible string?

That's correct.

> But then, the gyroadc only has one configuration register which applies
> to all channels, which selects how many bits to read from the connected
> SPI-ADC device(s) (sample width). This is not per-channel configurable.
> The driver would then have to check if all the compatible properties in
> the ADC subnodes select the same ADC or at least ADC which has the same

Correct again.

> sample width. This looks like a duplication to me. Thoughts ?

While the protocols have to be the same for each channel, you can connect
different devices. If you have e.g. a thermometer or barimeter that speaks
the same protocol as a max1162 (i.e. it responds with 2 zero bytes and 2
bytes of data), you can mix and match that with max1162 devices, once
you have added support for the thermometer or barimeter to your driver.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marek Vasut]

On 01/09/2017 11:14 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi!

> On Mon, Jan 9, 2017 at 10:55 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/09/2017 10:02 PM, Lars-Peter Clausen wrote:
>>> On 01/09/2017 09:56 PM, Marek Vasut wrote:
>>>> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>>>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>>>> [...]
>>>>>>>> +- 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
>>>>>>>
>>>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>>>> interfaces to an external ADC?
>>>>>>
>>>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>>>> directly. It sends out clock and reads in the data from the ADC .
>>>>>
>>>>> OK, thanks for the clarification. The commit message does not mention this
>>>>> at the moment and makes it sound like this is a built-in ADC.
>>>>>
>>>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>>>> setting rather than a description of the hardware, at least it is not very
>>>>> DT-ish.
>>>>>
>>>>> I'd go with something like
>>>>>
>>>>>
>>>>>     &adc {
>>>>>             compatible = "renesas,r8a7791-gyroadc";
>>>>>             ...
>>>>>
>>>>>             adc@0 {
>>>>>                     reg = <0>;
>>>>>                     compatible = "maxim,max1162";
>>>>
>>>> But then the max1162 ADC driver* will try to bind to this, right ?
>>>> And since the MAX1162 is an SPI device, it will fail to work as the
>>>> ADC driver does not provide any sort of SPI interface. Or is that
>>>> actually OK ?
>>>
>>> Only if you call of_register_spi_devices().
>>>
>>> We for example have devices that can either work in SPI or I2C mode and both
>>> use the same compatible string. Depending on whether the device is a subnode
>>> of a SPI or I2S controller it is registered as a SPI or I2C device.
>>>
>>> This situation is not that different.
>>
>> So in this case, the gyroadc driver would instead iterate over the
>> subnodes, check the compatible string and configure the channel mode
>> according to the subnode compatible string?
> 
> That's correct.
> 
>> But then, the gyroadc only has one configuration register which applies
>> to all channels, which selects how many bits to read from the connected
>> SPI-ADC device(s) (sample width). This is not per-channel configurable.
>> The driver would then have to check if all the compatible properties in
>> the ADC subnodes select the same ADC or at least ADC which has the same
> 
> Correct again.
> 
>> sample width. This looks like a duplication to me. Thoughts ?
> 
> While the protocols have to be the same for each channel, you can connect
> different devices. If you have e.g. a thermometer or barimeter that speaks
> the same protocol as a max1162 (i.e. it responds with 2 zero bytes and 2
> bytes of data), you can mix and match that with max1162 devices, once
> you have added support for the thermometer or barimeter to your driver.

Which driver, the gyroadc driver ? That's only an ADC driver, it
shouldn't be aware of any temperature/pressure sampling or any such
stuff IMO. What you suggest would result in overbloating the gyroadc
driver with support for all sorts of ADCs connected to it's channels
and providing all sorts of information, which I think would violate
the functional separation in the driver model.

-- 
Best regards,
Marek Vasut

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

[Geert Uytterhoeven]

Hi Marek,

On Tue, Jan 10, 2017 at 7:35 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On Mon, Jan 9, 2017 at 10:55 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 01/09/2017 10:02 PM, Lars-Peter Clausen wrote:
>>>> On 01/09/2017 09:56 PM, Marek Vasut wrote:
>>>>> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>>>>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>>>>> [...]
>>>>>>>>> +- 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
>>>>>>>>
>>>>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>>>>> interfaces to an external ADC?
>>>>>>>
>>>>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>>>>> directly. It sends out clock and reads in the data from the ADC .
>>>>>>
>>>>>> OK, thanks for the clarification. The commit message does not mention this
>>>>>> at the moment and makes it sound like this is a built-in ADC.
>>>>>>
>>>>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>>>>> setting rather than a description of the hardware, at least it is not very
>>>>>> DT-ish.
>>>>>>
>>>>>> I'd go with something like
>>>>>>
>>>>>>
>>>>>>     &adc {
>>>>>>             compatible = "renesas,r8a7791-gyroadc";
>>>>>>             ...
>>>>>>
>>>>>>             adc@0 {
>>>>>>                     reg = <0>;
>>>>>>                     compatible = "maxim,max1162";
>>>>>
>>>>> But then the max1162 ADC driver* will try to bind to this, right ?
>>>>> And since the MAX1162 is an SPI device, it will fail to work as the
>>>>> ADC driver does not provide any sort of SPI interface. Or is that
>>>>> actually OK ?
>>>>
>>>> Only if you call of_register_spi_devices().
>>>>
>>>> We for example have devices that can either work in SPI or I2C mode and both
>>>> use the same compatible string. Depending on whether the device is a subnode
>>>> of a SPI or I2S controller it is registered as a SPI or I2C device.
>>>>
>>>> This situation is not that different.
>>>
>>> So in this case, the gyroadc driver would instead iterate over the
>>> subnodes, check the compatible string and configure the channel mode
>>> according to the subnode compatible string?
>>
>> That's correct.
>>
>>> But then, the gyroadc only has one configuration register which applies
>>> to all channels, which selects how many bits to read from the connected
>>> SPI-ADC device(s) (sample width). This is not per-channel configurable.
>>> The driver would then have to check if all the compatible properties in
>>> the ADC subnodes select the same ADC or at least ADC which has the same
>>
>> Correct again.
>>
>>> sample width. This looks like a duplication to me. Thoughts ?
>>
>> While the protocols have to be the same for each channel, you can connect
>> different devices. If you have e.g. a thermometer or barimeter that speaks
>> the same protocol as a max1162 (i.e. it responds with 2 zero bytes and 2
>> bytes of data), you can mix and match that with max1162 devices, once
>> you have added support for the thermometer or barimeter to your driver.
>
> Which driver, the gyroadc driver ? That's only an ADC driver, it

The gyroadc driver.

> shouldn't be aware of any temperature/pressure sampling or any such
> stuff IMO. What you suggest would result in overbloating the gyroadc
> driver with support for all sorts of ADCs connected to it's channels
> and providing all sorts of information, which I think would violate
> the functional separation in the driver model.

If the need ever arises, it can be turned into a "gyroadc" framework,
and the actual sensor drivers can be enhanced to support the gyroadc
driver, in addition to regular SPI master drivers?

Something for the far future, of course ;-)
For now we can limit it to the 3 types of devices officially supported by
the GyroADC hardware.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marek Vasut]

On 01/10/2017 07:41 PM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Tue, Jan 10, 2017 at 7:35 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On Mon, Jan 9, 2017 at 10:55 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 01/09/2017 10:02 PM, Lars-Peter Clausen wrote:
>>>>> On 01/09/2017 09:56 PM, Marek Vasut wrote:
>>>>>> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>>>>>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>>>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>>>>>> [...]
>>>>>>>>>> +- 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
>>>>>>>>>
>>>>>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>>>>>> interfaces to an external ADC?
>>>>>>>>
>>>>>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>>>>>> directly. It sends out clock and reads in the data from the ADC .
>>>>>>>
>>>>>>> OK, thanks for the clarification. The commit message does not mention this
>>>>>>> at the moment and makes it sound like this is a built-in ADC.
>>>>>>>
>>>>>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>>>>>> setting rather than a description of the hardware, at least it is not very
>>>>>>> DT-ish.
>>>>>>>
>>>>>>> I'd go with something like
>>>>>>>
>>>>>>>
>>>>>>>     &adc {
>>>>>>>             compatible = "renesas,r8a7791-gyroadc";
>>>>>>>             ...
>>>>>>>
>>>>>>>             adc@0 {
>>>>>>>                     reg = <0>;
>>>>>>>                     compatible = "maxim,max1162";
>>>>>>
>>>>>> But then the max1162 ADC driver* will try to bind to this, right ?
>>>>>> And since the MAX1162 is an SPI device, it will fail to work as the
>>>>>> ADC driver does not provide any sort of SPI interface. Or is that
>>>>>> actually OK ?
>>>>>
>>>>> Only if you call of_register_spi_devices().
>>>>>
>>>>> We for example have devices that can either work in SPI or I2C mode and both
>>>>> use the same compatible string. Depending on whether the device is a subnode
>>>>> of a SPI or I2S controller it is registered as a SPI or I2C device.
>>>>>
>>>>> This situation is not that different.
>>>>
>>>> So in this case, the gyroadc driver would instead iterate over the
>>>> subnodes, check the compatible string and configure the channel mode
>>>> according to the subnode compatible string?
>>>
>>> That's correct.
>>>
>>>> But then, the gyroadc only has one configuration register which applies
>>>> to all channels, which selects how many bits to read from the connected
>>>> SPI-ADC device(s) (sample width). This is not per-channel configurable.
>>>> The driver would then have to check if all the compatible properties in
>>>> the ADC subnodes select the same ADC or at least ADC which has the same
>>>
>>> Correct again.
>>>
>>>> sample width. This looks like a duplication to me. Thoughts ?
>>>
>>> While the protocols have to be the same for each channel, you can connect
>>> different devices. If you have e.g. a thermometer or barimeter that speaks
>>> the same protocol as a max1162 (i.e. it responds with 2 zero bytes and 2
>>> bytes of data), you can mix and match that with max1162 devices, once
>>> you have added support for the thermometer or barimeter to your driver.
>>
>> Which driver, the gyroadc driver ? That's only an ADC driver, it
> 
> The gyroadc driver.
> 
>> shouldn't be aware of any temperature/pressure sampling or any such
>> stuff IMO. What you suggest would result in overbloating the gyroadc
>> driver with support for all sorts of ADCs connected to it's channels
>> and providing all sorts of information, which I think would violate
>> the functional separation in the driver model.
> 
> If the need ever arises, it can be turned into a "gyroadc" framework,
> and the actual sensor drivers can be enhanced to support the gyroadc
> driver, in addition to regular SPI master drivers?

So basically we'd have a gyroadc bus type etc ?

> Something for the far future, of course ;-)
> For now we can limit it to the 3 types of devices officially supported by
> the GyroADC hardware.

I'm fine with this, but this doesn't answer my question about
duplication. The gyroadc driver would have to check whether all
channels are configured to the same device type then ? Doesn't
seem quite right.

-- 
Best regards,
Marek Vasut

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

[Geert Uytterhoeven]

Hi Marek,

On Tue, Jan 10, 2017 at 7:52 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/10/2017 07:41 PM, Geert Uytterhoeven wrote:
>> On Tue, Jan 10, 2017 at 7:35 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On Mon, Jan 9, 2017 at 10:55 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>> On 01/09/2017 10:02 PM, Lars-Peter Clausen wrote:
>>>>>> On 01/09/2017 09:56 PM, Marek Vasut wrote:
>>>>>>> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>>>>>>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>>>>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>>>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>>>>>>> [...]
>>>>>>>>>>> +- 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
>>>>>>>>>>
>>>>>>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>>>>>>> interfaces to an external ADC?
>>>>>>>>>
>>>>>>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>>>>>>> directly. It sends out clock and reads in the data from the ADC .
>>>>>>>>
>>>>>>>> OK, thanks for the clarification. The commit message does not mention this
>>>>>>>> at the moment and makes it sound like this is a built-in ADC.
>>>>>>>>
>>>>>>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>>>>>>> setting rather than a description of the hardware, at least it is not very
>>>>>>>> DT-ish.
>>>>>>>>
>>>>>>>> I'd go with something like
>>>>>>>>
>>>>>>>>
>>>>>>>>     &adc {
>>>>>>>>             compatible = "renesas,r8a7791-gyroadc";
>>>>>>>>             ...
>>>>>>>>
>>>>>>>>             adc@0 {
>>>>>>>>                     reg = <0>;
>>>>>>>>                     compatible = "maxim,max1162";
>>>>>>>
>>>>>>> But then the max1162 ADC driver* will try to bind to this, right ?
>>>>>>> And since the MAX1162 is an SPI device, it will fail to work as the
>>>>>>> ADC driver does not provide any sort of SPI interface. Or is that
>>>>>>> actually OK ?
>>>>>>
>>>>>> Only if you call of_register_spi_devices().
>>>>>>
>>>>>> We for example have devices that can either work in SPI or I2C mode and both
>>>>>> use the same compatible string. Depending on whether the device is a subnode
>>>>>> of a SPI or I2S controller it is registered as a SPI or I2C device.
>>>>>>
>>>>>> This situation is not that different.
>>>>>
>>>>> So in this case, the gyroadc driver would instead iterate over the
>>>>> subnodes, check the compatible string and configure the channel mode
>>>>> according to the subnode compatible string?
>>>>
>>>> That's correct.
>>>>
>>>>> But then, the gyroadc only has one configuration register which applies
>>>>> to all channels, which selects how many bits to read from the connected
>>>>> SPI-ADC device(s) (sample width). This is not per-channel configurable.
>>>>> The driver would then have to check if all the compatible properties in
>>>>> the ADC subnodes select the same ADC or at least ADC which has the same
>>>>
>>>> Correct again.
>>>>
>>>>> sample width. This looks like a duplication to me. Thoughts ?
>>>>
>>>> While the protocols have to be the same for each channel, you can connect
>>>> different devices. If you have e.g. a thermometer or barimeter that speaks
>>>> the same protocol as a max1162 (i.e. it responds with 2 zero bytes and 2
>>>> bytes of data), you can mix and match that with max1162 devices, once
>>>> you have added support for the thermometer or barimeter to your driver.
>>>
>>> Which driver, the gyroadc driver ? That's only an ADC driver, it
>>
>> The gyroadc driver.
>>
>>> shouldn't be aware of any temperature/pressure sampling or any such
>>> stuff IMO. What you suggest would result in overbloating the gyroadc
>>> driver with support for all sorts of ADCs connected to it's channels
>>> and providing all sorts of information, which I think would violate
>>> the functional separation in the driver model.
>>
>> If the need ever arises, it can be turned into a "gyroadc" framework,
>> and the actual sensor drivers can be enhanced to support the gyroadc
>> driver, in addition to regular SPI master drivers?
>
> So basically we'd have a gyroadc bus type etc ?

Yep.

>> Something for the far future, of course ;-)
>> For now we can limit it to the 3 types of devices officially supported by
>> the GyroADC hardware.
>
> I'm fine with this, but this doesn't answer my question about
> duplication. The gyroadc driver would have to check whether all
> channels are configured to the same device type then ? Doesn't
> seem quite right.

Yes, describing the real topology in DT means the driver has to verify that
all existing child nodes contain the same sensor type.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marek Vasut]

On 01/10/2017 08:06 PM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Tue, Jan 10, 2017 at 7:52 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/10/2017 07:41 PM, Geert Uytterhoeven wrote:
>>> On Tue, Jan 10, 2017 at 7:35 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>> On Mon, Jan 9, 2017 at 10:55 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> On 01/09/2017 10:02 PM, Lars-Peter Clausen wrote:
>>>>>>> On 01/09/2017 09:56 PM, Marek Vasut wrote:
>>>>>>>> On 01/09/2017 09:07 PM, Lars-Peter Clausen wrote:
>>>>>>>>> On 01/09/2017 02:47 PM, Marek Vasut wrote:
>>>>>>>>>> On 01/09/2017 02:45 PM, Lars-Peter Clausen wrote:
>>>>>>>>>>> On 01/09/2017 01:03 AM, Marek Vasut wrote:
>>>>>>>>>>> [...]
>>>>>>>>>>>> +- 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
>>>>>>>>>>>
>>>>>>>>>>> So is this a ADC, or is this just a specialized SPI controller that
>>>>>>>>>>> interfaces to an external ADC?
>>>>>>>>>>
>>>>>>>>>> It's a special SPI controller, except one cannot access the SPI bus
>>>>>>>>>> directly. It sends out clock and reads in the data from the ADC .
>>>>>>>>>
>>>>>>>>> OK, thanks for the clarification. The commit message does not mention this
>>>>>>>>> at the moment and makes it sound like this is a built-in ADC.
>>>>>>>>>
>>>>>>>>> Also the renesas,gyroadc-mode property is more of a driver configuration
>>>>>>>>> setting rather than a description of the hardware, at least it is not very
>>>>>>>>> DT-ish.
>>>>>>>>>
>>>>>>>>> I'd go with something like
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>     &adc {
>>>>>>>>>             compatible = "renesas,r8a7791-gyroadc";
>>>>>>>>>             ...
>>>>>>>>>
>>>>>>>>>             adc@0 {
>>>>>>>>>                     reg = <0>;
>>>>>>>>>                     compatible = "maxim,max1162";
>>>>>>>>
>>>>>>>> But then the max1162 ADC driver* will try to bind to this, right ?
>>>>>>>> And since the MAX1162 is an SPI device, it will fail to work as the
>>>>>>>> ADC driver does not provide any sort of SPI interface. Or is that
>>>>>>>> actually OK ?
>>>>>>>
>>>>>>> Only if you call of_register_spi_devices().
>>>>>>>
>>>>>>> We for example have devices that can either work in SPI or I2C mode and both
>>>>>>> use the same compatible string. Depending on whether the device is a subnode
>>>>>>> of a SPI or I2S controller it is registered as a SPI or I2C device.
>>>>>>>
>>>>>>> This situation is not that different.
>>>>>>
>>>>>> So in this case, the gyroadc driver would instead iterate over the
>>>>>> subnodes, check the compatible string and configure the channel mode
>>>>>> according to the subnode compatible string?
>>>>>
>>>>> That's correct.
>>>>>
>>>>>> But then, the gyroadc only has one configuration register which applies
>>>>>> to all channels, which selects how many bits to read from the connected
>>>>>> SPI-ADC device(s) (sample width). This is not per-channel configurable.
>>>>>> The driver would then have to check if all the compatible properties in
>>>>>> the ADC subnodes select the same ADC or at least ADC which has the same
>>>>>
>>>>> Correct again.
>>>>>
>>>>>> sample width. This looks like a duplication to me. Thoughts ?
>>>>>
>>>>> While the protocols have to be the same for each channel, you can connect
>>>>> different devices. If you have e.g. a thermometer or barimeter that speaks
>>>>> the same protocol as a max1162 (i.e. it responds with 2 zero bytes and 2
>>>>> bytes of data), you can mix and match that with max1162 devices, once
>>>>> you have added support for the thermometer or barimeter to your driver.
>>>>
>>>> Which driver, the gyroadc driver ? That's only an ADC driver, it
>>>
>>> The gyroadc driver.
>>>
>>>> shouldn't be aware of any temperature/pressure sampling or any such
>>>> stuff IMO. What you suggest would result in overbloating the gyroadc
>>>> driver with support for all sorts of ADCs connected to it's channels
>>>> and providing all sorts of information, which I think would violate
>>>> the functional separation in the driver model.
>>>
>>> If the need ever arises, it can be turned into a "gyroadc" framework,
>>> and the actual sensor drivers can be enhanced to support the gyroadc
>>> driver, in addition to regular SPI master drivers?
>>
>> So basically we'd have a gyroadc bus type etc ?
> 
> Yep.
> 
>>> Something for the far future, of course ;-)
>>> For now we can limit it to the 3 types of devices officially supported by
>>> the GyroADC hardware.
>>
>> I'm fine with this, but this doesn't answer my question about
>> duplication. The gyroadc driver would have to check whether all
>> channels are configured to the same device type then ? Doesn't
>> seem quite right.
> 
> Yes, describing the real topology in DT means the driver has to verify that
> all existing child nodes contain the same sensor type.

Now that you put it this way, it does make sense. Thanks.

V4 is out btw.

-- 
Best regards,
Marek Vasut

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

[Geert Uytterhoeven]

Hi Lars,

On Mon, Jan 9, 2017 at 2:45 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 01/09/2017 01:03 AM, Marek Vasut wrote:
> [...]
>> +- 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
>
> So is this a ADC, or is this just a specialized SPI controller that
> interfaces to an external ADC?

It's the latter. It scans 4 or 8 ADCs continuously.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds