Re: [PATCH v7 5/5] iio: magnetometer: Add mmc5633 sensor

Frank Li
2025-10-28T15:31:24Z

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