| Message ID | 20250112104453.45673-3-apokusinski01@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: magnetometer: add support for Si7210 | expand |
On Sun, Jan 12, 2025 at 12:45 PM Antoni Pokusinski <apokusinski01@gmail.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@gmail.com>"); > +MODULE_DESCRIPTION("Silicon Labs Si7210 Hall Effect sensor I2C driver"); > +MODULE_LICENSE("GPL");
On Sun, 12 Jan 2025 11:44:53 +0100 Antoni Pokusinski <apokusinski01@gmail.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) > > Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com> Hi Antoni, Some issues in the endian handling as your fix for the build bot warnings is backwards I think. A few other comments inline. Jonathan > diff --git a/drivers/iio/magnetometer/si7210.c b/drivers/iio/magnetometer/si7210.c > new file mode 100644 > index 000000000000..107312d127e6 > --- /dev/null > +++ b/drivers/iio/magnetometer/si7210.c > @@ -0,0 +1,428 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Silicon Labs Si7210 Hall Effect sensor driver > + * > + * Copyright (c) 2024 Antoni Pokusinski <apokusinski01@gmail.com> > + * > + * Datasheet: > + * https://www.silabs.com/documents/public/data-sheets/si7210-datasheet.pdf > + */ > + > +#include <linux/bitfield.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> At lease linux/mod_devicetable.h is missing here. we more or less go by include what you use in kernel drivers so you should make minimal assumptions about what includes what. There are a few headers that are subsections of a wider interface where that isn't necessary but for most things should be here. > +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), > + }, > + { Slight preference for more compact }, { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > + } > +}; > + > +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; > + > + 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); use sizeof to replace that 2. > + if (ret < 0) > + return ret; > + > + *buf = cpu_to_be16(result); You've lost me. The regmap bulk will load in 'an order'. Not sure whether thant is big endian or little endian here, but it's definitely not CPU endian. I 'think' you can read directly into buf. Must be a wrong endian conversion somewhere though as on typical le system you are currently reversing the bytes here and at the callsite. However, I think what you actually want is static int si7210_fetch_measurement(struct si7210_data *data, struct iio_chan_spec const *chan, u16 *buf) __be16 result; ... ret = regmap_bulk_read(..., &result, sizeof(result)); ... *buf = be16_to_cpu(result); > + > + 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); > + if (ret < 0) > + return ret; > + > + *val = dspsig & GENMASK(14, 0); It is big endian. You can't do this and expect to hit the right bits. More to the point you can't set val to the big endian anyway so markings are wrong somewhere. > + 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; > + Big endian, You can't do any of this safely. Convert it to cpu endian (e.g. u16) first. > + temp = FIELD_GET(GENMASK(14, 3), dspsig); > + temp = div_s64(-383 * temp * temp, 100) + 160940 * temp - 279800000; > + temp = (1 + (data->temp_gain / 2048)) * temp + (1000000 / 16) * data->temp_offset; > + > + ret = regulator_get_voltage(data->vdd); > + if (ret < 0) > + return ret; > + > + temp -= 222 * div_s64(ret, 1000); > + > + *val = div_s64(temp, 1000); > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} ... > +static int si7210_device_wake(struct si7210_data *data) > +{ > + /* > + * 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 > + * byte. The device should respond with 0xFF. > + */ > + > + int ret; > + > + ret = i2c_smbus_read_byte(data->client); > + if (ret < 0) > + return ret; > + > + if (ret != 0xFF) > + return -EIO; > + > + return 0; > +} > + > +static int si7210_device_init(struct si7210_data *data) > +{ > + int ret; > + unsigned int i; > + > + ret = si7210_device_wake(data); > + if (ret < 0) > + return ret; > + > + fsleep(1000); > + > + ret = si7210_read_otpreg_val(data, SI7210_OTPREG_TMP_GAIN, &data->temp_gain); > + if (ret < 0) > + return ret; Blank line here and similar places where you have a call / check result pair before doing something more or less unrelated. > + ret = si7210_read_otpreg_val(data, SI7210_OTPREG_TMP_OFF, &data->temp_offset); > + if (ret < 0) > + return ret; > + > + for (i = 0; i < A_REGS_COUNT; i++) { > + ret = si7210_read_otpreg_val(data, a20_otp_regs[i], &data->scale_20_a[i]); > + if (ret < 0) > + return ret; > + } > + for (i = 0; i < A_REGS_COUNT; i++) { > + ret = si7210_read_otpreg_val(data, a200_otp_regs[i], &data->scale_200_a[i]); > + if (ret < 0) > + return ret; > + } > + > + ret = regmap_update_bits(data->regmap, SI7210_REG_ARAUTOINC, > + SI7210_MASK_ARAUTOINC, SI7210_MASK_ARAUTOINC); > + if (ret < 0) > + return ret; > + > + return si7210_set_scale(data, 20); > +} >
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@gmail.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). > 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@gmail.com>"); > > +MODULE_DESCRIPTION("Silicon Labs Si7210 Hall Effect sensor I2C driver"); > > +MODULE_LICENSE("GPL"); > > -- > With Best Regards, > Andy Shevchenko
On Tue, Jan 14, 2025 at 12:19 AM Antoni Pokusinski <apokusinski01@gmail.com> wrote: > 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`. ... > > > +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. I don't think you have tested all that I was talking about, i.e. have you tried to swap the positions of client and regmap? What I expect is that when you swap them you will see a good size reduction due to pointer arithmetics becoming no-op for the regmap pointer. And then the dropping of the client might waste all that beneficial size. > > > + 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; > > > +};
On Tue, Jan 14, 2025 at 11:43:11AM +0200, Andy Shevchenko wrote: > On Tue, Jan 14, 2025 at 12:19 AM Antoni Pokusinski > <apokusinski01@gmail.com> wrote: > > > 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`. > > ... > > > > > +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. > > I don't think you have tested all that I was talking about, i.e. have > you tried to swap the positions of client and regmap? What I expect is > that when you swap them you will see a good size reduction due to > pointer arithmetics becoming no-op for the regmap pointer. And then > the dropping of the client might waste all that beneficial size. > Ok, so I've tried to swap the `i2c_client` and `regmap` pointers and... there was no change shown by the bloat-o-meter. The only improvement was that the new object file (that is after moving the `regmap` to the beginning of the struct) was 8 bytes smaller in file size. Out of curiosity I've also tried moving the `regmap` further away in the structure (e.g. I placed it after the regulator and mutex) but there was still no change. I am a bit confused, since this behavior is different from what you described that it should be. > > > > + 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; > > > > +}; > > -- > With Best Regards, > Andy Shevchenko Kinds regards, Antoni
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig index 7177cd1d67cb..3debf1320ad1 100644 --- a/drivers/iio/magnetometer/Kconfig +++ b/drivers/iio/magnetometer/Kconfig @@ -235,6 +235,17 @@ config SENSORS_RM3100_SPI To compile this driver as a module, choose M here: the module will be called rm3100-spi. +config SI7210 + tristate "SI7210 Hall effect sensor" + depends on I2C + select REGMAP_I2C + help + Say Y here to add support for the SI7210 Hall effect sensor. + + This driver can also be compiled as a module. + To compile this driver as a module, choose M here: the module + will be called si7210. + config TI_TMAG5273 tristate "TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor" depends on I2C diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile index 3e4c2ecd9adf..7455115a052f 100644 --- a/drivers/iio/magnetometer/Makefile +++ b/drivers/iio/magnetometer/Makefile @@ -31,6 +31,8 @@ obj-$(CONFIG_SENSORS_RM3100) += rm3100-core.o obj-$(CONFIG_SENSORS_RM3100_I2C) += rm3100-i2c.o obj-$(CONFIG_SENSORS_RM3100_SPI) += rm3100-spi.o +obj-$(CONFIG_SI7210) += si7210.o + obj-$(CONFIG_TI_TMAG5273) += tmag5273.o obj-$(CONFIG_YAMAHA_YAS530) += yamaha-yas530.o diff --git a/drivers/iio/magnetometer/si7210.c b/drivers/iio/magnetometer/si7210.c new file mode 100644 index 000000000000..107312d127e6 --- /dev/null +++ b/drivers/iio/magnetometer/si7210.c @@ -0,0 +1,428 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Silicon Labs Si7210 Hall Effect sensor driver + * + * Copyright (c) 2024 Antoni Pokusinski <apokusinski01@gmail.com> + * + * Datasheet: + * https://www.silabs.com/documents/public/data-sheets/si7210-datasheet.pdf + */ + +#include <linux/bitfield.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> + +/* Registers offsets and masks */ +#define SI7210_REG_DSPSIGM 0xC1 +#define SI7210_REG_DSPSIGL 0xC2 + +#define SI7210_MASK_DSPSIGSEL GENMASK(2, 0) +#define SI7210_REG_DSPSIGSEL 0xC3 + +#define SI7210_MASK_STOP BIT(1) +#define SI7210_MASK_ONEBURST BIT(2) +#define SI7210_REG_POWER_CTRL 0xC4 + +#define SI7210_MASK_ARAUTOINC BIT(0) +#define SI7210_REG_ARAUTOINC 0xC5 + +#define SI7210_REG_A0 0xCA +#define SI7210_REG_A1 0xCB +#define SI7210_REG_A2 0xCC +#define SI7210_REG_A3 0xCE +#define SI7210_REG_A4 0xCF +#define SI7210_REG_A5 0xD0 + +#define SI7210_REG_OTP_ADDR 0xE1 +#define SI7210_REG_OTP_DATA 0xE2 + +#define SI7210_MASK_OTP_READ_EN BIT(1) +#define SI7210_REG_OTP_CTRL 0xE3 + +/* OTP data registers offsets */ +#define SI7210_OTPREG_TMP_OFF 0x1D +#define SI7210_OTPREG_TMP_GAIN 0x1E + +#define SI7210_OTPREG_A0_20 0x21 +#define SI7210_OTPREG_A1_20 0x22 +#define SI7210_OTPREG_A2_20 0x23 +#define SI7210_OTPREG_A3_20 0x24 +#define SI7210_OTPREG_A4_20 0x25 +#define SI7210_OTPREG_A5_20 0x26 + +#define SI7210_OTPREG_A0_200 0x27 +#define SI7210_OTPREG_A1_200 0x28 +#define SI7210_OTPREG_A2_200 0x29 +#define SI7210_OTPREG_A3_200 0x2A +#define SI7210_OTPREG_A4_200 0x2B +#define SI7210_OTPREG_A5_200 0x2C + +#define A_REGS_COUNT 6 + +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 +}; + +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 +}; + +static const struct regmap_range si7210_read_reg_ranges[] = { + regmap_reg_range(SI7210_REG_DSPSIGM, SI7210_REG_ARAUTOINC), + regmap_reg_range(SI7210_REG_A0, SI7210_REG_A2), + regmap_reg_range(SI7210_REG_A3, SI7210_REG_A5), + regmap_reg_range(SI7210_REG_OTP_ADDR, SI7210_REG_OTP_CTRL), +}; + +static const struct regmap_access_table si7210_readable_regs = { + .yes_ranges = si7210_read_reg_ranges, + .n_yes_ranges = ARRAY_SIZE(si7210_read_reg_ranges), +}; + +static const struct regmap_range si7210_write_reg_ranges[] = { + regmap_reg_range(SI7210_REG_DSPSIGSEL, SI7210_REG_ARAUTOINC), + regmap_reg_range(SI7210_REG_A0, SI7210_REG_A2), + regmap_reg_range(SI7210_REG_A3, SI7210_REG_A5), + regmap_reg_range(SI7210_REG_OTP_ADDR, SI7210_REG_OTP_CTRL), +}; + +static const struct regmap_access_table si7210_writeable_regs = { + .yes_ranges = si7210_write_reg_ranges, + .n_yes_ranges = ARRAY_SIZE(si7210_write_reg_ranges), +}; + +static const struct regmap_range si7210_volatile_reg_ranges[] = { + regmap_reg_range(SI7210_REG_DSPSIGM, SI7210_REG_DSPSIGL), + regmap_reg_range(SI7210_REG_POWER_CTRL, SI7210_REG_POWER_CTRL), +}; + +static const struct regmap_access_table si7210_volatile_regs = { + .yes_ranges = si7210_volatile_reg_ranges, + .n_yes_ranges = ARRAY_SIZE(si7210_volatile_reg_ranges), +}; + +static const struct regmap_config si7210_regmap_conf = { + .reg_bits = 8, + .val_bits = 8, + .max_register = SI7210_REG_OTP_CTRL, + + .rd_table = &si7210_readable_regs, + .wr_table = &si7210_writeable_regs, + .volatile_table = &si7210_volatile_regs, +}; + +struct si7210_data { + struct i2c_client *client; + 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), + } +}; + +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; + + 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); + if (ret < 0) + return ret; + + *buf = cpu_to_be16(result); + + 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); + if (ret < 0) + 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; + temp = (1 + (data->temp_gain / 2048)) * temp + (1000000 / 16) * data->temp_offset; + + ret = regulator_get_voltage(data->vdd); + if (ret < 0) + return ret; + + temp -= 222 * div_s64(ret, 1000); + + *val = div_s64(temp, 1000); + + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +static int si7210_set_scale(struct si7210_data *data, unsigned int scale) +{ + s8 *a_otp_values; + int ret; + + if (scale == 20) + a_otp_values = data->scale_20_a; + else if (scale == 200) + a_otp_values = data->scale_200_a; + else + return -EINVAL; + + guard(mutex)(&data->fetch_lock); + + /* Write the registers 0xCA - 0xCC */ + ret = regmap_bulk_write(data->regmap, SI7210_REG_A0, a_otp_values, 3); + if (ret < 0) + return ret; + + /* Write the registers 0xCE - 0xD0 */ + ret = regmap_bulk_write(data->regmap, SI7210_REG_A3, &a_otp_values[3], 3); + if (ret < 0) + return ret; + + data->curr_scale = scale; + + return 0; +} + +static int si7210_write_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); + unsigned int scale; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + if (val == 0 && val2 == 1250) + scale = 20; + else if (val == 0 && val2 == 12500) + scale = 200; + else + return -EINVAL; + + return si7210_set_scale(data, scale); + 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) + return ret; + + *val = (u8)otpdata; + + return 0; +} + +static int si7210_device_wake(struct si7210_data *data) +{ + /* + * 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 + * byte. The device should respond with 0xFF. + */ + + int ret; + + ret = i2c_smbus_read_byte(data->client); + if (ret < 0) + return ret; + + if (ret != 0xFF) + return -EIO; + + return 0; +} + +static int si7210_device_init(struct si7210_data *data) +{ + int ret; + unsigned int i; + + ret = si7210_device_wake(data); + if (ret < 0) + return ret; + + fsleep(1000); + + ret = si7210_read_otpreg_val(data, SI7210_OTPREG_TMP_GAIN, &data->temp_gain); + if (ret < 0) + return ret; + ret = si7210_read_otpreg_val(data, SI7210_OTPREG_TMP_OFF, &data->temp_offset); + if (ret < 0) + return ret; + + for (i = 0; i < A_REGS_COUNT; i++) { + ret = si7210_read_otpreg_val(data, a20_otp_regs[i], &data->scale_20_a[i]); + if (ret < 0) + return ret; + } + for (i = 0; i < A_REGS_COUNT; i++) { + ret = si7210_read_otpreg_val(data, a200_otp_regs[i], &data->scale_200_a[i]); + if (ret < 0) + return ret; + } + + ret = regmap_update_bits(data->regmap, SI7210_REG_ARAUTOINC, + SI7210_MASK_ARAUTOINC, SI7210_MASK_ARAUTOINC); + if (ret < 0) + return ret; + + return si7210_set_scale(data, 20); +} + +static const struct iio_info si7210_info = { + .read_raw = si7210_read_raw, + .write_raw = si7210_write_raw, +}; + +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; + + mutex_init(&data->fetch_lock); + + 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 const struct i2c_device_id si7210_id[] = { + { "si7210" }, + { } +}; +MODULE_DEVICE_TABLE(i2c, si7210_id); + +static const struct of_device_id si7210_dt_ids[] = { + { .compatible = "silabs,si7210" }, + { } +}; +MODULE_DEVICE_TABLE(of, si7210_dt_ids); + +static struct i2c_driver si7210_driver = { + .driver = { + .name = "si7210", + .of_match_table = si7210_dt_ids, + }, + .probe = si7210_probe, + .id_table = si7210_id, +}; + +module_i2c_driver(si7210_driver); +MODULE_AUTHOR("Antoni Pokusinski <apokusinski01@gmail.com>"); +MODULE_DESCRIPTION("Silicon Labs Si7210 Hall Effect sensor I2C driver"); +MODULE_LICENSE("GPL");
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) Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com> --- drivers/iio/magnetometer/Kconfig | 11 + drivers/iio/magnetometer/Makefile | 2 + drivers/iio/magnetometer/si7210.c | 428 ++++++++++++++++++++++++++++++ 3 files changed, 441 insertions(+) create mode 100644 drivers/iio/magnetometer/si7210.c