| Message ID | 20230524082744.3215427-1-bigunclemax@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for Allwinner GPADC on D1/T113s/R329 SoCs | expand |
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:30 +0300 Maxim Kiselev <bigunclemax@gmail.com> wrote: > The General Purpose ADC (GPADC) can convert the external signal into > a certain proportion of digital value, to realize the measurement of > analog signal, which can be applied to power detection and key detection. > > D1, T113s and R329 contain this GPADC IP. The only difference between > this SoCs is the number of available channels: > > T113 - 1 channel > D1 - 2 channels > R329 - 4 channels > > Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com> Hi Maxim, Various comments inline. Thanks, Jonathan > --- > drivers/iio/adc/Kconfig | 10 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/sun20i-gpadc-iio.c | 275 +++++++++++++++++++++++++++++ > 3 files changed, 286 insertions(+) > create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index eb2b09ef5d5b..988804f46bf6 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -1123,6 +1123,16 @@ config SUN4I_GPADC > To compile this driver as a module, choose M here: the module will be > called sun4i-gpadc-iio. > > +config SUN20I_GPADC > + tristate "Support for the Allwinner SoCs GPADC" > + depends on ARCH_SUNXI || COMPILE_TEST > + help > + Say yes here to build support for Allwinner (D1, T113 and R329) SoCs > + GPADC. This ADC provides up to 4 channels. Given you wrote code that clamps at 16 channels I'd avoid saying only 4 here as it will then need updating if another device turns up with more than 4. > + > + To compile this driver as a module, choose M here: the module will be > + called sun20i-gpadc-iio. > + > config TI_ADC081C > tristate "Texas Instruments ADC081C/ADC101C/ADC121C family" > depends on I2C > diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c > new file mode 100644 > index 000000000000..90f3bb2e41cd > --- /dev/null > +++ b/drivers/iio/adc/sun20i-gpadc-iio.c > @@ -0,0 +1,275 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * GPADC driver for sunxi platforms (D1, T113-S3 and R329) > + * Copyright (c) 2023 Maksim Kiselev <bigunclemax@gmail.com> > + */ > + > +#include <linux/completion.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> Use generic versions from property.h > +#include <linux/clk.h> Alphabetical order preferred. Fine to separate out the iio one as a more specific group at the end though. > +#include <linux/reset.h> > +#include <linux/platform_device.h> I'd expect to see mod_devicetable.h included. > +#include <linux/regmap.h> > +#include <linux/iio/iio.h> > + > +#define SUN20I_GPADC_SR 0x00 > + > +#define SUN20I_GPADC_CTRL 0x04 > +#define SUN20I_GPADC_CTRL_ADC_FIRST_DLY ((GENMASK(7, 0) & (x)) << 24) define the mask, and use FIELD_PREP to place the data in it. Same for other cases that look like this. > +#define SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN BIT(23) > +#define SUN20I_GPADC_CTRL_ADC_OP_BIAS ((GENMASK(1, 0) & (x)) << 20) > +#define SUN20I_GPADC_CTRL_WORK_MODE_SELECT(x) ((GENMASK(1, 0) & (x)) << 18) > +#define SUN20I_GPADC_CTRL_ADC_CALI_EN BIT(17) > +#define SUN20I_GPADC_CTRL_ADC_EN BIT(16) > + > +#define SUN20I_GPADC_CS_EN 0x08 > +#define SUN20I_GPADC_CS_EN_ADC_CHAN_CMP_EN(x) ((GENMASK(16, 0) & BIT(x)) << 16) > +#define SUN20I_GPADC_CS_EN_ADC_CHAN_SELECT(x) (GENMASK(16, 0) & BIT(x)) > + > +#define SUN20I_GPADC_DATA_INTC 0x28 > +#define SUN20I_GPADC_DATA_INTC_CHAN_DATA_IRQ_EN(x) (GENMASK(16, 0) & BIT(x)) > + > +#define SUN20I_GPADC_DATA_INTS 0x38 > +#define SUN20I_GPADC_DATA_INTS_DATA_PENDING(x) (GENMASK(16, 0) & BIT(x)) > +#define SUN20I_GPADC_DATA_INTS_DATA_PENDING_MASK GENMASK(16, 0) > + > +#define SUN20I_GPADC_CHAN_DATA(x) (0x80 + (x) * 4) > + > +#define SUN20I_GPADC_TIMEOUT msecs_to_jiffies(100) I'd either document why that time here, or push this down to were it's called. It's a real world value rather than a magic number so doesn't need a name unless more information is provided with that. > +#define SUN20I_GPADC_MAX_CHANNELS 16 > + ... > + > +static int sun20i_gpadc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + struct sun20i_gpadc_iio *info = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&info->lock); > + ret = sun20i_gpadc_adc_read(info, chan, val); > + mutex_unlock(&info->lock); > + return ret; > + case IIO_CHAN_INFO_SCALE: > + /* 1800mV / 4096 * raw */ Not * raw, because that bit is in userspace - or express this as value in mv = 1800mV / 4096 raw Also we have IIO_VAL_FRACTIONAL_LOG_2 - the advantage of that being that a consumer of this channel can do more efficient maths in some cases if it gets the values in that form. > + *val = 0; > + *val2 = 439453125; > + return IIO_VAL_INT_PLUS_NANO; > + default: > + return -EINVAL; > + } > +} > + > +static irqreturn_t sun20i_gpadc_irq_handler(int irq, void *data) > +{ > + struct sun20i_gpadc_iio *info = data; > + u32 reg; > + > + /* clear data interrupt status register */ > + regmap_read(info->regmap, SUN20I_GPADC_DATA_INTS, ®); > + regmap_write(info->regmap, SUN20I_GPADC_DATA_INTS, reg); > + > + complete(&info->completion); > + > + return IRQ_HANDLED; > +} > + > +static const struct iio_info sun20i_gpadc_iio_info = { > + .read_raw = sun20i_gpadc_read_raw, > +}; > + > +static int sun20i_gpadc_probe(struct platform_device *pdev) > +{ > + const struct sun20i_gpadc_configuration *config; > + struct iio_dev *indio_dev; > + struct sun20i_gpadc_iio *info; > + struct reset_control *rst; > + void __iomem *base; > + struct clk *clk; > + int irq; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info)); > + if (!indio_dev) > + return -ENOMEM; > + > + info = iio_priv(indio_dev); > + platform_set_drvdata(pdev, indio_dev); Why set this? I'm not immediately spotting it being used. > + > + mutex_init(&info->lock); > + init_completion(&info->completion); > + > + info->indio_dev = indio_dev; This is very rarely a good idea. It normally implies that the level of abstraction has gone wrong somewhere in the driver. Here I don't think you even use it - hence drop that pointer from the structure. > + indio_dev->info = &sun20i_gpadc_iio_info; > + indio_dev->name = dev_name(&pdev->dev); dev_name() has a habit of not reflecting the part number of the device, which is what should be used for indio_dev->name. Usual fix is put it in your device type specific structure - which is another reason you may want to keep the explicit compatibles. > + > + config = of_device_get_match_data(&pdev->dev); I think this already got pointed out. In IIO we are trying to use the generic firmware interfaces for everything. Whilst there are devices where it is unlikely that anyone will ever use them with other firmware types, using the generic interfaces is more consistent for reviewers of the wide class of devices that are used with ACPI etc. > + if (!config) > + return -ENODEV; > + > + /* Sanity check for possible later IP variants with more channels */ > + if (config->num_channels > SUN20I_GPADC_MAX_CHANNELS) > + return dev_err_probe(&pdev->dev, -EINVAL, > + "max channels exceeded\n"); This should be either by design or a compile time check if you really want to check it. > + > + indio_dev->channels = config->channels; > + indio_dev->num_channels = config->num_channels; > + > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + info->regmap = devm_regmap_init_mmio(&pdev->dev, base, > + &sun20i_gpadc_regmap_config); > + if (IS_ERR(info->regmap)) > + return dev_err_probe(&pdev->dev, PTR_ERR(info->regmap), > + "failed to init regmap\n"); > + > + clk = devm_clk_get_enabled(&pdev->dev, NULL); > + if (IS_ERR(clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(clk), > + "failed to enable bus clock\n"); > + > + rst = devm_reset_control_get_exclusive(&pdev->dev, NULL); > + if (IS_ERR(rst)) > + return dev_err_probe(&pdev->dev, PTR_ERR(rst), > + "failed to get reset control\n"); > + > + ret = reset_control_deassert(rst); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "failed to deassert reset\n"); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return dev_err_probe(&pdev->dev, irq, "failed to get irq\n"); > + > + ret = devm_request_irq(&pdev->dev, irq, sun20i_gpadc_irq_handler, > + 0, dev_name(&pdev->dev), info); > + if (ret < 0) > + return dev_err_probe(&pdev->dev, ret, > + "failed requesting irq %d\n", irq); > + > + /* set the single conversion mode and enable auto calibration*/ > + regmap_write(info->regmap, SUN20I_GPADC_CTRL, > + SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN | > + SUN20I_GPADC_CTRL_WORK_MODE_SELECT(0)); > + > + ret = devm_iio_device_register(&pdev->dev, indio_dev); > + if (ret < 0) > + return dev_err_probe(&pdev->dev, ret, > + "could not register the device\n"); > + > + return 0; > +} > + > +static const struct of_device_id sun20i_gpadc_of_id[] = { > + { > + .compatible = "allwinner,sun8i-t113-gpadc", > + .data = &sun20i_gpadc_config[0], > + }, > + { > + .compatible = "allwinner,sun20i-d1-gpadc", > + .data = &sun20i_d1_gpadc_channels[1] > + }, > + { > + .compatible = "allwinner,sun50i-r329-gpadc", > + .data = &sun50i_r329_gpadc_channels[2] I agree with the other reviewer who suggested the array was unnecessary - if you did want to keep it for some reason the introduce an enum so you can reference it by named index. Add commas to these lines, both for consistency and because maybe we will one day set more in that structure and it will reduce the noise level of such a patch. > + }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, sun20i_gpadc_of_id);
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