Il giorno mer 27 apr 2022 alle ore 15:23 Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> ha scritto:
>
As usual, some inline comments. OK for the rest.
[...]
>
> > +#define BNO055_ATTR_VALS(...) \
> > + .vals = (int[]){ __VA_ARGS__}, \
> > + .len = ARRAY_SIZE(((int[]){__VA_ARGS__}))
>
> Not sure this adds any readability to the code. Can we simply have an
> array of int for each case with the explicit ARRAY_SIZE() calls?
Do you mean moving the vals array out of the structs? Something like:
static int bno055_gyr_scale_vals[] = {125, 1877467, 250, 1877467,
500, 1877467, 1000, 1877467, 2000, 1877467};
static struct bno055_sysfs_attr_aux_data bno055_gyr_scale_aux = {
.fusion_vals = (int[]){1, 900},
.hw_xlate = (int[]){4, 3, 2, 1, 0},
.type = IIO_VAL_FRACTIONAL
?
But then I'd make also something like:
#define bno055_sysfs_attr_avail(priv, attr, vals, len) \
_bno055_sysfs_attr_avail(priv, attr##_vals,
ARRAY_SIZE(attr##_vals), attr##_aux, vals, len)
And the same for all other users of those structs.
My point is not about readability, but about avoiding as much as
possible bugs caused by mismatched attr_vals, attr_aux and
ARRAY_SIZE() arg. e.g:
bno055_sysfs_attr_avail(priv, bno_foo_vals, ARRAY_SIZE(bno_bar_vals),
bno_foobar_aux, vals, len)
I used to make quite a lot of mess until I grouped all the stuff in
one struct :/
[...]
>
> > + msleep(20);
>
> Perhaps a comment why so long sleep is needed.
DS says that switching mode can last from 7mS up to 19mS depending on
the case, but I don't know _why_ it takes so long. I may add a comment
that just states that it's a sensor requirement.
[...]
>
> > + for (i = 0; i < bno055_acc_range.len; i++)
> > + len += sysfs_emit_at(buf, len, "%d%c", bno055_acc_range.vals[i],
> > + (i == bno055_acc_range.len - 1) ? '\n' : ' ');
>
> You may move the condition out of the loop.
May you elaborate, please? Do you mean something like: loop one time
less, and then call sysfs_emit_at() once more outside the loop,
getting rid of the conditional ternary operator at all?
[...]
> > + if (indio_dev->active_scan_mask &&
> > + !bitmap_empty(indio_dev->active_scan_mask, _BNO055_SCAN_MAX))
> > + return -EBUSY;
> > +
> > + if (sysfs_streq(buf, "0")) {
> > + ret = bno055_operation_mode_set(priv, BNO055_OPR_MODE_AMG);
>
> return bno055_operation_mode_set(...);
Why? bno055_operation_mode_set() returns an error code, while here we
need to return the len, or propagate the error code only when it's the
case
> > + } else {
>
> ...and drop this with the following decreasing indentation.
if you want to drop this, then I can just duplicate if(ret) return
ret; i.e. add it after bno055_operation_mode_set(priv,
BNO055_OPR_MODE_AMG); and get rid of the else branch (see above)
[...]
>
> Can be removed to group all related checks together.
I'm not sure what you mean here, but see below
> > + if (ret)
> > + dev_notice(dev, "Calibration file load failed. See instruction in kernel Documentation/iio/bno055.rst");
> > +
> > + if (caldata) {
> > + caldata_data = caldata->data;
> > + caldata_size = caldata->size;
> > + }
> > + ret = bno055_init(priv, caldata_data, caldata_size);
>
> > + if (caldata)
> > + release_firmware(caldata);
>
> > + if (ret)
> > + return ret;
>
> Can be rewritten in a form of
>
> if (caldata) {
> ret = bno055_init();
> release_firmware(...);
> } else {
> ret = bno055_init();
> }
> if (ret)
> return ret;
>
> ?
Indeed I'd say it could be rewritten as:
if (ret)
ret = request_firmware(&caldata, BNO055_FW_GENERIC_NAME, dev);
if (ret) {
dev_notice(dev, "Calibration file load failed. See
instruction in kernel Documentation/iio/bno055.rst");
ret = bno055_init(priv, NULL, 0);
} else {
ret = bno055_init(priv, caldata->data, caldata->size);
release_firmware(caldata);
}
if (ret)
return ret;
