RE: [EXT] Re: [PATCH v13 3/7] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator

[Lakshay Piplani]

> > --- /dev/null
> > +++ b/drivers/mfd/p3h2840.c
> > @@ -0,0 +1,126 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2025-2026 NXP
> 
> Nit: '\n' here.
> 

Sure, will remove the extra blank line.

> > + * P3H2X4X i3c hub and regulator device.
> 
> Those describe the children, not this device.
> 

Agreed, will update this to describe the MFD device.

> No Author: ?

Okay, will add Authors

> 
> > + */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/i3c/device.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/p3h2840.h>
> > +#include <linux/regmap.h>
> > +
> > +static const struct mfd_cell p3h2x4x_devs[] = {
> > +             MFD_CELL_NAME("p3h2x4x-regulator"),
> > +             MFD_CELL_NAME("p3h2x4x-i3c-hub"),
> 
> Too many tabs.

Will fix the indentation.

> 
> > +};
> > +
> > +static const struct regmap_config p3h2x4x_regmap_config = {
> > +     .reg_bits = P3H2X4X_REG_BITS,
> > +     .val_bits = P3H2X4X_VAL_BITS,
> > +     .max_register = 0xFF,
> > +};
> > +
> > +static int p3h2x4x_device_probe_i3c(struct i3c_device *i3cdev)
> 
> How many of these are comming down the pipe?
> 
> Might be worth expanding drivers/mfd/simple-mfd-i2c.c instead?

The P3H2x4x MFD supports both I2C and I3C transports. While the I2C path is conceptually similar to simple-mfd-i2c.c,
the I3C path additionally requires device matching/probing, devm_regmap_init_i3c(), PID/manufacturer validation, and 
retaining the parent struct i3c_device * for use by the hub child driver. As these requirements are specific to I3C 
and not handled by the existing simple-MFD helpers, I believe a dedicated MFD driver is the more suitable approach.

