diff mbox series

[v2,2/2] iio: magnetometer: add ti tmag5273 driver

Message ID 20221121123542.1322367-3-gerald.loacker@wolfvision.net (mailing list archive)
State Changes Requested
Headers show
Series add ti tmag5273 driver | expand

Commit Message

Gerald Loacker Nov. 21, 2022, 12:35 p.m. UTC
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

Andy Shevchenko Nov. 21, 2022, 2:04 p.m. UTC | #1
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, &reg_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;
Gerald Loacker Nov. 21, 2022, 5:24 p.m. UTC | #2
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
Andy Shevchenko Nov. 21, 2022, 6:40 p.m. UTC | #3
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);
Andy Shevchenko Nov. 21, 2022, 6:56 p.m. UTC | #4
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.
Gerald Loacker Nov. 22, 2022, 7:39 a.m. UTC | #5
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
Gerald Loacker Nov. 23, 2022, 9:58 a.m. UTC | #6
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
Andy Shevchenko Nov. 23, 2022, 1:39 p.m. UTC | #7
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.
Jonathan Cameron Nov. 23, 2022, 5:56 p.m. UTC | #8
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
Jonathan Cameron Nov. 23, 2022, 6:10 p.m. UTC | #9
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! :)

> +	}
> +}
> +
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index ea7acec52f8b..9d20b5780051 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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>
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index b91fc5e6a26e..467819335588 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -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
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index b9f45b7fafc3..b1c784ea71c8 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -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
diff --git a/drivers/iio/magnetometer/tmag5273.c b/drivers/iio/magnetometer/tmag5273.c
new file mode 100644
index 000000000000..7a8217dfda3f
--- /dev/null
+++ b/drivers/iio/magnetometer/tmag5273.c
@@ -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");