Re: [v3 07/13] iio: imu: add Bosch Sensortec BNO055 core driver

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Apr 04, 2022 at 08:30:51AM +0200, Andrea Merello wrote:
> Il giorno lun 28 mar 2022 alle ore 15:02 Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> ha scritto:
> >
> > On Sun, Mar 27, 2022 at 05:11:04PM +0100, Jonathan Cameron wrote:
> > > On Tue, 22 Mar 2022 11:27:14 +0100
> > > Andrea Merello <andrea.merello@gmail.com> wrote:
> > >
> > > > Il giorno sab 19 feb 2022 alle ore 18:34 Jonathan Cameron
> > > > <jic23@kernel.org> ha scritto:
> > > > >
> > > > > On Thu, 17 Feb 2022 22:58:14 +0100 (CET)
> > > > > Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
> > > > >
> > > > > > On Thu, 17 Feb 2022, Andrea Merello wrote:
> > > > > >
> > > > > > nice work, minor comments below
> > > > >
> > > > > I'll review on top of Peter to save on duplication.
> > > > >
> > > > > Mostly really minor stuff.
> > >
> > > +CC Greg for binary attribute questions.
> > >
> > > >
> > > > :)
> > > >
> > > > As usual, comments inline; OK for all the rest.
> > > >
> > > > > Given this has crossed with the introduction of namespaces to quite
> > > > > a few IIO drivers (I have another series to do on that once I get
> > > > > caught up with reviews) I'd prefer it if you would move this into
> > > > > a symbol namespace (EXPORT_SYMBOL_NS_GPL() and appropriate namespace
> > > > > statements in the two bus modules.
> > > > >
> > > > > Save it being done as a follow up series.  If you prefer not to then
> > > > > that's fine too as it'll be a trivial follow up patch.
> > > >
> > > > I'll include it in V4 directly.
> > > >
> > > > [...]
> > > >
> > > > > > > +   case IIO_CHAN_INFO_SCALE:
> > > > > > > +           /* Table 3-31: 1 quaternion = 2^14 LSB */
> > > > > > > +           if (size < 2)
> > > > > > > +                   return -EINVAL;
> > > > > > > +           vals[0] = 1;
> > > > > > > +           vals[1] = 1 << 14;
> > > > > > > +           return IIO_VAL_FRACTIONAL_LOG2;
> > > > >
> > > > > This doesn't look right.  Not vals[1] = 14 given FRACTIONAL_LOG2?
> > > >
> > > > Hum.. maybe just IIO_VAL_FRACTIONAL ?
> > >
> > > That works as well, though I'd argue FRACTIONAL_LOG2 is the
> > > better option as it makes it clear the divisor is a power of 2
> > > and the precision might potentially be better as a result (I've not
> > > checked!)
> > >
> > > >
> > > > > > > +   default:
> > > > > > > +           return -EINVAL;
> > > > > > > +   }
> > > > > > > +}
> > > > > > > +
> > > >
> > > > [...]
> > > >
> > > > > > > +static IIO_DEVICE_ATTR_RO(sys_calibration_auto_status, 0);
> > > > > > > +static IIO_DEVICE_ATTR_RO(in_accel_calibration_auto_status, 0);
> > > > > > > +static IIO_DEVICE_ATTR_RO(in_gyro_calibration_auto_status, 0);
> > > > > > > +static IIO_DEVICE_ATTR_RO(in_magn_calibration_auto_status, 0);
> > > > > > > +static IIO_DEVICE_ATTR_RO(calibration_data, 0);
> > > > >
> > > > > This is documented as providing binary data but it's not using
> > > > > a binary attribute and that rather surprised me.
> > > > >
> > > > > Off the top of my head I can't recall why it matters though, so please
> > > > > take a look at whether a bin_attribute makes more sense for this.
> > > >
> > > > As far as I can see, it seems that a non-binary attributes only
> > > > support to be read at once while the binary attributes read()
> > > > operation supports random access i.e. it has the file position
> > > > parameter.
> > > >
> > > > The calibration data is "dynamic", it's read from the HW every time,
> > > > and I'm not sure it makes any sense to read it in several chunks (what
> > > > if we read a chunk and the calibration data is updated by the HW
> > > > before reading the second chunk?). So, despide the fitting "binary"
> > > > name I'm tempted to stick with regular attribute. However I'm not sure
> > > > this is the only difference related to binary attributes.
> > >
> > > +Cc Greg.  Valid choice to use a normal attribute for this?
> >
> > binary attributes are to ONLY be used for data that flows to/from a
> > device without the kernel ever modifying the data at all.  The kerneln
> > is just a pass-through here.
> >
> > There are a few minor exceptions, but they were exceptions, please don't
> > use them as a valid reason to use a binary attribute.
> >
> > does that help?
> 
> Thanks. Here the driver doesn't modify the data, so no probl here about this.
> 
> Would it be valid to restrict usage to only complete reads from the
> start to the end on calibration data i.e. returning -EINVAL when
> read() function is called with pos != 0 or count < actual data size ?

That's up to you.