> 
> > +{
> > +     struct device *dev = i3cdev_to_dev(i3cdev);
> > +     struct i3c_device_info info;
> 
> devinfo is more consistent.

Agreed, will use devinfo

> 
> > +     struct p3h2x4x_dev *ddata;
> 
> Drop the _dev part.

Sure, will rename it.

> 
> > +     int ret;
> > +
> > +     i3c_device_get_info(i3cdev, &info);
> > +
> > +     if (I3C_PID_MANUF_ID(info.pid) != I3C_MANUF_ID_NXP)
> > +             return -ENODEV;
> > +
> > +     ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > +     if (!ddata)
> > +             return -ENOMEM;
> > +
> > +     i3cdev_set_drvdata(i3cdev, ddata);
> > +
> > +     ddata->regmap = devm_regmap_init_i3c(i3cdev,
> &p3h2x4x_regmap_config);
> > +     if (IS_ERR(ddata->regmap))
> > +             return dev_err_probe(dev, PTR_ERR(ddata->regmap),
> > +                                  "Failed to register HUB regmap\n");
> > +
> > +     // The hub child driver retrieves information from i3cdev
> 
> C++ comments?

Will replace it with a C-style comment.

> 
> > +     ddata->i3cdev = i3cdev;
> > +
> > +     ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> > +                                p3h2x4x_devs, ARRAY_SIZE(p3h2x4x_devs),
> > +                                NULL, 0, NULL);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "Failed to add sub
> > + devices\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static int p3h2x4x_device_probe_i2c(struct i2c_client *client) {
> > +     struct p3h2x4x_dev *ddata;
> > +     int ret;
> > +
> > +     ddata = devm_kzalloc(&client->dev, sizeof(*ddata), GFP_KERNEL);
> > +     if (!ddata)
> > +             return -ENOMEM;
> > +
> > +     i2c_set_clientdata(client, ddata);
> > +
> > +     ddata->regmap = devm_regmap_init_i2c(client,
> &p3h2x4x_regmap_config);
> > +     if (IS_ERR(ddata->regmap))
> > +             return dev_err_probe(&client->dev, PTR_ERR(ddata->regmap),
> > +                                  "Failed to register HUB regmap\n");
> > +
> > +     ddata->i3cdev = NULL;
> > +
> > +     ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO,
> > +                                p3h2x4x_devs, ARRAY_SIZE(p3h2x4x_devs),
> > +                                NULL, 0, NULL);
> > +     if (ret)
> > +             return dev_err_probe(&client->dev, ret, "Failed to add
> > + sub devices\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct i3c_device_id p3h2x4x_i3c_ids[] = {
> > +     I3C_CLASS(I3C_DCR_HUB, NULL),
> > +     { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(i3c, p3h2x4x_i3c_ids);
> > +
> > +static const struct i2c_device_id p3h2x4x_i2c_id_table[] = {
> > +     { "nxp-i3c-hub" },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, p3h2x4x_i2c_id_table);
> > +
> > +static const struct of_device_id p3h2x4x_i2c_of_match[] = {
> > +     { .compatible = "nxp,p3h2840", },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, p3h2x4x_i2c_of_match);
> > +
> > +static struct i3c_driver p3h2x4x_i3c = {
> > +     .driver = {
> > +             .name = "p3h2x4x-i3c",
> > +     },
> > +     .probe = p3h2x4x_device_probe_i3c,
> > +     .id_table = p3h2x4x_i3c_ids,
> > +};
> > +
> > +static struct i2c_driver p3h2x4x_i2c = {
> > +     .driver = {
> > +             .name = "p3h2x4x-i2c",
> > +             .of_match_table = p3h2x4x_i2c_of_match,
> > +     },
> > +     .probe =  p3h2x4x_device_probe_i2c,
> 
> Is there an accidental double space before 'p3h2x4x_device_probe_i2c'?

Yes, will fix it.

> 
> > +     .id_table = p3h2x4x_i2c_id_table, };
> > +module_i3c_i2c_driver(p3h2x4x_i3c, &p3h2x4x_i2c);
> > +
> > +MODULE_AUTHOR("Aman Kumar Pandey
> <aman.kumarpandey@xxxxxxx>");
> > +MODULE_AUTHOR("Vikash Bansal <vikash.bansal@xxxxxxx>");
> > +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@xxxxxxx>");
> > +MODULE_DESCRIPTION("NXP P3H2X4X I3C HUB multi function driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
> > index 971d53349b6f..6188082599dd 100644
> > --- a/include/linux/i3c/device.h
> > +++ b/include/linux/i3c/device.h
> > @@ -85,6 +85,7 @@ struct i3c_xfer {
> >   */
> >  enum i3c_dcr {
> >       I3C_DCR_GENERIC_DEVICE = 0,
> > +     I3C_DCR_HUB = 194,
> 
> What is this value?

194 is the Device Characteristics Register (DCR) value assigned to I3C Hub devices by the MIPI I3C specification.

> 
> >  };
> >
> >  #define I3C_PID_MANUF_ID(pid)                (((pid) & GENMASK_ULL(47,
> 33)) >> 33)
> > diff --git a/include/linux/mfd/p3h2840.h b/include/linux/mfd/p3h2840.h
> > new file mode 100644 index 000000000000..0f0d5b442bef
> > --- /dev/null
> > +++ b/include/linux/mfd/p3h2840.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright 2025-2026 NXP
> > + * This header file contains register definitions and bit masks for the
> P3H2X4X.
> 
> This comment is superfluous .

Will remove it.

> 
> > + */
> > +
> > +#ifndef _LINUX_MFD_P3H2840_H
> > +#define _LINUX_MFD_P3H2840_H
> > +
> > +#include <linux/types.h>
> > +
> > +/* Device Configuration Registers */
> > +#define P3H2X4X_DEV_REG_PROTECTION_CODE                              0x10
> > +#define P3H2X4X_REGISTERS_LOCK_CODE                          0x00
> > +#define P3H2X4X_REGISTERS_UNLOCK_CODE                                0x69
> > +#define P3H2X4X_CP1_REGISTERS_UNLOCK_CODE                    0x6a
> > +
> > +/* Reg config for Regmap */
> > +#define P3H2X4X_REG_BITS                                     8
> > +#define P3H2X4X_VAL_BITS                                     8
> 
> It's not common to define these values.

Agreed, will use the values directly in the regmap config.

> 
> > +#define I3C_MANUF_ID_NXP                                     0x011b
> > +
> > +struct p3h2x4x_dev {
> > +     struct i3c_device *i3cdev;
> > +     struct regmap *regmap;
> > +};
> > +#endif /* _LINUX_MFD_P3H2840_H */
> > --
> > 2.25.1
> >
> 
> --
> Lee Jones