From: andy.shevchenko@gmail.com
To: Andreas Klinger <ak@it-klinger.de>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
Jonathan Cameron <jic23@kernel.org>,
Lars-Peter Clausen <lars@metafoo.de>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Angel Iglesias <ang.iglesiasg@gmail.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor
Date: Mon, 29 May 2023 01:28:47 +0300 [thread overview]
Message-ID: <ZHPVn4xzErSZfqVy@surfacebook> (raw)
In-Reply-To: <ZGNp3SqyOJeEcLsj@arbad>
Tue, May 16, 2023 at 01:32:45PM +0200, Andreas Klinger kirjoitti:
> Honeywell mprls0025pa is a series of pressure sensors.
>
> Add initial I2C support.
>
> Note:
> - IIO buffered mode is supported
> - SPI mode is not supported
...
> + * Data sheet:
Datasheet
> + * 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
Is it URL? Can we have it clickable, i.e. unwrapped?
...
Missing bits.h
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/math64.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/units.h>
...
> +/*
> + * support _RAW sysfs interface:
Support
> + *
> + * Calculation formula from the datasheet:
> + * pressure = (press_cnt - outputmin) * scale + pmin
> + * with:
> + * * pressure - measured pressure in Pascal
> + * * press_cnt - raw value read from sensor
> + * * pmin - minimum pressure range value of sensor (data->pmin)
> + * * pmax - maximum pressure range value of sensor (data->pmax)
> + * * outputmin - minimum numerical range raw value delivered by sensor
> + * (mpr_func_spec.output_min)
> + * * outputmax - maximum numerical range raw value delivered by sensor
> + * (mpr_func_spec.output_max)
> + * * scale - (pmax - pmin) / (outputmax - outputmin)
> + *
> + * formula of the userspace:
> + * pressure = (raw + offset) * scale
> + *
> + * Values given to the userspace in sysfs interface:
> + * * raw - press_cnt
> + * * offset - (-1 * outputmin) - pmin / scale
> + * note: With all sensors from the datasheet pmin = 0
> + * which reduces the offset to (-1 * outputmin)
> + */
...
> +/*
> + * transfer function A: 10% to 90% of 2^24
> + * transfer function B: 2.5% to 22.5% of 2^24
> + * transfer function C: 20% to 80% of 2^24
> + */
Kernel doc?
> +enum mpr_func_id {
> + MPR_FUNCTION_A,
> + MPR_FUNCTION_B,
> + MPR_FUNCTION_C,
> +};
...
> +struct mpr_func_spec {
> + u32 output_min;
> + u32 output_max;
> +};
Can the linear_range.h be used for this?
...
> +struct mpr_data {
> + struct i2c_client *client;
> + struct mutex lock; /*
> + * access to device during read
> + */
> + u32 pmin; /* minimal pressure in pascal */
> + u32 pmax; /* maximal pressure in pascal */
> + enum mpr_func_id function; /* transfer function */
> + u32 outmin; /*
> + * minimal numerical range raw
> + * value from sensor
> + */
> + u32 outmax; /*
> + * maximal numerical range raw
> + * value from sensor
> + */
> + int scale; /* int part of scale */
> + int scale2; /* nano part of scale */
> + int offset; /* int part of offset */
> + int offset2; /* nano part of offset */
> + struct gpio_desc *gpiod_reset; /* reset */
> + int irq; /*
> + * end of conversion irq;
> + * used to distinguish between
> + * irq mode and reading in a
> + * loop until data is ready
> + */
> + struct completion completion; /* handshake from irq to read */
> + struct mpr_chan chan; /*
> + * channel values for buffered
> + * mode
> + */
Why not kernel doc?
> +};
...
> +static void mpr_reset(struct mpr_data *data)
> +{
> + if (data->gpiod_reset) {
Actually this dups gpiod_*() checks, so only needed by udelay().
> + gpiod_set_value(data->gpiod_reset, 0);
> + udelay(10);
> + gpiod_set_value(data->gpiod_reset, 1);
Why not sleeping for all of them?
> + }
> +}
...
> + if (ret != sizeof(wdata)) {
> + dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
> + (u32)sizeof(wdata));
Casting? Use proper specifier, i.e. %zu.
> + return -EIO;
> + }
...
> + /* wait until status indicates data is ready */
> + for (i = 0; i < nloops; i++) {
> + /*
> + * datasheet only says to wait at least 5 ms for the
Datasheet
> + * data but leave the maximum response time open
> + * --> let's try it nloops (10) times which seems to be
> + * quite long
> + */
> + usleep_range(5000, 10000);
> + status = i2c_smbus_read_byte(data->client);
> + if (status < 0) {
> + dev_err(dev,
> + "error while reading, status: %d\n",
> + status);
> + return status;
> + }
> + if (!(status & MPR_I2C_BUSY))
> + break;
> + }
> + if (i == nloops) {
> + dev_err(dev, "timeout while reading\n");
> + return -ETIMEDOUT;
> + }
iopoll.h provides a macro for this. Reduces codebase a lot in this case.
> + }
...
> + dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
> + (u32)sizeof(buf));
Use proper specifier.
...
> + if (buf[0] & MPR_I2C_BUSY) {
> + /*
> + * it should never be the case that status still indicates
> + * business
> + */
> + dev_err(dev, "data still not ready: %08x\n", buf[0]);
Why 8? Is the buffer is of 32-bit values?
> + return -ETIMEDOUT;
> + }
...
> +err:
err_unlock:
> + mutex_unlock(&data->lock);
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
...
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return dev_err_probe(dev, -ENOMEM, "couldn't get iio_dev\n");
-ENOMEM shouldn't be without error message, we will get one in that case.
...
> + if (dev_fwnode(dev)) {
Why not simply use defaults?
> + ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
> + &data->pmin);
> + 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,
> + "honeywell,pmax-pascal could not be read\n");
> + ret = device_property_read_u32(dev,
> + "honeywell,transfer-function", &data->function);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "honeywell,transfer-function could not be read\n");
> + if (data->function > MPR_FUNCTION_C)
> + return dev_err_probe(dev, -EINVAL,
-ERANGE ?
> + "honeywell,transfer-function %d invalid\n",
> + data->function);
> + } else {
> + /* 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;
> + }
...
> + /*
> + * multiply with NANO before dividing by scale and later divide by NANO
Multiply
> + * again.
> + */
...
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "iio triggered buffer setup failed\n");
One line?
...
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "unable to register iio device\n");
Ditto.
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2023-05-28 22:28 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-16 11:30 [PATCH v5 0/3] Support Honeywell mprls0025pa pressure sensor Andreas Klinger
2023-05-16 11:32 ` [PATCH v5 1/3] dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor Andreas Klinger
2023-05-16 11:32 ` [PATCH v5 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor Andreas Klinger
2023-05-28 22:28 ` andy.shevchenko [this message]
2023-06-04 13:07 ` Jonathan Cameron
2023-05-16 11:33 ` [PATCH v5 3/3] MAINTAINERS: Add Honeywell mprls0025pa sensor Andreas Klinger
2023-05-20 16:24 ` [PATCH v5 0/3] Support Honeywell mprls0025pa pressure sensor Jonathan Cameron
2023-05-28 22:09 ` andy.shevchenko
2023-06-04 13:07 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZHPVn4xzErSZfqVy@surfacebook \
--to=andy.shevchenko@gmail.com \
--cc=ak@it-klinger.de \
--cc=ang.iglesiasg@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.