Hello Andy,
Thank you for the quick review. Please find my comments below.
On Thu, Jun 16, 2022 at 02:18:52PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 16, 2022 at 12:42 PM Dmitry Rokosov
> <DDRokosov@xxxxxxxxxxxxxx> wrote:
...
> > +enum msa311_fields {
> > + F_SOFT_RESET_I2C, F_SOFT_RESET_SPI,
> > +
> > + F_ORIENT_INT, F_S_TAP_INT, F_D_TAP_INT, F_ACTIVE_INT, F_FREEFALL_INT,
> > +
> > + F_NEW_DATA_INT,
> > +
> > + F_TAP_SIGN, F_TAP_FIRST_X, F_TAP_FIRST_Y, F_TAP_FIRST_Z, F_ACTV_SIGN,
> > + F_ACTV_FIRST_X, F_ACTV_FIRST_Y, F_ACTV_FIRST_Z,
> > +
> > + F_ORIENT_Z, F_ORIENT_X_Y,
> > +
> > + F_FS,
> > +
> > + F_X_AXIS_DIS, F_Y_AXIS_DIS, F_Z_AXIS_DIS, F_ODR,
> > +
> > + F_PWR_MODE, F_LOW_POWER_BW,
> > +
> > + F_X_POLARITY, F_Y_POLARITY, F_Z_POLARITY, F_X_Y_SWAP,
> > +
> > + F_ORIENT_INT_EN, F_S_TAP_INT_EN, F_D_TAP_INT_EN, F_ACTIVE_INT_EN_Z,
> > + F_ACTIVE_INT_EN_Y, F_ACTIVE_INT_EN_X,
> > +
> > + F_NEW_DATA_INT_EN, F_FREEFALL_INT_EN,
> > +
> > + F_INT1_ORIENT, F_INT1_S_TAP, F_INT1_D_TAP, F_INT1_ACTIVE,
> > + F_INT1_FREEFALL,
> > +
> > + F_INT1_NEW_DATA,
> > +
> > + F_INT1_OD, F_INT1_LVL,
> > +
> > + F_RESET_INT, F_LATCH_INT,
> > +
> > + F_FREEFALL_MODE, F_FREEFALL_HY,
> > +
> > + F_ACTIVE_DUR,
> > +
> > + F_TAP_QUIET, F_TAP_SHOCK, F_TAP_DUR,
> > +
> > + F_TAP_TH,
> > +
> > + F_ORIENT_HYST, F_ORIENT_BLOCKING, F_ORIENT_MODE,
> > +
> > + F_Z_BLOCKING,
> > +
> > + F_MAX_FIELDS,
>
> Not sure why you put those blank lines herey, it makes code not compact.
>
Here I use blank lines to split fields from different registers.
In other words, in the msa311_fields enum one line contains fields from one
register. But for some heavy registers (like TAP_ACTIVE_STS) we have so many
fields and their declaration doesn't fit to 80 symbols.
So I've made a decision to split registers using blank lines.
...
> > +struct msa311_priv {
>
> > + struct i2c_client *i2c;
>
> Not sure you need this. Dropping i2c dependency from this structure
> allows much easier to add, e.g. SPI support of the same hardware.
>
Mainly I use i2c pointer in the probe() path, and you are right, we can
change i2c pointer to dev and generalize msa311_priv struct from bus
perspective.
> > + struct mutex lock; /* state guard */
> > +
> > + struct iio_trigger *new_data_trig;
> > +
> > + struct regmap *regs;
>
> I believe this is used most, so making this a first member in the
> structure saves some instructions (check with bloat-o-meter).
>
Are you talking about archs where offset calculation adds more bytes to
instruction? And when offset equals to 0, we can save some space.
...
> > + struct regmap_field *fields[F_MAX_FIELDS];
> > +};
>
> ...
>
> > + wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
>
> This looks very odd from a physics perspective: sec * sec * sec == sec ?!
>
> Perhaps you meant some HZ* macros from units.h?
>
I suppose because of UHZ calculation I have to use NANO instead of
USEC_PER_SEC in the following line:
freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC +
msa311_odr_table[odr].val2;
But below line is right from physics perspective. 1sec = 1/Hz, so
msec = (USEC_PER_SEC / freq_uhz) * MSEC_PER_SEC:
wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
Or do you mean that I should change MSEC_PER_SEC to just MILLI?
> > + err = -EINVAL;
> > + mutex_lock(&msa311->lock);
> > + for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
> > + if (val == msa311_odr_table[odr].val &&
> > + val2 == msa311_odr_table[odr].val2) {
> > + err = msa311_set_odr(msa311, odr);
>
> > + if (err) {
> > + dev_err(dev, "cannot update freq (%d)\n", err);
> > + goto failed;
> > + }
>
> Why is this inside the loop and more important under lock? Also you
> may cover the initial error code by this message when moving it out of
> the loop and lock.
>
> Ditto for other code snippets in other function(s) where applicable.
>
Yes, I can move dev_err() outside of loop. But all ODR search loop
should be under lock fully, because other msa311 operations should not
be executed when we search proper ODR place.
...
> > + mutex_lock(&msa311->lock);
> > + err = regmap_field_write(msa311->fields[F_NEW_DATA_INT_EN], state);
> > + mutex_unlock(&msa311->lock);
>
> > +
>
> No need.
>
Sorry, I don't understand. We do not need to call it under lock, right?
I think we have to wrap it by msa311 lock, because other msa311
operations should not be executed when we enable or disable new data
interrupt (for example ODR value changing or something else).
> > + if (err)
> > + dev_err(dev,
> > + "cannot %s buffer due to new_data_int failure (%d)\n",
> > + state ? "enable" : "disable", err);
>
> str_enable_disable()
>
> ...
>
Kernel has solutions on all occasions :-)
...
> > + /* TODO: send motion events if needed */
>
> Are you going to address all TODOs? If no, drop ones that you are not
> going to address in the future, better to put into the top of the file
> comment what is supported and what's not.
>
> ...
>
Yes, I plan to maintain this driver and implement all motion event,
that's why I've placed TODO throughout the code.
> > + indio_dev->modes = INDIO_DIRECT_MODE; /* setup buffered mode later */
>
> Leaving it 0 is probably better, it's a pity that we don't have
> INDIO_NO_MODE_SET 0 there. However, I haven't checked if it's possible
> to leave it unset like this.
>
> ...
>
I set INDIO_DIRECT_MODE by default, because this driver support noirq
mode, when device tree doesn't have irq setup.
...
> > + mutex_lock(&msa311->lock);
> > + err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
> > + mutex_unlock(&msa311->lock);
>
> > +
>
> No need.
>
Again I don't understand why, sorry. We do not want to get sporadic
MSA311 attributes changing during power mode transition from another
userspace process.
--
Thank you,
Dmitry
