On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote: > On Sun, Apr 14, 2024 at 8:57 PM 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. > > ... > > > #include <linux/iio/events.h> > > #include <linux/iio/iio.h> > > #include <linux/iio/sysfs.h> > > > +#include <linux/regulator/consumer.h> > > Move it to be ordered and add a blank line to separate iio/*.h group. > > ... > > > + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); > > + if (IS_ERR(data->vdd_reg)) { > > + ret = PTR_ERR(data->vdd_reg); > > + if (ret == -ENODEV) > > + data->vdd_reg = NULL; > > > + else > > Redundant 'else' when you follow the pattern "check for error condition first". > > > + return dev_err_probe(&client->dev, ret, > > + "get regulator vdd failed\n"); > > + } > > ... > > > + if (data->vdd_reg) { > > + ret = regulator_enable(data->vdd_reg); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, > > + "regulator vdd enable failed\n"); > > + > > + usleep_range(1000, 2000); > > fsleep() > > > + } > > ... > > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > > + if (data->vdd_reg) > > + regulator_disable(data->vdd_reg); > > I forgot to check the order of freeing resources, be sure you have no > devm_*() releases happening before this call. If I understand what you're saying, this should be fine. The driver just uses devm to clean up acquired resources after remove is called. Or am I missing something and resources could be freed before calling stk3310_remove? > ... > > > + usleep_range(1000, 2000); > > fsleep() Everything else makes sense, I'll include those in v2 along with a patch to switch stk3310_init to dev_err_probe. Thanks for taking the time to review - Aren