| 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: <CAHp75VedQvf2xwY3fDWX=FQaHyhaUSVJW3Y6Yt2ecpwru756vw@mail.gmail.com>
Date: Sun, 12 Jan 2025 16:18:46 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Antoni Pokusinski <apokusinski01@...il.com>
Cc: jic23@...nel.org, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, andrej.skvortzov@...il.com, neil.armstrong@...aro.org,
icenowy@...c.io, megi@....cz, danila@...xyga.com,
javier.carrasco.cruz@...il.com, andy@...nel.org, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: magnetometer: si7210: add driver for Si7210
On Sun, Jan 12, 2025 at 12:45 PM Antoni Pokusinski
<apokusinski01@...il.com> wrote:
>
> Silicon Labs Si7210 is an I2C Hall effect magnetic position and
> temperature sensor. The driver supports the following functionalities:
> * reading the temperature measurements
> * reading the magnetic field measurements in a single-shot mode
> * choosing the magnetic field measurement scale (20 or 200 mT)
...
Many header inclusions are being missed.
+ array_size.h
> +#include <linux/bitfield.h>
+ bits.h (it's even mentioned in the top comment of bitfield.h)
+ cleanup.h
+ err.h
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/math64.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
+ types.h
+ asm/byteorder.h (or is it already available as linux/byteorder.h?),
but it seems that what you actually wanted is linux/unaligned.h (see
below why).
...
> +static const unsigned int a20_otp_regs[A_REGS_COUNT] = {
> + SI7210_OTPREG_A0_20, SI7210_OTPREG_A1_20, SI7210_OTPREG_A2_20,
> + SI7210_OTPREG_A3_20, SI7210_OTPREG_A4_20, SI7210_OTPREG_A5_20
Please, leave trailing comma(s) when it's clearly not a terminator entry.
> +};
> +
> +static const unsigned int a200_otp_regs[A_REGS_COUNT] = {
> + SI7210_OTPREG_A0_200, SI7210_OTPREG_A1_200, SI7210_OTPREG_A2_200,
> + SI7210_OTPREG_A3_200, SI7210_OTPREG_A4_200, SI7210_OTPREG_A5_200
Ditto.
> +};
...
> +struct si7210_data {
> + struct i2c_client *client;
Do we really need a room for that? Isn't it derivable from the below
regmap? Also note the frequency of use of client vs. regmap. The
result in the object file can be much better if regmap becomes the
first member here. Check it (with bloat-o-meter, for example).
> + struct regmap *regmap;
> + struct regulator *vdd;
> + struct mutex fetch_lock; /* lock for a single measurement fetch */
> + s8 temp_offset;
> + s8 temp_gain;
> + s8 scale_20_a[A_REGS_COUNT];
> + s8 scale_200_a[A_REGS_COUNT];
> + u8 curr_scale;
> +};
> +
> +static const struct iio_chan_spec si7210_channels[] = {
> + {
> + .type = IIO_MAGN,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
> + },
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + }
Leave trailing comma.
> +};
> +
> +static int si7210_fetch_measurement(struct si7210_data *data,
> + struct iio_chan_spec const *chan,
> + __be16 *buf)
> +{
> + u8 dspsigsel = chan->type == IIO_MAGN ? 0 : 1;
> + int ret, result;
Why is the result signed? I believe even regmap APIs have it unsigned
in the prototypes.
Ah, it's even worse... See below.
> + guard(mutex)(&data->fetch_lock);
> +
> + ret = regmap_update_bits(data->regmap, SI7210_REG_DSPSIGSEL,
> + SI7210_MASK_DSPSIGSEL, dspsigsel);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_update_bits(data->regmap, SI7210_REG_POWER_CTRL,
> + SI7210_MASK_ONEBURST | SI7210_MASK_STOP,
> + SI7210_MASK_ONEBURST & ~SI7210_MASK_STOP);
> + if (ret < 0)
> + return ret;
> +
> + /* Read the contents of the registers containing the result: DSPSIGM, DSPSIGL */
> + ret = regmap_bulk_read(data->regmap, SI7210_REG_DSPSIGM, &result, 2);
I stumbled over this...
> + if (ret < 0)
> + return ret;
> +
> + *buf = cpu_to_be16(result);
...and this piece and I think you got it wrong. What you should do is
just supply a buf with sizeof one element.
ret = ..., buf, sizeof(buf[0]));
Otherwise this needs a very good comment explaining what the heck is done here.
> + return 0;
> +}
> +
> +static int si7210_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct si7210_data *data = iio_priv(indio_dev);
> + long long temp;
> + __be16 dspsig;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = si7210_fetch_measurement(data, chan, &dspsig);
Oh, but why...
> + if (ret < 0)
...then the ' < 0' part? What is the positive ret meaning?
> + return ret;
> +
> + *val = dspsig & GENMASK(14, 0);
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = 0;
> + if (data->curr_scale == 20)
> + *val2 = 1250;
> + else /* data->curr_scale == 200 */
> + *val2 = 12500;
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_OFFSET:
> + *val = -16384;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = si7210_fetch_measurement(data, chan, &dspsig);
> + if (ret < 0)
> + return ret;
> +
> + temp = FIELD_GET(GENMASK(14, 3), dspsig);
> + temp = div_s64(-383 * temp * temp, 100) + 160940 * temp - 279800000;
HECTO/CENTI, but I think in this case it's not needed as it is most
likely in alignment with the datasheet.
> + temp = (1 + (data->temp_gain / 2048)) * temp + (1000000 / 16) * data->temp_offset;
But here MICRO? MEGA? would make sense to show the scale.
> + ret = regulator_get_voltage(data->vdd);
> + if (ret < 0)
> + return ret;
> + temp -= 222 * div_s64(ret, 1000);
This is conversion from uV to mV IIUC, so replacing it with MILLI
would make it harder to understand I suppose.
> + *val = div_s64(temp, 1000);
MILLI?
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int si7210_read_otpreg_val(struct si7210_data *data, unsigned int otpreg, u8 *val)
> +{
> + int ret;
> + unsigned int otpdata;
> +
> + ret = regmap_write(data->regmap, SI7210_REG_OTP_ADDR, otpreg);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_update_bits(data->regmap, SI7210_REG_OTP_CTRL,
> + SI7210_MASK_OTP_READ_EN, SI7210_MASK_OTP_READ_EN);
> + if (ret < 0)
> + return ret;
> + ret = regmap_read(data->regmap, SI7210_REG_OTP_DATA, &otpdata);
> + if (ret < 0)
What are those ' < 0' parts for in many cases? Does it mean we ignore
positive output? Why is it so?
> + return ret;
> +
> + *val = (u8)otpdata;
Why casting?
> + return 0;
> +}
...
> + /*
> + * According to the datasheet, the primary method to wake up a
> + * device is to send an empty write. However this is not feasible
> + * using current API so we use the other method i.e. read a single
the current
> + * byte. The device should respond with 0xFF.
> + */
> +
Unneeded blank line, and TBH, the comment sounds like it should be
rather for the entire function.
> + int ret;
> +
> + ret = i2c_smbus_read_byte(data->client);
> + if (ret < 0)
> + return ret;
> +
> + if (ret != 0xFF)
> + return -EIO;
> +
> + return 0;
Btw, is this the only reason for having the client member in the
private structure? If so, you can derive it from regmap.
...
> +static int si7210_probe(struct i2c_client *client)
> +{
> + struct si7210_data *data;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
(Almost) useless?
> + mutex_init(&data->fetch_lock);
Why not devm_mutex_init()?
> + data->regmap = devm_regmap_init_i2c(client, &si7210_regmap_conf);
> + if (IS_ERR(data->regmap))
> + return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
> + "failed to register regmap\n");
> +
> + data->vdd = devm_regulator_get(&client->dev, "vdd");
> + if (IS_ERR(data->vdd))
> + return dev_err_probe(&client->dev, PTR_ERR(data->vdd),
> + "failed to get VDD regulator\n");
> +
> + ret = regulator_enable(data->vdd);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = dev_name(&client->dev);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &si7210_info;
> + indio_dev->channels = si7210_channels;
> + indio_dev->num_channels = ARRAY_SIZE(si7210_channels);
> +
> + ret = si7210_device_init(data);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "device initialization failed\n");
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
...
> +static struct i2c_driver si7210_driver = {
> + .driver = {
> + .name = "si7210",
> + .of_match_table = si7210_dt_ids,
> + },
> + .probe = si7210_probe,
> + .id_table = si7210_id,
> +};
> +
Wrong place of this blank line...
> +module_i2c_driver(si7210_driver);
...should be here.
> +MODULE_AUTHOR("Antoni Pokusinski <apokusinski01@...il.com>");
> +MODULE_DESCRIPTION("Silicon Labs Si7210 Hall Effect sensor I2C driver");
> +MODULE_LICENSE("GPL");
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists