| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d19e7b07-369f-4909-b931-af881a2a4425@wanadoo.fr>
Date: Sun, 10 Aug 2025 19:35:19 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>, jic23@...nel.org,
robh@...nel.org, conor+dt@...nel.org, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 3/4] iio: adc: add ade9000 support
Le 08/08/2025 à 16:10, Antoniu Miclaus a écrit :
> Add driver support for the ade9000. highly accurate,
> fully integrated, multiphase energy and power quality
> monitoring device.
Hi,
...
> +static int ade9000_clkout_prepare(struct clk_hw *hw)
> +{
> + return 0;
> +}
> +
> +static void ade9000_clkout_unprepare(struct clk_hw *hw)
> +{
> +}
Is it really needed to have these 2 empty functions?
clk core seems to accept NULL for .prepare and .unprepare.
> +
> +static unsigned long ade9000_clkout_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + /* CLKOUT provides the same frequency as the crystal/external clock */
> + return parent_rate ? parent_rate : 24576000; /* Default 24.576 MHz */
> +}
> +
> +static const struct clk_ops ade9000_clkout_ops = {
> + .prepare = ade9000_clkout_prepare,
> + .unprepare = ade9000_clkout_unprepare,
> + .recalc_rate = ade9000_clkout_recalc_rate,
> +};
...
> +static irqreturn_t ade9000_irq0_thread(int irq, void *data)
> +{
> + struct iio_dev *indio_dev = data;
> + struct ade9000_state *st = iio_priv(indio_dev);
> + s64 timestamp = iio_get_time_ns(indio_dev);
> + u32 handled_irq = 0;
> + u32 interrupts;
> + u32 status;
> + int ret;
> +
> + ret = regmap_read(st->regmap, ADE9000_REG_STATUS0, &status);
> + if (ret) {
> + dev_err(&st->spi->dev, "IRQ0 read status fail");
> + return IRQ_HANDLED;
> + }
> +
> + ret = regmap_read(st->regmap, ADE9000_REG_MASK0, &interrupts);
> + if (ret) {
> + dev_err(&st->spi->dev, "IRQ0 read status fail");
s/status/interrupts/ maybe?
> + return IRQ_HANDLED;
> + }
> +
> + if ((status & ADE9000_ST0_PAGE_FULL_BIT) &&
> + (interrupts & ADE9000_ST0_PAGE_FULL_BIT)) {
> + /* Always use streaming mode */
> + ret = ade9000_iio_push_streaming(indio_dev);
> + if (ret) {
> + dev_err(&st->spi->dev, "IRQ0 IIO push fail");
> + return IRQ_HANDLED;
> + }
> +
> + handled_irq |= ADE9000_ST0_PAGE_FULL_BIT;
> + }
> +
> + if ((status & ADE9000_ST0_WFB_TRIG_BIT) &&
> + (interrupts & ADE9000_ST0_WFB_TRIG_BIT)) {
> + ret = ade9000_waveform_buffer_en(st, false);
> + if (ret) {
> + dev_err(&st->spi->dev, "IRQ0 WFB fail");
> + return IRQ_HANDLED;
> + }
> +
> + ret = ade9000_iio_push_buffer(indio_dev);
> + if (ret) {
> + dev_err(&st->spi->dev, "IRQ0 IIO push fail @ WFB TRIG");
> + return IRQ_HANDLED;
> + }
> +
> + handled_irq |= ADE9000_ST0_WFB_TRIG_BIT;
> + }
> +
> + if ((status & ADE9000_ST0_EGYRDY) &&
> + (interrupts & ADE9000_ST0_EGYRDY)) {
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_ENERGY,
> + ADE9000_ST0_EGYRDY,
> + IIO_EV_TYPE_MAG,
> + IIO_EV_DIR_NONE),
> + timestamp);
> +
> + handled_irq |= ADE9000_ST0_EGYRDY;
> + }
> +
> + ret = regmap_write(st->regmap, ADE9000_REG_STATUS0, handled_irq);
> + if (ret)
> + dev_err(&st->spi->dev, "IRQ0 write status fail");
> +
> + return IRQ_HANDLED;
> +}
...
> +static int ade9000_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct ade9000_state *st;
> + struct regmap *regmap;
> + int irq;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return dev_err_probe(dev, -ENOMEM, "Unable to allocate ADE9000 IIO");
> + st = iio_priv(indio_dev);
> +
> + regmap = devm_regmap_init(dev, NULL, spi, &ade9000_regmap_config);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap), "Unable to allocate ADE9000 regmap");
> + spi_set_drvdata(spi, st);
> +
> + irq = fwnode_irq_get_byname(dev_fwnode(dev), "irq0");
> + if (irq < 0)
> + return dev_err_probe(dev, -EINVAL, "Unable to find irq0");
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL,
> + ade9000_irq0_thread,
> + IRQF_ONESHOT,
> + KBUILD_MODNAME, indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to request threaded irq: %d", ret);
> +
> + irq = fwnode_irq_get_byname(dev_fwnode(dev), "irq1");
> + if (irq < 0)
> + return dev_err_probe(dev, -EINVAL, "Unable to find irq1");
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL,
> + ade9000_irq1_thread,
> + IRQF_ONESHOT,
> + KBUILD_MODNAME, indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to request threaded irq: %d", ret);
> +
> + irq = fwnode_irq_get_byname(dev_fwnode(dev), "dready");
> + if (irq < 0)
> + return dev_err_probe(dev, -EINVAL, "Unable to find dready");
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL,
> + ade9000_dready_thread,
> + IRQF_ONESHOT,
> + KBUILD_MODNAME, indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to request threaded irq: %d", ret);
> +
> + st->spi = spi;
> +
> + /* Optional external clock input */
> + st->clkin = devm_clk_get_optional_enabled(dev, "clkin");
> + if (IS_ERR(st->clkin))
> + return dev_err_probe(dev, PTR_ERR(st->clkin), "Failed to get and enable clkin: %ld", PTR_ERR(st->clkin));
No need to dupliacte PTR_ERR(clkin) in the error message. Same for
several messages above.
> +
> + /* Register clock output provider */
> + if (device_property_present(dev, "#clock-cells")) {
> + struct clk_init_data clk_init = {};
> + struct clk *clkout;
> + unsigned long parent_rate = 24576000; /* Default crystal frequency */
> +
> + if (st->clkin)
> + parent_rate = clk_get_rate(st->clkin);
parent_rate seems to be unused.
> +
> + clk_init.name = "clkout";
> + clk_init.ops = &ade9000_clkout_ops;
> + clk_init.flags = CLK_GET_RATE_NOCACHE;
> + clk_init.num_parents = 0;
Would it make sense to have clk_init a "static const struct
clk_init_data" defined outside of the probe to have the code be less
verbose?
> +
> + st->clkout_hw.init = &clk_init;
> +
> + clkout = devm_clk_register(dev, &st->clkout_hw);
> + if (IS_ERR(clkout))
> + return dev_err_probe(dev, PTR_ERR(clkout), "Failed to register clkout: %ld", PTR_ERR(clkout));
No need to dupliacte PTR_ERR(clkout) in the error message.
> +
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &st->clkout_hw);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to add clock provider: %d", ret);
No need to dupliacte ret in the error message.
> + }
> +
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->dev.parent = &st->spi->dev;
> + indio_dev->info = &ade9000_info;
> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> + indio_dev->setup_ops = &ade9000_buffer_ops;
> +
> + st->regmap = regmap;
> +
> + ret = devm_regulator_get_enable(&spi->dev, "vdd");
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to get and enable vdd regulator\n");
> +
> + ret = devm_regulator_get_enable_optional(dev, "vref");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret,
> + "Failed to get and enable vref regulator\n");
> +
> + /* Configure reference selection based on vref regulator availability */
> + if (ret != -ENODEV) {
> + ret = regmap_update_bits(st->regmap, ADE9000_REG_CONFIG1,
> + ADE9000_EXT_REF_MASK,
> + ADE9000_EXT_REF_MASK);
> + if (ret)
> + return ret;
> + }
> +
> + indio_dev->channels = ade9000_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ade9000_channels);
> + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> + &ade9000_buffer_ops);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to setup IIO buffer");
> +
> + ret = ade9000_reset(st);
> + if (ret)
> + return dev_err_probe(dev, ret, "ADE9000 reset failed");
> +
> + ret = ade9000_setup(st);
> + if (ret)
> + return dev_err_probe(dev, ret, "Unable to setup ADE9000");
Missing \n at the end of the message. Same for some other messages above
using dev_err_probe() or dev_err().
> +
> + return devm_iio_device_register(dev, indio_dev);
> +};
...
CJ
Powered by blists - more mailing lists