linux-kernel - Re: [PATCH v7 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: <aQDhv/r0l0oOjb9t@lizhi-Precision-Tower-5810>
Date: Tue, 28 Oct 2025 11:31:11 -0400
From: Frank Li <Frank.li@....com>
To: Andy Shevchenko <andriy.shevchenko@...el.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 v7 5/5] iio: magnetometer: Add mmc5633 sensor

On Tue, Oct 28, 2025 at 12:50:55PM +0200, Andy Shevchenko wrote:
> On Mon, Oct 27, 2025 at 04:08:33PM -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.
>
> ...
>
> + time.h // for time constants
>
> ...
>
> > +struct mmc5633_data {
> > +	struct device *dev;
> > +	struct i3c_device *i3cdev;
> > +	struct mutex mutex; /* protect to finish one whole measurement */
> > +	struct regmap *regmap;
>
> regmap has struct device, i3c_device presumable also, and here is struct
> device. Don't we have some overhead?

i3cdev is used for check it is i2c host or i3c host. If device connect to
i2c host, i3cdev will be NULL.

Only if connect to i3c host, driver can use i3c transfer api. The HDR
command is quite difference with SDR or I2C, which hard to wrap into regmap.

Anyway we need varible to indicate i3c or i2c. struct i3c_device *i3cdev
will be simple and needn't force convert struct device in regmap.

>
> > +};
>
> ...
>
...
>
> > +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;
>
> > +	dev_set_drvdata(dev, indio_dev);
>
> If you use regmap stored device this won't be needed. See below.
>
> > +	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 int mmc5633_suspend(struct device *dev)
> > +{
> > +	struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
>
> Than regmap will be derived directly from a device.

I have not got your idea. Can you point me a example?

Frank
>
> > +	regcache_cache_only(data->regmap, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mmc5633_resume(struct device *dev)
> > +{
>
> Ditto.
>
> > +	struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
> > +	int ret;
> > +
> > +	regcache_mark_dirty(data->regmap);
> > +	ret = regcache_sync_region(data->regmap, MMC5633_REG_CTRL0,
> > +				   MMC5633_REG_CTRL1);
> > +	if (ret < 0)
> > +		dev_err(dev, "Failed to restore control registers\n");
> > +
> > +	regcache_cache_only(data->regmap, false);
> > +
> > +	return 0;
> > +}
>
> ...
>
> > +	return mmc5633_common_probe(dev, regmap, client->name, NULL);
>
> dev can be derived from regmap.
>
> ...
>
>
> > +	return mmc5633_common_probe(dev, regmap, "mmc5633_i3c", i3cdev);
>
> Ditto.
>
> struct i3c_device doesn't have a name, does it?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>