mbox series

[v2,00/15] Add Mediatek MT6370 PMIC support

Message ID 20220613111146.25221-1-peterwu.pub@gmail.com
Headers show
Series Add Mediatek MT6370 PMIC support | expand

Message

ChiaEn Wu June 13, 2022, 11:11 a.m. UTC
From: ChiaEn Wu <chiaen_wu@richtek.com>

This patch series add Mediatek MT6370 PMIC support. The MT6370 is a
highly-integrated smart power management IC, which includes a single
cell Li-Ion/Li-Polymer switching battery charger, a USB
Type-C & Power Delivery (PD) controller, dual Flash LED current sources,
a RGB LED driver, a backlight WLED driver, a display bias driver and a
general LDO for portable devices.

Among with this we took some changes and refined the device tree files to
comply with DT specifications.

Thank you,
ChiaEn Wu

---
Changes in v2:
- In patch 01/15:
		- Add "unevaluatedProperties: false".
		- Delete "DT bindings".
		- Refine the description to fit in 80 columns.
		- Skip the connector description.
- In patch 02/15:
		- Refine items description of interrupt-name
		- Rename "usb-otg-vbus" to "usb-otg-vbus-regulator"
		- Add constraint properties for ADC
- In patch 03/15:
		- Skip not useful description of "^(multi-)?led@[0-3]$"
		  and reg.
		- Due to the dependency, remove the mention of mfd
		  document directory.
		- Delete Soft-start property. In design aspect, we think
		  soft-restart should always be enabled, our new chip
		  has deleted the related setting register , also, we don’t
		  allow user adjust this parameter in this chip.
		- Refine the commit message.
- In patch 04/15:
		- Skip not useful description of "^led@[0-1]$" and reg.
		- Add apace after '#'.
		- Refine the commit message.
- In patch 05/15:
		- Remove "binding documentation" in subject title
		- Refine description of mt6370 backlight binding
		  document
		- Refine properties name(bled-pwm-hys-input-bit,
		  bled-ovp-microvolt, bled-ocp-microamp) and their
		  description
- In patch 06/15:
		- Refine ADC and Regulator descriptions
		- Refine include header usage in example
		- Refine node name to generic node name("pmic@34")
		- Refine led example indentation
		- Refine license of mediatek,mt6370_adc.h
		- Rename the dts example from IRQ define to number.
		- Remove mediatek,mt6370.h
- In patch 07/15:
		- Add ABI documentation for mt6370 non-standard ADC
		  sysfs interfaces.
- In patch 08/15:
		- Add all IRQ define into mt6370.c.
		- Refine include header usage
- In patch 09/15:
		- No changes.
- In patch 10/15:
		- Use 'gpiod_get_from_of_node' to replace
		  'fwnode_gpiod_get_index'.
- In patch 11/15:
		- Refine Kconfig mt6370 help text
		- Refine mask&shift to FIELD_PREP()
		- Refine mutex lock name ("lock" -> "adc_lock")
		- Refine mt6370_adc_read_scale()
		- Refine mt6370_adc_read_offset()
		- Refine mt6370_channel_labels[] by using enum to index
		  chan spec
		- Refine MT6370_ADC_CHAN()
		- Refine indio_dev->name
		- Remove useless include header files
- In patch 12/15:
		- Refine mt6370_chg_otg_rdesc.of_match
		  ("mt6370,otg-vbus" -> "usb-otg-vbus-regulator") to match
		  DT binding
- In patch 13/15:
		- Refine Kconfig description.
		- Remove include "linux/of.h" and use
		  "linux/mod_devicetable.h".
		- Place a comma for the last element of the const
		  unsigned int array.
		- Add a comment line for the mutex 'lock'.
		- In probe function, use 'dev_err_probe' in some
		  judgement to reduce the LOC.
		- Refine include header usage.
		  BIT/GENMASK -> linux/bits.h
		  FIELD_GET -> linux/bitfield.h
- In patch 14/15:
		- Add blank line.
		- Replace container_of() with to_mt6370_led() .
		- Refine description of ramping.
		- Refine the mt6370_init_common_properties function.
		- Refine the probe return.
- In patch 15/15:
		- Refine MT6370 help text in Kconfig
		- Refine DT Parse function
		- Remove useless enum
		- Add comment for 6372 backward compatible in
		  bl_update_status() and
		  check_vendor_info()
		- Using dev_err_probe(); insteads dev_err()&return; in
		  the probe()

Alice Chen (2):
  dt-bindings: leds: Add Mediatek MT6370 flashlight
  leds: flashlight: mt6370: Add Mediatek MT6370 flashlight support

ChiYuan Huang (8):
  dt-bindings: usb: Add Mediatek MT6370 TCPC
  dt-bindings: leds: mt6370: Add Mediatek mt6370 current sink type LED
    indicator
  dt-bindings: backlight: Add Mediatek MT6370 backlight
  dt-bindings: mfd: Add Mediatek MT6370
  mfd: mt6370: Add Mediatek MT6370 support
  usb: typec: tcpci_mt6370: Add Mediatek MT6370 tcpci driver
  regulator: mt6370: Add mt6370 DisplayBias and VibLDO support
  leds: mt6370: Add Mediatek MT6370 current sink type LED Indicator
    support

ChiaEn Wu (5):
  dt-bindings: power: supply: Add Mediatek MT6370 Charger
  Documentation: ABI: testing: mt6370: Add ADC sysfs guideline
  iio: adc: mt6370: Add Mediatek MT6370 support
  power: supply: mt6370: Add Mediatek MT6370 charger driver
  video: backlight: mt6370: Add Mediatek MT6370 support

 .../ABI/testing/sysfs-bus-iio-adc-mt6370      |   36 +
 .../backlight/mediatek,mt6370-backlight.yaml  |  107 ++
 .../leds/mediatek,mt6370-flashlight.yaml      |   44 +
 .../leds/mediatek,mt6370-indicator.yaml       |   48 +
 .../bindings/mfd/mediatek,mt6370.yaml         |  279 ++++
 .../power/supply/mediatek,mt6370-charger.yaml |   60 +
 .../bindings/usb/mediatek,mt6370-tcpc.yaml    |   36 +
 drivers/iio/adc/Kconfig                       |    9 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/mt6370-adc.c                  |  262 ++++
 drivers/leds/Kconfig                          |   11 +
 drivers/leds/Makefile                         |    1 +
 drivers/leds/flash/Kconfig                    |    9 +
 drivers/leds/flash/Makefile                   |    1 +
 drivers/leds/flash/leds-mt6370-flash.c        |  657 ++++++++++
 drivers/leds/leds-mt6370.c                    |  989 ++++++++++++++
 drivers/mfd/Kconfig                           |   13 +
 drivers/mfd/Makefile                          |    1 +
 drivers/mfd/mt6370.c                          |  349 +++++
 drivers/power/supply/Kconfig                  |   11 +
 drivers/power/supply/Makefile                 |    1 +
 drivers/power/supply/mt6370-charger.c         | 1132 +++++++++++++++++
 drivers/regulator/Kconfig                     |    8 +
 drivers/regulator/Makefile                    |    1 +
 drivers/regulator/mt6370-regulator.c          |  388 ++++++
 drivers/usb/typec/tcpm/Kconfig                |    8 +
 drivers/usb/typec/tcpm/Makefile               |    1 +
 drivers/usb/typec/tcpm/tcpci_mt6370.c         |  212 +++
 drivers/video/backlight/Kconfig               |    9 +
 drivers/video/backlight/Makefile              |    1 +
 drivers/video/backlight/mt6370-backlight.c    |  339 +++++
 .../dt-bindings/iio/adc/mediatek,mt6370_adc.h |   18 +
 32 files changed, 5042 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-mt6370
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/mediatek,mt6370-backlight.yaml
 create mode 100644 Documentation/devicetree/bindings/leds/mediatek,mt6370-flashlight.yaml
 create mode 100644 Documentation/devicetree/bindings/leds/mediatek,mt6370-indicator.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/mediatek,mt6370.yaml
 create mode 100644 Documentation/devicetree/bindings/power/supply/mediatek,mt6370-charger.yaml
 create mode 100644 Documentation/devicetree/bindings/usb/mediatek,mt6370-tcpc.yaml
 create mode 100644 drivers/iio/adc/mt6370-adc.c
 create mode 100644 drivers/leds/flash/leds-mt6370-flash.c
 create mode 100644 drivers/leds/leds-mt6370.c
 create mode 100644 drivers/mfd/mt6370.c
 create mode 100644 drivers/power/supply/mt6370-charger.c
 create mode 100644 drivers/regulator/mt6370-regulator.c
 create mode 100644 drivers/usb/typec/tcpm/tcpci_mt6370.c
 create mode 100644 drivers/video/backlight/mt6370-backlight.c
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6370_adc.h

Comments

Daniel Thompson June 13, 2022, 5:08 p.m. UTC | #1
On Mon, Jun 13, 2022 at 07:11:46PM +0800, ChiaEn Wu wrote:
> +static int mt6370_init_backlight_properties(struct mt6370_priv *priv,
> +					    struct backlight_properties *props)

Most of the changes in this version looks good... but it looks the new
code in this function has a number of problems. See below...


> +{
> +	struct device *dev = priv->dev;
> +	u8 prop_val;
> +	u32 brightness;
> +	unsigned int mask, val;
> +	int ret;
> +
> +	/* Vendor optional properties
> +	 * if property not exist, keep value in default.
> +	 */

That's not the right strategy for booleans. Not existing means false
(e.g. flags should actively be unset).


> +	if (device_property_read_bool(dev, "mediatek,bled-pwm-enable")) {
> +		ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM,
> +					 MT6370_BL_PWM_EN_MASK,
> +					 MT6370_BL_PWM_EN_MASK);
> +		if (ret)
> +			return ret;
> +	}

As above comment... all of the boolean properties are now being read
incorrectly.


> +
> +	if (device_property_read_bool(dev, "mediatek,bled-pwm-hys-enable")) {
> +		ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM,
> +					 MT6370_BL_PWM_HYS_EN_MASK,
> +					 MT6370_BL_PWM_HYS_EN_MASK);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = device_property_read_u8(dev, "mediatek,bled-pwm-hys-input-bit",
> +				      &prop_val);
> +	if (!ret) {
> +		val = min_t(u8, prop_val, 3)
> +		      << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1);
> +		ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM,
> +					 MT6370_BL_PWM_HYS_SEL_MASK, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = device_property_read_u8(dev, "mediatek,bled-ovp-microvolt",
> +				      &prop_val);
> +	if (!ret) {
> +		val = min_t(u8, prop_val, 3)
> +		      << (ffs(MT6370_BL_OVP_SEL_MASK) - 1);

This has been renamed but still seems to the using 0, 1, 2, 3 rather
than an actual value in microvolts.


> +		ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL,
> +					 MT6370_BL_OVP_SEL_MASK, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (device_property_read_bool(dev, "mediatek,bled-ovp-shutdown")) {
> +		ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL,
> +					 MT6370_BL_OVP_EN_MASK,
> +					 MT6370_BL_OVP_EN_MASK);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = device_property_read_u8(dev, "mediatek,bled-ocp-microamp",
> +				      &prop_val);
> +	if (!ret) {
> +		val = min_t(u8, prop_val, 3)
> +		      << (ffs(MT6370_BL_OC_SEL_MASK) - 1);

Likewise, should this be accepting a value in microamps?


> +		ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL,
> +					 MT6370_BL_OC_SEL_MASK, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (device_property_read_bool(dev, "mediatek,bled-ocp-shutdown")) {
> +		ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL,
> +					 MT6370_BL_OC_EN_MASK,
> +					 MT6370_BL_OC_EN_MASK);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Common properties */
> +	ret = device_property_read_u32(dev, "max-brightness", &brightness);
> +	if (ret)
> +		brightness = MT6370_BL_MAX_BRIGHTNESS;
> +
> +	props->max_brightness = min_t(u32, brightness,
> +				      MT6370_BL_MAX_BRIGHTNESS);
> +
> +	ret = device_property_read_u32(dev, "default-brightness", &brightness);
> +	if (ret)
> +		brightness = props->max_brightness;
> +
> +	props->brightness = min_t(u32, brightness, props->max_brightness);
> +
> +
> +	ret = device_property_read_u8(dev, "mediatek,bled-channel-use",
> +				      &prop_val);
> +	if (ret) {
> +		dev_err(dev, "mediatek,bled-channel-use DT property missing\n");
> +		return ret;
> +	}
> +
> +	if (!prop_val || prop_val > MT6370_BL_MAX_CH) {
> +		dev_err(dev, "No channel specified (ch_val:%d)\n", prop_val);

Error string has not been updated to match condition that triggers it.


> +		return -EINVAL;
> +	}


Daniel.
Randy Dunlap June 13, 2022, 8:14 p.m. UTC | #2
On 6/13/22 04:11, ChiaEn Wu wrote:
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3b59456f5545..d9a7524a3e0e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -937,6 +937,19 @@ config MFD_MT6360
>  	  PMIC part includes 2-channel BUCKs and 2-channel LDOs
>  	  LDO part includes 4-channel LDOs
>  
> +config MFD_MT6370
> +	tristate "Mediatek MT6370 SubPMIC"
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	depends on I2C
> +	help
> +	  Say Y here to enable MT6370 SubPMIC functional support.
> +	  It integrate single cell battery charger with adc monitoring, RGB

	     integrates                                 ADC  (?)

> +	  LEDs, dual channel flashlight, WLED backlight driver, display bias
> +	  voltage supply, one general purpose LDO, and cc logic

	                                               CC   (?)
What is CC?

> +	  controller with USBPD commmunication capable.

	                                       capability.
Randy Dunlap June 13, 2022, 8:20 p.m. UTC | #3
Hi--

On 6/13/22 04:11, ChiaEn Wu wrote:
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6090e647daee..61e6ec416cb0 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -244,6 +244,17 @@ config LEDS_MT6323
>  	  This option enables support for on-chip LED drivers found on
>  	  Mediatek MT6323 PMIC.
>  
> +config LEDS_MT6370_RGB
> +	tristate "LED Support for Mediatek MT6370 PMIC"
> +	depends on LEDS_CLASS
> +	depends on MFD_MT6370
> +	select LINEAR_RANGE
> +	help
> +	  Say Y here to enable support for MT6370_RGB LED device.
> +	  In MT6370, there're four channel current-sink LED driver that support

Please spell out "there are" instead of using an unusual contraction.
Also:
	                                                    drivers that support

> +	  hardware pattern for reg, pwm, breath mode. Isink4 channel

	                            PWM,
What is "reg"?

> +	  can also be used as a CHG_VIN power good indicator.
Randy Dunlap June 13, 2022, 8:21 p.m. UTC | #4
On 6/13/22 04:11, ChiaEn Wu wrote:
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index a003e02e13ce..ec1589ad88bb 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -268,6 +268,15 @@ config BACKLIGHT_MAX8925
>  	  If you have a LCD backlight connected to the WLED output of MAX8925
>  	  WLED output, say Y here to enable this driver.
>  
> +config BACKLIGHT_MT6370
> +	tristate "Mediatek MT6370 Backlight Driver"
> +	depends on MFD_MT6370
> +	help
> +	  This enables support for Mediatek MT6370 Backlight driver.
> +	  It's commonly used to drive the display WLED. There're 4 channels

	                                                There are 4 channels

> +	  inisde, and each channel supports up to 30mA of current capability

	  inside,

> +	  with 2048 current steps in exponential or linear mapping curves.
Lee Jones June 15, 2022, 10:49 p.m. UTC | #5
On Mon, 13 Jun 2022, ChiaEn Wu wrote:

> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add Mediatek MT6370 MFD support.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  drivers/mfd/Kconfig  |  13 ++
>  drivers/mfd/Makefile |   1 +
>  drivers/mfd/mt6370.c | 349 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 363 insertions(+)
>  create mode 100644 drivers/mfd/mt6370.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3b59456f5545..d9a7524a3e0e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -937,6 +937,19 @@ config MFD_MT6360
>  	  PMIC part includes 2-channel BUCKs and 2-channel LDOs
>  	  LDO part includes 4-channel LDOs
>  
> +config MFD_MT6370
> +	tristate "Mediatek MT6370 SubPMIC"
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	depends on I2C
> +	help
> +	  Say Y here to enable MT6370 SubPMIC functional support.
> +	  It integrate single cell battery charger with adc monitoring, RGB

s/integrates/consists of a/

"ADC"

> +	  LEDs, dual channel flashlight, WLED backlight driver, display bias

> +	  voltage supply, one general purpose LDO, and cc logic
> +	  controller with USBPD commmunication capable.

The last part makes no sense - "and is USBPD"?

>  config MFD_MT6397
>  	tristate "MediaTek MT6397 PMIC Support"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 858cacf659d6..62b27125420e 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -242,6 +242,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)	+= intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)	+= intel_soc_pmic_chtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
>  obj-$(CONFIG_MFD_MT6360)	+= mt6360-core.o
> +obj-$(CONFIG_MFD_MT6370)	+= mt6370.o
>  mt6397-objs			:= mt6397-core.o mt6397-irq.o mt6358-irq.o
>  obj-$(CONFIG_MFD_MT6397)	+= mt6397.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)	+= intel_soc_pmic_mrfld.o
> diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c
> new file mode 100644
> index 000000000000..6af9f73c9c0c
> --- /dev/null
> +++ b/drivers/mfd/mt6370.c
> @@ -0,0 +1,349 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bits.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +enum {
> +	MT6370_USBC_I2C = 0,
> +	MT6370_PMU_I2C,
> +	MT6370_MAX_I2C
> +};
> +
> +#define MT6370_REG_DEV_INFO	0x100
> +#define MT6370_REG_CHG_IRQ1	0x1C0
> +#define MT6370_REG_CHG_MASK1	0x1E0
> +
> +#define MT6370_VENID_MASK	GENMASK(7, 4)
> +
> +#define MT6370_NUM_IRQREGS	16
> +#define MT6370_USBC_I2CADDR	0x4E
> +#define MT6370_REG_ADDRLEN	2
> +#define MT6370_REG_MAXADDR	0x1FF
> +
> +/* IRQ definitions */
> +#define MT6370_IRQ_DIRCHGON		0
> +#define MT6370_IRQ_CHG_TREG		4
> +#define MT6370_IRQ_CHG_AICR		5
> +#define MT6370_IRQ_CHG_MIVR		6
> +#define MT6370_IRQ_PWR_RDY		7
> +#define MT6370_IRQ_FL_CHG_VINOVP	11
> +#define MT6370_IRQ_CHG_VSYSUV		12
> +#define MT6370_IRQ_CHG_VSYSOV		13
> +#define MT6370_IRQ_CHG_VBATOV		14
> +#define MT6370_IRQ_CHG_VINOVPCHG	15
> +#define MT6370_IRQ_TS_BAT_COLD		20
> +#define MT6370_IRQ_TS_BAT_COOL		21
> +#define MT6370_IRQ_TS_BAT_WARM		22
> +#define MT6370_IRQ_TS_BAT_HOT		23
> +#define MT6370_IRQ_TS_STATC		24
> +#define MT6370_IRQ_CHG_FAULT		25
> +#define MT6370_IRQ_CHG_STATC		26
> +#define MT6370_IRQ_CHG_TMR		27
> +#define MT6370_IRQ_CHG_BATABS		28
> +#define MT6370_IRQ_CHG_ADPBAD		29
> +#define MT6370_IRQ_CHG_RVP		30
> +#define MT6370_IRQ_TSHUTDOWN		31
> +#define MT6370_IRQ_CHG_IINMEAS		32
> +#define MT6370_IRQ_CHG_ICCMEAS		33
> +#define MT6370_IRQ_CHGDET_DONE		34
> +#define MT6370_IRQ_WDTMR		35
> +#define MT6370_IRQ_SSFINISH		36
> +#define MT6370_IRQ_CHG_RECHG		37
> +#define MT6370_IRQ_CHG_TERM		38
> +#define MT6370_IRQ_CHG_IEOC		39
> +#define MT6370_IRQ_ADC_DONE		40
> +#define MT6370_IRQ_PUMPX_DONE		41
> +#define MT6370_IRQ_BST_BATUV		45
> +#define MT6370_IRQ_BST_MIDOV		46
> +#define MT6370_IRQ_BST_OLP		47
> +#define MT6370_IRQ_ATTACH		48
> +#define MT6370_IRQ_DETACH		49
> +#define MT6370_IRQ_HVDCP_STPDONE	51
> +#define MT6370_IRQ_HVDCP_VBUSDET_DONE	52
> +#define MT6370_IRQ_HVDCP_DET		53
> +#define MT6370_IRQ_CHGDET		54
> +#define MT6370_IRQ_DCDT			55
> +#define MT6370_IRQ_DIRCHG_VGOK		59
> +#define MT6370_IRQ_DIRCHG_WDTMR		60
> +#define MT6370_IRQ_DIRCHG_UC		61
> +#define MT6370_IRQ_DIRCHG_OC		62
> +#define MT6370_IRQ_DIRCHG_OV		63
> +#define MT6370_IRQ_OVPCTRL_SWON		67
> +#define MT6370_IRQ_OVPCTRL_UVP_D	68
> +#define MT6370_IRQ_OVPCTRL_UVP		69
> +#define MT6370_IRQ_OVPCTRL_OVP_D	70
> +#define MT6370_IRQ_OVPCTRL_OVP		71
> +#define MT6370_IRQ_FLED_STRBPIN		72
> +#define MT6370_IRQ_FLED_TORPIN		73
> +#define MT6370_IRQ_FLED_TX		74
> +#define MT6370_IRQ_FLED_LVF		75
> +#define MT6370_IRQ_FLED2_SHORT		78
> +#define MT6370_IRQ_FLED1_SHORT		79
> +#define MT6370_IRQ_FLED2_STRB		80
> +#define MT6370_IRQ_FLED1_STRB		81
> +#define mT6370_IRQ_FLED2_STRB_TO	82
> +#define MT6370_IRQ_FLED1_STRB_TO	83
> +#define MT6370_IRQ_FLED2_TOR		84
> +#define MT6370_IRQ_FLED1_TOR		85
> +#define MT6370_IRQ_OTP			93
> +#define MT6370_IRQ_VDDA_OVP		94
> +#define MT6370_IRQ_VDDA_UV		95
> +#define MT6370_IRQ_LDO_OC		103
> +#define MT6370_IRQ_BLED_OCP		118
> +#define MT6370_IRQ_BLED_OVP		119
> +#define MT6370_IRQ_DSV_VNEG_OCP		123
> +#define MT6370_IRQ_DSV_VPOS_OCP		124
> +#define MT6370_IRQ_DSV_BST_OCP		125
> +#define MT6370_IRQ_DSV_VNEG_SCP		126
> +#define MT6370_IRQ_DSV_VPOS_SCP		127
> +
> +struct mt6370_info {
> +	struct i2c_client *i2c[MT6370_MAX_I2C];
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct regmap_irq_chip_data *irq_data;
> +};

Can we shove all of the above into a header file?

> +static const struct regmap_irq mt6370_irqs[] = {
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHGON, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TREG, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_AICR, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_MIVR, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_PWR_RDY, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FL_CHG_VINOVP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VSYSUV, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VSYSOV, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VBATOV, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VINOVPCHG, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_COLD, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_COOL, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_WARM, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_HOT, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_STATC, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_FAULT, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_STATC, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TMR, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_BATABS, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_ADPBAD, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_RVP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_TSHUTDOWN, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_IINMEAS, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_ICCMEAS, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHGDET_DONE, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_WDTMR, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_SSFINISH, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_RECHG, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TERM, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_IEOC, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_ADC_DONE, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_PUMPX_DONE, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_BATUV, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_MIDOV, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_OLP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_ATTACH, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DETACH, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_STPDONE, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_VBUSDET_DONE, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_DET, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHGDET, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DCDT, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_VGOK, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_WDTMR, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_UC, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_OC, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_OV, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_SWON, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_UVP_D, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_UVP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_OVP_D, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_OVP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_STRBPIN, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_TORPIN, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_TX, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_LVF, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_SHORT, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_SHORT, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_STRB, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_STRB, 8),
> +	REGMAP_IRQ_REG_LINE(mT6370_IRQ_FLED2_STRB_TO, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_STRB_TO, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_TOR, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_TOR, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_OTP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_VDDA_OVP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_VDDA_UV, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_LDO_OC, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_BLED_OCP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_BLED_OVP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VNEG_OCP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VPOS_OCP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_BST_OCP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VNEG_SCP, 8),
> +	REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VPOS_SCP, 8)
> +};
> +
> +static const struct regmap_irq_chip mt6370_irq_chip = {
> +	.name		= "mt6370-irqs",
> +	.status_base	= MT6370_REG_CHG_IRQ1,
> +	.mask_base	= MT6370_REG_CHG_MASK1,
> +	.num_regs	= MT6370_NUM_IRQREGS,
> +	.irqs		= mt6370_irqs,
> +	.num_irqs	= ARRAY_SIZE(mt6370_irqs),
> +};
> +
> +static const struct resource mt6370_regulator_irqs[] = {
> +	DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VPOS_SCP, "db_vpos_scp"),
> +	DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VNEG_SCP, "db_vneg_scp"),
> +	DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_BST_OCP, "db_vbst_ocp"),
> +	DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VPOS_OCP, "db_vpos_ocp"),
> +	DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VNEG_OCP, "db_vneg_ocp"),
> +	DEFINE_RES_IRQ_NAMED(MT6370_IRQ_LDO_OC, "ldo_oc")
> +};
> +
> +static const struct mfd_cell mt6370_devices[] = {
> +	MFD_CELL_OF("adc", NULL, NULL, 0, 0, "mediatek,mt6370-adc"),
> +	MFD_CELL_OF("charger", NULL, NULL, 0, 0, "mediatek,mt6370-charger"),
> +	MFD_CELL_OF("backlight", NULL, NULL, 0, 0, "mediatek,mt6370-backlight"),
> +	MFD_CELL_OF("flashlight", NULL, NULL, 0, 0, "mediatek,mt6370-flashlight"),
> +	MFD_CELL_OF("indicator", NULL, NULL, 0, 0, "mediatek,mt6370-indicator"),
> +	MFD_CELL_OF("tcpc", NULL, NULL, 0, 0, "mediatek,mt6370-tcpc"),
> +	MFD_CELL_RES("regulator", mt6370_regulator_irqs)
> +};
> +
> +static int mt6370_check_vendor_info(struct mt6370_info *info)
> +{
> +	unsigned int devinfo;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, MT6370_REG_DEV_INFO, &devinfo);
> +	if (ret)
> +		return ret;
> +
> +	switch (FIELD_GET(MT6370_VENID_MASK, devinfo)) {
> +	case 0x8: /* RT5081 */
> +	case 0xA: /* RT5081A */
> +	case 0xE: /* MT6370 */
> +	case 0xF: /* MT6371 */
> +	case 0x9: /* MT6372P */
> +	case 0xB: /* MT6372CP */

Please define these and drop the comments.

> +		break;
> +	default:
> +		dev_err(info->dev, "Not invalid value 0x%02x\n", devinfo);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6370_regmap_read(void *context, const void *reg_buf,
> +			      size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct mt6370_info *info = context;
> +	u8 bank_idx = *(u8 *)reg_buf, bank_addr = *(u8 *)(reg_buf + 1);

Looks a little scruffy.  Perhaps allocate the values below.

> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(info->i2c[bank_idx], bank_addr,
> +					    val_size, val_buf);
> +	if (ret != val_size)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int mt6370_regmap_write(void *context, const void *data, size_t count)
> +{
> +	struct mt6370_info *info = context;
> +	u8 bank_idx = *(u8 *)data, bank_addr = *(u8 *)(data + 1);

As above.

> +	int len = count - MT6370_REG_ADDRLEN;
> +
> +	return i2c_smbus_write_i2c_block_data(info->i2c[bank_idx], bank_addr,
> +					      len, data + MT6370_REG_ADDRLEN);
> +}
> +
> +static const struct regmap_bus mt6370_regmap_bus = {
> +	.read		= mt6370_regmap_read,
> +	.write		= mt6370_regmap_write,
> +};
> +
> +static const struct regmap_config mt6370_regmap_config = {
> +	.reg_bits		= 16,
> +	.val_bits		= 8,
> +	.reg_format_endian	= REGMAP_ENDIAN_BIG,
> +	.max_register		= MT6370_REG_MAXADDR,
> +};
> +
> +static int mt6370_probe(struct i2c_client *i2c)
> +{
> +	struct mt6370_info *info;
> +	struct i2c_client *usbc_i2c;
> +	int ret;
> +
> +	info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->dev = &i2c->dev;
> +
> +	usbc_i2c = devm_i2c_new_dummy_device(&i2c->dev, i2c->adapter,
> +					     MT6370_USBC_I2CADDR);
> +	if (IS_ERR(usbc_i2c)) {
> +		ret = PTR_ERR(usbc_i2c);
> +		dev_err(&i2c->dev, "Failed to register usbc i2c client %d\n", ret);

"USB-C I2C"?

> +		return ret;
> +	}
> +
> +	/* Assign I2C client for PMU and TypeC */
> +	info->i2c[MT6370_PMU_I2C] = i2c;
> +	info->i2c[MT6370_USBC_I2C] = usbc_i2c;
> +
> +	info->regmap = devm_regmap_init(&i2c->dev, &mt6370_regmap_bus, info,
> +					&mt6370_regmap_config);
> +	if (IS_ERR(info->regmap)) {
> +		ret = PTR_ERR(info->regmap);
> +		dev_err(&i2c->dev, "Failed to register regmap (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mt6370_check_vendor_info(info);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to check vendor info (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_regmap_add_irq_chip(&i2c->dev, info->regmap, i2c->irq,
> +				       IRQF_ONESHOT, -1, &mt6370_irq_chip,
> +				       &info->irq_data);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to add irq chip (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> +				    mt6370_devices, ARRAY_SIZE(mt6370_devices),
> +				    NULL, 0,
> +				    regmap_irq_get_domain(info->irq_data));
> +}
> +
> +static const struct of_device_id mt6370_match_table[] = {
> +	{ .compatible = "mediatek,mt6370", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mt6370_match_table);
> +
> +static struct i2c_driver mt6370_driver = {
> +	.driver = {
> +		.name = "mt6370",
> +		.of_match_table = mt6370_match_table,
> +	},
> +	.probe_new = mt6370_probe,
> +};
> +module_i2c_driver(mt6370_driver);
> +
> +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> +MODULE_DESCRIPTION("MT6370 I2C MFD Driver");

Drop all references to "MFD" please.

> +MODULE_LICENSE("GPL v2");
ChiaEn Wu June 17, 2022, 9:34 a.m. UTC | #6
Hi Daniel,

Thanks for your helpful feedback!

Daniel Thompson <daniel.thompson@linaro.org> 於 2022年6月14日 週二 凌晨1:08寫道:
>
> On Mon, Jun 13, 2022 at 07:11:46PM +0800, ChiaEn Wu wrote:
> > +static int mt6370_init_backlight_properties(struct mt6370_priv *priv,
> > +                                         struct backlight_properties *props)
>
> Most of the changes in this version looks good... but it looks the new
> code in this function has a number of problems. See below...
>
>
> > +{
> > +     struct device *dev = priv->dev;
> > +     u8 prop_val;
> > +     u32 brightness;
> > +     unsigned int mask, val;
> > +     int ret;
> > +
> > +     /* Vendor optional properties
> > +      * if property not exist, keep value in default.
> > +      */
>
> That's not the right strategy for booleans. Not existing means false
> (e.g. flags should actively be unset).
>

I am so sorry for making these mistakes...
I will try to refine them in the right strategy in the next patch!

>
> > +     if (device_property_read_bool(dev, "mediatek,bled-pwm-enable")) {
> > +             ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM,
> > +                                      MT6370_BL_PWM_EN_MASK,
> > +                                      MT6370_BL_PWM_EN_MASK);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> As above comment... all of the boolean properties are now being read
> incorrectly.
>
>
> > +
> > +     if (device_property_read_bool(dev, "mediatek,bled-pwm-hys-enable")) {
> > +             ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM,
> > +                                      MT6370_BL_PWM_HYS_EN_MASK,
> > +                                      MT6370_BL_PWM_HYS_EN_MASK);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     ret = device_property_read_u8(dev, "mediatek,bled-pwm-hys-input-bit",
> > +                                   &prop_val);
> > +     if (!ret) {
> > +             val = min_t(u8, prop_val, 3)
> > +                   << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1);
> > +             ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM,
> > +                                      MT6370_BL_PWM_HYS_SEL_MASK, val);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     ret = device_property_read_u8(dev, "mediatek,bled-ovp-microvolt",
> > +                                   &prop_val);
> > +     if (!ret) {
> > +             val = min_t(u8, prop_val, 3)
> > +                   << (ffs(MT6370_BL_OVP_SEL_MASK) - 1);
>
> This has been renamed but still seems to the using 0, 1, 2, 3 rather
> than an actual value in microvolts.

I’m so sorry for using the not actual value in microvolts and microamps.
I will refine these mistakes along with DT in the next patch. Thank you!

>
>
> > +             ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL,
> > +                                      MT6370_BL_OVP_SEL_MASK, val);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (device_property_read_bool(dev, "mediatek,bled-ovp-shutdown")) {
> > +             ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL,
> > +                                      MT6370_BL_OVP_EN_MASK,
> > +                                      MT6370_BL_OVP_EN_MASK);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     ret = device_property_read_u8(dev, "mediatek,bled-ocp-microamp",
> > +                                   &prop_val);
> > +     if (!ret) {
> > +             val = min_t(u8, prop_val, 3)
> > +                   << (ffs(MT6370_BL_OC_SEL_MASK) - 1);
>
> Likewise, should this be accepting a value in microamps?
>
>
> > +             ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL,
> > +                                      MT6370_BL_OC_SEL_MASK, val);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (device_property_read_bool(dev, "mediatek,bled-ocp-shutdown")) {
> > +             ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL,
> > +                                      MT6370_BL_OC_EN_MASK,
> > +                                      MT6370_BL_OC_EN_MASK);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     /* Common properties */
> > +     ret = device_property_read_u32(dev, "max-brightness", &brightness);
> > +     if (ret)
> > +             brightness = MT6370_BL_MAX_BRIGHTNESS;
> > +
> > +     props->max_brightness = min_t(u32, brightness,
> > +                                   MT6370_BL_MAX_BRIGHTNESS);
> > +
> > +     ret = device_property_read_u32(dev, "default-brightness", &brightness);
> > +     if (ret)
> > +             brightness = props->max_brightness;
> > +
> > +     props->brightness = min_t(u32, brightness, props->max_brightness);
> > +
> > +
> > +     ret = device_property_read_u8(dev, "mediatek,bled-channel-use",
> > +                                   &prop_val);
> > +     if (ret) {
> > +             dev_err(dev, "mediatek,bled-channel-use DT property missing\n");
> > +             return ret;
> > +     }
> > +
> > +     if (!prop_val || prop_val > MT6370_BL_MAX_CH) {
> > +             dev_err(dev, "No channel specified (ch_val:%d)\n", prop_val);
>
> Error string has not been updated to match condition that triggers it.
>

I will refine this wrong error string in the next patch, thanks!

>
> > +             return -EINVAL;
> > +     }
>
>
> Daniel.

Best regards,
ChiaEn Wu
ChiaEn Wu June 17, 2022, 5:15 p.m. UTC | #7
Hi Lee,

Thanks for your helpful comments, we have some questions and replies below.

Lee Jones <lee.jones@linaro.org> 於 2022年6月16日 週四 清晨6:49寫道:

>
> On Mon, 13 Jun 2022, ChiaEn Wu wrote:
>
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add Mediatek MT6370 MFD support.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  drivers/mfd/Kconfig  |  13 ++
> >  drivers/mfd/Makefile |   1 +
> >  drivers/mfd/mt6370.c | 349 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 363 insertions(+)
> >  create mode 100644 drivers/mfd/mt6370.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 3b59456f5545..d9a7524a3e0e 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -937,6 +937,19 @@ config MFD_MT6360
> >         PMIC part includes 2-channel BUCKs and 2-channel LDOs
> >         LDO part includes 4-channel LDOs
> >
> > +config MFD_MT6370
> > +     tristate "Mediatek MT6370 SubPMIC"
> > +     select MFD_CORE
> > +     select REGMAP_I2C
> > +     select REGMAP_IRQ
> > +     depends on I2C
> > +     help
> > +       Say Y here to enable MT6370 SubPMIC functional support.
> > +       It integrate single cell battery charger with adc monitoring, RGB
>
> s/integrates/consists of a/
>
> "ADC"

We will fine it in the next patch.

>
> > +       LEDs, dual channel flashlight, WLED backlight driver, display bias
>
> > +       voltage supply, one general purpose LDO, and cc logic
> > +       controller with USBPD commmunication capable.
>
> The last part makes no sense - "and is USBPD"?

If we modify this help text to
"one general purpose LDO, and the USB Type-C & PD controller complies
with the latest USB Type-C and PD standards",
did these modifications meet your expectations?

>
> >  config MFD_MT6397
> >       tristate "MediaTek MT6397 PMIC Support"
> >       select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 858cacf659d6..62b27125420e 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -242,6 +242,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)        += intel_soc_pmic_bxtwc.o
> >  obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)   += intel_soc_pmic_chtwc.o
> >  obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)        += intel_soc_pmic_chtdc_ti.o
> >  obj-$(CONFIG_MFD_MT6360)     += mt6360-core.o
> > +obj-$(CONFIG_MFD_MT6370)     += mt6370.o
> >  mt6397-objs                  := mt6397-core.o mt6397-irq.o mt6358-irq.o
> >  obj-$(CONFIG_MFD_MT6397)     += mt6397.o
> >  obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)   += intel_soc_pmic_mrfld.o
> > diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c
> > new file mode 100644
> > index 000000000000..6af9f73c9c0c
> > --- /dev/null
> > +++ b/drivers/mfd/mt6370.c
> > @@ -0,0 +1,349 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bits.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +enum {
> > +     MT6370_USBC_I2C = 0,
> > +     MT6370_PMU_I2C,
> > +     MT6370_MAX_I2C
> > +};
> > +
> > +#define MT6370_REG_DEV_INFO  0x100
> > +#define MT6370_REG_CHG_IRQ1  0x1C0
> > +#define MT6370_REG_CHG_MASK1 0x1E0
> > +
> > +#define MT6370_VENID_MASK    GENMASK(7, 4)
> > +
> > +#define MT6370_NUM_IRQREGS   16
> > +#define MT6370_USBC_I2CADDR  0x4E
> > +#define MT6370_REG_ADDRLEN   2
> > +#define MT6370_REG_MAXADDR   0x1FF
> > +
> > +/* IRQ definitions */
> > +#define MT6370_IRQ_DIRCHGON          0
> > +#define MT6370_IRQ_CHG_TREG          4
> > +#define MT6370_IRQ_CHG_AICR          5
> > +#define MT6370_IRQ_CHG_MIVR          6
> > +#define MT6370_IRQ_PWR_RDY           7
> > +#define MT6370_IRQ_FL_CHG_VINOVP     11
> > +#define MT6370_IRQ_CHG_VSYSUV                12
> > +#define MT6370_IRQ_CHG_VSYSOV                13
> > +#define MT6370_IRQ_CHG_VBATOV                14
> > +#define MT6370_IRQ_CHG_VINOVPCHG     15
> > +#define MT6370_IRQ_TS_BAT_COLD               20
> > +#define MT6370_IRQ_TS_BAT_COOL               21
> > +#define MT6370_IRQ_TS_BAT_WARM               22
> > +#define MT6370_IRQ_TS_BAT_HOT                23
> > +#define MT6370_IRQ_TS_STATC          24
> > +#define MT6370_IRQ_CHG_FAULT         25
> > +#define MT6370_IRQ_CHG_STATC         26
> > +#define MT6370_IRQ_CHG_TMR           27
> > +#define MT6370_IRQ_CHG_BATABS                28
> > +#define MT6370_IRQ_CHG_ADPBAD                29
> > +#define MT6370_IRQ_CHG_RVP           30
> > +#define MT6370_IRQ_TSHUTDOWN         31
> > +#define MT6370_IRQ_CHG_IINMEAS               32
> > +#define MT6370_IRQ_CHG_ICCMEAS               33
> > +#define MT6370_IRQ_CHGDET_DONE               34
> > +#define MT6370_IRQ_WDTMR             35
> > +#define MT6370_IRQ_SSFINISH          36
> > +#define MT6370_IRQ_CHG_RECHG         37
> > +#define MT6370_IRQ_CHG_TERM          38
> > +#define MT6370_IRQ_CHG_IEOC          39
> > +#define MT6370_IRQ_ADC_DONE          40
> > +#define MT6370_IRQ_PUMPX_DONE                41
> > +#define MT6370_IRQ_BST_BATUV         45
> > +#define MT6370_IRQ_BST_MIDOV         46
> > +#define MT6370_IRQ_BST_OLP           47
> > +#define MT6370_IRQ_ATTACH            48
> > +#define MT6370_IRQ_DETACH            49
> > +#define MT6370_IRQ_HVDCP_STPDONE     51
> > +#define MT6370_IRQ_HVDCP_VBUSDET_DONE        52
> > +#define MT6370_IRQ_HVDCP_DET         53
> > +#define MT6370_IRQ_CHGDET            54
> > +#define MT6370_IRQ_DCDT                      55
> > +#define MT6370_IRQ_DIRCHG_VGOK               59
> > +#define MT6370_IRQ_DIRCHG_WDTMR              60
> > +#define MT6370_IRQ_DIRCHG_UC         61
> > +#define MT6370_IRQ_DIRCHG_OC         62
> > +#define MT6370_IRQ_DIRCHG_OV         63
> > +#define MT6370_IRQ_OVPCTRL_SWON              67
> > +#define MT6370_IRQ_OVPCTRL_UVP_D     68
> > +#define MT6370_IRQ_OVPCTRL_UVP               69
> > +#define MT6370_IRQ_OVPCTRL_OVP_D     70
> > +#define MT6370_IRQ_OVPCTRL_OVP               71
> > +#define MT6370_IRQ_FLED_STRBPIN              72
> > +#define MT6370_IRQ_FLED_TORPIN               73
> > +#define MT6370_IRQ_FLED_TX           74
> > +#define MT6370_IRQ_FLED_LVF          75
> > +#define MT6370_IRQ_FLED2_SHORT               78
> > +#define MT6370_IRQ_FLED1_SHORT               79
> > +#define MT6370_IRQ_FLED2_STRB                80
> > +#define MT6370_IRQ_FLED1_STRB                81
> > +#define mT6370_IRQ_FLED2_STRB_TO     82
> > +#define MT6370_IRQ_FLED1_STRB_TO     83
> > +#define MT6370_IRQ_FLED2_TOR         84
> > +#define MT6370_IRQ_FLED1_TOR         85
> > +#define MT6370_IRQ_OTP                       93
> > +#define MT6370_IRQ_VDDA_OVP          94
> > +#define MT6370_IRQ_VDDA_UV           95
> > +#define MT6370_IRQ_LDO_OC            103
> > +#define MT6370_IRQ_BLED_OCP          118
> > +#define MT6370_IRQ_BLED_OVP          119
> > +#define MT6370_IRQ_DSV_VNEG_OCP              123
> > +#define MT6370_IRQ_DSV_VPOS_OCP              124
> > +#define MT6370_IRQ_DSV_BST_OCP               125
> > +#define MT6370_IRQ_DSV_VNEG_SCP              126
> > +#define MT6370_IRQ_DSV_VPOS_SCP              127
> > +
> > +struct mt6370_info {
> > +     struct i2c_client *i2c[MT6370_MAX_I2C];
> > +     struct device *dev;
> > +     struct regmap *regmap;
> > +     struct regmap_irq_chip_data *irq_data;
> > +};
>
> Can we shove all of the above into a header file?

Well... In Patch v1, we put these "#define IRQ" into
"include/dt-bindings/mfd/mediatek,mt6370.h".
But the reviewer of DT files hoped us to remove this header file, we
put these "#define IRQ" in this .c file.
Shall we leave them here or put them into the header file in
"driver/power/supply/mt6370-charger.h"?

>
> > +static const struct regmap_irq mt6370_irqs[] = {
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHGON, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TREG, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_AICR, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_MIVR, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_PWR_RDY, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FL_CHG_VINOVP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VSYSUV, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VSYSOV, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VBATOV, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VINOVPCHG, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_COLD, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_COOL, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_WARM, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_HOT, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_STATC, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_FAULT, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_STATC, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TMR, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_BATABS, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_ADPBAD, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_RVP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_TSHUTDOWN, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_IINMEAS, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_ICCMEAS, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHGDET_DONE, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_WDTMR, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_SSFINISH, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_RECHG, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TERM, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_IEOC, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_ADC_DONE, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_PUMPX_DONE, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_BATUV, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_MIDOV, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_OLP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_ATTACH, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DETACH, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_STPDONE, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_VBUSDET_DONE, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_DET, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHGDET, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DCDT, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_VGOK, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_WDTMR, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_UC, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_OC, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_OV, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_SWON, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_UVP_D, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_UVP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_OVP_D, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_OVP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_STRBPIN, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_TORPIN, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_TX, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_LVF, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_SHORT, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_SHORT, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_STRB, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_STRB, 8),
> > +     REGMAP_IRQ_REG_LINE(mT6370_IRQ_FLED2_STRB_TO, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_STRB_TO, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_TOR, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_TOR, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_OTP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_VDDA_OVP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_VDDA_UV, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_LDO_OC, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_BLED_OCP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_BLED_OVP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VNEG_OCP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VPOS_OCP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_BST_OCP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VNEG_SCP, 8),
> > +     REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VPOS_SCP, 8)
> > +};
> > +
> > +static const struct regmap_irq_chip mt6370_irq_chip = {
> > +     .name           = "mt6370-irqs",
> > +     .status_base    = MT6370_REG_CHG_IRQ1,
> > +     .mask_base      = MT6370_REG_CHG_MASK1,
> > +     .num_regs       = MT6370_NUM_IRQREGS,
> > +     .irqs           = mt6370_irqs,
> > +     .num_irqs       = ARRAY_SIZE(mt6370_irqs),
> > +};
> > +
> > +static const struct resource mt6370_regulator_irqs[] = {
> > +     DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VPOS_SCP, "db_vpos_scp"),
> > +     DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VNEG_SCP, "db_vneg_scp"),
> > +     DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_BST_OCP, "db_vbst_ocp"),
> > +     DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VPOS_OCP, "db_vpos_ocp"),
> > +     DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VNEG_OCP, "db_vneg_ocp"),
> > +     DEFINE_RES_IRQ_NAMED(MT6370_IRQ_LDO_OC, "ldo_oc")
> > +};
> > +
> > +static const struct mfd_cell mt6370_devices[] = {
> > +     MFD_CELL_OF("adc", NULL, NULL, 0, 0, "mediatek,mt6370-adc"),
> > +     MFD_CELL_OF("charger", NULL, NULL, 0, 0, "mediatek,mt6370-charger"),
> > +     MFD_CELL_OF("backlight", NULL, NULL, 0, 0, "mediatek,mt6370-backlight"),
> > +     MFD_CELL_OF("flashlight", NULL, NULL, 0, 0, "mediatek,mt6370-flashlight"),
> > +     MFD_CELL_OF("indicator", NULL, NULL, 0, 0, "mediatek,mt6370-indicator"),
> > +     MFD_CELL_OF("tcpc", NULL, NULL, 0, 0, "mediatek,mt6370-tcpc"),
> > +     MFD_CELL_RES("regulator", mt6370_regulator_irqs)
> > +};
> > +
> > +static int mt6370_check_vendor_info(struct mt6370_info *info)
> > +{
> > +     unsigned int devinfo;
> > +     int ret;
> > +
> > +     ret = regmap_read(info->regmap, MT6370_REG_DEV_INFO, &devinfo);
> > +     if (ret)
> > +             return ret;
> > +
> > +     switch (FIELD_GET(MT6370_VENID_MASK, devinfo)) {
> > +     case 0x8: /* RT5081 */
> > +     case 0xA: /* RT5081A */
> > +     case 0xE: /* MT6370 */
> > +     case 0xF: /* MT6371 */
> > +     case 0x9: /* MT6372P */
> > +     case 0xB: /* MT6372CP */
>
> Please define these and drop the comments.

OK, we will refine them in the next patch! Thanks!

>
> > +             break;
> > +     default:
> > +             dev_err(info->dev, "Not invalid value 0x%02x\n", devinfo);
> > +             return -ENODEV;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int mt6370_regmap_read(void *context, const void *reg_buf,
> > +                           size_t reg_size, void *val_buf, size_t val_size)
> > +{
> > +     struct mt6370_info *info = context;
> > +     u8 bank_idx = *(u8 *)reg_buf, bank_addr = *(u8 *)(reg_buf + 1);
>
> Looks a little scruffy.  Perhaps allocate the values below.
>
> > +     int ret;
> > +
> > +     ret = i2c_smbus_read_i2c_block_data(info->i2c[bank_idx], bank_addr,
> > +                                         val_size, val_buf);
> > +     if (ret != val_size)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static int mt6370_regmap_write(void *context, const void *data, size_t count)
> > +{
> > +     struct mt6370_info *info = context;
> > +     u8 bank_idx = *(u8 *)data, bank_addr = *(u8 *)(data + 1);
>
> As above.

OK, we will try to refine these parts in the next patch.

>
> > +     int len = count - MT6370_REG_ADDRLEN;
> > +
> > +     return i2c_smbus_write_i2c_block_data(info->i2c[bank_idx], bank_addr,
> > +                                           len, data + MT6370_REG_ADDRLEN);
> > +}
> > +
> > +static const struct regmap_bus mt6370_regmap_bus = {
> > +     .read           = mt6370_regmap_read,
> > +     .write          = mt6370_regmap_write,
> > +};
> > +
> > +static const struct regmap_config mt6370_regmap_config = {
> > +     .reg_bits               = 16,
> > +     .val_bits               = 8,
> > +     .reg_format_endian      = REGMAP_ENDIAN_BIG,
> > +     .max_register           = MT6370_REG_MAXADDR,
> > +};
> > +
> > +static int mt6370_probe(struct i2c_client *i2c)
> > +{
> > +     struct mt6370_info *info;
> > +     struct i2c_client *usbc_i2c;
> > +     int ret;
> > +
> > +     info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
> > +     if (!info)
> > +             return -ENOMEM;
> > +
> > +     info->dev = &i2c->dev;
> > +
> > +     usbc_i2c = devm_i2c_new_dummy_device(&i2c->dev, i2c->adapter,
> > +                                          MT6370_USBC_I2CADDR);
> > +     if (IS_ERR(usbc_i2c)) {
> > +             ret = PTR_ERR(usbc_i2c);
> > +             dev_err(&i2c->dev, "Failed to register usbc i2c client %d\n", ret);
>
> "USB-C I2C"?

MT6370 have two slave address, one is for the USB PD module and
another one is for other modules.
So we use "USB-C I2C" to refer to the USB PD module.

>
> > +             return ret;
> > +     }
> > +
> > +     /* Assign I2C client for PMU and TypeC */
> > +     info->i2c[MT6370_PMU_I2C] = i2c;
> > +     info->i2c[MT6370_USBC_I2C] = usbc_i2c;
> > +
> > +     info->regmap = devm_regmap_init(&i2c->dev, &mt6370_regmap_bus, info,
> > +                                     &mt6370_regmap_config);
> > +     if (IS_ERR(info->regmap)) {
> > +             ret = PTR_ERR(info->regmap);
> > +             dev_err(&i2c->dev, "Failed to register regmap (%d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = mt6370_check_vendor_info(info);
> > +     if (ret) {
> > +             dev_err(&i2c->dev, "Failed to check vendor info (%d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = devm_regmap_add_irq_chip(&i2c->dev, info->regmap, i2c->irq,
> > +                                    IRQF_ONESHOT, -1, &mt6370_irq_chip,
> > +                                    &info->irq_data);
> > +     if (ret) {
> > +             dev_err(&i2c->dev, "Failed to add irq chip (%d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     return devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> > +                                 mt6370_devices, ARRAY_SIZE(mt6370_devices),
> > +                                 NULL, 0,
> > +                                 regmap_irq_get_domain(info->irq_data));
> > +}
> > +
> > +static const struct of_device_id mt6370_match_table[] = {
> > +     { .compatible = "mediatek,mt6370", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, mt6370_match_table);
> > +
> > +static struct i2c_driver mt6370_driver = {
> > +     .driver = {
> > +             .name = "mt6370",
> > +             .of_match_table = mt6370_match_table,
> > +     },
> > +     .probe_new = mt6370_probe,
> > +};
> > +module_i2c_driver(mt6370_driver);
> > +
> > +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> > +MODULE_DESCRIPTION("MT6370 I2C MFD Driver");
>
> Drop all references to "MFD" please.

OK, we will drop them in the next patch. Thank you so much!

>
> > +MODULE_LICENSE("GPL v2");
>
> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Best regards,
ChiaEn Wu
Lee Jones June 27, 2022, 2:14 p.m. UTC | #8
On Sat, 18 Jun 2022, ChiaEn Wu wrote:

> Hi Lee,
> 
> Thanks for your helpful comments, we have some questions and replies below.
> 
> Lee Jones <lee.jones@linaro.org> 於 2022年6月16日 週四 清晨6:49寫道:
> 
> >
> > On Mon, 13 Jun 2022, ChiaEn Wu wrote:
> >
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > Add Mediatek MT6370 MFD support.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > ---
> > >  drivers/mfd/Kconfig  |  13 ++
> > >  drivers/mfd/Makefile |   1 +
> > >  drivers/mfd/mt6370.c | 349 +++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 363 insertions(+)
> > >  create mode 100644 drivers/mfd/mt6370.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 3b59456f5545..d9a7524a3e0e 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -937,6 +937,19 @@ config MFD_MT6360
> > >         PMIC part includes 2-channel BUCKs and 2-channel LDOs
> > >         LDO part includes 4-channel LDOs
> > >
> > > +config MFD_MT6370
> > > +     tristate "Mediatek MT6370 SubPMIC"
> > > +     select MFD_CORE
> > > +     select REGMAP_I2C
> > > +     select REGMAP_IRQ
> > > +     depends on I2C
> > > +     help
> > > +       Say Y here to enable MT6370 SubPMIC functional support.
> > > +       It integrate single cell battery charger with adc monitoring, RGB
> >
> > s/integrates/consists of a/
> >
> > "ADC"
> 
> We will fine it in the next patch.
> 
> >
> > > +       LEDs, dual channel flashlight, WLED backlight driver, display bias
> >
> > > +       voltage supply, one general purpose LDO, and cc logic
> > > +       controller with USBPD commmunication capable.
> >
> > The last part makes no sense - "and is USBPD"?
> 
> If we modify this help text to
> "one general purpose LDO, and the USB Type-C & PD controller complies
> with the latest USB Type-C and PD standards",
> did these modifications meet your expectations?

"one general purpose LDO and a USB Type-C & PD controller that
complies with the latest USB Type-C and PD standards"

Better?

> > >  config MFD_MT6397
> > >       tristate "MediaTek MT6397 PMIC Support"
> > >       select MFD_CORE
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index 858cacf659d6..62b27125420e 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -242,6 +242,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)        += intel_soc_pmic_bxtwc.o
> > >  obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)   += intel_soc_pmic_chtwc.o
> > >  obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)        += intel_soc_pmic_chtdc_ti.o
> > >  obj-$(CONFIG_MFD_MT6360)     += mt6360-core.o
> > > +obj-$(CONFIG_MFD_MT6370)     += mt6370.o
> > >  mt6397-objs                  := mt6397-core.o mt6397-irq.o mt6358-irq.o
> > >  obj-$(CONFIG_MFD_MT6397)     += mt6397.o
> > >  obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)   += intel_soc_pmic_mrfld.o
> > > diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c
> > > new file mode 100644
> > > index 000000000000..6af9f73c9c0c
> > > --- /dev/null
> > > +++ b/drivers/mfd/mt6370.c
> > > @@ -0,0 +1,349 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <linux/bits.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +enum {
> > > +     MT6370_USBC_I2C = 0,
> > > +     MT6370_PMU_I2C,
> > > +     MT6370_MAX_I2C
> > > +};
> > > +
> > > +#define MT6370_REG_DEV_INFO  0x100
> > > +#define MT6370_REG_CHG_IRQ1  0x1C0
> > > +#define MT6370_REG_CHG_MASK1 0x1E0
> > > +
> > > +#define MT6370_VENID_MASK    GENMASK(7, 4)
> > > +
> > > +#define MT6370_NUM_IRQREGS   16
> > > +#define MT6370_USBC_I2CADDR  0x4E
> > > +#define MT6370_REG_ADDRLEN   2
> > > +#define MT6370_REG_MAXADDR   0x1FF
> > > +
> > > +/* IRQ definitions */
> > > +#define MT6370_IRQ_DIRCHGON          0
> > > +#define MT6370_IRQ_CHG_TREG          4
> > > +#define MT6370_IRQ_CHG_AICR          5
> > > +#define MT6370_IRQ_CHG_MIVR          6
> > > +#define MT6370_IRQ_PWR_RDY           7
> > > +#define MT6370_IRQ_FL_CHG_VINOVP     11
> > > +#define MT6370_IRQ_CHG_VSYSUV                12
> > > +#define MT6370_IRQ_CHG_VSYSOV                13
> > > +#define MT6370_IRQ_CHG_VBATOV                14
> > > +#define MT6370_IRQ_CHG_VINOVPCHG     15
> > > +#define MT6370_IRQ_TS_BAT_COLD               20
> > > +#define MT6370_IRQ_TS_BAT_COOL               21
> > > +#define MT6370_IRQ_TS_BAT_WARM               22
> > > +#define MT6370_IRQ_TS_BAT_HOT                23
> > > +#define MT6370_IRQ_TS_STATC          24
> > > +#define MT6370_IRQ_CHG_FAULT         25
> > > +#define MT6370_IRQ_CHG_STATC         26
> > > +#define MT6370_IRQ_CHG_TMR           27
> > > +#define MT6370_IRQ_CHG_BATABS                28
> > > +#define MT6370_IRQ_CHG_ADPBAD                29
> > > +#define MT6370_IRQ_CHG_RVP           30
> > > +#define MT6370_IRQ_TSHUTDOWN         31
> > > +#define MT6370_IRQ_CHG_IINMEAS               32
> > > +#define MT6370_IRQ_CHG_ICCMEAS               33
> > > +#define MT6370_IRQ_CHGDET_DONE               34
> > > +#define MT6370_IRQ_WDTMR             35
> > > +#define MT6370_IRQ_SSFINISH          36
> > > +#define MT6370_IRQ_CHG_RECHG         37
> > > +#define MT6370_IRQ_CHG_TERM          38
> > > +#define MT6370_IRQ_CHG_IEOC          39
> > > +#define MT6370_IRQ_ADC_DONE          40
> > > +#define MT6370_IRQ_PUMPX_DONE                41
> > > +#define MT6370_IRQ_BST_BATUV         45
> > > +#define MT6370_IRQ_BST_MIDOV         46
> > > +#define MT6370_IRQ_BST_OLP           47
> > > +#define MT6370_IRQ_ATTACH            48
> > > +#define MT6370_IRQ_DETACH            49
> > > +#define MT6370_IRQ_HVDCP_STPDONE     51
> > > +#define MT6370_IRQ_HVDCP_VBUSDET_DONE        52
> > > +#define MT6370_IRQ_HVDCP_DET         53
> > > +#define MT6370_IRQ_CHGDET            54
> > > +#define MT6370_IRQ_DCDT                      55
> > > +#define MT6370_IRQ_DIRCHG_VGOK               59
> > > +#define MT6370_IRQ_DIRCHG_WDTMR              60
> > > +#define MT6370_IRQ_DIRCHG_UC         61
> > > +#define MT6370_IRQ_DIRCHG_OC         62
> > > +#define MT6370_IRQ_DIRCHG_OV         63
> > > +#define MT6370_IRQ_OVPCTRL_SWON              67
> > > +#define MT6370_IRQ_OVPCTRL_UVP_D     68
> > > +#define MT6370_IRQ_OVPCTRL_UVP               69
> > > +#define MT6370_IRQ_OVPCTRL_OVP_D     70
> > > +#define MT6370_IRQ_OVPCTRL_OVP               71
> > > +#define MT6370_IRQ_FLED_STRBPIN              72
> > > +#define MT6370_IRQ_FLED_TORPIN               73
> > > +#define MT6370_IRQ_FLED_TX           74
> > > +#define MT6370_IRQ_FLED_LVF          75
> > > +#define MT6370_IRQ_FLED2_SHORT               78
> > > +#define MT6370_IRQ_FLED1_SHORT               79
> > > +#define MT6370_IRQ_FLED2_STRB                80
> > > +#define MT6370_IRQ_FLED1_STRB                81
> > > +#define mT6370_IRQ_FLED2_STRB_TO     82
> > > +#define MT6370_IRQ_FLED1_STRB_TO     83
> > > +#define MT6370_IRQ_FLED2_TOR         84
> > > +#define MT6370_IRQ_FLED1_TOR         85
> > > +#define MT6370_IRQ_OTP                       93
> > > +#define MT6370_IRQ_VDDA_OVP          94
> > > +#define MT6370_IRQ_VDDA_UV           95
> > > +#define MT6370_IRQ_LDO_OC            103
> > > +#define MT6370_IRQ_BLED_OCP          118
> > > +#define MT6370_IRQ_BLED_OVP          119
> > > +#define MT6370_IRQ_DSV_VNEG_OCP              123
> > > +#define MT6370_IRQ_DSV_VPOS_OCP              124
> > > +#define MT6370_IRQ_DSV_BST_OCP               125
> > > +#define MT6370_IRQ_DSV_VNEG_SCP              126
> > > +#define MT6370_IRQ_DSV_VPOS_SCP              127
> > > +
> > > +struct mt6370_info {
> > > +     struct i2c_client *i2c[MT6370_MAX_I2C];
> > > +     struct device *dev;
> > > +     struct regmap *regmap;
> > > +     struct regmap_irq_chip_data *irq_data;
> > > +};
> >
> > Can we shove all of the above into a header file?
> 
> Well... In Patch v1, we put these "#define IRQ" into
> "include/dt-bindings/mfd/mediatek,mt6370.h".
> But the reviewer of DT files hoped us to remove this header file, we
> put these "#define IRQ" in this .c file.
> Shall we leave them here or put them into the header file in
> "driver/power/supply/mt6370-charger.h"?

Where are they used?
ChiaEn Wu June 27, 2022, 3:35 p.m. UTC | #9
Hi Lee,

Thanks for your reply!

Lee Jones <lee.jones@linaro.org> 於 2022年6月27日 週一 晚上10:14寫道:
>
> On Sat, 18 Jun 2022, ChiaEn Wu wrote:
>
> > Hi Lee,
> >
> > Thanks for your helpful comments, we have some questions and replies below.
> >
> > Lee Jones <lee.jones@linaro.org> 於 2022年6月16日 週四 清晨6:49寫道:
> >
> > >
> > > On Mon, 13 Jun 2022, ChiaEn Wu wrote:
> > >
> > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > >
> > > > Add Mediatek MT6370 MFD support.
> > > >
> > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > > ---
> > > >  drivers/mfd/Kconfig  |  13 ++
> > > >  drivers/mfd/Makefile |   1 +
> > > >  drivers/mfd/mt6370.c | 349 +++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 363 insertions(+)
> > > >  create mode 100644 drivers/mfd/mt6370.c
> > > >
> > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > index 3b59456f5545..d9a7524a3e0e 100644
> > > > --- a/drivers/mfd/Kconfig
> > > > +++ b/drivers/mfd/Kconfig
> > > > @@ -937,6 +937,19 @@ config MFD_MT6360
> > > >         PMIC part includes 2-channel BUCKs and 2-channel LDOs
> > > >         LDO part includes 4-channel LDOs
> > > >
> > > > +config MFD_MT6370
> > > > +     tristate "Mediatek MT6370 SubPMIC"
> > > > +     select MFD_CORE
> > > > +     select REGMAP_I2C
> > > > +     select REGMAP_IRQ
> > > > +     depends on I2C
> > > > +     help
> > > > +       Say Y here to enable MT6370 SubPMIC functional support.
> > > > +       It integrate single cell battery charger with adc monitoring, RGB
> > >
> > > s/integrates/consists of a/
> > >
> > > "ADC"
> >
> > We will fine it in the next patch.
> >
> > >
> > > > +       LEDs, dual channel flashlight, WLED backlight driver, display bias
> > >
> > > > +       voltage supply, one general purpose LDO, and cc logic
> > > > +       controller with USBPD commmunication capable.
> > >
> > > The last part makes no sense - "and is USBPD"?
> >
> > If we modify this help text to
> > "one general purpose LDO, and the USB Type-C & PD controller complies
> > with the latest USB Type-C and PD standards",
> > did these modifications meet your expectations?
>
> "one general purpose LDO and a USB Type-C & PD controller that
> complies with the latest USB Type-C and PD standards"
>
> Better?

Yes, thanks! We will modify it like that in the next patch.

>
> > > >  config MFD_MT6397
> > > >       tristate "MediaTek MT6397 PMIC Support"
> > > >       select MFD_CORE
> > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > > index 858cacf659d6..62b27125420e 100644
> > > > --- a/drivers/mfd/Makefile
> > > > +++ b/drivers/mfd/Makefile
> > > > @@ -242,6 +242,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)        += intel_soc_pmic_bxtwc.o
> > > >  obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)   += intel_soc_pmic_chtwc.o
> > > >  obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)        += intel_soc_pmic_chtdc_ti.o
> > > >  obj-$(CONFIG_MFD_MT6360)     += mt6360-core.o
> > > > +obj-$(CONFIG_MFD_MT6370)     += mt6370.o
> > > >  mt6397-objs                  := mt6397-core.o mt6397-irq.o mt6358-irq.o
> > > >  obj-$(CONFIG_MFD_MT6397)     += mt6397.o
> > > >  obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)   += intel_soc_pmic_mrfld.o
> > > > diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c
> > > > new file mode 100644
> > > > index 000000000000..6af9f73c9c0c
> > > > --- /dev/null
> > > > +++ b/drivers/mfd/mt6370.c
> > > > @@ -0,0 +1,349 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +#include <linux/bits.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/mfd/core.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/regmap.h>
> > > > +
> > > > +enum {
> > > > +     MT6370_USBC_I2C = 0,
> > > > +     MT6370_PMU_I2C,
> > > > +     MT6370_MAX_I2C
> > > > +};
> > > > +
> > > > +#define MT6370_REG_DEV_INFO  0x100
> > > > +#define MT6370_REG_CHG_IRQ1  0x1C0
> > > > +#define MT6370_REG_CHG_MASK1 0x1E0
> > > > +
> > > > +#define MT6370_VENID_MASK    GENMASK(7, 4)
> > > > +
> > > > +#define MT6370_NUM_IRQREGS   16
> > > > +#define MT6370_USBC_I2CADDR  0x4E
> > > > +#define MT6370_REG_ADDRLEN   2
> > > > +#define MT6370_REG_MAXADDR   0x1FF
> > > > +
> > > > +/* IRQ definitions */
> > > > +#define MT6370_IRQ_DIRCHGON          0
> > > > +#define MT6370_IRQ_CHG_TREG          4
> > > > +#define MT6370_IRQ_CHG_AICR          5
> > > > +#define MT6370_IRQ_CHG_MIVR          6
> > > > +#define MT6370_IRQ_PWR_RDY           7
> > > > +#define MT6370_IRQ_FL_CHG_VINOVP     11
> > > > +#define MT6370_IRQ_CHG_VSYSUV                12
> > > > +#define MT6370_IRQ_CHG_VSYSOV                13
> > > > +#define MT6370_IRQ_CHG_VBATOV                14
> > > > +#define MT6370_IRQ_CHG_VINOVPCHG     15
> > > > +#define MT6370_IRQ_TS_BAT_COLD               20
> > > > +#define MT6370_IRQ_TS_BAT_COOL               21
> > > > +#define MT6370_IRQ_TS_BAT_WARM               22
> > > > +#define MT6370_IRQ_TS_BAT_HOT                23
> > > > +#define MT6370_IRQ_TS_STATC          24
> > > > +#define MT6370_IRQ_CHG_FAULT         25
> > > > +#define MT6370_IRQ_CHG_STATC         26
> > > > +#define MT6370_IRQ_CHG_TMR           27
> > > > +#define MT6370_IRQ_CHG_BATABS                28
> > > > +#define MT6370_IRQ_CHG_ADPBAD                29
> > > > +#define MT6370_IRQ_CHG_RVP           30
> > > > +#define MT6370_IRQ_TSHUTDOWN         31
> > > > +#define MT6370_IRQ_CHG_IINMEAS               32
> > > > +#define MT6370_IRQ_CHG_ICCMEAS               33
> > > > +#define MT6370_IRQ_CHGDET_DONE               34
> > > > +#define MT6370_IRQ_WDTMR             35
> > > > +#define MT6370_IRQ_SSFINISH          36
> > > > +#define MT6370_IRQ_CHG_RECHG         37
> > > > +#define MT6370_IRQ_CHG_TERM          38
> > > > +#define MT6370_IRQ_CHG_IEOC          39
> > > > +#define MT6370_IRQ_ADC_DONE          40
> > > > +#define MT6370_IRQ_PUMPX_DONE                41
> > > > +#define MT6370_IRQ_BST_BATUV         45
> > > > +#define MT6370_IRQ_BST_MIDOV         46
> > > > +#define MT6370_IRQ_BST_OLP           47
> > > > +#define MT6370_IRQ_ATTACH            48
> > > > +#define MT6370_IRQ_DETACH            49
> > > > +#define MT6370_IRQ_HVDCP_STPDONE     51
> > > > +#define MT6370_IRQ_HVDCP_VBUSDET_DONE        52
> > > > +#define MT6370_IRQ_HVDCP_DET         53
> > > > +#define MT6370_IRQ_CHGDET            54
> > > > +#define MT6370_IRQ_DCDT                      55
> > > > +#define MT6370_IRQ_DIRCHG_VGOK               59
> > > > +#define MT6370_IRQ_DIRCHG_WDTMR              60
> > > > +#define MT6370_IRQ_DIRCHG_UC         61
> > > > +#define MT6370_IRQ_DIRCHG_OC         62
> > > > +#define MT6370_IRQ_DIRCHG_OV         63
> > > > +#define MT6370_IRQ_OVPCTRL_SWON              67
> > > > +#define MT6370_IRQ_OVPCTRL_UVP_D     68
> > > > +#define MT6370_IRQ_OVPCTRL_UVP               69
> > > > +#define MT6370_IRQ_OVPCTRL_OVP_D     70
> > > > +#define MT6370_IRQ_OVPCTRL_OVP               71
> > > > +#define MT6370_IRQ_FLED_STRBPIN              72
> > > > +#define MT6370_IRQ_FLED_TORPIN               73
> > > > +#define MT6370_IRQ_FLED_TX           74
> > > > +#define MT6370_IRQ_FLED_LVF          75
> > > > +#define MT6370_IRQ_FLED2_SHORT               78
> > > > +#define MT6370_IRQ_FLED1_SHORT               79
> > > > +#define MT6370_IRQ_FLED2_STRB                80
> > > > +#define MT6370_IRQ_FLED1_STRB                81
> > > > +#define mT6370_IRQ_FLED2_STRB_TO     82
> > > > +#define MT6370_IRQ_FLED1_STRB_TO     83
> > > > +#define MT6370_IRQ_FLED2_TOR         84
> > > > +#define MT6370_IRQ_FLED1_TOR         85
> > > > +#define MT6370_IRQ_OTP                       93
> > > > +#define MT6370_IRQ_VDDA_OVP          94
> > > > +#define MT6370_IRQ_VDDA_UV           95
> > > > +#define MT6370_IRQ_LDO_OC            103
> > > > +#define MT6370_IRQ_BLED_OCP          118
> > > > +#define MT6370_IRQ_BLED_OVP          119
> > > > +#define MT6370_IRQ_DSV_VNEG_OCP              123
> > > > +#define MT6370_IRQ_DSV_VPOS_OCP              124
> > > > +#define MT6370_IRQ_DSV_BST_OCP               125
> > > > +#define MT6370_IRQ_DSV_VNEG_SCP              126
> > > > +#define MT6370_IRQ_DSV_VPOS_SCP              127
> > > > +
> > > > +struct mt6370_info {
> > > > +     struct i2c_client *i2c[MT6370_MAX_I2C];
> > > > +     struct device *dev;
> > > > +     struct regmap *regmap;
> > > > +     struct regmap_irq_chip_data *irq_data;
> > > > +};
> > >
> > > Can we shove all of the above into a header file?
> >
> > Well... In Patch v1, we put these "#define IRQ" into
> > "include/dt-bindings/mfd/mediatek,mt6370.h".
> > But the reviewer of DT files hoped us to remove this header file, we
> > put these "#define IRQ" in this .c file.
> > Shall we leave them here or put them into the header file in
> > "driver/power/supply/mt6370-charger.h"?
>
> Where are they used?

Sorry, I wrote the wrong path last time...
What I should say last time was to put them into the header file into
"driver/mfd/mt6370.h"
These "#define IRQ" are just used in "driver/mfd/mt6370.c"
I’m really sorry for making this mistake...

>
> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Best regards,
ChiaEn Wu
Lee Jones June 27, 2022, 4:28 p.m. UTC | #10
[...]

> > > > > +#define MT6370_IRQ_DSV_VPOS_OCP              124
> > > > > +#define MT6370_IRQ_DSV_BST_OCP               125
> > > > > +#define MT6370_IRQ_DSV_VNEG_SCP              126
> > > > > +#define MT6370_IRQ_DSV_VPOS_SCP              127
> > > > > +
> > > > > +struct mt6370_info {
> > > > > +     struct i2c_client *i2c[MT6370_MAX_I2C];
> > > > > +     struct device *dev;
> > > > > +     struct regmap *regmap;
> > > > > +     struct regmap_irq_chip_data *irq_data;
> > > > > +};
> > > >
> > > > Can we shove all of the above into a header file?
> > >
> > > Well... In Patch v1, we put these "#define IRQ" into
> > > "include/dt-bindings/mfd/mediatek,mt6370.h".
> > > But the reviewer of DT files hoped us to remove this header file, we
> > > put these "#define IRQ" in this .c file.
> > > Shall we leave them here or put them into the header file in
> > > "driver/power/supply/mt6370-charger.h"?
> >
> > Where are they used?
> 
> Sorry, I wrote the wrong path last time...
> What I should say last time was to put them into the header file into
> "driver/mfd/mt6370.h"
> These "#define IRQ" are just used in "driver/mfd/mt6370.c"
> I’m really sorry for making this mistake...

Yes, that would be fine.