Hi Andy,
Thanks for the review. I'm currently implementing some changes in the
driver according to the review, however I have some doubts regarding
removal of the `i2c_client` from `si7210_data`.
Kind regards,
Antoni
On Sun, Jan 12, 2025 at 04:18:46PM +0200, Andy Shevchenko wrote:
> On Sun, Jan 12, 2025 at 12:45 PM Antoni Pokusinski
> <apokusinski01@xxxxxxxxx> 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).
>
I used arm-linux-nm and the bloat-o-meter to compare the sizes and it
turned out that the version which contains the `i2c_client` has
slightly smaller size actually. Here are the results:
$ ./scripts/bloat-o-meter -p arm-linux- ./old_si7210.ko ./new_si7210.ko
add/remove: 0/0 grow/shrink: 1/0 up/down: 4/0 (4)
Function old new delta
si7210_probe 556 560 +4
Total: Before=4021, After=4025, chg +0.10%
Here is the diff (shortened for better readability) between
the old_si7210.ko (uses `si7210_data->i2c_client`) and
new_si7210.ko (does not use `si7210_data->i2c_client`):
struct si7210_data {
- struct i2c_client *client;
struct regmap *regmap;
...
static int si7210_device_wake(struct si7210_data *data)
{
+ struct device *dev = regmap_get_device(data->regmap);
int ret;
- ret = i2c_smbus_read_byte(data->client);
+ ret = i2c_smbus_read_byte(to_i2c_client(dev));
...
static int si7210_probe(struct i2c_client *client)
data = iio_priv(indio_dev);
- data->client = client;
Hence, I guess that it's actually better to leave the `i2c_client` as it is.
> > + 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?
>
The regmap functions return value <=0 so I decided to handle errors
in the 'if (ret < 0)' way.
But in the next version I'll change that to a simpler 'if (ret)'
wherever possible
> > + 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@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("Silicon Labs Si7210 Hall Effect sensor I2C driver");
> > +MODULE_LICENSE("GPL");
>
> --
> With Best Regards,
> Andy Shevchenko
