Re: [PATCH 2/3] iio: dac: ad5706r: Add support for AD5706R DAC

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Feb 20, 2026 at 10:48:59AM +0000, Nuno Sá wrote:
> On Fri, 2026-02-20 at 16:02 +0800, Alexis Czezar Torreno wrote:

...

> > +static void ad5706r_debugs_init(struct iio_dev *indio_dev)
> > +{
> > +	struct dentry *d = iio_get_debugfs_dentry(indio_dev);
> 
> It should have:
> 
> if (!IS_ENABLED(CONFIG_DEBUGFS))
> 	return

But why? The debugfs is a stub when disabled, nobody should do that
in the cases when the main purpose is not the debugfs code.

> > +	debugfs_create_file_unsafe("streaming_addr", 0600, d,
> > +				   indio_dev, &ad5706r_streaming_addr_fops);
> > +	debugfs_create_file_unsafe("streaming_len", 0600, d,
> > +				   indio_dev, &ad5706r_streaming_len_fops);
> > +	debugfs_create_file_unsafe("streaming_data", 0600, d,
> > +				   indio_dev, &ad5706r_streaming_data_fops);
> > +	debugfs_create_file_unsafe("streaming_reg_access", 0600, d,
> > +				   indio_dev, &ad5706r_streaming_reg_access_fops);
> > +	debugfs_create_file_unsafe("spi_speed_hz_write", 0600, d,
> > +				   indio_dev, &ad5706r_spi_speed_write_fops);
> > +	debugfs_create_file_unsafe("spi_speed_hz_read", 0600, d,
> > +				   indio_dev, &ad5706r_spi_speed_read_fops);
> > +}

...

> > +	/* Find which index has this register value */
> > +	for (i = 0; i < ARRAY_SIZE(mux_out_sel_reg_values); i++) {

	for (size_t i...)

> > +		if (mux_out_sel_reg_values[i] == reg_byte) {
> > +			st->mux_out_sel = i;
> > +			return i;  /* Return index, not register value */
> > +		}
> > +	}

...

> > +	return ret ? ret : len;

Use Elvis operator

	return ret ?: len;

...


> > +	{},

IIO has a style for terminator entry, along with confusing trailing comma.
If it's a sentinel, it must be one even at a compile time.

> > +};

...

> > +	st->debug_spi_speed_hz_write = 10000000;
> > +	st->debug_spi_speed_hz_read = 10000000;

units.h and other headers for your help

10 * HZ_PER_MHZ

...

> > +	st->sampling_frequency = 1000000;

In the similar way.

...

> > +	st->reference_volts = 2500;

2.5kV?! I think you mistakenly put volts where should be _mV

...

> > +	for (i = 0; i < 4; i++) {

Magic 4.

> > +		st->hw_active_edge[i] = HW_ACTIVE_EDGE_RISING_EDGE;
> > +		st->range_sel[i] = RANGE_SEL_50;
> > +		st->output_state[i] = OUTPUT_STATE_NORMAL_SW;
> > +		st->ldac_trigger_chn[i] = LDAC_TRIGGER_CHN_HW_TRIGGER;
> > +		st->toggle_trigger_chn[i] = TOGGLE_TRIGGER_CHN_HW_TRIGGER;
> > +		st->dither_trigger_chn[i] = DITHER_TRIGGER_CHN_HW_TRIGGER;
> > +		st->multi_dac_sel_ch[i] = MULTI_DAC_SEL_CH_EXCLUDE;

Hmm... Perhaps memsetXX()? But original loop with the defined iterator will be
okay:

	for (unsigned int i = 0; i < $MAGIC_CONST; i++) {

> > +	}

...

> > +	st->resetb_gpio = devm_gpiod_get_optional(dev, "dac-resetb", GPIOD_OUT_LOW);
> > +	if (IS_ERR(st->resetb_gpio)) {
> > +		return dev_err_probe(dev, PTR_ERR(st->resetb_gpio),
> > +				     "Failed to get RESET_B GPIO\n");
> > +	}

> > +	st->shdn_gpio = devm_gpiod_get_optional(dev, "dac-shdn", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(st->shdn_gpio)) {
> > +		return dev_err_probe(dev, PTR_ERR(st->shdn_gpio),
> > +				     "Failed to get SHDN GPIO\n");
> > +	}

The {} are not needed when the body is a single call.

...

> > +static const struct of_device_id ad5706r_of_match[] = {
> > +	{ .compatible = "adi,ad5706r" },
> > +	{ },

See above about terminator entry style.

> > +};

-- 
With Best Regards,
Andy Shevchenko