| 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: <CAHp75VeKPr=3H_wOvcesqj4OsrqN7zwRFFk3ys3O012JpQtxrQ@mail.gmail.com>
Date: Thu, 7 Aug 2025 22:57:16 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Dixit Parmar <dixitparmar19@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 1/2] iio: magnetometer: add support for Infineon
TLV493D 3D Magentic sensor
On Thu, Aug 7, 2025 at 4:57 AM Dixit Parmar <dixitparmar19@...il.com> wrote:
>
> The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> applications includes joysticks, control elements (white goods,
> multifunction knops), or electric meters (anti tampering) and any
> other application that requires accurate angular measurements at
> low power consumptions.
>
> The Sensor is configured over I2C, and as part of Sensor measurement
> data it provides 3-Axis magnetic fields and temperature core measurement.
>
> The driver supports raw value read and buffered input via external trigger
> to allow streaming values with the same sensing timestamp.
>
> While the sensor has an interrupt pin multiplexed with an I2C SCL pin.
> But for bus configurations interrupt(INT) is not recommended, unless timing
> constraints between I2C data transfers and interrupt pulses are monitored
> and aligned.
>
> The Sensor's I2C register map and mode information is described in product
> User Manual [1].
...
> + help
> + Say Y here to add support for the Infineon TLV493D-A1B6 Low-
> + Power 3D Megnetic Sensor.
Megnetic?
> + This driver can also be compiled as a module.
> + To compile this driver as a module, choose M here: the module
> + will be called tlv493d.
...
> +#define TLV493D_RD_REG_BX 0x00
> +#define TLV493D_RD_REG_BY 0x01
> +#define TLV493D_RD_REG_BZ 0x02
> +#define TLV493D_RD_REG_TEMP 0x03
> +#define TLV493D_RD_REG_BX2 0x04
> +#define TLV493D_RD_REG_BZ2 0x05
> +#define TLV493D_RD_REG_TEMP2 0x06
> +#define TLV493D_RD_REG_RES1 0x07
> +#define TLV493D_RD_REG_RES2 0x08
> +#define TLV493D_RD_REG_RES3 0x09
> +#define TLV493D_RD_REG_MAX 0x0a
+ blank line
> +#define TLV493D_WR_REG_RES 0x00
I would name it _RES0 in analogue with the _RES2 below.
> +#define TLV493D_WR_REG_MODE1 0x01
> +#define TLV493D_WR_REG_RES2 0x02
> +#define TLV493D_WR_REG_MODE2 0x03
> +#define TLV493D_WR_REG_MAX 0x04
...
> +enum tlv493d_channels {
> + TLV493D_AXIS_X = 0,
Why assignment? Is this HW defined value? Then you must assign all of
them explicitly to make code robust to changes.
> + TLV493D_AXIS_Y,
> + TLV493D_AXIS_Z,
> + TLV493D_TEMPERATURE
> +};
> +
> +enum tlv493d_op_mode {
> + TLV493D_OP_MODE_POWERDOWN = 0,
Ditto.
> + TLV493D_OP_MODE_FAST,
> + TLV493D_OP_MODE_LOWPOWER,
> + TLV493D_OP_MODE_ULTRA_LOWPOWER,
> + TLV493D_OP_MODE_MASTERCONTROLLED
> +};
...
> +struct tlv493d_data {
> + struct device *dev;
> + struct i2c_client *client;
Why do you need both?
> + /* protects from simultaneous sensor access and register readings */
> + struct mutex lock;
> + enum tlv493d_op_mode mode;
> + u8 wr_regs[TLV493D_WR_REG_MAX];
> +};
...
> + data->wr_regs[TLV493D_WR_REG_MODE1] |= mode1_cfg;
> + data->wr_regs[TLV493D_WR_REG_MODE2] |= mode2_cfg;
No mask for the existing values in the respective wr_regs? Wouldn't
you need to use FIELD_MODIFY() instead?
...
> +static s16 tlv493d_get_channel_data(u8 *b, enum tlv493d_channels ch)
> +{
> + u16 val = 0;
I would move the default assignment to the 'default' case. This makes
the intention clearer.
> + switch (ch) {
> + case TLV493D_AXIS_X:
> + val = FIELD_GET(TLV493D_BX_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 |
> + FIELD_GET(TLV493D_BX2_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4;
> + break;
> + case TLV493D_AXIS_Y:
> + val = FIELD_GET(TLV493D_BY_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> + FIELD_GET(TLV493D_BX2_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
> + break;
> + case TLV493D_AXIS_Z:
> + val = FIELD_GET(TLV493D_BZ_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 |
> + FIELD_GET(TLV493D_BZ2_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]);
> + break;
> + case TLV493D_TEMPERATURE:
> + val = FIELD_GET(TLV493D_TEMP_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 |
> + FIELD_GET(TLV493D_TEMP2_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]);
> + break;
> + }
> +
> + return sign_extend32(val, 11);
> +}
...
> +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> + s16 *z, s16 *t)
> +{
> + u8 buff[7] = {};
> + int err, ret;
> + u32 sleep_us = tlv493d_sample_rate_us[data->mode];
> +
> + guard(mutex)(&data->lock);
No include for this API.
> + ret = pm_runtime_resume_and_get(data->dev);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * Poll until data is valid,
> + * For a valid data TLV493D_TEMP_CHANNEL bit of TLV493D_RD_REG_TEMP should be set to 0.
> + * The sampling time depends on the sensor mode. poll 3x the time of the sampling time.
> + */
> + ret = read_poll_timeout(i2c_master_recv, err, err ||
> + FIELD_GET(TLV493D_TEMP_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> + sleep_us, (3 * sleep_us), false, data->client, buff,
Redundant parentheses.
> + ARRAY_SIZE(buff));
Missing include for this macro.
> + if (ret) {
> + dev_err(data->dev, "i2c read poll timeout, error:%d\n", ret);
> + goto out;
> + }
> + if (err < 0) {
> + dev_err(data->dev, "i2c read data failed, error:%d\n", err);
> + ret = err;
> + goto out;
> + }
> +
> + *x = tlv493d_get_channel_data(buff, TLV493D_AXIS_X);
> + *y = tlv493d_get_channel_data(buff, TLV493D_AXIS_Y);
> + *z = tlv493d_get_channel_data(buff, TLV493D_AXIS_Z);
> + *t = tlv493d_get_channel_data(buff, TLV493D_TEMPERATURE);
> +
> +out:
Labels are better made when they define what they are going to perform.
out_put_autosuspend:
> + pm_runtime_put_autosuspend(data->dev);
> + return ret;
> +}
...
> + ret = tlv493d_set_operating_mode(data, data->mode);
> + if (ret < 0) {
Is ' < 0' part required here?
> + dev_err(data->dev, "failed to set operating mode\n");
> + return ret;
> + }
> +
> + return 0;
If not, these all lines can be transformed to just
return ret;
> +}
...
> +static irqreturn_t tlv493d_trigger_handler(int irq, void *ptr)
> +{
> + struct iio_poll_func *pf = ptr;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct tlv493d_data *data = iio_priv(indio_dev);
> +
> + struct {
> + s16 channels[3];
> + s16 temperature;
> + aligned_s64 timestamp;
> + } scan;
> +
No blank lines in the definition block.
> + s16 x, y, z, t;
> + int ret;
> +
> + ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
> + if (ret) {
> + dev_err(data->dev, "failed to read sensor data\n");
> + goto trig_out;
> + }
> +
> + scan.channels[0] = x;
> + scan.channels[1] = y;
> + scan.channels[2] = z;
> + scan.temperature = t;
> + iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan),
> + pf->timestamp);
> +trig_out:
Make sure you use a consistent pattern for labels.
out_trigger_notify:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
...
> + data->dev = dev;
> + data->client = client;
Choose one of them, the other can be derived.
...
> + return dev_err_probe(dev, ret, "failed to initialize\n");
Missing include for this API.
...
> +static const struct i2c_device_id tlv493d_id[] = {
> + { "tlv493d" },
> + { }
> +};
> +static const struct of_device_id tlv493d_of_match[] = {
> + { .compatible = "infineon,tlv493d-a1b6", },
Inner comma is redundant.
> + { }
> +};
Missing include for both of the ID tables.
...
> +static struct i2c_driver tlv493d_driver = {
> + .driver = {
> + .name = "tlv493d",
> + .of_match_table = tlv493d_of_match,
> + .pm = pm_ptr(&tlv493d_pm_ops),
Missing include for this macro I believe.
> + },
> + .probe = tlv493d_probe,
> + .id_table = tlv493d_id,
> +};
> +
Remove this blank line.
> +module_i2c_driver(tlv493d_driver);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists