On Tue, 02 Jul 2024 13:52:22 +0200 Thomas Bonnefille <thomas.bonnefille@xxxxxxxxxxx> wrote: > This adds a driver for the common Sophgo SARADC. > > Signed-off-by: Thomas Bonnefille <thomas.bonnefille@xxxxxxxxxxx> 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@xxxxxxxxxxx> > + */ > + > +#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@xxxxxxxxxxx>"); > +MODULE_DESCRIPTION("Sophgo SARADC driver"); > +MODULE_LICENSE("GPL"); >