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

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

On Tue, Nov 15, 2022 at 08:37:18AM +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.
> 
> Datasheet: https://www.ti.com/lit/gpn/tmag5273

> 

Drop this blank line to make above to be a tag.

> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>

...

> +#define TMAG5273_MANUFACTURER_ID        0x5449

Can you add ASCII comment on that magic value?

...

> +#define TMAG5273_AUTOSLEEP_DELAY	 5000

Units?

> +struct tmag5273_data {
> +	struct device *dev;
> +	unsigned int devid;
> +	unsigned int version;

> +	char name[16];
> +	int conv_avg;
> +	int max_avg;
> +	int range;
> +	u32 angle_en;
> +	struct regmap *map;
> +	struct regulator *vcc;

+ Blank line

> +	/* 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.
> +	 */

/*
 * Wrong multi-line
 * comment style. Fix
 * it accordingly.
 */

> +	struct mutex lock;
> +};

...

> +static const struct {
> +	int avg;
> +	u8 reg_val;
> +} tmag5273_avg_table[] = {
> +	{ 1, 0x00 }, { 2, 0x01 },  { 4, 0x02 },
> +	{ 8, 0x03 }, { 16, 0x04 }, { 32, 0x05 },

Isn't the second one is just an index?

> +};


...

> +/*
> + * magnetic range in mT for different TMAG5273 versions

Magnetic

> + * only version 1 and 2 are valid, version 0 and 3 are reserved
> + */
> +static const struct {
> +	int range;
> +	u8 reg_val;
> +} tmag5273_range_table[4][2] = {

I believe you can drop 4.

> +	{ { 0, 0 }, { 0, 3 } },
> +	{ { 40, 0 }, { 80, 3 } },
> +	{ { 133, 0 }, { 266, 3 } },
> +	{ { 0, 0 }, { 0, 3 } },
> +};

...

> +/*

Shouldn't it be a kernel doc? Ditto for the rest.

> + * tmag5273_measure() - Make a measure from the hardware
> + * @tmag5273: The device state
> + * @t: the processed temperature measurement
> + * @x: the raw x axis measurement
> + * @y: the raw x axis measurement
> + * @z: the raw x axis measurement
> + * @angle: the calculated angle
> + * @magnitude: the calculated magnitude
> + * @return: 0 on success or error code
> + */

...

> +	/*
> +	 * convert device specific value to millicelsius

Convert

> +	 * use multiply by 16639 and divide by 10000 to achieve divide by 60.1
> +	 *   and convert to millicelsius

One space is enough and missing period.

> +	 */

> +}

...

> +static ssize_t tmag5273_show_conv_avg(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct tmag5273_data *data = iio_priv(indio_dev);
> +
> +	return sprintf(buf, "%d\n", data->conv_avg);

Must be sysfs_emit().

> +}

...

> +	for (i = 0; i < ARRAY_SIZE(tmag5273_avg_table); i++) {
> +		if (tmag5273_avg_table[i].avg == val)
> +			break;
> +	}

> +

Redundant blank line.

> +	if (i == ARRAY_SIZE(tmag5273_avg_table))
> +		return -EINVAL;

...

> +static IIO_DEVICE_ATTR(conv_avg, 0644, tmag5273_show_conv_avg,
> +		       tmag5273_store_conv_avg, 0);

IIO_DEVICE_ATTR_RW() ?

...

> +	for (i = 0; i < ARRAY_SIZE(tmag5273_avg_table); i++) {
> +		if (tmag5273_avg_table[i].avg > data->max_avg)
> +			break;
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
> +				 tmag5273_avg_table[i].avg);

sysfs_emit_at()

> +	}
> +	/* replace last space with a newline */
> +	if (len > 0)
> +		buf[len - 1] = '\n';

...

> +static IIO_DEVICE_ATTR(conv_avg_available, 0444, tmag5273_conv_avg_available,
> +		       NULL, 0);

IIO_DEVICE_ATTR_RO()

...

> +	return sprintf(buf, "%d\n", data->range);

sysfs_emit().

...

> +	for (i = 0; i < ARRAY_SIZE(tmag5273_range_table[0]); i++) {
> +		if (tmag5273_range_table[data->version][i].range == val)
> +			break;
> +	}

> +

Redundant blank line.

> +	if (i == ARRAY_SIZE(tmag5273_range_table[0]))
> +		return -EINVAL;

...

> +static ssize_t tmag5273_store_range(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct tmag5273_data *data = iio_priv(indio_dev);
> +	int range, ret;
> +
> +	ret = kstrtoint(buf, 0, &range);

Here and in the other ->store() do you really have negative values possible?
Can you revisit all those kstrtoX() calls and check the arguments, so you can
narrow down the range of the input where it's appropriate?

> +	if (ret)
> +		return ret;
> +
> +	ret = tmag5273_write_range(data, range);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(range, 0644, tmag5273_show_range,
> +		       tmag5273_store_range, 0);

IIO_DEVICE_ATTR_RW()

> +	for (i = 0; i < ARRAY_SIZE(tmag5273_range_table[0]); i++) {
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
> +				 tmag5273_range_table[data->version][i].range);
> +	}

sysfs_emit();

> +	/* replace last space with a newline */
> +	if (len > 0)
> +		buf[len - 1] = '\n';
> +
> +	return len;
> +}

...

> +static IIO_DEVICE_ATTR(range_available, 0444, tmag5273_range_available, NULL,
> +		       0);

One line.

...

> +static struct attribute *tmag5273_attributes[] = {
> +	&iio_dev_attr_conv_avg.dev_attr.attr,
> +	&iio_dev_attr_conv_avg_available.dev_attr.attr,
> +	&iio_dev_attr_range.dev_attr.attr,
> +	&iio_dev_attr_range_available.dev_attr.attr,

> +	NULL,

No comma in terminator entry.

> +};

> +static const struct attribute_group tmag5273_attrs_group = {
> +	.attrs = tmag5273_attributes,
> +};

...

> +static bool tmag5273_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg >= TMAG5273_T_MSB_RESULT &&
> +		reg <= TMAG5273_MAGNITUDE_RESULT);

Drop parentheses and make it one line.

> +}

...

> +static int tmag5273_probe(struct i2c_client *i2c,
> +			  const struct i2c_device_id *id)

Why ->probe_new() can't be utilized?

...

> +	struct device_node *node = dev->of_node;

What for? Use device property API (see below).

...

> +	data->map = devm_regmap_init_i2c(i2c, &tmag5273_regmap_config);
> +	if (IS_ERR(data->map)) {

> +		ret = PTR_ERR(data->map);
> +		dev_err_probe(dev, ret, "failed to allocate register map\n");

	ret = dev_err_probe();

But don't you have an ordering issue here?

> +		goto out_disable_vcc;
> +	}

...

> +		strncpy(data->name, "TMAG5273", sizeof(data->name) - 2);
> +		switch (data->version) {
> +		case 1:
> +			strncat(data->name, "x1", 2);
> +			break;
> +		case 2:
> +			strncat(data->name, "x2", 2);
> +			break;
> +		default:
> +			break;
> +		}

This all can be replaced with fewer lines:

		if (data->version < 1 || data->version > 2)
			snprintf(data->name, "TMAG5273", sizeof(data->name));
		else
			snprintf(data->name, "TMAG5273x%.1u", sizeof(data->name), data->version);

...

> +		dev_info(dev, "%s", data->name);

Useless?

...

> +	of_property_read_u32(node, "tmag5273,angle-enable", &data->angle_en);

Missing header for that, but the question is why? What's wrong with device
property API instead?

...

> +static const struct i2c_device_id tmag5273_id[] = {
> +	{
> +		"tmag5273",
> +	},

Can be one line.

> +	{ /* sentinel */ },

No comma for terminator entry.

> +};

...

> +	{ /* sentinel */ },

Ditto.

-- 
With Best Regards,
Andy Shevchenko