* [PATCH v2 0/4] Renesas RZ/G2L ADC driver support
@ 2021-07-19 8:58 Lad Prabhakar
2021-07-19 8:58 ` [PATCH v2 1/4] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Lad Prabhakar @ 2021-07-19 8:58 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring, Jonathan Cameron,
Lars-Peter Clausen, Michael Turquette, Magnus Damm, Stephen Boyd,
Philipp Zabel, Alexandru Ardelean
Cc: linux-iio, devicetree, linux-clk, linux-kernel,
linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar
Hi All,
This patch series adds ADC support for Renesas RZ/G2L family.
Cheers,
Prabhakar
Changes for v2:
* Update binding doc, dropped gpios/renesas-rzg2l,adc-trigger-mode
properties included channel property to represent each wired channel.
* Fixed review comments pointed by Alexandru, implemented pm runtime
support, dropped mlock usage
* Fixed review comments pointed by Jonathan, renamed the macros,
simplified the code.
* Included clock and DT patches
Lad Prabhakar (4):
dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L
A/D converter
iio: adc: Add driver for Renesas RZ/G2L A/D converter
clk: renesas: r9a07g044-cpg: Add clock and reset entries for ADC
arm64: dts: renesas: r9a07g044: Add ADC node
.../bindings/iio/adc/renesas,rzg2l-adc.yaml | 134 +++++
MAINTAINERS | 8 +
arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 42 ++
drivers/clk/renesas/r9a07g044-cpg.c | 6 +
drivers/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/rzg2l_adc.c | 545 ++++++++++++++++++
7 files changed, 746 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
create mode 100644 drivers/iio/adc/rzg2l_adc.c
base-commit: 2734d6c1b1a089fb593ef6a23d4b70903526fe0c
--
2.17.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/4] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter
2021-07-19 8:58 [PATCH v2 0/4] Renesas RZ/G2L ADC driver support Lad Prabhakar
@ 2021-07-19 8:58 ` Lad Prabhakar
2021-07-19 13:47 ` Rob Herring
2021-07-19 8:58 ` [PATCH v2 2/4] iio: adc: Add driver " Lad Prabhakar
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Lad Prabhakar @ 2021-07-19 8:58 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring, Jonathan Cameron,
Lars-Peter Clausen, Michael Turquette, Magnus Damm, Stephen Boyd,
Philipp Zabel, Alexandru Ardelean
Cc: linux-iio, devicetree, linux-clk, linux-kernel,
linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar
Add binding documentation for Renesas RZ/G2L A/D converter block.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
.../bindings/iio/adc/renesas,rzg2l-adc.yaml | 134 ++++++++++++++++++
1 file changed, 134 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
diff --git a/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml b/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
new file mode 100644
index 000000000000..c80201d6a716
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
@@ -0,0 +1,134 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/renesas,rzg2l-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/G2L ADC
+
+maintainers:
+ - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
+
+description: |
+ A/D Converter block is a successive approximation analog-to-digital converter
+ with a 12-bit accuracy. Up to eight analog input channels can be selected.
+ Conversions can be performed in single or repeat mode. Result of the ADC is
+ stored in a 32-bit data register corresponding to each channel.
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - renesas,r9a07g044-adc # RZ/G2{L,LC}
+ - const: renesas,rzg2l-adc
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: converter clock
+ - description: peripheral clock
+
+ clock-names:
+ items:
+ - const: adclk
+ - const: pclk
+
+ power-domains:
+ maxItems: 1
+
+ resets:
+ maxItems: 2
+
+ reset-names:
+ items:
+ - const: presetn
+ - const: adrst-n
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - power-domains
+ - resets
+ - reset-names
+
+patternProperties:
+ "^channel@[0-7]$":
+ $ref: "adc.yaml"
+ type: object
+ description: |
+ Represents the external channels which are connected to the ADC.
+
+ properties:
+ reg:
+ description: |
+ The channel number. It can have up to 8 channels numbered from 0 to 7.
+ items:
+ - minimum: 0
+ maximum: 7
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/r9a07g044-cpg.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ adc: adc@10059000 {
+ compatible = "renesas,r9a07g044-adc", "renesas,rzg2l-adc";
+ reg = <0x10059000 0x400>;
+ interrupts = <GIC_SPI 347 IRQ_TYPE_EDGE_RISING>;
+ clocks = <&cpg CPG_MOD R9A07G044_ADC_ADCLK>,
+ <&cpg CPG_MOD R9A07G044_ADC_PCLK>;
+ clock-names = "adclk", "pclk";
+ power-domains = <&cpg>;
+ resets = <&cpg R9A07G044_ADC_PRESETN>,
+ <&cpg R9A07G044_ADC_ADRST_N>;
+ reset-names = "presetn", "adrst-n";
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@0 {
+ reg = <0>;
+ };
+ channel@1 {
+ reg = <1>;
+ };
+ channel@2 {
+ reg = <2>;
+ };
+ channel@3 {
+ reg = <3>;
+ };
+ channel@4 {
+ reg = <4>;
+ };
+ channel@5 {
+ reg = <5>;
+ };
+ channel@6 {
+ reg = <6>;
+ };
+ channel@7 {
+ reg = <7>;
+ };
+ };
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/4] iio: adc: Add driver for Renesas RZ/G2L A/D converter
2021-07-19 8:58 [PATCH v2 0/4] Renesas RZ/G2L ADC driver support Lad Prabhakar
2021-07-19 8:58 ` [PATCH v2 1/4] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar
@ 2021-07-19 8:58 ` Lad Prabhakar
2021-07-24 18:06 ` Jonathan Cameron
2021-07-19 8:58 ` [PATCH v2 3/4] clk: renesas: r9a07g044-cpg: Add clock and reset entries for ADC Lad Prabhakar
2021-07-19 8:58 ` [PATCH v2 4/4] arm64: dts: renesas: r9a07g044: Add ADC node Lad Prabhakar
3 siblings, 1 reply; 10+ messages in thread
From: Lad Prabhakar @ 2021-07-19 8:58 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring, Jonathan Cameron,
Lars-Peter Clausen, Michael Turquette, Magnus Damm, Stephen Boyd,
Philipp Zabel, Alexandru Ardelean
Cc: linux-iio, devicetree, linux-clk, linux-kernel,
linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar
Add ADC driver support for Renesas RZ/G2L A/D converter in SW
trigger mode.
A/D Converter block is a successive approximation analog-to-digital
converter with a 12-bit accuracy and supports a maximum of 8 input
channels.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
MAINTAINERS | 8 +
drivers/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/rzg2l_adc.c | 545 ++++++++++++++++++++++++++++++++++++
4 files changed, 564 insertions(+)
create mode 100644 drivers/iio/adc/rzg2l_adc.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 6c8be735cc91..6a52f9f4604c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15839,6 +15839,14 @@ L: linux-renesas-soc@vger.kernel.org
S: Maintained
F: drivers/phy/renesas/phy-rcar-gen3-usb*.c
+RENESAS RZ/G2L A/D DRIVER
+M: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
+L: linux-iio@vger.kernel.org
+L: linux-renesas-soc@vger.kernel.org
+S: Supported
+F: Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
+F: drivers/iio/adc/rzg2l_adc.c
+
RESET CONTROLLER FRAMEWORK
M: Philipp Zabel <p.zabel@pengutronix.de>
S: Maintained
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index db0c8fb60515..af168e1c9fdb 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -887,6 +887,16 @@ config ROCKCHIP_SARADC
To compile this driver as a module, choose M here: the
module will be called rockchip_saradc.
+config RZG2L_ADC
+ tristate "Renesas RZ/G2L ADC driver"
+ depends on ARCH_R9A07G044 || COMPILE_TEST
+ help
+ Say yes here to build support for the ADC found in Renesas
+ RZ/G2L family.
+
+ To compile this driver as a module, choose M here: the
+ module will be called rzg2l_adc.
+
config SC27XX_ADC
tristate "Spreadtrum SC27xx series PMICs ADC"
depends on MFD_SC27XX_PMIC || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index f70d877c555a..d68550f493e3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
+obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
obj-$(CONFIG_STX104) += stx104.o
diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
new file mode 100644
index 000000000000..033cdec9e976
--- /dev/null
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -0,0 +1,545 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RZ/G2L A/D Converter driver
+ *
+ * Copyright (c) 2021 Renesas Electronics Europe GmbH
+ *
+ * Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+#define DRIVER_NAME "rzg2l-adc"
+
+#define RZG2L_ADM(n) ((n) * 0x4)
+#define RZG2L_ADM0_ADCE BIT(0)
+#define RZG2L_ADM0_ADBSY BIT(1)
+#define RZG2L_ADM0_PWDWNB BIT(2)
+#define RZG2L_ADM0_SRESB BIT(15)
+#define RZG2L_ADM1_TRG BIT(0)
+#define RZG2L_ADM1_MS BIT(2)
+#define RZG2L_ADM1_BS BIT(4)
+#define RZG2L_ADM1_EGA_CLEAR ~GENMASK(13, 12)
+#define RZG2L_ADM2_CHSEL_CLEAR ~GENMASK(7, 0)
+#define RZG2L_ADM3_ADSMP 0x578
+#define RZG2L_ADM3_ADCMP (0xe << 16)
+#define RZG2L_ADM3_ADIL_CLEAR ~GENMASK(31, 24)
+
+#define RZG2L_ADINT 0x20
+#define RZG2L_ADINT_CH_CLEAR ~GENMASK(7, 0)
+#define RZG2L_ADINT_CSEEN BIT(16)
+#define RZG2L_ADINT_INTS BIT(31)
+#define RZG2L_ADSTS 0x24
+#define RZG2L_ADINT_INTST_MASK GENMASK(7, 0)
+#define RZG2L_ADSTS_CSEST BIT(16)
+#define RZG2L_ADIVC 0x28
+#define RZG2L_ADIVC_DIVADC_CLEAR ~GENMASK(8, 0)
+#define RZG2L_ADIVC_DIVADC_4 0x4
+#define RZG2L_ADFIL 0x2c
+#define RZG2L_ADCR(n) (0x30 + ((n) * 0x4))
+#define RZG2L_ADCR_AD_MASK GENMASK(11, 0)
+
+#define RZG2L_ADC_MAX_CHANNELS 8
+#define RZG2L_ADC_CHN_MASK 0x7
+#define RZG2L_ADC_TIMEOUT usecs_to_jiffies(1 * 4)
+
+struct rzg2l_adc_data {
+ const struct iio_chan_spec *channels;
+ u8 num_channels;
+};
+
+struct rzg2l_adc {
+ void __iomem *base;
+ struct clk *pclk;
+ struct clk *adclk;
+ struct reset_control *presetn;
+ struct reset_control *adrstn;
+ struct completion completion;
+ const struct rzg2l_adc_data *data;
+ u16 last_val[RZG2L_ADC_MAX_CHANNELS];
+};
+
+static const char * const rzg2l_adc_channel_name[] = {
+ "adc0",
+ "adc1",
+ "adc2",
+ "adc3",
+ "adc4",
+ "adc5",
+ "adc6",
+ "adc7",
+};
+
+static unsigned int rzg2l_adc_readl(struct rzg2l_adc *adc, u32 reg)
+{
+ return readl(adc->base + reg);
+}
+
+static void rzg2l_adc_writel(struct rzg2l_adc *adc, unsigned int reg, u32 val)
+{
+ writel(val, adc->base + reg);
+}
+
+static void rzg2l_adc_pwr(struct rzg2l_adc *adc, bool on)
+{
+ u32 reg;
+
+ reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
+ if (on)
+ reg |= RZG2L_ADM0_PWDWNB;
+ else
+ reg &= ~RZG2L_ADM0_PWDWNB;
+ rzg2l_adc_writel(adc, RZG2L_ADM(0), reg);
+ udelay(2);
+}
+
+static void rzg2l_adc_start_stop(struct rzg2l_adc *adc, bool start)
+{
+ int timeout = 5;
+ u32 reg;
+
+ /* stop A/D conversion */
+ reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
+ if (start)
+ reg |= RZG2L_ADM0_ADCE;
+ else
+ reg &= ~RZG2L_ADM0_ADCE;
+ rzg2l_adc_writel(adc, RZG2L_ADM(0), reg);
+
+ if (start)
+ return;
+
+ do {
+ usleep_range(100, 200);
+ reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
+ timeout--;
+ if (!timeout) {
+ pr_err("%s stopping ADC timed out\n", __func__);
+ break;
+ }
+ } while (((reg & RZG2L_ADM0_ADBSY) || (reg & RZG2L_ADM0_ADCE)));
+}
+
+static void rzg2l_set_trigger(struct rzg2l_adc *adc)
+{
+ u32 reg;
+
+ /* SW trigger */
+ reg = rzg2l_adc_readl(adc, RZG2L_ADM(1));
+ reg &= RZG2L_ADM1_EGA_CLEAR;
+ reg &= ~RZG2L_ADM1_BS;
+ reg |= RZG2L_ADM1_MS;
+ reg &= ~RZG2L_ADM1_TRG;
+ rzg2l_adc_writel(adc, RZG2L_ADM(1), reg);
+}
+
+static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
+{
+ u32 reg;
+
+ if (rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_ADBSY)
+ return -EBUSY;
+
+ rzg2l_set_trigger(adc);
+
+ /* select channel */
+ reg = rzg2l_adc_readl(adc, RZG2L_ADM(2));
+ reg &= RZG2L_ADM2_CHSEL_CLEAR;
+ reg |= BIT(ch);
+ rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
+
+ reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
+ reg &= RZG2L_ADM3_ADIL_CLEAR;
+ reg |= RZG2L_ADM3_ADCMP;
+ reg |= RZG2L_ADM3_ADSMP;
+ rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
+
+ reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
+ reg &= RZG2L_ADIVC_DIVADC_CLEAR;
+ reg |= RZG2L_ADIVC_DIVADC_4;
+ rzg2l_adc_writel(adc, RZG2L_ADIVC, reg);
+
+ reg = rzg2l_adc_readl(adc, RZG2L_ADINT);
+ reg &= ~RZG2L_ADINT_INTS;
+ reg &= RZG2L_ADINT_CH_CLEAR;
+ reg |= RZG2L_ADINT_CSEEN;
+ reg |= BIT(ch);
+ rzg2l_adc_writel(adc, RZG2L_ADINT, reg);
+
+ return 0;
+}
+
+static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on)
+{
+ struct device *dev = indio_dev->dev.parent;
+
+ if (on)
+ return pm_runtime_resume_and_get(dev);
+
+ return pm_runtime_put_sync(dev);
+}
+
+static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
+{
+ int ret;
+
+ ret = rzg2l_adc_set_power(indio_dev, true);
+ if (ret)
+ return ret;
+
+ ret = rzg2l_adc_conversion_setup(adc, ch);
+ if (ret) {
+ rzg2l_adc_set_power(indio_dev, false);
+ return ret;
+ }
+
+ reinit_completion(&adc->completion);
+
+ rzg2l_adc_start_stop(adc, true);
+
+ if (!wait_for_completion_timeout(&adc->completion, RZG2L_ADC_TIMEOUT)) {
+ rzg2l_adc_writel(adc, RZG2L_ADINT,
+ rzg2l_adc_readl(adc, RZG2L_ADINT) & RZG2L_ADINT_CH_CLEAR);
+ rzg2l_adc_start_stop(adc, false);
+ rzg2l_adc_set_power(indio_dev, false);
+ return -ETIMEDOUT;
+ }
+
+ return rzg2l_adc_set_power(indio_dev, false);
+}
+
+static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct rzg2l_adc *adc = iio_priv(indio_dev);
+ int ret;
+ u8 ch;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (chan->type != IIO_VOLTAGE)
+ return -EINVAL;
+
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ ch = chan->channel & RZG2L_ADC_CHN_MASK;
+ ret = rzg2l_adc_conversion(indio_dev, adc, ch);
+ if (ret) {
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+ }
+ *val = adc->last_val[ch];
+ iio_device_release_direct_mode(indio_dev);
+
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int rzg2l_adc_read_label(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan,
+ char *label)
+{
+ if (chan->channel >= RZG2L_ADC_MAX_CHANNELS)
+ return -EINVAL;
+
+ return snprintf(label, PAGE_SIZE, "%s\n", rzg2l_adc_channel_name[chan->channel]);
+}
+
+static const struct iio_info rzg2l_adc_iio_info = {
+ .read_raw = rzg2l_adc_read_raw,
+ .read_label = rzg2l_adc_read_label,
+};
+
+static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
+{
+ struct rzg2l_adc *adc = (struct rzg2l_adc *)dev_id;
+ u8 intst;
+ u32 reg;
+ u8 i;
+
+ reg = rzg2l_adc_readl(adc, RZG2L_ADSTS);
+ if (reg & RZG2L_ADSTS_CSEST) {
+ rzg2l_adc_writel(adc, RZG2L_ADSTS, reg);
+ return IRQ_HANDLED;
+ }
+
+ intst = reg & RZG2L_ADINT_INTST_MASK;
+ if (!intst)
+ return IRQ_HANDLED;
+
+ for (i = 0; i < RZG2L_ADC_MAX_CHANNELS; i++) {
+ if (intst & BIT(i))
+ adc->last_val[i] = rzg2l_adc_readl(adc, RZG2L_ADCR(i)) & RZG2L_ADCR_AD_MASK;
+ }
+
+ rzg2l_adc_writel(adc, RZG2L_ADSTS, reg);
+
+ complete(&adc->completion);
+
+ return IRQ_HANDLED;
+}
+
+static int rzg2l_adc_parse_of(struct platform_device *pdev, struct rzg2l_adc *adc)
+{
+ struct iio_chan_spec *chan_array;
+ struct fwnode_handle *fwnode;
+ struct rzg2l_adc_data *data;
+ unsigned int channel;
+ int num_channels;
+ int ret;
+ u8 i;
+
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ num_channels = device_get_child_node_count(&pdev->dev);
+ if (!num_channels) {
+ dev_err(&pdev->dev, "no channel children\n");
+ return -ENODEV;
+ }
+
+ if (num_channels > RZG2L_ADC_MAX_CHANNELS) {
+ dev_err(&pdev->dev, "num of channel children out of range\n");
+ return -EINVAL;
+ }
+
+ chan_array = devm_kcalloc(&pdev->dev, num_channels, sizeof(*chan_array),
+ GFP_KERNEL);
+ if (!chan_array)
+ return -ENOMEM;
+
+ i = 0;
+ device_for_each_child_node(&pdev->dev, fwnode) {
+ ret = fwnode_property_read_u32(fwnode, "reg", &channel);
+ if (ret)
+ return ret;
+
+ if (channel >= RZG2L_ADC_MAX_CHANNELS)
+ return -EINVAL;
+
+ chan_array[i].type = IIO_VOLTAGE;
+ chan_array[i].indexed = 1;
+ chan_array[i].channel = channel;
+ chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+ chan_array[i].datasheet_name = rzg2l_adc_channel_name[channel];
+ i++;
+ }
+
+ data->num_channels = num_channels;
+ data->channels = chan_array;
+ adc->data = data;
+
+ return 0;
+}
+
+static int rzg2l_adc_sw_reset(struct rzg2l_adc *adc)
+{
+ int timeout = 5;
+ u32 val;
+
+ val = rzg2l_adc_readl(adc, RZG2L_ADM(0));
+ val |= RZG2L_ADM0_SRESB;
+ rzg2l_adc_writel(adc, RZG2L_ADM(0), val);
+
+ while (!(rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_SRESB)) {
+ if (!timeout)
+ return -EBUSY;
+ timeout--;
+ usleep_range(100, 200);
+ }
+
+ return 0;
+}
+
+static int rzg2l_adc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct iio_dev *indio_dev;
+ struct rzg2l_adc *adc;
+ int ret;
+ int irq;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ adc = iio_priv(indio_dev);
+
+ ret = rzg2l_adc_parse_of(pdev, adc);
+ if (ret)
+ return ret;
+
+ adc->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(adc->base))
+ return PTR_ERR(adc->base);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(dev, "no irq resource\n");
+ return irq;
+ }
+
+ adc->pclk = devm_clk_get(dev, "pclk");
+ if (IS_ERR(adc->pclk)) {
+ dev_err(dev, "Failed to get pclk");
+ return PTR_ERR(adc->pclk);
+ }
+
+ adc->adclk = devm_clk_get(dev, "adclk");
+ if (IS_ERR(adc->adclk)) {
+ dev_err(dev, "Failed to get adclk");
+ return PTR_ERR(adc->adclk);
+ }
+
+ adc->adrstn = devm_reset_control_get_exclusive(dev, "adrst-n");
+ if (IS_ERR(adc->adrstn)) {
+ dev_err(dev, "failed to get adrstn\n");
+ return PTR_ERR(adc->adrstn);
+ }
+
+ adc->presetn = devm_reset_control_get_exclusive(dev, "presetn");
+ if (IS_ERR(adc->presetn)) {
+ dev_err(dev, "failed to get presetn\n");
+ return PTR_ERR(adc->presetn);
+ }
+
+ ret = reset_control_deassert(adc->adrstn);
+ if (ret)
+ return ret;
+
+ ret = reset_control_deassert(adc->presetn);
+ if (ret)
+ goto assert_adrstn;
+
+ ret = clk_prepare_enable(adc->pclk);
+ if (ret)
+ goto assert_presetn;
+
+ ret = rzg2l_adc_sw_reset(adc);
+ if (ret)
+ goto unprepare_pclk;
+
+ clk_disable_unprepare(adc->pclk);
+
+ init_completion(&adc->completion);
+
+ platform_set_drvdata(pdev, indio_dev);
+
+ ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
+ 0, dev_name(dev), adc);
+ if (ret < 0)
+ goto assert_presetn;
+
+ indio_dev->name = DRIVER_NAME;
+ indio_dev->info = &rzg2l_adc_iio_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = adc->data->channels;
+ indio_dev->num_channels = adc->data->num_channels;
+
+ pm_runtime_enable(dev);
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ goto err_iio_device_register;
+
+ return 0;
+
+unprepare_pclk:
+ clk_disable_unprepare(adc->pclk);
+err_iio_device_register:
+ pm_runtime_disable(dev);
+assert_presetn:
+ reset_control_assert(adc->presetn);
+assert_adrstn:
+ reset_control_assert(adc->adrstn);
+ return ret;
+}
+
+static int rzg2l_adc_remove(struct platform_device *pdev)
+{
+ struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+ struct rzg2l_adc *adc = iio_priv(indio_dev);
+
+ reset_control_assert(adc->presetn);
+ reset_control_assert(adc->adrstn);
+ pm_runtime_disable(indio_dev->dev.parent);
+
+ return 0;
+}
+
+static const struct of_device_id rzg2l_adc_match[] = {
+ { .compatible = "renesas,rzg2l-adc",},
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzg2l_adc_match);
+
+static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct rzg2l_adc *adc = iio_priv(indio_dev);
+
+ rzg2l_adc_pwr(adc, false);
+ clk_disable_unprepare(adc->adclk);
+ clk_disable_unprepare(adc->pclk);
+
+ return 0;
+}
+
+static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct rzg2l_adc *adc = iio_priv(indio_dev);
+ int ret;
+
+ ret = clk_prepare_enable(adc->pclk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(adc->adclk);
+ if (ret)
+ return ret;
+
+ rzg2l_adc_pwr(adc, true);
+
+ return 0;
+}
+
+static const struct dev_pm_ops rzg2l_adc_pm_ops = {
+ SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend,
+ rzg2l_adc_pm_runtime_resume,
+ NULL)
+};
+
+static struct platform_driver rzg2l_adc_driver = {
+ .probe = rzg2l_adc_probe,
+ .remove = rzg2l_adc_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .of_match_table = rzg2l_adc_match,
+ .pm = &rzg2l_adc_pm_ops,
+ },
+};
+
+module_platform_driver(rzg2l_adc_driver);
+
+MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/G2L ADC driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/4] clk: renesas: r9a07g044-cpg: Add clock and reset entries for ADC
2021-07-19 8:58 [PATCH v2 0/4] Renesas RZ/G2L ADC driver support Lad Prabhakar
2021-07-19 8:58 ` [PATCH v2 1/4] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar
2021-07-19 8:58 ` [PATCH v2 2/4] iio: adc: Add driver " Lad Prabhakar
@ 2021-07-19 8:58 ` Lad Prabhakar
2021-07-19 9:15 ` Geert Uytterhoeven
2021-07-19 8:58 ` [PATCH v2 4/4] arm64: dts: renesas: r9a07g044: Add ADC node Lad Prabhakar
3 siblings, 1 reply; 10+ messages in thread
From: Lad Prabhakar @ 2021-07-19 8:58 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring, Jonathan Cameron,
Lars-Peter Clausen, Michael Turquette, Magnus Damm, Stephen Boyd,
Philipp Zabel, Alexandru Ardelean
Cc: linux-iio, devicetree, linux-clk, linux-kernel,
linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar
Add clock and reset entries for ADC block in CPG driver.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
drivers/clk/renesas/r9a07g044-cpg.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index ae24e0397d3c..f4ebbde358c6 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -102,6 +102,10 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
0x584, 4),
DEF_MOD("sci0", R9A07G044_SCI0_CLKP, R9A07G044_CLK_P0,
0x588, 0),
+ DEF_MOD("adc_adclk", R9A07G044_ADC_ADCLK, R9A07G044_CLK_TSU,
+ 0x5a8, 0),
+ DEF_MOD("adc_pclk", R9A07G044_ADC_PCLK, R9A07G044_CLK_P0,
+ 0x5a8, 1),
};
static struct rzg2l_reset r9a07g044_resets[] = {
@@ -114,6 +118,8 @@ static struct rzg2l_reset r9a07g044_resets[] = {
DEF_RST(R9A07G044_SCIF3_RST_SYSTEM_N, 0x884, 3),
DEF_RST(R9A07G044_SCIF4_RST_SYSTEM_N, 0x884, 4),
DEF_RST(R9A07G044_SCI0_RST, 0x888, 0),
+ DEF_RST(R9A07G044_ADC_PRESETN, 0x8a8, 0),
+ DEF_RST(R9A07G044_ADC_ADRST_N, 0x8a8, 1),
};
static const unsigned int r9a07g044_crit_mod_clks[] __initconst = {
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 4/4] arm64: dts: renesas: r9a07g044: Add ADC node
2021-07-19 8:58 [PATCH v2 0/4] Renesas RZ/G2L ADC driver support Lad Prabhakar
` (2 preceding siblings ...)
2021-07-19 8:58 ` [PATCH v2 3/4] clk: renesas: r9a07g044-cpg: Add clock and reset entries for ADC Lad Prabhakar
@ 2021-07-19 8:58 ` Lad Prabhakar
3 siblings, 0 replies; 10+ messages in thread
From: Lad Prabhakar @ 2021-07-19 8:58 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring, Jonathan Cameron,
Lars-Peter Clausen, Michael Turquette, Magnus Damm, Stephen Boyd,
Philipp Zabel, Alexandru Ardelean
Cc: linux-iio, devicetree, linux-clk, linux-kernel,
linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar
Add ADC node to R9A07G044 (RZ/G2L) SoC DTSI.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 42 ++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 01482d227506..2c07c500f617 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -89,6 +89,48 @@
status = "disabled";
};
+ adc: adc@10059000 {
+ compatible = "renesas,r9a07g044-adc", "renesas,rzg2l-adc";
+ reg = <0 0x10059000 0 0x400>;
+ interrupts = <GIC_SPI 347 IRQ_TYPE_EDGE_RISING>;
+ clocks = <&cpg CPG_MOD R9A07G044_ADC_ADCLK>,
+ <&cpg CPG_MOD R9A07G044_ADC_PCLK>;
+ clock-names = "adclk", "pclk";
+ resets = <&cpg R9A07G044_ADC_PRESETN>,
+ <&cpg R9A07G044_ADC_ADRST_N>;
+ reset-names = "presetn", "adrst-n";
+ power-domains = <&cpg>;
+ status = "disabled";
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@0 {
+ reg = <0>;
+ };
+ channel@1 {
+ reg = <1>;
+ };
+ channel@2 {
+ reg = <2>;
+ };
+ channel@3 {
+ reg = <3>;
+ };
+ channel@4 {
+ reg = <4>;
+ };
+ channel@5 {
+ reg = <5>;
+ };
+ channel@6 {
+ reg = <6>;
+ };
+ channel@7 {
+ reg = <7>;
+ };
+ };
+
cpg: clock-controller@11010000 {
compatible = "renesas,r9a07g044-cpg";
reg = <0 0x11010000 0 0x10000>;
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/4] clk: renesas: r9a07g044-cpg: Add clock and reset entries for ADC
2021-07-19 8:58 ` [PATCH v2 3/4] clk: renesas: r9a07g044-cpg: Add clock and reset entries for ADC Lad Prabhakar
@ 2021-07-19 9:15 ` Geert Uytterhoeven
0 siblings, 0 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2021-07-19 9:15 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Rob Herring, Jonathan Cameron, Lars-Peter Clausen,
Michael Turquette, Magnus Damm, Stephen Boyd, Philipp Zabel,
Alexandru Ardelean, linux-iio,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-clk, Linux Kernel Mailing List, Linux-Renesas, Prabhakar,
Biju Das
On Mon, Jul 19, 2021 at 10:59 AM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add clock and reset entries for ADC block in CPG driver.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk-for-v5.15.
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter
2021-07-19 8:58 ` [PATCH v2 1/4] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar
@ 2021-07-19 13:47 ` Rob Herring
0 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2021-07-19 13:47 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Geert Uytterhoeven, Rob Herring, Magnus Damm, Jonathan Cameron,
Philipp Zabel, linux-clk, Prabhakar, devicetree,
Lars-Peter Clausen, Michael Turquette, Biju Das, linux-kernel,
linux-renesas-soc, Stephen Boyd, linux-iio, Alexandru Ardelean
On Mon, 19 Jul 2021 09:58:37 +0100, Lad Prabhakar wrote:
> Add binding documentation for Renesas RZ/G2L A/D converter block.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> .../bindings/iio/adc/renesas,rzg2l-adc.yaml | 134 ++++++++++++++++++
> 1 file changed, 134 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.example.dts:26.28-29 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1418: dt_binding_check] Error 2
\ndoc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/1506856
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] iio: adc: Add driver for Renesas RZ/G2L A/D converter
2021-07-19 8:58 ` [PATCH v2 2/4] iio: adc: Add driver " Lad Prabhakar
@ 2021-07-24 18:06 ` Jonathan Cameron
2021-07-25 12:18 ` Lad, Prabhakar
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2021-07-24 18:06 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Geert Uytterhoeven, Rob Herring, Lars-Peter Clausen,
Michael Turquette, Magnus Damm, Stephen Boyd, Philipp Zabel,
Alexandru Ardelean, linux-iio, devicetree, linux-clk,
linux-kernel, linux-renesas-soc, Prabhakar, Biju Das
On Mon, 19 Jul 2021 09:58:38 +0100
Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add ADC driver support for Renesas RZ/G2L A/D converter in SW
> trigger mode.
>
> A/D Converter block is a successive approximation analog-to-digital
> converter with a 12-bit accuracy and supports a maximum of 8 input
> channels.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Hi Lad,
A few additional comments inline.
Thanks,
Jonathan
> ---
> MAINTAINERS | 8 +
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/rzg2l_adc.c | 545 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 564 insertions(+)
> create mode 100644 drivers/iio/adc/rzg2l_adc.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6c8be735cc91..6a52f9f4604c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15839,6 +15839,14 @@ L: linux-renesas-soc@vger.kernel.org
> S: Maintained
> F: drivers/phy/renesas/phy-rcar-gen3-usb*.c
>
> +RENESAS RZ/G2L A/D DRIVER
> +M: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> +L: linux-iio@vger.kernel.org
> +L: linux-renesas-soc@vger.kernel.org
> +S: Supported
> +F: Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
> +F: drivers/iio/adc/rzg2l_adc.c
> +
> RESET CONTROLLER FRAMEWORK
> M: Philipp Zabel <p.zabel@pengutronix.de>
> S: Maintained
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index db0c8fb60515..af168e1c9fdb 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -887,6 +887,16 @@ config ROCKCHIP_SARADC
> To compile this driver as a module, choose M here: the
> module will be called rockchip_saradc.
>
> +config RZG2L_ADC
> + tristate "Renesas RZ/G2L ADC driver"
> + depends on ARCH_R9A07G044 || COMPILE_TEST
> + help
> + Say yes here to build support for the ADC found in Renesas
> + RZ/G2L family.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called rzg2l_adc.
> +
> config SC27XX_ADC
> tristate "Spreadtrum SC27xx series PMICs ADC"
> depends on MFD_SC27XX_PMIC || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index f70d877c555a..d68550f493e3 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
> obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
> obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
> obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> +obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
> obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
> obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
> obj-$(CONFIG_STX104) += stx104.o
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> new file mode 100644
> index 000000000000..033cdec9e976
> --- /dev/null
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -0,0 +1,545 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ/G2L A/D Converter driver
> + *
> + * Copyright (c) 2021 Renesas Electronics Europe GmbH
> + *
> + * Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
Add mod_devicetable.h and drop the of specific headers
as nothing in them used directly as far as I can tell.
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +#define DRIVER_NAME "rzg2l-adc"
> +
> +#define RZG2L_ADM(n) ((n) * 0x4)
> +#define RZG2L_ADM0_ADCE BIT(0)
> +#define RZG2L_ADM0_ADBSY BIT(1)
> +#define RZG2L_ADM0_PWDWNB BIT(2)
> +#define RZG2L_ADM0_SRESB BIT(15)
> +#define RZG2L_ADM1_TRG BIT(0)
> +#define RZG2L_ADM1_MS BIT(2)
> +#define RZG2L_ADM1_BS BIT(4)
> +#define RZG2L_ADM1_EGA_CLEAR ~GENMASK(13, 12)
> +#define RZG2L_ADM2_CHSEL_CLEAR ~GENMASK(7, 0)
I'd rather see these defined as _MASK and the
inversion made explicit inline.
> +#define RZG2L_ADM3_ADSMP 0x578
> +#define RZG2L_ADM3_ADCMP (0xe << 16)
> +#define RZG2L_ADM3_ADIL_CLEAR ~GENMASK(31, 24)
> +
> +#define RZG2L_ADINT 0x20
> +#define RZG2L_ADINT_CH_CLEAR ~GENMASK(7, 0)
> +#define RZG2L_ADINT_CSEEN BIT(16)
> +#define RZG2L_ADINT_INTS BIT(31)
> +#define RZG2L_ADSTS 0x24
> +#define RZG2L_ADINT_INTST_MASK GENMASK(7, 0)
> +#define RZG2L_ADSTS_CSEST BIT(16)
> +#define RZG2L_ADIVC 0x28
> +#define RZG2L_ADIVC_DIVADC_CLEAR ~GENMASK(8, 0)
> +#define RZG2L_ADIVC_DIVADC_4 0x4
Perhaps adopt the _MASK and FIELD_PREP() approach to setting
particular values. That way it is obvious at the call site that
DVADC_4 is a value within the DIVADC_MASK covered field.
> +#define RZG2L_ADFIL 0x2c
> +#define RZG2L_ADCR(n) (0x30 + ((n) * 0x4))
> +#define RZG2L_ADCR_AD_MASK GENMASK(11, 0)
> +
> +#define RZG2L_ADC_MAX_CHANNELS 8
> +#define RZG2L_ADC_CHN_MASK 0x7
> +#define RZG2L_ADC_TIMEOUT usecs_to_jiffies(1 * 4)
> +
> +struct rzg2l_adc_data {
> + const struct iio_chan_spec *channels;
> + u8 num_channels;
> +};
> +
> +struct rzg2l_adc {
> + void __iomem *base;
> + struct clk *pclk;
> + struct clk *adclk;
> + struct reset_control *presetn;
> + struct reset_control *adrstn;
> + struct completion completion;
> + const struct rzg2l_adc_data *data;
> + u16 last_val[RZG2L_ADC_MAX_CHANNELS];
> +};
> +
> +static const char * const rzg2l_adc_channel_name[] = {
> + "adc0",
Is it useful to expose these as labels to userspace?
Seems unnecessary given they map directly to the channel
numbers.
> + "adc1",
> + "adc2",
> + "adc3",
> + "adc4",
> + "adc5",
> + "adc6",
> + "adc7",
> +};
> +
> +static unsigned int rzg2l_adc_readl(struct rzg2l_adc *adc, u32 reg)
> +{
> + return readl(adc->base + reg);
> +}
> +
> +static void rzg2l_adc_writel(struct rzg2l_adc *adc, unsigned int reg, u32 val)
> +{
> + writel(val, adc->base + reg);
> +}
> +
> +static void rzg2l_adc_pwr(struct rzg2l_adc *adc, bool on)
> +{
> + u32 reg;
> +
> + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
> + if (on)
> + reg |= RZG2L_ADM0_PWDWNB;
> + else
> + reg &= ~RZG2L_ADM0_PWDWNB;
> + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg);
> + udelay(2);
> +}
> +
> +static void rzg2l_adc_start_stop(struct rzg2l_adc *adc, bool start)
> +{
> + int timeout = 5;
> + u32 reg;
> +
> + /* stop A/D conversion */
> + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
> + if (start)
> + reg |= RZG2L_ADM0_ADCE;
> + else
> + reg &= ~RZG2L_ADM0_ADCE;
> + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg);
> +
> + if (start)
> + return;
> +
> + do {
> + usleep_range(100, 200);
> + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
> + timeout--;
> + if (!timeout) {
> + pr_err("%s stopping ADC timed out\n", __func__);
> + break;
> + }
> + } while (((reg & RZG2L_ADM0_ADBSY) || (reg & RZG2L_ADM0_ADCE)));
> +}
> +
> +static void rzg2l_set_trigger(struct rzg2l_adc *adc)
> +{
> + u32 reg;
> +
> + /* SW trigger */
> + reg = rzg2l_adc_readl(adc, RZG2L_ADM(1));
> + reg &= RZG2L_ADM1_EGA_CLEAR;
> + reg &= ~RZG2L_ADM1_BS;
> + reg |= RZG2L_ADM1_MS;
> + reg &= ~RZG2L_ADM1_TRG;
> + rzg2l_adc_writel(adc, RZG2L_ADM(1), reg);
> +}
> +
> +static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
> +{
> + u32 reg;
> +
> + if (rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_ADBSY)
> + return -EBUSY;
> +
> + rzg2l_set_trigger(adc);
> +
> + /* select channel */
> + reg = rzg2l_adc_readl(adc, RZG2L_ADM(2));
> + reg &= RZG2L_ADM2_CHSEL_CLEAR;
> + reg |= BIT(ch);
> + rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
> +
> + reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
> + reg &= RZG2L_ADM3_ADIL_CLEAR;
> + reg |= RZG2L_ADM3_ADCMP;
> + reg |= RZG2L_ADM3_ADSMP;
No loss in readability in combining the two lines above and shorter
code is always good. Having the mask on a separate line makes
sense but the |= pair doesn't.
What is a bit unusual here is you clear some bits then write different
ones. That is presumably relying on the fact that the ADCCMP part
is always set to the same value. That seems unwise to assume from
a long term maintainability point of view.
> + rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
> +
> + reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
> + reg &= RZG2L_ADIVC_DIVADC_CLEAR;
> + reg |= RZG2L_ADIVC_DIVADC_4;
> + rzg2l_adc_writel(adc, RZG2L_ADIVC, reg);
> +
> + reg = rzg2l_adc_readl(adc, RZG2L_ADINT);
> + reg &= ~RZG2L_ADINT_INTS;
> + reg &= RZG2L_ADINT_CH_CLEAR;
> + reg |= RZG2L_ADINT_CSEEN;
> + reg |= BIT(ch);
> + rzg2l_adc_writel(adc, RZG2L_ADINT, reg);
> +
> + return 0;
> +}
> +
> +static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on)
> +{
> + struct device *dev = indio_dev->dev.parent;
> +
> + if (on)
> + return pm_runtime_resume_and_get(dev);
> +
> + return pm_runtime_put_sync(dev);
> +}
> +
> +static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
> +{
> + int ret;
> +
> + ret = rzg2l_adc_set_power(indio_dev, true);
> + if (ret)
> + return ret;
> +
> + ret = rzg2l_adc_conversion_setup(adc, ch);
> + if (ret) {
> + rzg2l_adc_set_power(indio_dev, false);
> + return ret;
> + }
> +
> + reinit_completion(&adc->completion);
> +
> + rzg2l_adc_start_stop(adc, true);
> +
> + if (!wait_for_completion_timeout(&adc->completion, RZG2L_ADC_TIMEOUT)) {
> + rzg2l_adc_writel(adc, RZG2L_ADINT,
> + rzg2l_adc_readl(adc, RZG2L_ADINT) & RZG2L_ADINT_CH_CLEAR);
> + rzg2l_adc_start_stop(adc, false);
> + rzg2l_adc_set_power(indio_dev, false);
> + return -ETIMEDOUT;
> + }
> +
> + return rzg2l_adc_set_power(indio_dev, false);
> +}
> +
> +static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct rzg2l_adc *adc = iio_priv(indio_dev);
> + int ret;
> + u8 ch;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (chan->type != IIO_VOLTAGE)
> + return -EINVAL;
> +
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> +
> + ch = chan->channel & RZG2L_ADC_CHN_MASK;
> + ret = rzg2l_adc_conversion(indio_dev, adc, ch);
> + if (ret) {
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + }
> + *val = adc->last_val[ch];
> + iio_device_release_direct_mode(indio_dev);
> +
> + return IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int rzg2l_adc_read_label(struct iio_dev *iio_dev,
> + const struct iio_chan_spec *chan,
> + char *label)
> +{
> + if (chan->channel >= RZG2L_ADC_MAX_CHANNELS)
> + return -EINVAL;
> +
> + return snprintf(label, PAGE_SIZE, "%s\n", rzg2l_adc_channel_name[chan->channel]);
sysfs_emit()?.
> +}
> +
> +static const struct iio_info rzg2l_adc_iio_info = {
> + .read_raw = rzg2l_adc_read_raw,
> + .read_label = rzg2l_adc_read_label,
> +};
> +
> +static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
> +{
> + struct rzg2l_adc *adc = (struct rzg2l_adc *)dev_id;
> + u8 intst;
> + u32 reg;
> + u8 i;
> +
> + reg = rzg2l_adc_readl(adc, RZG2L_ADSTS);
> + if (reg & RZG2L_ADSTS_CSEST) {
Probably good to add a brief comment on what is going on in this check.
Perhaps even an error print if it's an error path.
> + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg);
> + return IRQ_HANDLED;
> + }
> +
> + intst = reg & RZG2L_ADINT_INTST_MASK;
> + if (!intst)
I'm guessing this means spurious interrupt in which case IRQ_NONE
so the core irq code knows about it.
> + return IRQ_HANDLED;
> +
> + for (i = 0; i < RZG2L_ADC_MAX_CHANNELS; i++) {
> + if (intst & BIT(i))
for_each_bit_set() and make intst a long so the types are right for
that macro
> + adc->last_val[i] = rzg2l_adc_readl(adc, RZG2L_ADCR(i)) & RZG2L_ADCR_AD_MASK;
> + }
> +
> + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg);
> +
> + complete(&adc->completion);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int rzg2l_adc_parse_of(struct platform_device *pdev, struct rzg2l_adc *adc)
> +{
> + struct iio_chan_spec *chan_array;
> + struct fwnode_handle *fwnode;
> + struct rzg2l_adc_data *data;
> + unsigned int channel;
> + int num_channels;
> + int ret;
> + u8 i;
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + num_channels = device_get_child_node_count(&pdev->dev);
> + if (!num_channels) {
> + dev_err(&pdev->dev, "no channel children\n");
> + return -ENODEV;
> + }
> +
> + if (num_channels > RZG2L_ADC_MAX_CHANNELS) {
> + dev_err(&pdev->dev, "num of channel children out of range\n");
> + return -EINVAL;
> + }
> +
> + chan_array = devm_kcalloc(&pdev->dev, num_channels, sizeof(*chan_array),
> + GFP_KERNEL);
> + if (!chan_array)
> + return -ENOMEM;
> +
> + i = 0;
> + device_for_each_child_node(&pdev->dev, fwnode) {
> + ret = fwnode_property_read_u32(fwnode, "reg", &channel);
> + if (ret)
> + return ret;
> +
> + if (channel >= RZG2L_ADC_MAX_CHANNELS)
> + return -EINVAL;
> +
> + chan_array[i].type = IIO_VOLTAGE;
> + chan_array[i].indexed = 1;
> + chan_array[i].channel = channel;
> + chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> + chan_array[i].datasheet_name = rzg2l_adc_channel_name[channel];
> + i++;
> + }
> +
> + data->num_channels = num_channels;
> + data->channels = chan_array;
> + adc->data = data;
> +
> + return 0;
> +}
> +
> +static int rzg2l_adc_sw_reset(struct rzg2l_adc *adc)
> +{
> + int timeout = 5;
> + u32 val;
> +
> + val = rzg2l_adc_readl(adc, RZG2L_ADM(0));
> + val |= RZG2L_ADM0_SRESB;
> + rzg2l_adc_writel(adc, RZG2L_ADM(0), val);
> +
> + while (!(rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_SRESB)) {
> + if (!timeout)
> + return -EBUSY;
> + timeout--;
> + usleep_range(100, 200);
> + }
> +
> + return 0;
> +}
> +
> +static int rzg2l_adc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct iio_dev *indio_dev;
> + struct rzg2l_adc *adc;
> + int ret;
> + int irq;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + adc = iio_priv(indio_dev);
> +
> + ret = rzg2l_adc_parse_of(pdev, adc);
*_parse_properties() or *_parse_firmware()
as it's not of specific now.
> + if (ret)
> + return ret;
> +
> + adc->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(adc->base))
> + return PTR_ERR(adc->base);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(dev, "no irq resource\n");
> + return irq;
> + }
> +
> + adc->pclk = devm_clk_get(dev, "pclk");
> + if (IS_ERR(adc->pclk)) {
> + dev_err(dev, "Failed to get pclk");
> + return PTR_ERR(adc->pclk);
> + }
> +
> + adc->adclk = devm_clk_get(dev, "adclk");
> + if (IS_ERR(adc->adclk)) {
> + dev_err(dev, "Failed to get adclk");
> + return PTR_ERR(adc->adclk);
> + }
> +
> + adc->adrstn = devm_reset_control_get_exclusive(dev, "adrst-n");
> + if (IS_ERR(adc->adrstn)) {
> + dev_err(dev, "failed to get adrstn\n");
> + return PTR_ERR(adc->adrstn);
> + }
> +
> + adc->presetn = devm_reset_control_get_exclusive(dev, "presetn");
> + if (IS_ERR(adc->presetn)) {
> + dev_err(dev, "failed to get presetn\n");
> + return PTR_ERR(adc->presetn);
> + }
> +
> + ret = reset_control_deassert(adc->adrstn);
> + if (ret)
> + return ret;
devm_add_action_or_reset() here to make the reasserting of these part
of the automated cleanup.
> +
> + ret = reset_control_deassert(adc->presetn);
> + if (ret)
> + goto assert_adrstn;
> +
> + ret = clk_prepare_enable(adc->pclk);
> + if (ret)
> + goto assert_presetn;
> +
> + ret = rzg2l_adc_sw_reset(adc);
> + if (ret)
> + goto unprepare_pclk;
> +
> + clk_disable_unprepare(adc->pclk);
As below, reorder as:
ret = rzg2l_adc_sw_reset(adc);
clk_disable_unprepare(adc->pclk);
if (ret)
goto assert_preset_n;
> +
> + init_completion(&adc->completion);
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
> + ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
> + 0, dev_name(dev), adc);
Another aspect of the comment below about mixing and matching between
devm and not. If the order of remove is not that opposite of probe
then there should be a comment saying why. There are far too many
race conditions introduced by people doing things in a different order
without carefully considering why.
> + if (ret < 0)
> + goto assert_presetn;
> +
> + indio_dev->name = DRIVER_NAME;
> + indio_dev->info = &rzg2l_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = adc->data->channels;
> + indio_dev->num_channels = adc->data->num_channels;
> +
> + pm_runtime_enable(dev);
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + goto err_iio_device_register;
> +
> + return 0;
> +
> +unprepare_pclk:
> + clk_disable_unprepare(adc->pclk);
Bad sign that you have this here and not in remove..
When you have cases like this, it's normally better to
either factor out the relevant code into a function
that lets you do this call whether or not there is an
error, or to just move this up to the relevant if statements.
> +err_iio_device_register:
> + pm_runtime_disable(dev);
> +assert_presetn:
> + reset_control_assert(adc->presetn);
> +assert_adrstn:
> + reset_control_assert(adc->adrstn);
> + return ret;
> +}
> +
> +static int rzg2l_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct rzg2l_adc *adc = iio_priv(indio_dev);
> +
> + reset_control_assert(adc->presetn);
> + reset_control_assert(adc->adrstn);
> + pm_runtime_disable(indio_dev->dev.parent);
You can't mix and match devm managed and unmanaged like this.
The userspace interfce is still exposed after you've put the
device into reset which is unlikely to be a good idea.
One option is to use devm_add_action_or_reset() to make
this stuff happen in the managed flow, which happens after
remove is done. That would let you drop remove() entirely.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rzg2l_adc_match[] = {
> + { .compatible = "renesas,rzg2l-adc",},
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rzg2l_adc_match);
> +
> +static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct rzg2l_adc *adc = iio_priv(indio_dev);
> +
> + rzg2l_adc_pwr(adc, false);
> + clk_disable_unprepare(adc->adclk);
> + clk_disable_unprepare(adc->pclk);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct rzg2l_adc *adc = iio_priv(indio_dev);
> + int ret;
> +
> + ret = clk_prepare_enable(adc->pclk);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(adc->adclk);
> + if (ret)
> + return ret;
> +
> + rzg2l_adc_pwr(adc, true);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rzg2l_adc_pm_ops = {
> + SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend,
> + rzg2l_adc_pm_runtime_resume,
> + NULL)
> +};
> +
> +static struct platform_driver rzg2l_adc_driver = {
> + .probe = rzg2l_adc_probe,
> + .remove = rzg2l_adc_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .of_match_table = rzg2l_adc_match,
> + .pm = &rzg2l_adc_pm_ops,
> + },
> +};
> +
> +module_platform_driver(rzg2l_adc_driver);
> +
> +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas RZ/G2L ADC driver");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] iio: adc: Add driver for Renesas RZ/G2L A/D converter
2021-07-24 18:06 ` Jonathan Cameron
@ 2021-07-25 12:18 ` Lad, Prabhakar
2021-07-25 14:55 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Lad, Prabhakar @ 2021-07-25 12:18 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lad Prabhakar, Geert Uytterhoeven, Rob Herring,
Lars-Peter Clausen, Michael Turquette, Magnus Damm, Stephen Boyd,
Philipp Zabel, Alexandru Ardelean, linux-iio,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-clk, LKML, Linux-Renesas, Biju Das
Hi Jonathan,
Thank you for the review.
On Sat, Jul 24, 2021 at 7:03 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 19 Jul 2021 09:58:38 +0100
> Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> > Add ADC driver support for Renesas RZ/G2L A/D converter in SW
> > trigger mode.
> >
> > A/D Converter block is a successive approximation analog-to-digital
> > converter with a 12-bit accuracy and supports a maximum of 8 input
> > channels.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> Hi Lad,
>
> A few additional comments inline.
>
> Thanks,
>
> Jonathan
>
> > ---
> > MAINTAINERS | 8 +
> > drivers/iio/adc/Kconfig | 10 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/rzg2l_adc.c | 545 ++++++++++++++++++++++++++++++++++++
> > 4 files changed, 564 insertions(+)
> > create mode 100644 drivers/iio/adc/rzg2l_adc.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6c8be735cc91..6a52f9f4604c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15839,6 +15839,14 @@ L: linux-renesas-soc@vger.kernel.org
> > S: Maintained
> > F: drivers/phy/renesas/phy-rcar-gen3-usb*.c
> >
> > +RENESAS RZ/G2L A/D DRIVER
> > +M: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > +L: linux-iio@vger.kernel.org
> > +L: linux-renesas-soc@vger.kernel.org
> > +S: Supported
> > +F: Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
> > +F: drivers/iio/adc/rzg2l_adc.c
> > +
> > RESET CONTROLLER FRAMEWORK
> > M: Philipp Zabel <p.zabel@pengutronix.de>
> > S: Maintained
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index db0c8fb60515..af168e1c9fdb 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -887,6 +887,16 @@ config ROCKCHIP_SARADC
> > To compile this driver as a module, choose M here: the
> > module will be called rockchip_saradc.
> >
> > +config RZG2L_ADC
> > + tristate "Renesas RZ/G2L ADC driver"
> > + depends on ARCH_R9A07G044 || COMPILE_TEST
> > + help
> > + Say yes here to build support for the ADC found in Renesas
> > + RZ/G2L family.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called rzg2l_adc.
> > +
> > config SC27XX_ADC
> > tristate "Spreadtrum SC27xx series PMICs ADC"
> > depends on MFD_SC27XX_PMIC || COMPILE_TEST
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index f70d877c555a..d68550f493e3 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -82,6 +82,7 @@ obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
> > obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
> > obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
> > obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> > +obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
> > obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
> > obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
> > obj-$(CONFIG_STX104) += stx104.o
> > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > new file mode 100644
> > index 000000000000..033cdec9e976
> > --- /dev/null
> > +++ b/drivers/iio/adc/rzg2l_adc.c
> > @@ -0,0 +1,545 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * RZ/G2L A/D Converter driver
> > + *
> > + * Copyright (c) 2021 Renesas Electronics Europe GmbH
> > + *
> > + * Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
>
> Add mod_devicetable.h and drop the of specific headers
> as nothing in them used directly as far as I can tell.
>
OK.
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > +
> > +#define DRIVER_NAME "rzg2l-adc"
> > +
> > +#define RZG2L_ADM(n) ((n) * 0x4)
> > +#define RZG2L_ADM0_ADCE BIT(0)
> > +#define RZG2L_ADM0_ADBSY BIT(1)
> > +#define RZG2L_ADM0_PWDWNB BIT(2)
> > +#define RZG2L_ADM0_SRESB BIT(15)
> > +#define RZG2L_ADM1_TRG BIT(0)
> > +#define RZG2L_ADM1_MS BIT(2)
> > +#define RZG2L_ADM1_BS BIT(4)
> > +#define RZG2L_ADM1_EGA_CLEAR ~GENMASK(13, 12)
> > +#define RZG2L_ADM2_CHSEL_CLEAR ~GENMASK(7, 0)
>
> I'd rather see these defined as _MASK and the
> inversion made explicit inline.
>
OK will use them as _MASK's and invert inline when required.
> > +#define RZG2L_ADM3_ADSMP 0x578
> > +#define RZG2L_ADM3_ADCMP (0xe << 16)
> > +#define RZG2L_ADM3_ADIL_CLEAR ~GENMASK(31, 24)
> > +
> > +#define RZG2L_ADINT 0x20
> > +#define RZG2L_ADINT_CH_CLEAR ~GENMASK(7, 0)
> > +#define RZG2L_ADINT_CSEEN BIT(16)
> > +#define RZG2L_ADINT_INTS BIT(31)
> > +#define RZG2L_ADSTS 0x24
> > +#define RZG2L_ADINT_INTST_MASK GENMASK(7, 0)
> > +#define RZG2L_ADSTS_CSEST BIT(16)
> > +#define RZG2L_ADIVC 0x28
> > +#define RZG2L_ADIVC_DIVADC_CLEAR ~GENMASK(8, 0)
> > +#define RZG2L_ADIVC_DIVADC_4 0x4
>
> Perhaps adopt the _MASK and FIELD_PREP() approach to setting
> particular values. That way it is obvious at the call site that
> DVADC_4 is a value within the DIVADC_MASK covered field.
>
Agreed will change that.
> > +#define RZG2L_ADFIL 0x2c
> > +#define RZG2L_ADCR(n) (0x30 + ((n) * 0x4))
> > +#define RZG2L_ADCR_AD_MASK GENMASK(11, 0)
> > +
> > +#define RZG2L_ADC_MAX_CHANNELS 8
> > +#define RZG2L_ADC_CHN_MASK 0x7
> > +#define RZG2L_ADC_TIMEOUT usecs_to_jiffies(1 * 4)
> > +
> > +struct rzg2l_adc_data {
> > + const struct iio_chan_spec *channels;
> > + u8 num_channels;
> > +};
> > +
> > +struct rzg2l_adc {
> > + void __iomem *base;
> > + struct clk *pclk;
> > + struct clk *adclk;
> > + struct reset_control *presetn;
> > + struct reset_control *adrstn;
> > + struct completion completion;
> > + const struct rzg2l_adc_data *data;
> > + u16 last_val[RZG2L_ADC_MAX_CHANNELS];
> > +};
> > +
> > +static const char * const rzg2l_adc_channel_name[] = {
> > + "adc0",
> Is it useful to expose these as labels to userspace?
> Seems unnecessary given they map directly to the channel
> numbers.
>
Exposing them as this may vary depending on the wiring on the board,
so it would be better for user space to know which channels are
available.
> > + "adc1",
> > + "adc2",
> > + "adc3",
> > + "adc4",
> > + "adc5",
> > + "adc6",
> > + "adc7",
> > +};
> > +
> > +static unsigned int rzg2l_adc_readl(struct rzg2l_adc *adc, u32 reg)
> > +{
> > + return readl(adc->base + reg);
> > +}
> > +
> > +static void rzg2l_adc_writel(struct rzg2l_adc *adc, unsigned int reg, u32 val)
> > +{
> > + writel(val, adc->base + reg);
> > +}
> > +
> > +static void rzg2l_adc_pwr(struct rzg2l_adc *adc, bool on)
> > +{
> > + u32 reg;
> > +
> > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
> > + if (on)
> > + reg |= RZG2L_ADM0_PWDWNB;
> > + else
> > + reg &= ~RZG2L_ADM0_PWDWNB;
> > + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg);
> > + udelay(2);
> > +}
> > +
> > +static void rzg2l_adc_start_stop(struct rzg2l_adc *adc, bool start)
> > +{
> > + int timeout = 5;
> > + u32 reg;
> > +
> > + /* stop A/D conversion */
> > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
> > + if (start)
> > + reg |= RZG2L_ADM0_ADCE;
> > + else
> > + reg &= ~RZG2L_ADM0_ADCE;
> > + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg);
> > +
> > + if (start)
> > + return;
> > +
> > + do {
> > + usleep_range(100, 200);
> > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
> > + timeout--;
> > + if (!timeout) {
> > + pr_err("%s stopping ADC timed out\n", __func__);
> > + break;
> > + }
> > + } while (((reg & RZG2L_ADM0_ADBSY) || (reg & RZG2L_ADM0_ADCE)));
> > +}
> > +
> > +static void rzg2l_set_trigger(struct rzg2l_adc *adc)
> > +{
> > + u32 reg;
> > +
> > + /* SW trigger */
> > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(1));
> > + reg &= RZG2L_ADM1_EGA_CLEAR;
> > + reg &= ~RZG2L_ADM1_BS;
> > + reg |= RZG2L_ADM1_MS;
> > + reg &= ~RZG2L_ADM1_TRG;
> > + rzg2l_adc_writel(adc, RZG2L_ADM(1), reg);
> > +}
> > +
> > +static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
> > +{
> > + u32 reg;
> > +
> > + if (rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_ADBSY)
> > + return -EBUSY;
> > +
> > + rzg2l_set_trigger(adc);
> > +
> > + /* select channel */
> > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(2));
> > + reg &= RZG2L_ADM2_CHSEL_CLEAR;
> > + reg |= BIT(ch);
> > + rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
> > +
> > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
> > + reg &= RZG2L_ADM3_ADIL_CLEAR;
> > + reg |= RZG2L_ADM3_ADCMP;
> > + reg |= RZG2L_ADM3_ADSMP;
>
> No loss in readability in combining the two lines above and shorter
> code is always good. Having the mask on a separate line makes
> sense but the |= pair doesn't.
>
Agreed will move to the same line.
> What is a bit unusual here is you clear some bits then write different
> ones. That is presumably relying on the fact that the ADCCMP part
> is always set to the same value. That seems unwise to assume from
> a long term maintainability point of view.
>
The ADIL bits have to be set to zero, so I am clearing the ADIL bits
(bit 24-bits 31) and the ADCMP/ADSMP should be set to specific values
oxe/0x578 respectively.
> > + rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
> > +
> > + reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
> > + reg &= RZG2L_ADIVC_DIVADC_CLEAR;
> > + reg |= RZG2L_ADIVC_DIVADC_4;
> > + rzg2l_adc_writel(adc, RZG2L_ADIVC, reg);
> > +
> > + reg = rzg2l_adc_readl(adc, RZG2L_ADINT);
> > + reg &= ~RZG2L_ADINT_INTS;
> > + reg &= RZG2L_ADINT_CH_CLEAR;
> > + reg |= RZG2L_ADINT_CSEEN;
> > + reg |= BIT(ch);
> > + rzg2l_adc_writel(adc, RZG2L_ADINT, reg);
> > +
> > + return 0;
> > +}
> > +
> > +static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on)
> > +{
> > + struct device *dev = indio_dev->dev.parent;
> > +
> > + if (on)
> > + return pm_runtime_resume_and_get(dev);
> > +
> > + return pm_runtime_put_sync(dev);
> > +}
> > +
> > +static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
> > +{
> > + int ret;
> > +
> > + ret = rzg2l_adc_set_power(indio_dev, true);
> > + if (ret)
> > + return ret;
> > +
> > + ret = rzg2l_adc_conversion_setup(adc, ch);
> > + if (ret) {
> > + rzg2l_adc_set_power(indio_dev, false);
> > + return ret;
> > + }
> > +
> > + reinit_completion(&adc->completion);
> > +
> > + rzg2l_adc_start_stop(adc, true);
> > +
> > + if (!wait_for_completion_timeout(&adc->completion, RZG2L_ADC_TIMEOUT)) {
> > + rzg2l_adc_writel(adc, RZG2L_ADINT,
> > + rzg2l_adc_readl(adc, RZG2L_ADINT) & RZG2L_ADINT_CH_CLEAR);
> > + rzg2l_adc_start_stop(adc, false);
> > + rzg2l_adc_set_power(indio_dev, false);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return rzg2l_adc_set_power(indio_dev, false);
> > +}
> > +
> > +static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct rzg2l_adc *adc = iio_priv(indio_dev);
> > + int ret;
> > + u8 ch;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EINVAL;
> > +
> > + ret = iio_device_claim_direct_mode(indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + ch = chan->channel & RZG2L_ADC_CHN_MASK;
> > + ret = rzg2l_adc_conversion(indio_dev, adc, ch);
> > + if (ret) {
> > + iio_device_release_direct_mode(indio_dev);
> > + return ret;
> > + }
> > + *val = adc->last_val[ch];
> > + iio_device_release_direct_mode(indio_dev);
> > +
> > + return IIO_VAL_INT;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int rzg2l_adc_read_label(struct iio_dev *iio_dev,
> > + const struct iio_chan_spec *chan,
> > + char *label)
> > +{
> > + if (chan->channel >= RZG2L_ADC_MAX_CHANNELS)
> > + return -EINVAL;
> > +
> > + return snprintf(label, PAGE_SIZE, "%s\n", rzg2l_adc_channel_name[chan->channel]);
>
> sysfs_emit()?.
>
OK
> > +}
> > +
> > +static const struct iio_info rzg2l_adc_iio_info = {
> > + .read_raw = rzg2l_adc_read_raw,
> > + .read_label = rzg2l_adc_read_label,
> > +};
> > +
> > +static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
> > +{
> > + struct rzg2l_adc *adc = (struct rzg2l_adc *)dev_id;
> > + u8 intst;
> > + u32 reg;
> > + u8 i;
> > +
> > + reg = rzg2l_adc_readl(adc, RZG2L_ADSTS);
> > + if (reg & RZG2L_ADSTS_CSEST) {
>
> Probably good to add a brief comment on what is going on in this check.
> Perhaps even an error print if it's an error path.
>
Agreed will add a comment, this condition happens on channel selection error.
> > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + intst = reg & RZG2L_ADINT_INTST_MASK;
> > + if (!intst)
>
> I'm guessing this means spurious interrupt in which case IRQ_NONE
> so the core irq code knows about it.
>
Agreed IRQ_NONE should be returned.
> > + return IRQ_HANDLED;
> > +
> > + for (i = 0; i < RZG2L_ADC_MAX_CHANNELS; i++) {
> > + if (intst & BIT(i))
>
> for_each_bit_set() and make intst a long so the types are right for
> that macro
>
>
Agreed will do.
> > + adc->last_val[i] = rzg2l_adc_readl(adc, RZG2L_ADCR(i)) & RZG2L_ADCR_AD_MASK;
> > + }
> > +
> > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg);
> > +
> > + complete(&adc->completion);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int rzg2l_adc_parse_of(struct platform_device *pdev, struct rzg2l_adc *adc)
> > +{
> > + struct iio_chan_spec *chan_array;
> > + struct fwnode_handle *fwnode;
> > + struct rzg2l_adc_data *data;
> > + unsigned int channel;
> > + int num_channels;
> > + int ret;
> > + u8 i;
> > +
> > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + num_channels = device_get_child_node_count(&pdev->dev);
> > + if (!num_channels) {
> > + dev_err(&pdev->dev, "no channel children\n");
> > + return -ENODEV;
> > + }
> > +
> > + if (num_channels > RZG2L_ADC_MAX_CHANNELS) {
> > + dev_err(&pdev->dev, "num of channel children out of range\n");
> > + return -EINVAL;
> > + }
> > +
> > + chan_array = devm_kcalloc(&pdev->dev, num_channels, sizeof(*chan_array),
> > + GFP_KERNEL);
> > + if (!chan_array)
> > + return -ENOMEM;
> > +
> > + i = 0;
> > + device_for_each_child_node(&pdev->dev, fwnode) {
> > + ret = fwnode_property_read_u32(fwnode, "reg", &channel);
> > + if (ret)
> > + return ret;
> > +
> > + if (channel >= RZG2L_ADC_MAX_CHANNELS)
> > + return -EINVAL;
> > +
> > + chan_array[i].type = IIO_VOLTAGE;
> > + chan_array[i].indexed = 1;
> > + chan_array[i].channel = channel;
> > + chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > + chan_array[i].datasheet_name = rzg2l_adc_channel_name[channel];
> > + i++;
> > + }
> > +
> > + data->num_channels = num_channels;
> > + data->channels = chan_array;
> > + adc->data = data;
> > +
> > + return 0;
> > +}
> > +
> > +static int rzg2l_adc_sw_reset(struct rzg2l_adc *adc)
> > +{
> > + int timeout = 5;
> > + u32 val;
> > +
> > + val = rzg2l_adc_readl(adc, RZG2L_ADM(0));
> > + val |= RZG2L_ADM0_SRESB;
> > + rzg2l_adc_writel(adc, RZG2L_ADM(0), val);
> > +
> > + while (!(rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_SRESB)) {
> > + if (!timeout)
> > + return -EBUSY;
> > + timeout--;
> > + usleep_range(100, 200);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rzg2l_adc_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct iio_dev *indio_dev;
> > + struct rzg2l_adc *adc;
> > + int ret;
> > + int irq;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*adc)
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + adc = iio_priv(indio_dev);
> > +
> > + ret = rzg2l_adc_parse_of(pdev, adc);
>
> *_parse_properties() or *_parse_firmware()
> as it's not of specific now.
>
OK will rename it to rzg2l_adc_parse_properties().
> > + if (ret)
> > + return ret;
> > +
> > + adc->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(adc->base))
> > + return PTR_ERR(adc->base);
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(dev, "no irq resource\n");
> > + return irq;
> > + }
> > +
> > + adc->pclk = devm_clk_get(dev, "pclk");
> > + if (IS_ERR(adc->pclk)) {
> > + dev_err(dev, "Failed to get pclk");
> > + return PTR_ERR(adc->pclk);
> > + }
> > +
> > + adc->adclk = devm_clk_get(dev, "adclk");
> > + if (IS_ERR(adc->adclk)) {
> > + dev_err(dev, "Failed to get adclk");
> > + return PTR_ERR(adc->adclk);
> > + }
> > +
> > + adc->adrstn = devm_reset_control_get_exclusive(dev, "adrst-n");
> > + if (IS_ERR(adc->adrstn)) {
> > + dev_err(dev, "failed to get adrstn\n");
> > + return PTR_ERR(adc->adrstn);
> > + }
> > +
> > + adc->presetn = devm_reset_control_get_exclusive(dev, "presetn");
> > + if (IS_ERR(adc->presetn)) {
> > + dev_err(dev, "failed to get presetn\n");
> > + return PTR_ERR(adc->presetn);
> > + }
> > +
> > + ret = reset_control_deassert(adc->adrstn);
> > + if (ret)
> > + return ret;
>
> devm_add_action_or_reset() here to make the reasserting of these part
> of the automated cleanup.
>
OK will use devm_add_action_or_reset().
> > +
> > + ret = reset_control_deassert(adc->presetn);
> > + if (ret)
> > + goto assert_adrstn;
> > +
> > + ret = clk_prepare_enable(adc->pclk);
> > + if (ret)
> > + goto assert_presetn;
> > +
> > + ret = rzg2l_adc_sw_reset(adc);
> > + if (ret)
> > + goto unprepare_pclk;
> > +
> > + clk_disable_unprepare(adc->pclk);
>
> As below, reorder as:
>
> ret = rzg2l_adc_sw_reset(adc);
> clk_disable_unprepare(adc->pclk);
> if (ret)
> goto assert_preset_n;
>
Agreed.
> > +
> > + init_completion(&adc->completion);
> > +
> > + platform_set_drvdata(pdev, indio_dev);
> > +
> > + ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
> > + 0, dev_name(dev), adc);
>
> Another aspect of the comment below about mixing and matching between
> devm and not. If the order of remove is not that opposite of probe
> then there should be a comment saying why. There are far too many
> race conditions introduced by people doing things in a different order
> without carefully considering why.
>
Agreed will completely move to devm
> > + if (ret < 0)
> > + goto assert_presetn;
> > +
> > + indio_dev->name = DRIVER_NAME;
> > + indio_dev->info = &rzg2l_adc_iio_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->channels = adc->data->channels;
> > + indio_dev->num_channels = adc->data->num_channels;
> > +
> > + pm_runtime_enable(dev);
> > +
> > + ret = devm_iio_device_register(dev, indio_dev);
> > + if (ret)
> > + goto err_iio_device_register;
> > +
> > + return 0;
> > +
> > +unprepare_pclk:
> > + clk_disable_unprepare(adc->pclk);
>
> Bad sign that you have this here and not in remove..
> When you have cases like this, it's normally better to
> either factor out the relevant code into a function
> that lets you do this call whether or not there is an
> error, or to just move this up to the relevant if statements.
>
As agreed above, will alter the code.
>
> > +err_iio_device_register:
> > + pm_runtime_disable(dev);
> > +assert_presetn:
> > + reset_control_assert(adc->presetn);
> > +assert_adrstn:
> > + reset_control_assert(adc->adrstn);
> > + return ret;
> > +}
> > +
> > +static int rzg2l_adc_remove(struct platform_device *pdev)
> > +{
> > + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > + struct rzg2l_adc *adc = iio_priv(indio_dev);
> > +
> > + reset_control_assert(adc->presetn);
> > + reset_control_assert(adc->adrstn);
> > + pm_runtime_disable(indio_dev->dev.parent);
>
> You can't mix and match devm managed and unmanaged like this.
> The userspace interfce is still exposed after you've put the
> device into reset which is unlikely to be a good idea.
>
> One option is to use devm_add_action_or_reset() to make
> this stuff happen in the managed flow, which happens after
> remove is done. That would let you drop remove() entirely.
>
Agreed will use devm_add_action_or_reset() and get rid of remove callback().
Cheers,
Prabhakar
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id rzg2l_adc_match[] = {
> > + { .compatible = "renesas,rzg2l-adc",},
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rzg2l_adc_match);
> > +
> > +static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct rzg2l_adc *adc = iio_priv(indio_dev);
> > +
> > + rzg2l_adc_pwr(adc, false);
> > + clk_disable_unprepare(adc->adclk);
> > + clk_disable_unprepare(adc->pclk);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct rzg2l_adc *adc = iio_priv(indio_dev);
> > + int ret;
> > +
> > + ret = clk_prepare_enable(adc->pclk);
> > + if (ret)
> > + return ret;
> > +
> > + ret = clk_prepare_enable(adc->adclk);
> > + if (ret)
> > + return ret;
> > +
> > + rzg2l_adc_pwr(adc, true);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops rzg2l_adc_pm_ops = {
> > + SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend,
> > + rzg2l_adc_pm_runtime_resume,
> > + NULL)
> > +};
> > +
> > +static struct platform_driver rzg2l_adc_driver = {
> > + .probe = rzg2l_adc_probe,
> > + .remove = rzg2l_adc_remove,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .of_match_table = rzg2l_adc_match,
> > + .pm = &rzg2l_adc_pm_ops,
> > + },
> > +};
> > +
> > +module_platform_driver(rzg2l_adc_driver);
> > +
> > +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L ADC driver");
> > +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] iio: adc: Add driver for Renesas RZ/G2L A/D converter
2021-07-25 12:18 ` Lad, Prabhakar
@ 2021-07-25 14:55 ` Jonathan Cameron
0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2021-07-25 14:55 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Lad Prabhakar, Geert Uytterhoeven, Rob Herring,
Lars-Peter Clausen, Michael Turquette, Magnus Damm, Stephen Boyd,
Philipp Zabel, Alexandru Ardelean, linux-iio,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-clk, LKML, Linux-Renesas, Biju Das
On Sun, 25 Jul 2021 13:18:53 +0100
"Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> Hi Jonathan,
>
> Thank you for the review.
>
> On Sat, Jul 24, 2021 at 7:03 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 19 Jul 2021 09:58:38 +0100
> > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> >
> > > Add ADC driver support for Renesas RZ/G2L A/D converter in SW
> > > trigger mode.
> > >
> > > A/D Converter block is a successive approximation analog-to-digital
> > > converter with a 12-bit accuracy and supports a maximum of 8 input
> > > channels.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > Hi Lad,
> >
> > A few additional comments inline.
> >
> > Thanks,
> >
> > Jonathan
> >
...
>
> > > +#define RZG2L_ADFIL 0x2c
> > > +#define RZG2L_ADCR(n) (0x30 + ((n) * 0x4))
> > > +#define RZG2L_ADCR_AD_MASK GENMASK(11, 0)
> > > +
> > > +#define RZG2L_ADC_MAX_CHANNELS 8
> > > +#define RZG2L_ADC_CHN_MASK 0x7
> > > +#define RZG2L_ADC_TIMEOUT usecs_to_jiffies(1 * 4)
> > > +
> > > +struct rzg2l_adc_data {
> > > + const struct iio_chan_spec *channels;
> > > + u8 num_channels;
> > > +};
> > > +
> > > +struct rzg2l_adc {
> > > + void __iomem *base;
> > > + struct clk *pclk;
> > > + struct clk *adclk;
> > > + struct reset_control *presetn;
> > > + struct reset_control *adrstn;
> > > + struct completion completion;
> > > + const struct rzg2l_adc_data *data;
> > > + u16 last_val[RZG2L_ADC_MAX_CHANNELS];
> > > +};
> > > +
> > > +static const char * const rzg2l_adc_channel_name[] = {
> > > + "adc0",
> > Is it useful to expose these as labels to userspace?
> > Seems unnecessary given they map directly to the channel
> > numbers.
> >
> Exposing them as this may vary depending on the wiring on the board,
> so it would be better for user space to know which channels are
> available.
Hmm. One thing to take into account is the IIO ABI doesn't require
channel numbers to be consecutive. There are a few drivers where
they aren't due to channel optionality.
Perhaps that would make sense here? If not, I'm fine with leaving
these as you have it. They do no harm.
>
> > > + "adc1",
> > > + "adc2",
> > > + "adc3",
> > > + "adc4",
> > > + "adc5",
> > > + "adc6",
> > > + "adc7",
> > > +};
> > > +
...
> > > +static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
> > > +{
> > > + u32 reg;
> > > +
> > > + if (rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_ADBSY)
> > > + return -EBUSY;
> > > +
> > > + rzg2l_set_trigger(adc);
> > > +
> > > + /* select channel */
> > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(2));
> > > + reg &= RZG2L_ADM2_CHSEL_CLEAR;
> > > + reg |= BIT(ch);
> > > + rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
> > > +
> > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
> > > + reg &= RZG2L_ADM3_ADIL_CLEAR;
> > > + reg |= RZG2L_ADM3_ADCMP;
> > > + reg |= RZG2L_ADM3_ADSMP;
> >
> > No loss in readability in combining the two lines above and shorter
> > code is always good. Having the mask on a separate line makes
> > sense but the |= pair doesn't.
> >
> Agreed will move to the same line.
>
> > What is a bit unusual here is you clear some bits then write different
> > ones. That is presumably relying on the fact that the ADCCMP part
> > is always set to the same value. That seems unwise to assume from
> > a long term maintainability point of view.
> >
> The ADIL bits have to be set to zero, so I am clearing the ADIL bits
> (bit 24-bits 31) and the ADCMP/ADSMP should be set to specific values
> oxe/0x578 respectively.
Understood, but from this 'local' bit of code it's not obvious that they don't
have other bits say, perhaps ADSMP is set to 0x483 for example by some
other code? (obviously it isn't, but it's nice to not have to sanity check
the rest of the driver to be sure!) So normal thing to do would be to also
mask those bits out.
I'm a bit curious on whether there are other bits in this register that make it
useful to do the read modify write cycle? (doesn't seem to be a public
datasheet from a quick google...)
>
> > > + rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
> > > +
> > > + reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
> > > + reg &= RZG2L_ADIVC_DIVADC_CLEAR;
> > > + reg |= RZG2L_ADIVC_DIVADC_4;
> > > + rzg2l_adc_writel(adc, RZG2L_ADIVC, reg);
> > > +
> > > + reg = rzg2l_adc_readl(adc, RZG2L_ADINT);
> > > + reg &= ~RZG2L_ADINT_INTS;
> > > + reg &= RZG2L_ADINT_CH_CLEAR;
> > > + reg |= RZG2L_ADINT_CSEEN;
> > > + reg |= BIT(ch);
> > > + rzg2l_adc_writel(adc, RZG2L_ADINT, reg);
> > > +
> > > + return 0;
> > > +}
> > > +
...
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-07-25 14:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-19 8:58 [PATCH v2 0/4] Renesas RZ/G2L ADC driver support Lad Prabhakar
2021-07-19 8:58 ` [PATCH v2 1/4] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar
2021-07-19 13:47 ` Rob Herring
2021-07-19 8:58 ` [PATCH v2 2/4] iio: adc: Add driver " Lad Prabhakar
2021-07-24 18:06 ` Jonathan Cameron
2021-07-25 12:18 ` Lad, Prabhakar
2021-07-25 14:55 ` Jonathan Cameron
2021-07-19 8:58 ` [PATCH v2 3/4] clk: renesas: r9a07g044-cpg: Add clock and reset entries for ADC Lad Prabhakar
2021-07-19 9:15 ` Geert Uytterhoeven
2021-07-19 8:58 ` [PATCH v2 4/4] arm64: dts: renesas: r9a07g044: Add ADC node Lad Prabhakar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.