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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 16 Jan 2022 16:28:08 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, January 16, 2022 1:44 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 v2 1/3] iio: dac: add support for ltc2688
> > 
> > [External]
> > 
> > On Sat, 15 Jan 2022 10:27:03 +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>  
> > 
> > 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.

Radians for consistency is a must as users reading the docs may see the main
_phase descriptions and have no reason to think this one might be different.
 

> 
> > 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.

Hohum. Reading the cover letter? :)  Next you'll be suggesting
I read manuals of new hardware!  I'll take a look.

Jonathan