[v2,2/2] iio: magnetometer: add ti tmag5273 driver
Commit Message
Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
Additionally to temperature and magnetic X, Y and Z-axes the angle and
magnitude are reported.
The sensor is operating in continuous measurement mode and changes to sleep
mode if not used for 5 seconds.
Datasheet: https://www.ti.com/lit/gpn/tmag5273
Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
Changes in v2:
- Implemented suggestions from review and cleaned up probe function. This
results in changes all over the tmag5273.c code.
MAINTAINERS | 1 +
drivers/iio/magnetometer/Kconfig | 12 +
drivers/iio/magnetometer/Makefile | 2 +
drivers/iio/magnetometer/tmag5273.c | 725 ++++++++++++++++++++++++++++
4 files changed, 740 insertions(+)
create mode 100644 drivers/iio/magnetometer/tmag5273.c
Comments
On Mon, Nov 21, 2022 at 01:35:42PM +0100, Gerald Loacker wrote:
> Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
> Additionally to temperature and magnetic X, Y and Z-axes the angle and
> magnitude are reported.
> The sensor is operating in continuous measurement mode and changes to sleep
> mode if not used for 5 seconds.
Thank you for an update! My comments below.
(I believe the next version will be ready for inclusion)
...
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/pm_runtime.h>
+ Blank line (to make below as an explicit group of headers)
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
...
> +/*
> + * bits in the TMAG5273_MANUFACTURER_ID_LSB / MSB register
> + * 16-bit read-only unique manufacturer ID
Is it ASCII or not? ('TI' suggests something...)
If it is, please put it in the comments explicitly in ASCII.
> + */
> +#define TMAG5273_MANUFACTURER_ID 0x5449
...
> +#define TMAG5273_MAG_CH_EN_X_Y_Z 0x07
This is hexadecimal, while below you are using decimal values.
Also if this is like above, isn't it a bit mask? (Otherwise
using decimal value hints that it's probably not)
...
> +#define TMAG5273_ANGLE_EN_OFF 0
> +#define TMAG5273_ANGLE_EN_X_Y 1
> +#define TMAG5273_ANGLE_EN_Y_Z 2
> +#define TMAG5273_ANGLE_EN_X_Z 3
...
> + /*
> + * Locks the sensor for exclusive use during a measurement (which
> + * involves several register transactions so the regmap lock is not
> + * enough) so that measurements get serialized in a first-come-first-
> + * serve manner.
It will be better to have 'first-come-first-serve' non-broken between lines.
> + */
> +/*
> + * Magnetic resolution in Gauss for different TMAG5273 versions.
> + * Scale[Gauss] = Range[mT] * 1000 / 2^15 * 10, (1 mT = 10 Gauss)
> + * Only version 1 and 2 are valid, version 0 and 3 are reserved.
> + */
...
> +static const struct {
> + unsigned int scale_int;
> + unsigned int scale_micro;
Can we have a separate patch to define this one eventually in the (one of) IIO
generic headers? It's a bit pity that every new driver seems to reinvent the
wheel.
> +} tmag5273_scale_table[4][2] = {
> + { { 0, 0 }, { 0, 0 } },
> + { { 0, 12200 }, { 0, 24400 } },
> + { { 0, 40600 }, { 0, 81200 } },
> + { { 0, 0 }, { 0, 0 } },
> +};
You probably can reformat there to have fixed-width columns to see better the
pairs of the values. And as I told you before, 4 is not needed (AFAIR, or 2 in
the square brackets).
...
> + ret = regmap_bulk_read(data->map, TMAG5273_ANGLE_RESULT_MSB, reg_data,
> + sizeof(reg_data[0]));
This is strange. The sizeof of a single element is 2, while you define a
pointer to the entire array.
So, the
ret = regmap_bulk_read(data->map, TMAG5273_ANGLE_RESULT_MSB, ®_data[0],
sizeof(reg_data[0]));
will show better the intention.
...
> + *length = ARRAY_SIZE(
> + tmag5273_scale_table[data->version]) * 2;
Ugly indentation. Better
*length =
ARRAY_SIZE(tmag5273_scale_table[data->version]) * 2;
or
*length = 2 *
ARRAY_SIZE(tmag5273_scale_table[data->version]);
...
> + ret = tmag5273_get_measure(data, &t, &x, &y, &z, &angle,
> + &magnitude);
I would use one line for this, but it's up to you (I think that Jonathan
wouldn't mind either choice).
...
> + default:
> + /* Unknown request */
Useless comment ?
> + return -EINVAL;
> + }
...
> + for (i = 0; i < ARRAY_SIZE(tmag5273_scale_table[0]);
> + i++) {
I would definitely go with one line here.
> + if (tmag5273_scale_table[data->version][i]
> + .scale_micro == val2)
Ugly indentation.
> + break;
> + }
...
> + ret = regmap_update_bits(
Try to reformat all these left open parentheses.
> + data->map, TMAG5273_SENSOR_CONFIG_2,
> + TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK,
> + data->scale_index == MAGN_LOW ? 0 :
> + (TMAG5273_Z_RANGE_MASK |
> + TMAG5273_X_Y_RANGE_MASK));
Do you need parentheses here?
...
> +#define TMAG5273_AXIS_CHANNEL(axis, index) \
> + { \
> + .type = IIO_MAGN, \
> + .modified = 1, \
> + .channel2 = IIO_MOD_##axis, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_all = \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .info_mask_shared_by_all_available = \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .address = index, \
> + .scan_index = index, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_CPU, \
> + }, \
> + }
Why not using TABs for the formatting trailing \:s?
...
> + if (!device_property_read_string(data->dev, "ti,angle-measurement",
> + &angle_measurement)) {
> + if (!strcmp(angle_measurement, "off"))
> + data->angle_measurement = TMAG5273_ANGLE_EN_OFF;
> + else if (!strcmp(angle_measurement, "x-y"))
> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
> + else if (!strcmp(angle_measurement, "y-z"))
> + data->angle_measurement = TMAG5273_ANGLE_EN_Y_Z;
> + else if (!strcmp(angle_measurement, "x-z"))
> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Z;
> + else
> + dev_warn(data->dev,
> + "failed to read angle-measurement\n");
Can't you use match_string()?
And you actually can do a bit differently, can you?
angle_measurement = "...default...";
if (device_property_...)
dev_warn(data->dev, "failed to read angle-measurement\n");
ret = match_string();
if (ret < 0)
dev_warn(data->dev, "invalid angle-measurement value\n");
else
data->angle_measurement = ret;
> + }
...
> + switch (data->devid) {
> + case TMAG5273_MANUFACTURER_ID:
> + snprintf(data->name, sizeof(data->name), "tmag5273x%1u",
There is a difference between %1u and %.1u. And I believe you wanted the
latter, but...
> + data->version);
> + if (data->version < 1 || data->version > 2)
> + dev_warn(data->dev, "Unsupported device version 0x%x\n",
> + data->version);
...with the current approach you may replace above with
dev_warn(data->dev, "Unsupported device %s\n", data->name);
> + break;
> + default:
> + dev_warn(data->dev, "Unknown device ID 0x%x\n", data->devid);
> + break;
> + }
...
> + struct iio_dev *indio_dev;
> + struct device *dev = &i2c->dev;
> + struct tmag5273_data *data;
> + int ret;
In reversed xmas tree order it would look a bit better:
struct device *dev = &i2c->dev;
struct tmag5273_data *data;
struct iio_dev *indio_dev;
int ret;
Am 21.11.2022 um 15:04 schrieb Andy Shevchenko:
> On Mon, Nov 21, 2022 at 01:35:42PM +0100, Gerald Loacker wrote:
>> Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
>> Additionally to temperature and magnetic X, Y and Z-axes the angle and
>> magnitude are reported.
>> The sensor is operating in continuous measurement mode and changes to sleep
>> mode if not used for 5 seconds.
>
> Thank you for an update! My comments below.
> (I believe the next version will be ready for inclusion)
>
> ...
>
>> +/*
>> + * bits in the TMAG5273_MANUFACTURER_ID_LSB / MSB register
>> + * 16-bit read-only unique manufacturer ID
>
> Is it ASCII or not? ('TI' suggests something...)
> If it is, please put it in the comments explicitly in ASCII.
>
Ok, got it.
>> + */
>> +#define TMAG5273_MANUFACTURER_ID 0x5449
>
> ...
>
>> +#define TMAG5273_MAG_CH_EN_X_Y_Z 0x07
>
> This is hexadecimal, while below you are using decimal values.
> Also if this is like above, isn't it a bit mask? (Otherwise
> using decimal value hints that it's probably not)
>
Ok, I will use decimal value. It's not a bit mask, as there are special
modes (XYX, ...) defined as well. I didn't want to list all 12 configs,
but this could clarify this.
>
> ...
>
>> +static const struct {
>> + unsigned int scale_int;
>> + unsigned int scale_micro;
>
> Can we have a separate patch to define this one eventually in the (one of) IIO
> generic headers? It's a bit pity that every new driver seems to reinvent the
> wheel.
>
I'll try to find a solution.
>> +} tmag5273_scale_table[4][2] = {
>> + { { 0, 0 }, { 0, 0 } },
>> + { { 0, 12200 }, { 0, 24400 } },
>> + { { 0, 40600 }, { 0, 81200 } },
>> + { { 0, 0 }, { 0, 0 } },
>> +};
>
> You probably can reformat there to have fixed-width columns to see better the
> pairs of the values. And as I told you before, 4 is not needed (AFAIR, or 2 in
> the square brackets).
>
Ok, got it.
>> + for (i = 0; i < ARRAY_SIZE(tmag5273_scale_table[0]);
>> + i++) {
>
> I would definitely go with one line here.
>
>> + if (tmag5273_scale_table[data->version][i]
>> + .scale_micro == val2)
>
> Ugly indentation.
>
You're right, best to rename tmag5273_scale_table to tmag5273_scale.
> ...
>
>> + if (!device_property_read_string(data->dev, "ti,angle-measurement",
>> + &angle_measurement)) {
>> + if (!strcmp(angle_measurement, "off"))
>> + data->angle_measurement = TMAG5273_ANGLE_EN_OFF;
>> + else if (!strcmp(angle_measurement, "x-y"))
>> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
>> + else if (!strcmp(angle_measurement, "y-z"))
>> + data->angle_measurement = TMAG5273_ANGLE_EN_Y_Z;
>> + else if (!strcmp(angle_measurement, "x-z"))
>> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Z;
>> + else
>> + dev_warn(data->dev,
>> + "failed to read angle-measurement\n");
>
> Can't you use match_string()?
>
Yes, I will use match_string().
> And you actually can do a bit differently, can you?
>
> angle_measurement = "...default...";
> if (device_property_...)
> dev_warn(data->dev, "failed to read angle-measurement\n");
I think we shouldn't warn here, as angle_measurement isn't a required
property. Besides that I will do it this way.
> ret = match_string();
> if (ret < 0)
> dev_warn(data->dev, "invalid angle-measurement value\n");
> else
> data->angle_measurement = ret;
>
>> + }
>
> ...
>
>> + switch (data->devid) {
>> + case TMAG5273_MANUFACTURER_ID:
>> + snprintf(data->name, sizeof(data->name), "tmag5273x%1u",
>
> There is a difference between %1u and %.1u. And I believe you wanted the
> latter, but...
>
%1u works fine for me. Can you point me to the documentation for %.1u?
>> + data->version);
>> + if (data->version < 1 || data->version > 2)
>> + dev_warn(data->dev, "Unsupported device version 0x%x\n",
>> + data->version);
>
> ...with the current approach you may replace above with
>
> dev_warn(data->dev, "Unsupported device %s\n", data->name);
>
Ok, fine.
>> + break;
>> + default:
>> + dev_warn(data->dev, "Unknown device ID 0x%x\n", data->devid);
>> + break;
>> + }
>
> ...
>
Others are clear.
Thanks,
Gerald
On Mon, Nov 21, 2022 at 06:24:18PM +0100, Gerald Loacker wrote:
> Am 21.11.2022 um 15:04 schrieb Andy Shevchenko:
> > On Mon, Nov 21, 2022 at 01:35:42PM +0100, Gerald Loacker wrote:
...
> >> + if (!device_property_read_string(data->dev, "ti,angle-measurement",
> >> + &angle_measurement)) {
> >> + if (!strcmp(angle_measurement, "off"))
> >> + data->angle_measurement = TMAG5273_ANGLE_EN_OFF;
> >> + else if (!strcmp(angle_measurement, "x-y"))
> >> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
> >> + else if (!strcmp(angle_measurement, "y-z"))
> >> + data->angle_measurement = TMAG5273_ANGLE_EN_Y_Z;
> >> + else if (!strcmp(angle_measurement, "x-z"))
> >> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Z;
> >> + else
> >> + dev_warn(data->dev,
> >> + "failed to read angle-measurement\n");
> >
> > Can't you use match_string()?
> >
>
> Yes, I will use match_string().
>
> > And you actually can do a bit differently, can you?
> >
> > angle_measurement = "...default...";
> > if (device_property_...)
> > dev_warn(data->dev, "failed to read angle-measurement\n");
>
> I think we shouldn't warn here, as angle_measurement isn't a required
> property. Besides that I will do it this way.
>
> > ret = match_string();
> > if (ret < 0)
> > dev_warn(data->dev, "invalid angle-measurement value\n");
> > else
> > data->angle_measurement = ret;
> >
> >> + }
After looking into DT patch I think you can even use
device_property_match_string(),
...
> >> + snprintf(data->name, sizeof(data->name), "tmag5273x%1u",
> >
> > There is a difference between %1u and %.1u. And I believe you wanted the
> > latter, but...
>
> %1u works fine for me. Can you point me to the documentation for %.1u?
man printf(3)
It was a surprise to me that many developers don't know the difference here.
The %NNNNu defines the _minimum_ digits to print, while the %.NNNu defines
the _exact_ amount of digits to print (NNN -- a number).
> >> + data->version);
On Mon, Nov 21, 2022 at 08:40:53PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 21, 2022 at 06:24:18PM +0100, Gerald Loacker wrote:
> > Am 21.11.2022 um 15:04 schrieb Andy Shevchenko:
> > > On Mon, Nov 21, 2022 at 01:35:42PM +0100, Gerald Loacker wrote:
...
> > >> + snprintf(data->name, sizeof(data->name), "tmag5273x%1u",
> > >
> > > There is a difference between %1u and %.1u. And I believe you wanted the
> > > latter, but...
> >
> > %1u works fine for me. Can you point me to the documentation for %.1u?
>
> man printf(3)
>
> It was a surprise to me that many developers don't know the difference here.
> The %NNNNu defines the _minimum_ digits to print, while the %.NNNu defines
> the _exact_ amount of digits to print (NNN -- a number).
>
> > >> + data->version);
Even me trapped to this one more time :-)
The above is only applies to the strings and floats. So in your case it has no
difference.
Am 21.11.2022 um 19:40 schrieb Andy Shevchenko:
> On Mon, Nov 21, 2022 at 06:24:18PM +0100, Gerald Loacker wrote:
>> Am 21.11.2022 um 15:04 schrieb Andy Shevchenko:
>>> On Mon, Nov 21, 2022 at 01:35:42PM +0100, Gerald Loacker wrote:
>
> ...
>
>>>> + if (!device_property_read_string(data->dev, "ti,angle-measurement",
>>>> + &angle_measurement)) {
>>>> + if (!strcmp(angle_measurement, "off"))
>>>> + data->angle_measurement = TMAG5273_ANGLE_EN_OFF;
>>>> + else if (!strcmp(angle_measurement, "x-y"))
>>>> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
>>>> + else if (!strcmp(angle_measurement, "y-z"))
>>>> + data->angle_measurement = TMAG5273_ANGLE_EN_Y_Z;
>>>> + else if (!strcmp(angle_measurement, "x-z"))
>>>> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Z;
>>>> + else
>>>> + dev_warn(data->dev,
>>>> + "failed to read angle-measurement\n");
>>>
>>> Can't you use match_string()?
>>>
>>
>> Yes, I will use match_string().
>>
>>> And you actually can do a bit differently, can you?
>>>
>>> angle_measurement = "...default...";
>>> if (device_property_...)
>>> dev_warn(data->dev, "failed to read angle-measurement\n");
>>
>> I think we shouldn't warn here, as angle_measurement isn't a required
>> property. Besides that I will do it this way.
>>
>>> ret = match_string();
>>> if (ret < 0)
>>> dev_warn(data->dev, "invalid angle-measurement value\n");
>>> else
>>> data->angle_measurement = ret;
>>>
>>>> + }
>
> After looking into DT patch I think you can even use
> device_property_match_string(),
>
> ...
>
device_property_match_string() is searching only for one string, so here
match_string() you suggested before fits better.
Regards,
Gerald
Am 21.11.2022 um 15:04 schrieb Andy Shevchenko:
> On Mon, Nov 21, 2022 at 01:35:42PM +0100, Gerald Loacker wrote:
>> Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
>> Additionally to temperature and magnetic X, Y and Z-axes the angle and
>> magnitude are reported.
>> The sensor is operating in continuous measurement mode and changes to sleep
>> mode if not used for 5 seconds.
>
> Thank you for an update! My comments below.
> (I believe the next version will be ready for inclusion)
>
> ...
>
>> +static const struct {
>> + unsigned int scale_int;
>> + unsigned int scale_micro;
>
> Can we have a separate patch to define this one eventually in the (one of) IIO
> generic headers? It's a bit pity that every new driver seems to reinvent the
> wheel.
>
>> +} tmag5273_scale_table[4][2] = {
>> + { { 0, 0 }, { 0, 0 } },
>> + { { 0, 12200 }, { 0, 24400 } },
>> + { { 0, 40600 }, { 0, 81200 } },
>> + { { 0, 0 }, { 0, 0 } },
>> +};
>
I'm thinking of defining structs for all similar types of IIO output
formats in iio.h like this:
struct iio_val_int_plus_micro {
int val_int;
int val_micro;
};
struct iio_val_int_plus_nano {
int val_int;
int val_nano;
};
struct iio_val_int_plus_micro_db {
int val_int;
int val_micro_db;
};
struct iio_val_fractional {
int dividend;
int divisor;
};
struct iio_val_fractional_log2 {
int dividend;
int divisor;
};
Do you agree?
Regards,
Gerald
On Wed, Nov 23, 2022 at 10:58:47AM +0100, Gerald Loacker wrote:
> Am 21.11.2022 um 15:04 schrieb Andy Shevchenko:
> > On Mon, Nov 21, 2022 at 01:35:42PM +0100, Gerald Loacker wrote:
...
> >> +static const struct {
> >> + unsigned int scale_int;
> >> + unsigned int scale_micro;
> >
> > Can we have a separate patch to define this one eventually in the (one of) IIO
> > generic headers? It's a bit pity that every new driver seems to reinvent the
> > wheel.
> >
> >> +} tmag5273_scale_table[4][2] = {
> >> + { { 0, 0 }, { 0, 0 } },
> >> + { { 0, 12200 }, { 0, 24400 } },
> >> + { { 0, 40600 }, { 0, 81200 } },
> >> + { { 0, 0 }, { 0, 0 } },
> >> +};
> >
>
> I'm thinking of defining structs for all similar types of IIO output
> formats in iio.h like this:
>
>
> struct iio_val_int_plus_micro {
> int val_int;
> int val_micro;
> };
>
> struct iio_val_int_plus_nano {
> int val_int;
> int val_nano;
> };
>
> struct iio_val_int_plus_micro_db {
> int val_int;
> int val_micro_db;
> };
...
> struct iio_val_fractional {
> int dividend;
> int divisor;
> };
This one...
> struct iio_val_fractional_log2 {
> int dividend;
> int divisor;
> };
...and this one repeat struct s32_fract (or u32_fract, whatever suits better).
> Do you agree?
Me, yes, but you need a blessing by maintainers of IIO.
On Wed, 23 Nov 2022 15:39:57 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Nov 23, 2022 at 10:58:47AM +0100, Gerald Loacker wrote:
> > Am 21.11.2022 um 15:04 schrieb Andy Shevchenko:
> > > On Mon, Nov 21, 2022 at 01:35:42PM +0100, Gerald Loacker wrote:
>
> ...
>
> > >> +static const struct {
> > >> + unsigned int scale_int;
> > >> + unsigned int scale_micro;
> > >
> > > Can we have a separate patch to define this one eventually in the (one of) IIO
> > > generic headers? It's a bit pity that every new driver seems to reinvent the
> > > wheel.
> > >
> > >> +} tmag5273_scale_table[4][2] = {
> > >> + { { 0, 0 }, { 0, 0 } },
> > >> + { { 0, 12200 }, { 0, 24400 } },
> > >> + { { 0, 40600 }, { 0, 81200 } },
> > >> + { { 0, 0 }, { 0, 0 } },
> > >> +};
> > >
> >
> > I'm thinking of defining structs for all similar types of IIO output
> > formats in iio.h like this:
> >
> >
> > struct iio_val_int_plus_micro {
> > int val_int;
> > int val_micro;
> > };
> >
> > struct iio_val_int_plus_nano {
> > int val_int;
> > int val_nano;
> > };
> >
> > struct iio_val_int_plus_micro_db {
> > int val_int;
> > int val_micro_db;
> > };
>
> ...
>
> > struct iio_val_fractional {
> > int dividend;
> > int divisor;
> > };
>
> This one...
>
> > struct iio_val_fractional_log2 {
> > int dividend;
> > int divisor;
> > };
>
> ...and this one repeat struct s32_fract (or u32_fract, whatever suits better).
>
> > Do you agree?
>
> Me, yes, but you need a blessing by maintainers of IIO.
I'm not 100% convinced it matters, particularly as one of the two typical
use paths has to cast them to an int * anyway (as it can take any of the
above, or a 1D array of ints). However, if it makes drivers a little
easier to read then fair enough. I'm not keen to see a brute force
set of patches updating existing drivers that treat them as simple array
of ints though. Fine to convert any drivers with a local equivalent of these
structures defined.
Jonathan
On Mon, 21 Nov 2022 13:35:42 +0100
Gerald Loacker <gerald.loacker@wolfvision.net> wrote:
> Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
> Additionally to temperature and magnetic X, Y and Z-axes the angle and
> magnitude are reported.
> The sensor is operating in continuous measurement mode and changes to sleep
> mode if not used for 5 seconds.
>
> Datasheet: https://www.ti.com/lit/gpn/tmag5273
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
Hi Gerald,
A few additional comment from me. I quickly read Andy's and have tried
not to overlap too much!
Thanks,
Jonathan
> +static int tmag5273_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, int *val,
> + int *val2, long mask)
> +{
> + struct tmag5273_data *data = iio_priv(indio_dev);
> + s16 t, x, y, z;
> + u16 angle, magnitude;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + case IIO_CHAN_INFO_RAW:
> + ret = pm_runtime_resume_and_get(data->dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = tmag5273_get_measure(data, &t, &x, &y, &z, &angle,
> + &magnitude);
> + if (ret)
> + return ret;
> +
> + pm_runtime_mark_last_busy(data->dev);
> + pm_runtime_put_autosuspend(data->dev);
> +
> + switch (chan->address) {
> + case TEMPERATURE:
> + *val = t;
> + return IIO_VAL_INT;
> + case AXIS_X:
> + *val = x;
> + return IIO_VAL_INT;
> + case AXIS_Y:
> + *val = y;
> + return IIO_VAL_INT;
> + case AXIS_Z:
> + *val = z;
> + return IIO_VAL_INT;
> + case ANGLE:
> + *val = angle;
> + return IIO_VAL_INT;
> + case MAGNITUDE:
> + *val = magnitude;
> + return IIO_VAL_INT;
> + default:
> + dev_err(data->dev, "Unknown channel %lu\n",
> + chan->address);
Should be impossible to get here so no need for print.
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_TEMP:
> + /*
> + * Convert device specific value to millicelsius.
> + * Resolution from the sensor is 60.1 LSB/celsius and
> + * the reference value at 25 celsius is 17508 LSBs.
> + */
> + *val = 10000;
> + *val2 = 601;
> + return IIO_VAL_FRACTIONAL;
> + case IIO_MAGN:
> + /* Magnetic resolution in uT */
> + *val = 0;
> + *val2 = tmag5273_scale_table[data->version]
> + [data->scale_index]
> + .scale_micro;
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_ANGL:
> + /*
> + * Angle is in degrees and has four fractional bits,
> + * therefore use 1/16 * pi/180 to convert to radiants.
> + */
> + *val = 1000;
> + *val2 = 916732;
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_OFFSET:
> + switch (chan->type) {
> + case IIO_TEMP:
> + *val = -266314;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + *val = data->conv_avg;
> + return IIO_VAL_INT;
> +
> + default:
> + /* Unknown request */
> + return -EINVAL;
> + }
> +}
> +
> +static int tmag5273_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct tmag5273_data *data = iio_priv(indio_dev);
> + int i, ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + if (val == data->conv_avg)
> + return 0;
> + return regmap_update_bits(data->map, TMAG5273_DEVICE_CONFIG_1,
> + TMAG5273_AVG_MODE_MASK, val);
Update conv_avg? Or don't store cached value at all, and always read it from
the device (or regmap cache possibly if you have that enabled)
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_MAGN:
> + if (val != 0)
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(tmag5273_scale_table[0]);
> + i++) {
What Andy said. Sometimes wrapping is just too ugly!
> + if (tmag5273_scale_table[data->version][i]
> + .scale_micro == val2)
This line break is nasty as well. The whole thing is deeply enough nested
I'd just rip it out to a helper function called something like
tmag5273_write_scale() (or _magn_scale())
> + break;
> + }
> + if (i == ARRAY_SIZE(tmag5273_scale_table[0]))
> + return -EINVAL;
> + data->scale_index = i;
> +
> + ret = regmap_update_bits(
> + data->map, TMAG5273_SENSOR_CONFIG_2,
> + TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK,
> + data->scale_index == MAGN_LOW ? 0 :
> + (TMAG5273_Z_RANGE_MASK |
> + TMAG5273_X_Y_RANGE_MASK));
> + if (ret)
> + return ret;
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static void tmag5273_read_device_property(struct tmag5273_data *data)
> +{
> + const char *angle_measurement;
> +
> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
> +
> + if (!device_property_read_string(data->dev, "ti,angle-measurement",
> + &angle_measurement)) {
> + if (!strcmp(angle_measurement, "off"))
> + data->angle_measurement = TMAG5273_ANGLE_EN_OFF;
> + else if (!strcmp(angle_measurement, "x-y"))
> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
> + else if (!strcmp(angle_measurement, "y-z"))
> + data->angle_measurement = TMAG5273_ANGLE_EN_Y_Z;
> + else if (!strcmp(angle_measurement, "x-z"))
> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Z;
> + else
> + dev_warn(data->dev,
> + "failed to read angle-measurement\n");
Somewhat inaccurate warning. I'd print something like
"unexpected read angle-measurement property: %s\n", angle_measurement);
We read it. We just have no idea what it means! :)
> + }
> +}
> +
Hi Gerald,
I love your patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.1-rc6 next-20221123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Gerald-Loacker/add-ti-tmag5273-driver/20221121-203801
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20221121123542.1322367-3-gerald.loacker%40wolfvision.net
patch subject: [PATCH v2 2/2] iio: magnetometer: add ti tmag5273 driver
config: parisc-allmodconfig
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/fbe8af573f21cce30ccb7a8edbfc74770ab4b1a3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Gerald-Loacker/add-ti-tmag5273-driver/20221121-203801
git checkout fbe8af573f21cce30ccb7a8edbfc74770ab4b1a3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash drivers/iio/magnetometer/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/iio/magnetometer/tmag5273.c: In function 'tmag5273_chip_init':
>> drivers/iio/magnetometer/tmag5273.c:66:42: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
66 | #define TMAG5273_AVG_32_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 5)
| ^~~~~~~~~~
drivers/iio/magnetometer/tmag5273.c:506:28: note: in expansion of macro 'TMAG5273_AVG_32_MODE'
506 | TMAG5273_AVG_32_MODE);
| ^~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/FIELD_PREP +66 drivers/iio/magnetometer/tmag5273.c
58
59 /* bits in the TMAG5273_DEVICE_CONFIG_1 register */
60 #define TMAG5273_AVG_MODE_MASK GENMASK(4, 2)
61 #define TMAG5273_AVG_1_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 0)
62 #define TMAG5273_AVG_2_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 1)
63 #define TMAG5273_AVG_4_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 2)
64 #define TMAG5273_AVG_8_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 3)
65 #define TMAG5273_AVG_16_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 4)
> 66 #define TMAG5273_AVG_32_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 5)
67
@@ -20618,6 +20618,7 @@ M: Gerald Loacker <gerald.loacker@wolfvision.net>
L: linux-iio@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
+F: drivers/iio/magnetometer/tmag5273.c
TI TRF7970A NFC DRIVER
M: Mark Greer <mgreer@animalcreek.com>
@@ -208,6 +208,18 @@ config SENSORS_RM3100_SPI
To compile this driver as a module, choose M here: the module
will be called rm3100-spi.
+config TI_TMAG5273
+ tristate "TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ Say Y here to add support for the TI TMAG5273 Low-Power
+ Linear 3D 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 tmag5273.
+
config YAMAHA_YAS530
tristate "Yamaha YAS530 family of 3-Axis Magnetometers (I2C)"
depends on I2C
@@ -29,4 +29,6 @@ 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_TI_TMAG5273) += tmag5273.o
+
obj-$(CONFIG_YAMAHA_YAS530) += yamaha-yas530.o
new file mode 100644
@@ -0,0 +1,725 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for the TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor
+ *
+ * Copyright (C) 2022 WolfVision GmbH
+ *
+ * Author: Gerald Loacker <gerald.loacker@wolfvision.net>
+ */
+
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/pm_runtime.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#include <asm/unaligned.h>
+
+#define TMAG5273_DEVICE_CONFIG_1 0x00
+#define TMAG5273_DEVICE_CONFIG_2 0x01
+#define TMAG5273_SENSOR_CONFIG_1 0x02
+#define TMAG5273_SENSOR_CONFIG_2 0x03
+#define TMAG5273_X_THR_CONFIG 0x04
+#define TMAG5273_Y_THR_CONFIG 0x05
+#define TMAG5273_Z_THR_CONFIG 0x06
+#define TMAG5273_T_CONFIG 0x07
+#define TMAG5273_INT_CONFIG_1 0x08
+#define TMAG5273_MAG_GAIN_CONFIG 0x09
+#define TMAG5273_MAG_OFFSET_CONFIG_1 0x0A
+#define TMAG5273_MAG_OFFSET_CONFIG_2 0x0B
+#define TMAG5273_I2C_ADDRESS 0x0C
+#define TMAG5273_DEVICE_ID 0x0D
+#define TMAG5273_MANUFACTURER_ID_LSB 0x0E
+#define TMAG5273_MANUFACTURER_ID_MSB 0x0F
+#define TMAG5273_T_MSB_RESULT 0x10
+#define TMAG5273_T_LSB_RESULT 0x11
+#define TMAG5273_X_MSB_RESULT 0x12
+#define TMAG5273_X_LSB_RESULT 0x13
+#define TMAG5273_Y_MSB_RESULT 0x14
+#define TMAG5273_Y_LSB_RESULT 0x15
+#define TMAG5273_Z_MSB_RESULT 0x16
+#define TMAG5273_Z_LSB_RESULT 0x17
+#define TMAG5273_CONV_STATUS 0x18
+#define TMAG5273_ANGLE_RESULT_MSB 0x19
+#define TMAG5273_ANGLE_RESULT_LSB 0x1A
+#define TMAG5273_MAGNITUDE_RESULT 0x1B
+#define TMAG5273_DEVICE_STATUS 0x1C
+
+#define TMAG5273_AUTOSLEEP_DELAY_MS 5000
+#define TMAG5273_MAX_AVERAGE 32
+
+/*
+ * bits in the TMAG5273_MANUFACTURER_ID_LSB / MSB register
+ * 16-bit read-only unique manufacturer ID
+ */
+#define TMAG5273_MANUFACTURER_ID 0x5449
+
+/* bits in the TMAG5273_DEVICE_CONFIG_1 register */
+#define TMAG5273_AVG_MODE_MASK GENMASK(4, 2)
+#define TMAG5273_AVG_1_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 0)
+#define TMAG5273_AVG_2_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 1)
+#define TMAG5273_AVG_4_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 2)
+#define TMAG5273_AVG_8_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 3)
+#define TMAG5273_AVG_16_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 4)
+#define TMAG5273_AVG_32_MODE FIELD_PREP(TMAG5273_AVG_MODE_MASK, 5)
+
+/* bits in the TMAG5273_DEVICE_CONFIG_2 register */
+#define TMAG5273_OP_MODE_MASK GENMASK(1, 0)
+#define TMAG5273_OP_MODE_STANDBY FIELD_PREP(TMAG5273_OP_MODE_MASK, 0)
+#define TMAG5273_OP_MODE_SLEEP FIELD_PREP(TMAG5273_OP_MODE_MASK, 1)
+#define TMAG5273_OP_MODE_CONT FIELD_PREP(TMAG5273_OP_MODE_MASK, 2)
+#define TMAG5273_OP_MODE_WAKEUP FIELD_PREP(TMAG5273_OP_MODE_MASK, 3)
+
+/* bits in the TMAG5273_SENSOR_CONFIG_1 register */
+#define TMAG5273_MAG_CH_EN_MASK GENMASK(7, 4)
+#define TMAG5273_MAG_CH_EN_X_Y_Z 0x07
+
+/* bits in the TMAG5273_SENSOR_CONFIG_2 register */
+#define TMAG5273_Z_RANGE_MASK BIT(0)
+#define TMAG5273_X_Y_RANGE_MASK BIT(1)
+#define TMAG5273_ANGLE_EN_MASK GENMASK(3, 2)
+#define TMAG5273_ANGLE_EN_OFF 0
+#define TMAG5273_ANGLE_EN_X_Y 1
+#define TMAG5273_ANGLE_EN_Y_Z 2
+#define TMAG5273_ANGLE_EN_X_Z 3
+
+/* bits in the TMAG5273_T_CONFIG register */
+#define TMAG5273_T_CH_EN BIT(0)
+
+/* bits in the TMAG5273_DEVICE_ID register */
+#define TMAG5273_VERSION_MASK GENMASK(1, 0)
+
+/* bits in the TMAG5273_CONV_STATUS register */
+#define TMAG5273_CONV_STATUS_COMPLETE BIT(0)
+
+enum tmag5273_channels {
+ TEMPERATURE = 0,
+ AXIS_X,
+ AXIS_Y,
+ AXIS_Z,
+ ANGLE,
+ MAGNITUDE,
+};
+
+enum tmag5273_scale_index { MAGN_LOW = 0, MAGN_HIGH };
+
+/* state container for the TMAG5273 driver */
+struct tmag5273_data {
+ struct device *dev;
+ unsigned int devid;
+ unsigned int version;
+ char name[16];
+ unsigned int conv_avg;
+ unsigned int scale;
+ enum tmag5273_scale_index scale_index;
+ unsigned int angle_measurement;
+ struct regmap *map;
+ struct regulator *vcc;
+
+ /*
+ * Locks the sensor for exclusive use during a measurement (which
+ * involves several register transactions so the regmap lock is not
+ * enough) so that measurements get serialized in a first-come-first-
+ * serve manner.
+ */
+ struct mutex lock;
+};
+
+/*
+ * Averaging enables additional sampling of the sensor data to reduce the noise
+ * effect, but also increases conversion time.
+ */
+unsigned int tmag5273_avg_table[] = {
+ 1, 2, 4, 8, 16, 32,
+};
+
+/*
+ * Magnetic resolution in Gauss for different TMAG5273 versions.
+ * Scale[Gauss] = Range[mT] * 1000 / 2^15 * 10, (1 mT = 10 Gauss)
+ * Only version 1 and 2 are valid, version 0 and 3 are reserved.
+ */
+static const struct {
+ unsigned int scale_int;
+ unsigned int scale_micro;
+} tmag5273_scale_table[4][2] = {
+ { { 0, 0 }, { 0, 0 } },
+ { { 0, 12200 }, { 0, 24400 } },
+ { { 0, 40600 }, { 0, 81200 } },
+ { { 0, 0 }, { 0, 0 } },
+};
+
+static int tmag5273_get_measure(struct tmag5273_data *data, s16 *t, s16 *x,
+ s16 *y, s16 *z, u16 *angle, u16 *magnitude)
+{
+ unsigned int status, val;
+ __be16 reg_data[4];
+ int ret;
+
+ mutex_lock(&data->lock);
+
+ /*
+ * Max. conversion time is 2425 us in 32x averaging mode for all three
+ * channels. Since we are in continuous measurement mode, a measurement
+ * may already be there, so poll for completed measurement with
+ * timeout.
+ */
+ ret = regmap_read_poll_timeout(data->map, TMAG5273_CONV_STATUS, status,
+ status & TMAG5273_CONV_STATUS_COMPLETE,
+ 100, 10000);
+ if (ret) {
+ dev_err_probe(data->dev, ret,
+ "timeout waiting for measurement\n");
+ goto out_unlock;
+ }
+
+ ret = regmap_bulk_read(data->map, TMAG5273_T_MSB_RESULT, reg_data,
+ sizeof(reg_data));
+ if (ret)
+ goto out_unlock;
+ *t = be16_to_cpu(reg_data[0]);
+ *x = be16_to_cpu(reg_data[1]);
+ *y = be16_to_cpu(reg_data[2]);
+ *z = be16_to_cpu(reg_data[3]);
+
+ ret = regmap_bulk_read(data->map, TMAG5273_ANGLE_RESULT_MSB, reg_data,
+ sizeof(reg_data[0]));
+ if (ret)
+ goto out_unlock;
+ /*
+ * angle has 9 bits integer value and 4 bits fractional part
+ * 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
+ * 0 0 0 a a a a a a a a a f f f f
+ */
+ *angle = be16_to_cpu(reg_data[0]);
+
+ ret = regmap_read(data->map, TMAG5273_MAGNITUDE_RESULT, &val);
+ if (ret < 0)
+ goto out_unlock;
+ *magnitude = val;
+
+out_unlock:
+ mutex_unlock(&data->lock);
+ return ret;
+}
+
+static int tmag5273_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ struct tmag5273_data *data = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ *vals = tmag5273_avg_table;
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(tmag5273_avg_table);
+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_MAGN:
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ *vals = (int *)tmag5273_scale_table[data->version];
+ *length = ARRAY_SIZE(
+ tmag5273_scale_table[data->version]) * 2;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int tmag5273_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, int *val,
+ int *val2, long mask)
+{
+ struct tmag5273_data *data = iio_priv(indio_dev);
+ s16 t, x, y, z;
+ u16 angle, magnitude;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ case IIO_CHAN_INFO_RAW:
+ ret = pm_runtime_resume_and_get(data->dev);
+ if (ret < 0)
+ return ret;
+
+ ret = tmag5273_get_measure(data, &t, &x, &y, &z, &angle,
+ &magnitude);
+ if (ret)
+ return ret;
+
+ pm_runtime_mark_last_busy(data->dev);
+ pm_runtime_put_autosuspend(data->dev);
+
+ switch (chan->address) {
+ case TEMPERATURE:
+ *val = t;
+ return IIO_VAL_INT;
+ case AXIS_X:
+ *val = x;
+ return IIO_VAL_INT;
+ case AXIS_Y:
+ *val = y;
+ return IIO_VAL_INT;
+ case AXIS_Z:
+ *val = z;
+ return IIO_VAL_INT;
+ case ANGLE:
+ *val = angle;
+ return IIO_VAL_INT;
+ case MAGNITUDE:
+ *val = magnitude;
+ return IIO_VAL_INT;
+ default:
+ dev_err(data->dev, "Unknown channel %lu\n",
+ chan->address);
+ return -EINVAL;
+ }
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_TEMP:
+ /*
+ * Convert device specific value to millicelsius.
+ * Resolution from the sensor is 60.1 LSB/celsius and
+ * the reference value at 25 celsius is 17508 LSBs.
+ */
+ *val = 10000;
+ *val2 = 601;
+ return IIO_VAL_FRACTIONAL;
+ case IIO_MAGN:
+ /* Magnetic resolution in uT */
+ *val = 0;
+ *val2 = tmag5273_scale_table[data->version]
+ [data->scale_index]
+ .scale_micro;
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_ANGL:
+ /*
+ * Angle is in degrees and has four fractional bits,
+ * therefore use 1/16 * pi/180 to convert to radiants.
+ */
+ *val = 1000;
+ *val2 = 916732;
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
+ case IIO_CHAN_INFO_OFFSET:
+ switch (chan->type) {
+ case IIO_TEMP:
+ *val = -266314;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ *val = data->conv_avg;
+ return IIO_VAL_INT;
+
+ default:
+ /* Unknown request */
+ return -EINVAL;
+ }
+}
+
+static int tmag5273_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ struct tmag5273_data *data = iio_priv(indio_dev);
+ int i, ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ if (val == data->conv_avg)
+ return 0;
+ return regmap_update_bits(data->map, TMAG5273_DEVICE_CONFIG_1,
+ TMAG5273_AVG_MODE_MASK, val);
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_MAGN:
+ if (val != 0)
+ return -EINVAL;
+
+ for (i = 0; i < ARRAY_SIZE(tmag5273_scale_table[0]);
+ i++) {
+ if (tmag5273_scale_table[data->version][i]
+ .scale_micro == val2)
+ break;
+ }
+ if (i == ARRAY_SIZE(tmag5273_scale_table[0]))
+ return -EINVAL;
+ data->scale_index = i;
+
+ ret = regmap_update_bits(
+ data->map, TMAG5273_SENSOR_CONFIG_2,
+ TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK,
+ data->scale_index == MAGN_LOW ? 0 :
+ (TMAG5273_Z_RANGE_MASK |
+ TMAG5273_X_Y_RANGE_MASK));
+ if (ret)
+ return ret;
+
+ return 0;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+#define TMAG5273_AXIS_CHANNEL(axis, index) \
+ { \
+ .type = IIO_MAGN, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_##axis, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_type_available = \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_all = \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .info_mask_shared_by_all_available = \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .address = index, \
+ .scan_index = index, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 16, \
+ .storagebits = 16, \
+ .endianness = IIO_CPU, \
+ }, \
+ }
+
+static const struct iio_chan_spec tmag5273_channels[] = {
+ {
+ .type = IIO_TEMP,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OFFSET),
+ .address = TEMPERATURE,
+ .scan_index = TEMPERATURE,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_CPU,
+ },
+ },
+ TMAG5273_AXIS_CHANNEL(X, AXIS_X),
+ TMAG5273_AXIS_CHANNEL(Y, AXIS_Y),
+ TMAG5273_AXIS_CHANNEL(Z, AXIS_Z),
+ {
+ .type = IIO_ANGL,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_shared_by_all =
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+ .info_mask_shared_by_all_available =
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+ .address = ANGLE,
+ .scan_index = ANGLE,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_CPU,
+ },
+ },
+ {
+ .type = IIO_DISTANCE,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_all =
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+ .info_mask_shared_by_all_available =
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+ .address = MAGNITUDE,
+ .scan_index = MAGNITUDE,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_CPU,
+ },
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(6),
+};
+
+static const struct iio_info tmag5273_info = {
+ .read_avail = &tmag5273_read_avail,
+ .read_raw = &tmag5273_read_raw,
+ .write_raw = &tmag5273_write_raw,
+};
+
+static bool tmag5273_volatile_reg(struct device *dev, unsigned int reg)
+{
+ return reg >= TMAG5273_T_MSB_RESULT && reg <= TMAG5273_MAGNITUDE_RESULT;
+}
+
+static const struct regmap_config tmag5273_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = 0xff,
+ .volatile_reg = tmag5273_volatile_reg,
+};
+
+static int tmag5273_set_operating_mode(struct tmag5273_data *data,
+ unsigned int val)
+{
+ return regmap_write(data->map, TMAG5273_DEVICE_CONFIG_2, val);
+}
+
+static void tmag5273_read_device_property(struct tmag5273_data *data)
+{
+ const char *angle_measurement;
+
+ data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
+
+ if (!device_property_read_string(data->dev, "ti,angle-measurement",
+ &angle_measurement)) {
+ if (!strcmp(angle_measurement, "off"))
+ data->angle_measurement = TMAG5273_ANGLE_EN_OFF;
+ else if (!strcmp(angle_measurement, "x-y"))
+ data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
+ else if (!strcmp(angle_measurement, "y-z"))
+ data->angle_measurement = TMAG5273_ANGLE_EN_Y_Z;
+ else if (!strcmp(angle_measurement, "x-z"))
+ data->angle_measurement = TMAG5273_ANGLE_EN_X_Z;
+ else
+ dev_warn(data->dev,
+ "failed to read angle-measurement\n");
+ }
+}
+
+static int tmag5273_chip_init(struct tmag5273_data *data)
+{
+ int ret;
+
+ ret = regmap_write(data->map, TMAG5273_DEVICE_CONFIG_1,
+ TMAG5273_AVG_32_MODE);
+ if (ret)
+ return ret;
+ data->conv_avg = 32;
+
+ ret = regmap_write(data->map, TMAG5273_DEVICE_CONFIG_2,
+ TMAG5273_OP_MODE_CONT);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(data->map, TMAG5273_SENSOR_CONFIG_1,
+ FIELD_PREP(TMAG5273_MAG_CH_EN_MASK,
+ TMAG5273_MAG_CH_EN_X_Y_Z));
+ if (ret)
+ return ret;
+
+ ret = regmap_write(data->map, TMAG5273_SENSOR_CONFIG_2,
+ FIELD_PREP(TMAG5273_ANGLE_EN_MASK,
+ data->angle_measurement));
+ if (ret)
+ return ret;
+ data->scale_index = MAGN_LOW;
+
+ return regmap_write(data->map, TMAG5273_T_CONFIG, TMAG5273_T_CH_EN);
+}
+
+static int tmag5273_wake_up_and_check_device_id(struct tmag5273_data *data)
+{
+ __le16 devid;
+ int val, ret;
+
+ /*
+ * If we come from sleep with power already activated, the
+ * first I2C command wakes up the chip but will fail.
+ * Time to go to stand-by mode from sleep mode is 50us
+ * typically. During this time no I2C access is possible.
+ */
+ regmap_read(data->map, TMAG5273_DEVICE_ID, &val);
+ usleep_range(80, 200);
+ ret = regmap_read(data->map, TMAG5273_DEVICE_ID, &val);
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "failed to power on device\n");
+ data->version = FIELD_PREP(TMAG5273_VERSION_MASK, val);
+
+ ret = regmap_bulk_read(data->map, TMAG5273_MANUFACTURER_ID_LSB, &devid,
+ sizeof(devid));
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "failed to read device ID\n");
+ data->devid = le16_to_cpu(devid);
+
+ switch (data->devid) {
+ case TMAG5273_MANUFACTURER_ID:
+ snprintf(data->name, sizeof(data->name), "tmag5273x%1u",
+ data->version);
+ if (data->version < 1 || data->version > 2)
+ dev_warn(data->dev, "Unsupported device version 0x%x\n",
+ data->version);
+ break;
+ default:
+ dev_warn(data->dev, "Unknown device ID 0x%x\n", data->devid);
+ break;
+ }
+
+ return 0;
+}
+
+static void tmag5273_power_down(void *data)
+{
+ tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);
+}
+
+static int tmag5273_probe(struct i2c_client *i2c)
+{
+ struct iio_dev *indio_dev;
+ struct device *dev = &i2c->dev;
+ struct tmag5273_data *data;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return dev_err_probe(dev, -ENOMEM,
+ "IIO device allocation failed\n");
+
+ data = iio_priv(indio_dev);
+ data->dev = dev;
+ i2c_set_clientdata(i2c, indio_dev);
+
+ data->map = devm_regmap_init_i2c(i2c, &tmag5273_regmap_config);
+ if (IS_ERR(data->map))
+ return dev_err_probe(dev, PTR_ERR(data->map),
+ "failed to allocate register map\n");
+
+ mutex_init(&data->lock);
+
+ ret = devm_regulator_get_enable(dev, "vcc");
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to enable regulator\n");
+
+ ret = tmag5273_wake_up_and_check_device_id(data);
+ if (ret)
+ return ret;
+
+ ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to power on device\n");
+
+ /*
+ * Register powerdown deferred callback which suspends the chip
+ * after module unloaded.
+ *
+ * TMAG5273 should be in SUSPEND mode in the two cases:
+ * 1) When driver is loaded, but we do not have any data or
+ * configuration requests to it (we are solving it using
+ * autosuspend feature).
+ * 2) When driver is unloaded and device is not used (devm action is
+ * used in this case).
+ */
+ ret = devm_add_action_or_reset(dev, tmag5273_power_down, data);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to add powerdown action\n");
+
+ ret = pm_runtime_set_active(dev);
+ if (ret < 0)
+ return ret;
+
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return ret;
+
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_autosuspend_delay(dev, TMAG5273_AUTOSLEEP_DELAY_MS);
+ pm_runtime_use_autosuspend(dev);
+
+ tmag5273_read_device_property(data);
+
+ ret = tmag5273_chip_init(data);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to init device\n");
+
+ indio_dev->info = &tmag5273_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->name = data->name;
+ indio_dev->channels = tmag5273_channels;
+ indio_dev->num_channels = ARRAY_SIZE(tmag5273_channels);
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "device register failed\n");
+
+ return 0;
+}
+
+static int tmag5273_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tmag5273_data *data = iio_priv(indio_dev);
+ int ret;
+
+ ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);
+ if (ret)
+ dev_err(dev, "failed to power off device (%pe)\n",
+ ERR_PTR(ret));
+
+ return ret;
+}
+
+static int tmag5273_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tmag5273_data *data = iio_priv(indio_dev);
+ int ret;
+
+ /*
+ * Time to go to stand-by mode from sleep mode is 50us
+ * typically. During this time no I2C access is possible.
+ */
+ tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
+ usleep_range(80, 200);
+ ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
+ if (ret)
+ dev_err(dev, "failed to power on device (%pe)\n", ERR_PTR(ret));
+
+ return ret;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(tmag5273_pm_ops, tmag5273_runtime_suspend,
+ tmag5273_runtime_resume, NULL);
+
+static const struct i2c_device_id tmag5273_id[] = {
+ { "tmag5273" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, tmag5273_id);
+
+static const struct of_device_id tmag5273_of_match[] = {
+ { .compatible = "ti,tmag5273" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, tmag5273_of_match);
+
+static struct i2c_driver tmag5273_driver = {
+ .driver = {
+ .name = "tmag5273",
+ .of_match_table = tmag5273_of_match,
+ .pm = pm_ptr(&tmag5273_pm_ops),
+ },
+ .probe_new = tmag5273_probe,
+ .id_table = tmag5273_id,
+};
+module_i2c_driver(tmag5273_driver);
+
+MODULE_DESCRIPTION("TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor driver");
+MODULE_AUTHOR("Gerald Loacker <gerald.loacker@wolfvision.net>");
+MODULE_LICENSE("GPL");