Il giorno lun 28 mar 2022 alle ore 15:02 Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx> wrote: > > > > > Il giorno sab 19 feb 2022 alle ore 18:34 Jonathan Cameron > > > <jic23@xxxxxxxxxx> ha scritto: > > > > > > > > On Thu, 17 Feb 2022 22:58:14 +0100 (CET) > > > > Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx> 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 ? > > greg k-h
