On Wed, 24 Apr 2024 02:16:06 +0300 Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@xxxxxxxxxxxxxxxxx> wrote: > > > > From: Ondrej Jirman <megi@xxxxxx> > > > > VDD power input can be used to completely power off the chip during > > system suspend. Do so if available. > > ... > > > ret = stk3310_init(indio_dev); > > if (ret < 0) > > - return ret; > > + goto err_vdd_disable; > > This is wrong. You will have the regulator being disabled _before_ > IRQ. Note, that the original code likely has a bug which sets states > before disabling IRQ and removing a handler. > > Side note, you may make the driver neater with help of > > struct device *dev = &client->dev; > > defined in this patch. > > ... > > > static int stk3310_suspend(struct device *dev) > > { > > struct stk3310_data *data; > > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > > Side note: This may be updated (in a separate change) to use > dev_get_drvdata() directly. > > Jonathan, do we have something like iio_priv_from_drvdata(struct > device *dev)? Seems many drivers may utilise it. Not yet, but I'm not sure it's a good idea as there is no inherent reason to assume the drvdata is a struct iio_dev. It often is but adding a function that assumes that is a path to subtle bugs. Jonathan > > > } > > ... > > > static int stk3310_resume(struct device *dev) > > Ditto. > > -- > With Best Regards, > Andy Shevchenko