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

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2022-02-07 at 21:19 +0100, Nuno Sá wrote:
> On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:
> > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:
> > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:
> > 
> > ...
> > 
> > > > > +#include <linux/of.h>
> > > > 
> > > > property.h please/
> > > 
> > > That probably means property and of both included. See below in
> > > the
> > > clock_get comments...
> > 
> > Why? OF won't be used at all.
> > 
> see below on the clock function...
> > 
> > ...
> > 
> > > > > +       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...
> > 
> > It's not bad, but I think that division between register and value
> > is
> > a good
> > thing to have.
> > 
> > > Can make that change though.
> > 
> > Would be nice!
> > 
> > ...
> > 
> > > > 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 😉.
> > 
> > Hmm... It was some time I have looked there. Any message ID to
> > share,
> > so
> > I can find it quickly?
> > 
> 
> https://lore.kernel.org/all/20211112152235.12fdcc49@jic23-huawei/
> 
> > ...
> > 
> > > > > +       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...
> > 
> > This is about consistency. It may work as is, but it feels not good
> > when for
> > int (or unsigned int) one uses fixed-width types. Also it's non-
> > written advice
> > to use fixed-width variables when it's about programming registers
> > or
> > so, for
> > the rest, use POD types.
> > 
> > ...
> 
> I can understand your reasoning but again this is something that
> I never really saw being enforced. So, I'm more than ok to change it
> if it really becomes something that we will try to "enforce" in IIO.
> Otherwise it just feels as a random nitpick :).
> 
> > 
> > > > > +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__.
> > 
> > Ah, okay, would be a limitation for non-OF platforms.
> > 
> > > Hence,
> > > once we are here, this can never be optional. That said, we need
> > > device_node 
> > 
> > That's fine, since CCF is OF-centric API.
> > 
> > > and hence of.h
> > 
> > Why? This header doesn't bring anything you will use here.
> 
> Correct me if Im missing something. AFAIU, the idea is to use
> 'device_for_each_child_node()' which returns a fwnode_handle. That
> means, that we will have to pass that to this function and use
> 'to_of_node()' to pass a device_node to 'devm_get_clk_from_child()'.
> 
> This means, we need of.h for 'to_of_node()'...
> 
> 

Andy, Jonathan,

I would definetly like to have some settlement on the above (before
sending a v4). It kind of was discussed a bit already in [1] where I
felt we had to live with OF for this driver (that is why I kept OF in
v3. With the above, I cannot
really see how we can have device API with also including OF... If I
missing something please let me know :)


[1]: https://lore.kernel.org/all/CAHp75VczFs8QpsY7tuB-h4X=H54nyjABA4qDSmpQ+FRYAHZdrA@mail.gmail.com/
- Nuno Sá