Hi,
On 26/10/2024 23:17, Andy Shevchenko wrote:
Mon, Oct 21, 2024 at 02:38:55PM +0200, Neil Armstrong kirjoitti:
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.
...
Some headers are missing...
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+ pm.h
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+ types.h
Ack
...
+#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]))
These and in the code seems like you make a home grown endianes conversion.
I suppose all of them have to be __beXX with the respective masks that cover
all the bits.
This has nothing do to with endianess conversion, the bits of the 4 channels
are split on 2 registers, look at the defines.
+static int als31300_get_measure(struct als31300_data *data,
+ u16 *t, s16 *x, s16 *y, s16 *z)
+{
+ unsigned int count = 0;
+ u32 buf[2];
Shouldn't this be __be64 buf?
No this is 2 separate registers with different fields, not a single 64bit big-endian register
+ int ret;
+
+ guard(mutex)(&data->mutex);
+
+ ret = pm_runtime_resume_and_get(data->dev);
+ if (ret)
+ return ret;
+ /* Max update rate is 2KHz, wait up to 1ms */
+ while (count < 50) {
+ /* Read Data */
+ ret = regmap_bulk_read(data->map, ALS31300_VOL_MSB, buf, 2);
At bare minimum ARRAY_SIZE(), but see above, I think it should use the respective type.
I'll use ARRAY_SIZE
+ if (ret) {
+ dev_err(data->dev, "read data failed, error %d\n", ret);
+ goto out;
+ }
+
+ /* Check if data is valid, happens right after getting out of sleep mode */
+ if (FIELD_GET(ALS31300_VOL_MSB_NEW_DATA, buf[0]))
+ break;
+
+ usleep_range(10, 20);
+ ++count;
+ }
+
+ if (count >= 50) {
+ ret = -ETIMEDOUT;
+ goto out;
+ }
This one of the longest variant of implementing
do {
...
} while (!FIELD_GET(...) && --count)
But you also may consider something from iopoll.h (I don't remember if we have
regmap_read_poll_timeout() for bulk transfers.
There's no poll for bulk read, I'll try using an IOPOLL function
+ *t = ALS31300_TEMPERATURE_GET(buf);
+ *x = ALS31300_DATA_X_GET(buf);
+ *y = ALS31300_DATA_Y_GET(buf);
+ *z = ALS31300_DATA_Z_GET(buf);
+
+out:
+ pm_runtime_mark_last_busy(data->dev);
+ pm_runtime_put_autosuspend(data->dev);
Last discussion with Rafael on the topic of the above puts a question mark to
all these cases when runtime PM idle callback has not been implemented.
Shouldn't this be simply
pm_runtime_put(data->dev);
?
Probably, I just used the same schema as other similar IIO drivers
+ return ret;
+}
...
+ 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
+ */
+ *val = 1000 * 302;
MILLI from units.h ?
Well, sure, but it's still 1000, should I also define ONE to 1 and so on ?
+ *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
+ * 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;
+ }
...
+ struct {
+ u16 temperature;
+ s16 channels[3];
+ aligned_s64 timestamp;
+ } __packed scan;
Why __packed?
Probably unneeded, will drop
...
+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);
+
+ return ret;
return 0;
It's the same but ok
+}
...
+static int als31300_probe(struct i2c_client *i2c)
+{
+ struct device *dev = &i2c->dev;
+ struct als31300_data *data;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ data->dev = dev;
+ i2c_set_clientdata(i2c, indio_dev);
+ mutex_init(&data->mutex);
Why not devm_mutex_init()? How does this being cleaned up?
Indeed, good catch
+ data->variant_info = i2c_get_match_data(i2c);
+ if (!data->variant_info)
+ return -EINVAL;
+
+ data->map = devm_regmap_init_i2c(i2c, &als31300_regmap_config);
+ if (IS_ERR(data->map))
+ return dev_err_probe(dev, PTR_ERR(data->map),
+ "failed to allocate register map\n");
+
+ ret = devm_regulator_get_enable(dev, "vcc");
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to enable regulator\n");
+
+ ret = als31300_set_operating_mode(data, ALS31300_VOL_MODE_ACTIVE_MODE);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to power on device\n");
+
+ ret = devm_add_action_or_reset(dev, als31300_power_down, data);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to add powerdown action\n");
+
+ indio_dev->info = &als31300_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->name = i2c->name;
+ indio_dev->channels = als31300_channels;
+ indio_dev->num_channels = ARRAY_SIZE(als31300_channels);
+ indio_dev->available_scan_masks = als31300_scan_masks;
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ iio_pollfunc_store_time,
+ als31300_trigger_handler,
+ &als31300_setup_ops);
+ if (ret < 0) {
+ dev_err(dev, "iio triggered buffer setup failed\n");
+ return ret;
Why not return dev_err_probe(...);
Sure
+ }
+
+ ret = pm_runtime_set_active(dev);
+ if (ret < 0)
+ return ret;
+
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return ret;
+
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_autosuspend_delay(dev, 200);
+ pm_runtime_use_autosuspend(dev);
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "device register failed\n");
+
+ return 0;
+}
Thanks,
Neil
