| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aPek1GqhhyOWFfLG@smile.fi.intel.com>
Date: Tue, 21 Oct 2025 18:20:52 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Akhilesh Patil <akhilesh@...iitb.ac.in>
Cc: jic23@...nel.org, dlechner@...libre.com, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, nuno.sa@...log.com,
andy@...nel.org, marcelo.schmitt1@...il.com, vassilisamir@...il.com,
salah.triki@...il.com, skhan@...uxfoundation.org,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, akhileshpatilvnit@...il.com
Subject: Re: [PATCH v3 2/2] iio: pressure: adp810: Add driver for adp810
sensor
On Tue, Oct 21, 2025 at 11:20:30AM +0530, Akhilesh Patil wrote:
> Add driver for Aosong adp810 differential pressure and temperature sensor.
> This sensor provides an I2C interface for reading data.
> Calculate CRC of the data received using standard crc8 library to verify
> data integrity.
Thanks for an update! Looks almost good to me, some comments below.
...
> +M: Akhilesh Patil <akhilesh@...iitb.ac.in>
> +L: linux-iio@...r.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
Putting it here makes checkpatch unhappy. If someone thinks that is a false
positive of the tool, perhaps one needs to fix that.
> +F: drivers/iio/pressure/adp810.c
...
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_ABP060MG) += abp060mg.o
> +obj-$(CONFIG_ADP810) += adp810.o
> obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
> obj-$(CONFIG_BMP280) += bmp280.o
> bmp280-objs := bmp280-core.o bmp280-regmap.o
> @@ -15,6 +16,7 @@ obj-$(CONFIG_DPS310) += dps310.o
> obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
> obj-$(CONFIG_HID_SENSOR_PRESS) += hid-sensor-press.o
> obj-$(CONFIG_HP03) += hp03.o
> +obj-$(CONFIG_HP206C) += hp206c.o
> obj-$(CONFIG_HSC030PA) += hsc030pa.o
> obj-$(CONFIG_HSC030PA_I2C) += hsc030pa_i2c.o
> obj-$(CONFIG_HSC030PA_SPI) += hsc030pa_spi.o
> @@ -34,11 +36,9 @@ obj-$(CONFIG_SDP500) += sdp500.o
> obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o
> st_pressure-y := st_pressure_core.o
> st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o
> +obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> +obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> obj-$(CONFIG_T5403) += t5403.o
> -obj-$(CONFIG_HP206C) += hp206c.o
> obj-$(CONFIG_ZPA2326) += zpa2326.o
> obj-$(CONFIG_ZPA2326_I2C) += zpa2326_i2c.o
> obj-$(CONFIG_ZPA2326_SPI) += zpa2326_spi.o
> -
> -obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> -obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
I would split order fix into a separate change, but if maintainers are okay
with this approach, I would not object.
...
> +#include <linux/cleanup.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/unaligned.h>
Something is still missing.
...
> +struct adp810_read_buf {
> + __be16 dp;
> + u8 dp_crc;
> + __be16 tmp;
> + u8 tmp_crc;
> + __be16 sf;
> + u8 sf_crc;
All these types are provided in types.h
> +} __packed;
...
> +static int adp810_measure(struct adp810_data *data, struct adp810_read_buf *buf)
> +{
> + struct i2c_client *client = data->client;
> + struct device *dev = &client->dev;
> + int ret;
> + u16 trig_cmd = ADP810_TRIGGER_COMMAND;
Shouldn't this be __be16 or __le16? Or is that really a full 16-bit command?
I have a gut feeling that this should be u8 x[2] = { ... }; instead.
> + /* Send trigger to the sensor for measurement */
> + ret = i2c_master_send(client, (char *)&trig_cmd, sizeof(trig_cmd));
> + if (ret < 0) {
> + dev_err(dev, "Error sending trigger command\n");
> + return ret;
> + }
> + if (ret != sizeof(trig_cmd))
> + return -EIO;
-EIO is defined down from linux/errno.h.
> + /*
> + * Wait for the sensor to acquire data. As per datasheet section 5.3.1,
> + * wait for at least 10ms before reading measurements from the sensor.
> + */
> + msleep(ADP810_MEASURE_LATENCY_MS);
> +
> + /* Read sensor values */
> + ret = i2c_master_recv(client, (char *)buf, sizeof(*buf));
> + if (ret < 0) {
> + dev_err(dev, "Error reading from sensor\n");
> + return ret;
> + }
> + if (ret != sizeof(*buf))
> + return -EIO;
> +
> + /* CRC checks */
> + crc8_populate_msb(crc_table, ADP810_CRC8_POLYNOMIAL);
> + if (buf->dp_crc != crc8(crc_table, (u8 *)&buf->dp, 0x2, CRC8_INIT_VALUE)) {
> + dev_err(dev, "CRC error for pressure\n");
> + return -EIO;
> + }
> +
> + if (buf->tmp_crc != crc8(crc_table, (u8 *)&buf->tmp, 0x2, CRC8_INIT_VALUE)) {
> + dev_err(dev, "CRC error for temperature\n");
> + return -EIO;
> + }
> +
> + if (buf->sf_crc != crc8(crc_table, (u8 *)&buf->sf, 0x2, CRC8_INIT_VALUE)) {
> + dev_err(dev, "CRC error for scale\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}
...
> + indio_dev->num_channels = ARRAY_SIZE(adp810_channels);
ARRAY_SIZE() is defined in a specific header.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists