Re: [PATCH v3 3/4] iio: adc: add ade9000 support

[Christophe JAILLET]

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