| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 23 Nov 2022 18:10:21 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Gerald Loacker <gerald.loacker@...fvision.net>
CC: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Nikita Yushchenko <nikita.yoush@...entembedded.com>,
Jakob Hauser <jahau@...ketmail.com>,
Michael Riesch <michael.riesch@...fvision.net>
Subject: Re: [PATCH v2 2/2] iio: magnetometer: add ti tmag5273 driver
On Mon, 21 Nov 2022 13:35:42 +0100
Gerald Loacker <gerald.loacker@...fvision.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@...fvision.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! :)
> + }
> +}
> +
Powered by blists - more mailing lists