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;
--
With Best Regards,
Andy Shevchenko
