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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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;

-- 
With Best Regards,
Andy Shevchenko