| 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: <20250112110130.6fbb848d@jic23-huawei>
Date: Sun, 12 Jan 2025 11:01:30 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Vasiliy Doylov via B4 Relay
<devnull+nekodevelopper.gmail.com@...nel.org>
Cc: nekodevelopper@...il.com, Lars-Peter Clausen <lars@...afoo.de>, Rob
Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 3/4] iio: accel: mc3230: add mc3510c support
On Sat, 11 Jan 2025 23:11:08 +0300
Vasiliy Doylov via B4 Relay <devnull+nekodevelopper.gmail.com@...nel.org> wrote:
> From: Vasiliy Doylov <nekodevelopper@...il.com>
>
> This commit integrates support for the mc3510c into the mc3230 driver.
>
> Tested on Huawei MediaPad T3 10 (huawei-agassi)
>
> Signed-off-by: Vasiliy Doylov <nekodevelopper@...il.com>
General approach to this sort of change is a first 'no operation' patch
that refactors the driver to allow for multiple device support, and a second
patch that adds the support. In this case the second patch is very simple
though so I don't mind that much.
Mostly looks good, but a few things that are non obvious the first
time you write a patch like this. Mostly avoiding pitfalls we have fallen
down in the past ;)
Jonathan
> ---
> drivers/iio/accel/mc3230.c | 55 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c
> index 3cad6f2d7a2a79df38f90e5656763f6ed019a920..ebbb96c658d87a83007c7c3c7212ce9ebf039963 100644
> --- a/drivers/iio/accel/mc3230.c
> +++ b/drivers/iio/accel/mc3230.c
> @@ -22,20 +22,41 @@
> #define MC3230_MODE_OPCON_STANDBY 0x03
>
> #define MC3230_REG_CHIP_ID 0x18
> -#define MC3230_CHIP_ID 0x01
> -
> #define MC3230_REG_PRODUCT_CODE 0x3b
> -#define MC3230_PRODUCT_CODE 0x19
>
> /*
> * The accelerometer has one measurement range:
> *
> * -1.5g - +1.5g (8-bit, signed)
> *
> - * scale = (1.5 + 1.5) * 9.81 / (2^8 - 1) = 0.115411765
> */
>
> -static const int mc3230_nscale = 115411765;
> +enum mc3xxx_chips {
In IIO drivers avoid use of wild cards in naming of anything.
They go wrong far too often as other parts match the coding and
drivers start supporting additional devices that don't
Name everything after the first supported part unless it applies
only to a different device, in which case name it after the first
device it applies to. This is a hard learned lesson over the years!
> + MC3230,
> + MC3510C,
> +};
> +
> +struct mc3xxx_chip_info {
> + const char *name;
> + const u8 chip_id;
> + const u8 product_code;
> + const int scale;
> +};
> +
> +static struct mc3xxx_chip_info mc3xxx_chip_info_tbl[] = {
> + [MC3230] = {
> + .name = "mc3230",
> + .chip_id = 0x01,
> + .product_code = 0x19,
> + .scale = 115411765, // (1.5 + 1.5) * 9.81 / (2^8 - 1) = 0.115411765
As noted in Markuss' review /* */ for comments preferred for consistency
reasons.
> + },
> + [MC3510C] = {
> + .name = "mc3510c",
> + .chip_id = 0x23,
> + .product_code = 0x10,
> + .scale = 625000000, // Was obtained empirically
> + },
> +};
We used to do this table thing a lot, but have over time come to conclusion
it is much clearer just to have separate named structures and no enum.
struct mxc3230_chip_info mx3230_chip_info = {
};
struct mxc3230_chip_info mx3510c_chip_info = {
};
etc
>
> #define MC3230_CHANNEL(reg, axis) { \
> .type = IIO_ACCEL, \
> @@ -50,6 +71,7 @@ static const int mc3230_nscale = 115411765;
> struct mc3230_data {
> struct i2c_client *client;
> struct iio_mount_matrix orientation;
> + const struct mc3xxx_chip_info *chip_info;
> };
>
> static const struct iio_mount_matrix *
> @@ -111,7 +133,7 @@ static int mc3230_read_raw(struct iio_dev *indio_dev,
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> *val = 0;
> - *val2 = mc3230_nscale;
> + *val2 = data->chip_info->scale;
> return IIO_VAL_INT_PLUS_NANO;
> default:
> return -EINVAL;
> @@ -127,15 +149,23 @@ static int mc3230_probe(struct i2c_client *client)
> int ret;
> struct iio_dev *indio_dev;
> struct mc3230_data *data;
> + const struct mc3xxx_chip_info *chip_info;
>
> + chip_info = i2c_get_match_data(client);
Whilst very unlikely to fail (it won't), usual convention is to check
the chip_info is not NULL anyway. Maybe in future that function will gain
paths that more likely to fail than today so good to be paranoid on this one.
We aren't completely consistent on this, so some drivers may not check it.
> /* First check chip-id and product-id */
> ret = i2c_smbus_read_byte_data(client, MC3230_REG_CHIP_ID);
> - if (ret != MC3230_CHIP_ID)
> + if (ret != chip_info->chip_id) {
> + dev_err(&client->dev,
> + "chip id check fail: 0x%x != 0x%x !\n", ret, chip_info->chip_id);
dev_info() and do not fail on this. Also, indent the message to align
below the &
Hard matches against chip IDs break the concept of Device Tree fallback
compatibles. If a new device is released that is backwards compatible
with an older one we want the driver to work. It is fine to print
a message thought to say we don't recognise it.
> return (ret < 0) ? ret : -ENODEV;
> + }
>
> ret = i2c_smbus_read_byte_data(client, MC3230_REG_PRODUCT_CODE);
> - if (ret != MC3230_PRODUCT_CODE)
> + if (ret != chip_info->product_code) {
> + dev_err(&client->dev,
> + "product code check fail: 0x%x != 0x%x !\n", ret, chip_info->product_code);
> return (ret < 0) ? ret : -ENODEV;
As above.
> + }
Powered by blists - more mailing lists