lkml.org 
[lkml]   [2022]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 2/2] iio: magnetometer: add ti tmag5273 driver
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


\
 
 \ /
  Last update: 2022-11-21 15:08    [W:0.126 / U:2.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site