Messages in this thread | | | Date | Sun, 28 Apr 2024 17:34:22 +0100 | From | Jonathan Cameron <> | Subject | Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend |
| |
On Wed, 24 Apr 2024 02:16:06 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolutionorg> wrote: > > > > From: Ondrej Jirman <megi@xff.cz> > > > > 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
| |