Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 9 Sep 2024 09:51:35 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 9/9/24 4:47 AM, Tinaco, Mariel wrote:
> > 
> >   
> 
> ...
> 
> >>> +	*val = get_unaligned_le16(state->spi_tx_buf);  
> >>
> >> With spi_tx_buf changed to __le16, this can use le16_to_cpu() instead of
> >> get_unaligned_le16().
> >>  
> > 
> > I suppose we use the cpu_to_le16 for the set_hvdac_word function?
> > 
> > static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val)
> > {
> > 	state->spi_tx_buf = cpu_to_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val));
> > 
> > 	return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD(index),
> > 				 &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH);
> > }
> >   
> 
> Yes, that looks correct.
> 
> 
> >>> +static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev, uintptr_t  
> >> private,  
> >>> +				      const struct iio_chan_spec *chan,
> >>> +				      const char *buf, size_t len) {
> >>> +	struct ad8460_state *state = iio_priv(indio_dev);
> >>> +	bool toggle_en;
> >>> +	int ret;
> >>> +
> >>> +	ret = kstrtobool(buf, &toggle_en);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> >>> +		return ad8460_enable_apg_mode(state, toggle_en);
> >>> +	unreachable();
> >>> +}  
> >>
> >> Hmm... do we need to make an unscoped version of
> >> iio_device_claim_direct_scoped()?
> >>  
> > 
> > So iio_device_claim_direct_scoped is used here because the buffer enable/disable
> > accesses this enable_apg_mode function. Is it also a standard practice to put the
> > kstrobool() util inside the scope?
> >   
> 
> Since this is at the end of a function with nothing after it, it
> would be nice if we could avoid the indent and unreachable();
> 
> The idea would be to write the last 3 lines like this instead:
> 
> 	guard_cond(iio_device_claim_direct, return -EBUSY, indio_dev);
> 
> But I didn't see a `guard_cond()` analog of `guard()` in
> linux/cleanup.h. So this is probably fine for now and adding
> `guard_cond()` (if it is actually a good idea in the first
> place) can be a job for another time.

That's a fun topic if you check the archives. Linus really
didn't like the proposal
https://lore.kernel.org/all/170905253339.2268463.9376907713092612237.stgit@dwillia2-xfh.jf.intel.com/

Mind you I don't think he much likes scoped_cond_guard() either!


Jonathan