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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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! :)

> +	}
> +}
> +