| Message ID | 20230524082744.3215427-1-bigunclemax@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for Allwinner GPADC on D1/T113s/R329 SoCs | expand |
On Wed, 24 May 2023 11:27:29 +0300 Maxim Kiselev <bigunclemax@gmail.com> wrote: Hi Maxim, many thanks for your work on this and for posting it! > This series adds support for general purpose ADC (GPADC) on new > Allwinner's SoCs, such as D1, T113s and R329. The implemented driver > provides basic functionality for getting ADC channels data. > > All of the listed SoCs have the same IP. The only difference is the number > of available channels: > T113 - 1 channel > D1 - 2 channels > R329 - 4 channels This may sound kind of obvious, but wouldn't it be easier to model this with one compatible string, and have the number of channels as a DT property? Or, alternatively, using iio/multiplexer/io-channel-mux.yaml, since it's only one ADC anyway? And btw: it seems that the T507 (the H616 die with a different pinout) has the same IP, with four channels: http://dl.linux-sunxi.org/T507/ Cheers, Andre > This series is just an RFC and I would be glad to see any comments > about it. > > > Maxim Kiselev (4): > iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC > dt-bindings: iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC > ARM: dts: sun8i: t113s: Add GPADC node > riscv: dts: allwinner: d1: Add GPADC node > > .../iio/adc/allwinner,sun20i-d1-gpadc.yaml | 52 ++++ > arch/arm/boot/dts/sun8i-t113s.dtsi | 12 + > arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi | 10 + > drivers/iio/adc/Kconfig | 10 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/sun20i-gpadc-iio.c | 275 ++++++++++++++++++ > 6 files changed, 360 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml > create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c >
Hi Andre, thanks for you comments > This may sound kind of obvious, but wouldn't it be easier to model this > with one compatible string, and have the number of channels as a DT > property? Yes, I completely agree that using separate config for each SoCs is looks overcomplicated because the only difference is the number of channels. I thought about a DT property with channels number but I didn't find another ADC driver with the same approach (except i2c ADC's with child nodes). > Or, alternatively, using iio/multiplexer/io-channel-mux.yaml, since it's > only one ADC anyway? I'm sorry, I didn't quite understand what you're suggesting. > And btw: it seems that the T507 (the H616 die with a different pinout) has > the same IP, with four channels: > http://dl.linux-sunxi.org/T507/ Oh, thanks for pointing that. I'll add it to the list in the next version.
On Wed, 24 May 2023 14:36:28 +0300 Maxim Kiselev <bigunclemax@gmail.com> wrote: > Hi Andre, > > thanks for you comments > > > This may sound kind of obvious, but wouldn't it be easier to model this > > with one compatible string, and have the number of channels as a DT > > property? > > Yes, I completely agree that using separate config for each SoCs is looks > overcomplicated because the only difference is the number of channels. > I thought about a DT property with channels number but I didn't find > another ADC driver with the same approach (except i2c ADC's with child nodes). If you are 100% sure that that devices are either 1) Detectable at runtime 2) Identical in functionality. So that in neither case will any changes on driver support expose differences in the future then a single compatible is fine. The back up is that you use fallback compatibles - list more than one. Whilst it doesn't matter (as no differences found) the driver can use the first one. If differences become apparent later, others may be used. I'm not however keen on a simple channel count parameter. If you want to go that way, it's better to provide the fine control of individual channel child nodes (see Documentation/devicetree/bindings/iio/adc/adc.yaml) That way the control is on which channels are wired to something useful, rather than whether the device can read them or not (which is pointless if no one wired them up. > > > Or, alternatively, using iio/multiplexer/io-channel-mux.yaml, since it's > > only one ADC anyway? > I'm sorry, I didn't quite understand what you're suggesting. That's normally only used for a separate MUX where we need a separate driver to handle it. If used on a device like this it would expose additional complexity to userspace with no benefits in generality etc. > > > And btw: it seems that the T507 (the H616 die with a different pinout) has > > the same IP, with four channels: > > http://dl.linux-sunxi.org/T507/ > > Oh, thanks for pointing that. I'll add it to the list in the next version.
On Sun, 28 May 2023 16:22:57 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Wed, 24 May 2023 14:36:28 +0300 > Maxim Kiselev <bigunclemax@gmail.com> wrote: > > > Hi Andre, > > > > thanks for you comments > > > > > This may sound kind of obvious, but wouldn't it be easier to model this > > > with one compatible string, and have the number of channels as a DT > > > property? > > > > Yes, I completely agree that using separate config for each SoCs is looks > > overcomplicated because the only difference is the number of channels. > > I thought about a DT property with channels number but I didn't find > > another ADC driver with the same approach (except i2c ADC's with child nodes). > If you are 100% sure that that devices are either > 1) Detectable at runtime > 2) Identical in functionality. > > So that in neither case will any changes on driver support expose differences > in the future then a single compatible is fine. > > The back up is that you use fallback compatibles - list more than one. > Whilst it doesn't matter (as no differences found) the driver can use > the first one. If differences become apparent later, others may be used. > > I'm not however keen on a simple channel count parameter. If you want > to go that way, it's better to provide the fine control of individual channel > child nodes (see Documentation/devicetree/bindings/iio/adc/adc.yaml) > > That way the control is on which channels are wired to something useful, rather > than whether the device can read them or not (which is pointless if no one > wired them up. Another thing to note is that with generic compatibles you loose the ability to have the dt-schmema validate that sets of parameters make sense. If it's just the channels available that may not matter however. > > > > > > > Or, alternatively, using iio/multiplexer/io-channel-mux.yaml, since it's > > > only one ADC anyway? > > I'm sorry, I didn't quite understand what you're suggesting. > > That's normally only used for a separate MUX where we need a separate driver > to handle it. If used on a device like this it would expose additional complexity > to userspace with no benefits in generality etc. > > > > > > And btw: it seems that the T507 (the H616 die with a different pinout) has > > > the same IP, with four channels: > > > http://dl.linux-sunxi.org/T507/ > > > > Oh, thanks for pointing that. I'll add it to the list in the next version. >
On Wed, 24 May 2023 11:27:29 +0300 Maxim Kiselev <bigunclemax@gmail.com> wrote: > Hi, > > This series adds support for general purpose ADC (GPADC) on new > Allwinner's SoCs, such as D1, T113s and R329. The implemented driver > provides basic functionality for getting ADC channels data. > > All of the listed SoCs have the same IP. The only difference is the number > of available channels: > T113 - 1 channel > D1 - 2 channels > R329 - 4 channels > > This series is just an RFC and I would be glad to see any comments > about it. Why 'just an RFC'? Normal to call out aspects that mean it doesn't yet make sense for it to be picked up for upstream. Looks pretty good to me in general rather than RFC material so perhaps I'm missing something. Jonathan > > > Maxim Kiselev (4): > iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC > dt-bindings: iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC > ARM: dts: sun8i: t113s: Add GPADC node > riscv: dts: allwinner: d1: Add GPADC node > > .../iio/adc/allwinner,sun20i-d1-gpadc.yaml | 52 ++++ > arch/arm/boot/dts/sun8i-t113s.dtsi | 12 + > arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi | 10 + > drivers/iio/adc/Kconfig | 10 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/sun20i-gpadc-iio.c | 275 ++++++++++++++++++ > 6 files changed, 360 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml > create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c >
Hi, This series adds support for general purpose ADC (GPADC) on new Allwinner's SoCs, such as D1, T113s and R329. The implemented driver provides basic functionality for getting ADC channels data. All of the listed SoCs have the same IP. The only difference is the number of available channels: T113 - 1 channel D1 - 2 channels R329 - 4 channels This series is just an RFC and I would be glad to see any comments about it. Maxim Kiselev (4): iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC dt-bindings: iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC ARM: dts: sun8i: t113s: Add GPADC node riscv: dts: allwinner: d1: Add GPADC node .../iio/adc/allwinner,sun20i-d1-gpadc.yaml | 52 ++++ arch/arm/boot/dts/sun8i-t113s.dtsi | 12 + arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi | 10 + drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/sun20i-gpadc-iio.c | 275 ++++++++++++++++++ 6 files changed, 360 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c