add 88pm80x mfd driver

88PM800 and 88PM805 are two discrete chips used for power management. Hardware designer can use them together or only one of them according to requirement.

88pm80x_i2c.c provides common i2c driver handling for both 800 and 805, such as i2c_driver init, regmap init, read/write api etc.

88pm800_core.c handles specifically for 800, such as chip init, irq init/handle, mfd device register, including rtc, onkey, regulator( to be add later) etc. besides that, 800 has three i2c device, one regular i2c client, two other i2c dummy for gpadc and power purpose.

88pm805_core.c handles specifically for 805, such as chip init, irq init/handle, mfd device register, including codec, headset/mic detect etc.

the i2c operation of both 800 and 805 are via regmap.

Signed-off-by Qiao Zhou <[email protected]>

drivers/mfd/88pm80x-i2c.c new file +10 -0

+
+/**
+ * pm80x_reg_read: Read a single 88PM80x register.
+ *
+ * @map: regmap to read from.
+ * @reg: Register to read.
+ */
+int pm80x_reg_read(struct regmap *map, u32 reg)
+{
+	unsigned int val;

drivers/mfd/Kconfig +12 -0

index e129c82..96dd2d7 100644
@@ -20,6 +20,18 @@ config MFD_88PM860X
 	  select individual components like voltage regulators, RTC and
 	  battery-charger under the corresponding menus.
 
+config MFD_88PM80X
+	bool "Support Marvell 88PM800/88PM805"
+	depends on I2C=y && GENERIC_HARDIRQS
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	select MFD_CORE
+	help
+	  This supports for Marvell 88PM800/88PM805 Power Management IC.
+	  This includes the I2C driver and the core APIs _only_, you have to
+	  select individual components like voltage regulators, RTC and
+	  battery-charger under the corresponding menus.
+
 config MFD_SM501
 	tristate "Support for Silicon Motion SM501"
 	 ---help---

drivers/mfd/Makefile +0 -0

index 75f6ed6..dc2584e 100644

include/linux/mfd/88pm80x.h new file +20 -0

new file mode 100644
index 0000000..115348a
@@ -0,0 +1,521 @@
+#ifndef __LINUX_MFD_88PM80X_H
+#define __LINUX_MFD_88PM80X_H
+
+#define PM80X_VERSION_MASK		(0xFF)	/* 80X chip ID mask */
+
+enum {
+	/* Procida */
+	PM800_CHIP_A0  = 0x60,
+	PM800_CHIP_A1  = 0x61,
+	PM800_CHIP_B0  = 0x62,
+	PM800_CHIP_C0  = 0x63,
+	PM800_CHIP_END = PM800_CHIP_C0,
+
+	/* Make sure to update this to the last stepping */
+	PM8XXX_CHIP_END = PM800_CHIP_END
+};
+
+enum {
+	PM800_ID_INVALID,
+	PM800_ID_VIBRATOR,

include/linux/mfd/88pm80x.h new file +13 -0

+struct pm80x_subchip {
+	struct i2c_client *power_page;	/* chip client for power page */
+	struct i2c_client *gpadc_page;	/* chip client for gpadc page */
+	struct regmap *regmap_power;
+	struct regmap *regmap_gpadc;
+	unsigned short power_page_addr;	/* power page I2C address */
+	unsigned short gpadc_page_addr;	/* gpadc page I2C address */
+};
+
+struct pm80x_chip {
+	struct pm80x_subchip *subchip;
+	struct device *dev;
+	struct i2c_client *client;

include/linux/mfd/88pm80x.h new file +5 -0

+struct pm80x_rtc_pdata {
+	int		vrtc;
+	int		rtc_wakeup;
+};
+

drivers/mfd/88pm800-core.c line 371

Quoted 3 lines

drivers/mfd/88pm800-core.c new file +3 -0

+	ret = mfd_add_devices(chip->dev, 0, &onkey_devs[0],
+			      ARRAY_SIZE(onkey_devs), &onkey_resources[0],
+			      chip->irq_base);

According to what I discussed with Mark in the previous version, I think you need to pass 0 instead of chip->irq_base here, and transform the interrupt numbers using the domain in the client drivers.

drivers/mfd/88pm80x-i2c.c line 34

Quoted 6 lines

drivers/mfd/88pm80x-i2c.c new file +6 -0

+
+const struct i2c_device_id pm80x_id_table[] = {
+	{"88PM800", CHIP_PM800},
+	{"88PM805", CHIP_PM805},
+};
+MODULE_DEVICE_TABLE(i2c, pm80x_id_table);

Since these are separate modules now, you have to move the device table into the split files as well.

Quoted 10 lines

drivers/mfd/88pm80x-i2c.c new file +10 -0

+
+/**
+ * pm80x_reg_read: Read a single 88PM80x register.
+ *
+ * @map: regmap to read from.
+ * @reg: Register to read.
+ */
+int pm80x_reg_read(struct regmap *map, u32 reg)
+{
+	unsigned int val;

drivers/mfd/88pm80x-i2c.c line 54

Quoted 10 lines

drivers/mfd/88pm80x-i2c.c new file +10 -0

+	int ret;
+
+	ret = regmap_read(map, reg, &val);
+
+	if (ret < 0)
+		return ret;
+	else
+		return val;
+}
+EXPORT_SYMBOL_GPL(pm80x_reg_read);

In your introductory email you write

"Exported r/w API which requires regmap handle. as currently the pm800 chip has 3 i2c device, only passing a pm80x_chip info can't ensure r/w the register in correct i2c device."

Your first driver version had this, then you removed the functions after I asked you to, and now they are back, so I assume there is something I don't see yet. It looks like the function is just an unnecessary wrapper that is better open-coded in the caller. Can you explain again what the difference is?

Quoted 7 lines

+/**
+ * pm80x_bulk_read: Read multiple 88PM80x registers
+ *
+ * @map: regmap to read from
+ * @reg: First register
+ * @buf: Buffer to fill. The data will be returned big endian.
+ * @count: Number of registers

drivers/mfd/88pm80x-i2c.c line 67

Quoted 5 lines

+ */

drivers/mfd/88pm80x-i2c.c new file +4 -0

+int pm80x_bulk_read(struct regmap *map, u32 reg, u8 *buf, int count)
+{
+	return regmap_raw_read(map, reg, buf, count);
+}

Unused function? Either export this if you want to provide it as the general API, or drop the function.

Review

Quoted 6 lines

(quoted diff) +4 -0

+int __devinit pm80x_device_init(struct pm80x_chip *chip,
+				struct pm80x_platform_data *pdata)
 
+void __devexit pm80x_device_exit(struct pm80x_chip *chip)
 
+SIMPLE_DEV_PM_OPS(pm80x_pm_ops, pm80x_suspend, pm80x_resume);

I would think that these need to be exported as well, at least if you want the driver to be modular.

Quoted 28 lines

drivers/mfd/Kconfig +12 -0

index e129c82..96dd2d7 100644
@@ -20,6 +20,18 @@ config MFD_88PM860X
 	  select individual components like voltage regulators, RTC and
 	  battery-charger under the corresponding menus.
 
+config MFD_88PM80X
+	bool "Support Marvell 88PM800/88PM805"
+	depends on I2C=y && GENERIC_HARDIRQS
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	select MFD_CORE
+	help
+	  This supports for Marvell 88PM800/88PM805 Power Management IC.
+	  This includes the I2C driver and the core APIs _only_, you have to
+	  select individual components like voltage regulators, RTC and
+	  battery-charger under the corresponding menus.
+
 config MFD_SM501
 	tristate "Support for Silicon Motion SM501"
 	 ---help---

drivers/mfd/Makefile +0 -0

index 75f6ed6..dc2584e 100644

drivers/mfd/Makefile line 10

Quoted 8 lines

drivers/mfd/Makefile +2 -0

@@ -3,7 +3,9 @@
 #
 
 88pm860x-objs			:= 88pm860x-core.o 88pm860x-i2c.o
+88pm80x-objs			:= 88pm800-core.o 88pm805-core.o 88pm80x-i2c.o
 obj-$(CONFIG_MFD_88PM860X)	+= 88pm860x.o
+obj-$(CONFIG_MFD_88PM80X)	+= 88pm80x.o
 obj-$(CONFIG_MFD_SM501)		+= sm501.o
 obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o

I just noticed that it can currently only be builtin. Is there a strict

Quoted 1 line

requirement for that? If not, better make it a tristate option.

If you do that, aside from adding the exports mentioned above, you have to use three separate modules, as in

obj-$(CONFIG_MFD_88PM80X) += 88pm800-core.o 88pm805-core.o 88pm80x-i2c.o

or even

obj-$(CONFIG_MFD_88PM800) += 88pm800-core.o 88pm800-core.o
obj-$(CONFIG_MFD_88PM805) += 88pm800-core.o 88pm805-core.o

with the respective Kconfig change.

Quoted 34 lines

include/linux/mfd/88pm80x.h new file +28 -0

new file mode 100644
index 0000000..115348a
@@ -0,0 +1,521 @@
+#ifndef __LINUX_MFD_88PM80X_H
+#define __LINUX_MFD_88PM80X_H
+
+#define PM80X_VERSION_MASK		(0xFF)	/* 80X chip ID mask */
+
+enum {
+	/* Procida */
+	PM800_CHIP_A0  = 0x60,
+	PM800_CHIP_A1  = 0x61,
+	PM800_CHIP_B0  = 0x62,
+	PM800_CHIP_C0  = 0x63,
+	PM800_CHIP_END = PM800_CHIP_C0,
+
+	/* Make sure to update this to the last stepping */
+	PM8XXX_CHIP_END = PM800_CHIP_END
+};
+
+enum {
+	PM800_ID_INVALID,
+	PM800_ID_VIBRATOR,
+	PM800_ID_SOUND,
+	PM800_ID_MAX,
+};
+
+enum {
+	PM800_ID_BUCK1 = 0,
+	PM800_ID_BUCK2,
+	PM800_ID_BUCK3,

include/linux/mfd/88pm80x.h new file +1 -0

+	PM800_ID_BUCK4,

include/linux/mfd/88pm80x.h line 47

Quoted 1 line

include/linux/mfd/88pm80x.h new file +1 -0

+	PM800_ID_BUCK5,

I would still argue that the majority of the constants in this file should get moved into the driver .c file that uses them. Putting them into the header is better done only for interfaces between the driver parts, and for constants that are used by multiple drivers.

Quoted 19 lines

include/linux/mfd/88pm80x.h new file +19 -0

+struct pm80x_subchip {
+	struct i2c_client *power_page;	/* chip client for power page */
+	struct i2c_client *gpadc_page;	/* chip client for gpadc page */
+	struct regmap *regmap_power;
+	struct regmap *regmap_gpadc;
+	unsigned short power_page_addr;	/* power page I2C address */
+	unsigned short gpadc_page_addr;	/* gpadc page I2C address */
+};
+
+struct pm80x_chip {
+	struct pm80x_subchip *subchip;
+	struct device *dev;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct regmap_irq_chip *regmap_irq_chip;
+	struct regmap_irq_chip_data *irq_data;
+	unsigned char version;
+	int id;
+	int irq;

include/linux/mfd/88pm80x.h line 491

Quoted 4 lines

include/linux/mfd/88pm80x.h new file +4 -0

+	int irq_mode;
+	int irq_base;
+	unsigned int wu_flag;
+};

One thing I forgot to ask in the previous review although I had already noticed it then: What is the separation of pm80x_chip and pm80x_subchip used for?

Quoted 11 lines

include/linux/mfd/88pm80x.h new file +11 -0

+struct pm80x_rtc_pdata {
+	int		vrtc;
+	int		rtc_wakeup;
+};
+
+struct pm80x_platform_data {
+	struct pm80x_rtc_pdata *rtc;
+	unsigned short pm800_addr;
+	unsigned short pm805_addr;
+	unsigned short power_page_addr;	/* power page I2C address */
+	unsigned short gpadc_page_addr;	/* gpadc page I2C address */

include/linux/mfd/88pm80x.h line 507

Quoted 4 lines

include/linux/mfd/88pm80x.h new file +4 -0

+	int irq_mode;		/* Clear interrupt by read/write(0/1) */
+	int irq_base;		/* IRQ base of chip */
+	int batt_det;		/* enable/disable */
+};

You removed the callback here as I asked you to, which I think is a useful cleanup, but you can actually managed to convinced me that it would be ok to have it, so I don't mind if you want to put it back and use auxdata.

On the other hand, I think it probably makes sense to drop the irq_base member in this struct and rely on irq domains to allocate them dynamically as mentioned before.

Arnd

Quoted 8 lines

drivers/mfd/88pm800-core.c line 371

Quoted 3 lines

drivers/mfd/88pm800-core.c new file +3 -0

+	ret = mfd_add_devices(chip->dev, 0, &onkey_devs[0],
+			      ARRAY_SIZE(onkey_devs), &onkey_resources[0],
+			      chip->irq_base);

According to what I discussed with Mark in the previous version, I think you need to pass 0 instead of chip->irq_base here, and transform the interrupt numbers using the domain in the client drivers.

Quoted 9 lines

drivers/mfd/88pm80x-i2c.c line 34

Quoted 6 lines

drivers/mfd/88pm80x-i2c.c new file +6 -0

+
+const struct i2c_device_id pm80x_id_table[] = {
+	{"88PM800", CHIP_PM800},
+	{"88PM805", CHIP_PM805},
+};
+MODULE_DEVICE_TABLE(i2c, pm80x_id_table);

Since these are separate modules now, you have to move the device table into the split files as well.

Is it ok to export it in 88pm80x.h?

Quoted 27 lines

Quoted 10 lines

drivers/mfd/88pm80x-i2c.c new file +10 -0

+
+/**
+ * pm80x_reg_read: Read a single 88PM80x register.
+ *
+ * @map: regmap to read from.
+ * @reg: Register to read.
+ */
+int pm80x_reg_read(struct regmap *map, u32 reg)
+{
+	unsigned int val;

drivers/mfd/88pm80x-i2c.c line 54

Quoted 10 lines

drivers/mfd/88pm80x-i2c.c new file +10 -0

+	int ret;
+
+	ret = regmap_read(map, reg, &val);
+
+	if (ret < 0)
+		return ret;
+	else
+		return val;
+}
+EXPORT_SYMBOL_GPL(pm80x_reg_read);

In your introductory email you write

"Exported r/w API which requires regmap handle. as currently the pm800 chip has 3 i2c device, only passing a pm80x_chip info can't ensure r/w the register in correct i2c device."

Quoted 5 lines

Your first driver version had this, then you removed the functions after I asked you to, and now they are back, so I assume there is something I don't see yet. It looks like the function is just an unnecessary wrapper that is better open-coded in the caller. Can you explain again what the difference is?

After you suggest to change the r/w API so that caller doesn't care about it's via i2c or spi, it makes sense. However due to pm800 has 3 i2c devices, and it's hard to export such interface for pm800. Currently to add such interface via regmap handle, caller still doesn't care about the actual hw implement, also it's clear that all pm80x sub-driver or plat call the unified r/w API.

Quoted 7 lines

Quoted 7 lines

+/**
+ * pm80x_bulk_read: Read multiple 88PM80x registers
+ *
+ * @map: regmap to read from
+ * @reg: First register
+ * @buf: Buffer to fill. The data will be returned big endian.
+ * @count: Number of registers

Quoted 8 lines

drivers/mfd/88pm80x-i2c.c line 67

Quoted 5 lines

+ */

drivers/mfd/88pm80x-i2c.c new file +4 -0

+int pm80x_bulk_read(struct regmap *map, u32 reg, u8 *buf, int count)
+{
+	return regmap_raw_read(map, reg, buf, count);
+}

Unused function? Either export this if you want to provide it as the general API, or drop the function.

It's used by rtc driver.

Quoted 5 lines

I would still argue that the majority of the constants in this file should get moved into the driver .c file that uses them. Putting them into the header is better done only for interfaces between the driver parts, and for constants that are used by multiple drivers.

These registers still in this header file are either used by mfd core driver, regulator/rtc/onkey/codec, or used in platform initial setting.

Quoted 19 lines

Quoted 19 lines

include/linux/mfd/88pm80x.h new file +19 -0

+struct pm80x_subchip {
+	struct i2c_client *power_page;	/* chip client for power page */
+	struct i2c_client *gpadc_page;	/* chip client for gpadc page */
+	struct regmap *regmap_power;
+	struct regmap *regmap_gpadc;
+	unsigned short power_page_addr;	/* power page I2C address */
+	unsigned short gpadc_page_addr;	/* gpadc page I2C address */
+};
+
+struct pm80x_chip {
+	struct pm80x_subchip *subchip;
+	struct device *dev;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct regmap_irq_chip *regmap_irq_chip;
+	struct regmap_irq_chip_data *irq_data;
+	unsigned char version;
+	int id;
+	int irq;

Quoted 8 lines

include/linux/mfd/88pm80x.h line 491

Quoted 4 lines

include/linux/mfd/88pm80x.h new file +4 -0

+	int irq_mode;
+	int irq_base;
+	unsigned int wu_flag;
+};

One thing I forgot to ask in the previous review although I had already noticed it then: What is the separation of pm80x_chip and pm80x_subchip used for?

Pm80x_chip is common for both pm800 and pm805, while pm80x_subchip handles the other i2c devices as discussed before, only that these two i2c devices are not as much as pm800 and pm805 i2c device, and they are only for gpadc & power settings purpose. The subchip can be treated as some special feature/function for pm800.

Quoted 11 lines

Quoted 11 lines

include/linux/mfd/88pm80x.h new file +11 -0

+struct pm80x_rtc_pdata {
+	int		vrtc;
+	int		rtc_wakeup;
+};
+
+struct pm80x_platform_data {
+	struct pm80x_rtc_pdata *rtc;
+	unsigned short pm800_addr;
+	unsigned short pm805_addr;
+	unsigned short power_page_addr;	/* power page I2C address */
+	unsigned short gpadc_page_addr;	/* gpadc page I2C address */

Quoted 8 lines

include/linux/mfd/88pm80x.h line 507

Quoted 4 lines

include/linux/mfd/88pm80x.h new file +4 -0

+	int irq_mode;		/* Clear interrupt by read/write(0/1) */
+	int irq_base;		/* IRQ base of chip */
+	int batt_det;		/* enable/disable */
+};

You removed the callback here as I asked you to, which I think is a useful cleanup, but you can actually managed to convinced me that it would be ok to have it, so I don't mind if you want to put it back and use auxdata.

Actually it's convenient for us to put it back currently, and would use auxdata instead.

Quoted 3 lines

On the other hand, I think it probably makes sense to drop the irq_base member in this struct and rely on irq domains to allocate them dynamically as mentioned before.

Do you mean that both regmap_add_irq_chip and mfd_add_devices api pass -1 as the irq_base, so that system can dynamically allocate the irq_base for it?

Given the proper regmap_irq_chip & device resource, is there anything else needed? As I don't want to miss the exact meaning of " transform the interrupt numbers using the domain in the client drivers" you mentioned above.

Quoted 6 lines · Qiao Zhou

Quoted 3 lines

On the other hand, I think it probably makes sense to drop the irq_base member in this struct and rely on irq domains to allocate them dynamically as mentioned before.

Do you mean that both regmap_add_irq_chip and mfd_add_devices api pass -1 as the irq_base, so that system can dynamically allocate the irq_base for it?

regmap_add_irq_chip should pass -1, mfd_add_devices should pass 0. Mark can probably correct me if that's wrong.

Quoted 5 lines · Qiao Zhou

Quoted 4 lines

Quoted 4 lines

On the other hand, I think it probably makes sense to drop the irq_base member in this struct and rely on irq domains to allocate them dynamically as mentioned before.

Quoted 5 lines

Quoted 2 lines

Do you mean that both regmap_add_irq_chip and mfd_add_devices api
pass -1 as the irq_base, so that system can dynamically allocate the irq_base for it?

regmap_add_irq_chip should pass -1, mfd_add_devices should pass 0. Mark can probably correct me if that's wrong.

That's right.

Quoted 17 lines

Quoted 17 lines

drivers/mfd/88pm800-core.c line 371

Quoted 3 lines

drivers/mfd/88pm800-core.c new file +3 -0

+	ret = mfd_add_devices(chip->dev, 0, &onkey_devs[0],
+			      ARRAY_SIZE(onkey_devs), &onkey_resources[0],
+			      chip->irq_base);

According to what I discussed with Mark in the previous version, I think you need to pass 0 instead of chip->irq_base here, and transform the interrupt numbers using the domain in the client drivers.

drivers/mfd/88pm80x-i2c.c line 34

Quoted 6 lines

drivers/mfd/88pm80x-i2c.c new file +6 -0

+
+const struct i2c_device_id pm80x_id_table[] = {
+	{"88PM800", CHIP_PM800},
+	{"88PM805", CHIP_PM805},
+};
+MODULE_DEVICE_TABLE(i2c, pm80x_id_table);

Since these are separate modules now, you have to move the device table into the split files as well.

Quoted 1 line

Is it ok to export it in 88pm80x.h?

You mean putting the MODULE_DEVICE_TABLE() into 88pm80x.h? No, that would not work.

The correct way is to have two MODULE_DEVICE_TABLE statements, one per file. Actually the table only makes sense for loadable modules, so if you decide to keep the driver only built-in, it's best to just drop this table.

Quoted 33 lines

Quoted 32 lines

Quoted 10 lines

drivers/mfd/88pm80x-i2c.c new file +10 -0

+
+/**
+ * pm80x_reg_read: Read a single 88PM80x register.
+ *
+ * @map: regmap to read from.
+ * @reg: Register to read.
+ */
+int pm80x_reg_read(struct regmap *map, u32 reg)
+{
+	unsigned int val;

drivers/mfd/88pm80x-i2c.c line 54

Quoted 10 lines

drivers/mfd/88pm80x-i2c.c new file +10 -0

+	int ret;
+
+	ret = regmap_read(map, reg, &val);
+
+	if (ret < 0)
+		return ret;
+	else
+		return val;
+}
+EXPORT_SYMBOL_GPL(pm80x_reg_read);

In your introductory email you write

"Exported r/w API which requires regmap handle. as currently the pm800 chip has 3 i2c device, only passing a pm80x_chip info can't ensure r/w the register in correct i2c device."

Your first driver version had this, then you removed the functions after I asked you to, and now they are back, so I assume there is something I don't see yet. It looks like the function is just an unnecessary wrapper that is better open-coded in the caller. Can you explain again what the difference is?

Quoted 5 lines

After you suggest to change the r/w API so that caller doesn't care about it's via i2c or spi, it makes sense. However due to pm800 has 3 i2c devices, and it's hard to export such interface for pm800. Currently to add such interface via regmap handle, caller still doesn't care about the actual hw implement, also it's clear that all pm80x sub-driver or plat call the unified r/w API.

But there is nothing unified about the function above, it just calls regmap_read(), so the caller already has access to the regmap pointer.

Quoted 15 lines

Quoted 15 lines

drivers/mfd/88pm80x-i2c.c line 67

Quoted 12 lines

drivers/mfd/88pm80x-i2c.c new file +12 -0

+/**
+ * pm80x_bulk_read: Read multiple 88PM80x registers
+ *
+ * @map: regmap to read from
+ * @reg: First register
+ * @buf: Buffer to fill.  The data will be returned big endian.
+ * @count: Number of registers
+ */
+int pm80x_bulk_read(struct regmap *map, u32 reg, u8 *buf, int count)
+{
+	return regmap_raw_read(map, reg, buf, count);
+}

Unused function? Either export this if you want to provide it as the general API, or drop the function.

Quoted 1 line

It's used by rtc driver.

Then it needs to be exported, so the rtc driver can be a module.

Quoted 6 lines

Quoted 4 lines

On the other hand, I think it probably makes sense to drop the irq_base member in this struct and rely on irq domains to allocate them dynamically as mentioned before.

Do you mean that both regmap_add_irq_chip and mfd_add_devices api pass -1 as the irq_base, so that system can dynamically allocate the irq_base for it?

regmap_add_irq_chip should pass -1, mfd_add_devices should pass 0. Mark can probably correct me if that's wrong.

Quoted 3 lines

Given the proper regmap_irq_chip & device resource, is there anything else needed? As I don't want to miss the exact meaning of " transform the interrupt numbers using the domain in the client drivers" you mentioned above.

The drivers using the numbers need to call regmap_irq_get_virq() to get the real interrupt number. See include/linux/mfd/wm8994/core.h for an example.

Arnd