On Sun, 16 Jan 2022 16:28:08 +0000
"Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote:
> > 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.
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
