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.
:)
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 ?
> > > + 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.
> > > +
> > > +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.
[...]
> > > +
> > > + 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) ?
[...]
