| Message ID | 20220613111146.25221-1-peterwu.pub@gmail.com |
|---|---|
| Headers | show |
| Series | Add Mediatek MT6370 PMIC support | expand |
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.
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.
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.
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.
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");
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
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
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?
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
[...] > > > > > +#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.
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