| Message ID | 20240702-sg2002-adc-v1-2-ac66e076a756@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add SARADC support on Sophgo SoC | expand |
On Tue, 02 Jul 2024 13:52:22 +0200 Thomas Bonnefille <thomas.bonnefille@bootlin.com> wrote: > This adds a driver for the common Sophgo SARADC. > > Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com> Hi Thomas, Welcome to IIO. Various comments inline Thanks, Jonathan > diff --git a/drivers/iio/adc/sophgo-adc.c b/drivers/iio/adc/sophgo-adc.c > new file mode 100644 > index 000000000000..a94d839d40ec > --- /dev/null > +++ b/drivers/iio/adc/sophgo-adc.c > @@ -0,0 +1,223 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Sophgo SARADC Driver > + * > + * Copyright (C) Bootlin 2024 > + * Author: Thomas Bonnefille <thomas.bonnefille@bootlin.com> > + */ > + > +#include <linux/clk.h> > +#include <linux/completion.h> > +#include <linux/dev_printk.h> > +#include <linux/interrupt.h> > +#include <linux/iio/iio.h> > +#include <linux/iopoll.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> probably want mod_devicetable.h instead. > +#include <linux/platform_device.h> > + > +#define SOPHGO_SARADC_CTRL_REG 0x04 > +#define SOPHGO_SARADC_EN BIT(0) > +#define SOPHGO_SARADC_SEL(x) BIT((x)+4) > +#define SOPHGO_SARADC_STATUS_REG 0x08 > +#define SOPHGO_SARADC_BUSY BIT(0) > +#define SOPHGO_SARADC_CYC_SET_REG 0x0C > +#define SOPHGO_SARADC_DEF_CYC_SETTINGS 0xF1F0F > +#define SOPHGO_SARADC_CH_RESULT_REG(x) (0x10+4*(x)) > +#define SARADC_CH_RESULT(x) ((x) & 0xfff) > +#define SARADC_CH_VALID(x) ((x) & BIT(15)) Prefer defined masks and FIELD_GET() to actually get the values. Also use consistent prefix for defines. > +#define SOPHGO_SARADC_INTR_EN_REG 0x20 > +#define SOPHGO_SARADC_INTR_CLR_REG 0x24 > + > +#define SOPHGO_SARADC_CHANNEL(index) \ > + { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = index, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_index = index, \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 12, \ No need to define these if not providing buffered capture. > + }, \ > + } > + > +struct sophgo_saradc { > + struct completion completion; > + struct iio_info info; > + void __iomem *regs; > + struct mutex lock; Add a comment on what data this lock protects. > + struct clk *clk; After changes below, probably don't need to hang on to the clk. > + int irq; Rarely need to keep hold of the irq after probe. Use a local variable for it in instead. > +}; > + > +static const struct iio_chan_spec sophgo_channels[] = { > + SOPHGO_SARADC_CHANNEL(1), > + SOPHGO_SARADC_CHANNEL(2), > + SOPHGO_SARADC_CHANNEL(3), > +}; > + > +static void sophgo_saradc_start_measurement(struct sophgo_saradc *saradc, > + int channel) > +{ > + writel(0, saradc->regs + SOPHGO_SARADC_CTRL_REG); > + writel(SOPHGO_SARADC_SEL(channel) | SOPHGO_SARADC_EN, > + saradc->regs + SOPHGO_SARADC_CTRL_REG); > +} > + > +static int sophgo_saradc_wait(struct sophgo_saradc *saradc) > +{ > + if (saradc->irq < 0) { > + u32 reg; > + > + return readl_poll_timeout(saradc->regs + SOPHGO_SARADC_STATUS_REG, > + reg, !(reg & SOPHGO_SARADC_BUSY), > + 500, 1000000); > + } else { Returned above, so no need for else which helps with indent. > + int ret; > + > + ret = wait_for_completion_timeout(&saradc->completion, > + msecs_to_jiffies(1000)) > 0 > + ? 0 : -ETIMEDOUT; > + return ret; return wait_for_completion. > + } > +} > + > +static int sophgo_saradc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + struct sophgo_saradc *saradc = iio_priv(indio_dev); > + u32 sample; > + int ret; > + > + mutex_lock(&saradc->lock); Use scoped_guard() here so you can return directly on error. > + sophgo_saradc_start_measurement(saradc, chan->scan_index); > + ret = sophgo_saradc_wait(saradc); > + if (ret < 0) { > + mutex_unlock(&saradc->lock); > + return ret; > + } > + > + sample = readl(saradc->regs + SOPHGO_SARADC_CH_RESULT_REG(chan->scan_index)); > + mutex_unlock(&saradc->lock); > + > + if (SARADC_CH_VALID(sample)) { > + *val = SARADC_CH_RESULT(sample); > + return IIO_VAL_INT; > + } errors out of line preferred. if (!SARADC_CH_VALID(sample)) return -ENODATA; *val... > + return -ENODATA; > + case IIO_CHAN_INFO_SCALE: > + *val = 3300; > + *val2 = 12; > + return IIO_VAL_FRACTIONAL_LOG2; > + default: > + return -EINVAL; > + } > +} > + > +static irqreturn_t sophgo_saradc_interrupt_handler(int irq, void *dev_id) > +{ > + struct sophgo_saradc *saradc = dev_id; dev_id is an odd name for something that isn't an id. No register to confirm the interupt status? Fine if not, but relatively unusual. > + > + writel(1, saradc->regs + SOPHGO_SARADC_INTR_CLR_REG); > + complete(&saradc->completion); > + return IRQ_HANDLED; > +} > + Single blank line > + > +static const struct of_device_id sophgo_saradc_match[] = { > + { .compatible = "sophgo,cv18xx-saradc", }, > + { }, No comma needed on 'null' terminators like this as we won't ever add anything after them. > +}; > +MODULE_DEVICE_TABLE(of, sophgo_saradc_match); > + > +static int sophgo_saradc_probe(struct platform_device *pdev) > +{ > + struct sophgo_saradc *saradc; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*saradc)); > + if (!indio_dev) > + return -ENOMEM; > + > + saradc = iio_priv(indio_dev); > + indio_dev->name = "Sophgo SARADC"; Hmm. Whilst the ABI docs don't say it, I don't think we have names with either capitals or spaces. sophgo-saradc or similar preferred. > + indio_dev->info = &saradc->info; This is making some static const data dynamic. I can't see why you'd do this > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->num_channels = 3; ARRAY_SIZE(sophgo_channels) > + indio_dev->channels = sophgo_channels; > + > + saradc->clk = devm_clk_get(&pdev->dev, NULL); If it's optional, then use dev_clk_get_optional() > + if (IS_ERR(saradc->clk)) { > + dev_dbg(&pdev->dev, "Can't get clock from device tree, using No-Die domain"); > + } else { > + ret = clk_prepare_enable(saradc->clk); Perhaps devm_clk_get_optional_enabled() is appropriate, then you can use null return to detect that it wasn't present - vs an ERR_PTR() for it was but didn't enable. > + if (ret) > + return ret; > + } > + > + saradc->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(saradc->regs)) { > + ret = PTR_ERR(saradc->regs); > + goto error_disable_clock; > + } > + > + saradc->irq = platform_get_irq_optional(pdev, 0); > + if (saradc->irq >= 0) { > + ret = devm_request_irq(&pdev->dev, saradc->irq, > + sophgo_saradc_interrupt_handler, 0, > + dev_name(&pdev->dev), saradc); > + if (ret) > + goto error_disable_clock; > + > + writel(1, saradc->regs + SOPHGO_SARADC_INTR_EN_REG); > + > + init_completion(&saradc->completion); Usually best to init a completion that is irq related before requesting the irq. Avoids any possible races. > + } > + > + saradc->info.read_raw = &sophgo_saradc_read_raw; Why? > + > + mutex_init(&saradc->lock); > + platform_set_drvdata(pdev, indio_dev); > + writel(SOPHGO_SARADC_DEF_CYC_SETTINGS, saradc->regs + SOPHGO_SARADC_CYC_SET_REG); > + ret = devm_iio_device_register(&pdev->dev, indio_dev); > + if (ret) > + goto error_disable_clock; > + > + return 0; > + > +error_disable_clock: > + if (!IS_ERR(saradc->clk)) Don't mix and match devm and non devm management. As above, I think you can just use devm_clk_get_optional_enabled() > + clk_disable_unprepare(saradc->clk); > + return ret; > +} > + > +static void sophgo_saradc_remove(struct platform_device *pdev) > +{ > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + struct sophgo_saradc *saradc = iio_priv(indio_dev); > + > + if (!IS_ERR(saradc->clk)) > + clk_disable_unprepare(saradc->clk); > +} > + Single blank line is enough. > + > +static struct platform_driver sophgo_saradc_driver = { > + .driver = { > + .name = "sophgo-saradc", > + .of_match_table = sophgo_saradc_match, > + }, > + .probe = sophgo_saradc_probe, > + .remove_new = sophgo_saradc_remove, > +}; > +module_platform_driver(sophgo_saradc_driver); > + > +MODULE_AUTHOR("Thomas Bonnefille <thomas.bonnefille@bootlin.com>"); > +MODULE_DESCRIPTION("Sophgo SARADC driver"); > +MODULE_LICENSE("GPL"); >
On 2024/7/2 19:52, Thomas Bonnefille wrote: [......] > SOUND > M: Jaroslav Kysela <perex@perex.cz> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 8db68b80b391..826871a2e61a 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -1122,6 +1122,15 @@ config SC27XX_ADC > This driver can also be built as a module. If so, the module > will be called sc27xx_adc. > > +config SOPHGO_ADC > + tristate "Sophgo ADC" > + depends on ARCH_SOPHGO || COMPILE_TEST > + help > + Say yes here to build support for the ADC integrated in Sophgo SoCs. > + > + This driver can also be built as a module. If so, the module > + will be called sophgo_adc. > + I believe this adc driver is only for sophgo cv18xx, sophgo has other soc chipset, such as sg2024 etc., so it's better use add more limitation for this. > config SPEAR_ADC > tristate "ST SPEAr ADC" > depends on PLAT_SPEAR || COMPILE_TEST > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index edb32ce2af02..106a83d50d01 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -102,6 +102,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o > obj-$(CONFIG_RICHTEK_RTQ6056) += rtq6056.o > obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o > obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o > +obj-$(CONFIG_SOPHGO_ADC) += sophgo-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 > diff --git a/drivers/iio/adc/sophgo-adc.c b/drivers/iio/adc/sophgo-adc.c As I mentioned upon, soghgo has many other product code, and "sophgo-cv18xx-adc.c" should be more accurate. [......]
diff --git a/MAINTAINERS b/MAINTAINERS index dc87898c518c..fc74eeaf5678 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20905,6 +20905,7 @@ SOPHGO SARADC DEVICE DRIVER M: Thomas Bonnefille <thomas.bonnefille@bootlin.com> S: Maintained F: Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml +F: drivers/iio/adc/sophgo-adc.c SOUND M: Jaroslav Kysela <perex@perex.cz> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 8db68b80b391..826871a2e61a 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -1122,6 +1122,15 @@ config SC27XX_ADC This driver can also be built as a module. If so, the module will be called sc27xx_adc. +config SOPHGO_ADC + tristate "Sophgo ADC" + depends on ARCH_SOPHGO || COMPILE_TEST + help + Say yes here to build support for the ADC integrated in Sophgo SoCs. + + This driver can also be built as a module. If so, the module + will be called sophgo_adc. + config SPEAR_ADC tristate "ST SPEAr ADC" depends on PLAT_SPEAR || COMPILE_TEST diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index edb32ce2af02..106a83d50d01 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -102,6 +102,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o obj-$(CONFIG_RICHTEK_RTQ6056) += rtq6056.o obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o +obj-$(CONFIG_SOPHGO_ADC) += sophgo-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 diff --git a/drivers/iio/adc/sophgo-adc.c b/drivers/iio/adc/sophgo-adc.c new file mode 100644 index 000000000000..a94d839d40ec --- /dev/null +++ b/drivers/iio/adc/sophgo-adc.c @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Sophgo SARADC Driver + * + * Copyright (C) Bootlin 2024 + * Author: Thomas Bonnefille <thomas.bonnefille@bootlin.com> + */ + +#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/dev_printk.h> +#include <linux/interrupt.h> +#include <linux/iio/iio.h> +#include <linux/iopoll.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/platform_device.h> + +#define SOPHGO_SARADC_CTRL_REG 0x04 +#define SOPHGO_SARADC_EN BIT(0) +#define SOPHGO_SARADC_SEL(x) BIT((x)+4) +#define SOPHGO_SARADC_STATUS_REG 0x08 +#define SOPHGO_SARADC_BUSY BIT(0) +#define SOPHGO_SARADC_CYC_SET_REG 0x0C +#define SOPHGO_SARADC_DEF_CYC_SETTINGS 0xF1F0F +#define SOPHGO_SARADC_CH_RESULT_REG(x) (0x10+4*(x)) +#define SARADC_CH_RESULT(x) ((x) & 0xfff) +#define SARADC_CH_VALID(x) ((x) & BIT(15)) +#define SOPHGO_SARADC_INTR_EN_REG 0x20 +#define SOPHGO_SARADC_INTR_CLR_REG 0x24 + +#define SOPHGO_SARADC_CHANNEL(index) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = index, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .scan_index = index, \ + .scan_type = { \ + .sign = 'u', \ + .realbits = 12, \ + }, \ + } + +struct sophgo_saradc { + struct completion completion; + struct iio_info info; + void __iomem *regs; + struct mutex lock; + struct clk *clk; + int irq; +}; + +static const struct iio_chan_spec sophgo_channels[] = { + SOPHGO_SARADC_CHANNEL(1), + SOPHGO_SARADC_CHANNEL(2), + SOPHGO_SARADC_CHANNEL(3), +}; + +static void sophgo_saradc_start_measurement(struct sophgo_saradc *saradc, + int channel) +{ + writel(0, saradc->regs + SOPHGO_SARADC_CTRL_REG); + writel(SOPHGO_SARADC_SEL(channel) | SOPHGO_SARADC_EN, + saradc->regs + SOPHGO_SARADC_CTRL_REG); +} + +static int sophgo_saradc_wait(struct sophgo_saradc *saradc) +{ + if (saradc->irq < 0) { + u32 reg; + + return readl_poll_timeout(saradc->regs + SOPHGO_SARADC_STATUS_REG, + reg, !(reg & SOPHGO_SARADC_BUSY), + 500, 1000000); + } else { + int ret; + + ret = wait_for_completion_timeout(&saradc->completion, + msecs_to_jiffies(1000)) > 0 + ? 0 : -ETIMEDOUT; + return ret; + } +} + +static int sophgo_saradc_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_RAW: + struct sophgo_saradc *saradc = iio_priv(indio_dev); + u32 sample; + int ret; + + mutex_lock(&saradc->lock); + sophgo_saradc_start_measurement(saradc, chan->scan_index); + ret = sophgo_saradc_wait(saradc); + if (ret < 0) { + mutex_unlock(&saradc->lock); + return ret; + } + + sample = readl(saradc->regs + SOPHGO_SARADC_CH_RESULT_REG(chan->scan_index)); + mutex_unlock(&saradc->lock); + + if (SARADC_CH_VALID(sample)) { + *val = SARADC_CH_RESULT(sample); + return IIO_VAL_INT; + } + return -ENODATA; + case IIO_CHAN_INFO_SCALE: + *val = 3300; + *val2 = 12; + return IIO_VAL_FRACTIONAL_LOG2; + default: + return -EINVAL; + } +} + +static irqreturn_t sophgo_saradc_interrupt_handler(int irq, void *dev_id) +{ + struct sophgo_saradc *saradc = dev_id; + + writel(1, saradc->regs + SOPHGO_SARADC_INTR_CLR_REG); + complete(&saradc->completion); + return IRQ_HANDLED; +} + + +static const struct of_device_id sophgo_saradc_match[] = { + { .compatible = "sophgo,cv18xx-saradc", }, + { }, +}; +MODULE_DEVICE_TABLE(of, sophgo_saradc_match); + +static int sophgo_saradc_probe(struct platform_device *pdev) +{ + struct sophgo_saradc *saradc; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*saradc)); + if (!indio_dev) + return -ENOMEM; + + saradc = iio_priv(indio_dev); + indio_dev->name = "Sophgo SARADC"; + indio_dev->info = &saradc->info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->num_channels = 3; + indio_dev->channels = sophgo_channels; + + saradc->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(saradc->clk)) { + dev_dbg(&pdev->dev, "Can't get clock from device tree, using No-Die domain"); + } else { + ret = clk_prepare_enable(saradc->clk); + if (ret) + return ret; + } + + saradc->regs = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(saradc->regs)) { + ret = PTR_ERR(saradc->regs); + goto error_disable_clock; + } + + saradc->irq = platform_get_irq_optional(pdev, 0); + if (saradc->irq >= 0) { + ret = devm_request_irq(&pdev->dev, saradc->irq, + sophgo_saradc_interrupt_handler, 0, + dev_name(&pdev->dev), saradc); + if (ret) + goto error_disable_clock; + + writel(1, saradc->regs + SOPHGO_SARADC_INTR_EN_REG); + + init_completion(&saradc->completion); + } + + saradc->info.read_raw = &sophgo_saradc_read_raw; + + mutex_init(&saradc->lock); + platform_set_drvdata(pdev, indio_dev); + writel(SOPHGO_SARADC_DEF_CYC_SETTINGS, saradc->regs + SOPHGO_SARADC_CYC_SET_REG); + ret = devm_iio_device_register(&pdev->dev, indio_dev); + if (ret) + goto error_disable_clock; + + return 0; + +error_disable_clock: + if (!IS_ERR(saradc->clk)) + clk_disable_unprepare(saradc->clk); + return ret; +} + +static void sophgo_saradc_remove(struct platform_device *pdev) +{ + struct iio_dev *indio_dev = platform_get_drvdata(pdev); + struct sophgo_saradc *saradc = iio_priv(indio_dev); + + if (!IS_ERR(saradc->clk)) + clk_disable_unprepare(saradc->clk); +} + + +static struct platform_driver sophgo_saradc_driver = { + .driver = { + .name = "sophgo-saradc", + .of_match_table = sophgo_saradc_match, + }, + .probe = sophgo_saradc_probe, + .remove_new = sophgo_saradc_remove, +}; +module_platform_driver(sophgo_saradc_driver); + +MODULE_AUTHOR("Thomas Bonnefille <thomas.bonnefille@bootlin.com>"); +MODULE_DESCRIPTION("Sophgo SARADC driver"); +MODULE_LICENSE("GPL");
This adds a driver for the common Sophgo SARADC. Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com> --- MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 9 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/sophgo-adc.c | 223 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 234 insertions(+)