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

[David Lechner]

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.