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

Andy Shevchenko
2026-02-20T11:00:24Z

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