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? > > + > > + if (partid == MSA311_WHO_AM_I) > > + dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid); > > + else > > + dev_warn(dev, "invalid partid (%#x), expected (%#x)\n", > > + partid, MSA311_WHO_AM_I); > > + > > + return 0; > > +} > > [...] > > > +static int msa311_probe(struct i2c_client *i2c) > > +{ > > + struct device *dev = &i2c->dev; > > + struct msa311_priv *msa311; > > + struct iio_dev *indio_dev; > > + int err; > > + > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*msa311)); > > + if (!indio_dev) > > + return dev_err_probe(dev, -ENOMEM, > > + "iio device allocation failed\n"); > > + > > + msa311 = iio_priv(indio_dev); > > + msa311->dev = dev; > > + i2c_set_clientdata(i2c, indio_dev); > > + > > + err = msa311_regmap_init(msa311); > > + if (err) > > + return err; > > + > > + mutex_init(&msa311->lock); > > + > > + msa311->vdd = devm_regulator_get_optional(dev, "vdd"); > > + if (IS_ERR(msa311->vdd)) { > > + err = PTR_ERR(msa311->vdd); > > + if (err == -ENODEV) > > + msa311->vdd = NULL; > > + else > > + return dev_err_probe(dev, PTR_ERR(msa311->vdd), > > + "cannot get vdd supply\n"); > > + } > > + > > + 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. > > + 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
