| Message ID | 20230601223104.1243871-2-bigunclemax@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Allwinner GPADC on D1/T113s/R329/T507 SoCs | expand |
On Fri, Jun 02, 2023 at 01:30:39AM +0300, Maksim Kiselev wrote: > From: Maxim Kiselev <bigunclemax@gmail.com> > > 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. > > Theoretically, this ADC can support up to 16 channels. All SoCs below > contain this GPADC IP. The only difference between them is the number > of available channels: > > T113 - 1 channel > D1 - 2 channels > R329 - 4 channels > T507 - 4 channels ... > +struct sun20i_gpadc_iio { > + struct regmap *regmap; > + struct completion completion; > + struct mutex lock; The locks should be explained (what are they for? what do they protect?). > + int lastch; > +}; ... > +static const struct regmap_config sun20i_gpadc_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .fast_io = true, I forgot if I asked about regmap lock do you need it? > +}; ... > + if (!wait_for_completion_timeout(&info->completion, > + msecs_to_jiffies(100))) { Dunno if it's better to have this parameter to be defined with self-explanatory name. > + ret = -ETIMEDOUT; > + goto err; > + } ... > +err: err_unlock: > + mutex_unlock(&info->lock); > + > + return ret; ... > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = sun20i_gpadc_adc_read(info, chan, val); > + return ret; return sun...; > + case IIO_CHAN_INFO_SCALE: > + /* value in mv = 1800mV / 4096 raw */ > + *val = 1800; > + *val2 = 12; > + return IIO_VAL_FRACTIONAL_LOG2; > + default: > + return -EINVAL; > + } ... > + if (num_channels > SUN20I_GPADC_MAX_CHANNELS) { > + dev_err(dev, "num of channel children out of range"); > + return -EINVAL; > + } Is it really critical error? ... > + channels = devm_kcalloc(dev, num_channels, > + sizeof(*channels), GFP_KERNEL); At least one more parameter can be located on the previous line. > + if (!channels) > + return -ENOMEM; ... > +err_child_out: err_put_child: The goto labels should be self-explanatory of what to expect when goto. > + fwnode_handle_put(node); > + > + return ret; ... > + ret = devm_request_irq(dev, irq, sun20i_gpadc_irq_handler, > + 0, dev_name(dev), info); > + if (ret < 0) Here... > + return dev_err_probe(dev, ret, > + "failed requesting irq %d\n", irq); ... > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret < 0) ...and here, do the positive returned values even possible? > + return dev_err_probe(dev, ret, > + "could not register the device\n"); ... > + { .compatible = "allwinner,sun20i-d1-gpadc", }, Inner comma is not needed.
> > + if (!wait_for_completion_timeout(&info->completion, > > + msecs_to_jiffies(100))) { > > Dunno if it's better to have this parameter to be defined with self-explanatory > name. Probably a response to my earlier comment. I'd agree with a good name but GPADC_TIMEOUT which was the earlier naming is less use than a value and it's not obvious what that name should be. A nice datasheet reference would be good to have though. > > > + ret = -ETIMEDOUT; > > + goto err; > > + } > > > > + if (num_channels > SUN20I_GPADC_MAX_CHANNELS) { > > + dev_err(dev, "num of channel children out of range"); > > + return -EINVAL; > > + } > > Is it really critical error? Overflow of registers - so yes. I wondered this on v1 and went digging :) Now, there are no such devices known, so meh on whether check is useful. > > ...
On Fri, 2 Jun 2023 01:30:39 +0300 Maksim Kiselev <bigunclemax@gmail.com> wrote: > From: Maxim Kiselev <bigunclemax@gmail.com> > > 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. > > Theoretically, this ADC can support up to 16 channels. All SoCs below > contain this GPADC IP. The only difference between them is the number > of available channels: > > T113 - 1 channel > D1 - 2 channels > R329 - 4 channels > T507 - 4 channels > > Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com> Hi Maxim, A few more minor comments from me. Looking good in general though. > diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c > new file mode 100644 > index 000000000000..f4f1dcb06ea5 > --- /dev/null > +++ b/drivers/iio/adc/sun20i-gpadc-iio.c > @@ -0,0 +1,296 @@ > +// 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/bitfield.h> > +#include <linux/clk.h> > +#include <linux/completion.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/property.h> > +#include <linux/regmap.h> > +#include <linux/reset.h> > + > +#include <linux/iio/iio.h> > + > +#define SUN20I_GPADC_DRIVER_NAME "sun20i-gpadc" > + > +/* Register map definition */ > +#define SUN20I_GPADC_SR 0x00 > +#define SUN20I_GPADC_CTRL 0x04 > +#define SUN20I_GPADC_CS_EN 0x08 > +#define SUN20I_GPADC_FIFO_INTC 0x0c > +#define SUN20I_GPADC_FIFO_INTS 0x10 > +#define SUN20I_GPADC_FIFO_DATA 0X14 > +#define SUN20I_GPADC_CB_DATA 0X18 > +#define SUN20I_GPADC_DATAL_INTC 0x20 > +#define SUN20I_GPADC_DATAH_INTC 0x24 > +#define SUN20I_GPADC_DATA_INTC 0x28 > +#define SUN20I_GPADC_DATAL_INTS 0x30 > +#define SUN20I_GPADC_DATAH_INTS 0x34 > +#define SUN20I_GPADC_DATA_INTS 0x38 > +#define SUN20I_GPADC_CH_CMP_DATA(x) (0x40 + (x) * 4) > +#define SUN20I_GPADC_CH_DATA(x) (0x80 + (x) * 4) > + > +/* ADC bit shift */ Not sure what this comment means? I'd just drop it. > +#define SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN_MASK BIT(23) > +#define SUN20I_GPADC_CTRL_WORK_MODE_MASK GENMASK(19, 18) > +#define SUN20I_GPADC_CTRL_ADC_EN_MASK BIT(16) > +#define SUN20I_GPADC_CS_EN_ADC_CH(x) BIT(x) > +#define SUN20I_GPADC_DATA_INTC_CH_DATA_IRQ_EN(x) BIT(x) > +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: > + ret = sun20i_gpadc_adc_read(info, chan, val); > + return ret; return sun20i_gpadc_adc_read() and drop the local variable ret as no longer used. > + case IIO_CHAN_INFO_SCALE: > + /* value in mv = 1800mV / 4096 raw */ > + *val = 1800; > + *val2 = 12; > + return IIO_VAL_FRACTIONAL_LOG2; > + default: > + return -EINVAL; > + } > +} > +static int sun20i_gpadc_alloc_channels(struct iio_dev *indio_dev, > + struct device *dev) > +{ > + unsigned int channel; > + int num_channels, i, ret; > + struct iio_chan_spec *channels; > + struct fwnode_handle *node; > + > + num_channels = device_get_child_node_count(dev); > + if (num_channels == 0) { > + dev_err(dev, "no channel children"); > + return -ENODEV; > + } > + > + if (num_channels > SUN20I_GPADC_MAX_CHANNELS) { > + dev_err(dev, "num of channel children out of range"); > + return -EINVAL; > + } > + > + channels = devm_kcalloc(dev, num_channels, > + sizeof(*channels), GFP_KERNEL); > + if (!channels) > + return -ENOMEM; > + > + i = 0; > + device_for_each_child_node(dev, node) { > + ret = fwnode_property_read_u32(node, "reg", &channel); > + if (ret) > + goto err_child_out; You are fairly verbose on error messages elsewhere - which is somewhat of a personal choice, but if you are going to do that I'd expect to see on here as well. > + > + channels[i].type = IIO_VOLTAGE; > + channels[i].indexed = 1; > + channels[i].channel = channel; > + channels[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW); > + channels[i].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE); > + > + i++; > + } > + > + indio_dev->channels = channels; > + indio_dev->num_channels = num_channels; > + > + return 0; > + > +err_child_out: > + fwnode_handle_put(node); > + > + return ret; > +} > + > +static int sun20i_gpadc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + 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(dev, sizeof(*info)); > + if (!indio_dev) > + return -ENOMEM; > + > + info = iio_priv(indio_dev); > + info->lastch = -1; That naming briefly had me confused as my brain tried to figure it out as a typo :). Perhaps last_ch is clearer? Or just go with last_channel and be really clear. > + > + mutex_init(&info->lock); > + init_completion(&info->completion); > + > + ret = sun20i_gpadc_alloc_channels(indio_dev, dev); > + if (ret) > + return ret; > + > + indio_dev->info = &sun20i_gpadc_iio_info; > + indio_dev->name = SUN20I_GPADC_DRIVER_NAME; We try to make this name identify the chip in question. If the driver name is sufficient for these platforms then fair enough. It should certainly be enough to distinguish this from other ADCs on the platform.
пт, 2 июн. 2023 г. в 17:49, Andy Shevchenko <andriy.shevchenko@linux.intel.com>: ... > The locks should be explained (what are they for? what do they protect?). I added the explanation comment in the next version. ... > > +static const struct regmap_config sun20i_gpadc_regmap_config = { > > + .reg_bits = 32, > > + .val_bits = 32, > > + .reg_stride = 4, > > + .fast_io = true, > > I forgot if I asked about regmap lock do you need it? I think we could drop the regmap altogether. As Andy suggested in previous series. ... > > + if (num_channels > SUN20I_GPADC_MAX_CHANNELS) { > > + dev_err(dev, "num of channel children out of range"); > > + return -EINVAL; > > + } > > Is it really critical error? Yes, as Jonathan already noted, this may lead to out of range error. вс, 4 июн. 2023 г. в 13:46, Jonathan Cameron <jic23@kernel.org>: ... > We try to make this name identify the chip in question. > If the driver name is sufficient for these platforms then fair enough. > It should certainly be enough to distinguish this from other ADCs on the > platform. I believe the driver name should be enough. All listed SoCs have the same GPADC register layout and differ only in the number of channels.
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index eb2b09ef5d5b..deff7ae704ce 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, T507 and R329) + SoCs GPADC. This ADC provides up to 16 channels. + + 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/Makefile b/drivers/iio/adc/Makefile index e07e4a3e6237..fc4ef71d5f8f 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -95,6 +95,7 @@ obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o obj-$(CONFIG_SPEAR_ADC) += spear_adc.o obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o +obj-$(CONFIG_SUN20I_GPADC) += sun20i-gpadc-iio.o obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o obj-$(CONFIG_STM32_ADC) += stm32-adc.o obj-$(CONFIG_STM32_DFSDM_CORE) += stm32-dfsdm-core.o diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c new file mode 100644 index 000000000000..f4f1dcb06ea5 --- /dev/null +++ b/drivers/iio/adc/sun20i-gpadc-iio.c @@ -0,0 +1,296 @@ +// 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/bitfield.h> +#include <linux/clk.h> +#include <linux/completion.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/property.h> +#include <linux/regmap.h> +#include <linux/reset.h> + +#include <linux/iio/iio.h> + +#define SUN20I_GPADC_DRIVER_NAME "sun20i-gpadc" + +/* Register map definition */ +#define SUN20I_GPADC_SR 0x00 +#define SUN20I_GPADC_CTRL 0x04 +#define SUN20I_GPADC_CS_EN 0x08 +#define SUN20I_GPADC_FIFO_INTC 0x0c +#define SUN20I_GPADC_FIFO_INTS 0x10 +#define SUN20I_GPADC_FIFO_DATA 0X14 +#define SUN20I_GPADC_CB_DATA 0X18 +#define SUN20I_GPADC_DATAL_INTC 0x20 +#define SUN20I_GPADC_DATAH_INTC 0x24 +#define SUN20I_GPADC_DATA_INTC 0x28 +#define SUN20I_GPADC_DATAL_INTS 0x30 +#define SUN20I_GPADC_DATAH_INTS 0x34 +#define SUN20I_GPADC_DATA_INTS 0x38 +#define SUN20I_GPADC_CH_CMP_DATA(x) (0x40 + (x) * 4) +#define SUN20I_GPADC_CH_DATA(x) (0x80 + (x) * 4) + +/* ADC bit shift */ +#define SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN_MASK BIT(23) +#define SUN20I_GPADC_CTRL_WORK_MODE_MASK GENMASK(19, 18) +#define SUN20I_GPADC_CTRL_ADC_EN_MASK BIT(16) +#define SUN20I_GPADC_CS_EN_ADC_CH(x) BIT(x) +#define SUN20I_GPADC_DATA_INTC_CH_DATA_IRQ_EN(x) BIT(x) + +/* ADC parameters */ +#define SUN20I_GPADC_WORK_MODE_SINGLE 0 + +#define SUN20I_GPADC_MAX_CHANNELS 16 + +struct sun20i_gpadc_iio { + struct regmap *regmap; + struct completion completion; + struct mutex lock; + int lastch; +}; + +static const struct regmap_config sun20i_gpadc_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, + .fast_io = true, +}; + +static int sun20i_gpadc_adc_read(struct sun20i_gpadc_iio *info, + struct iio_chan_spec const *chan, int *val) +{ + int ret = IIO_VAL_INT; + + mutex_lock(&info->lock); + + reinit_completion(&info->completion); + + if (info->lastch != chan->channel) { + info->lastch = chan->channel; + + /* enable the analog input channel */ + regmap_write(info->regmap, SUN20I_GPADC_CS_EN, + SUN20I_GPADC_CS_EN_ADC_CH(chan->channel)); + + /* enable the data irq for input channel */ + regmap_write(info->regmap, SUN20I_GPADC_DATA_INTC, + SUN20I_GPADC_DATA_INTC_CH_DATA_IRQ_EN(chan->channel)); + } + + /* enable the ADC function */ + regmap_update_bits(info->regmap, SUN20I_GPADC_CTRL, + SUN20I_GPADC_CTRL_ADC_EN_MASK, SUN20I_GPADC_CTRL_ADC_EN_MASK); + + if (!wait_for_completion_timeout(&info->completion, + msecs_to_jiffies(100))) { + ret = -ETIMEDOUT; + goto err; + } + + /* read the ADC data */ + regmap_read(info->regmap, SUN20I_GPADC_CH_DATA(chan->channel), val); + +err: + mutex_unlock(&info->lock); + + return ret; +} + +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: + ret = sun20i_gpadc_adc_read(info, chan, val); + return ret; + case IIO_CHAN_INFO_SCALE: + /* value in mv = 1800mV / 4096 raw */ + *val = 1800; + *val2 = 12; + return IIO_VAL_FRACTIONAL_LOG2; + default: + return -EINVAL; + } +} + +static irqreturn_t sun20i_gpadc_irq_handler(int irq, void *data) +{ + struct sun20i_gpadc_iio *info = data; + + /* clear data interrupt status register */ + regmap_write(info->regmap, SUN20I_GPADC_DATA_INTS, ~0); + + complete(&info->completion); + + return IRQ_HANDLED; +} + +static const struct iio_info sun20i_gpadc_iio_info = { + .read_raw = sun20i_gpadc_read_raw, +}; + +static void sun20i_gpadc_reset_assert(void *data) +{ + struct reset_control *rst = data; + + reset_control_assert(rst); +} + +static int sun20i_gpadc_alloc_channels(struct iio_dev *indio_dev, + struct device *dev) +{ + unsigned int channel; + int num_channels, i, ret; + struct iio_chan_spec *channels; + struct fwnode_handle *node; + + num_channels = device_get_child_node_count(dev); + if (num_channels == 0) { + dev_err(dev, "no channel children"); + return -ENODEV; + } + + if (num_channels > SUN20I_GPADC_MAX_CHANNELS) { + dev_err(dev, "num of channel children out of range"); + return -EINVAL; + } + + channels = devm_kcalloc(dev, num_channels, + sizeof(*channels), GFP_KERNEL); + if (!channels) + return -ENOMEM; + + i = 0; + device_for_each_child_node(dev, node) { + ret = fwnode_property_read_u32(node, "reg", &channel); + if (ret) + goto err_child_out; + + channels[i].type = IIO_VOLTAGE; + channels[i].indexed = 1; + channels[i].channel = channel; + channels[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW); + channels[i].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE); + + i++; + } + + indio_dev->channels = channels; + indio_dev->num_channels = num_channels; + + return 0; + +err_child_out: + fwnode_handle_put(node); + + return ret; +} + +static int sun20i_gpadc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + 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(dev, sizeof(*info)); + if (!indio_dev) + return -ENOMEM; + + info = iio_priv(indio_dev); + info->lastch = -1; + + mutex_init(&info->lock); + init_completion(&info->completion); + + ret = sun20i_gpadc_alloc_channels(indio_dev, dev); + if (ret) + return ret; + + indio_dev->info = &sun20i_gpadc_iio_info; + indio_dev->name = SUN20I_GPADC_DRIVER_NAME; + + base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(base)) + return PTR_ERR(base); + + info->regmap = devm_regmap_init_mmio(dev, base, + &sun20i_gpadc_regmap_config); + if (IS_ERR(info->regmap)) + return dev_err_probe(dev, PTR_ERR(info->regmap), + "failed to init regmap\n"); + + clk = devm_clk_get_enabled(dev, NULL); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), + "failed to enable bus clock\n"); + + rst = devm_reset_control_get_exclusive(dev, NULL); + if (IS_ERR(rst)) + return dev_err_probe(dev, PTR_ERR(rst), + "failed to get reset control\n"); + + ret = reset_control_deassert(rst); + if (ret) + return dev_err_probe(dev, ret, + "failed to deassert reset\n"); + + ret = devm_add_action_or_reset(dev, sun20i_gpadc_reset_assert, rst); + if (ret) + return ret; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + ret = devm_request_irq(dev, irq, sun20i_gpadc_irq_handler, + 0, dev_name(dev), info); + if (ret < 0) + return dev_err_probe(dev, ret, + "failed requesting irq %d\n", irq); + + regmap_write(info->regmap, SUN20I_GPADC_CTRL, + FIELD_PREP(SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN_MASK, 1) | + FIELD_PREP(SUN20I_GPADC_CTRL_WORK_MODE_MASK, SUN20I_GPADC_WORK_MODE_SINGLE)); + + ret = devm_iio_device_register(dev, indio_dev); + if (ret < 0) + return dev_err_probe(dev, ret, + "could not register the device\n"); + + return 0; +} + +static const struct of_device_id sun20i_gpadc_of_id[] = { + { .compatible = "allwinner,sun20i-d1-gpadc", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, sun20i_gpadc_of_id); + +static struct platform_driver sun20i_gpadc_driver = { + .driver = { + .name = SUN20I_GPADC_DRIVER_NAME, + .of_match_table = sun20i_gpadc_of_id, + }, + .probe = sun20i_gpadc_probe, +}; +module_platform_driver(sun20i_gpadc_driver); + +MODULE_DESCRIPTION("ADC driver for sunxi platforms"); +MODULE_AUTHOR("Maksim Kiselev <bigunclemax@gmail.com>"); +MODULE_LICENSE("GPL");