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

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

On Mon, 21 Feb 2022 20:49:48 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Feb 21, 2022 at 05:30:45PM +0000, Jonathan Cameron wrote:
> > 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.  
> 
> With the same logic it can be unsigned short, no?

It could be any integer as long as it is at least as large as a u16.
But it it is larger than a u16 you'll need an additional check on the
maximum.

> 
> The point is to use u16 when it's indeed fixed-width value that goes to
> hardware or being used as part of a protocol. And thus mentioning of the
> IOCTL protocols may justify the choice. Then the question to the other
> values, shouldn't they be also fixed-width ones?

If we had a fixed width type that took the values 0-4 sure using such
a magic type would make sense, but we don't.

Note that internally kstrtou16 is just strtoull and a range check.
The one other case we have here does pretty much the same thing.

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