Hello Jonathan, Sorry for the delayed response. On Sun, Jun 19, 2022 at 01:27:03PM +0100, Jonathan Cameron wrote: > On Thu, 16 Jun 2022 10:42:14 +0000 > Dmitry Rokosov <DDRokosov@xxxxxxxxxxxxxx> wrote: > > > MSA311 is a tri-axial, low-g accelerometer with I2C digital output for > > sensitivity consumer applications. It has dynamical user selectable full > > scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements > > with output data rates from 1Hz to 1000Hz. > > > > Datasheet can be found at following URL: > > https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf > > > > This driver supports following MSA311 features: > > - IIO interface > > - Different power modes: NORMAL and SUSPEND (using pm_runtime) > > - ODR (Output Data Rate) selection > > - Scale and samp_freq selection > > - IIO triggered buffer, IIO reg access > > - NEW_DATA interrupt + trigger > > > > Below features to be done: > > - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL > > - Low Power mode > > > > Signed-off-by: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> > Hi Dmitry, > > A few things I missed before + I'm still not happy with the runtime > pm handling. One case that isn't covered well is !CONFIG_RUNTIME_PM > > Thanks, > > Jonathan > ... > > +static irqreturn_t msa311_buffer_thread(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct msa311_priv *msa311 = iio_priv(indio_dev); > > + struct device *dev = &msa311->i2c->dev; > > + const struct iio_chan_spec *chan; > > + __le16 axis; > > + int bit = 0, err, i = 0; > > + > > + /* Ensure correct alignment of time stamp when present */ > > + struct { > > + __le16 channels[MSA311_SI_Z + 1]; > > + s64 ts __aligned(8); > > + } buf; > > + > > + memset(&buf, 0, sizeof(buf)); > > + > > + mutex_lock(&msa311->lock); > > + > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > + indio_dev->masklength) { > > + chan = &msa311_channels[bit]; > > Nothing to do with your driver, but feels like it's worth > exploring a > for_each_chan_in_iio_scan(struct iio_chan_spec, struct iio_dev) macro. > > I'll add that to my todo list. > If you don't mind, I can prepare such a patch. ... > When this unwind we will disable autosuspend etc, but leave the device > in whatever state it happens to be in at that stage (if I understand > this handling correctly). That might seem like a bad thing, but if > we register a devm_add_action_or_reset() callback before this which > disables the device independently of anything to do with runtime PM, > then the device will > a) Be turned off as desired. > b) It'll still be turned off even if runtime pm is disabled for the system > which is nice. > > Given the particular state register must be writeable and is presumably > idempotent, can we just call > err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_SUSPEND); > Unconditionally in such a callback? I think it's a good idea. I didn't think about the configs when runtime pm is disabled. So looks like we need to make sure that device is workable from a pm perspective, and it is achievable only using a direct msa311_set_pwr_mode() call as you suggested below. > > + err = devm_pm_runtime_enable(dev); > > + if (err) > > + return err; > > + > > + /* Resume msa311 logic before any interactions with registers */ > > + err = pm_runtime_resume_and_get(dev); > I missed this before, but if runtime pm is disabled, this won't do anything > so device won't be powered on. > > One common(ish) way to handle this is the following sequence. > > 1) Power up supply regs etc and a register a devm_ callback to turn them off again. > 2) Put the device into a non suspend state (not using runtime pm calls). > 3) Register a callback to turn it off again (that is safe against it being > turned off via another path such as runtime pm). > 4) pm_runtime_set_active() to let the runtime pm code know it is turned on. > 5) devm_pm_runtime_enable() > 6) autosuspend setup and enablement. > > If runtime pm isn't enabled then only 1-3 happen. We waste power but the > device works. -- Thank you, Dmitry
