RE: [PATCH 1/3] iio: dac: add support for ltc2688

["Sa, Nuno" <Nuno.Sa@analog.com>]

> From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> Sent: Thursday, December 30, 2021 4:28 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> On Fri, 17 Dec 2021 12:31:57 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> > > Sent: Thursday, December 16, 2021 3:11 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob
> > > Herring <robh+dt@kernel.org>; Lars-Peter Clausen
> > > <lars@metafoo.de>; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>
> > > 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@analog.com> 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@analog.com>
> > >
> > > 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 :).
> 
> Doh.  Guess I didn't look at the cover letter. Now replied to that as
> well.
> 
> >
> > 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.
> 
> I wonder if we also need the case where the toggle source is invisible
> to us
> as it's the output of some other hardware.  Obviously would be nice to
> model
> that hardware in DT but that might not always be possible.

Yeah, could fall into the host GPIO usecase. We don't really control it
in the driver but the user can still use it to drive the dac output. Anyways,
as stated in the cover, let's leave things as is for now...

> 
> > > > +
> > > > +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.
> 
> Either option is fine for me.  Set val2 inside _scale_get() or return 0
> from that and then do a return IIO_VAL_INT_PLUS_MICRO here.
> 
> The particular combination at the moment is rather inconsistent as
> val, val2 and the return value should all come from the same 'source'
> whether it's here, or in _scale_get()

Already did as you first advised...

> >
> > > > +	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 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...
> 
> Common pattern, but not always a good idea.  All depends on how
> much
> common code there is.  In this case I don't think there is enough for it
> to make sense.

Agreed. 

> >
> > > > +	.private = (_what),				\
> > > > +	.shared = (_shared),				\
> > > > +}
> > > > +
> 
> ...
> 
> > > > +	 */
> > > > +	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,
> > > &ltc2688_dither_phase_enum),
> > > > +	IIO_ENUM_AVAILABLE("dither_phase", IIO_SEPARATE,
> > > > +			   &ltc2688_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).
> 
> I'm not sure how the per channel version will look so leaving this
> entirely
> up to you!
> 
> > >
> > > ...
> > >
> > > > +
> > > > +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://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/s
> ource/drivers/base/property.c*L741__;Iw!!A3Ni8CS0y2Y!sxVAp8P8XS
> 0R5sR447hKmVu7dK01fdsMfL6_c4woz7kDsbsI2fKLKfWopK4mOQ$
> 
> Ah.  Good point. Ignore that one then.
> 
> >
> > > > +	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...
> >
> 
> Hmm. I need to revisit that discussion and see where we got to.
> 
> > >
> > > > +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.
> 
> I'm lost on this.   My confusion is how you get >
> LTC2688_CMD_UPDATE_ALL
> Possibly that's what you are referring to with teh read command being
> 0xf...

Exactly... So I'm basically using the regmap read bit to get the read command
as in the datasheet. And that bit is still not set when we get into these callbacks
which means you can get 0x77 here which is < LTC2688_CMD_UPDATE_ALL but
still not writable...

I did had a comment for v2. Let's see if it's god enough :)

- Nuno Sá