| 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: <aQoyzHrQB-egSPqR@smile.fi.intel.com>
Date: Tue, 4 Nov 2025 19:07:24 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Frank Li <Frank.Li@....com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
Miquel Raynal <miquel.raynal@...tlin.com>,
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-i3c@...ts.infradead.org,
linux-kernel@...r.kernel.org, imx@...ts.linux.dev,
linux-iio@...r.kernel.org, joshua.yeong@...rfivetech.com,
devicetree@...r.kernel.org, linux@...ck-us.net,
Carlos Song <carlos.song@....com>,
Adrian Fluturel <fluturel.adrian@...il.com>
Subject: Re: [PATCH v10 6/6] iio: magnetometer: Add mmc5633 sensor
On Tue, Nov 04, 2025 at 10:46:44AM -0500, Frank Li wrote:
> Add mmc5633 sensor basic support.
> - Support read 20 bits X/Y/Z magnetic.
> - Support I3C HDR mode to send start measurememt command.
> - Support I3C HDR mode to read all sensors data by one command.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@...el.com>
with some nit-picks below.
...
> +static int mmc5633_take_measurement(struct mmc5633_data *data, int address)
> +{
> + unsigned int reg_status;
> + int ret;
> + int val;
regmap API uses unsigned int for value field.
> + val = (address == MMC5633_TEMPERATURE) ? MMC5633_CTRL0_MEAS_T : MMC5633_CTRL0_MEAS_M;
> + ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, val);
> + if (ret < 0)
> + return ret;
> +
> + val = (address == MMC5633_TEMPERATURE) ?
> + MMC5633_STATUS1_MEAS_T_DONE_BIT : MMC5633_STATUS1_MEAS_M_DONE_BIT;
> + ret = regmap_read_poll_timeout(data->regmap, MMC5633_REG_STATUS1, reg_status,
> + reg_status & val,
> + 10 * USEC_PER_MSEC,
> + 100 * 10 * USEC_PER_MSEC);
> + if (ret) {
> + dev_err(regmap_get_device(data->regmap), "data not ready\n");
> + return ret;
> + }
> +
> + return 0;
> +}
...
> +static int mmc5633_read_measurement(struct mmc5633_data *data, int address, void *buf, size_t sz)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + u8 data_cmd[2], status[2];
> + int ret, val, ready;
Ditto.
> + if (mmc5633_is_support_hdr(data)) {
> + struct i3c_xfer xfers_wr_cmd[] = {
> + {
> + .cmd = 0x3b,
> + .len = 2,
> + .data.out = data_cmd,
> + }
> + };
> + struct i3c_xfer xfers_rd_sta_cmd[] = {
> + {
> + .cmd = 0x23 | BIT(7), /* RDSTA CMD */
> + .len = 2,
> + .data.in = status,
> + },
> + };
> + struct i3c_xfer xfers_rd_data_cmd[] = {
> + {
> + .cmd = 0x22 | BIT(7), /* RDLONG CMD */
> + .len = sz,
> + .data.in = buf,
> + },
> + };
Still do not see a justification for an array to use, but as I said I leave it
to Jonathan to decide.
> + data_cmd[0] = 0;
> + data_cmd[1] = (address == MMC5633_TEMPERATURE) ?
> + MMC5633_HDR_CTRL0_MEAS_T : MMC5633_HDR_CTRL0_MEAS_M;
> +
> + ret = i3c_device_do_xfers(data->i3cdev, xfers_wr_cmd,
> + ARRAY_SIZE(xfers_wr_cmd), I3C_HDR_DDR);
> + if (ret < 0)
> + return ret;
> +
> + ready = (address == MMC5633_TEMPERATURE) ?
> + MMC5633_STATUS1_MEAS_T_DONE_BIT : MMC5633_STATUS1_MEAS_M_DONE_BIT;
> + ret = read_poll_timeout(i3c_device_do_xfers, val,
> + val ||
> + status[0] & ready,
One line and probably you want to add parentheses for better understanding, not
everybody remembers the operator precedence by heart.
> + 10 * USEC_PER_MSEC,
> + 100 * 10 * USEC_PER_MSEC, 0,
> + data->i3cdev, xfers_rd_sta_cmd,
> + ARRAY_SIZE(xfers_rd_sta_cmd), I3C_HDR_DDR);
> + if (ret) {
> + dev_err(dev, "data not ready\n");
> + return ret;
> + }
> + if (val) {
> + dev_err(dev, "i3c transfer error\n");
> + return val;
> + }
> + return i3c_device_do_xfers(data->i3cdev, xfers_rd_data_cmd,
> + ARRAY_SIZE(xfers_rd_data_cmd), I3C_HDR_DDR);
> + }
> +
> + /* Fallback to use SDR/I2C mode */
> + ret = mmc5633_take_measurement(data, address);
> + if (ret < 0)
> + return ret;
> +
> + if (address == MMC5633_TEMPERATURE)
> + /*
> + * Put tempeature to last byte of buff to align HDR case.
> + * I3C will early terminate data read if previous data is not
> + * available.
> + */
> + return regmap_bulk_read(data->regmap, MMC5633_REG_TOUT, buf + sz - 1, 1);
> +
> + return regmap_bulk_read(data->regmap, MMC5633_REG_XOUT0, buf, sz);
> +}
...
> +static int mmc5633_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals,
> + int *type,
> + int *length,
> + long mask)
Other functions use different style, I believe it can occupy less LoCs.
...
> +static int mmc5633_i3c_probe(struct i3c_device *i3cdev)
> +{
> + struct device *dev = i3cdev_to_dev(i3cdev);
> + struct regmap *regmap;
> + char *name;
> + name = devm_kasprintf(dev, GFP_KERNEL, "mmc5633(%s)", dev_name(dev));
I agree on idea behind this, but I think () can be problematic for some shell
scripts, I would go with the simple mmc5633_%s or similar.
> + if (!name)
> + return -ENOMEM;
> +
> + regmap = devm_regmap_init_i3c(i3cdev, &mmc5633_regmap_config);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed to register i3c regmap\n");
> +
> + return mmc5633_common_probe(regmap, name, i3cdev);
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists