On Wed, Aug 03, 2022 at 09:18:26PM +0200, Christophe JAILLET wrote: > Le 03/08/2022 à 20:39, Dmitry Rokosov a écrit : > > Hello Christophe, > > > > Thank you for quick review > > > > On Wed, Aug 03, 2022 at 08:11:05PM +0200, Christophe JAILLET wrote: > > > Le 03/08/2022 à 15:11, Dmitry Rokosov a écrit : > > > > MSA311 is a tri-axial, low-g accelerometer with I2C digital output for > > > > sensitivity consumer applications. It has dynamic user-selectable full > > > > scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements > > > > with output data rates from 1Hz to 1000Hz. > > > > > > > > Spec: 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-i4r8oA+eLlH99rHkP+FxIw@xxxxxxxxxxxxxxxx> > > > > --- > > > > MAINTAINERS | 6 + > > > > drivers/iio/accel/Kconfig | 13 + > > > > drivers/iio/accel/Makefile | 2 + > > > > drivers/iio/accel/msa311.c | 1323 ++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 1344 insertions(+) > > > > create mode 100644 drivers/iio/accel/msa311.c > > > > > > > > > > Hi, > > > a few nits below. > > > > > > [...] > > > > > > > +static int msa311_check_partid(struct msa311_priv *msa311) > > > > +{ > > > > + struct device *dev = msa311->dev; > > > > + unsigned int partid; > > > > + int err; > > > > + > > > > + err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid); > > > > + if (err) > > > > + return dev_err_probe(dev, err, > > > > + "failed to read partid (%d)\n", err); > > > > > > No need for "(%d)" and err. > > > > > > > Do you mean for all dev_err() calls? I think sometimes it's helpful to > > know the actual error value got from external API, isn't? Could you please > > explain your point if possible? > > > > No, my comment is only for dev_err_probe() function. > Having ret for dev_err() calls is fine. > > See: https://elixir.bootlin.com/linux/v5.19/source/drivers/base/core.c#L4732 > > dev_err_probe() already has a "error %pe:..., ERR_PTR(err)" > This means that if ret = -ENOMEM: > "(%d)" --> "(-12)" > "error %pe:" --> "error -ENOMEM:" > > So there is no real need to duplicate the error code in the message itself, > it is already displayed in a human readable manner. > > What your code does would result in: > "error -ENOMEM: failed to read partid (-12)\n" https://elixir.bootlin.com/linux/v5.19/source/drivers/base/core.c#L4732> Hmm, very interesting. Thank you for good point. I will fix it in the v5, and will use %pe for all dev_err() calls, I suppose it's helpful. [...] > > > > + if (msa311->vdd) { > > > > + err = regulator_enable(msa311->vdd); > > > > + if (err) > > > > + return dev_err_probe(dev, err, > > > > + "cannot enable vdd supply\n"); > > > > + > > > > + err = devm_add_action_or_reset(dev, msa311_vdd_disable, > > > > + msa311->vdd); > > > > + if (err) { > > > > + regulator_disable(msa311->vdd); > > > > > > Double regulator_disable(), because of the _or_reset()? > > > > > > > Yep. If devm_add_action_or_reset() returns an error, we will not > > call regulator_disable() by devm subsystem. It means, we have to > > call it directly. > > No. > > See https://elixir.bootlin.com/linux/v5.19/source/include/linux/device.h#L249 > > If devm_add_action_or_reset() fails, "action" is called. This is the meaning > of the _or_reset suffix. > > So here, msa311_vdd_disable() would be called and this function is: > > +static void msa311_vdd_disable(void *vdd) > +{ > + regulator_disable(vdd); > +} > > and "vdd" will be the value of "msa311->vdd" > > So, unless I missed something obvious, your code will call twice > regulator_disable(msa311->vdd). > > One in devm_add_action_or_reset() and one explicitly after the "if (err)" > You are totally right, thanks a lot for the explanation. I will fix that in the v5. > > Hoping I'm clear and that I didn't miss something obvious. > > CJ > > > > > > > + return dev_err_probe(dev, err, > > > > + "cannot add vdd disable action\n"); > > > > + } > > > > + } > > > > + > > > > + err = msa311_check_partid(msa311); > > > > + if (err) > > > > + return err; > > > > + > > > > + err = msa311_soft_reset(msa311); > > > > + if (err) > > > > + return err; > > > > + > > > > + err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL); > > > > + if (err) > > > > + return dev_err_probe(dev, err, > > > > + "failed to power on device (%d)\n", err); > > > > > > No need for "(%d)" and err > > > > Asked for the clarification above. > > > > > > > > CJ > > > -- Thank you, Dmitry
