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.