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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 21 Feb 2022 19:04:38 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Feb 21, 2022 at 01:48:12PM +0100, Nuno Sá wrote:
> > On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote:  
> > > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote:  
> > > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote:  
> > > > > On Mon, Feb 07, 2022 at 09:19:46PM +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:  
> 
> ...
> 
> > > > > > > > > > +       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.  
> > > > 
> > > > Ok, going a bit back in the discussion, you argued that in one place I
> > > > was using long while here u16. Well, in the place I'm using long, that
> > > > was on purpose because that value is to be compared against an array of
> > > > longs (which has to be long because it depends on CCF rates). I guess I
> > > > can als0 use s64, but there is also a reason why long was used.
> > > > 
> > > > In the u16 case, we really want to have 2 bytes because I'm going to use
> > > > that value to write the dac code which is 2 bytes.  
> > > 
> > > Okay, that's what I want to hear. If it's indeed goes to be a value to the
> > > register, then it's fine.
> > > 
> > > Perhaps a comment?  
> > 
> > I guess you mean to have a comment to state that here we have fixed
> > size type (as opposed to long, used in another place), because we
> > directly use the value on a register write?
> > 
> > Asking it because I'm not planning to add comments in all the places
> > where I have fixed size types for register read/writes...  
> 
> Thinking more about it and now I'm convinced that using the value that goes to
> the register in ABI is bad idea (means that user space must not care about the
> size or contents of the hardware register and should be abstract representation
> of the HW).
> 
> OTOH this seems to be "raw" value of something. So, I maybe missed the convention
> in IIO about this kind of values WRT the variable types used on ABI side.
> 
> That said, I leave it to Jonathan since I'm not convinced that u16 is a proper
> choice here.

From a userspace point of view it doesn't care as it's writing a string.
In this particular case the string only has valid values that from 0-(2^16-1)
(i.e. 16 bits).  So if it writes outside of that range it is an error.
You could read it into an unsigned long and then check against the range,
but there is little point given you'd still return an error if it was out of
range.  The fact that kstrto16() does that for you really just a shortcut
though it will return -ERANGE rather than perhaps -EINVAL which might be used
for a more generic "not this value".

Userspace can also read the range that is acceptable from
out_voltage0_raw_available [0 1 2^16-1] and hence not write an invalid value
in the first place - which is obviously preferred to getting an error.
Scaling etc is also expressed to userspace so it it wants to write a particular
voltage it can perform the appropriate scaling. Note that moving linear scaling
like this to userspace allows easy use of floating point + may be a significant
performance advantage if using the chrdev interface which uses the same
approach (and values) as the sysfs interface.

Jonathan


 
> 
> > > > > > 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 :).  
> > > > > 
> > > > > No, this is about consistency and common sense. If you define type uXX,
> > > > > we have an API for that exact type. It's confusing why POD type APIs
> > > > > are used with fixed-width types or vise versa.
> > > > > 
> > > > > Moreover (which is pure theoretical, though) some architectures might
> > > > > have no (mutual) equivalency between these types.  
>