Hi Jonathan, Thank you for the review. On Sat, Jul 3, 2021 at 6:17 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > On Tue, 29 Jun 2021 23:03:27 +0100 > Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote: > > > Add binding documentation for Renesas RZ/G2L A/D converter block. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > Hi, > > See inline > > Jonathan > > > --- > > .../bindings/iio/adc/renesas,rzg2l-adc.yaml | 121 ++++++++++++++++++ > > 1 file changed, 121 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..db935d6d59eb > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml > > @@ -0,0 +1,121 @@ > > +# 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@xxxxxxxxxxxxxx> > > + > > +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: > > + oneOf: > > + - 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 > > + > > + renesas-rzg2l,adc-trigger-mode: > > + $ref: /schemas/types.yaml#/definitions/uint8 > > + description: Trigger mode for A/D converter > > + enum: > > + - 0 # Software trigger mode (Defaults) > > + - 1 # Asynchronous trigger using ADC_TRG trigger input pin > > + - 2 # Synchronous trigger (Trigger from MTU3a/GPT) > > Is this a function of the board in some fashion? If not it sounds like > something that should be in control of userspace. Normally we'd > do that by having the driver register some iio_triggers and depending > on which one is selected do the equivalent of what you have here. > Agreed for Asynchronous and Synchronous triggers. WRT Software trigger should this be registered as a iio_triggers too or read_raw() callback (with IIO_CHAN_INFO_RAW case) should be treated as Software trigger? > > + default: 0 > > + > > + gpios: > > + description: > > + ADC_TRG trigger input pin > > + maxItems: 1 > Why is this mode useful? I'm assuming the gpio write would take a register > write and the software trigger mode also requires a register write. > Yes gpio write would take a register write. > Normally the reason for a pin like this is to support synchronising with > external hardware. If that's the case, we should call that out here. > often the pin isn't even connected to a gpio in our control. > (i.e. it's a trigger signal from some other device.) > So just setting the GPIO pin as input should do the trick. > > + > > + renesas-rzg2l,adc-channels: > > + $ref: /schemas/types.yaml#/definitions/uint8-array > > + description: Input channels available on platform > > + uniqueItems: true > > + minItems: 1 > > + maxItems: 8 > > + items: > > + enum: [0, 1, 2, 3, 4, 5, 6, 7] > > Is this a function of different devices (should have different compatibles) > or of what is wired up. If it's what is wired up, then how do you know which Its channels which are wired, for example if channels 0-5 are wired up the board dts would include the property "renesas-rzg2l,adc-channels = /bits/ 8 <0 1 2 3 4 5>;" > subset of channels are connected? We have the generic adc channel binding > in iio/adc/adc.yaml for the case where we only want to expose those channels > that are wired up. It uses a node per channel. > Agreed will do that and drop the custom "renesas-rzg2l,adc-channels" Cheers, Prabhakar > > + > > + "#io-channel-cells": > > + const: 1 > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + - power-domains > > + - resets > > + - reset-names > > + - renesas-rzg2l,adc-channels > > + - "#io-channel-cells" > > + > > +allOf: > > + - if: > > + properties: > > + renesas-rzg2l,adc-trigger-mode: > > + const: 1 > > + then: > > + required: > > + - gpios > > + > > +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"; > > + #io-channel-cells = <1>; > > + renesas-rzg2l,adc-trigger-mode = /bits/ 8 <0>; > > + renesas-rzg2l,adc-channels = /bits/ 8 <0 1 2 3 4 5 6>; > > + }; >
