> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> Sent: Saturday, February 5, 2022 6:30 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Rob
> Herring <robh+dt@xxxxxxxxxx>; Jonathan Cameron
> <jic23@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>;
> Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>
> Subject: Re: [PATCH v3 1/3] iio: dac: add support for ltc2688
>
> [External]
>
> On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá 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.
>
> ...
>
> > +#include <linux/of.h>
>
> property.h please/
That probably means property and of both included. See below in the
clock_get comments...
> ...
>
> > +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 = st->tx_data,
> > + .bits_per_word = 8,
> > + .len = 3,
> > + .cs_change = 1,
> > + }, {
> > + .tx_buf = st->tx_data + 3,
> > + .rx_buf = st->rx_data,
> > + .bits_per_word = 8,
> > + .len = 3,
> > + },
> > + };
> > + int ret;
>
> > + memcpy(st->tx_data, reg, reg_size);
> > +
> > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > + if (ret)
> > + return ret;
> > +
> > + memcpy(val, &st->rx_data[1], val_size);
> > +
> > + return 0;
> > +}
>
> First of all, yuo have fixed len in transfer sizes, so what the purpose of
> the reg_size / val_size?
Well, reg_size is 1 byte and val_size is 2 as defined in the regmap_bus
struct. And that is what it must be used for the transfer to work. I
could also hardcode 1 and 2 but I preferred to use the parameters. I guess
you can argue (and probably this is why you are complaining about this)
for me to use reg_size + val_size in the transfer length for consistency.
That's fair but I do not think this is __that__ bad... Can make that change
though.
> Second, why do you need this specific function instead of regmap bulk
> ops against be24/le24?
>
Not sure I'm following this one... If you mean why am I using a custom
regmap_bus implementation, that was already explained in the RFC patch.
And IIRC, you were the one already asking 😉.
> ...
>
> > +unlock:
>
> out_unlock: ?
> (And in similar cases)
Well, why not? can do that...
> ...
>
> > + if (ret)
> > + return ret;
> > +
> > + return len;
>
> In some cases the return ret ?: len; is used, in some like above. Maybe
> a bit
> of consistency?
That's fair... At this point, I will go with the one that evolves less changes
unless noted otherwise.
> ...
>
> > + if (private == LTC2688_INPUT_B_AVAIL)
> > + return sysfs_emit(buf, "[%u %u %u]\n",
> ltc2688_raw_range[0],
> > + ltc2688_raw_range[1],
> > + ltc2688_raw_range[2] / 4);
>
> Is it standard form "[A B C]" for ranges in IIO? I haven't looked into the
> code
> deeply (and datasheet at all) to understand meaning. To me range is
> usually out
> of two numbers.
>
> > + if (private == LTC2688_DITHER_OFF)
> > + return sysfs_emit(buf, "0\n");
>
> > + ret = ltc2688_dac_code_read(st, chan->channel, private, &val);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "%u\n", val);
>
> These three types of output for one sysfs node? Seems it's not align
> with the
> idea behind sysfs. It maybe that I missed something.
>
> ...
>
> > + ret = kstrtou16(buf, 10, &val);
>
> In other function you have long, here u16. I would expect that the
> types are of
> the same class, e.g. if here you have u16, then there something like
> s32 / s64.
> Or here something like unsigned short.
>
> A bit of elaboration why u16 is chosen here?
Well, I never really saw any enforcement here to be honest (rather than using
stdint types...). So I pretty much just use these in unsigned types because
I'm lazy and u16 is faster to type than unsigned short... In this case, unless Jonathan
really asks for it, I prefer not to go all over the driver and change this...
> ...
>
> > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),
> \
> > + .ext_info = ltc2688_ext_info \
>
> + Comma
>
> ...
>
> > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > + struct ltc2688_chan *chan,
> > + struct device_node *np, int tgp)
> > +{
> > + unsigned long rate;
> > + struct clk *clk;
> > + int ret, f;
> > +
> > + clk = devm_get_clk_from_child(&st->spi->dev, np, NULL);
> > + if (IS_ERR(clk))
>
> Make it optional for non-OF, can be done as easy as
>
> if (IS_ERR(clk)) {
> if (PTR_ERR(clk) == -ENOENT)
> clk = NULL;
> else
> return dev_err_probe(...);
> }
>
> > + return dev_err_probe(&st->spi->dev, PTR_ERR(clk),
> > + "failed to get tgp clk.\n");
Well, I might be missing the point but I think this is not so straight....
We will only get here if the property " adi,toggle-dither-input" is given
in which case having the associated clocks is __mandatory__. Hence,
once we are here, this can never be optional. That said, we need
device_node and hence of.h to be included and this was the main reason
why I changed from property.h to of.h (once I started to use
'devm_get_clk_from_child()'. I don’t really think that using both of and
property is a good idea and I raised this in the previous version of this series
and no one made it clear that using both of and property would be acceptable
so I kept my move to of in the current version.
> > + ret = clk_prepare_enable(clk);
> > + if (ret)
> > + return dev_err_probe(&st->spi->dev, ret,
> > + "failed to enable tgp clk.\n");
> > +
> > + ret = devm_add_action_or_reset(&st->spi->dev,
> ltc2688_clk_disable, clk);
> > + if (ret)
> > + return ret;
> > +
> > + if (chan->toggle_chan)
> > + return 0;
> > +
> > + /* calculate available dither frequencies */
> > + rate = clk_get_rate(clk);
> > + for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++)
> > + chan->dither_frequency[f] =
> DIV_ROUND_CLOSEST(rate, ltc2688_period[f]);
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > +{
> > + struct device *dev = &st->spi->dev;
> > + struct device_node *child;
> > + u32 reg, clk_input, val, tmp[2];
> > + int ret, span;
> > +
> > + for_each_available_child_of_node(dev->of_node, child) {
>
> device_for_each_child_node()
>
> > + struct ltc2688_chan *chan;
> > +
> > + ret = of_property_read_u32(child, "reg", ®);
> > + if (ret) {
> > + of_node_put(child);
> > + return dev_err_probe(dev, ret,
> > + "Failed to get reg
> property\n");
> > + }
> > +
> > + if (reg >= LTC2688_DAC_CHANNELS) {
> > + of_node_put(child);
> > + return dev_err_probe(dev, -EINVAL,
> > + "reg bigger than: %d\n",
> > + LTC2688_DAC_CHANNELS);
> > + }
> > +
> > + val = 0;
> > + chan = &st->channels[reg];
> > + if (of_property_read_bool(child, "adi,toggle-mode")) {
> > + chan->toggle_chan = true;
> > + /* assume sw toggle ABI */
> > + st->iio_chan[reg].ext_info =
> ltc2688_toggle_sym_ext_info;
> > + /*
> > + * Clear IIO_CHAN_INFO_RAW bit as toggle
> channels expose
> > + * out_voltage_raw{0|1} files.
> > + */
>
> > + clear_bit(IIO_CHAN_INFO_RAW,
> > + &st-
> >iio_chan[reg].info_mask_separate);
>
> Do you need atomic operation here?
Not really, but I still prefer to use 'clear_bit()' rather than doing it
by hand... Is there another utility for this?
> > + }
> > +
> > + ret = of_property_read_u32_array(child, "adi,output-
> range-microvolt",
> > + tmp,
> ARRAY_SIZE(tmp));
> > + if (!ret) {
> > + span = ltc2688_span_lookup(st, (int)tmp[0] /
> 1000,
> > + tmp[1] / 1000);
> > + if (span < 0) {
> > + of_node_put(child);
> > + return dev_err_probe(dev, -EINVAL,
> > + "output range not
> valid:[%d %d]\n",
> > + tmp[0], tmp[1]);
> > + }
> > +
> > + val |= FIELD_PREP(LTC2688_CH_SPAN_MSK,
> span);
> > + }
> > +
> > + ret = of_property_read_u32(child, "adi,toggle-dither-
> input",
> > + &clk_input);
> > + if (!ret) {
> > + if (clk_input >= LTC2688_CH_TGP_MAX) {
> > + of_node_put(child);
> > + return dev_err_probe(dev, -EINVAL,
> > + "toggle-dither-input
> inv value(%d)\n",
> > + clk_input);
> > + }
> > +
> > + ret = ltc2688_tgp_clk_setup(st, chan, child,
> clk_input);
> > + if (ret) {
> > + of_node_put(child);
> > + return ret;
> > + }
> > +
> > + /*
> > + * 0 means software toggle which is the default
> mode.
> > + * Hence the +1.
> > + */
> > + val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK,
> clk_input + 1);
> > +
> > + /*
> > + * If a TGPx is given, we automatically assume a
> dither
> > + * capable channel (unless toggle is already
> enabled).
> > + * 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) {
> > + val |=
> FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
> > + st->iio_chan[reg].ext_info =
> ltc2688_dither_ext_info;
> > + } else {
> > + /* wait, no sw toggle after all */
> > + st->iio_chan[reg].ext_info =
> ltc2688_toggle_ext_info;
> > + }
> > + }
> > +
> > + if (of_property_read_bool(child, "adi,overrange")) {
> > + chan->overrange = true;
> > + val |= LTC2688_CH_OVERRANGE_MSK;
> > + }
> > +
> > + if (!val)
> > + continue;
> > +
> > + ret = regmap_write(st->regmap,
> LTC2688_CMD_CH_SETTING(reg),
> > + val);
> > + if (ret) {
> > + of_node_put(child);
> > + return dev_err_probe(dev, -EINVAL,
> > + "failed to set chan
> settings\n");
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +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
>
> + Comma.
>
> > +};
> > +
> > +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
>
> Ditto.
>
> > +};
>
> ...
>
> > + vref_reg = devm_regulator_get_optional(dev, "vref");
>
> > + if (!IS_ERR(vref_reg)) {
>
> Why not positive conditional check (and hence standard pattern --
> error
> handling first)?
>
No reason.. I can flip the logic...
- Nuno Sá
