| 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: <aPnrc-Z5WVJorGr7@smile.fi.intel.com>
Date: Thu, 23 Oct 2025 11:46:43 +0300
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, Carlos Song <carlos.song@....com>,
Adrian Fluturel <fluturel.adrian@...il.com>
Subject: Re: [PATCH v6 5/5] iio: magnetometer: Add mmc5633 sensor
On Tue, Oct 14, 2025 at 12:40:04PM -0400, 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.
...
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/i3c/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/unaligned.h>
Follow IWYU principle, many are missing.
...
> +#define MMC5633_STATUS1_MEAS_T_DONE_BIT BIT(7)
> +#define MMC5633_STATUS1_MEAS_M_DONE_BIT BIT(6)
+ bits.h
...
> +#define MMC5633_WAIT_SET_RESET_US 1000
Perhaps (1 * MSEC_PER_USEC) ?
...
> +#define MMC5633_HDR_CTRL0_MEAS_M 0x01
> +#define MMC5633_HDR_CTRL0_MEAS_T 0x03
> +#define MMC5633_HDR_CTRL0_SET 0X05
Keep the style of the values consistent.
> +#define MMC5633_HDR_CTRL0_RESET 0x07
...
> +struct mmc5633_data {
> + struct device *dev;
> + struct i3c_device *i3cdev;
> + struct mutex mutex; /* protect to finish one whole measurement */
+ mutex.h
> + struct regmap *regmap;
> +};
...
> +static int mmc5633_get_samp_freq_index(struct mmc5633_data *data,
> + int val, int val2)
> +{
> + int i;
Why signed?
> + for (i = 0; i < ARRAY_SIZE(mmc5633_samp_freq); i++)
+ array_size.h
> + if (mmc5633_samp_freq[i].val == val &&
> + mmc5633_samp_freq[i].val2 == val2)
> + return i;
> + return -EINVAL;
> +}
...
> +static int mmc5633_init(struct mmc5633_data *data)
> +{
> + unsigned int reg_id, ret;
Have you ever compiled this?
Why ret is unsigned?
> + ret = regmap_read(data->regmap, MMC5633_REG_ID, ®_id);
> + if (ret < 0)
> + return dev_err_probe(data->dev, ret,
> + "Error reading product id\n");
+ dev_printk.h
> + /*
> + * Make sure we restore sensor characteristics, by doing
> + * a SET/RESET sequence, the axis polarity being naturally
> + * aligned after RESET.
> + */
> + ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_SET);
> + if (ret < 0)
> + return ret;
> + fsleep(MMC5633_WAIT_SET_RESET_US);
Do you have a reference to a datasheet? Perhaps add a small comment with it.
> + ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_RESET);
> + if (ret < 0)
> + return ret;
> +
> + /* set default sampling frequency */
> + return regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
> + MMC5633_CTRL1_BW_MASK,
> + FIELD_PREP(MMC5633_CTRL1_BW_MASK, 0));
> +}
...
> +static int mmc5633_take_measurement(struct mmc5633_data *data, int address)
> +{
> + unsigned int reg_status;
> + int ret;
> + int val;
> +
> + 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, 10000, 10000 * 100);
10 * MSEC_PER_USEC, 100 * 10 * MSEC_PER_USEC
> + if (ret) {
> + dev_err(data->dev, "data not ready\n");
> + return ret;
> + }
> +
> + return 0;
> +}
...
> +static int mmc5633_read_measurement(struct mmc5633_data *data, int address, void *buf, size_t sz)
> +{
> + u8 data_cmd[2], status[2];
> + int ret, val, ready;
> +
> + if (mmc5633_is_support_hdr(data)) {
> + struct i3c_xfer xfers_wr_cmd[] = {
> + {
> + .cmd = 0x3b,
> + .len = 2,
> + .data.out = data_cmd,
> + }
> + };
> +
Redundant blank line.
> + struct i3c_xfer xfers_rd_sta_cmd[] = {
> + {
> + .cmd = 0x23 | BIT(7), /* RDSTA CMD */
> + .len = 2,
> + .data.in = status,
> + },
> + };
> +
Ditto.
> + struct i3c_xfer xfers_rd_data_cmd[] = {
> + {
> + .cmd = 0x22 | BIT(7), /* RDLONG CMD */
> + .len = sz,
> + .data.in = buf,
> + },
> + };
> +
> + 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, 1, 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,
> + 10000, 10000 * 100, 0,
Use MSEC_PER_USEC as per above.
> + data->i3cdev, xfers_rd_sta_cmd, 1, I3C_HDR_DDR);
> +
Redundant blank line, and instead...
> + if (ret || val) {
> + dev_err(data->dev, "data not ready\n");
> + return ret ? ret : -EIO;
> + }
...split this conditional to the linear ones.
> + return i3c_device_do_xfers(data->i3cdev, xfers_rd_data_cmd, 1, 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_XOUT_L, buf, sz);
> +}
...
> +#define MMC5633_ALL_SIZE (3 * 3 + 1) /* each channel have 3 byte and TEMP */
have --> has and put comment on top of the definition.
...
> + char buf[MMC5633_ALL_SIZE];
> + unsigned int reg;
> + int ret, i;
Why is 'i' signed?
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + scoped_guard(mutex, &data->mutex) {
+ cleanup.h
> + ret = mmc5633_read_measurement(data, chan->address, buf, MMC5633_ALL_SIZE);
> + if (ret < 0)
> + return ret;
> + }
> +
> + ret = mmc5633_get_raw(data, chan->address, buf, val);
> + if (ret < 0)
> + return ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + if (chan->type == IIO_MAGN) {
> + *val = 0;
> + *val2 = 62500;
> + } else {
> + *val = 0;
> + *val2 = 800000000; /* 0.8C */
> + }
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_OFFSET:
> + if (chan->type == IIO_TEMP) {
> + *val = -75;
> + return IIO_VAL_INT;
> + }
> + return -EINVAL;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + scoped_guard(mutex, &data->mutex) {
> + ret = regmap_read(data->regmap, MMC5633_REG_CTRL1, ®);
> + if (ret < 0)
> + return ret;
> + }
> +
> + i = FIELD_GET(MMC5633_CTRL1_BW_MASK, reg);
+ bitfield.h
> + if (i >= ARRAY_SIZE(mmc5633_samp_freq))
> + return -EINVAL;
> +
> + *val = mmc5633_samp_freq[i].val;
> + *val2 = mmc5633_samp_freq[i].val2;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
...
> +static bool mmc5633_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case MMC5633_REG_CTRL0:
> + case MMC5633_REG_CTRL1:
> + return true;
> + default:
> + return false;
> + }
> +}
+ types.h
...
> +static int mmc5633_common_probe(struct device *dev, struct regmap *regmap,
> + char *name, struct i3c_device *i3cdev)
> +{
> + struct mmc5633_data *data;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
+ errno.h
> + dev_set_drvdata(dev, indio_dev);
+ device.h
> + data = iio_priv(indio_dev);
> +
> + data->regmap = regmap;
> + data->i3cdev = i3cdev;
> + data->dev = dev;
> +
> + ret = devm_mutex_init(dev, &data->mutex);
> + if (ret)
> + return ret;
> +
> + indio_dev->info = &mmc5633_info;
> + indio_dev->name = name;
> + indio_dev->channels = mmc5633_channels;
> + indio_dev->num_channels = ARRAY_SIZE(mmc5633_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = mmc5633_init(data);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "mmc5633 chip init failed\n");
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
...
> +static DEFINE_SIMPLE_DEV_PM_OPS(mmc5633_pm_ops, mmc5633_suspend,
> + mmc5633_resume);
One line (despite of being longer than 80).
...
> +static const struct of_device_id mmc5633_of_match[] = {
> + { .compatible = "memsic,mmc5603", },
> + { .compatible = "memsic,mmc5633", },
Remove inner commas.
> + { }
> +};
...
> +static int mmc5633_i3c_probe(struct i3c_device *i3cdev)
> +{
> + struct device *dev = i3cdev_to_dev(i3cdev);
> + struct regmap *regmap;
> +
> + regmap = devm_regmap_init_i3c(i3cdev, &mmc5633_regmap_config);
> + if (IS_ERR(regmap))
+ err.h
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed to register i3c regmap\n");
> +
> + return mmc5633_common_probe(dev, regmap, "mmc5633_i3c", i3cdev);
> +}
...
> +static struct i3c_driver mmc5633_i3c_driver = {
> + .driver = {
> + .name = "mmc5633_i3c",
> + },
> + .probe = mmc5633_i3c_probe,
> + .id_table = mmc5633_i3c_ids,
> +};
> +
Redundant blank line.
> +module_i3c_i2c_driver(mmc5633_i3c_driver, &mmc5633_i2c_driver)
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists