| Message ID | 20220722102407.2205-1-peterwu.pub@gmail.com |
|---|---|
| Headers | show |
| Series | Add MediaTek MT6370 PMIC support | expand |
Il 22/07/22 12:24, ChiaEn Wu ha scritto: > From: ChiYuan Huang <cy_huang@richtek.com> > > The MediaTek 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. > > In MediaTek MT6370, there are four channel current-sink RGB LEDs that > support hardware pattern for constant current, PWM, and breath mode. > Isink4 channel can also be used as a CHG_VIN power good indicator. > > Signed-off-by: Alice Chen <alice_chen@richtek.com> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > > v6 > - Remove the 'ko' from mt6370 led Kconfig description. > - Add both authors for Alice and ChiYuan. > - Use pdata to distinguish the code from mt6370/71 to mt6372. > - Instead of 'state' define, use the 'state' enum. > - Fix the typo for 'MT6372_PMW_DUTY'. > - For pwm_duty define, replace with bit macro - 1. > - Refine all the labels from 'out' to 'out_unlock'. > - Use struct 'dev' variable and 'dev_err_probe' to optimize the LOC. > - Revise for the array initialization from {0} to {}. > - Move into rgb folder and rename file name to 'leds-mt6370-rgb'. > - Refine the 'comma' usage in struct/enum. > --- > drivers/leds/rgb/Kconfig | 13 + > drivers/leds/rgb/Makefile | 1 + > drivers/leds/rgb/leds-mt6370-rgb.c | 1004 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 1018 insertions(+) > create mode 100644 drivers/leds/rgb/leds-mt6370-rgb.c >
On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiYuan Huang <cy_huang@richtek.com> > > This adds support for the MediaTek MT6370 SubPMIC. MediaTek MT6370 is a > SubPMIC consisting of a single cell battery charger with ADC monitoring, > RGB LEDs, dual channel flashlight, WLED backlight driver, display bias > voltage supply, one general purpose LDO, and the USB Type-C & PD controller > complies with the latest USB Type-C and PD standards. ... > +#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 These two more logically to have near to other _REG_* definitions above.
On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiaEn Wu <chiaen_wu@richtek.com> > > MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger > with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight > driver, display bias voltage supply, one general purpose LDO, and the > USB Type-C & PD controller complies with the latest USB Type-C and PD > standards. > > Add a support the MT6370 ADC driver for system monitoring, including support for the > charger current, voltage, and temperature. ... > +#define MT6370_AICR_400_mA 0x6 > +#define MT6370_ICHG_500_mA 0x4 > +#define MT6370_ICHG_900_mA 0x8 ^^^^ (Note this and read below) ... > + reg_val = FIELD_GET(MT6370_AICR_ICHG_MASK, reg_val); > + if (reg_val < MT6370_AICR_400_mA) > + *val1 = 3350; > + else > + *val1 = 5000; Here... ... > + if (reg_val < MT6370_ICHG_500_mA) > + *val1 = 2375; > + else if (reg_val >= MT6370_ICHG_500_mA && > + reg_val < MT6370_ICHG_900_mA) > + *val1 = 2680; > + else > + *val1 = 5000; ...and especially here it is so counterintuitive to have an if-else chain because the values are not ordered by semantic meaning. What if the new standard/hardware decides to use 0x7 for 100mA (hypothetically)? So, please use switch cases or other robust methods.
On Mon, Jul 25, 2022 at 4:00 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > ... > > > +#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 > > These two more logically to have near to other _REG_* definitions above. Hi Andy, Thanks for your review. Do you mean that we should move '#define MT6370_USBC_I2CADDR' and '#define MT6370_REG_MAXADDR' after the line '#define MT6370_REG_CHG_MASK1'? ------------------------------------------------------------------- #define MT6370_REG_DEV_INFO 0x100 #define MT6370_REG_CHG_IRQ1 0x1C0 #define MT6370_REG_CHG_MASK1 0x1E0 #define MT6370_USBC_I2CADDR 0x4E #define MT6370_REG_MAXADDR 0x1FF #define MT6370_VENID_MASK GENMASK(7, 4) #define MT6370_NUM_IRQREGS 16 #define MT6370_REG_ADDRLEN 2 ------------------------------------------------------------------- Like this? > > -- > With Best Regards, > Andy Shevchenko
On Mon, Jul 25, 2022 at 10:30 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > On Mon, Jul 25, 2022 at 4:00 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: ... > > > +#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 > > > > These two more logically to have near to other _REG_* definitions above. > > Hi Andy, > Thanks for your review. > Do you mean that we should move '#define MT6370_USBC_I2CADDR' and > '#define MT6370_REG_MAXADDR' after the line '#define > MT6370_REG_CHG_MASK1'? > ------------------------------------------------------------------- > #define MT6370_REG_DEV_INFO 0x100 > #define MT6370_REG_CHG_IRQ1 0x1C0 > #define MT6370_REG_CHG_MASK1 0x1E0 > #define MT6370_USBC_I2CADDR 0x4E > #define MT6370_REG_MAXADDR 0x1FF > > #define MT6370_VENID_MASK GENMASK(7, 4) > > #define MT6370_NUM_IRQREGS 16 > #define MT6370_REG_ADDRLEN 2 > ------------------------------------------------------------------- > Like this? You lost me. Namespace has a meaning, i.e. grouping items of a kind. In your proposal I don't see that. If REG_MAXADDR and REG_ADDRLEN are _not_ of the _REG_ kind as per above, why do they have this namespace in the first place?
On Mon, Jul 25, 2022 at 4:43 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > ... > > > > > +#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 > > > > > > These two more logically to have near to other _REG_* definitions above. > > ... > > You lost me. Namespace has a meaning, i.e. grouping items of a kind. > In your proposal I don't see that. If REG_MAXADDR and REG_ADDRLEN are > _not_ of the _REG_ kind as per above, why do they have this namespace > in the first place? oh... Sorry, I just got the wrong meaning maybe it should be revised like this, right?? ------------------------------------------------------------------- #define MT6370_REG_DEV_INFO 0x100 #define MT6370_REG_CHG_IRQ1 0x1C0 #define MT6370_REG_CHG_MASK1 0x1E0 #define MT6370_REG_MAXADDR 0x1FF // Move it to here #define MT6370_VENID_MASK GENMASK(7, 4) #define MT6370_NUM_IRQREGS 16 #define MT6370_USBC_I2CADDR 0x4E #define MT6370_MAX_ADDRLEN 2 // Rename ------------------------------------------------------------------- Thanks!
On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiaEn Wu <chiaen_wu@richtek.com> > > MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger > with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight > driver, display bias voltage supply, one general purpose LDO, and the > USB Type-C & PD controller complies with the latest USB Type-C and PD > standards. > > This adds support for MediaTek MT6370 Backlight driver. It's commonly used Read Submitting Patches, please! (In this case, find "This patch" in the above mentioned document, read and act accordingly) > to drive the display WLED. There are 4 channels inside, and each channel > supports up to 30mA of current capability with 2048 current steps in > exponential or linear mapping curves. ... > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); (see below) ... > + /* > + * To make MT6372 using 14 bits to control the brightness > + * backward compatible with 11 bits brightness control > + * (like MT6370 and MT6371 do), we left shift the value > + * and pad with 1 to remaining bits. Hence, the MT6372's to the remaining > + * backlight brightness will be almost the same as MT6370's > + * and MT6371's. > + */ > + if (priv->vid_type == MT6370_VID_6372) { > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > + } Nice! Why not... ... > + gpiod_set_value(priv->enable_gpio, brightness ? 1 : 0); !!brightness will do as well. ... > + brightness = brightness_val[1] << fls(MT6370_BL_DIM2_MASK); > + val |= prop_val << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1); > + val |= ovp_uV << (ffs(MT6370_BL_OVP_SEL_MASK) - 1); > + val |= ocp_uA << (ffs(MT6370_BL_OC_SEL_MASK) - 1); > + val = prop_val << (ffs(MT6370_BL_CH_MASK) - 1); ...to use respective _SHIFTs in all these? ... > + priv->enable_gpio = devm_gpiod_get_optional(dev, "enable", > + GPIOD_OUT_HIGH); > + if (IS_ERR(priv->enable_gpio)) > + dev_err(dev, "Failed to get 'enable' gpio\n"); What does this mean? Shouldn't be return dev_err_probe()?
On Mon, Jul 25, 2022 at 11:06 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > On Mon, Jul 25, 2022 at 4:43 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: ... > > > > > +#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 > > > > > > > > These two more logically to have near to other _REG_* definitions above. ... > > You lost me. Namespace has a meaning, i.e. grouping items of a kind. > > In your proposal I don't see that. If REG_MAXADDR and REG_ADDRLEN are > > _not_ of the _REG_ kind as per above, why do they have this namespace > > in the first place? > oh... Sorry, I just got the wrong meaning > maybe it should be revised like this, right?? I don't know. I am not an author of the code, I do not have access (and don't want to) to the hardware datasheets, all up to you. From the style perspective below looks good. > ------------------------------------------------------------------- > #define MT6370_REG_DEV_INFO 0x100 > #define MT6370_REG_CHG_IRQ1 0x1C0 > #define MT6370_REG_CHG_MASK1 0x1E0 > #define MT6370_REG_MAXADDR 0x1FF // Move it to here > > #define MT6370_VENID_MASK GENMASK(7, 4) > > #define MT6370_NUM_IRQREGS 16 > #define MT6370_USBC_I2CADDR 0x4E > > #define MT6370_MAX_ADDRLEN 2 // Rename
On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote: > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > index a003e02..846dbe7 100644 > --- a/drivers/video/backlight/Kconfig > +++ b/drivers/video/backlight/Kconfig > @@ -268,6 +268,18 @@ 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 are 4 channels > + inside, and each channel supports up to 30mA of current capability > + with 2048 current steps in exponential or linear mapping curves. Does the MT6372 support more steps than this? In other words does it use a fourteen bit scale or does it use an 11-bit scale at a different register location? > + > + This driver can also be built as a module. If so, the module > + will be called "mt6370-backlight". > + > [...] > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c > new file mode 100644 > index 0000000..ba00a8f > --- /dev/null > +++ b/drivers/video/backlight/mt6370-backlight.c > [...] > +static int mt6370_bl_update_status(struct backlight_device *bl_dev) > +{ > + struct mt6370_priv *priv = bl_get_data(bl_dev); > + int brightness = backlight_get_brightness(bl_dev); > + unsigned int enable_val; > + u8 brightness_val[2]; > + int ret; > + > + if (brightness) { > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > + > + /* > + * To make MT6372 using 14 bits to control the brightness > + * backward compatible with 11 bits brightness control > + * (like MT6370 and MT6371 do), we left shift the value > + * and pad with 1 to remaining bits. Hence, the MT6372's > + * backlight brightness will be almost the same as MT6370's > + * and MT6371's. > + */ > + if (priv->vid_type == MT6370_VID_6372) { > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > + } This somewhat depends on the answer to the first question above, but what is the point of this shifting? If the range is 14-bit then the driver should set max_brightness to 16384 and present the full range of the MT6372 to the user. Especially when using linear mappings (which are a totally pointless scale to use for a backlight) the extra steps are useful for backlight animation. Daniel.
On Mon, Jul 25, 2022 at 6:31 PM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote: > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > > index a003e02..846dbe7 100644 > > --- a/drivers/video/backlight/Kconfig > > +++ b/drivers/video/backlight/Kconfig > > @@ -268,6 +268,18 @@ 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 are 4 channels > > + inside, and each channel supports up to 30mA of current capability > > + with 2048 current steps in exponential or linear mapping curves. > > Does the MT6372 support more steps than this? In other words does it use > a fourteen bit scale or does it use an 11-bit scale at a different > register location? Hi Daniel, Thanks for your reply. Yes, MT6372 can support 16384 steps and uses a 14-bit scale register location. But the maximum current of each channel of MT6372 is the same as MT6370 and MT6371, both 30mA. The main reason why MT6372 is designed this way is that one of the customers asked for a more delicate adjustment of the backlight brightness. But other customers actually do not have such requirements. Therefore, we designed it this way for maximum compatibility in software. > > > > + > > + This driver can also be built as a module. If so, the module > > + will be called "mt6370-backlight". > > + > > [...] > > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c > > new file mode 100644 > > index 0000000..ba00a8f > > --- /dev/null > > +++ b/drivers/video/backlight/mt6370-backlight.c > > [...] > > +static int mt6370_bl_update_status(struct backlight_device *bl_dev) > > +{ > > + struct mt6370_priv *priv = bl_get_data(bl_dev); > > + int brightness = backlight_get_brightness(bl_dev); > > + unsigned int enable_val; > > + u8 brightness_val[2]; > > + int ret; > > + > > + if (brightness) { > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > + > > + /* > > + * To make MT6372 using 14 bits to control the brightness > > + * backward compatible with 11 bits brightness control > > + * (like MT6370 and MT6371 do), we left shift the value > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > + * backlight brightness will be almost the same as MT6370's > > + * and MT6371's. > > + */ > > + if (priv->vid_type == MT6370_VID_6372) { > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > + } > > This somewhat depends on the answer to the first question above, but > what is the point of this shifting? If the range is 14-bit then the > driver should set max_brightness to 16384 and present the full range of > the MT6372 to the user. So should we make all 16384 steps of MT6372 available to users? Does that mean the DTS needs to be modified as well? Or, for the reasons, I have just explained (just one customer has this requirement), then we do not make any changes for compatibility reasons? Thanks. > > Especially when using linear mappings (which are a totally pointless > scale to use for a backlight) the extra steps are useful for backlight > animation. > > > Daniel.
On Tue, Jul 26, 2022 at 10:20:02AM +0800, ChiaEn Wu wrote: > On Mon, Jul 25, 2022 at 6:31 PM Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote: > > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > > > index a003e02..846dbe7 100644 > > > --- a/drivers/video/backlight/Kconfig > > > +++ b/drivers/video/backlight/Kconfig > > > @@ -268,6 +268,18 @@ 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 are 4 channels > > > + inside, and each channel supports up to 30mA of current capability > > > + with 2048 current steps in exponential or linear mapping curves. > > > > Does the MT6372 support more steps than this? In other words does it use > > a fourteen bit scale or does it use an 11-bit scale at a different > > register location? > > Hi Daniel, > > Thanks for your reply. > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > location. But the maximum current of each > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > The main reason why MT6372 is designed this way is that one of the > customers asked for a more delicate > adjustment of the backlight brightness. But other customers actually > do not have such requirements. > Therefore, we designed it this way for maximum compatibility in software. I don't think that is an acceptable approach for the upstream kernel. To be "compatible" with (broken) software this driver ends up reducing the capability of the upstream kernel to the point it becomes unable to meet requirements for delicate adjustment (requirements that were sufficiently important to change the hardware design so you could meet them). > > > + > > > + This driver can also be built as a module. If so, the module > > > + will be called "mt6370-backlight". > > > + > > > [...] > > > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c > > > new file mode 100644 > > > index 0000000..ba00a8f > > > --- /dev/null > > > +++ b/drivers/video/backlight/mt6370-backlight.c > > > [...] > > > +static int mt6370_bl_update_status(struct backlight_device *bl_dev) > > > +{ > > > + struct mt6370_priv *priv = bl_get_data(bl_dev); > > > + int brightness = backlight_get_brightness(bl_dev); > > > + unsigned int enable_val; > > > + u8 brightness_val[2]; > > > + int ret; > > > + > > > + if (brightness) { > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > + > > > + /* > > > + * To make MT6372 using 14 bits to control the brightness > > > + * backward compatible with 11 bits brightness control > > > + * (like MT6370 and MT6371 do), we left shift the value > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > + * backlight brightness will be almost the same as MT6370's > > > + * and MT6371's. > > > + */ > > > + if (priv->vid_type == MT6370_VID_6372) { > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > + } > > > > This somewhat depends on the answer to the first question above, but > > what is the point of this shifting? If the range is 14-bit then the > > driver should set max_brightness to 16384 and present the full range of > > the MT6372 to the user. > > So should we make all 16384 steps of MT6372 available to users? Yes. > Does that mean the DTS needs to be modified as well? Yes... the property to set initial brightness needs a 14-bit range. It would also be a good idea to discuss with the DT maintainers whether you should introduce a second compatible string (ending 6372) in order to allow the DT validation checks to detect accidental use of MT6372 ranges on MT6370 hardware. > Or, for the reasons, I have just explained (just one customer has this > requirement), then we do not make any changes for compatibility > reasons? I'd be curious what the compatiblity reasons are. In other words what software breaks? Normally the userspace backlight code reads the max_brightness property and configures things accordingly (and therefore if you the component that breaks is something like an Android HAL then fix the HAL instead). Daniel.
On Tue, Jul 26, 2022 at 5:31 PM Daniel Thompson <daniel.thompson@linaro.org> wrote: ... > > > Does the MT6372 support more steps than this? In other words does it use > > > a fourteen bit scale or does it use an 11-bit scale at a different > > > register location? > > > > Hi Daniel, > > > > Thanks for your reply. > > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > > location. But the maximum current of each > > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > > The main reason why MT6372 is designed this way is that one of the > > customers asked for a more delicate > > adjustment of the backlight brightness. But other customers actually > > do not have such requirements. > > Therefore, we designed it this way for maximum compatibility in software. Sorry for I used of the wrong word, I mean is 'driver', not higher-level software > > I don't think that is an acceptable approach for the upstream kernel. > > To be "compatible" with (broken) software this driver ends up reducing > the capability of the upstream kernel to the point it becomes unable to > meet requirements for delicate adjustment (requirements that were > sufficiently important to change the hardware design so you could meet > them). Originally, we just wanted to use one version of the driver to cover all the SubPMIC of the 6370 series(6370~6372). And, the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. To achieve this goal, we have designed it to look like this. > > ... > > > > + > > > > + if (brightness) { > > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > > + > > > > + /* > > > > + * To make MT6372 using 14 bits to control the brightness > > > > + * backward compatible with 11 bits brightness control > > > > + * (like MT6370 and MT6371 do), we left shift the value > > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > > + * backlight brightness will be almost the same as MT6370's > > > > + * and MT6371's. > > > > + */ > > > > + if (priv->vid_type == MT6370_VID_6372) { > > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > > + } > > > > > > This somewhat depends on the answer to the first question above, but > > > what is the point of this shifting? If the range is 14-bit then the > > > driver should set max_brightness to 16384 and present the full range of > > > the MT6372 to the user. > > > > So should we make all 16384 steps of MT6372 available to users? > > Yes. > > > > Does that mean the DTS needs to be modified as well? > > Yes... the property to set initial brightness needs a 14-bit range. > > It would also be a good idea to discuss with the DT maintainers whether > you should introduce a second compatible string (ending 6372) in order > to allow the DT validation checks to detect accidental use of MT6372 > ranges on MT6370 hardware. hmmm... I have just thought about it, maybe I can just modify the maximum value of default-brightness and max-brightness in DT to 16384, modify the description and add some comments. And then on the driver side, we can use mt6370_check_vendor_info( ) to determine whether it is MT6372. If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set. In mt6370_bl_get_brightness(), first brightness_val * 8 and then return; If I do this change, does this meet your requirements? > > > > Or, for the reasons, I have just explained (just one customer has this > > requirement), then we do not make any changes for compatibility > > reasons? > > I'd be curious what the compatiblity reasons are. In other words what > software breaks? The reason is as above. We just hope the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. Not software breaks. Thanks! > > Normally the userspace backlight code reads the max_brightness property > and configures things accordingly (and therefore if you the component > that breaks is something like an Android HAL then fix the HAL instead). > > > Daniel.
On Tue, Jul 26, 2022 at 07:28:48PM +0800, ChiaEn Wu wrote: > On Tue, Jul 26, 2022 at 5:31 PM Daniel Thompson > <daniel.thompson@linaro.org> wrote: > ... > > > > Does the MT6372 support more steps than this? In other words does it use > > > > a fourteen bit scale or does it use an 11-bit scale at a different > > > > register location? > > > > > > Hi Daniel, > > > > > > Thanks for your reply. > > > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > > > location. But the maximum current of each > > > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > > > The main reason why MT6372 is designed this way is that one of the > > > customers asked for a more delicate > > > adjustment of the backlight brightness. But other customers actually > > > do not have such requirements. > > > Therefore, we designed it this way for maximum compatibility in software. > > Sorry for I used of the wrong word, I mean is 'driver', not > higher-level software > > > > > I don't think that is an acceptable approach for the upstream kernel. > > > > To be "compatible" with (broken) software this driver ends up reducing > > the capability of the upstream kernel to the point it becomes unable to > > meet requirements for delicate adjustment (requirements that were > > sufficiently important to change the hardware design so you could meet > > them). > > Originally, we just wanted to use one version of the driver to cover > all the SubPMIC of the 6370 series(6370~6372). > And, the users who use this series SubPMIC can directly apply this > driver to drive the backlight device without knowing the underlying > hardware. > To achieve this goal, we have designed it to look like this. You don't need a second driver to support two different values for max-brightness. The same driver can support both ranges with nothing but a small tweak during the driver probe function. > ... > > > > > + > > > > > + if (brightness) { > > > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > > > + > > > > > + /* > > > > > + * To make MT6372 using 14 bits to control the brightness > > > > > + * backward compatible with 11 bits brightness control > > > > > + * (like MT6370 and MT6371 do), we left shift the value > > > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > > > + * backlight brightness will be almost the same as MT6370's > > > > > + * and MT6371's. > > > > > + */ > > > > > + if (priv->vid_type == MT6370_VID_6372) { > > > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > > > + } > > > > > > > > This somewhat depends on the answer to the first question above, but > > > > what is the point of this shifting? If the range is 14-bit then the > > > > driver should set max_brightness to 16384 and present the full range of > > > > the MT6372 to the user. > > > > > > So should we make all 16384 steps of MT6372 available to users? > > > > Yes. > > > > > > > Does that mean the DTS needs to be modified as well? > > > > Yes... the property to set initial brightness needs a 14-bit range. > > > > It would also be a good idea to discuss with the DT maintainers whether > > you should introduce a second compatible string (ending 6372) in order > > to allow the DT validation checks to detect accidental use of MT6372 > > ranges on MT6370 hardware. > > hmmm... I have just thought about it, > maybe I can just modify the maximum value of default-brightness and > max-brightness in DT to 16384, > modify the description and add some comments. What for? All the other backlight drivers (there are >130 of them) expose the hardware range[1]. Most userspaces will already know how to handle that (by reading the max_brightness and, if it is recent enough, also the scale properties in sysfs). I'm still don't understand why one should fix a bug in the userspace by implementing a hack in the driver. [1] Well almost. The PWM backlight driver does contain support for dead-spot avoidance and to allow the adoption of exponential scale. However this purpose of these is based on how PWM backlights work > And then on the driver side, > we can use mt6370_check_vendor_info( ) to determine whether it is MT6372. > If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set. > In mt6370_bl_get_brightness(), first brightness_val * 8 and then return; > > If I do this change, does this meet your requirements? Not really. It's still misleading a sophisticated userspace, which may make bad rounding decisions for backlight animation, in order to support a broken one. > > > Or, for the reasons, I have just explained (just one customer has this > > > requirement), then we do not make any changes for compatibility > > > reasons? > > > > I'd be curious what the compatiblity reasons are. In other words what > > software breaks? > > The reason is as above. We just hope the users who use this series SubPMIC can > directly apply this driver to drive the backlight device without > knowing the underlying hardware. > Not software breaks. As above, ignoring the max_brightness property is a bug in the userspace. I'm still unclear why sending faked ranges to userspace it a better solution than fixing the userspace. Daniel.
On Tue, Jul 26, 2022 at 7:59 PM Daniel Thompson <daniel.thompson@linaro.org> wrote: ... > > > > > Does the MT6372 support more steps than this? In other words does it use > > > > > a fourteen bit scale or does it use an 11-bit scale at a different > > > > > register location? > > > > > > > > Hi Daniel, > > > > > > > > Thanks for your reply. > > > > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > > > > location. But the maximum current of each > > > > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > > > > The main reason why MT6372 is designed this way is that one of the > > > > customers asked for a more delicate > > > > adjustment of the backlight brightness. But other customers actually > > > > do not have such requirements. > > > > Therefore, we designed it this way for maximum compatibility in software. > > > > Sorry for I used of the wrong word, I mean is 'driver', not > > higher-level software > > > > > > > > I don't think that is an acceptable approach for the upstream kernel. > > > > > > To be "compatible" with (broken) software this driver ends up reducing > > > the capability of the upstream kernel to the point it becomes unable to > > > meet requirements for delicate adjustment (requirements that were > > > sufficiently important to change the hardware design so you could meet > > > them). > > > > Originally, we just wanted to use one version of the driver to cover > > all the SubPMIC of the 6370 series(6370~6372). > > And, the users who use this series SubPMIC can directly apply this > > driver to drive the backlight device without knowing the underlying > > hardware. > > To achieve this goal, we have designed it to look like this. > > You don't need a second driver to support two different values for > max-brightness. The same driver can support both ranges with nothing but > a small tweak during the driver probe function. > > > > ... > > > > > > + > > > > > > + if (brightness) { > > > > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > > > > + > > > > > > + /* > > > > > > + * To make MT6372 using 14 bits to control the brightness > > > > > > + * backward compatible with 11 bits brightness control > > > > > > + * (like MT6370 and MT6371 do), we left shift the value > > > > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > > > > + * backlight brightness will be almost the same as MT6370's > > > > > > + * and MT6371's. > > > > > > + */ > > > > > > + if (priv->vid_type == MT6370_VID_6372) { > > > > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > > > > + } > > > > > > > > > > This somewhat depends on the answer to the first question above, but > > > > > what is the point of this shifting? If the range is 14-bit then the > > > > > driver should set max_brightness to 16384 and present the full range of > > > > > the MT6372 to the user. > > > > > > > > So should we make all 16384 steps of MT6372 available to users? > > > > > > Yes. > > > > > > > > > > Does that mean the DTS needs to be modified as well? > > > > > > Yes... the property to set initial brightness needs a 14-bit range. > > > > > > It would also be a good idea to discuss with the DT maintainers whether > > > you should introduce a second compatible string (ending 6372) in order > > > to allow the DT validation checks to detect accidental use of MT6372 > > > ranges on MT6370 hardware. > > > > hmmm... I have just thought about it, > > maybe I can just modify the maximum value of default-brightness and > > max-brightness in DT to 16384, > > modify the description and add some comments. > > What for? > > All the other backlight drivers (there are >130 of them) expose the hardware > range[1]. Most userspaces will already know how to handle that (by reading > the max_brightness and, if it is recent enough, also the scale > properties in sysfs). > > I'm still don't understand why one should fix a bug in the userspace by > implementing a hack in the driver. > > > [1] Well almost. The PWM backlight driver does contain support for > dead-spot avoidance and to allow the adoption of exponential scale. > However this purpose of these is based on how PWM backlights work > > > And then on the driver side, > > we can use mt6370_check_vendor_info() to determine whether it is MT6372. > > If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set. > > In mt6370_bl_get_brightness(), first brightness_val * 8 and then return; > > > > If I do this change, does this meet your requirements? > > Not really. > > It's still misleading a sophisticated userspace, which may make bad > rounding decisions for backlight animation, in order to support a broken > one. > > > > > > Or, for the reasons, I have just explained (just one customer has this > > > > requirement), then we do not make any changes for compatibility > > > > reasons? > > > > > > I'd be curious what the compatiblity reasons are. In other words what > > > software breaks? > > > > The reason is as above. We just hope the users who use this series SubPMIC can > > directly apply this driver to drive the backlight device without > > knowing the underlying hardware. > > Not software breaks. > > As above, ignoring the max_brightness property is a bug in the > userspace. I'm still unclear why sending faked ranges to userspace > it a better solution than fixing the userspace. Ok, I got it! If I add a second compatible string (like 'mediatek,mt6372-backlight') in the DT section, and append 'if-then-else' to determine the correct maximum value of 'default-brightness' and 'max-brightness', Also, I will append 'bled exponential mode' to make user control using linear or exponential mode. These changes I will explain to DT's maintainer again. Back to the driver section, do I still need to use the register to confirm again whether this SubPMIC used now is MT6372 and record this information? (using 'mt6370_check_vendor_info()') I am afraid that the user who uses the MT6370 hardware, but the DT compatible string is set to 'mediatek,mt6372-backlight'. This may cause errors when update/get brightness values. So I hope the driver here can check again to make sure the 'default-brightness', 'max-brightness', can be updated/got correctly. I don't know if this will make you feel redundant if I do this?? Thank you so much!
On Wed, Jul 27, 2022 at 02:24:46PM +0800, ChiaEn Wu wrote: > On Tue, Jul 26, 2022 at 7:59 PM Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > > So should we make all 16384 steps of MT6372 available to users? > > > > > > > > Yes. > > > > > > > > > > > > > Does that mean the DTS needs to be modified as well? > > > > > > > > Yes... the property to set initial brightness needs a 14-bit range. > > > > > > > > It would also be a good idea to discuss with the DT maintainers whether > > > > you should introduce a second compatible string (ending 6372) in order > > > > to allow the DT validation checks to detect accidental use of MT6372 > > > > ranges on MT6370 hardware. [snip] > > > > I'd be curious what the compatiblity reasons are. In other words what > > > > software breaks? > > > > > > The reason is as above. We just hope the users who use this series SubPMIC can > > > directly apply this driver to drive the backlight device without > > > knowing the underlying hardware. > > > Not software breaks. > > > > As above, ignoring the max_brightness property is a bug in the > > userspace. I'm still unclear why sending faked ranges to userspace > > it a better solution than fixing the userspace. > > Ok, I got it! > If I add a second compatible string (like 'mediatek,mt6372-backlight') > in the DT section, > and append 'if-then-else' to determine the correct maximum value of > 'default-brightness' and 'max-brightness', > Also, I will append 'bled exponential mode' to make user control using > linear or exponential mode. I'd be very pleased to see support for exponential mode added: it's a much better way to control LEDs and backlights. > These changes I will explain to DT's maintainer again. Excellent. I know DT maintainers are copied into this thread but they will probably not be following this patch's thread so it is better to discuss in the mail thread for the DT bindings! > Back to the driver section, > do I still need to use the register to confirm again whether this > SubPMIC used now is MT6372 and record this information? (using > 'mt6370_check_vendor_info()') > I am afraid that the user who uses the MT6370 hardware, but the DT > compatible string is set to 'mediatek,mt6372-backlight'. > This may cause errors when update/get brightness values. > So I hope the driver here can check again to make sure the > 'default-brightness', 'max-brightness', can be updated/got correctly. > I don't know if this will make you feel redundant if I do this?? Yes, it's good idea to check the hardware model during probe. I'd suggest just reporting this as an error ("Buggy DT, wrong compatible string") rather than trying to re-intepret the parameters. Daniel.
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. In this series of patches, we based on Andy Shevchenko's mfd patch used to adjust the Makefile order. (https://lore.kernel.org/all/20220616182524.7956-2-andriy.shevchenko@linux.intel.com/) Among with this, we took some changes for MT6370 and refined the MT6370 device tree files to comply with DT specifications. "[PATCH v6 06/13] dt-bindings: mfd: Add MediaTek MT6370" depends on previous DT binding patches, so before applying this patch, please apply other DT patches first. Thanks! Thank you, ChiaEn Wu --- Changes in v6: - In Patch 03/13: - Add 'reg' property of led of multi-led to prevent checking error. - In Patch 08/13: - Convert tcpci as device resource managed with 'devm_add_action_or_reset' api. - Refine remvoe callback. - Refine the commit text from 'this commit add' to 'add'. - In Patch 09/13: - Using 'struct device *dev = &pdev->dev' in probe() - Revise the sixth parameter of regmap_read_poll_timeout() by replacing '1000' with 'MILLI' - Revise the units of three macros - MT6370_AICR_400MA --> MT6370_AICR_400_mA - MT6370_ICHG_500MA --> MT6370_ICHG_500_mA - MT6370_ICHG_900MA --> MT6370_ICHG_900_mA - In patch 10/13: - Remove the varable (*psy_desc) of struct mt6370_priv - Remove the deprecated usb type (POWER_SUPPLY_TYPE_USB_CDP and POWER_SUPPLY_TYPE_USB_DCP) - Remove useless remove() - Revise all units from mini- to micro- - Revise get/set power_supply_prop (change to directly return get/set regmap_field) - Revise probe() and use devm_add_action_or_reset() for handling of the workqueue/delayed_work/mutex - Revise mt6370_chg_psy_desc - Add '.name = "mt6370-charger"' - Use 'static const' - In patch 11/13: - Remove the 'ko' from mt6370 led Kconfig description. - Add both authors for Alice and ChiYuan. - Use pdata to distinguish the code from mt6370/71 to mt6372. - Instead of 'state' define, use the 'state' enum. - Fix the typo for 'MT6372_PMW_DUTY'. - For pwm_duty define, replace with bit macro - 1. - Refine all the labels from 'out' to 'out_unlock'. - Use struct 'dev' variable and 'dev_err_probe' to optimize the LOC. - Revise for the array initialization from {0} to {}. - Move into rgb folder and rename file name to 'leds-mt6370-rgb'. - Refine the 'comma' usage in struct/enum. - In patch 12/13: - Use 'GENMASK' instead of 'BIT'. - Use dev_err_probe to decrease LOC. - Use 'dev' variable to make probe function more clean. - Refine the return of _mt6370_flash_brightness_set function. - Refine the descriptions. - Use mt6370_clamp() instead of clamp_align(). - Use device resource managed API for v4l2 flash_release. Changes in v5: - In patch 07/13: - Add the comma in the last REGMAP_IRQ_REG_LINE(), DEFINE_RES_IRQ_NAMED() and MFD_CELL_RES() - Add the prefix in the first parameter of all mfd_cell - Move enum and struct mt6370_info to mt6370.h - Remove struct device *dev in struct mt6370_info - Revise the description of Kconfig help text - Revise MODULE_DESCRIPTION() - In patch 08/13: - Add comma for the last index of mt6370_reg_init. - Use dev_err_probe to decrease LOC. - Use 'dev' variable to make probe function more clean. - Refine kconfig text. - Remove both 'else' in set_vbus callback. - Remove comma for of_device_id if the assigned member is only one. - In patch 09/13: - Replace using snprintf() with sysfs_emit() in mt6370_adc_read_label() - Remove macro ADC_CONV_TIME_US - Revise all variable ordering - Revise the description of Kconfig help text - Revise MODULE_DESCRIPTION() - In patch 10/13: - Replace unsigned int type of pwr_rdy with bool in mt6370_chg_set_online() - Remove redundant 'else' in mt6370_chg_field_get() - Revise 'if-else' in mt6370_chg_field_set() - Revise 'if' condition in mt6370_chg_enable_irq() - Revise all text 'otg' --> 'OTG' - Revise MT6370_MIVR_IBUS_TH_100_MA --> MT6370_MIVR_IBUS_TH_100_mA - Revise the description of Kconfig help text - In patch 12/13: - Refine the coding style. - Use "dev" instead of "&pdev->dev". - In patch 13/13: - Add missed <mod_devicetable.h> - Add struct device *dev in probe() to make code cleaning - Remove useless including header file <gpio/driver.h>, <of.h> - Remove useless variable uasage in mt6370_init_backlight_properties() - Remove redundant checking enable_gpio in mt6370_bl_update_status() - Remove redundant parentheses in mt6370_bl_get_brightness() - Revise the description of Kconfig help text - Revise the calculation of hys_th_steps Changes in v4: - In patch 02/13: - Add minItems of "io-channel-names" - Replace text "Mediatek" with "MediaTek" - In patch 06/13: - Roll back all "$ref: " to v2 patch style (using "/schemas/...") - In patch 07/13: - Replace text "Mediatek" with "MediaTek" in Kconfig - Replace "first break and then return" with "return directly" in "mt6370_check_vendor_info()" - Add module name related description in Kconfig helptext - Add Copyright in the source code - Add header file "mt6370.h" for all "#define IRQ" - Adjust Makefile order of MT6370 - Refine "bank_idx" and "bank_addr" in "mt6375_regmap_read()" / "mt6375_regmap_write()" - Refine redundant "else if" in "mt6370_regmap_read()" - In patch 08/13: - Replace text "Mediatek" with "MediaTek" in Kconfig - Replace "first ret=regulator_(dis/en)able and then return" with "return directly" in "mt6370_tcpc_set_vbus()" - Replace header file <linux/of.h> with <linux/mod_devicetable.h> - Add Copyright in the source code - Add module name related description in Kconfig helptext - Remove header file <linux/of.h> - Refine all probe error by using dev_err_probe() - In patch 09/13: - Replace text "Mediatek" with "MediaTek" - Replace all "first dev_err() and then return" with "return dev_err_probe()" - Add Copyright in the source code - Add module name related description in Kconfig - Add unit suffix of macro "ADC_CONV_POLLING_TIME" - Add new macro "ADC_CONV_TIME_MS" - Adjust the position of include file <mediatek,mt6370_adc.h> - Adjust the postions between <linux/module.h> and <linux/mod_devicetable.h> - Fix some incorrect characters - In patch 10/13: - Replace text "Mediatek" with "MediaTek" in Kconfig and MODULE_DESCRIPTION() - Replace "mt6370_chg_val_to_reg" and "mt6370_chg_reg_to_val" with "linear_range" API - Replace "first break and then return" with "return directly" in all cases of get/set power_supply_property - Replace all "first dev_err() and then return" with "return dev_err_probe()" - Replace all "return IS_ERR(priv->rdev) ? PTR_ERR(priv->rdev) : 0" with "PTR_ERR_OR_ZERO()" - Replace "priv->dev->of_node" with "dev_of_node()" - Add Copyright in the source code - Add module name related description in Kconfig helptext - Add proper unit of "MT6370_MIVR_IBUS_TH" - Add error check in "mt6370_chg_get_status" - Remove including <mediatek,mt6370_adc.h> header file - Remove redundant comma of every enum terminator line - Remove unwanted blank lines - Remove the useless label (toggle_cfo_exit:) - Remove using atomic - Remove using of_match_ptr() - Fix some incorrect characters - Fix updating wrong bits when using ena_gpiod of OTG regulator - Adjust the probe order in probe() - In patch 11/13: - Replace text "Mediatek" with "MediaTek" in Kconfig - Replace text "const" with "constant" in Kconfig - Add Copyright in the source code - In patch 12/13: - Replace text "Mediatek" with "MediaTek" in Kconfig - Add Copyright in the source code - In patch 13/13: - Replace text "Mediatek" with "MediaTek" in Kconfig - Add Copyright in the source code - Revise the comment of "PWM HYS STEPS" Changes in v3: - Remove ADC ABI file, which is added in v2 Patch 7 - In patch 02/14: - Add items and remove maxItems of io-channels - Add io-channel-names and describe each item - Add "unevaluatedProperties: false" in "usb-otg-vbus-regulator" - Rename "enable-gpio" to "enable-gpios" in "usb-otg-vbus-regulator" - In patch 03/14: - Use leds-class-multicolor.yaml instead of common.yaml. - Split multi-led and led node. - Add subdevice "led" in "multi-led". - In patch 04/14: - Remove the description of enum. - In patch 05/14: - Rename "mediatek,bled-pwm-hys-input-threshold-steps" to "mediatek,bled-pwm-hys-input-th-steps" - Refine "bled-pwm-hys-input-th-steps", "bled-ovp-microvolt", "bled-ocp-microamp" enum values - In patch 06/14: - Use " in entire patchset - Refine ADC description - Rename "enable-gpio" to "enable-gpios" in "regualtor" - In patch 07/14: - Refine Kconfig help text - Refine error message of unknown vendor ID in mt6370_check_vendor_info() - Refine return value handling of mt6370_regmap_read() - Refine all probe error by using dev_err_probe() - Refine "bank_idx" and "bank_addr" in mt6370_regmap_read() and mt6370_regmap_write() - Add "#define VENID*" and drop the comments in mt6370_check_vendor_info() - Drop "MFD" in MODULE_DESCRIPTION() - In patch 09/14: - Refine Kconfig help text - In patch 10/14: - Refine Kconfig help text - Refine all channel value in read_scale() a. current: uA --> mA b. voltage: uV --> mV c. temperature: degrees Celsius --> milli degrees Celsius - Add "default:" condition of switch statement in read_scale() and read_raw() - Add error message for reading ADC register failed - Add the comment for adc_lock - Add <linux/mod_devicetable.h> header file for struct of_device_id - Replace "adc" text with "ADC" in all of the error messages - In patch 12/14: - Refine the grammer of the Kconfig. - Change reg mode to the const current mode. - In patch 14/14: - Refine bool properties parsing (pwm-enable, ovp-shutdown, ocp-shutdown) in DT parsing function - Refine u32 and u8 properties parsing (pwm-hys-input-th-steps, ovp-microvolt, ocp-microamp), from using register value to using actual value - Refine error string of "channle-use" parsing failed - Refine Kconfig help text 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: flash: mt6370: Add MediaTek MT6370 flashlight support ChiYuan Huang (7): 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 leds: rgb: mt6370: Add MediaTek MT6370 current sink type LED Indicator support ChiaEn Wu (4): dt-bindings: power: supply: Add MediaTek MT6370 Charger iio: adc: mt6370: Add MediaTek MT6370 support power: supply: mt6370: Add MediaTek MT6370 charger driver video: backlight: mt6370: Add MediaTek MT6370 support .../leds/backlight/mediatek,mt6370-backlight.yaml | 92 ++ .../bindings/leds/mediatek,mt6370-flashlight.yaml | 41 + .../bindings/leds/mediatek,mt6370-indicator.yaml | 81 ++ .../devicetree/bindings/mfd/mediatek,mt6370.yaml | 280 ++++++ .../power/supply/mediatek,mt6370-charger.yaml | 88 ++ .../bindings/usb/mediatek,mt6370-tcpc.yaml | 36 + drivers/iio/adc/Kconfig | 12 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/mt6370-adc.c | 274 ++++++ drivers/leds/flash/Kconfig | 12 + drivers/leds/flash/Makefile | 1 + drivers/leds/flash/leds-mt6370-flash.c | 633 ++++++++++++ drivers/leds/rgb/Kconfig | 13 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-mt6370-rgb.c | 1004 ++++++++++++++++++++ drivers/mfd/Kconfig | 16 + drivers/mfd/Makefile | 1 + drivers/mfd/mt6370.c | 281 ++++++ drivers/mfd/mt6370.h | 99 ++ drivers/power/supply/Kconfig | 14 + drivers/power/supply/Makefile | 1 + drivers/power/supply/mt6370-charger.c | 973 +++++++++++++++++++ drivers/usb/typec/tcpm/Kconfig | 11 + drivers/usb/typec/tcpm/Makefile | 1 + drivers/usb/typec/tcpm/tcpci_mt6370.c | 208 ++++ drivers/video/backlight/Kconfig | 12 + drivers/video/backlight/Makefile | 1 + drivers/video/backlight/mt6370-backlight.c | 339 +++++++ include/dt-bindings/iio/adc/mediatek,mt6370_adc.h | 18 + 29 files changed, 4544 insertions(+) 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/rgb/leds-mt6370-rgb.c create mode 100644 drivers/mfd/mt6370.c create mode 100644 drivers/mfd/mt6370.h create mode 100644 drivers/power/supply/mt6370-charger.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