by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 05/10] iio: pressure: mprls0025pa.c fix error flag check

On Sun, 24 Dec 2023 16:34:50 +0200
Petre Rodan <[email protected]> wrote:

> Take into account all 3 error flags while interacting with the sensor.
> Based on the datasheet, in table 14 on page 14, the status byte
> contains:
> bit 5 busy flag - 1 if device is busy
> bit 2 memory integrity/error flag - 1 if integrity test failed
> bit 0 math saturation - 1 if internal math saturation has occurred
>
> Signed-off-by: Petre Rodan <[email protected]>
> Signed-off-by: Andreas Klinger <[email protected]>
I've not problem with the patch, but the 'fix' in the titles means this should
have a fixes tag. I'm not sure that improving the resilience to device errors
is something we count as a fix however. Maybe more of a feature or improvement
in which case don't say fix.
Also, drop the .c in the patch title to be inline with similar patches in IIO.

Thanks,

Jonathan

> ---
> drivers/iio/pressure/mprls0025pa.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index e3f0de020a40..233cc1dc38ad 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -39,6 +39,8 @@
> #define MPR_I2C_MEMORY BIT(2) /* integrity test passed */
> #define MPR_I2C_MATH BIT(0) /* internal math saturation */
>
> +#define MPR_I2C_ERR_FLAG (MPR_I2C_BUSY | MPR_I2C_MEMORY | MPR_I2C_MATH)
> +
> /*
> * support _RAW sysfs interface:
> *
> @@ -213,7 +215,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> status);
> return status;
> }
> - if (!(status & MPR_I2C_BUSY))
> + if (!(status & MPR_I2C_ERR_FLAG))
> break;
> }
> if (i == nloops) {
> @@ -233,7 +235,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> return -EIO;
> }
>
> - if (buf[0] & MPR_I2C_BUSY) {
> + if (buf[0] & MPR_I2C_ERR_FLAG) {
> /*
> * it should never be the case that status still indicates
> * business
> --
> 2.41.0
>
>

2023-12-26 16:39:25

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 06/10] iio: pressure: mprls0025pa.c remove dangerous defaults

On Sun, 24 Dec 2023 16:34:51 +0200
Petre Rodan <[email protected]> wrote:

> This driver supports 32*3 combinations of fixed ranges and transfer
> functions, plus custom ranges.
>
> So statistically a user has more than 99% chance that the provided
> default configuration will generate invalid pressure readings if the
> bindings are not initialized and the driver is instantiated via sysfs.

I guess 99% is strong enough that it's unlikely we will break anyone so
fair enough to drop this attempt to guess the values.

>
> The current patch removes this loophole making sure the driver loads
> only if the dt has been initialized.

Drop the reference to DT. It's correctly using generic firmware accessors
so should work fine with ACPI PRP0001 for example as well as DT.

Usually we just refer to the need for firmware properties.
There are lots of places they could be coming from.

>
> Signed-off-by: Petre Rodan <[email protected]>
> Signed-off-by: Andreas Klinger <[email protected]>
> ---
> drivers/iio/pressure/mprls0025pa.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index 233cc1dc38ad..63c46592956f 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -375,11 +375,8 @@ static int mpr_probe(struct i2c_client *client)
> "honeywell,transfer-function %d invalid\n",
> data->function);
> } else {

I'd prefer the condition flipped even though patch will be more noisy.

if (!dev_fwnode(dev))
return dev_err_probe()...

...

As end result will be more readable.

> - /* when loaded as i2c device we need to use default values */
> - dev_notice(dev, "firmware node not found; using defaults\n");
> - data->pmin = 0;
> - data->pmax = 172369; /* 25 psi */
> - data->function = MPR_FUNCTION_A;
> + return dev_err_probe(dev, -EINVAL,
> + "driver needs to be initialized in the dt\n");
> }
>
> data->outmin = mpr_func_spec[data->function].output_min;
> --
> 2.41.0
>

2023-12-26 16:49:39

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 08/10] iio: pressure: mprls0025pa.c refactor

On Sun, 24 Dec 2023 16:34:53 +0200
Petre Rodan <[email protected]> wrote:

> Refactor driver by splitting the code into core and i2c.
>
> Seemingly redundant read/write function parameters are required for
> compatibility with the SPI driver.
>
> Signed-off-by: Petre Rodan <[email protected]>
> Signed-off-by: Andreas Klinger <[email protected]>

A few minor comments inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index e14cdee7989f..cb5d6c0cca7e 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
...

> -static int mpr_probe(struct i2c_client *client)
> +int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
> {
> int ret;
> struct mpr_data *data;
> struct iio_dev *indio_dev;
> - struct device *dev = &client->dev;
> s64 scale, offset;
> u32 func;
>
> - if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE))
> - return dev_err_probe(dev, -EOPNOTSUPP,
> - "I2C functionality not supported\n");
> -
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> - return dev_err_probe(dev, -ENOMEM, "couldn't get iio_dev\n");
> + return -ENOMEM;
>
> data = iio_priv(indio_dev);
> - data->client = client;
> - data->irq = client->irq;
> + data->dev = dev;
> + data->ops = ops;
> + data->irq = irq;
>
> mutex_init(&data->lock);
> init_completion(&data->completion);
> @@ -350,32 +284,36 @@ static int mpr_probe(struct i2c_client *client)
> return dev_err_probe(dev, ret,
> "can't get and enable vdd supply\n");
>
> - if (dev_fwnode(dev)) {

So you now rely on device_property_read_u32() failing I guess to cover this
which is fine. However do that in the earlier patch instead of burying that
change in here.

Should make it easier to tell what changed here as well.

> - ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
> + ret = data->ops->init(data->dev);
> + if (ret)
> + return ret;
> +
> + ret = device_property_read_u32(dev,
> + "honeywell,transfer-function", &func);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "honeywell,transfer-function could not be read\n");
> + data->function = func - 1;
> + if (data->function > MPR_FUNCTION_C)
> + return dev_err_probe(dev, -EINVAL,
> + "honeywell,transfer-function %d invalid\n",
> + data->function);
> +
> + ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
> &data->pmin);
> - if (ret)
> - return dev_err_probe(dev, ret,
> + if (ret)
> + return dev_err_probe(dev, ret,
> "honeywell,pmin-pascal could not be read\n");
>
> - ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
> - &data->pmax);
> - if (ret)
> - return dev_err_probe(dev, ret,
> + ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
> + &data->pmax);
> + if (ret)
> + return dev_err_probe(dev, ret,
> "honeywell,pmax-pascal could not be read\n");
> - ret = device_property_read_u32(dev,
> - "honeywell,transfer-function", &func);
> - if (ret)
> - return dev_err_probe(dev, ret,
> - "honeywell,transfer-function could not be read\n");
> - data->function = func - 1;
> - if (data->function > MPR_FUNCTION_C)
> - return dev_err_probe(dev, -EINVAL,
> - "honeywell,transfer-function %d invalid\n",
> - data->function);
> - } else {
> +
> + if (data->pmin >= data->pmax)
> return dev_err_probe(dev, -EINVAL,
> - "driver needs to be initialized in the dt\n");
> - }
> + "pressure limits are invalid\n");
>
> data->outmin = mpr_func_spec[data->function].output_min;
> data->outmax = mpr_func_spec[data->function].output_max;
> @@ -394,7 +332,7 @@ static int mpr_probe(struct i2c_client *client)
>
> if (data->irq > 0) {
> ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> - IRQF_TRIGGER_RISING, client->name, data);
> + IRQF_TRIGGER_RISING, dev_name(dev), data);

Even though you'll change it again here, would have been nice to have
the alignment fixed in the earlier patch then the code update here.

> if (ret)
> return dev_err_probe(dev, ret,
> "request irq %d failed\n", data->irq);
> @@ -421,29 +359,8 @@ static int mpr_probe(struct i2c_client *client)
>
> return 0;
> }

...

> diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
> new file mode 100644
> index 000000000000..89d6a206192b
> --- /dev/null
> +++ b/drivers/iio/pressure/mprls0025pa_i2c.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * MPRLS0025PA - Honeywell MicroPressure pressure sensor series driver
> + *
> + * Copyright (c) Andreas Klinger <[email protected]>
> + *
> + * Data sheet:
> + * https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +
> +#include <linux/iio/iio.h>

Why include this? Can't see an IIO specific stuff in here.

