On Mon, 9 Sep 2024 09:51:35 -0500 David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx/ Mind you I don't think he much likes scoped_cond_guard() either! Jonathan