> From: Jonathan Cameron <jic23@xxxxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, December 16, 2021 3:11 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Rob
> Herring <robh+dt@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>
> Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
>
> [External]
>
> On Tue, 14 Dec 2021 17:56:06 +0100
> Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
>
> > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > precision reference. It is guaranteed monotonic and has built in
> > rail-to-rail output buffers that can source or sink up to 20 mA.
> >
> > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
>
> I'm not that keen on toggle having to be clock driven, but I guess we
> can
> always change that later when usecases come along.
>
I did wrote about some concerns with toggle (among others) in the cover.
When you have the time, some feedback in there would be very welcome :).
Anyways, for toggle mode, I do agree that "has to be clock driven" is likely to harsh.
Right now if a toggle channel is associated with a TGPx pin, then a clock is
mandatory and that's the condition that probably should be made optional.
Someone can very well want to drive the outputs with a GPIO even though
in that case we could argue to use the SW_TOGGLE.
>
> > +
> > +static int ltc2688_spi_read(void *context, const void *reg, size_t
> reg_size,
> > + void *val, size_t val_size)
> > +{
> > + struct ltc2688_state *st = context;
> > + struct spi_transfer xfers[] = {
> > + {
> > + .tx_buf = reg,
> > + .bits_per_word = 8,
> > + /*
> > + * This means that @val will also be part of the
> > + * transfer as there's no pad bits. That's fine as
> these
> > + * bits are don't care for the device and we fill
> > + * @val with the proper value afterwards. Using
> regmap
> > + * pad bits to get reg_size right would just make
> the
> > + * write part more cumbersome than this...
> > + */
> > + .len = reg_size + 2,
>
> what Lars said :)
Agree...
> > + .cs_change = 1,
> > + }, {
> > + .tx_buf = st->tx_data,
> > + .rx_buf = st->rx_data,
> > + .bits_per_word = 8,
> > + .len = 3,
> > + },
> > + };
> > + int ret;
> > +
> > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > + if (ret)
> > + return ret;
> > +
> > + memcpy(val, &st->rx_data[1], val_size);
> > +
> > + return 0;
> > +}
> > +
> > +static int ltc2688_spi_write(void *context, const void *data, size_t
> count)
> > +{
> > + struct ltc2688_state *st = context;
> > +
> > + return spi_write(st->spi, data, count);
> > +}
> > +
>
>
>
> > +};
> > +
> > +static int ltc2688_dac_code_write(struct ltc2688_state *st, u32
> chan, u32 input,
> > + u16 code)
> > +{
> > + struct ltc2688_chan *c = &st->channels[chan];
> > + int ret, reg;
> > +
> > + /* 2 LSBs set to 0 if writing dither amplitude */
> > + if (!c->toggle_chan && input == LT2688_INPUT_B) {
> > + if (code > GENMASK(13, 0))
> > + return -EINVAL;
> > +
> > + code <<= 2;
>
> FIELD_PREP preferred.
Will do...
> > + }
> > +
> > + mutex_lock(&st->lock);
> > + /* select the correct input register to read from */
> > + ret = regmap_update_bits(st->regmap,
> LTC2688_CMD_A_B_SELECT, BIT(chan),
> > + input << chan);
> > + if (ret)
> > + goto unlock;
> > +
> > + /*
> > + * If in dither/toggle mode the dac should be updated by an
> > + * external signal (or sw toggle) and not here.
> > + */
> > + if (c->mode == LTC2688_MODE_DEFAULT)
> > + reg = LTC2688_CMD_CH_CODE_UPDATE(chan);
> > + else
> > + reg = LTC2688_CMD_CH_CODE(chan);
> > +
> > + ret = regmap_write(st->regmap, reg, code);
> > +unlock:
> > + mutex_unlock(&st->lock);
> > + return ret;
> > +}
>
> > +
> > +static int ltc2688_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int *val,
> > + int *val2, long m)
> > +{
> > + struct ltc2688_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (m) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = ltc2688_dac_code_read(st, chan->channel,
> LT2688_INPUT_A,
> > + val);
> > + if (ret)
> > + return ret;
> > +
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_OFFSET:
> > + return ltc2688_offset_get(st, chan->channel, val);
> > + case IIO_CHAN_INFO_SCALE:
> > + *val2 = 16;
> > + return ltc2688_scale_get(st, chan->channel, val);
>
> I'm not against functions returning the IIO_VAL_* like this, but if they
> are I expect the function to set val2 as well.
>
> I'd suggest return 0 on success and then do similar to what you have
> done for code_read above.
Typically I do like to save lines of code when doable and readability is
not hurt which is the case. I'm not doing the same for the code_read
because that one is also used from the extended_info interface. That
said, I don't have strong feeling about this so I can do as you suggest.
> > + case IIO_CHAN_INFO_CALIBBIAS:
> > + ret = regmap_read(st->regmap,
> > + LTC2688_CMD_CH_OFFSET(chan-
> >channel), val);
> > + if (ret)
> > + return ret;
> > +
> > + /* Just 13 bits used. 2LSB ignored */
> > + *val >>= 2;
> FIELD_GET() would get rid of need for the comment.
>
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_CALIBSCALE:
> > + ret = regmap_read(st->regmap,
> > + LTC2688_CMD_CH_GAIN(chan-
> >channel), val);
> > + if (ret)
> > + return ret;
> > +
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
> > +
> > +static ssize_t ltc2688_read_ext(struct iio_dev *indio_dev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + char *buf)
> > +{
> > + struct ltc2688_state *st = iio_priv(indio_dev);
> > + u32 val;
> > + int ret;
> > +
> > + switch (private) {
> > + case LTC2688_DITHER_TOGGLE_ENABLE:
>
> As below. I'd have separate functions rather than multiplexing this
> to little benefit.
>
> > + return ltc2688_reg_bool_get(st, chan->channel,
> > +
> LTC2688_CMD_TOGGLE_DITHER_EN, buf);
> > + case LTC2688_POWERDOWN:
> > + return ltc2688_reg_bool_get(st, chan->channel,
> > +
> LTC2688_CMD_POWERDOWN, buf);
> > + case LTC2688_INPUT_B:
> > + ret = ltc2688_dac_code_read(st, chan->channel,
> LT2688_INPUT_B,
> > + &val);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "%u\n", val);
> > + case LTC2688_INPUT_B_AVAIL:
> > + /* dither amplitude has 1/4 of the span */
> > + return sysfs_emit(buf, "[%u %u %u]\n",
> ltc2688_raw_range[0],
> > + ltc2688_raw_range[1],
> > + ltc2688_raw_range[2] / 4);
> > + case LTC2688_SW_TOGGLE:
> > + return ltc2688_reg_bool_get(st, chan->channel,
> > + LTC2688_CMD_SW_TOGGLE,
> buf);
> > + case LTC2688_DITHER_FREQ:
> > + return ltc2688_dither_freq_get(st, chan->channel,
> buf);
> > + case LTC2688_DITHER_FREQ_AVAIL:
> > + return ltc2688_dither_freq_avail(st, chan->channel,
> buf);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static ssize_t ltc2688_write_ext(struct iio_dev *indio_dev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + const char *buf, size_t len)
> > +{
> > + struct ltc2688_state *st = iio_priv(indio_dev);
> > + u16 val;
> > + int ret;
> > + bool en;
> > +
> > + switch (private) {
> > + case LTC2688_DITHER_TOGGLE_ENABLE:
>
> As commented on below, I'd just have a function for each case block
> you have
> here.
>
> > + ret = kstrtobool(buf, &en);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ltc2688_dither_toggle_set(st, chan->channel, en);
> > + if (ret)
> > + return ret;
> > +
> > + return len;
> > + case LTC2688_POWERDOWN:
> > + ret = ltc2688_reg_bool_set(st, chan->channel,
> > + LTC2688_CMD_POWERDOWN,
> buf);
> > + if (ret)
> > + return ret;
> > +
> > + return len;
> > + case LTC2688_INPUT_B:
> > + ret = kstrtou16(buf, 10, &val);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ltc2688_dac_code_write(st, chan->channel,
> LT2688_INPUT_B,
> > + val);
> > + if (ret)
> > + return ret;
> > +
> > + return len;
> > + case LTC2688_SW_TOGGLE:
> > + ret = ltc2688_reg_bool_set(st, chan->channel,
> > + LTC2688_CMD_SW_TOGGLE,
> buf);
> > + if (ret)
> > + return ret;
> > +
> > + return len;
> > + case LTC2688_DITHER_FREQ:
> > + ret = ltc2688_dither_freq_set(st, chan->channel, buf);
> > + if (ret)
> > + return ret;
> > +
> > + return len;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ltc2688_get_dither_phase(struct iio_dev *dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct ltc2688_state *st = iio_priv(dev);
> > + int ret, regval;
> > +
> > + ret = regmap_read(st->regmap,
> LTC2688_CMD_CH_SETTING(chan->channel),
> > + ®val);
> > + if (ret)
> > + return ret;
> > +
> > + return FIELD_GET(LTC2688_CH_DIT_PH_MSK, regval);
> > +}
> > +
> > +static int ltc2688_set_dither_phase(struct iio_dev *dev,
> > + const struct iio_chan_spec *chan,
> > + unsigned int phase)
> > +{
> > + struct ltc2688_state *st = iio_priv(dev);
> > +
> > + return regmap_update_bits(st->regmap,
> > + LTC2688_CMD_CH_SETTING(chan-
> >channel),
> > + LTC2688_CH_DIT_PH_MSK,
> > +
> FIELD_PREP(LTC2688_CH_DIT_PH_MSK, phase));
> > +}
> > +
> > +static int ltc2688_reg_access(struct iio_dev *indio_dev,
> > + unsigned int reg,
> > + unsigned int writeval,
> > + unsigned int *readval)
> > +{
> > + struct ltc2688_state *st = iio_priv(indio_dev);
> > +
> > + if (readval)
> > + return regmap_read(st->regmap, reg, readval);
> > + else
> > + return regmap_write(st->regmap, reg, writeval);
> > +}
> > +
> > +static const char * const ltc2688_dither_phase[] = {
> > + "0", "90", "180", "270",
> > +};
> > +
> > +static const struct iio_enum ltc2688_dither_phase_enum = {
> > + .items = ltc2688_dither_phase,
> > + .num_items = ARRAY_SIZE(ltc2688_dither_phase),
> > + .set = ltc2688_set_dither_phase,
> > + .get = ltc2688_get_dither_phase,
> > +};
> > +
> > +#define LTC2688_CHAN_EXT_INFO(_name, _what, _shared) { \
> > + .name = _name, \
> > + .read = ltc2688_read_ext, \
> > + .write = ltc2688_write_ext, \
>
> I'm not really convinced big multiplexer functions are a good idea here.
> They seem to save little code and hurt readability a bit.
I think this is a very common pattern seen in IIO and probably HWMON no?
Anyways, I'm ok with either way so I can just extend the macro to accept
the individual functions. I have to admit that in some cases (when locking is
required in some case blocks) I'm also not a big fan of these multiplexes
functions. And I think I'm calling individual functions in all the case blocks
anyways...
> > + .private = (_what), \
> > + .shared = (_shared), \
> > +}
> > +
> > +/*
> > + * For toggle mode we only expose the symbol attr (sw_toggle) in
> case a TGPx is
> > + * not provided in dts.
> > + */
> > +#define LTC2688_CHAN_TOGGLE(t, name) ({
> \
> > + static const struct iio_chan_spec_ext_info t ## _ext_info[] = {
> \
> > + LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B,
> IIO_SEPARATE), \
> > + LTC2688_CHAN_EXT_INFO("toggle_en",
> LTC2688_DITHER_TOGGLE_ENABLE, \
> > + IIO_SEPARATE),
> \
> > + LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE), \
> > + LTC2688_CHAN_EXT_INFO(name,
> LTC2688_SW_TOGGLE, IIO_SEPARATE), \
> > + {}
> \
> > + };
> \
> > + t ## _ext_info;
> \
> > +})
> > +
> > +static struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = {
> > + LTC2688_CHAN_EXT_INFO("dither_raw", LTC2688_INPUT_B,
> IIO_SEPARATE),
> > + LTC2688_CHAN_EXT_INFO("dither_raw_available",
> LTC2688_INPUT_B_AVAIL,
> > + IIO_SEPARATE),
> > + /*
> > + * Not IIO_ENUM because the available freq needs to be
> computed at
> > + * probe. We could still use it, but it didn't felt much right.
> > + *
>
> no extra blank line.
>
> > + */
> > + LTC2688_CHAN_EXT_INFO("dither_frequency",
> LTC2688_DITHER_FREQ,
> > + IIO_SEPARATE),
> > + LTC2688_CHAN_EXT_INFO("dither_frequency_available",
> > + LTC2688_DITHER_FREQ_AVAIL,
> IIO_SEPARATE),
> > + IIO_ENUM("dither_phase", IIO_SEPARATE,
> <c2688_dither_phase_enum),
> > + IIO_ENUM_AVAILABLE("dither_phase", IIO_SEPARATE,
> > + <c2688_dither_phase_enum),
> > + LTC2688_CHAN_EXT_INFO("dither_en",
> LTC2688_DITHER_TOGGLE_ENABLE,
> > + IIO_SEPARATE),
> > + LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),
> > + {}
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
> > + LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),
> > + {}
> > +};
> > +
>
> > +
> > +enum {
> > + LTC2688_CHAN_TD_TGP1,
> > + LTC2688_CHAN_TD_TGP2,
> > + LTC2688_CHAN_TD_TGP3,
> > + LTC2688_CHAN_TD_MAX
> > +};
>
> > +/* Helper struct to deal with dither channels binded to TGPx pins */
> > +struct ltc2688_dither_helper {
> > + u8 chan[LTC2688_DAC_CHANNELS];
> > + u8 n_chans;
> > +};
> > +
> bitmap perhaps given ordering doesn't matter (I think)
>
Yeah, did not thought about it but I think it will look better with a bitmap yes.
Although I'm not sure if I will continue with this approach or make the clocks
property a per channel one (more on this in the cover letter).
>
> ...
>
> > +
> > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > +{
> > + struct fwnode_handle *fwnode = dev_fwnode(&st->spi-
> >dev), *child;
> > + struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =
> {0};
> > + u32 reg, clk_input, val, mask, tmp[2];
> > + unsigned long clk_msk = 0;
> > + int ret, span;
> > +
>
> I think you need to sanity check you have a fwnode
AFAICT, it's done by us already :)
https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L741
> > + fwnode_for_each_available_child_node(fwnode, child) {
>
> I guess this is because of the whole
> device_for_each_available_child_node() not
> existing discussion that isn't resolved.
exactly... I wanted the available option and this was the only way I
could find...
> > + struct ltc2688_chan *chan;
> > +
> > + ret = fwnode_property_read_u32(child, "reg", ®);
> > + if (ret) {
> > + fwnode_handle_put(child);
> > + return dev_err_probe(&st->spi->dev, ret,
> > + "Failed to get reg
> property\n");
> > + }
> > +
> > + if (reg >= LTC2688_DAC_CHANNELS) {
> > + fwnode_handle_put(child);
> > + return dev_err_probe(&st->spi->dev, -EINVAL,
> > + "reg bigger than: %d\n",
> > + LTC2688_DAC_CHANNELS);
> > + }
> > +
> > + chan = &st->channels[reg];
> > + if (fwnode_property_read_bool(child, "adi,toggle-
> mode")) {
> > + chan->toggle_chan = true;
> > + /* assume sw toggle ABI */
> > + ltc2688_channels[reg].ext_info =
> LTC2688_CHAN_TOGGLE(__s, "symbol");
>
> That's a little nasty. Pick it up from a static const array defined outside
> this function.
Ehehe, personally I do not think is that bad but ok, Lars was also complaining
about it so better listen to you :)
> > + }
> > +
> > + ret = fwnode_property_read_u32_array(child,
> "adi,output-range-millivolt",
> > + tmp,
> ARRAY_SIZE(tmp));
> > + if (!ret) {
> > + span = ltc2688_span_lookup(st, tmp[0],
> tmp[1]);
> > + if (span < 0)
> > + return dev_err_probe(&st->spi->dev, -
> EINVAL,
> > + "output range not
> valid:[%d %d]\n",
> > + tmp[0], tmp[1]);
> > +
> > + mask |= LTC2688_CH_SPAN_MSK;
> > + val |= FIELD_PREP(LTC2688_CH_SPAN_MSK,
> span);
> > + }
> > +
> > + ret = fwnode_property_read_u32(child, "adi,toggle-
> dither-input",
> > + &clk_input);
> > + if (!ret) {
> > + int cur_chan = tgp[clk_input].n_chans;
> > +
> > + if (clk_input > LTC2688_CHAN_TD_TGP3) {
> > + fwnode_handle_put(child);
> > + return dev_err_probe(&st->spi->dev, -
> EINVAL,
> > + "toggle-dither-input
> inv value(%d)\n",
> > + clk_input);
> > + }
> > +
> > + mask |= LTC2688_CH_TD_SEL_MSK;
> > + /*
> > + * 0 means software toggle which is the default
> mode.
> > + * Hence the +1.
> > + */
> > + val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK,
> clk_input + 1);
> > + clk_msk |= BIT(clk_input);
> > + /*
> > + * If a TGPx is given, we automatically assume a
> dither
> > + * capable channel (unless toggle is already
> enabled).
> > + * Hence, we prepar our TGPx <-> channel map
> to make it
>
> prepare
>
> > + * easier to handle the clocks and available
> frequency
> > + * calculations... On top of this we just set here
> the
> > + * dither bit in the channel settings. It won't
> have any
> > + * effect until the global toggle/dither bit is
> enabled.
> > + */
> > + if (!chan->toggle_chan) {
> > + tgp[clk_input].chan[cur_chan] = reg;
> > + tgp[clk_input].n_chans++;
> > + mask |= LTC2688_CH_MODE_MSK;
> > + val |=
> FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
> > + ltc2688_channels[reg].ext_info =
> ltc2688_dither_ext_info;
> > + } else {
> > + /* wait, no sw toggle after all */
> > + ltc2688_channels[reg].ext_info =
> LTC2688_CHAN_TOGGLE(__no_s, NULL);
> > + }
> > + }
> > +
> > + if (fwnode_property_read_bool(child,
> "adi,overrange")) {
> > + chan->overrange = true;
> > + val |= LTC2688_CH_OVERRANGE_MSK;
> > + mask |= BIT(3);
> > + }
> > +
> > + if (!mask)
> > + continue;
> > +
> > + ret = regmap_update_bits(st->regmap,
> > +
> LTC2688_CMD_CH_SETTING(reg), mask,
> > + val);
>
> Maybe I'm missing something, but why not just write the whole
> register?
> Certainly most if not all of the value is controlled by this function or
> known
> at this point to be in the reset state.
Yeah, that's true. The register will be 0 in reset so just writing will
be enough. Nice catch.
>
> > + if (ret) {
> > + fwnode_handle_put(child);
> > + return dev_err_probe(&st->spi->dev, -EINVAL,
> > + "failed to set chan
> settings\n");
> > + }
> > +
> > + mask = 0;
> > + val = 0;
>
> Why not at the start of the loop? Particularly as I can't see them being
> initialised for the first loop.
Hmm I'm actually wondering how did this worked in my tests. I will just
move val to the beginning and mask can be dropped.
>
> > + }
> > +
> > + return ltc2688_tgp_setup(st, clk_msk, tgp);
> > +}
>
>
> ...
>
> > +static bool ltc2688_reg_writable(struct device *dev, unsigned int
> reg)
> > +{
> > + if (reg <= LTC2688_CMD_UPDATE_ALL && reg !=
> LTC2688_CMD_THERMAL_STAT)
>
> Isn't UPDATE_ALL the last register? So how do you get higher than
> that?
> Definitely needs a comment if there is a reason that check is
> necessary.
If you look at the commands table you see that on the write side
we jump from 0x76 to 0x78 (UPDATE_ALL=0x7c). 0x77 refers to
reading the thermal status reg which is not writable. Actually in the
end, as it's a read the command for reading the thermal status will
be 0xf7.
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +struct regmap_bus ltc2688_regmap_bus = {
> > + .read = ltc2688_spi_read,
> > + .write = ltc2688_spi_write,
> > + .read_flag_mask = LTC2688_READ_OPERATION,
> > + .reg_format_endian_default = REGMAP_ENDIAN_BIG,
> > + .val_format_endian_default = REGMAP_ENDIAN_BIG
> > +};
> > +
> > +static const struct regmap_config ltc2688_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 16,
> > + .readable_reg = ltc2688_reg_readable,
> > + .writeable_reg = ltc2688_reg_writable,
> > + /* ignoring the no op command */
> > + .max_register = LTC2688_CMD_UPDATE_ALL
> > +};
> > +
> > +static const struct iio_info ltc2688_info = {
> > + .write_raw = ltc2688_write_raw,
> > + .read_raw = ltc2688_read_raw,
> > + .read_avail = ltc2688_read_avail,
> > + .debugfs_reg_access = ltc2688_reg_access,
> > +};
> > +
> > +static int ltc2688_probe(struct spi_device *spi)
> > +{
> > + struct ltc2688_state *st;
> > + struct iio_dev *indio_dev;
> > + struct regulator *vref_reg;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + st = iio_priv(indio_dev);
> > + st->spi = spi;
>
> blank line so it's clear the comment refers to the next block.
>
> > + /* Just this once. No need to di it in every regmap read */
>
> do
>
> > + st->tx_data[0] = LTC2688_CMD_NOOP;
> > + mutex_init(&st->lock);
> > +
> > + st->regmap = devm_regmap_init(&spi->dev,
> <c2688_regmap_bus, st,
>
> A lot of references to &spi->dev so probably worth a local
> struct device *dev = &spi->dev; Might let a few things fit
> on fewer lines, but either way it'll be a little tidier.
can do that...
- Nuno Sá