> +
> +#include "mprls0025pa.h"
> +
> +static int mpr_i2c_init(struct device *unused)
> +{
> + return 0;
> +}
> +
> +static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
> +{
> + int ret;
> + struct i2c_client *client = to_i2c_client(data->dev);
> +
> + if (cnt > MPR_MEASUREMENT_RD_SIZE)
> + return -EOVERFLOW;
> +
> + memset(data->buffer, 0, MPR_MEASUREMENT_RD_SIZE);
> + ret = i2c_master_recv(client, data->buffer, cnt);
> + if (ret != cnt) {
> + return -EIO;
As below.

> + }
> +
> + return 0;
> +}
> +
> +static int mpr_i2c_write(struct mpr_data *data, const u8 cmd, const u8 unused)
> +{
> + int ret;
> + struct i2c_client *client = to_i2c_client(data->dev);
> + u8 wdata[MPR_PKT_SYNC_LEN];
> +
> + memset(wdata, 0, sizeof(wdata));
> + wdata[0] = cmd;
> +
> + ret = i2c_master_send(client, wdata, MPR_PKT_SYNC_LEN);
> + if (ret != MPR_PKT_SYNC_LEN) {

No {} as per Coding Style docs for single statement blocks like this.

Ideally we tend to handle ret < 0 separately from ret != MPR_PKT_SYNC_LEN
as then we don't eat a possible more useful error code.

> + return -EIO;
> + }
> +
> + return 0;
> +}

2023-12-26 16:51:52

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 10/10] iio: pressure: mprls0025pa.c add SPI driver

On Sun, 24 Dec 2023 16:34:55 +0200
Petre Rodan <[email protected]> wrote:

> Add SPI component of the driver.
>
> Tested with mprls0015pa0000sa in spi mode on BeagleBone Black on
> slightly patched 6.7.0-rc6 mainline.
>
> Tested with mprls0025pa in i2c mode on BeagleBone Black with togreg
> branch on
> git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> (tag: iio-for-6.8a)
>
> Signed-off-by: Petre Rodan <[email protected]>
> Tested-by: Andreas Klinger <[email protected]>
This and previous patch look fine to me.

Thanks,

Jonathan

2023-12-27 14:33:52

On Wed, Dec 27, 2023 at 06:34:25PM +0200, Andy Shevchenko wrote:
> On Sun, Dec 24, 2023 at 04:34:52PM +0200, Petre Rodan wrote:
> > Fix indentation and whitespace in code that will not get refactored.
> >
> > Make URL inside comment copy-paste friendly.
>
> > return dev_err_probe(dev, ret,
> > - "honeywell,pmin-pascal could not be read\n");
> > + "honeywell,pmin-pascal could not be read\n");
>
> As done elsewhere, here and in other similar places fix the indentation
> by making first character on the latter line to be in the same column as
> the first character after the opening parenthesis.

I triple-checked that I am following the max 80 column rule, the parenthesis
rule and the 'do not split printk messages' rules in all my code in these 10 patches.
precisely so I don't get feedback like this one.
if the parenthesis rule makes the line longer then 80 chars I right-align to
column 80 as seen above.
that is what I understand from the latest coding style document and that is what
I will follow.

in this particular case if I were to ignore the 80 column rule we would end up on
column 90 if I were to follow your feedback (open parenthesis is at column 45
and the error takes 45 chars more).

peter

Attachments:

(No filename) (1.23 kB)
signature.asc (849.00 B)
Download all attachments

2023-12-30 11:34:10

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 07/10] iio: pressure: mprls0025pa.c whitespace cleanup

On Wed, 27 Dec 2023 19:39:28 +0200
Petre Rodan <[email protected]> wrote:

> On Wed, Dec 27, 2023 at 06:34:25PM +0200, Andy Shevchenko wrote:
> > On Sun, Dec 24, 2023 at 04:34:52PM +0200, Petre Rodan wrote:
> > > Fix indentation and whitespace in code that will not get refactored.
> > >
> > > Make URL inside comment copy-paste friendly.
> >
> > > return dev_err_probe(dev, ret,
> > > - "honeywell,pmin-pascal could not be read\n");
> > > + "honeywell,pmin-pascal could not be read\n");
> >
> > As done elsewhere, here and in other similar places fix the indentation
> > by making first character on the latter line to be in the same column as
> > the first character after the opening parenthesis.
>
> I triple-checked that I am following the max 80 column rule, the parenthesis
> rule and the 'do not split printk messages' rules in all my code in these 10 patches.
> precisely so I don't get feedback like this one.
> if the parenthesis rule makes the line longer then 80 chars I right-align to
> column 80 as seen above.

