linux-kernel - Re: [PATCH v5 5/5] iio: magnetometer: Add mmc5633 sensor

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251012180119.312191fd@jic23-huawei>
Date: Sun, 12 Oct 2025 18:01:19 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Frank Li <Frank.Li@....com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>, Miquel Raynal
 <miquel.raynal@...tlin.com>, 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 v5 5/5] iio: magnetometer: Add mmc5633 sensor

On Tue, 07 Oct 2025 16:06:17 -0400
Frank Li <Frank.Li@....com> 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.
> 
> Co-developed-by: Carlos Song <carlos.song@....com>
> Signed-off-by: Carlos Song <carlos.song@....com>
> Co-developed-by: Adrian Fluturel <fluturel.adrian@...il.com>
> Signed-off-by: Adrian Fluturel <fluturel.adrian@...il.com>
> Signed-off-by: Frank Li <Frank.Li@....com>

Until now I missed the ACPI IDs that look to be almost certainly invalid.
See below.


> ---
> Change in V4
> - use { 1, 2000 }
> - Add _US for timeout
> - Use GEN_MASK for MMC5633_CTRL1_BW_MASK
> - Use { } for terminator.
> - remove !!
> - fix mix tab and space
> - add mmc5603 (merge https://lore.kernel.org/all/20251003000731.22927-1-fluturel.adrian@gmail.com/)
> - add tempature measure support
> 
> Change in v3
> - remove mmc5633_hw_set
> - make -> Make
> - change indention for mmc5633_samp_freq
> - use u8 arrary to handle dword data
> - get_unaligned_be16() to get raw data
> - add helper function to check if i3c support hdr
> - use read_avail() callback
> 
> change in v2
> - new patch

> diff --git a/drivers/iio/magnetometer/mmc5633.c b/drivers/iio/magnetometer/mmc5633.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..9b04cba6dbf633b7e0d136629a5aebffd072a68d
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mmc5633.c


> +static int mmc5633_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct mmc5633_data *data = iio_priv(indio_dev);
> +	int i, ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		i = mmc5633_get_samp_freq_index(data, val, val2);
> +		if (i < 0)
> +			return -EINVAL;
> +
> +		scoped_guard(mutex, &data->mutex) {
> +			ret = regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
> +						 MMC5633_CTRL1_BW_MASK,
> +						 FIELD_PREP(MMC5633_CTRL1_BW_MASK, i));
> +			if (ret)
> +				return ret;
> +		};
> +		return ret;

Trivial but to get here it must be 0 so make that apparent via
		return 0;
Then everyone can see the 'good' exit really easily.

> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static const struct i2c_device_id mmc5633_i2c_id[] = {
> +	{ "mmc5603" },
> +	{ "mmc5633" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, mmc5633_i2c_id);
> +
> +static const struct acpi_device_id mmc5633_acpi_match[] = {

Guess I missed these in previous versions. Sorry about that!

Where do these come from? 

MMC is not a registered PNP ID according to
https://uefi.org/PNP_ID_List?pnp_search=mmc

So are these in the wild? If they are provide the DSDT snippet
and a reference for where.  Also add a comment after the ID to provide
one device on which it is used.

ACPI IDs can take two valid forms. (1)   A 4 letter prefix that is
a registered ACPI ID.  e.g. HISI is the HiSilicon one I use for
the day job. And 4 digits that must be unique.  Typically each company
has a tracker for this and a process to get one assigned.
(2) A 3 letter PNP ID.  Similar otherwise.  Memsic has neither
unless issued very recently. 

So if these turn up on a device that uses ACPI either the manufacturer
of that should get MEMSIC to follow process to get an ID or use their
own.  It's common to find random sensors with IDs from device manufacturers
for this reason.  Here NXP could use their own ID for example.
They have a registered PNP ID which is the obvious NXP.  To do that
though you need to make sure you get a unique ID.



> +	{ "MMC5603", 0 },
> +	{ "mmc5633", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, mmc5633_acpi_match);
> +
> +static struct i2c_driver mmc5633_i2c_driver = {
> +	.driver = {
> +		.name = "mmc5633_i2c",
> +		.of_match_table = mmc5633_of_match,
> +		.acpi_match_table = mmc5633_acpi_match,
> +		.pm = pm_sleep_ptr(&mmc5633_pm_ops),
> +	},
> +	.probe = mmc5633_i2c_probe,
> +	.id_table =  mmc5633_i2c_id,

Really trivial but there is a extra space after the =