| 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: <ff90fb92-ab4a-4719-b9c4-7b21245207b1@linaro.org>
Date: Wed, 30 Oct 2024 16:17:45 +0100
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
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 29/10/2024 20:48, Andy Shevchenko wrote:
> 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?
Ack
>
>> +#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
Ack
>
> ...
>
>> + /*
>> + * 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.
Ack
>
>> + */
>
> ...
>
>> + 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.
Ack
>
>> + */
>> + *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
Good catch
>
>> + * 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.
Ack
>
>> + 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() ?
Sure
>
>> + 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).
>
Ack, thanks for the review!
Neil
Powered by blists - more mailing lists