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"
+
+ 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.
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)"
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
