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

[Nuno Sá]

On Fri, 2026-02-20 at 13:00 +0200, Andy Shevchenko wrote:
> 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.

Because the compiler can then optimize away all of the above code...

- Nuno Sá

> 
> > > +	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.
> 
> > > +};