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?
>
> > > > +
> > > > +static IIO_DEVICE_ATTR_RO(serial_number, 0);
> > > > +
> > > > +static struct attribute *bno055_attrs[] = {
> > > > + &iio_dev_attr_in_accel_range_raw_available.dev_attr.attr,
> >
> > discussed in ABI documentation review.
> > I think these should be range_input to avoid implication they are
> > in _raw units (i.e. need _scale to be applied)
>
> They are raw indeed; they need scale to be applied, then they become m/s^2.
>
> I'll fix the doc to clarify this.
Ah. Ok.
>
> [...]
>
> > > > +
> > > > + priv->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > > > + if (IS_ERR(priv->reset_gpio))
> > > > + return dev_err_probe(dev, PTR_ERR(priv->reset_gpio), "Failed to get reset GPIO");
> > > > +
> > > > + priv->clk = devm_clk_get_optional(dev, "clk");
> > > > + if (IS_ERR(priv->clk))
> > > > + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get CLK");
> > > > +
> > > > + ret = clk_prepare_enable(priv->clk);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = devm_add_action_or_reset(dev, bno055_clk_disable, priv->clk);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + if (priv->reset_gpio) {
> > > > + usleep_range(5000, 10000);
> > > > + gpiod_set_value_cansleep(priv->reset_gpio, 1);
> > > > + usleep_range(650000, 750000);
> >
> > Not a toggle on the reset? I'd expect it to be set and then unset after
> > a pulse.
>
> Isn't the above devm_gpiod_get_optional() call that also initialize
> the initial GPIO value (then just wait and flip here) ?
good point. Missed that.
Jonathan
>
> [...]
