Message-ID: <1406f120-9542-d90e-1258-5a8a300c1822@wanadoo.fr>
Date:   Wed, 3 Aug 2022 21:18:26 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     ddrokosov@...rdevices.ru
Cc:     andy.shevchenko@...il.com, devicetree@...r.kernel.org,
        jic23@...nel.org, kernel@...rdevices.ru, lars@...afoo.de,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        robh+dt@...nel.org, shawnguo@...nel.org, stano.jakubek@...il.com,
        stephan@...hold.net
Subject: Re: [PATCH v4 2/3] iio: add MEMSensing MSA311 3-axis accelerometer
 driver

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@...lic.gmane.org>
>>> ---
>>>    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
> 

Powered by blists - more mailing lists