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: <aO1WnvWaTDhU9PNo@lizhi-Precision-Tower-5810>
Date: Mon, 13 Oct 2025 15:44:30 -0400
From: Frank Li <Frank.li@....com>
To: Jonathan Cameron <jic23@...nel.org>
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 Sun, Oct 12, 2025 at 06:01:19PM +0100, Jonathan Cameron wrote:
> 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?

Copy from drivers/iio/magnetometer/mmc35240.c

>
> 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.

I can remove acpi part. Add it when someone really use it. I don't use this
now.

Frank
>
>
>
> > +	{ "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 =
>