* [PATCH v3 0/3] Renesas RZ/G2L ADC driver support
@ 2021-07-26 18:28 Lad Prabhakar
2021-07-26 18:28 ` [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Lad Prabhakar @ 2021-07-26 18:28 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring, Jonathan Cameron,
Lars-Peter Clausen, Magnus Damm, Philipp Zabel,
Alexandru Ardelean, linux-iio, devicetree, linux-renesas-soc
Cc: linux-kernel, Prabhakar, Biju Das, Lad Prabhakar
Hi All,
This patch series adds ADC support for Renesas RZ/G2L family.
Patches apply on top of v5.14-rc2.
Cheers,
Prabhakar
Changes for v3 (as requested by Jonathan):
* Made use of FIELD_PREP()
* Renamed _CLEAR to _MASK and inverted inline as required
* Moved |= pair's on same lines
* Made use of sysfs_emit() while reading the labels
* Used for_each_bit_set() in rzg2l_adc_isr()
* Renamed rzg2l_adc_parse_of() -> rzg2l_adc_parse_properties()
* Used devm_add_action_or_reset() for asserting the reset signals and
disabling pm_runtime and eventually removing remove() callback
* Added comments in isr handler for channel select interrupt
* Moved enabling/disabling of pclk during hw init in rzg2l_adc_hw_init()
* Dropped clock patch 3/4 (https://lore.kernel.org/patchwork/patch/1462152/)
from previous series as its queued up in renesas-clk-for-v5.15
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
v1: https://patchwork.kernel.org/project/linux-renesas-soc/cover/
20210629220328.13366-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
Lad Prabhakar (3):
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
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/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/rzg2l_adc.c | 595 ++++++++++++++++++
6 files changed, 790 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
create mode 100644 drivers/iio/adc/rzg2l_adc.c
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter 2021-07-26 18:28 [PATCH v3 0/3] Renesas RZ/G2L ADC driver support Lad Prabhakar @ 2021-07-26 18:28 ` Lad Prabhakar 2021-07-26 21:42 ` Rob Herring 2021-07-27 20:30 ` Rob Herring 2021-07-26 18:28 ` [PATCH v3 2/3] iio: adc: Add driver " Lad Prabhakar 2021-07-26 18:28 ` [PATCH v3 3/3] arm64: dts: renesas: r9a07g044: Add ADC node Lad Prabhakar 2 siblings, 2 replies; 15+ messages in thread From: Lad Prabhakar @ 2021-07-26 18:28 UTC (permalink / raw) To: Geert Uytterhoeven, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Magnus Damm, Philipp Zabel, Alexandru Ardelean, linux-iio, devicetree, linux-renesas-soc Cc: linux-kernel, 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] 15+ messages in thread
* Re: [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter 2021-07-26 18:28 ` [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar @ 2021-07-26 21:42 ` Rob Herring 2021-07-27 8:06 ` Lad, Prabhakar 2021-07-27 20:30 ` Rob Herring 1 sibling, 1 reply; 15+ messages in thread From: Rob Herring @ 2021-07-26 21:42 UTC (permalink / raw) To: Lad Prabhakar Cc: Geert Uytterhoeven, linux-iio, Rob Herring, Jonathan Cameron, devicetree, Alexandru Ardelean, Prabhakar, linux-kernel, Biju Das, linux-renesas-soc, Lars-Peter Clausen, Magnus Damm, Philipp Zabel On Mon, 26 Jul 2021 19:28:48 +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/1510163 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] 15+ messages in thread
* Re: [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter 2021-07-26 21:42 ` Rob Herring @ 2021-07-27 8:06 ` Lad, Prabhakar 0 siblings, 0 replies; 15+ messages in thread From: Lad, Prabhakar @ 2021-07-27 8:06 UTC (permalink / raw) To: Rob Herring Cc: Lad Prabhakar, Geert Uytterhoeven, linux-iio, Rob Herring, Jonathan Cameron, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Alexandru Ardelean, LKML, Biju Das, Linux-Renesas, Lars-Peter Clausen, Magnus Damm, Philipp Zabel Hi Rob, On Mon, Jul 26, 2021 at 10:42 PM Rob Herring <robh@kernel.org> wrote: > > On Mon, 26 Jul 2021 19:28:48 +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/1510163 > > This check can fail if there are any dependencies. The base for a patch > series is generally the most recent rc1. > The base patch series required for this path is v5.14-rc2. (I had mentioned it in the cover letter, maybe I should have added a note here too) Sorry for the inconvenience. Cheers, Prabhakar > 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] 15+ messages in thread
* Re: [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter 2021-07-26 18:28 ` [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar 2021-07-26 21:42 ` Rob Herring @ 2021-07-27 20:30 ` Rob Herring 1 sibling, 0 replies; 15+ messages in thread From: Rob Herring @ 2021-07-27 20:30 UTC (permalink / raw) To: Lad Prabhakar Cc: Geert Uytterhoeven, Jonathan Cameron, Lars-Peter Clausen, Magnus Damm, Philipp Zabel, Alexandru Ardelean, open list:IIO SUBSYSTEM AND DRIVERS, devicetree, open list:MEDIA DRIVERS FOR RENESAS - FCP, linux-kernel, Prabhakar, Biju Das On Mon, Jul 26, 2021 at 12:31 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> 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 Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter 2021-07-26 18:28 [PATCH v3 0/3] Renesas RZ/G2L ADC driver support Lad Prabhakar 2021-07-26 18:28 ` [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar @ 2021-07-26 18:28 ` Lad Prabhakar 2021-07-27 7:13 ` Philipp Zabel 2021-07-31 17:11 ` Jonathan Cameron 2021-07-26 18:28 ` [PATCH v3 3/3] arm64: dts: renesas: r9a07g044: Add ADC node Lad Prabhakar 2 siblings, 2 replies; 15+ messages in thread From: Lad Prabhakar @ 2021-07-26 18:28 UTC (permalink / raw) To: Geert Uytterhoeven, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Magnus Damm, Philipp Zabel, Alexandru Ardelean, linux-iio, devicetree, linux-renesas-soc Cc: linux-kernel, 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 | 595 ++++++++++++++++++++++++++++++++++++ 4 files changed, 614 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..d05a3208ff9d --- /dev/null +++ b/drivers/iio/adc/rzg2l_adc.c @@ -0,0 +1,595 @@ +// 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/mod_devicetable.h> +#include <linux/module.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_MASK GENMASK(13, 12) +#define RZG2L_ADM2_CHSEL_MASK GENMASK(7, 0) +#define RZG2L_ADM3_ADIL_MASK GENMASK(31, 24) +#define RZG2L_ADM3_ADCMP_MASK GENMASK(23, 16) +#define RZG2L_ADM3_ADCMP_E FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe) +#define RZG2L_ADM3_ADSMP_MASK GENMASK(15, 0) + +#define RZG2L_ADINT 0x20 +#define RZG2L_ADINT_INTEN_MASK GENMASK(7, 0) +#define RZG2L_ADINT_CSEEN BIT(16) +#define RZG2L_ADINT_INTS BIT(31) + +#define RZG2L_ADSTS 0x24 +#define RZG2L_ADSTS_CSEST BIT(16) +#define RZG2L_ADSTS_INTST_MASK GENMASK(7, 0) + +#define RZG2L_ADIVC 0x28 +#define RZG2L_ADIVC_DIVADC_MASK GENMASK(8, 0) +#define RZG2L_ADIVC_DIVADC_4 FIELD_PREP(RZG2L_ADIVC_DIVADC_MASK, 0x4) + +#define RZG2L_ADFIL 0x2c + +#define RZG2L_ADCR(n) (0x30 + ((n) * 0x4)) +#define RZG2L_ADCR_AD_MASK GENMASK(11, 0) + +#define RZG2L_ADSMP_DEFUALT_SAMPLING 0x578 + +#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; + + /* + * Setup ADM1 for SW trigger + * EGA[13:12] - Set 00 to indicate hardware trigger is invalid + * BS[4] - Enable 1-buffer mode + * MS[1] - Enable Select mode + * TRG[0] - Enable software trigger mode + */ + reg = rzg2l_adc_readl(adc, RZG2L_ADM(1)); + reg &= ~RZG2L_ADM1_EGA_MASK; + reg &= ~RZG2L_ADM1_BS; + reg &= ~RZG2L_ADM1_TRG; + reg |= RZG2L_ADM1_MS; + 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 analog input channel subjected to conversion. */ + reg = rzg2l_adc_readl(adc, RZG2L_ADM(2)); + reg &= ~RZG2L_ADM2_CHSEL_MASK; + reg |= BIT(ch); + rzg2l_adc_writel(adc, RZG2L_ADM(2), reg); + + /* + * Setup ADINT + * INTS[31] - Select pulse signal + * CSEEN[16] - Enable channel select error interrupt + * INTEN[7:0] - Select channel interrupt + */ + reg = rzg2l_adc_readl(adc, RZG2L_ADINT); + reg &= ~RZG2L_ADINT_INTS; + reg &= ~RZG2L_ADINT_INTEN_MASK; + reg |= (RZG2L_ADINT_CSEEN | 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_INTEN_MASK); + 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 sysfs_emit(label, "%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; + unsigned long intst; + u32 reg; + int ch; + + reg = rzg2l_adc_readl(adc, RZG2L_ADSTS); + + /* A/D conversion channel select error interrupt */ + if (reg & RZG2L_ADSTS_CSEST) { + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); + return IRQ_HANDLED; + } + + intst = reg & RZG2L_ADSTS_INTST_MASK; + if (!intst) + return IRQ_NONE; + + for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS) { + if (intst & BIT(ch)) + adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) & + RZG2L_ADCR_AD_MASK; + } + + /* clear the channel interrupt */ + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); + + complete(&adc->completion); + + return IRQ_HANDLED; +} + +static int rzg2l_adc_parse_properties(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_hw_init(struct rzg2l_adc *adc) +{ + int timeout = 5; + u32 reg; + int ret; + + ret = clk_prepare_enable(adc->pclk); + if (ret) + return ret; + + /* SW reset */ + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0)); + reg |= RZG2L_ADM0_SRESB; + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg); + + while (!(rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_SRESB)) { + if (!timeout) { + ret = -EBUSY; + goto exit_hw_init; + } + timeout--; + usleep_range(100, 200); + } + + /* Only division by 4 can be set */ + reg = rzg2l_adc_readl(adc, RZG2L_ADIVC); + reg &= ~RZG2L_ADIVC_DIVADC_MASK; + reg |= RZG2L_ADIVC_DIVADC_4; + rzg2l_adc_writel(adc, RZG2L_ADIVC, reg); + + /* + * Setup AMD3 + * ADIL[31:24] - Should be always set to 0 + * ADCMP[23:16] - Should be always set to 0xe + * ADSMP[15:0] - Set default (0x578) sampling period + */ + reg = rzg2l_adc_readl(adc, RZG2L_ADM(3)); + reg &= ~RZG2L_ADM3_ADIL_MASK; + reg &= ~RZG2L_ADM3_ADCMP_MASK; + reg &= ~RZG2L_ADM3_ADSMP_MASK; + reg |= (RZG2L_ADM3_ADCMP_E | RZG2L_ADSMP_DEFUALT_SAMPLING); + rzg2l_adc_writel(adc, RZG2L_ADM(3), reg); + +exit_hw_init: + clk_disable_unprepare(adc->pclk); + + return 0; +} + +static void rzg2l_adc_pm_runtime_disable(void *data) +{ + struct iio_dev *indio_dev = data; + + pm_runtime_disable(indio_dev->dev.parent); +} + +static void rzg2l_adc_reset_assert(void *data) +{ + struct reset_control *reset = data; + + reset_control_assert(reset); +} + +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_properties(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); + } + + ret = devm_add_action_or_reset(&pdev->dev, + rzg2l_adc_reset_assert, adc->adrstn); + if (ret) { + dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n", + ret); + return ret; + } + + 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 = devm_add_action_or_reset(&pdev->dev, + rzg2l_adc_reset_assert, adc->presetn); + if (ret) { + dev_err(&pdev->dev, "failed to register presetn assert devm action, %d\n", + ret); + return ret; + } + + ret = reset_control_deassert(adc->adrstn); + if (ret) { + dev_err(&pdev->dev, "failed to deassert adrstn pin, %d\n", ret); + return ret; + } + + ret = reset_control_deassert(adc->presetn); + if (ret) { + dev_err(&pdev->dev, "failed to deassert presetn pin, %d\n", ret); + return ret; + } + + ret = rzg2l_adc_hw_init(adc); + if (ret) { + dev_err(&pdev->dev, "failed to initialize ADC HW, %d\n", ret); + return ret; + } + + ret = devm_request_irq(dev, irq, rzg2l_adc_isr, + 0, dev_name(dev), adc); + if (ret < 0) + return ret; + + init_completion(&adc->completion); + + platform_set_drvdata(pdev, indio_dev); + + 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_add_action_or_reset(&pdev->dev, + rzg2l_adc_pm_runtime_disable, indio_dev); + if (ret) { + dev_err(&pdev->dev, "failed to register pm_runtime_disable devm action, %d\n", + ret); + return ret; + } + + return devm_iio_device_register(dev, indio_dev); +} + +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, + .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] 15+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter 2021-07-26 18:28 ` [PATCH v3 2/3] iio: adc: Add driver " Lad Prabhakar @ 2021-07-27 7:13 ` Philipp Zabel 2021-07-27 8:02 ` Lad, Prabhakar 2021-07-31 17:11 ` Jonathan Cameron 1 sibling, 1 reply; 15+ messages in thread From: Philipp Zabel @ 2021-07-27 7:13 UTC (permalink / raw) To: Lad Prabhakar, Geert Uytterhoeven, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Magnus Damm, Alexandru Ardelean, linux-iio, devicetree, linux-renesas-soc Cc: linux-kernel, Prabhakar, Biju Das Hi Prabhakar, On Mon, 2021-07-26 at 19:28 +0100, Lad Prabhakar 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> > --- > MAINTAINERS | 8 + > drivers/iio/adc/Kconfig | 10 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/rzg2l_adc.c | 595 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 614 insertions(+) > create mode 100644 drivers/iio/adc/rzg2l_adc.c > [...] > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c > new file mode 100644 > index 000000000000..d05a3208ff9d > --- /dev/null > +++ b/drivers/iio/adc/rzg2l_adc.c > @@ -0,0 +1,595 @@ [...] > +static void rzg2l_adc_pm_runtime_disable(void *data) > +{ > + struct iio_dev *indio_dev = data; > + > + pm_runtime_disable(indio_dev->dev.parent); > +} > + > +static void rzg2l_adc_reset_assert(void *data) > +{ > + struct reset_control *reset = data; > + > + reset_control_assert(reset); > +} > + > +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_properties(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); > + } I'd request the "presetn" control up here, so if that fails we don't touch the "adrst-n" reset line. > + ret = devm_add_action_or_reset(&pdev->dev, > + rzg2l_adc_reset_assert, adc->adrstn); > + if (ret) { > + dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n", > + ret); > + return ret; > + } This is the wrong way around. Installing devres actions should be done after the thing they are supposed to revert in case of error. You should move this down below the reset_control_deassert(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 = devm_add_action_or_reset(&pdev->dev, > + rzg2l_adc_reset_assert, adc->presetn); > + if (ret) { > + dev_err(&pdev->dev, "failed to register presetn assert devm action, %d\n", > + ret); > + return ret; > + } Same as above, this belongs after the presetn deassert below. > + > + ret = reset_control_deassert(adc->adrstn); > + if (ret) { > + dev_err(&pdev->dev, "failed to deassert adrstn pin, %d\n", ret); > + return ret; > + } Here is the place to install the adrstn assert action. > + ret = reset_control_deassert(adc->presetn); > + if (ret) { > + dev_err(&pdev->dev, "failed to deassert presetn pin, %d\n", ret); > + return ret; > + } And here is the place to install the presetn assert action. regards Philipp ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter 2021-07-27 7:13 ` Philipp Zabel @ 2021-07-27 8:02 ` Lad, Prabhakar 2021-07-27 8:13 ` Philipp Zabel 0 siblings, 1 reply; 15+ messages in thread From: Lad, Prabhakar @ 2021-07-27 8:02 UTC (permalink / raw) To: Philipp Zabel Cc: Lad Prabhakar, Geert Uytterhoeven, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Magnus Damm, Alexandru Ardelean, linux-iio, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, LKML, Biju Das Hi Philipp, Thank you for the review. On Tue, Jul 27, 2021 at 8:13 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Prabhakar, > > On Mon, 2021-07-26 at 19:28 +0100, Lad Prabhakar 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> > > --- > > MAINTAINERS | 8 + > > drivers/iio/adc/Kconfig | 10 + > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/rzg2l_adc.c | 595 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 614 insertions(+) > > create mode 100644 drivers/iio/adc/rzg2l_adc.c > > > [...] > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c > > new file mode 100644 > > index 000000000000..d05a3208ff9d > > --- /dev/null > > +++ b/drivers/iio/adc/rzg2l_adc.c > > @@ -0,0 +1,595 @@ > [...] > > +static void rzg2l_adc_pm_runtime_disable(void *data) > > +{ > > + struct iio_dev *indio_dev = data; > > + > > + pm_runtime_disable(indio_dev->dev.parent); > > +} > > + > > +static void rzg2l_adc_reset_assert(void *data) > > +{ > > + struct reset_control *reset = data; > > + > > + reset_control_assert(reset); > > +} > > + > > +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_properties(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); > > + } > > I'd request the "presetn" control up here, so if that fails we don't > touch the "adrst-n" reset line. > Ok will move the devm_reset_control_get_exclusive() call for presetn here. > > + ret = devm_add_action_or_reset(&pdev->dev, > > + rzg2l_adc_reset_assert, adc->adrstn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n", > > + ret); > > + return ret; > > + } > > This is the wrong way around. Installing devres actions should be done > after the thing they are supposed to revert in case of error. You should > move this down below the reset_control_deassert(adc->adrstn). > Ouch my understanding was, there won't be any harm in asserting the reset line. Agree with will move this below reset_control_deassert(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 = devm_add_action_or_reset(&pdev->dev, > > + rzg2l_adc_reset_assert, adc->presetn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register presetn assert devm action, %d\n", > > + ret); > > + return ret; > > + } > > Same as above, this belongs after the presetn deassert below. > Agreed. > > + > > + ret = reset_control_deassert(adc->adrstn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to deassert adrstn pin, %d\n", ret); > > + return ret; > > + } > > Here is the place to install the adrstn assert action. > Agreed will move the devres action here. > > + ret = reset_control_deassert(adc->presetn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to deassert presetn pin, %d\n", ret); > > + return ret; > > + } > > And here is the place to install the presetn assert action. > Agreed will move the devres action here. Cheers, Prabhakar > regards > Philipp ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter 2021-07-27 8:02 ` Lad, Prabhakar @ 2021-07-27 8:13 ` Philipp Zabel 2021-07-27 8:54 ` Lad, Prabhakar 0 siblings, 1 reply; 15+ messages in thread From: Philipp Zabel @ 2021-07-27 8:13 UTC (permalink / raw) To: Lad, Prabhakar Cc: Lad Prabhakar, Geert Uytterhoeven, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Magnus Damm, Alexandru Ardelean, linux-iio, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, LKML, Biju Das On Tue, 2021-07-27 at 09:02 +0100, Lad, Prabhakar wrote: [...] > > > + ret = devm_add_action_or_reset(&pdev->dev, > > > + rzg2l_adc_reset_assert, adc->adrstn); > > > + if (ret) { > > > + dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n", > > > + ret); > > > + return ret; > > > + } > > > > This is the wrong way around. Installing devres actions should be done > > after the thing they are supposed to revert in case of error. You should > > move this down below the reset_control_deassert(adc->adrstn). > > > Ouch my understanding was, there won't be any harm in asserting the > reset line. Agree with will move this below > reset_control_deassert(adc->adrstn). You are probably right, but it's still better do it correctly. Just imagine one of the reset lines turns out to be shared later, or somebody else will look at this driver for inspiration. regards Philipp ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter 2021-07-27 8:13 ` Philipp Zabel @ 2021-07-27 8:54 ` Lad, Prabhakar 0 siblings, 0 replies; 15+ messages in thread From: Lad, Prabhakar @ 2021-07-27 8:54 UTC (permalink / raw) To: Philipp Zabel Cc: Lad Prabhakar, Geert Uytterhoeven, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Magnus Damm, Alexandru Ardelean, linux-iio, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, LKML, Biju Das Hi Philipp, On Tue, Jul 27, 2021 at 9:13 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Tue, 2021-07-27 at 09:02 +0100, Lad, Prabhakar wrote: > [...] > > > > + ret = devm_add_action_or_reset(&pdev->dev, > > > > + rzg2l_adc_reset_assert, adc->adrstn); > > > > + if (ret) { > > > > + dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n", > > > > + ret); > > > > + return ret; > > > > + } > > > > > > This is the wrong way around. Installing devres actions should be done > > > after the thing they are supposed to revert in case of error. You should > > > move this down below the reset_control_deassert(adc->adrstn). > > > > > Ouch my understanding was, there won't be any harm in asserting the > > reset line. Agree with will move this below > > reset_control_deassert(adc->adrstn). > > You are probably right, but it's still better do it correctly. Just > imagine one of the reset lines turns out to be shared later, or somebody > else will look at this driver for inspiration. > Agreed, will do it the right way! Cheers, Prabhakar ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter 2021-07-26 18:28 ` [PATCH v3 2/3] iio: adc: Add driver " Lad Prabhakar 2021-07-27 7:13 ` Philipp Zabel @ 2021-07-31 17:11 ` Jonathan Cameron 2021-07-31 18:31 ` Lad, Prabhakar 1 sibling, 1 reply; 15+ messages in thread From: Jonathan Cameron @ 2021-07-31 17:11 UTC (permalink / raw) To: Lad Prabhakar Cc: Geert Uytterhoeven, Rob Herring, Lars-Peter Clausen, Magnus Damm, Philipp Zabel, Alexandru Ardelean, linux-iio, devicetree, linux-renesas-soc, linux-kernel, Prabhakar, Biju Das On Mon, 26 Jul 2021 19:28:49 +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 | 595 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 614 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..d05a3208ff9d > --- /dev/null > +++ b/drivers/iio/adc/rzg2l_adc.c > @@ -0,0 +1,595 @@ > +// 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/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/reset.h> > + > +#define DRIVER_NAME "rzg2l-adc" As only used in one place, just put it inline there so we don't need to go find if we want to know the value - I'm lazy. > + > +#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_MASK GENMASK(13, 12) > +#define RZG2L_ADM2_CHSEL_MASK GENMASK(7, 0) > +#define RZG2L_ADM3_ADIL_MASK GENMASK(31, 24) > +#define RZG2L_ADM3_ADCMP_MASK GENMASK(23, 16) > +#define RZG2L_ADM3_ADCMP_E FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe) > +#define RZG2L_ADM3_ADSMP_MASK GENMASK(15, 0) > + > +#define RZG2L_ADINT 0x20 > +#define RZG2L_ADINT_INTEN_MASK GENMASK(7, 0) > +#define RZG2L_ADINT_CSEEN BIT(16) > +#define RZG2L_ADINT_INTS BIT(31) > + > +#define RZG2L_ADSTS 0x24 > +#define RZG2L_ADSTS_CSEST BIT(16) > +#define RZG2L_ADSTS_INTST_MASK GENMASK(7, 0) > + > +#define RZG2L_ADIVC 0x28 > +#define RZG2L_ADIVC_DIVADC_MASK GENMASK(8, 0) > +#define RZG2L_ADIVC_DIVADC_4 FIELD_PREP(RZG2L_ADIVC_DIVADC_MASK, 0x4) > + > +#define RZG2L_ADFIL 0x2c > + > +#define RZG2L_ADCR(n) (0x30 + ((n) * 0x4)) > +#define RZG2L_ADCR_AD_MASK GENMASK(11, 0) > + > +#define RZG2L_ADSMP_DEFUALT_SAMPLING 0x578 > + > +#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 */ This comment seems odd. If it is stopping conversion, how is this function used to start conversion? Perhaps some more details. > + 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; > + > + /* > + * Setup ADM1 for SW trigger > + * EGA[13:12] - Set 00 to indicate hardware trigger is invalid > + * BS[4] - Enable 1-buffer mode > + * MS[1] - Enable Select mode > + * TRG[0] - Enable software trigger mode > + */ > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(1)); > + reg &= ~RZG2L_ADM1_EGA_MASK; > + reg &= ~RZG2L_ADM1_BS; > + reg &= ~RZG2L_ADM1_TRG; > + reg |= RZG2L_ADM1_MS; > + 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 analog input channel subjected to conversion. */ > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(2)); > + reg &= ~RZG2L_ADM2_CHSEL_MASK; > + reg |= BIT(ch); > + rzg2l_adc_writel(adc, RZG2L_ADM(2), reg); > + > + /* > + * Setup ADINT > + * INTS[31] - Select pulse signal > + * CSEEN[16] - Enable channel select error interrupt > + * INTEN[7:0] - Select channel interrupt > + */ > + reg = rzg2l_adc_readl(adc, RZG2L_ADINT); > + reg &= ~RZG2L_ADINT_INTS; > + reg &= ~RZG2L_ADINT_INTEN_MASK; > + reg |= (RZG2L_ADINT_CSEEN | 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_INTEN_MASK); > + 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]; You should not rely on implementation details of iio_device_claim_direct_mode() to protect your driver specific data structures. Use your own lock. Those calls are intended for safe operation when buffered mode in IIO is supported and currently your driver doesn't support that at all so please remove them. > + 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 sysfs_emit(label, "%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; No need for explicit cast from void * to another pointer type. This is always valid without in C. > + unsigned long intst; > + u32 reg; > + int ch; > + > + reg = rzg2l_adc_readl(adc, RZG2L_ADSTS); > + > + /* A/D conversion channel select error interrupt */ > + if (reg & RZG2L_ADSTS_CSEST) { > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > + return IRQ_HANDLED; > + } > + > + intst = reg & RZG2L_ADSTS_INTST_MASK; > + if (!intst) > + return IRQ_NONE; > + > + for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS) { > + if (intst & BIT(ch)) I'm missing how this if can fail given we only end up in here when the bit is set. > + adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) & > + RZG2L_ADCR_AD_MASK; > + } > + > + /* clear the channel interrupt */ > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > + > + complete(&adc->completion); > + > + return IRQ_HANDLED; > +} > + > +static int rzg2l_adc_parse_properties(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_hw_init(struct rzg2l_adc *adc) > +{ > + int timeout = 5; > + u32 reg; > + int ret; > + > + ret = clk_prepare_enable(adc->pclk); > + if (ret) > + return ret; > + > + /* SW reset */ > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0)); > + reg |= RZG2L_ADM0_SRESB; > + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg); > + > + while (!(rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_SRESB)) { > + if (!timeout) { > + ret = -EBUSY; > + goto exit_hw_init; > + } > + timeout--; > + usleep_range(100, 200); > + } > + > + /* Only division by 4 can be set */ > + reg = rzg2l_adc_readl(adc, RZG2L_ADIVC); > + reg &= ~RZG2L_ADIVC_DIVADC_MASK; > + reg |= RZG2L_ADIVC_DIVADC_4; > + rzg2l_adc_writel(adc, RZG2L_ADIVC, reg); > + > + /* > + * Setup AMD3 > + * ADIL[31:24] - Should be always set to 0 > + * ADCMP[23:16] - Should be always set to 0xe > + * ADSMP[15:0] - Set default (0x578) sampling period > + */ > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(3)); > + reg &= ~RZG2L_ADM3_ADIL_MASK; > + reg &= ~RZG2L_ADM3_ADCMP_MASK; > + reg &= ~RZG2L_ADM3_ADSMP_MASK; > + reg |= (RZG2L_ADM3_ADCMP_E | RZG2L_ADSMP_DEFUALT_SAMPLING); > + rzg2l_adc_writel(adc, RZG2L_ADM(3), reg); > + > +exit_hw_init: > + clk_disable_unprepare(adc->pclk); > + > + return 0; > +} > + > +static void rzg2l_adc_pm_runtime_disable(void *data) > +{ > + struct iio_dev *indio_dev = data; Register the devm call with the dev instead of the iio_dev. As this should be right next to the enable, it will be obvious what it should be. > + > + pm_runtime_disable(indio_dev->dev.parent); > +} > + > +static void rzg2l_adc_reset_assert(void *data) > +{ > + struct reset_control *reset = data; Why do we need the local variable? reset_control_assert(data); is fine. > + > + reset_control_assert(reset); > +} > + > +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_properties(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); Please group things together, so we 'get' whatever it is next to where we use it. It might make the error handling a tiny bit more complex, but it makes the code easier to review as we don't have to jump backwards and forwards all the time. With devm_ in heavy use as you have here, that's all magic in the error paths anyway :) > + 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); > + } > + > + ret = devm_add_action_or_reset(&pdev->dev, > + rzg2l_adc_reset_assert, adc->adrstn); > + if (ret) { > + dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n", > + ret); > + return ret; > + } > + > + 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 = devm_add_action_or_reset(&pdev->dev, > + rzg2l_adc_reset_assert, adc->presetn); As already raised, wrong place. > + if (ret) { > + dev_err(&pdev->dev, "failed to register presetn assert devm action, %d\n", > + ret); > + return ret; > + } > + > + ret = reset_control_deassert(adc->adrstn); > + if (ret) { > + dev_err(&pdev->dev, "failed to deassert adrstn pin, %d\n", ret); > + return ret; > + } > + > + ret = reset_control_deassert(adc->presetn); > + if (ret) { > + dev_err(&pdev->dev, "failed to deassert presetn pin, %d\n", ret); > + return ret; > + } > + > + ret = rzg2l_adc_hw_init(adc); > + if (ret) { > + dev_err(&pdev->dev, "failed to initialize ADC HW, %d\n", ret); > + return ret; > + } > + > + ret = devm_request_irq(dev, irq, rzg2l_adc_isr, > + 0, dev_name(dev), adc); > + if (ret < 0) > + return ret; > + > + init_completion(&adc->completion); > + > + platform_set_drvdata(pdev, indio_dev); > + > + 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); I think you also want to set the state to suspended to ensure the resume is called on get. > + ret = devm_add_action_or_reset(&pdev->dev, > + rzg2l_adc_pm_runtime_disable, indio_dev); > + if (ret) { > + dev_err(&pdev->dev, "failed to register pm_runtime_disable devm action, %d\n", > + ret); This is vanishingly unlikely as it's a tiny memory allocation. So I would dropt he error print in the interests of simplicity. Some of the other items above we don't print on are more likely to fail. > + return ret; > + } > + > + return devm_iio_device_register(dev, indio_dev); > +} ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter 2021-07-31 17:11 ` Jonathan Cameron @ 2021-07-31 18:31 ` Lad, Prabhakar 2021-08-01 16:59 ` Jonathan Cameron 0 siblings, 1 reply; 15+ messages in thread From: Lad, Prabhakar @ 2021-07-31 18:31 UTC (permalink / raw) To: Jonathan Cameron Cc: Lad Prabhakar, Geert Uytterhoeven, Rob Herring, Lars-Peter Clausen, Magnus Damm, Philipp Zabel, Alexandru Ardelean, linux-iio, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, LKML, Biju Das Hi Jonathan, Thank you for the review. On Sat, Jul 31, 2021 at 6:09 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 26 Jul 2021 19:28:49 +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 | 595 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 614 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..d05a3208ff9d > > --- /dev/null > > +++ b/drivers/iio/adc/rzg2l_adc.c > > @@ -0,0 +1,595 @@ > > +// 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/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/reset.h> > > + > > +#define DRIVER_NAME "rzg2l-adc" > > As only used in one place, just put it inline there so we don't need > to go find if we want to know the value - I'm lazy. > Its being used in two places 1: indio_dev->name = DRIVER_NAME #In probe call 2: .name = DRIVER_NAME # In platform_driver struct Let me know if you want me to replace them inline and drop the above macro. > > + > > +#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_MASK GENMASK(13, 12) > > +#define RZG2L_ADM2_CHSEL_MASK GENMASK(7, 0) > > +#define RZG2L_ADM3_ADIL_MASK GENMASK(31, 24) > > +#define RZG2L_ADM3_ADCMP_MASK GENMASK(23, 16) > > +#define RZG2L_ADM3_ADCMP_E FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe) > > +#define RZG2L_ADM3_ADSMP_MASK GENMASK(15, 0) > > + > > +#define RZG2L_ADINT 0x20 > > +#define RZG2L_ADINT_INTEN_MASK GENMASK(7, 0) > > +#define RZG2L_ADINT_CSEEN BIT(16) > > +#define RZG2L_ADINT_INTS BIT(31) > > + > > +#define RZG2L_ADSTS 0x24 > > +#define RZG2L_ADSTS_CSEST BIT(16) > > +#define RZG2L_ADSTS_INTST_MASK GENMASK(7, 0) > > + > > +#define RZG2L_ADIVC 0x28 > > +#define RZG2L_ADIVC_DIVADC_MASK GENMASK(8, 0) > > +#define RZG2L_ADIVC_DIVADC_4 FIELD_PREP(RZG2L_ADIVC_DIVADC_MASK, 0x4) > > + > > +#define RZG2L_ADFIL 0x2c > > + > > +#define RZG2L_ADCR(n) (0x30 + ((n) * 0x4)) > > +#define RZG2L_ADCR_AD_MASK GENMASK(11, 0) > > + > > +#define RZG2L_ADSMP_DEFUALT_SAMPLING 0x578 > > + > > +#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 */ > > This comment seems odd. If it is stopping conversion, how is this function > used to start conversion? Perhaps some more details. > My bad its should be start/stop, said that already the function is named rzg2l_adc_start_stop() so will drop this comment. > > + 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; > > + > > + /* > > + * Setup ADM1 for SW trigger > > + * EGA[13:12] - Set 00 to indicate hardware trigger is invalid > > + * BS[4] - Enable 1-buffer mode > > + * MS[1] - Enable Select mode > > + * TRG[0] - Enable software trigger mode > > + */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(1)); > > + reg &= ~RZG2L_ADM1_EGA_MASK; > > + reg &= ~RZG2L_ADM1_BS; > > + reg &= ~RZG2L_ADM1_TRG; > > + reg |= RZG2L_ADM1_MS; > > + 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 analog input channel subjected to conversion. */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(2)); > > + reg &= ~RZG2L_ADM2_CHSEL_MASK; > > + reg |= BIT(ch); > > + rzg2l_adc_writel(adc, RZG2L_ADM(2), reg); > > + > > + /* > > + * Setup ADINT > > + * INTS[31] - Select pulse signal > > + * CSEEN[16] - Enable channel select error interrupt > > + * INTEN[7:0] - Select channel interrupt > > + */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADINT); > > + reg &= ~RZG2L_ADINT_INTS; > > + reg &= ~RZG2L_ADINT_INTEN_MASK; > > + reg |= (RZG2L_ADINT_CSEEN | 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_INTEN_MASK); > > + 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]; > > You should not rely on implementation details of iio_device_claim_direct_mode() > to protect your driver specific data structures. Use your own lock. > Those calls are intended for safe operation when buffered mode in IIO is supported > and currently your driver doesn't support that at all so please remove them. > sure will add drivers own lock. > > + 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 sysfs_emit(label, "%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; > > No need for explicit cast from void * to another pointer type. > This is always valid without in C. > Agreed. > > + unsigned long intst; > > + u32 reg; > > + int ch; > > + > > + reg = rzg2l_adc_readl(adc, RZG2L_ADSTS); > > + > > + /* A/D conversion channel select error interrupt */ > > + if (reg & RZG2L_ADSTS_CSEST) { > > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > > + return IRQ_HANDLED; > > + } > > + > > + intst = reg & RZG2L_ADSTS_INTST_MASK; > > + if (!intst) > > + return IRQ_NONE; > > + > > + for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS) { > > + if (intst & BIT(ch)) > > I'm missing how this if can fail given we only end up in here when the bit is > set. > ADC has 8 channels RZG2L_ADSTS register bits [0:7] will be set to 1 when the given channel ADC conversion has been completed. So the above if condition checks if the bit is set to 1 and then reads the corresponding value of that channel. > > + adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) & > > + RZG2L_ADCR_AD_MASK; > > + } > > + > > + /* clear the channel interrupt */ > > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > > + > > + complete(&adc->completion); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int rzg2l_adc_parse_properties(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_hw_init(struct rzg2l_adc *adc) > > +{ > > + int timeout = 5; > > + u32 reg; > > + int ret; > > + > > + ret = clk_prepare_enable(adc->pclk); > > + if (ret) > > + return ret; > > + > > + /* SW reset */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0)); > > + reg |= RZG2L_ADM0_SRESB; > > + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg); > > + > > + while (!(rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_SRESB)) { > > + if (!timeout) { > > + ret = -EBUSY; > > + goto exit_hw_init; > > + } > > + timeout--; > > + usleep_range(100, 200); > > + } > > + > > + /* Only division by 4 can be set */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADIVC); > > + reg &= ~RZG2L_ADIVC_DIVADC_MASK; > > + reg |= RZG2L_ADIVC_DIVADC_4; > > + rzg2l_adc_writel(adc, RZG2L_ADIVC, reg); > > + > > + /* > > + * Setup AMD3 > > + * ADIL[31:24] - Should be always set to 0 > > + * ADCMP[23:16] - Should be always set to 0xe > > + * ADSMP[15:0] - Set default (0x578) sampling period > > + */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(3)); > > + reg &= ~RZG2L_ADM3_ADIL_MASK; > > + reg &= ~RZG2L_ADM3_ADCMP_MASK; > > + reg &= ~RZG2L_ADM3_ADSMP_MASK; > > + reg |= (RZG2L_ADM3_ADCMP_E | RZG2L_ADSMP_DEFUALT_SAMPLING); > > + rzg2l_adc_writel(adc, RZG2L_ADM(3), reg); > > + > > +exit_hw_init: > > + clk_disable_unprepare(adc->pclk); > > + > > + return 0; > > +} > > + > > +static void rzg2l_adc_pm_runtime_disable(void *data) > > +{ > > + struct iio_dev *indio_dev = data; > > Register the devm call with the dev instead of the iio_dev. > As this should be right next to the enable, it will be obvious > what it should be. > Agreed will do. > > + > > + pm_runtime_disable(indio_dev->dev.parent); > > +} > > + > > +static void rzg2l_adc_reset_assert(void *data) > > +{ > > + struct reset_control *reset = data; > Why do we need the local variable? > > reset_control_assert(data); > > is fine. > Agreed. > > + > > + reset_control_assert(reset); > > +} > > + > > +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_properties(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); > > Please group things together, so we 'get' whatever it is next to where > we use it. It might make the error handling a tiny bit more complex, but > it makes the code easier to review as we don't have to jump backwards and forwards > all the time. With devm_ in heavy use as you have here, that's all magic > in the error paths anyway :) > sure will move this just above the devm_request_irq(). > > + 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); > > + } > > + > > + ret = devm_add_action_or_reset(&pdev->dev, > > + rzg2l_adc_reset_assert, adc->adrstn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n", > > + ret); > > + return ret; > > + } > > + > > + 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 = devm_add_action_or_reset(&pdev->dev, > > + rzg2l_adc_reset_assert, adc->presetn); > > As already raised, wrong place. > Agreed. > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register presetn assert devm action, %d\n", > > + ret); > > + return ret; > > + } > > + > > + ret = reset_control_deassert(adc->adrstn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to deassert adrstn pin, %d\n", ret); > > + return ret; > > + } > > + > > + ret = reset_control_deassert(adc->presetn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to deassert presetn pin, %d\n", ret); > > + return ret; > > + } > > + > > + ret = rzg2l_adc_hw_init(adc); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to initialize ADC HW, %d\n", ret); > > + return ret; > > + } > > + > > + ret = devm_request_irq(dev, irq, rzg2l_adc_isr, > > + 0, dev_name(dev), adc); > > + if (ret < 0) > > + return ret; > > + > > + init_completion(&adc->completion); > > + > > + platform_set_drvdata(pdev, indio_dev); > > + > > + 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); > > I think you also want to set the state to suspended to ensure the resume is > called on get. > pm_runtime_set_suspended() should only be called when runtime is disabled or is it that I am missing something ? > > + ret = devm_add_action_or_reset(&pdev->dev, > > + rzg2l_adc_pm_runtime_disable, indio_dev); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register pm_runtime_disable devm action, %d\n", > > + ret); > > This is vanishingly unlikely as it's a tiny memory allocation. So I would dropt he error > print in the interests of simplicity. Some of the other items above we don't print > on are more likely to fail. > Sure will drop this. Cheers, Prabhakar > > + return ret; > > + } > > + > > + return devm_iio_device_register(dev, indio_dev); > > +} ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter 2021-07-31 18:31 ` Lad, Prabhakar @ 2021-08-01 16:59 ` Jonathan Cameron 2021-08-01 19:51 ` Lad, Prabhakar 0 siblings, 1 reply; 15+ messages in thread From: Jonathan Cameron @ 2021-08-01 16:59 UTC (permalink / raw) To: Lad, Prabhakar Cc: Lad Prabhakar, Geert Uytterhoeven, Rob Herring, Lars-Peter Clausen, Magnus Damm, Philipp Zabel, Alexandru Ardelean, linux-iio, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, LKML, Biju Das On Sat, 31 Jul 2021 19:31:52 +0100 "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > Hi Jonathan, > > Thank you for the review. > ... > > > +#define DRIVER_NAME "rzg2l-adc" > > > > As only used in one place, just put it inline there so we don't need > > to go find if we want to know the value - I'm lazy. > > > Its being used in two places > 1: indio_dev->name = DRIVER_NAME #In probe call > 2: .name = DRIVER_NAME # In platform_driver struct > > Let me know if you want me to replace them inline and drop the above macro. oops. Searching apparently failed me ;) Fine as is. ... > > > +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; > > > > No need for explicit cast from void * to another pointer type. > > This is always valid without in C. > > > Agreed. > > > > + unsigned long intst; > > > + u32 reg; > > > + int ch; > > > + > > > + reg = rzg2l_adc_readl(adc, RZG2L_ADSTS); > > > + > > > + /* A/D conversion channel select error interrupt */ > > > + if (reg & RZG2L_ADSTS_CSEST) { > > > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > > > + return IRQ_HANDLED; > > > + } > > > + > > > + intst = reg & RZG2L_ADSTS_INTST_MASK; > > > + if (!intst) > > > + return IRQ_NONE; > > > + > > > + for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS) { > > > + if (intst & BIT(ch)) > > > > I'm missing how this if can fail given we only end up in here when the bit is > > set. > > > ADC has 8 channels RZG2L_ADSTS register bits [0:7] will be set to 1 > when the given channel ADC conversion has been completed. So the above > if condition checks if the bit is set to 1 and then reads the > corresponding value of that channel. Just looking at the two lines of code above for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS) will only call the the next line if the bit is set. E.g. It will only call it if (intst & BIT(ch)) So you can only get into the body of the for loop if this bit is set and the condition is always true. Hence drop if (intst & BIT(ch)) > > > > + adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) & > > > + RZG2L_ADCR_AD_MASK; > > > + } > > > + > > > + /* clear the channel interrupt */ > > > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > > > + > > > + complete(&adc->completion); > > > + > > > + return IRQ_HANDLED; > > > +} > > > + ... > > > + > > > + pm_runtime_enable(dev); > > > > I think you also want to set the state to suspended to ensure the resume is > > called on get. > > > pm_runtime_set_suspended() should only be called when runtime is > disabled or is it that I am missing something ? If you want the runtime pm code to assume your device is suspended initially then you set the state before you call pm_runtime_enable(dev). J ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter 2021-08-01 16:59 ` Jonathan Cameron @ 2021-08-01 19:51 ` Lad, Prabhakar 0 siblings, 0 replies; 15+ messages in thread From: Lad, Prabhakar @ 2021-08-01 19:51 UTC (permalink / raw) To: Jonathan Cameron Cc: Lad Prabhakar, Geert Uytterhoeven, Rob Herring, Lars-Peter Clausen, Magnus Damm, Philipp Zabel, Alexandru Ardelean, linux-iio, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, LKML, Biju Das Hi Jonathan, On Sun, Aug 1, 2021 at 5:57 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 31 Jul 2021 19:31:52 +0100 > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > Hi Jonathan, > > > > Thank you for the review. > > > > ... > > > > > +#define DRIVER_NAME "rzg2l-adc" > > > > > > As only used in one place, just put it inline there so we don't need > > > to go find if we want to know the value - I'm lazy. > > > > > Its being used in two places > > 1: indio_dev->name = DRIVER_NAME #In probe call > > 2: .name = DRIVER_NAME # In platform_driver struct > > > > Let me know if you want me to replace them inline and drop the above macro. > > oops. Searching apparently failed me ;) Fine as is. > > ... > > > > > > +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; > > > > > > No need for explicit cast from void * to another pointer type. > > > This is always valid without in C. > > > > > Agreed. > > > > > > + unsigned long intst; > > > > + u32 reg; > > > > + int ch; > > > > + > > > > + reg = rzg2l_adc_readl(adc, RZG2L_ADSTS); > > > > + > > > > + /* A/D conversion channel select error interrupt */ > > > > + if (reg & RZG2L_ADSTS_CSEST) { > > > > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > > > > + return IRQ_HANDLED; > > > > + } > > > > + > > > > + intst = reg & RZG2L_ADSTS_INTST_MASK; > > > > + if (!intst) > > > > + return IRQ_NONE; > > > > + > > > > + for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS) { > > > > + if (intst & BIT(ch)) > > > > > > I'm missing how this if can fail given we only end up in here when the bit is > > > set. > > > > > ADC has 8 channels RZG2L_ADSTS register bits [0:7] will be set to 1 > > when the given channel ADC conversion has been completed. So the above > > if condition checks if the bit is set to 1 and then reads the > > corresponding value of that channel. > > Just looking at the two lines of code above > for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS) > will only call the the next line if the bit is set. E.g. It will only call > it > if (intst & BIT(ch)) > > So you can only get into the body of the for loop if this bit is set and the > condition is always true. Hence drop > if (intst & BIT(ch)) > Agreed can be dropped. > > > > > > + adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) & > > > > + RZG2L_ADCR_AD_MASK; > > > > + } > > > > + > > > > + /* clear the channel interrupt */ > > > > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > > > > + > > > > + complete(&adc->completion); > > > > + > > > > + return IRQ_HANDLED; > > > > +} > > > > + > > ... > > > > > + > > > > + pm_runtime_enable(dev); > > > > > > I think you also want to set the state to suspended to ensure the resume is > > > called on get. > > > > > pm_runtime_set_suspended() should only be called when runtime is > > disabled or is it that I am missing something ? > > If you want the runtime pm code to assume your device is suspended initially > then you set the state before you call pm_runtime_enable(dev). > OK will call pm_runtime_set_suspended() before enabling. Cheers, Prabhakar ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 3/3] arm64: dts: renesas: r9a07g044: Add ADC node 2021-07-26 18:28 [PATCH v3 0/3] Renesas RZ/G2L ADC driver support Lad Prabhakar 2021-07-26 18:28 ` [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar 2021-07-26 18:28 ` [PATCH v3 2/3] iio: adc: Add driver " Lad Prabhakar @ 2021-07-26 18:28 ` Lad Prabhakar 2 siblings, 0 replies; 15+ messages in thread From: Lad Prabhakar @ 2021-07-26 18:28 UTC (permalink / raw) To: Geert Uytterhoeven, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Magnus Damm, Philipp Zabel, Alexandru Ardelean, linux-iio, devicetree, linux-renesas-soc Cc: linux-kernel, 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 9a7489dc70d1..28aafa34d583 100644 --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi @@ -169,6 +169,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] 15+ messages in thread
end of thread, other threads:[~2021-08-01 19:51 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-26 18:28 [PATCH v3 0/3] Renesas RZ/G2L ADC driver support Lad Prabhakar 2021-07-26 18:28 ` [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar 2021-07-26 21:42 ` Rob Herring 2021-07-27 8:06 ` Lad, Prabhakar 2021-07-27 20:30 ` Rob Herring 2021-07-26 18:28 ` [PATCH v3 2/3] iio: adc: Add driver " Lad Prabhakar 2021-07-27 7:13 ` Philipp Zabel 2021-07-27 8:02 ` Lad, Prabhakar 2021-07-27 8:13 ` Philipp Zabel 2021-07-27 8:54 ` Lad, Prabhakar 2021-07-31 17:11 ` Jonathan Cameron 2021-07-31 18:31 ` Lad, Prabhakar 2021-08-01 16:59 ` Jonathan Cameron 2021-08-01 19:51 ` Lad, Prabhakar 2021-07-26 18:28 ` [PATCH v3 3/3] 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.