I'm not aware of (and can't immediately see) anything about right aligning to 80
columns. It's fine to align it less if line length is long but normally people
go with aligning to one tab more than the start of the block.

> that is what I understand from the latest coding style document and that is what
> I will follow.
>
> in this particular case if I were to ignore the 80 column rule we would end up on
> column 90 if I were to follow your feedback (open parenthesis is at column 45
> and the error takes 45 chars more).

It's fine to do this in the interests of readability.

People differ in opinion on what constitutes 'significant readability' and I'd
be happy with either a shorter alignment (single tab more than line above)
or going over 80 chars in this case.

Jonathan

>
> peter
>

2023-12-30 11:35:01

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 08/10] iio: pressure: mprls0025pa.c refactor

On Wed, 27 Dec 2023 18:37:42 +0200
Andy Shevchenko <[email protected]> wrote:

> On Wed, Dec 27, 2023 at 04:33:37PM +0200, Petre Rodan wrote:
> > On Tue, Dec 26, 2023 at 04:49:22PM +0000, Jonathan Cameron wrote:
>
> ,,,
>
> > > > ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> > > > - IRQF_TRIGGER_RISING, client->name, data);
> > > > + IRQF_TRIGGER_RISING, dev_name(dev), data);
> > >
> > > Even though you'll change it again here, would have been nice to have
> > > the alignment fixed in the earlier patch then the code update here.
> >
> > I tried this, but due to the fact that the line has to be right-aligned to
> > column 80 we will still see a whitespace difference due to the length diff of
> > the name-related argument.
>
> You can split in the previous patch accordingly, so data comes to a new line.
>
> ...
>
> > > > +#include <linux/iio/iio.h>
> > >
> > > Why include this? Can't see an IIO specific stuff in here.
> >
> > tried to remove it and
> >
> > CC [M] mprls0025pa_i2c.o
> > mprls0025pa.h:89:63: error: 'IIO_DMA_MINALIGN' undeclared here (not in a function); did you mean 'ARCH_DMA_MINALIGN'?
> > 89 | u8 buffer[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
>
> > I guess it makes more sense to move it to the .h file, where buffer[] is defined.
>
> Yes, C-code and especially headers should follow IWYI principle. The real user
> of that definition is _the header_ file, and not C in this case.

Absolutely - it is clear why this should be included from the header file.

Jonathan

>
>

2024-01-06 14:03:35

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v2 07/10] iio: pressure: mprls0025pa.c whitespace cleanup

On Wed, Dec 27, 2023 at 07:39:28PM +0200, Petre Rodan wrote:
> On Wed, Dec 27, 2023 at 06:34:25PM +0200, Andy Shevchenko wrote:
> > On Sun, Dec 24, 2023 at 04:34:52PM +0200, Petre Rodan wrote:
> > > Fix indentation and whitespace in code that will not get refactored.
> > >
> > > Make URL inside comment copy-paste friendly.
> >
> > > return dev_err_probe(dev, ret,
> > > - "honeywell,pmin-pascal could not be read\n");
> > > + "honeywell,pmin-pascal could not be read\n");
> >
> > As done elsewhere, here and in other similar places fix the indentation
> > by making first character on the latter line to be in the same column as
> > the first character after the opening parenthesis.
>
> I triple-checked that I am following the max 80 column rule, the parenthesis
> rule and the 'do not split printk messages' rules in all my code in these 10 patches.
> precisely so I don't get feedback like this one.
> if the parenthesis rule makes the line longer then 80 chars I right-align to
> column 80 as seen above.
> that is what I understand from the latest coding style document and that is what
> I will follow.
>
> in this particular case if I were to ignore the 80 column rule we would end up on
> column 90 if I were to follow your feedback (open parenthesis is at column 45
> and the error takes 45 chars more).

checkpatch has got an exceptional rule _not_ to warn on the long string
literals for 10+ years. It had happened much earlier than 100 relaxation one.

--
With Best Regards,
Andy Shevchenko