| 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
| ||
|
Message-ID: <CAHp75VdH7bxuPW6Fx4Mcq18hQfr1sDhBYDwGn8OeurQOAar2kg@mail.gmail.com>
Date: Tue, 29 Oct 2024 21:48:42 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Neil Armstrong <neil.armstrong@...aro.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org
Subject: Re: [PATCH v3 3/3] iio: magnetometer: add Allegro MicroSystems
ALS31300 3-D Linear Hall Effect driver
On Tue, Oct 29, 2024 at 4:13 PM Neil Armstrong
<neil.armstrong@...aro.org> wrote:
>
> The Allegro MicroSystems ALS31300 is a 3-D Linear Hall Effect Sensor
> mainly used for 3D head-on motion sensing applications.
>
> The device is configured over I2C, and as part of the Sensor data the
> temperature core is also provided.
>
> While the device provides an IRQ gpio, it depends on a configuration
> programmed into the internal EEPROM, thus only the default mode is
> supported and buffered input via trigger is also supported to allow
> streaming values with the same sensing timestamp.
>
> The device can be configured with different sensitivities in factory,
> but the sensitivity value used to calculate value into the Gauss
> unit is not available from registers, thus the sensitivity is provided
> by the compatible/device-id string which is based on the part number
> as described in the datasheet page 2.
Thank you for an update, this looks more or less good. I have a few
nit-picks below. With them addressed,
Reviewed-by: Andy Shevchenko <andy@...nel.org>
...
> +#include <linux/types.h>
> +#include <linux/units.h>
It's a bit of an unusual order. Do you mean to put them after the
regulator/*.h one?
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
...
> +#define ALS31300_DATA_X_GET(b) \
> + sign_extend32(FIELD_GET(ALS31300_VOL_MSB_X_AXIS, b[0]) << 4 | \
> + FIELD_GET(ALS31300_VOL_LSB_X_AXIS, b[1]), 11)
> +#define ALS31300_DATA_Y_GET(b) \
> + sign_extend32(FIELD_GET(ALS31300_VOL_MSB_Y_AXIS, b[0]) << 4 | \
> + FIELD_GET(ALS31300_VOL_LSB_Y_AXIS, b[1]), 11)
> +#define ALS31300_DATA_Z_GET(b) \
> + sign_extend32(FIELD_GET(ALS31300_VOL_MSB_Z_AXIS, b[0]) << 4 | \
> + FIELD_GET(ALS31300_VOL_LSB_Z_AXIS, b[1]), 11)
> +#define ALS31300_TEMPERATURE_GET(b) \
> + (FIELD_GET(ALS31300_VOL_MSB_TEMPERATURE, b[0]) << 6 | \
> + FIELD_GET(ALS31300_VOL_LSB_TEMPERATURE, b[1]))
Yeah, I have got that the data is interlaced, and it's still possible
to use the __be64, but the resulting code might be too overengineered
for this simple case (as it would require bitmap operations to remap
interlaced bits and an additional churn on top of u64 to be
represented as set of unsigned long:s).
...
> +/* The whole measure is split into 2x32bit registers, we need to read them both at once */
32-bit
...
> + /*
> + * Loop until data is valid, new data should have the
> + * ALS31300_VOL_MSB_NEW_DATA bit set to 1.
> + * Max update rate is 2KHz, wait up to 1ms
Missing period at the end.
> + */
...
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + case IIO_CHAN_INFO_RAW:
> + ret = als31300_get_measure(data, &t, &x, &y, &z);
> + if (ret)
> + return ret;
> +
> + 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;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_TEMP:
> + /*
> + * Fractional part of:
> + * 1000 * 302 * (value - 1708)
> + * temp = ----------------------------
> + * 4096
> + * to convert temperature in millicelcius
Missing period at the end.
> + */
> + *val = MILLI * 302;
> + *val2 = 4096;
> + return IIO_VAL_FRACTIONAL;
> + case IIO_MAGN:
> + /*
> + * Devices are configured in factory
> + * with different sensitivities:
> + * - 500 GAUSS <-> 4 LSB/Gauss
> + * - 1000 GAUSS <-> 2 LSB/Gauss
> + * - 2000 GAUSS <-> 1 LSB/Gauss
> + * with translates by a division of the returned
> + * value to get Gauss value.
> + * The sensisitivity cannot be read at runtime
sensitivity
> + * so the value depends on the model compatible
> + * or device id.
> + */
> + *val = 1;
> + *val2 = data->variant_info->sensitivity;
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_OFFSET:
> + switch (chan->type) {
> + case IIO_TEMP:
> + *val = -1708;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +
Seems like a stray blank line here.
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int als31300_set_operating_mode(struct als31300_data *data,
> + unsigned int val)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(data->map, ALS31300_VOL_MODE,
> + ALS31300_VOL_MODE_SLEEP, val);
> + if (ret) {
> + dev_err(data->dev, "failed to set operating mode (%pe)\n", ERR_PTR(ret));
> + return ret;
> + }
> +
> + /* The time it takes to exit sleep mode is equivalent to Power-On Delay Time */
> + if (val == ALS31300_VOL_MODE_ACTIVE_MODE)
> + usleep_range(600, 650);
fsleep() ?
> + return 0;
> +}
...
> + devm_mutex_init(dev, &data->mutex);
Hmm... While not critical, this may still return an error. There were
only two (out of ~15) users that ignored the error code, in v6.12
there are two more in IIO, while one is checking for it. I would like
to check for an error and bail out (and maybe I'll update the other
drivers in IIO to follow, including one GPIO module that ignores it).
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists