> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, January 16, 2022 1:44 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 v2 1/3] iio: dac: add support for ltc2688
>
> [External]
>
> On Sat, 15 Jan 2022 10:27:03 +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>
>
> A few minor additions inline.
>
> In particular I think we can work around that lack of
> device_for_each_available_child_node() issue and use generic fw
> propreties.
> rather than of ones. That way we can separate things from the
> question of
> how to 'fix' that issue.
>
> One thing I'm not sure on is the phase units. Right now I think you are
> exposing just the raw register value whereas I think that needs
> converting
> to radians.
It's returning degrees which I think is fairly ok. But I know that in general
we report radians, so I'm more than fine in changing this if you prefer it.
> Jonathan
>
>
>
> ...
>
> > +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) {
>
> Gah. This still going on with there not being a generic _available_
> specific form. We need to kick that again because I'm not keen to
> merge another driver we'll need to tidy up later to use generic
> properties.
>
> Best bet is probably to just define
> device_for_each_available_child_node() and see if anyone shoots
> it down (even if it does the same as device_for_each_child_node()
> in at least some cases).
>
> Or thinking about it.. Here you could use
> device_for_each_child_node()
> and then call fwnode_device_is_available() on the result and continue
> if not true.
>
> Will always return true (I think) but will make the intent clear.
>
> We can tidy up to a new for_* if / when it becomes available.
>
Hmm, not sure I'm following you... I mean, I understand what you're
saying here but there is a reason for why I changed the whole thing to
use OF. Please take a look at the cover... I explain why I've done it.
>
> > + 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);
> > + }
> > +
> > + 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;
> > +}
> > +
> > +static int ltc2688_setup(struct ltc2688_state *st, struct regulator
> *vref)
> > +{
> > + struct gpio_desc *gpio;
> > + int ret;
> > +
> > + /*
> > + * If we have a reset pin, use that to reset the board, If not, use
> > + * the reset bit.
> > + */
> > + gpio = devm_gpiod_get_optional(&st->spi->dev, "clr",
> GPIOD_OUT_HIGH);
> > + if (IS_ERR(gpio))
> > + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio),
> > + "Failed to get reset gpio");
> > + if (gpio) {
> > + usleep_range(1000, 1200);
> > + /* bring device out of reset */
> > + gpiod_set_value_cansleep(gpio, 0);
> > + } else {
> > + ret = regmap_update_bits(st->regmap,
> LTC2688_CMD_CONFIG,
> > + LTC2688_CONFIG_RST,
> > + LTC2688_CONFIG_RST);
> > + if (ret < 0)
> A bit of a mixture in here on whether you prefer if (ret) or if (ret < 0)
> for error returns you know can't be postive.
>
> I'd go with if (ret) for all of them. It makes things consistent with the
> cases where you directly return the value without checking it for less
> than 0 like below.
Yeah, sometimes my hands get confused :). Or probably, just copy pasting.
But I agree with you, when ret > 0 has no meaning I always want to do if (ret)
but you know...
- Nuno Sá
