Re: [PATCH v6 1/4] mfd: rt4831: Adds support for Richtek RT4831
Lee Jones 於 2021年4月19日 週一 下午3:24寫道： > > On Mon, 19 Apr 2021, Lee Jones wrote: > > > On Mon, 19 Apr 2021, Lee Jones wrote: > > > > > On Mon, 19 Apr 2021, ChiYuan Huang wrote: > > > > > > > Hi, Linux mfd reviewers: > > > >It's been three weeks not to get any response from you. > > > > Is there something wrong about this mfd patch? > > > > If yes, please feel free to let me know. > > > > > > Couple of things: > > > > > > First, if you think a patch had fallen through the gaps, which does > > > happen sometimes, it is generally considered acceptable to submit a > > > [RESEND] ~2 weeks after the initial submission. FYI: This was such a > > > patch. It was not on, or had fallen off of my radar for some reason. > > > > > > Secondly, we are really late in the release cycle. -rc8 has just been > > > released. Quite a few maintainers slow down at ~-rc6. Particularly > > > for new drivers. > > > > > > No need to resubmit this driver this time. It is now on my to-review > > > list and I will tend to it shortly. > > > > > > Thanks for your patience. > > > > Also you are missing a DT review on patch 4. > > ... looks like you forgot to Cc them! > Yap, really. I''ll resend patch 4 and cc them. Thx. > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v6 1/4] mfd: rt4831: Adds support for Richtek RT4831
Hi, Linux mfd reviewers:
It's been three weeks not to get any response from you.
Is there something wrong about this mfd patch?
If yes, please feel free to let me know.
cy_huang 於 2021年3月28日 週日 下午11:24寫道：
>
> From: ChiYuan Huang
>
> This adds support Richtek RT4831 core. It includes four channel WLED driver
> and Display Bias Voltage outputs.
>
> Signed-off-by: ChiYuan Huang
> ---
> The RT4831 regulator patches are already on main stream, and can be referred
> to
> 9351ab8b0cb6 regulator: rt4831: Adds support for Richtek RT4831 DSV regulator
> 934b05e81862 regulator: rt4831: Adds DT binding document for Richtek RT4831
> DSV regulator
>
> since v6
> - Respin the date from 2020 to 2021.
> - Rmove the shutdown routine.
> - Change the macro OF_MFD_CELL to MFD_CELL_OF.
>
>
> since v5
> - Rename file name from rt4831-core.c to rt4831.c
> - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> - Change variable 'val' to the meaningful name 'chip_id'.
> - Refine the error log when vendor id is not matched.
> - Remove of_match_ptr.
>
> since v2
> - Refine Kconfig descriptions.
> - Add copyright.
> - Refine error logs in probe.
> - Refine comment lines in remove and shutdown.
> ---
> drivers/mfd/Kconfig | 10 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/rt4831.c | 115
> +++
> 3 files changed, 126 insertions(+)
> create mode 100644 drivers/mfd/rt4831.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b74efa4..3f43834 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1065,6 +1065,16 @@ config MFD_RDC321X
> southbridge which provides access to GPIOs and Watchdog using the
> southbridge PCI device configuration space.
>
> +config MFD_RT4831
> + tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> + depends on I2C
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> + This enables support for the Richtek RT4831 that includes 4 channel
> + WLED driving and Display Bias Voltage. It's commonly used to provide
> + power to the LCD display and LCD backlight.
> +
> config MFD_RT5033
> tristate "Richtek RT5033 Power Management IC"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 834f546..5986914 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> obj-$(CONFIG_MFD_HI6421_PMIC) += hi6421-pmic-core.o
> obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> obj-$(CONFIG_MFD_DLN2) += dln2.o
> +obj-$(CONFIG_MFD_RT4831) += rt4831.o
> obj-$(CONFIG_MFD_RT5033) += rt5033.o
> obj-$(CONFIG_MFD_SKY81452) += sky81452.o
>
> diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> new file mode 100644
> index ..b169781
> --- /dev/null
> +++ b/drivers/mfd/rt4831.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021 Richtek Technology Corp.
> + *
> + * Author: ChiYuan Huang
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define RT4831_REG_REVISION0x01
> +#define RT4831_REG_ENABLE 0x08
> +#define RT4831_REG_I2CPROT 0x15
> +
> +#define RICHTEK_VENDOR_ID 0x03
> +#define RT4831_VID_MASKGENMASK(1, 0)
> +#define RT4831_RESET_MASK BIT(7)
> +#define RT4831_I2CSAFETMR_MASK BIT(0)
> +
> +static const struct mfd_cell rt4831_subdevs[] = {
> + MFD_CELL_OF("rt4831-backlight", NULL, NULL, 0, 0,
> "richtek,rt4831-backlight"),
> + MFD_CELL_NAME("rt4831-regulator")
> +};
> +
> +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
> +{
> + if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> + return true;
> + return false;
> +}
> +
> +static const struct regmap_config rt4831_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = RT4831_REG_I2CPROT,
> +
> + .readable_reg = rt4831_is_accessible_reg,
> + .writeable_reg = rt4831_is_accessible_reg,
> +};
> +
> +static int rt4831_probe(struct i2c_client *client)
> +{
> + struct gpio_desc *enable_gpio;
> + struct regmap *regmap;
> + unsigned int chip_id;
> + int ret;
> +
> + enable_gpio = devm_gpiod_get_optional(&c
Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control
HI, reviews:
ChiYuan Huang 於 2021年1月19日 週二 下午4:23寫道：
>
> Chunfeng Yun 於 2021年1月19日 週二 下午3:38寫道：
> >
> > On Mon, 2021-01-18 at 16:28 +0800, ChiYuan Huang wrote:
> > > Guenter Roeck 於 2021年1月18日 週一 上午1:43寫道：
> > > >
> > > > On 1/15/21 6:13 AM, cy_huang wrote:
> > > > > From: ChiYuan Huang
> > > > >
> > > > > MT6360 not support for TCPC command to control source and sink.
> > > >
> > > > does not
> > > >
> > > Ack
> > > > > Uses external 5V vbus regulator as the vbus source control.
> > > > >
> > > > Use
> > > >
> > > Ack
> > > > > Also adds the capability to report vsafe0v.
> > > > >
> > > > add
> > > >
> > > Ack
> > > > So far this driver works without regulator. Unless I am missing
> > > > something,
> > > > this patch makes regulator support mandatory, meaning existing code
> > > > will fail.
> > > > I am not sure if that is appropriate/acceptable. Can we be sure that
> > > > this will
> > > > work for existing users of this driver ?
> > > >
> > > Yes, I already checked all the src/snk functionality based on the
> > > latest typec code.
> > > It'll be common for our TCPC. It didn't support for TCPC command.
> > > From the recent patches, actually, I have the local change to test the
> > > src capability.
> > > But I didn't submit it. It's almost the same to add set_vbus callback.
> > > That's why I submit this change after tcpci 'set_vbus callback' is added.
> > >
> > > > Thanks,
> > > > Guenter
> > > >
> > > > > Signed-off-by: ChiYuan Huang
> > > > > ---
> > > > > drivers/usb/typec/tcpm/tcpci_mt6360.c | 29
> > > > > +
> > > > > 1 file changed, 29 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > > b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > > index f1bd9e0..0edf4b6 100644
> > > > > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > > @@ -11,6 +11,7 @@
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > >
> > > > > #include "tcpci.h"
> > > > > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> > > > > struct tcpci_data tdata;
> > > > > struct tcpci *tcpci;
> > > > > struct device *dev;
> > > > > + struct regulator *vbus;
> > > > > int irq;
> > > > > };
> > > > >
> > > > > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct
> > > > > regmap *regmap,
> > > > > return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > > > > }
> > > > >
> > > > > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct
> > > > > tcpci_data *data, bool src, bool snk)
> > > > > +{
> > > > > + struct mt6360_tcpc_info *mti = container_of(data, struct
> > > > > mt6360_tcpc_info, tdata);
> > > > > + int ret;
> > > > > +
> > > > > + /* To correctly handle the already enabled vbus and disable its
> > > > > supply first */
> > > > > + if (regulator_is_enabled(mti->vbus)) {
> > > > > + ret = regulator_disable(mti->vbus);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > + }
> > > >
> > > > Is it really a good idea to disable vbus if it happens to be already
> > > > enabled
> > > > and there is (another ?) request to enable it ?
> > > >
> > > Yes, for the state change from src_attach_wait to src_attach,
> > > It need to meet the requirement that the vbus is at vsafe0v.
> > > So to disable it first is needed.
> > > And to prevent other users from enabling/disabling external vbus
> > > regulator in any case.
> > > I think we may change regulator_get to 'regulator_get_exclusive&
Re: [PATCH v6 4/4] backlight: rt4831: Adds support for Richtek RT4831 backlight
Hi, Daniel: Daniel Thompson 於 2021年3月29日 週一 下午6:26寫道： > > On Sun, Mar 28, 2021 at 11:24:19PM +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Adds support for Richtek RT4831 backlight. > > > > Signed-off-by: ChiYuan Huang > > --- > > since v6 > > - Fix Kconfig typo. > > - Remove internal mutex lock. > > - Add the prefix for max brightness. > > - rename init_device_properties to parse_backlight_properties. > > - Remove some warning message if default value is adopted. > > - Add backlight property scale to LINEAR mapping. > > - Fix regmap get to check NULL not IS_ERR. > > --- > > drivers/video/backlight/Kconfig| 8 ++ > > drivers/video/backlight/Makefile | 1 + > > drivers/video/backlight/rt4831-backlight.c | 203 > > + > > 3 files changed, 212 insertions(+) > > create mode 100644 drivers/video/backlight/rt4831-backlight.c > > > > diff --git a/drivers/video/backlight/Kconfig > > b/drivers/video/backlight/Kconfig > > index d83c87b..de96441 100644 > > --- a/drivers/video/backlight/Kconfig > > +++ b/drivers/video/backlight/Kconfig > > @@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED > > If you have the Qualcomm PMIC, say Y to enable a driver for the > > WLED block. Currently it supports PM8941 and PMI8998. > > > > +config BACKLIGHT_RT4831 > > + tristate "Richtek RT4831 Backlight Driver" > > + depends on MFD_RT4831 > > + help > > + This enables support for Richtek RT4831 Backlight driver. > > + It's common used to drive the display WLED. There're four channels > > Nitpicking but I was expecting the original typo be converted to > "commonly". > OK, I'll correct this typo in v7 next. And will merge the reviewed-by line. Thx. > > With that addressed: > Reviewed-by: Daniel Thompson > > > Daniel.
Re: [PATCH v2 1/2] leds: rt4505: Add support for Richtek RT4505 flash LED controller
Pavel Machek 於 2021年3月25日 週四 下午6:01寫道： > > Hi! > > > > > create mode 100644 drivers/leds/flash/Kconfig > > > > create mode 100644 drivers/leds/flash/Makefile > > > > create mode 100644 drivers/leds/flash/leds-rt4505.c > > > > > > Acked-by: Jacek Anaszewski > > > > > Any problem with this patch? Do I need to submit it again? > > It won't apply on current next. > > So please: Merge ACKs, reorder it so that docs goes first, port it to > > To gitolite.kernel.org:pub/scm/linux/kernel/git/pavel/linux-leds.git >34731ed13e8a..85674b0e40d9 for-next -> for-next > > and resubmit. Thx. It's clear. So the next I need to do is 1. Merge ACKs 2. Reorder this patch from the docs first After done, do I need to change the patch revision from v2 to v3 before submitng it? > > Thanks you, > Pavel > -- > http://www.livejournal.com/~pavelmachek
Re: [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
HI, Lee:
ChiYuan Huang 於 2021年1月13日 週三 下午10:09寫道：
>
> Lee Jones 於 2021年1月13日 週三 下午8:21寫道：
> >
> > On Thu, 17 Dec 2020, cy_huang wrote:
> >
> > > From: ChiYuan Huang
> > >
> > > This adds support Richtek RT4831 core. It includes four channel WLED
> > > driver
> > > and Display Bias Voltage outputs.
> > >
> > > Signed-off-by: ChiYuan Huang
> > > ---
> > > since v5
> > > - Rename file name from rt4831-core.c to rt4831.c
> > > - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> > > - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> > > - Change variable 'val' to the meaningful name 'chip_id'.
> > > - Refine the error log when vendor id is not matched.
> > > - Remove of_match_ptr.
> > >
> > > since v2
> > > - Refine Kconfig descriptions.
> > > - Add copyright.
> > > - Refine error logs in probe.
> > > - Refine comment lines in remove and shutdown.
> > > ---
> > > drivers/mfd/Kconfig | 10 +
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/rt4831.c | 124
> > > +++
> > > 3 files changed, 135 insertions(+)
> > > create mode 100644 drivers/mfd/rt4831.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 8b99a13..dfb2640 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> > > southbridge which provides access to GPIOs and Watchdog using the
> > > southbridge PCI device configuration space.
> > >
> > > +config MFD_RT4831
> > > + tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > > + depends on I2C
> > > + select MFD_CORE
> > > + select REGMAP_I2C
> > > + help
> > > + This enables support for the Richtek RT4831 that includes 4
> > > channel
> > > + WLED driving and Display Bias Voltage. It's commonly used to
> > > provide
> > > + power to the LCD display and LCD backlight.
> > > +
> > > config MFD_RT5033
> > > tristate "Richtek RT5033 Power Management IC"
> > > depends on I2C
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index 1780019..28d247b 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> > > obj-$(CONFIG_MFD_HI6421_PMIC)+= hi6421-pmic-core.o
> > > obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> > > obj-$(CONFIG_MFD_DLN2) += dln2.o
> > > +obj-$(CONFIG_MFD_RT4831) += rt4831.o
> > > obj-$(CONFIG_MFD_RT5033) += rt5033.o
> > > obj-$(CONFIG_MFD_SKY81452) += sky81452.o
> > >
> > > diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> > > new file mode 100644
> > > index ..2bf8364
> > > --- /dev/null
> > > +++ b/drivers/mfd/rt4831.c
> > > @@ -0,0 +1,124 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (c) 2020 Richtek Technology Corp.
> >
> > Nit: If you respin this, please bump the date.
> >
> Okay.
> > > + * Author: ChiYuan Huang
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define RT4831_REG_REVISION 0x01
> > > +#define RT4831_REG_ENABLE0x08
> > > +#define RT4831_REG_I2CPROT 0x15
> > > +
> > > +#define RICHTEK_VENDOR_ID0x03
> > > +#define RT4831_VID_MASK GENMASK(1, 0)
> > > +#define RT4831_RESET_MASKBIT(7)
> > > +#define RT4831_I2CSAFETMR_MASK BIT(0)
> > > +
> > > +static const struct mfd_cell rt4831_subdevs[] = {
> > > + OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0,
> > > "richtek,rt4831-backlight"),
> > > + MFD_CELL_NAME("rt4831-regulator")
> > > +};
> > > +
> > > +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int
> > > reg)
> > > +{
> > > + if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> > > +
Re: [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight
Dear reviewers:
Didn't get any response about this backlight patch.
Is there any part need to be refined?
cy_huang 於 2020年12月17日 週四 下午11:01寫道：
>
> From: ChiYuan Huang
>
> Adds support for Richtek RT4831 backlight.
>
> Signed-off-by: ChiYuan Huang
> ---
> drivers/video/backlight/Kconfig| 8 ++
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/rt4831-backlight.c | 219
> +
> 3 files changed, 228 insertions(+)
> create mode 100644 drivers/video/backlight/rt4831-backlight.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index d83c87b..666bdb0 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED
> If you have the Qualcomm PMIC, say Y to enable a driver for the
> WLED block. Currently it supports PM8941 and PMI8998.
>
> +config BACKLIGHT_RT4831
> + tristate "Richtek RT4831 Backlight Driver"
> + depends on MFD_RT4831
> + help
> + This enables support for Richtek RT4831 Backlight driver.
> + It's commont used to drive the display WLED. There're four channels
> + inisde, and each channel can provide up to 30mA current.
> +
> config BACKLIGHT_SAHARA
> tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
> depends on X86
> diff --git a/drivers/video/backlight/Makefile
> b/drivers/video/backlight/Makefile
> index 685f3f1..cae2c83 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_BACKLIGHT_PANDORA) +=
> pandora_bl.o
> obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
> obj-$(CONFIG_BACKLIGHT_PWM)+= pwm_bl.o
> obj-$(CONFIG_BACKLIGHT_QCOM_WLED) += qcom-wled.o
> +obj-$(CONFIG_BACKLIGHT_RT4831) += rt4831-backlight.o
> obj-$(CONFIG_BACKLIGHT_SAHARA) += kb3886_bl.o
> obj-$(CONFIG_BACKLIGHT_SKY81452) += sky81452-backlight.o
> obj-$(CONFIG_BACKLIGHT_TOSA) += tosa_bl.o
> diff --git a/drivers/video/backlight/rt4831-backlight.c
> b/drivers/video/backlight/rt4831-backlight.c
> new file mode 100644
> index ..816c4d6
> --- /dev/null
> +++ b/drivers/video/backlight/rt4831-backlight.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define RT4831_REG_BLCFG 0x02
> +#define RT4831_REG_BLDIML 0x04
> +#define RT4831_REG_ENABLE 0x08
> +
> +#define BL_MAX_BRIGHTNESS 2048
> +
> +#define RT4831_BLOVP_MASK GENMASK(7, 5)
> +#define RT4831_BLOVP_SHIFT 5
> +#define RT4831_BLPWMEN_MASKBIT(0)
> +#define RT4831_BLEN_MASK BIT(4)
> +#define RT4831_BLCH_MASK GENMASK(3, 0)
> +#define RT4831_BLDIML_MASK GENMASK(2, 0)
> +#define RT4831_BLDIMH_MASK GENMASK(10, 3)
> +#define RT4831_BLDIMH_SHIFT3
> +
> +struct rt4831_priv {
> + struct regmap *regmap;
> + struct mutex lock;
> + struct backlight_device *bl;
> +};
> +
> +static int rt4831_bl_update_status(struct backlight_device *bl_dev)
> +{
> + struct rt4831_priv *priv = bl_get_data(bl_dev);
> + int brightness = backlight_get_brightness(bl_dev);
> + unsigned int enable = brightness ? RT4831_BLEN_MASK : 0;
> + u8 v[2];
> + int ret;
> +
> + mutex_lock(&priv->lock);
> +
> + if (brightness) {
> + v[0] = (brightness - 1) & RT4831_BLDIML_MASK;
> + v[1] = ((brightness - 1) & RT4831_BLDIMH_MASK) >>
> RT4831_BLDIMH_SHIFT;
> +
> + ret = regmap_raw_write(priv->regmap, RT4831_REG_BLDIML, v,
> sizeof(v));
> + if (ret)
> + goto unlock;
> + }
> +
> + ret = regmap_update_bits(priv->regmap, RT4831_REG_ENABLE,
> RT4831_BLEN_MASK, enable);
> +
> +unlock:
> + mutex_unlock(&priv->lock);
> + return ret;
> +}
> +
> +static int rt4831_bl_get_brightness(struct backlight_device *bl_dev)
> +{
> + struct rt4831_priv *priv = bl_get_data(bl_dev);
> + unsigned int val;
> + u8 v[2];
> + int ret;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = regmap_read(priv->regmap, RT4831_REG_ENABLE, &val);
> + if (ret)
> + return ret;
> +
> + if (!(val & RT4831_BLEN_MASK)) {
> +
Re: [PATCH v2 1/2] leds: rt4505: Add support for Richtek RT4505 flash LED controller
Hi, Jacek: Jacek Anaszewski 於 2020年11月3日 週二 上午4:44寫道： > > Hi ChiYuan, > > On 11/2/20 3:42 AM, cy_huang wrote: > > From: ChiYuan Huang > > > > Add support for RT4505 flash LED controller. It can support up to 1.5A > > flash current with hardware timeout and low input voltage protection. > > > > Signed-off-by: ChiYuan Huang > > --- > > Changes since v1 to v2 > > > > - Create flash directory into drvers/leds. > > - Coding style fix to meet 80 charactors per line limit. > > - Refine the description in the Kconfig help text. > > - Change all descriptions for 'led' text to uppercase 'LED'. > > > > --- > > drivers/leds/Kconfig | 2 + > > drivers/leds/Makefile| 3 + > > drivers/leds/flash/Kconfig | 17 ++ > > drivers/leds/flash/Makefile | 2 + > > drivers/leds/flash/leds-rt4505.c | 430 > > +++ > > 5 files changed, 454 insertions(+) > > create mode 100644 drivers/leds/flash/Kconfig > > create mode 100644 drivers/leds/flash/Makefile > > create mode 100644 drivers/leds/flash/leds-rt4505.c > > Acked-by: Jacek Anaszewski > Any problem with this patch? Do I need to submit it again? > -- > Best regards, > Jacek Anaszewski
Re: [PATCH 2/2] usb typec: tcpci: mt6360: Add vbus supply into dt-binding description
Rob Herring 於 2021年1月20日 週三 上午7:11寫道：
>
> On Fri, Jan 15, 2021 at 10:13:21PM +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Add external vbus source into dt-binding description.
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > index 1e8e1c2..b8d842b 100644
> > --- a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > @@ -26,6 +26,11 @@ properties:
> > items:
> >- const: PD_IRQB
> >
> > + vbus-supply:
> > +description:
> > + Vbus source supply regulator.
> > +maxItems: 1
>
> vbus-supply is already in the 'connector' node, you don't need it here.
>
If not put here, 'regulator_get' only can follow the legacy way to get
vbus regulator.
Currently, there's no one to use the 'vbus-supply' property.
>From my understanding, the 'vbus-supply' is the chip level property,
not connector type property.
> > +
> >connector:
> > type: object
> > $ref: ../connector/usb-connector.yaml#
> > @@ -38,6 +43,7 @@ required:
> >- compatible
> >- interrupts
> >- interrupt-names
> > + - vbus-supply
> >
> > examples:
> >- |
> > @@ -54,6 +60,7 @@ examples:
> >compatible = "mediatek,mt6360-tcpc";
> >interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
> >interrupt-names = "PD_IRQB";
> > + vbus-supply = <&otg_vbus>;
> >
> >connector {
> > compatible = "usb-c-connector";
> > --
> > 2.7.4
> >
Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control
Chunfeng Yun 於 2021年1月19日 週二 下午3:38寫道：
>
> On Mon, 2021-01-18 at 16:28 +0800, ChiYuan Huang wrote:
> > Guenter Roeck 於 2021年1月18日 週一 上午1:43寫道：
> > >
> > > On 1/15/21 6:13 AM, cy_huang wrote:
> > > > From: ChiYuan Huang
> > > >
> > > > MT6360 not support for TCPC command to control source and sink.
> > >
> > > does not
> > >
> > Ack
> > > > Uses external 5V vbus regulator as the vbus source control.
> > > >
> > > Use
> > >
> > Ack
> > > > Also adds the capability to report vsafe0v.
> > > >
> > > add
> > >
> > Ack
> > > So far this driver works without regulator. Unless I am missing something,
> > > this patch makes regulator support mandatory, meaning existing code will
> > > fail.
> > > I am not sure if that is appropriate/acceptable. Can we be sure that this
> > > will
> > > work for existing users of this driver ?
> > >
> > Yes, I already checked all the src/snk functionality based on the
> > latest typec code.
> > It'll be common for our TCPC. It didn't support for TCPC command.
> > From the recent patches, actually, I have the local change to test the
> > src capability.
> > But I didn't submit it. It's almost the same to add set_vbus callback.
> > That's why I submit this change after tcpci 'set_vbus callback' is added.
> >
> > > Thanks,
> > > Guenter
> > >
> > > > Signed-off-by: ChiYuan Huang
> > > > ---
> > > > drivers/usb/typec/tcpm/tcpci_mt6360.c | 29
> > > > +
> > > > 1 file changed, 29 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > index f1bd9e0..0edf4b6 100644
> > > > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > @@ -11,6 +11,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > >
> > > > #include "tcpci.h"
> > > > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> > > > struct tcpci_data tdata;
> > > > struct tcpci *tcpci;
> > > > struct device *dev;
> > > > + struct regulator *vbus;
> > > > int irq;
> > > > };
> > > >
> > > > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap
> > > > *regmap,
> > > > return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > > > }
> > > >
> > > > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data
> > > > *data, bool src, bool snk)
> > > > +{
> > > > + struct mt6360_tcpc_info *mti = container_of(data, struct
> > > > mt6360_tcpc_info, tdata);
> > > > + int ret;
> > > > +
> > > > + /* To correctly handle the already enabled vbus and disable its
> > > > supply first */
> > > > + if (regulator_is_enabled(mti->vbus)) {
> > > > + ret = regulator_disable(mti->vbus);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > >
> > > Is it really a good idea to disable vbus if it happens to be already
> > > enabled
> > > and there is (another ?) request to enable it ?
> > >
> > Yes, for the state change from src_attach_wait to src_attach,
> > It need to meet the requirement that the vbus is at vsafe0v.
> > So to disable it first is needed.
> > And to prevent other users from enabling/disabling external vbus
> > regulator in any case.
> > I think we may change regulator_get to 'regulator_get_exclusive'.
> > From the design, 5v regulator only can be controlled via typec framework.
> > If other user touch it, it'll affect the typec state transition.
> How about to process the case that even switch usb controller to device
> mode, platform also need to keep vbus on? e.g. Iphone Carplay
>
>
It must be processed by USBPD data role swap.
Type C only decide the initial role (SNK: power snk and ufp; SRC:
power src and DFP).
Only USBPD can change the power/data/vconn role individually.
> > > > +
> > > > + if (src) {
> > > > + ret = regulator_enable(mti->vbus);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data
> > > > *tdata)
> > > > {
> > > > struct regmap *regmap = tdata->regmap;
> > > > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct
> > > > platform_device *pdev)
> > > > if (mti->irq < 0)
> > > > return mti->irq;
> > > >
> > > > + mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > > > + if (IS_ERR(mti->vbus))
> > > > + return PTR_ERR(mti->vbus);
> > > > +
> > > > mti->tdata.init = mt6360_tcpc_init;
> > > > + mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > > > + mti->tdata.vbus_vsafe0v = 1;
> > > > mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> > > > if (IS_ERR(mti->tcpci)) {
> > > > dev_err(&pdev->dev, "Failed to register tcpci port\n");
> > > >
> > >
>
Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control
Chunfeng Yun 於 2021年1月19日 週二 下午3:33寫道：
>
> On Sun, 2021-01-17 at 09:43 -0800, Guenter Roeck wrote:
> > On 1/15/21 6:13 AM, cy_huang wrote:
> > > From: ChiYuan Huang
> > >
> > > MT6360 not support for TCPC command to control source and sink.
> >
> > does not
> >
> > > Uses external 5V vbus regulator as the vbus source control.
> > >
> > Use
> >
> > > Also adds the capability to report vsafe0v.
> > >
> > add
> >
> > So far this driver works without regulator. Unless I am missing something,
> > this patch makes regulator support mandatory, meaning existing code will
> > fail.
> If don't provide vbus-supply in DTS, regulator framework will provide a
> dummy regulator, so the code will not fail.
In the last reply, I will change from regulator_get to
regulator_get_exclusive, it will return -ENODEV.
The IS_ERR can catch this situation, no dummy regulator will be returned.
And assume no vbus 5v for source & snk attached, It will cause typec
state machine repeated from
drp -> src_attach_wait -> src_attached -> PD_T_PS_SOURCE_on timeout.
It will be stuck in the loop until snk detached.
> > I am not sure if that is appropriate/acceptable. Can we be sure that this
> > will
> > work for existing users of this driver ?
>
> >
> > Thanks,
> > Guenter
> >
> > > Signed-off-by: ChiYuan Huang
> > > ---
> > > drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +
> > > 1 file changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > index f1bd9e0..0edf4b6 100644
> > > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > @@ -11,6 +11,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > >
> > > #include "tcpci.h"
> > > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> > > struct tcpci_data tdata;
> > > struct tcpci *tcpci;
> > > struct device *dev;
> > > + struct regulator *vbus;
> > > int irq;
> > > };
> > >
> > > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap
> > > *regmap,
> > > return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > > }
> > >
> > > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data
> > > *data, bool src, bool snk)
> > > +{
> > > + struct mt6360_tcpc_info *mti = container_of(data, struct
> > > mt6360_tcpc_info, tdata);
> > > + int ret;
> > > +
> > > + /* To correctly handle the already enabled vbus and disable its
> > > supply first */
> > > + if (regulator_is_enabled(mti->vbus)) {
> > > + ret = regulator_disable(mti->vbus);
> > > + if (ret)
> > > + return ret;
> > > + }
> >
> > Is it really a good idea to disable vbus if it happens to be already enabled
> > and there is (another ?) request to enable it ?
> >
> > > +
> > > + if (src) {
> > > + ret = regulator_enable(mti->vbus);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data
> > > *tdata)
> > > {
> > > struct regmap *regmap = tdata->regmap;
> > > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device
> > > *pdev)
> > > if (mti->irq < 0)
> > > return mti->irq;
> > >
> > > + mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > > + if (IS_ERR(mti->vbus))
> > > + return PTR_ERR(mti->vbus);
> > > +
> > > mti->tdata.init = mt6360_tcpc_init;
> > > + mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > > + mti->tdata.vbus_vsafe0v = 1;
> > > mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> > > if (IS_ERR(mti->tcpci)) {
> > > dev_err(&pdev->dev, "Failed to register tcpci port\n");
> > >
> >
>
Re: [PATCH 2/2] usb typec: tcpci: mt6360: Add vbus supply into dt-binding description
Rob Herring 於 2021年1月17日 週日 下午11:46寫道： > > On Fri, 15 Jan 2021 22:13:21 +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Add external vbus source into dt-binding description. > > > > Signed-off-by: ChiYuan Huang > > --- > > Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml | 7 +++ > > 1 file changed, 7 insertions(+) > > > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml: > properties:vbus-supply: 'maxItems' is not one of ['description', > 'deprecated'] > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml: > ignoring, error in schema: properties: vbus-supply > warning: no schema found in file: > ./Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml > > See https://patchwork.ozlabs.org/patch/1427073 > > This check can fail if there are any dependencies. The base for a patch > series is generally the most recent rc1. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit. > Thanks, after I re-installed the yamlint, the error can be seen. Refer to https://www.kernel.org/doc/Documentation/devicetree/bindings/example-schema.yaml *-supply is only a phandle In next series patch, I'll remove the maxItems in vbus-supply. I already checked the below change. make dt_binding_check can be passed vbus-supply: description: Vbus source supply regulator. -maxItems: 1 connector: type: object
Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control
Guenter Roeck 於 2021年1月18日 週一 上午1:43寫道：
>
> On 1/15/21 6:13 AM, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > MT6360 not support for TCPC command to control source and sink.
>
> does not
>
Ack
> > Uses external 5V vbus regulator as the vbus source control.
> >
> Use
>
Ack
> > Also adds the capability to report vsafe0v.
> >
> add
>
Ack
> So far this driver works without regulator. Unless I am missing something,
> this patch makes regulator support mandatory, meaning existing code will fail.
> I am not sure if that is appropriate/acceptable. Can we be sure that this will
> work for existing users of this driver ?
>
Yes, I already checked all the src/snk functionality based on the
latest typec code.
It'll be common for our TCPC. It didn't support for TCPC command.
>From the recent patches, actually, I have the local change to test the
src capability.
But I didn't submit it. It's almost the same to add set_vbus callback.
That's why I submit this change after tcpci 'set_vbus callback' is added.
> Thanks,
> Guenter
>
> > Signed-off-by: ChiYuan Huang
> > ---
> > drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +
> > 1 file changed, 29 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > index f1bd9e0..0edf4b6 100644
> > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > @@ -11,6 +11,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "tcpci.h"
> > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> > struct tcpci_data tdata;
> > struct tcpci *tcpci;
> > struct device *dev;
> > + struct regulator *vbus;
> > int irq;
> > };
> >
> > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap
> > *regmap,
> > return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > }
> >
> > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data
> > *data, bool src, bool snk)
> > +{
> > + struct mt6360_tcpc_info *mti = container_of(data, struct
> > mt6360_tcpc_info, tdata);
> > + int ret;
> > +
> > + /* To correctly handle the already enabled vbus and disable its
> > supply first */
> > + if (regulator_is_enabled(mti->vbus)) {
> > + ret = regulator_disable(mti->vbus);
> > + if (ret)
> > + return ret;
> > + }
>
> Is it really a good idea to disable vbus if it happens to be already enabled
> and there is (another ?) request to enable it ?
>
Yes, for the state change from src_attach_wait to src_attach,
It need to meet the requirement that the vbus is at vsafe0v.
So to disable it first is needed.
And to prevent other users from enabling/disabling external vbus
regulator in any case.
I think we may change regulator_get to 'regulator_get_exclusive'.
>From the design, 5v regulator only can be controlled via typec framework.
If other user touch it, it'll affect the typec state transition.
> > +
> > + if (src) {
> > + ret = regulator_enable(mti->vbus);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> > {
> > struct regmap *regmap = tdata->regmap;
> > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device
> > *pdev)
> > if (mti->irq < 0)
> > return mti->irq;
> >
> > + mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > + if (IS_ERR(mti->vbus))
> > + return PTR_ERR(mti->vbus);
> > +
> > mti->tdata.init = mt6360_tcpc_init;
> > + mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > + mti->tdata.vbus_vsafe0v = 1;
> > mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> > if (IS_ERR(mti->tcpci)) {
> > dev_err(&pdev->dev, "Failed to register tcpci port\n");
> >
>
Re: [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
Lee Jones 於 2021年1月13日 週三 下午8:21寫道：
>
> On Thu, 17 Dec 2020, cy_huang wrote:
>
> > From: ChiYuan Huang
> >
> > This adds support Richtek RT4831 core. It includes four channel WLED driver
> > and Display Bias Voltage outputs.
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > since v5
> > - Rename file name from rt4831-core.c to rt4831.c
> > - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> > - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> > - Change variable 'val' to the meaningful name 'chip_id'.
> > - Refine the error log when vendor id is not matched.
> > - Remove of_match_ptr.
> >
> > since v2
> > - Refine Kconfig descriptions.
> > - Add copyright.
> > - Refine error logs in probe.
> > - Refine comment lines in remove and shutdown.
> > ---
> > drivers/mfd/Kconfig | 10 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/rt4831.c | 124
> > +++
> > 3 files changed, 135 insertions(+)
> > create mode 100644 drivers/mfd/rt4831.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 8b99a13..dfb2640 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> > southbridge which provides access to GPIOs and Watchdog using the
> > southbridge PCI device configuration space.
> >
> > +config MFD_RT4831
> > + tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > + depends on I2C
> > + select MFD_CORE
> > + select REGMAP_I2C
> > + help
> > + This enables support for the Richtek RT4831 that includes 4 channel
> > + WLED driving and Display Bias Voltage. It's commonly used to provide
> > + power to the LCD display and LCD backlight.
> > +
> > config MFD_RT5033
> > tristate "Richtek RT5033 Power Management IC"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 1780019..28d247b 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> > obj-$(CONFIG_MFD_HI6421_PMIC)+= hi6421-pmic-core.o
> > obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> > obj-$(CONFIG_MFD_DLN2) += dln2.o
> > +obj-$(CONFIG_MFD_RT4831) += rt4831.o
> > obj-$(CONFIG_MFD_RT5033) += rt5033.o
> > obj-$(CONFIG_MFD_SKY81452) += sky81452.o
> >
> > diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> > new file mode 100644
> > index ..2bf8364
> > --- /dev/null
> > +++ b/drivers/mfd/rt4831.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2020 Richtek Technology Corp.
>
> Nit: If you respin this, please bump the date.
>
Okay.
> > + * Author: ChiYuan Huang
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define RT4831_REG_REVISION 0x01
> > +#define RT4831_REG_ENABLE0x08
> > +#define RT4831_REG_I2CPROT 0x15
> > +
> > +#define RICHTEK_VENDOR_ID0x03
> > +#define RT4831_VID_MASK GENMASK(1, 0)
> > +#define RT4831_RESET_MASKBIT(7)
> > +#define RT4831_I2CSAFETMR_MASK BIT(0)
> > +
> > +static const struct mfd_cell rt4831_subdevs[] = {
> > + OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0,
> > "richtek,rt4831-backlight"),
> > + MFD_CELL_NAME("rt4831-regulator")
> > +};
> > +
> > +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
> > +{
> > + if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> > + return true;
> > + return false;
> > +}
> > +
> > +static const struct regmap_config rt4831_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = RT4831_REG_I2CPROT,
> > +
> > + .readable_reg = rt4831_is_accessible_reg,
> > + .writeable_reg = rt4831_is_accessible_reg,
> > +};
> > +
> > +static int rt4831_probe(struct i2c_client *client)
> > +{
> > + struct gpio_desc *enable_gpio;
> > + struct regmap *regmap;
> > + unsigned int chip_id;
> > + int ret;
> > +
> > + enable_
Re: [PATCH v4 1/3] mfd: rt4831: Adds support for Richtek RT4831 MFD core
Lee Jones 於 2020年12月16日 週三 下午10:12寫道：
>
> On Sat, 12 Dec 2020, cy_huang wrote:
>
> > From: ChiYuan Huang
> >
> > This adds support Richtek RT4831 MFD core. It includes four channel WLED
> > driver
>
> Drop mentions of MFD. Just core driver will do.
>
> > and Display Bias Voltage outputs.
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > since v2
> > - Refine Kconfig descriptions.
> > - Add copyright.
> > - Refine error logs in probe.
> > - Refine comment lines in remove and shutdown.
> > ---
> > drivers/mfd/Kconfig | 10
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/rt4831-core.c | 124
> > ++
> > 3 files changed, 135 insertions(+)
> > create mode 100644 drivers/mfd/rt4831-core.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 8b99a13..dfb2640 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> > southbridge which provides access to GPIOs and Watchdog using the
> > southbridge PCI device configuration space.
> >
> > +config MFD_RT4831
> > + tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > + depends on I2C
> > + select MFD_CORE
> > + select REGMAP_I2C
> > + help
> > + This enables support for the Richtek RT4831 that includes 4 channel
> > + WLED driving and Display Bias Voltage. It's commonly used to provide
> > + power to the LCD display and LCD backlight.
> > +
> > config MFD_RT5033
> > tristate "Richtek RT5033 Power Management IC"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 1780019..4108141 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> > obj-$(CONFIG_MFD_HI6421_PMIC)+= hi6421-pmic-core.o
> > obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> > obj-$(CONFIG_MFD_DLN2) += dln2.o
> > +obj-$(CONFIG_MFD_RT4831) += rt4831-core.o
>
> Why is this called -core ...
>
> > obj-$(CONFIG_MFD_RT5033) += rt5033.o
>
> ... and this isn't?
>
Ok, I'm rename the mfd file to rt4831 only.
Due to this mfd is the parent of all sub device, to use 'core' is
trying to distinguish from rt4831-regulator or rt4831-backlight.
My original thought is not to let the user confused.
If to add the postfix '-core' in the file name is bad, I think it can
be removed.
> > obj-$(CONFIG_MFD_SKY81452) += sky81452.o
> >
> > diff --git a/drivers/mfd/rt4831-core.c b/drivers/mfd/rt4831-core.c
> > new file mode 100644
> > index ..f837c06
> > --- /dev/null
> > +++ b/drivers/mfd/rt4831-core.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2020 Richtek Technology Corp.
> > + *
> > + * Author: ChiYuan Huang
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define RT4831_REG_REVISION 0x01
> > +#define RT4831_REG_ENABLE0x08
> > +#define RT4831_REG_I2CPROT 0x15
> > +
> > +#define RICHTEK_VID 0x03
> > +#define RT4831_VID_MASK GENMASK(1, 0)
> > +#define RT4831_RESET_MASKBIT(7)
> > +#define RT4831_I2CSAFETMR_MASK BIT(0)
> > +
> > +static const struct mfd_cell rt4831_subdevs[] = {
> > + OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0,
> > "richtek,rt4831-backlight"),
> > + MFD_CELL_NAME("rt4831-regulator")
> > +};
>
> Just a little note about these helpers. I'm planning on unifying the
> names pretty soon. So if you have to rebase, please watch out for the
> rename.
>
> Essentially OF_MFD_CELL() will soon become MFD_CELL_OF().
>
Yes, I'll check this change. BTW, is it possible to get this patch
that I can integrate it into my codebase in advance.
> > +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
> > +{
> > + if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> > + return true;
> > + return false;
> > +}
> > +
> > +static const struct regmap_config rt4831_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = RT
Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
Hi, Lee: Lee Jones 於 2020年12月15日 週二 下午3:53寫道： > > On Mon, 14 Dec 2020, Daniel Thompson wrote: > > > On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote: > > > Hi, > > > > > > Daniel Thompson 於 2020年12月14日 週一 下午5:59寫道： > > > > > > > > Hi CY > > > > > > > > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote: > > > > > From: ChiYuan Huang > > > > > > > > > > Adds DT binding document for Richtek RT4831 backlight. > > > > > > > > > > Signed-off-by: ChiYuan Huang > > > > > > > > This patch got keyword filtered and brought to my attention > > > > but the rest of the series did not. > > > > > > > > If it was a backlight patch series you need to send it To: the > > > > all the backlight maintainers. > > > > > > > Yes, I'm waiting for mfd reviewing. > > > Due to mfd patch, I need to add backlight dt-binding patch prior to > > > backlight source code. > > > Or autobuild robot will said mfd dt-binding build fail from Rob. > > > That's why I send the backlight dt-binding prior to the source code. > > > > > > I still have backlight/regulator source code patch after mfd reviewing. > > > Do you want me to send all the patches without waiting for mfd reviewing? > > > > To some extent it's up to you. > > > > I think I would have shared all the pieces at once (although not it Lee, > > as mfd maintainer, had suggested otherwise). > > You should not need to concern yourself with patch ordering outside > of the realms of the set i.e. [PATCH 1/x], [PATCH 2/x], etc. > > If you just send the whole patch set and you do not specify otherwise, > it will be applied, in order, as a set. > > Sending subsystem patches without the correct maintainers as recipients > is bad form. Many of us have filters on, so this tactic will seldom > work in any case. > In my case, there're mfd/backlight/regulator for RT4831. You mean I can just send the whole patch set directly to whole mfd/backlight/regulator maintainers. And you can filter like as the keyword to review the related contents, right? >From my original thought, the order is mfd -> backlight-> regulator, one by one due to different maintainers. Maybe I think too much about the patch ordering If so, after getting the comment from Rob, I'll send the whole patch to you. Thanks for the notice. > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
cy_huang(黃啟原) 於 2020年12月15日 週二 下午1:08寫道： > > On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote: > > > > Hi, > > > > Daniel Thompson 於 2020年12月14日 週一 > > 下午5:59寫道： > > > > > > > > > Hi CY > > > > > > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote: > > > > > > > > From: ChiYuan Huang > > > > > > > > Adds DT binding document for Richtek RT4831 backlight. > > > > > > > > Signed-off-by: ChiYuan Huang > > > This patch got keyword filtered and brought to my attention > > > but the rest of the series did not. > > > > > > If it was a backlight patch series you need to send it To: the > > > all the backlight maintainers. > > > > > Yes, I'm waiting for mfd reviewing. > > Due to mfd patch, I need to add backlight dt-binding patch prior to > > backlight source code. > > Or autobuild robot will said mfd dt-binding build fail from Rob. > > That's why I send the backlight dt-binding prior to the source code. > > > > I still have backlight/regulator source code patch after mfd > > reviewing. > > Do you want me to send all the patches without waiting for mfd > > reviewing? > What happened to the regulator part of the binding? I said you could > merge it into the mfd schema, not drop it. Bindings should be complete > so we get a full picture of a device. > Sorry I found the gmail account already loop in. Just my gmail problem. I cannot see the email in it. The reply is below. Yes, I remove the regulator dt-binding and directly merge into mfd schema. Could you check the v4 3/3 patch? Or you just want me to remove regulator dt-binding example, not whole dt-binding file? > > Rob > * Email Confidentiality Notice > > The information contained in this e-mail message (including any attachments) > may be confidential, proprietary, privileged, or otherwise exempt from > disclosure under applicable laws. It is intended to be conveyed only to the > designated recipient(s). Any use, dissemination, distribution, printing, > retaining or copying of this e-mail (including its attachments) by unintended > recipient(s) is strictly prohibited and may be unlawful. If you are not an > intended recipient of this e-mail, or believe that you have received this > e-mail in error, please notify the sender immediately (by replying to this > e-mail), delete any and all copies of this e-mail (including any attachments) > from your system, and do not disclose the content of this e-mail to any other > person. Thank you!
Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
Hi,
Daniel Thompson 於 2020年12月14日 週一 下午5:59寫道：
>
> Hi CY
>
> On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Adds DT binding document for Richtek RT4831 backlight.
> >
> > Signed-off-by: ChiYuan Huang
>
> This patch got keyword filtered and brought to my attention
> but the rest of the series did not.
>
> If it was a backlight patch series you need to send it To: the
> all the backlight maintainers.
>
Yes, I'm waiting for mfd reviewing.
Due to mfd patch, I need to add backlight dt-binding patch prior to
backlight source code.
Or autobuild robot will said mfd dt-binding build fail from Rob.
That's why I send the backlight dt-binding prior to the source code.
I still have backlight/regulator source code patch after mfd reviewing.
Do you want me to send all the patches without waiting for mfd reviewing?
>
> Daniel.
>
>
> > ---
> > since v3
> > - Move inlcude/dt-bindings/leds/rt4831-backlight.h from v3 mfd binding
> > patch to here.
> > - Add dual license tag in header and backlight binding document.
> > - Left backlight dt-binding example only.
> > ---
> > .../leds/backlight/richtek,rt4831-backlight.yaml | 76
> > ++
> > include/dt-bindings/leds/rt4831-backlight.h| 23 +++
> > 2 files changed, 99 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
> >
> > diff --git
> > a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> >
> > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > new file mode 100644
> > index ..f24c8d1
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > @@ -0,0 +1,76 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT4831 Backlight
> > +
> > +maintainers:
> > + - ChiYuan Huang
> > +
> > +description: |
> > + RT4831 is a mutifunctional device that can provide power to the LCD
> > display
> > + and LCD backlight.
> > +
> > + For the LCD backlight, it can provide four channel WLED driving
> > capability.
> > + Each channel driving current is up to 30mA
> > +
> > + Datasheet is available at
> > + https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> > +
> > +properties:
> > + compatible:
> > +const: richtek,rt4831-backlight
> > +
> > + default-brightness:
> > +description: |
> > + The default brightness that applied to the system on start-up.
> > +$ref: /schemas/types.yaml#/definitions/uint32
> > +minimum: 0
> > +maximum: 2048
> > +
> > + max-brightness:
> > +description: |
> > + The max brightness for the H/W limit
> > +$ref: /schemas/types.yaml#/definitions/uint32
> > +minimum: 0
> > +maximum: 2048
> > +
> > + richtek,pwm-enable:
> > +description: |
> > + Specify the backlight dimming following by PWM duty or by SW control.
> > +type: boolean
> > +
> > + richtek,bled-ovp-sel:
> > +description: |
> > + Backlight OVP level selection, currently support 17V/21V/25V/29V.
> > +$ref: /schemas/types.yaml#/definitions/uint8
> > +default: 1
> > +minimum: 0
> > +maximum: 3
> > +
> > + richtek,channel-use:
> > +description: |
> > + Backlight LED channel to be used.
> > + BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable or
> > disable.
> > +$ref: /schemas/types.yaml#/definitions/uint8
> > +minimum: 1
> > +maximum: 15
> > +
> > +required:
> > + - compatible
> > + - richtek,channel-use
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +backlight {
> > + compatible = "richtek,rt4831-backlight";
> > + default-brightness = <1024>;
> > + max-brightness = <2048>;
> > + richtek,bled-ovp-sel = /bits/ 8 ;
> > + richtek,channel-use = /bits/ 8 ;
> > +};
> > diff --git a/include/dt-bindings/leds/r
Re: [PATCH v3 3/4] regulator: rt4831: Adds DT binding document for Richtek RT4831 DSV regulator
Rob Herring 於 2020年12月10日 週四 下午11:17寫道：
>
> On Tue, Dec 08, 2020 at 11:54:45PM +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Adds DT binding document for Richtek RT4831 DSV regulator.
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > since v3
> > - Add dual license tag in regulator binding document.
> > - Left regulator dt-binding example only.
> > ---
> > .../regulator/richtek,rt4831-regulator.yaml| 57
> > ++
> > 1 file changed, 57 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/regulator/richtek,rt4831-regulator.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/regulator/richtek,rt4831-regulator.yaml
> > b/Documentation/devicetree/bindings/regulator/richtek,rt4831-regulator.yaml
> > new file mode 100644
> > index ..c6741f2
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/regulator/richtek,rt4831-regulator.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/regulator/richtek,rt4831-regulator.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT4831 Display Bias Voltage Regulator
> > +
> > +maintainers:
> > + - ChiYuan Huang
> > +
> > +description: |
> > + RT4831 is a multifunctional device that can provide power to the LCD
> > display
> > + and LCD backlight.
> > +
> > + For Display Bias Voltage DSVP and DSVN, the output range is about 4V to
> > 6.5V.
> > + It is sufficient to meet the current LCD power requirement.
> > +
> > + DSVLCM is a boost regulator in IC internal as DSVP and DSVN input power.
> > + Its voltage should be configured above 0.15V to 0.2V gap larger than the
> > + voltage needed for DSVP and DSVN. Too much voltage gap could improve the
> > + voltage drop from the heavy loading scenario. But it also make the power
> > + efficiency worse. It's a trade-off.
> > +
> > + Datasheet is available at
> > + https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> > +
> > +patternProperties:
> > + "^DSV(LCM|P|N)$":
> > +type: object
> > +$ref: regulator.yaml#
> > +description:
> > + Properties for single Display Bias Voltage regulator.
>
> Just put this into the MFD schema directly if you don't have any custom
> properties to add.
>
OK, I'll directly remove rt4831 regulator dt-binding and put the
descriptions into mfd binding.
Ack in next series of patch.
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +regulators {
> > + DSVLCM {
> > +regulator-min-microvolt = <400>;
> > +regulator-max-microvolt = <715>;
> > +regulator-allow-bypass;
> > + };
> > + DSVP {
> > +regulator-name = "rt4831-dsvp";
> > +regulator-min-microvolt = <400>;
> > +regulator-max-microvolt = <650>;
> > +regulator-boot-on;
> > + };
> > + DSVN {
> > +regulator-name = "rt4831-dsvn";
> > +regulator-min-microvolt = <400>;
> > +regulator-max-microvolt = <650>;
> > +regulator-boot-on;
> > + };
> > +};
> > --
> > 2.7.4
> >
Re: [PATCH v2 2/4] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
Rob Herring 於 2020年12月8日 週二 上午12:42寫道：
>
> On Fri, Dec 04, 2020 at 12:06:33AM +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Adds DT binding document for Richtek RT4831 backlight.
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > .../leds/backlight/richtek,rt4831-backlight.yaml | 86
> > ++
> > 1 file changed, 86 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> >
> > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > new file mode 100644
> > index ..df1439a
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > @@ -0,0 +1,86 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT4831 Backlight
> > +
> > +maintainers:
> > + - ChiYuan Huang
> > +
> > +description: |
> > + RT4831 is a mutifunctional device that can provide power to the LCD
> > display
> > + and LCD backlight.
> > +
> > + For the LCD backlight, it can provide four channel WLED driving
> > capability.
> > + Each channel driving current is up to 30mA
> > +
> > + Datasheet is available at
> > + https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> > +
> > +properties:
> > + compatible:
> > +const: richtek,rt4831-backlight
> > +
> > + default-brightness:
> > +description: |
> > + The default brightness that applied to the system on start-up.
> > +$ref: /schemas/types.yaml#/definitions/uint32
> > +minimum: 0
> > +maximum: 2048
> > +
> > + max-brightness:
> > +description: |
> > + The max brightness for the H/W limit
> > +$ref: /schemas/types.yaml#/definitions/uint32
> > +minimum: 0
> > +maximum: 2048
> > +
> > + richtek,pwm-enable:
> > +description: |
> > + Specify the backlight dimming following by PWM duty or by SW control.
> > +type: boolean
> > +
> > + richtek,bled-ovp-sel:
> > +description: |
> > + Backlight OVP level selection, currently support 17V/21V/25V/29V.
> > +$ref: /schemas/types.yaml#/definitions/uint8
> > +default: 1
> > +minimum: 0
> > +maximum: 3
> > +
> > + richtek,channel-use:
> > +description: |
> > + Backlight LED channel to be used.
> > + BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable or
> > disable.
> > +$ref: /schemas/types.yaml#/definitions/uint8
> > +minimum: 1
> > +maximum: 15
> > +
> > +required:
> > + - compatible
> > + - richtek,channel-use
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + rt4831@11 {
> > +compatible = "richtek,rt4831";
> > +reg = <0x11>;
> > +
> > +backlight {
> > + compatible = "richtek,rt4831-backlight";
> > + default-brightness = <1024>;
> > + max-brightness = <2048>;
> > + richtek,bled-ovp-sel = /bits/ 8 ;
> > + richtek,channel-use = /bits/ 8 ;
> > +};
> > + };
>
> Just do 1 complete example in the mfd binding.
>
Ack.
> > +};
> > --
> > 2.7.4
> >
Re: [PATCH v2 4/4] mfd: rt4831: Adds DT binding document for Richtek RT4831 MFD core
Rob Herring 於 2020年12月8日 週二 上午12:41寫道：
>
> On Fri, Dec 04, 2020 at 12:06:35AM +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Adds DT binding document for Richtek RT4831 MFD core.
> >
> > This patch depends on
> >
> > "backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight".
> > "regulator: rt4831: Adds DT binding document for Richtek RT4831 DSV
> > regulator".
>
> You generally don't need to state dependencies on other patches in the
> series. You need to state dependencies that are either pending in a
> maintainers tree or patches not yet applied. IOW, anything not in Linus'
> tree. And that information goes below the '---'.
Ok, I'll add the depend on tag below ---
>
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > Changes since v2
> > - Add "patch depends on" in commit description.
> > - Adds regulator-allow-bypass flag in DSVLCM.
> > ---
> > .../devicetree/bindings/mfd/richtek,rt4831.yaml| 90
> > ++
> > include/dt-bindings/leds/rt4831-backlight.h| 23 ++
>
> This goes in the backlight binding patch.
>
> > 2 files changed, 113 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> > create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> > b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> > new file mode 100644
> > index ..c6ca953
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> > @@ -0,0 +1,90 @@
> > +# SPDX-License-Identifier: GPL-2.0
>
> For new bindings:
>
> (GPL-2.0-only OR BSD-2-Clause)
>
Ack, change three binding files for dual license.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/richtek,rt4831.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT4831 DSV and Backlight Integrated IC
> > +
> > +maintainers:
> > + - ChiYuan Huang
> > +
> > +description: |
> > + RT4831 is a multifunctional device that can provide power to the LCD
> > display
> > + and LCD backlight.
> > +
> > + For Display Bias Voltage DSVP and DSVN, the output range is about 4V to
> > 6.5V.
> > + It's sufficient to meet the current LCD power requirement.
> > +
> > + For the LCD backlight, it can provide four channel WLED driving
> > capability.
> > + Each channel driving current is up to 30mA
> > +
> > + Datasheet is available at
> > + https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> > +
> > +properties:
> > + compatible:
> > +const: richtek,rt4831
> > +
> > + reg:
> > +description: I2C device address.
> > +maxItems: 1
> > +
> > + enable-gpios:
> > +description: |
> > + GPIO to enable/disable the chip. It is optional.
> > + Some usage directly tied this pin to follow VIO 1.8V power on
> > sequence.
> > +maxItems: 1
> > +
> > + regulators:
> > +$ref: ../regulator/richtek,rt4831-regulator.yaml
> > +
> > + backlight:
> > +$ref: ../leds/backlight/richtek,rt4831-backlight.yaml
> > +
> > +required:
> > + - compatible
> > + - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + rt4831@11 {
> > +compatible = "richtek,rt4831";
> > +reg = <0x11>;
> > +
> > +regulators {
> > + DSVLCM {
> > +regulator-min-microvolt = <400>;
> > +regulator-max-microvolt = <715>;
> > +regulator-allow-bypass;
> > + };
> > + DSVP {
> > +regulator-name = "rt4831-dsvp";
> > +regulator-min-microvolt = <400>;
> > +regulator-max-microvolt = <650>;
> > +regulator-boot-on;
> > + };
> > + DSVN {
> > +regulator-name = "rt4831-dsvn";
> > +regulator-min-microvolt = <400>;
> > +regulator-max-microvolt = <650>;
> > +regulator-boot-on;
> > + };
>
Re: [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core
Lee Jones 於 2020年12月2日 週三 下午4:49寫道：
>
> On Wed, 02 Dec 2020, ChiYuan Huang wrote:
>
> > Lee Jones 於 2020年11月26日 週四 上午12:42寫道：
> > >
> > > On Mon, 02 Nov 2020, cy_huang wrote:
> > >
> > > > From: ChiYuan Huang
> > > >
> > > > Adds support Richtek RT4831 MFD core.
> > > > RT4831 includes backlight and DSV part that can provode display panel
> > > > for postive and negative voltage and WLED driving.
> > > >
> > > > Signed-off-by: ChiYuan Huang
> > > > ---
> > > > drivers/mfd/Kconfig | 11 +
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/rt4831-core.c | 119
> > > > ++
> > > > 3 files changed, 131 insertions(+)
> > > > create mode 100644 drivers/mfd/rt4831-core.c
> > > >
> > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > index 8b99a13..a22f002 100644
> > > > --- a/drivers/mfd/Kconfig
> > > > +++ b/drivers/mfd/Kconfig
> > > > @@ -1088,6 +1088,17 @@ config MFD_RDC321X
> > > > southbridge which provides access to GPIOs and Watchdog using
> > > > the
> > > > southbridge PCI device configuration space.
> > > >
> > > > +config MFD_RT4831
> > > > + tristate "Richtek RT4831 WLED and DSV IC"
> > >
> > > Please expand on WLED and DSV.
> > >
> > > This is documentation and should leave nothing to the imagination.
> > >
> > Rewrite to "Richtek RT4831 four channel WLED and display bias
> > voltage", is it okay?
>
> I had to look-up WLED, but I guess it's okay.
>
> "Display Bias Voltage"
>
OK, I'll change this line to
"Richtek RT4831 fource channel WLED and Display Bias Voltage"
> > > > + depends on I2C
> > > > + select MFD_CORE
> > > > + select REGMAP_I2C
> > > > + help
> > > > + This enables support for the Richtek RT4831.
> > > > + RT4831 includes WLED driver and DisplayBias voltage(+/-)
> > > > regulator.
> > > > + It's common used to provide the display power and to drive the
> > > > + display backlight WLED.
> > >
> > > Please don't line-wrap unnecessarily.
> > >
> > > Please re-work the last sentence, as it doesn't quite make sense.
> > >
> > Rewrite the whole sentence like as below
> > "This enables support for the Richtek RT4831. It includes 4 channel
> > WLED driving and Display Bias voltage output. It's commonly used to
> > provide the LCD power and to drive LCD backlight."
>
> "Display Bias Voltage"
>
> "provide power to the LCD display"
>
Thanks. looks better
> [...]
>
> > > > +static int rt4831_probe(struct i2c_client *client)
> > > > +{
> > > > + struct gpio_desc *enable;
> > > > + struct regmap *regmap;
> > > > + unsigned int val;
> > > > + int ret;
> > > > +
> > > > + enable = devm_gpiod_get_optional(&client->dev, "enable",
> > > > GPIOD_OUT_HIGH);
> > > > + if (IS_ERR(enable)) {
> > > > + dev_err(&client->dev, "Failed to get chip enable gpio\n");
> > >
> > > "Failed to get 'enable' GPIO chip"
> > >
> > May I remove "chip" word? It seems redundant.
> > "Failed to get 'enable' GPIO" is better.
> > Because 'enable' is a physical input pin for RT4831.
>
> Sounds good.
>
> [...]
>
> > > > +static int rt4831_remove(struct i2c_client *client)
> > > > +{
> > > > + struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > > +
> > > > + /* Make sure all off before module removal */
> > >
> > > "Disable all are disabled before ..."
>
> This should have said:
>
> "Ensure all are disabled before ..."
>
> > May I rewrite it to "Configure WLED driving and DSV output all to be
> > disabled before MFD module removal"?
>
> You don't need to mention MFD or modules here since we know how the
> Device Driver model works and what .remove() does.
>
> What about:
>
> "Disable WLED and DSV outputs"
Do you mean that only keep this comment line is better? NO more
redundant description like "before ".
If yes, I'll only leave the comment like as you said in remove/shutdown ops.
> > > > + return regmap_update_bits(regmap, RT4831_REG_ENABLE,
> > > > RT4831_RESET_MASK, RT4831_RESET_MASK);
> > > > +}
> > > > +
> > > > +static void rt4831_shutdown(struct i2c_client *client)
> > > > +{
> > > > + struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > > +
> > > > + /* Make sure all off before machine shutdown */
> > >
> > > As above.
> > >
> > like as above " before 'machine shutdown'
>
> "Disable WLED and DSV outputs"
Same as above.
Thanks for all the suggestion.
If any misunderstanding, please kindly let me know.
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core
Lee Jones 於 2020年11月26日 週四 上午12:42寫道：
>
> On Mon, 02 Nov 2020, cy_huang wrote:
>
> > From: ChiYuan Huang
> >
> > Adds support Richtek RT4831 MFD core.
> > RT4831 includes backlight and DSV part that can provode display panel
> > for postive and negative voltage and WLED driving.
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > drivers/mfd/Kconfig | 11 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/rt4831-core.c | 119
> > ++
> > 3 files changed, 131 insertions(+)
> > create mode 100644 drivers/mfd/rt4831-core.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 8b99a13..a22f002 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1088,6 +1088,17 @@ config MFD_RDC321X
> > southbridge which provides access to GPIOs and Watchdog using the
> > southbridge PCI device configuration space.
> >
> > +config MFD_RT4831
> > + tristate "Richtek RT4831 WLED and DSV IC"
>
> Please expand on WLED and DSV.
>
> This is documentation and should leave nothing to the imagination.
>
Rewrite to "Richtek RT4831 four channel WLED and display bias
voltage", is it okay?
> > + depends on I2C
> > + select MFD_CORE
> > + select REGMAP_I2C
> > + help
> > + This enables support for the Richtek RT4831.
> > + RT4831 includes WLED driver and DisplayBias voltage(+/-) regulator.
> > + It's common used to provide the display power and to drive the
> > + display backlight WLED.
>
> Please don't line-wrap unnecessarily.
>
> Please re-work the last sentence, as it doesn't quite make sense.
>
Rewrite the whole sentence like as below
"This enables support for the Richtek RT4831. It includes 4 channel
WLED driving and Display Bias voltage output. It's commonly used to
provide the LCD power and to drive LCD backlight."
> > config MFD_RT5033
> > tristate "Richtek RT5033 Power Management IC"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 1780019..4108141 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> > obj-$(CONFIG_MFD_HI6421_PMIC)+= hi6421-pmic-core.o
> > obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> > obj-$(CONFIG_MFD_DLN2) += dln2.o
> > +obj-$(CONFIG_MFD_RT4831) += rt4831-core.o
> > obj-$(CONFIG_MFD_RT5033) += rt5033.o
> > obj-$(CONFIG_MFD_SKY81452) += sky81452.o
> >
> > diff --git a/drivers/mfd/rt4831-core.c b/drivers/mfd/rt4831-core.c
> > new file mode 100644
> > index ..5342959
> > --- /dev/null
> > +++ b/drivers/mfd/rt4831-core.c
> > @@ -0,0 +1,119 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> No copyright?
>
Yes, I'll add the copyright like as below
/*
* Copyright (c) 2020 Richtek Inc.
*
* Author: ChiYuan Huang
*/
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define RT4831_REG_REVISION 0x01
> > +#define RT4831_REG_ENABLE0x08
> > +#define RT4831_REG_I2CPROT 0x15
> > +
> > +#define RICHTEK_VID 0x03
> > +#define RT4831_VID_MASK GENMASK(1, 0)
> > +#define RT4831_RESET_MASKBIT(7)
> > +#define RT4831_I2CSAFETMR_MASK BIT(0)
> > +
> > +static const struct mfd_cell rt4831_subdevs[] = {
> > + OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0,
> > "richtek,rt4831-backlight"),
> > + MFD_CELL_NAME("rt4831-regulator")
> > +};
> > +
> > +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
> > +{
> > + if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> > + return true;
> > + return false;
> > +}
> > +
> > +static const struct regmap_config rt4831_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = RT4831_REG_I2CPROT,
> > +
> > + .readable_reg = rt4831_is_accessible_reg,
> > + .writeable_reg = rt4831_is_accessible_reg,
> > +};
> > +
> > +static int rt4831_probe(struct i2c_client *client)
> > +{
> > + struct gpio_desc *enable;
> > + struct regmap *regmap;
> > + unsigned int val;
> > + int ret;
> > +
> > +
Re: [PATCH v2 1/2] leds: rt4505: Add support for Richtek RT4505 flash LED controller
Hi, Jacek/Pavel: Linus Walleij 於 2020年11月16日 週一 上午7:55寫道： > > On Mon, Nov 2, 2020 at 3:42 AM cy_huang wrote: > > > drivers/leds/flash/Kconfig | 17 ++ > > drivers/leds/flash/Makefile | 2 + > > drivers/leds/flash/leds-rt4505.c | 430 > > +++ > > Hm Pavel also asked me to create this directory and structure, > so if this gets applied to the MFD tree there will be some > serious clash. > > I can rebase on this patch set if Lee applies it first, they you > need an immutable branch from Lee first to set up the build > infrastructure in the LEDs tree, and then I can rebase on your > tree once you have pulled in that. > > Yours, > Linus Walleij >From linus walleij's mail, how can I support him? Or just wait my patch to be committed on linux main stream? Due to this, how's the status about rt4505 patch? Sincerely, ChiYuan Huang.
Re: [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core
HI, Lee:
Any advice about this rt4831 mfd patch?
cy_huang 於 2020年11月2日 週一 上午11:13寫道：
>
> From: ChiYuan Huang
>
> Adds support Richtek RT4831 MFD core.
> RT4831 includes backlight and DSV part that can provode display panel
> for postive and negative voltage and WLED driving.
>
> Signed-off-by: ChiYuan Huang
> ---
> drivers/mfd/Kconfig | 11 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/rt4831-core.c | 119
> ++
> 3 files changed, 131 insertions(+)
> create mode 100644 drivers/mfd/rt4831-core.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b99a13..a22f002 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1088,6 +1088,17 @@ config MFD_RDC321X
> southbridge which provides access to GPIOs and Watchdog using the
> southbridge PCI device configuration space.
>
> +config MFD_RT4831
> + tristate "Richtek RT4831 WLED and DSV IC"
> + depends on I2C
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> + This enables support for the Richtek RT4831.
> + RT4831 includes WLED driver and DisplayBias voltage(+/-) regulator.
> + It's common used to provide the display power and to drive the
> + display backlight WLED.
> +
> config MFD_RT5033
> tristate "Richtek RT5033 Power Management IC"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 1780019..4108141 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> obj-$(CONFIG_MFD_HI6421_PMIC) += hi6421-pmic-core.o
> obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> obj-$(CONFIG_MFD_DLN2) += dln2.o
> +obj-$(CONFIG_MFD_RT4831) += rt4831-core.o
> obj-$(CONFIG_MFD_RT5033) += rt5033.o
> obj-$(CONFIG_MFD_SKY81452) += sky81452.o
>
> diff --git a/drivers/mfd/rt4831-core.c b/drivers/mfd/rt4831-core.c
> new file mode 100644
> index ..5342959
> --- /dev/null
> +++ b/drivers/mfd/rt4831-core.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define RT4831_REG_REVISION0x01
> +#define RT4831_REG_ENABLE 0x08
> +#define RT4831_REG_I2CPROT 0x15
> +
> +#define RICHTEK_VID0x03
> +#define RT4831_VID_MASKGENMASK(1, 0)
> +#define RT4831_RESET_MASK BIT(7)
> +#define RT4831_I2CSAFETMR_MASK BIT(0)
> +
> +static const struct mfd_cell rt4831_subdevs[] = {
> + OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0,
> "richtek,rt4831-backlight"),
> + MFD_CELL_NAME("rt4831-regulator")
> +};
> +
> +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
> +{
> + if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> + return true;
> + return false;
> +}
> +
> +static const struct regmap_config rt4831_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = RT4831_REG_I2CPROT,
> +
> + .readable_reg = rt4831_is_accessible_reg,
> + .writeable_reg = rt4831_is_accessible_reg,
> +};
> +
> +static int rt4831_probe(struct i2c_client *client)
> +{
> + struct gpio_desc *enable;
> + struct regmap *regmap;
> + unsigned int val;
> + int ret;
> +
> + enable = devm_gpiod_get_optional(&client->dev, "enable",
> GPIOD_OUT_HIGH);
> + if (IS_ERR(enable)) {
> + dev_err(&client->dev, "Failed to get chip enable gpio\n");
> + return PTR_ERR(enable);
> + }
> +
> + regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err(&client->dev, "Failed to init regmap\n");
> + return PTR_ERR(regmap);
> + }
> +
> + ret = regmap_read(regmap, RT4831_REG_REVISION, &val);
> + if (ret) {
> + dev_err(&client->dev, "Fail to get version id\n");
> + return ret;
> + }
> +
> + if ((val & RT4831_VID_MASK) != RICHTEK_VID) {
> + dev_err(&client->dev, "VID not matched, val = 0x%02x\n", val);
> + return -ENODEV;
> + }
> +
> + /*
> +* Used to prevent the abnormal shutdown.
> +* If SCL/SDA both keep low for one second to reset HW.
> +*/
Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core
Rob Herring 於 2020年11月3日 週二 上午9:58寫道： > > On Mon, Nov 2, 2020 at 7:14 PM ChiYuan Huang wrote: > > > > Rob Herring 於 2020年11月3日 週二 上午1:21寫道： > > > > > > On Mon, 02 Nov 2020 11:13:23 +0800, cy_huang wrote: > > > > From: ChiYuan Huang > > > > > > > > Adds DT binding document for Richtek RT4831 MFD core. > > > > > > > > Signed-off-by: ChiYuan Huang > > > > --- > > > > .../devicetree/bindings/mfd/richtek,rt4831.yaml| 89 > > > > ++ > > > > include/dt-bindings/leds/rt4831-backlight.h| 23 ++ > > > > 2 files changed, 112 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml > > > > create mode 100644 include/dt-bindings/leds/rt4831-backlight.h > > > > > > > > > > > > > My bot found errors running 'make dt_binding_check' on your patch: > > > > > > yamllint warnings/errors: > > > > > > dtschema/dtc warnings/errors: > > > Unknown file referenced: [Errno 2] No such file or directory: > > > '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/regulator/richtek,rt4831-regulator.yaml' > > > xargs: dt-doc-validate: exited with status 255; aborting > > > make[1]: *** [Documentation/devicetree/bindings/Makefile:59: > > > Documentation/devicetree/bindings/processed-schema-examples.json] Error > > > 124 > > > make: *** [Makefile:1364: dt_binding_check] Error 2 > > > > > > > > > See https://patchwork.ozlabs.org/patch/1391911 > > > > > > The base for the patch is generally the last rc1. Any dependencies > > > should be noted. > > > > > > If you already ran 'make dt_binding_check' and didn't see the above > > > error(s), then make sure 'yamllint' is installed and dt-schema is up to > > > date: > > > > > > pip3 install dtschema --upgrade > > > > > > Please check and re-submit. > > > > > Sorry, I have one question. > > If the richtek,rt4831.yaml is depend upon the other yaml, do I need to > > merge it all into one patch? > > Currently, my submitting order is mfd, backlight, and regulator. > > Each part divided into two patches (one for source code, another for > > dt_binding_document) > > Doesn't have to be 1 patch, but should be one series with MFD coming > last as it references the others. Example goes in the MFD binding. I > need to see a complete picture for what the device is to effectively > review the binding. > Got it. Next, I'll add the regulator and backlight dt-binding into the series patch. And add the description into mfd core dt-binding like as below. This patch depends on "backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight module". "regulator: rt4831: Adds DT binding document for Richtek RT4831 DSV module". > Rob
Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core
Rob Herring 於 2020年11月3日 週二 上午1:21寫道： > > On Mon, 02 Nov 2020 11:13:23 +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Adds DT binding document for Richtek RT4831 MFD core. > > > > Signed-off-by: ChiYuan Huang > > --- > > .../devicetree/bindings/mfd/richtek,rt4831.yaml| 89 > > ++ > > include/dt-bindings/leds/rt4831-backlight.h| 23 ++ > > 2 files changed, 112 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml > > create mode 100644 include/dt-bindings/leds/rt4831-backlight.h > > > > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Unknown file referenced: [Errno 2] No such file or directory: > '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/regulator/richtek,rt4831-regulator.yaml' > xargs: dt-doc-validate: exited with status 255; aborting > make[1]: *** [Documentation/devicetree/bindings/Makefile:59: > Documentation/devicetree/bindings/processed-schema-examples.json] Error 124 > make: *** [Makefile:1364: dt_binding_check] Error 2 > > > See https://patchwork.ozlabs.org/patch/1391911 > > The base for the patch is generally the last rc1. Any dependencies > should be noted. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit. > Sorry, I have one question. If the richtek,rt4831.yaml is depend upon the other yaml, do I need to merge it all into one patch? Currently, my submitting order is mfd, backlight, and regulator. Each part divided into two patches (one for source code, another for dt_binding_document) I have tried dt_binding_check. It cause the error like your bot said. If dt_binding_check want to be pass, it means I should merge all dt binding document into one patch due to the dependency. If yes, I'll do it in next series patch.
Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core
Hi,
I seems I typo the wrong comment headline, not RT4505. It's RT4831.
Please just review the contents, I'll fix it in next series patch.
cy_huang 於 2020年11月2日 週一 上午11:13寫道：
>
> From: ChiYuan Huang
>
> Adds DT binding document for Richtek RT4831 MFD core.
>
> Signed-off-by: ChiYuan Huang
> ---
> .../devicetree/bindings/mfd/richtek,rt4831.yaml| 89
> ++
> include/dt-bindings/leds/rt4831-backlight.h| 23 ++
> 2 files changed, 112 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
>
> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> new file mode 100644
> index ..c602d50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/richtek,rt4831.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT4831 DSV and Backlight Integrated IC
> +
> +maintainers:
> + - ChiYuan Huang
> +
> +description: |
> + RT4831 is a mutifunctional device that can provide display panel power for
> + positive/negative voltage and also display panel wled driving.
> +
> + For the display voltage output, the range is about 4V to 6.5V. It is
> sufficient
> + to meet the current display panel design.
> +
> + For the panel backlight, it can provide four channels driving capability
> + Each driving current is up to 30mA
> +
> + Datasheet is available at
> + https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> +
> +properties:
> + compatible:
> +const: richtek,rt4831
> +
> + reg:
> +description: I2C device address.
> +maxItems: 1
> +
> + enable-gpios:
> +description: |
> + GPIO to enable/disable the chip. It is optional.
> + Some usage directly tied this pin to follow VIO 1.8V power on sequence.
> +maxItems: 1
> +
> + regulators:
> +$ref: ../regulator/richtek,rt4831-regulator.yaml
> +
> + backlight:
> +$ref: ../leds/backlight/richtek,rt4831-backlight.yaml
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rt4831@11 {
> +compatible = "richtek,rt4831";
> +reg = <0x11>;
> +
> +regulators {
> + DSVLCM {
> +regulator-min-microvolt = <400>;
> +regulator-max-microvolt = <715>;
> + };
> + DSVP {
> +regulator-name = "rt4831-dsvp";
> +regulator-min-microvolt = <400>;
> +regulator-max-microvolt = <650>;
> +regulator-boot-on;
> + };
> + DSVN {
> +regulator-name = "rt4831-dsvn";
> +regulator-min-microvolt = <400>;
> +regulator-max-microvolt = <650>;
> +regulator-boot-on;
> + };
> +};
> +
> +backlight {
> + compatible = "richtek,rt4831-backlight";
> + default-brightness = <1024>;
> + max-brightness = <2048>;
> + richtek,bled-ovp-sel = /bits/ 8 ;
> + richtek,channel-use = /bits/ 8 ;
> +};
> + };
> +};
> diff --git a/include/dt-bindings/leds/rt4831-backlight.h
> b/include/dt-bindings/leds/rt4831-backlight.h
> new file mode 100644
> index ..7084906
> --- /dev/null
> +++ b/include/dt-bindings/leds/rt4831-backlight.h
> @@ -0,0 +1,23 @@
> +/*
> + * This header provides constants for rt4831 backlight bindings.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef _DT_BINDINGS_RT4831_BACKLIGHT_H
> +#define _DT_BINDINGS_RT4831_BACKLIGHT_H
> +
> +#define RT4831_BLOVPLVL_17V0
> +#define RT4831_BLOVPLVL_21V1
> +#define RT4831_BLOVPLVL_25V2
> +#define RT4831_BLOVPLVL_29V3
> +
> +#define RT4831_BLED_CH1EN (1 << 0)
> +#define RT4831_BLED_CH2EN (1 << 1)
> +#define RT4831_BLED_CH3EN (1 << 2)
> +#define RT4831_BLED_CH4EN (1 << 3)
> +#define RT4831_BLED_ALLCHEN((1 << 4) - 1)
> +
> +#endif /* _DT_BINDINGS_RT4831_BACKLIGHT_H */
> --
> 2.7.4
>
Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller
Jacek Anaszewski 於 2020年10月28日 週三 下午7:07寫道：
>
> On 10/28/20 5:57 AM, ChiYuan Huang wrote:
> > Hi,
> >
> > Jacek Anaszewski 於 2020年10月28日 週三 上午12:40寫道：
> >>
> >> Hi Pavel, ChiYuan,
> >>
> >> On 10/27/20 9:29 AM, Pavel Machek wrote:
> >>> Hi!
> >>>
> >>>> From: ChiYuan Huang
> >>>>
> >>>> Add support for RT4505 flash led controller. It can support up to 1.5A
> >>>> flash current with hardware timeout and low input voltage
> >>>> protection.
> >>>
> >>> Please use upper-case "LED" everywhere.
> >>>
> >>> This should be 2nd in the series, after DT changes.
> >>>
> >>>> Signed-off-by: ChiYuan Huang
> >>>> ---
> >>>>drivers/leds/Kconfig | 11 ++
> >>>>drivers/leds/Makefile | 1 +
> >>>>drivers/leds/leds-rt4505.c | 397
> >>>> +
> >>>>3 files changed, 409 insertions(+)
> >>>>create mode 100644 drivers/leds/leds-rt4505.c
> >> [...]
> >>>> +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum
> >>>> led_brightness level)
> >>>> +{
> >>>
> >>> 80 columns, where easy.
> >>>
> >>>> +struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv,
> >>>> flash.led_cdev);
> >>>> +u32 val = 0;
> >>>> +int ret;
> >>>> +
> >>>> +mutex_lock(&priv->lock);
> >>>> +
> >>>> +if (level != LED_OFF) {
> >>>> +ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED,
> >>>> RT4505_ITORCH_MASK,
> >>>> + (level - 1) <<
> >>>> RT4505_ITORCH_SHIFT);
> >>>> +if (ret)
> >>>> +goto unlock;
> >>>> +
> >>>> +val = RT4505_TORCH_SET;
> >>>> +}
> >>>> +
> >>>> +ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE,
> >>>> RT4505_ENABLE_MASK, val);
> >>>> +
> >>>> +unlock:
> >>>> +mutex_unlock(&priv->lock);
> >>>> +return ret;
> >>>> +}
> >>>
> >>> Why is the locking needed? What will the /sys/class/leds interface
> >>> look like on system with your flash?
> >>
> >> The locking is needed since this can be called via led_set_brightness()
> >> from any place in the kernel, and especially from triggers.
> >>From this case, It means only led classdev
> > brihtness_get/brightness_set need to be protected.
> > I search led_flash_classdev, it only can be controlled via sysfs or V4l2.
> > Like as described in last mail, flash related operation is protected
> > by led access_lock and v4l2 framework.
> > I'll keep the locking only in led classdev brightness_get/brightness_set
> > API.
> > If I misunderstand something, please help to point out.
>
> Locking have to be used consistently for each access to the resource
> being protected with the lock. Otherwise you can end up in a situation
> when rt4505_torch_brightness_set and rt4505_flash_brightness_set will
> try concurrently alter hardware state. Regardless of how harmful could
> it be in case of this particular device it is certainly against
> programming rules.
>
Sure, any resource access must be protected.
I'll keep the locking like as the original patch.
Thx.
> --
> Best regards,
> Jacek Anaszewski
Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller
Hi,
Jacek Anaszewski 於 2020年10月28日 週三 上午12:40寫道：
>
> Hi Pavel, ChiYuan,
>
> On 10/27/20 9:29 AM, Pavel Machek wrote:
> > Hi!
> >
> >> From: ChiYuan Huang
> >>
> >> Add support for RT4505 flash led controller. It can support up to 1.5A
> >> flash current with hardware timeout and low input voltage
> >> protection.
> >
> > Please use upper-case "LED" everywhere.
> >
> > This should be 2nd in the series, after DT changes.
> >
> >> Signed-off-by: ChiYuan Huang
> >> ---
> >> drivers/leds/Kconfig | 11 ++
> >> drivers/leds/Makefile | 1 +
> >> drivers/leds/leds-rt4505.c | 397
> >> +
> >> 3 files changed, 409 insertions(+)
> >> create mode 100644 drivers/leds/leds-rt4505.c
> [...]
> >> +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum
> >> led_brightness level)
> >> +{
> >
> > 80 columns, where easy.
> >
> >> +struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv,
> >> flash.led_cdev);
> >> +u32 val = 0;
> >> +int ret;
> >> +
> >> +mutex_lock(&priv->lock);
> >> +
> >> +if (level != LED_OFF) {
> >> +ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED,
> >> RT4505_ITORCH_MASK,
> >> + (level - 1) << RT4505_ITORCH_SHIFT);
> >> +if (ret)
> >> +goto unlock;
> >> +
> >> +val = RT4505_TORCH_SET;
> >> +}
> >> +
> >> +ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE,
> >> RT4505_ENABLE_MASK, val);
> >> +
> >> +unlock:
> >> +mutex_unlock(&priv->lock);
> >> +return ret;
> >> +}
> >
> > Why is the locking needed? What will the /sys/class/leds interface
> > look like on system with your flash?
>
> The locking is needed since this can be called via led_set_brightness()
> from any place in the kernel, and especially from triggers.
>From this case, It means only led classdev
brihtness_get/brightness_set need to be protected.
I search led_flash_classdev, it only can be controlled via sysfs or V4l2.
Like as described in last mail, flash related operation is protected
by led access_lock and v4l2 framework.
I'll keep the locking only in led classdev brightness_get/brightness_set API.
If I misunderstand something, please help to point out.
Thx.
>
> --
> Best regards,
> Jacek Anaszewski
Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller
ChiYuan Huang 於 2020年10月27日 週二 下午6:46寫道：
>
> Pavel Machek 於 2020年10月27日 週二 下午6:15寫道：
> >
> > Hi!
> >
> > > > Please use upper-case "LED" everywhere.
> > > >
> > > > This should be 2nd in the series, after DT changes.
> > > Sure, will ack in next series patch.
> >
> > Feel free to wait for dt ACKs before resending.
> >
> Yes, sure.
> > > > > + help
> > > > > + This option enables support for the RT4505 flash led
> > > > > controller.
> > > >
> > > > Information where it is used would be welcome here.
> > > How about to add the below line for the extra information?
> > > Usually used to company with the camera device on smartphone/tablet
> > > products
> >
> > Yes, that would help.
> >
> > "It is commonly used in smartphones, such as Bell Packard T899" would
> > be even better.
>
> Sorry, We don't focus on specific products. It's a general part flash
> led controller.
> I'll change it like as below
> "It's commonly used in smartphones and tablets to assist the builtin camera."
> >
> > > > > + ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE,
> > > > > RT4505_ENABLE_MASK, val);
> > > > > +
> > > > > +unlock:
> > > > > + mutex_unlock(&priv->lock);
> > > > > + return ret;
> > > > > +}
> > > >
> > > > Why is the locking needed? What will the /sys/class/leds interface
> > > > look like on system with your flash?
> > >
> > > The original thought is because there's still another way to control
> > > flash like as v4l2.
> > > But after reviewing the source code, led sysfs node will be protected
> > > by led_cdev->led_access.
> > > And V4L2 flash will also be protected by v4l2_fh_is_singular API.
> > > I think the whole locking in the source code code may be removed. Right?
> >
> > Well, maybe you need it, I did not check..
I found all led class is guaranteed by led_cdev->led_access lock.
And v4l2 ctrl handleris guaranteed by ctrl->handler->lock that defined
in v4l2-ctrl.c.
When entering v4l2 control mode, all led sysfs will be disabled.
To remove all lock operation is correct. Due to all operations are guaranteed.
In next series patch, I'll remove it.
How do you think?
> >
> > What will the /sys/class/leds interface look like on system with your flash?
> >
> > > > > + *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true
> > > > > : false;
> > > >
> > > > No need for ? ... part.
> > > Do you mean this function is not needed? If yes, it can be removed.
> > > But if it removed, led sysfs flash_strobe show will be not supported.
> >
> > I meant "replace line with: *state = (val & RT4505_FLASH_GET) ==
> > RT4505_FLASH_GET;"
> Oh, I got it. redundant judgement.
> >
> > > > > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned
> > > > > int reg)
> > > > > +{
> > > > > + if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG &&
> > > > > reg <= RT4505_REG_FLAGS))
> > > > > + return true;
> > > >
> > > > Make this two stagements.
> > > Like as the below one?? Or separate it into two if case.
> > > if (reg == RT4505_REG_RESET ||
> > >reg >= RT4505_REG_CONFIG && reg <= RT4505_REG_FLAGS))
> >
> > That would be fine, too... if you use just one space before "&&" :-).
> Thx.
> >
> > Best regards,
> > Pavel
> > --
> > http://www.livejournal.com/~pavelmachek
Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller
Pavel Machek 於 2020年10月27日 週二 下午6:15寫道：
>
> Hi!
>
> > > Please use upper-case "LED" everywhere.
> > >
> > > This should be 2nd in the series, after DT changes.
> > Sure, will ack in next series patch.
>
> Feel free to wait for dt ACKs before resending.
>
Yes, sure.
> > > > + help
> > > > + This option enables support for the RT4505 flash led controller.
> > >
> > > Information where it is used would be welcome here.
> > How about to add the below line for the extra information?
> > Usually used to company with the camera device on smartphone/tablet
> > products
>
> Yes, that would help.
>
> "It is commonly used in smartphones, such as Bell Packard T899" would
> be even better.
Sorry, We don't focus on specific products. It's a general part flash
led controller.
I'll change it like as below
"It's commonly used in smartphones and tablets to assist the builtin camera."
>
> > > > + ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE,
> > > > RT4505_ENABLE_MASK, val);
> > > > +
> > > > +unlock:
> > > > + mutex_unlock(&priv->lock);
> > > > + return ret;
> > > > +}
> > >
> > > Why is the locking needed? What will the /sys/class/leds interface
> > > look like on system with your flash?
> >
> > The original thought is because there's still another way to control
> > flash like as v4l2.
> > But after reviewing the source code, led sysfs node will be protected
> > by led_cdev->led_access.
> > And V4L2 flash will also be protected by v4l2_fh_is_singular API.
> > I think the whole locking in the source code code may be removed. Right?
>
> Well, maybe you need it, I did not check..
>
> What will the /sys/class/leds interface look like on system with your flash?
>
> > > > + *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true :
> > > > false;
> > >
> > > No need for ? ... part.
> > Do you mean this function is not needed? If yes, it can be removed.
> > But if it removed, led sysfs flash_strobe show will be not supported.
>
> I meant "replace line with: *state = (val & RT4505_FLASH_GET) ==
> RT4505_FLASH_GET;"
Oh, I got it. redundant judgement.
>
> > > > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int
> > > > reg)
> > > > +{
> > > > + if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG && reg
> > > > <= RT4505_REG_FLAGS))
> > > > + return true;
> > >
> > > Make this two stagements.
> > Like as the below one?? Or separate it into two if case.
> > if (reg == RT4505_REG_RESET ||
> >reg >= RT4505_REG_CONFIG && reg <= RT4505_REG_FLAGS))
>
> That would be fine, too... if you use just one space before "&&" :-).
Thx.
>
> Best regards,
> Pavel
> --
> http://www.livejournal.com/~pavelmachek
Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller
Hi, Parvel:
Continue the last mail, I'm confused about the rule 80-characters-per-line.
I check some information about submitting changes.
https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col
Could you help to explain the current rule about this limit? still 80
characters per line? or it has been changed to 100.
ChiYuan Huang 於 2020年10月27日 週二 下午5:27寫道：
>
> Pavel Machek 於 2020年10月27日 週二 下午4:29寫道：
> >
> > Hi!
> >
> > > From: ChiYuan Huang
> > >
> > > Add support for RT4505 flash led controller. It can support up to 1.5A
> > > flash current with hardware timeout and low input voltage
> > > protection.
> >
> > Please use upper-case "LED" everywhere.
> >
> > This should be 2nd in the series, after DT changes.
> Sure, will ack in next series patch.
> >
> > > Signed-off-by: ChiYuan Huang
> > > ---
> > > drivers/leds/Kconfig | 11 ++
> > > drivers/leds/Makefile | 1 +
> > > drivers/leds/leds-rt4505.c | 397
> > > +
> > > 3 files changed, 409 insertions(+)
> > > create mode 100644 drivers/leds/leds-rt4505.c
> >
> > Lets put this into drivers/leds/flash/. (Yes, you'll have to create
> > it).
> OK
> >
> >
> > > + help
> > > + This option enables support for the RT4505 flash led controller.
> >
> > Information where it is used would be welcome here.
> How about to add the below line for the extra information?
> Usually used to company with the camera device on smartphone/tablet products
> >
> > > + It can support up to 1.5A flash strobe current with hardware
> > > timeout
> > > + and low input voltage protection.
> >
> > This does not / should not be here.
> OK
> > > +
> > > +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum
> > > led_brightness level)
> > > +{
> >
> > 80 columns, where easy.
> I'll review all source code to meet the 80 column limit.
> >
> > > + struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv,
> > > flash.led_cdev);
> > > + u32 val = 0;
> > > + int ret;
> > > +
> > > + mutex_lock(&priv->lock);
> > > +
> > > + if (level != LED_OFF) {
> > > + ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED,
> > > RT4505_ITORCH_MASK,
> > > + (level - 1) <<
> > > RT4505_ITORCH_SHIFT);
> > > + if (ret)
> > > + goto unlock;
> > > +
> > > + val = RT4505_TORCH_SET;
> > > + }
> > > +
> > > + ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE,
> > > RT4505_ENABLE_MASK, val);
> > > +
> > > +unlock:
> > > + mutex_unlock(&priv->lock);
> > > + return ret;
> > > +}
> >
> > Why is the locking needed? What will the /sys/class/leds interface
> > look like on system with your flash?
>
> The original thought is because there's still another way to control
> flash like as v4l2.
> But after reviewing the source code, led sysfs node will be protected
> by led_cdev->led_access.
> And V4L2 flash will also be protected by v4l2_fh_is_singular API.
> I think the whole locking in the source code code may be removed. Right?
> >
> > > +static int rt4505_flash_strobe_get(struct led_classdev_flash *fled_cdev,
> > > bool *state)
> > > +{
> > > + struct rt4505_priv *priv = container_of(fled_cdev, struct
> > > rt4505_priv, flash);
> > > + u32 val;
> > > + int ret;
> > > +
> > > + mutex_lock(&priv->lock);
> > > +
> > > + ret = regmap_read(priv->regmap, RT4505_REG_ENABLE, &val);
> > > + if (ret)
> > > + goto unlock;
> > > +
> > > + *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true :
> > > false;
> >
> > No need for ? ... part.
> Do you mean this function is not needed? If yes, it can be removed.
> But if it removed, led sysfs flash_strobe show will be not supported.
> >
> > > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int
> > > reg)
> > > +{
> > > + if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG && reg <=
> > > RT4505_REG_FLAGS))
> > > + return true;
> >
> > Make this two stagements.
> Like as the below one?? Or separate it into two if case.
> if (reg == RT4505_REG_RESET ||
>reg >= RT4505_REG_CONFIG && reg <= RT4505_REG_FLAGS))
> >
> > Otherwise... looks like easy simple driver, thanks.
> >
> > Best regards,
> > Pavel
> > --
> > http://www.livejournal.com/~pavelmachek
Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller
Pavel Machek 於 2020年10月27日 週二 下午4:29寫道：
>
> Hi!
>
> > From: ChiYuan Huang
> >
> > Add support for RT4505 flash led controller. It can support up to 1.5A
> > flash current with hardware timeout and low input voltage
> > protection.
>
> Please use upper-case "LED" everywhere.
>
> This should be 2nd in the series, after DT changes.
Sure, will ack in next series patch.
>
> > Signed-off-by: ChiYuan Huang
> > ---
> > drivers/leds/Kconfig | 11 ++
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-rt4505.c | 397
> > +
> > 3 files changed, 409 insertions(+)
> > create mode 100644 drivers/leds/leds-rt4505.c
>
> Lets put this into drivers/leds/flash/. (Yes, you'll have to create
> it).
OK
>
>
> > + help
> > + This option enables support for the RT4505 flash led controller.
>
> Information where it is used would be welcome here.
How about to add the below line for the extra information?
Usually used to company with the camera device on smartphone/tablet products
>
> > + It can support up to 1.5A flash strobe current with hardware timeout
> > + and low input voltage protection.
>
> This does not / should not be here.
OK
> > +
> > +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum
> > led_brightness level)
> > +{
>
> 80 columns, where easy.
I'll review all source code to meet the 80 column limit.
>
> > + struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv,
> > flash.led_cdev);
> > + u32 val = 0;
> > + int ret;
> > +
> > + mutex_lock(&priv->lock);
> > +
> > + if (level != LED_OFF) {
> > + ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED,
> > RT4505_ITORCH_MASK,
> > + (level - 1) << RT4505_ITORCH_SHIFT);
> > + if (ret)
> > + goto unlock;
> > +
> > + val = RT4505_TORCH_SET;
> > + }
> > +
> > + ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE,
> > RT4505_ENABLE_MASK, val);
> > +
> > +unlock:
> > + mutex_unlock(&priv->lock);
> > + return ret;
> > +}
>
> Why is the locking needed? What will the /sys/class/leds interface
> look like on system with your flash?
The original thought is because there's still another way to control
flash like as v4l2.
But after reviewing the source code, led sysfs node will be protected
by led_cdev->led_access.
And V4L2 flash will also be protected by v4l2_fh_is_singular API.
I think the whole locking in the source code code may be removed. Right?
>
> > +static int rt4505_flash_strobe_get(struct led_classdev_flash *fled_cdev,
> > bool *state)
> > +{
> > + struct rt4505_priv *priv = container_of(fled_cdev, struct
> > rt4505_priv, flash);
> > + u32 val;
> > + int ret;
> > +
> > + mutex_lock(&priv->lock);
> > +
> > + ret = regmap_read(priv->regmap, RT4505_REG_ENABLE, &val);
> > + if (ret)
> > + goto unlock;
> > +
> > + *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true :
> > false;
>
> No need for ? ... part.
Do you mean this function is not needed? If yes, it can be removed.
But if it removed, led sysfs flash_strobe show will be not supported.
>
> > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
> > +{
> > + if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG && reg <=
> > RT4505_REG_FLAGS))
> > + return true;
>
> Make this two stagements.
Like as the below one?? Or separate it into two if case.
if (reg == RT4505_REG_RESET ||
reg >= RT4505_REG_CONFIG && reg <= RT4505_REG_FLAGS))
>
> Otherwise... looks like easy simple driver, thanks.
>
> Best regards,
> Pavel
> --
> http://www.livejournal.com/~pavelmachek
Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
Guenter Roeck 於 2020年10月11日 週日 上午3:31寫道： > > On 10/10/20 4:21 AM, Jun Li wrote: > > > > > >> -Original Message----- > >> From: ChiYuan Huang > >> Sent: Saturday, October 10, 2020 12:06 AM > >> To: Jun Li > >> Cc: Jun Li ; Guenter Roeck ; > >> Greg KH ; Heikki Krogerus > >> ; Linux USB List > >> ; lkml ; > >> cy_huang > >> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, > >> hard_reset_count > >> not reset issue > >> > >> Jun Li 於 2020年10月9日 週五 下午2:12寫道： > >>> > >>> > >>> > >>>> -Original Message- > >>>> From: ChiYuan Huang > >>>> Sent: Wednesday, October 7, 2020 6:13 PM > >>>> To: Jun Li > >>>> Cc: Guenter Roeck ; Greg KH > >>>> ; Heikki Krogerus > >>>> ; Linux USB List > >>>> ; lkml ; > >>>> cy_huang ; Jun Li > >>>> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, > >>>> hard_reset_count not reset issue > >>>> > >>>> ChiYuan Huang 於 2020年10月7日 週三 上午1:39寫 > >> 道： > >>>>> > >>>>> Jun Li 於 2020年10月7日 週三 上午12:52寫 > >> 道： > >>>>>> > >>>>>> ChiYuan Huang 于2020年10月6日周二 下午12:38 > >> 写 > >>>> 道： > >>>>>>> > >>>>>>> Guenter Roeck 於 2020年10月5日 週一 下午 > >> 11:30 > >>>> 寫道： > >>>>>>>> > >>>>>>>> On 10/5/20 4:08 AM, Greg KH wrote: > >>>>>>>> [ ... ] > >>>>>>>>>>> What ever happened with this patch, is there still disagreement? > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Yes, there is. I wouldn't have added the conditional > >>>>>>>>>> without reason, and I am concerned that removing it > >>>>>>>>>> entirely will open > >>>> another problem. > >>>>>>>>>> Feel free to apply, though - I can't prove that my > >>>>>>>>>> concern is valid, and after all we'll get reports from > >>>>>>>>>> the field later if > >>>> it is. > >>>>>>>>> > >>>>>>>>> Ok, can I get an ack so I know who to come back to in the > >>>>>>>>> future if there are issues? :) > >>>>>>>>> > >>>>>>>> > >>>>>>>> Not from me, for the reasons I stated. I would be ok with > >>>>>>>> something > >>>> like: > >>>>>>>> > >>>>>>>> - if (tcpm_port_is_disconnected(port)) > >>>>>>>> + if (tcpm_port_is_disconnected(port) || > >>>>>>>> + (tcpm_cc_is_open(port->cc1) && > >>>>>>>> + tcpm_cc_is_open(port->cc2))) > >>>>>>>> > >>>>>>>> to narrow down the condition. > >>>>>>> > >>>>>>> I have tried the above comment and It doesn't work. > >>>>>>> How about to change the judgement like as below > >>>>>>> > >>>>>>> - if (tcpm_port_is_disconnected(port)) > >>>>>>> + if (tcpm_port_is_disconnected(port) || > >>>>>>> + !port->vbus_present) > >>>>>>> > >>>>>>> The hard_reset_count not reset issue is following by the below > >>>>>>> order 1. VBUS off ( at the same time, cc is still detected as > >>>>>>> attached) > >>>>>>> port->attached become false and cc is not open > >>>>>>> 2. After that, cc detached. > >>>>>>> due to port->attached is false, tcpm_detach() directly return. > >>>>>> > >>>>>> If tcpm_detach() return directly, then how your patch can reset > >>>>>> hard_reset_count? > >>>>>> > >>>>> Yes, it can. We know vbus_present change from true to false and cc > >>>>> detach both trigger tcpm_detach. > >>>>> My change is whenever
Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
Jun Li 於 2020年10月9日 週五 下午2:12寫道：
>
>
>
> > -Original Message-
> > From: ChiYuan Huang
> > Sent: Wednesday, October 7, 2020 6:13 PM
> > To: Jun Li
> > Cc: Guenter Roeck ; Greg KH
> > ; Heikki Krogerus
> > ; Linux USB List
> > ; lkml ;
> > cy_huang ; Jun Li
> > Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc,
> > hard_reset_count
> > not reset issue
> >
> > ChiYuan Huang 於 2020年10月7日 週三 上午1:39寫道：
> > >
> > > Jun Li 於 2020年10月7日 週三 上午12:52寫道：
> > > >
> > > > ChiYuan Huang 于2020年10月6日周二 下午12:38写
> > 道：
> > > > >
> > > > > Guenter Roeck 於 2020年10月5日 週一 下午11:30
> > 寫道：
> > > > > >
> > > > > > On 10/5/20 4:08 AM, Greg KH wrote:
> > > > > > [ ... ]
> > > > > > >>> What ever happened with this patch, is there still disagreement?
> > > > > > >>>
> > > > > > >>
> > > > > > >> Yes, there is. I wouldn't have added the conditional without
> > > > > > >> reason, and I am concerned that removing it entirely will open
> > another problem.
> > > > > > >> Feel free to apply, though - I can't prove that my concern is
> > > > > > >> valid, and after all we'll get reports from the field later if
> > it is.
> > > > > > >
> > > > > > > Ok, can I get an ack so I know who to come back to in the
> > > > > > > future if there are issues? :)
> > > > > > >
> > > > > >
> > > > > > Not from me, for the reasons I stated. I would be ok with something
> > like:
> > > > > >
> > > > > > - if (tcpm_port_is_disconnected(port))
> > > > > > + if (tcpm_port_is_disconnected(port) ||
> > > > > > + (tcpm_cc_is_open(port->cc1) &&
> > > > > > + tcpm_cc_is_open(port->cc2)))
> > > > > >
> > > > > > to narrow down the condition.
> > > > >
> > > > > I have tried the above comment and It doesn't work.
> > > > > How about to change the judgement like as below
> > > > >
> > > > > - if (tcpm_port_is_disconnected(port))
> > > > > + if (tcpm_port_is_disconnected(port) ||
> > > > > + !port->vbus_present)
> > > > >
> > > > > The hard_reset_count not reset issue is following by the below
> > > > > order 1. VBUS off ( at the same time, cc is still detected as
> > > > > attached)
> > > > > port->attached become false and cc is not open
> > > > > 2. After that, cc detached.
> > > > > due to port->attached is false, tcpm_detach() directly return.
> > > >
> > > > If tcpm_detach() return directly, then how your patch can reset
> > > > hard_reset_count?
> > > >
> > > Yes, it can. We know vbus_present change from true to false and cc
> > > detach both trigger tcpm_detach.
> > > My change is whenever tcpm_detach to be called, hard_reset_count will
> > > be reset to zero.
> > >
> > > > I am seeing the same issue on my platform, the proposed change:
> > > > - if (tcpm_port_is_disconnected(port))
> > > > - port->hard_reset_count = 0;
> > > > + port->hard_reset_count = 0;
> > > > can't resolve it on my platform.
> > > >
> > > I'm not sure what's your condition. Could you directly paste the tcpm
> > > log for the check?
> > > > How about reset hard_reset_count in SNK_READY?
> > > > @@ -3325,6 +3329,7 @@ static void run_state_machine(struct tcpm_port
> > *port)
> > > > case SNK_READY:
> > > > port->try_snk_count = 0;
> > > > port->update_sink_caps = false;
> > > > + port->hard_reset_count = 0;
> > > > if (port->explicit_contract) {
> > > > typec_set_pwr_opmode(port->typec_port,
> > > > TYPEC_PWR_MODE_PD);
> > > >
> > > > can this resolve your problem?
> > > I'm not sure. It need to have a try, then I can answer you.
> > > But from USBPD spec,
Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
ChiYuan Huang 於 2020年10月7日 週三 上午1:39寫道：
>
> Jun Li 於 2020年10月7日 週三 上午12:52寫道：
> >
> > ChiYuan Huang 于2020年10月6日周二 下午12:38写道：
> > >
> > > Guenter Roeck 於 2020年10月5日 週一 下午11:30寫道：
> > > >
> > > > On 10/5/20 4:08 AM, Greg KH wrote:
> > > > [ ... ]
> > > > >>> What ever happened with this patch, is there still disagreement?
> > > > >>>
> > > > >>
> > > > >> Yes, there is. I wouldn't have added the conditional without reason,
> > > > >> and I am concerned that removing it entirely will open another
> > > > >> problem.
> > > > >> Feel free to apply, though - I can't prove that my concern is valid,
> > > > >> and after all we'll get reports from the field later if it is.
> > > > >
> > > > > Ok, can I get an ack so I know who to come back to in the future if
> > > > > there are issues? :)
> > > > >
> > > >
> > > > Not from me, for the reasons I stated. I would be ok with something
> > > > like:
> > > >
> > > > - if (tcpm_port_is_disconnected(port))
> > > > + if (tcpm_port_is_disconnected(port) ||
> > > > + (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))
> > > >
> > > > to narrow down the condition.
> > >
> > > I have tried the above comment and It doesn't work.
> > > How about to change the judgement like as below
> > >
> > > - if (tcpm_port_is_disconnected(port))
> > > + if (tcpm_port_is_disconnected(port) || !port->vbus_present)
> > >
> > > The hard_reset_count not reset issue is following by the below order
> > > 1. VBUS off ( at the same time, cc is still detected as attached)
> > > port->attached become false and cc is not open
> > > 2. After that, cc detached.
> > > due to port->attached is false, tcpm_detach() directly return.
> >
> > If tcpm_detach() return directly, then how your patch can reset
> > hard_reset_count?
> >
> Yes, it can. We know vbus_present change from true to false and cc
> detach both trigger tcpm_detach.
> My change is whenever tcpm_detach to be called, hard_reset_count will
> be reset to zero.
>
> > I am seeing the same issue on my platform, the proposed change:
> > - if (tcpm_port_is_disconnected(port))
> > - port->hard_reset_count = 0;
> > + port->hard_reset_count = 0;
> > can't resolve it on my platform.
> >
> I'm not sure what's your condition. Could you directly paste the tcpm
> log for the check?
> > How about reset hard_reset_count in SNK_READY?
> > @@ -3325,6 +3329,7 @@ static void run_state_machine(struct tcpm_port *port)
> > case SNK_READY:
> > port->try_snk_count = 0;
> > port->update_sink_caps = false;
> > + port->hard_reset_count = 0;
> > if (port->explicit_contract) {
> > typec_set_pwr_opmode(port->typec_port,
> > TYPEC_PWR_MODE_PD);
> >
> > can this resolve your problem?
> I'm not sure. It need to have a try, then I can answer you.
> But from USBPD spec, the hard_reset_count need to reset zero only when
> 1. At src state, pe_src_send_cap and receive GoodCRC
> 2. At snk state, pe_snk_evaluate_cap need to reset hard_reset_count
> >
> > Li Jun
> > >
> > > And that's why hard_reset_count is not reset to 0.
I tried in snk_ready to reset hard_reset_count.
At normal case, it works.
But it seems still the possible fail case like as below.
200ms -> cc debounce max time
240ms -> snk_waitcap max time
If the plugin/out period is between (200+240) and (200+ 2* 240)ms ,
and the src side plug out like as the described scenario.
>From this case, hard_reset_count may still 1.
And we expect the next plugin hard_reset_count is 0. But not, actually
it never reset.
So at next plugin, only one hard_reset will be sent and reach
hard_reset_count max (2).
This case is hard to reproduce. But actually it's possible.
> > > >
> > > > Guenter
Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
Jun Li 於 2020年10月7日 週三 上午12:52寫道：
>
> ChiYuan Huang 于2020年10月6日周二 下午12:38写道：
> >
> > Guenter Roeck 於 2020年10月5日 週一 下午11:30寫道：
> > >
> > > On 10/5/20 4:08 AM, Greg KH wrote:
> > > [ ... ]
> > > >>> What ever happened with this patch, is there still disagreement?
> > > >>>
> > > >>
> > > >> Yes, there is. I wouldn't have added the conditional without reason,
> > > >> and I am concerned that removing it entirely will open another problem.
> > > >> Feel free to apply, though - I can't prove that my concern is valid,
> > > >> and after all we'll get reports from the field later if it is.
> > > >
> > > > Ok, can I get an ack so I know who to come back to in the future if
> > > > there are issues? :)
> > > >
> > >
> > > Not from me, for the reasons I stated. I would be ok with something like:
> > >
> > > - if (tcpm_port_is_disconnected(port))
> > > + if (tcpm_port_is_disconnected(port) ||
> > > + (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))
> > >
> > > to narrow down the condition.
> >
> > I have tried the above comment and It doesn't work.
> > How about to change the judgement like as below
> >
> > - if (tcpm_port_is_disconnected(port))
> > + if (tcpm_port_is_disconnected(port) || !port->vbus_present)
> >
> > The hard_reset_count not reset issue is following by the below order
> > 1. VBUS off ( at the same time, cc is still detected as attached)
> > port->attached become false and cc is not open
> > 2. After that, cc detached.
> > due to port->attached is false, tcpm_detach() directly return.
>
> If tcpm_detach() return directly, then how your patch can reset
> hard_reset_count?
>
Yes, it can. We know vbus_present change from true to false and cc
detach both trigger tcpm_detach.
My change is whenever tcpm_detach to be called, hard_reset_count will
be reset to zero.
> I am seeing the same issue on my platform, the proposed change:
> - if (tcpm_port_is_disconnected(port))
> - port->hard_reset_count = 0;
> + port->hard_reset_count = 0;
> can't resolve it on my platform.
>
I'm not sure what's your condition. Could you directly paste the tcpm
log for the check?
> How about reset hard_reset_count in SNK_READY?
> @@ -3325,6 +3329,7 @@ static void run_state_machine(struct tcpm_port *port)
> case SNK_READY:
> port->try_snk_count = 0;
> port->update_sink_caps = false;
> + port->hard_reset_count = 0;
> if (port->explicit_contract) {
> typec_set_pwr_opmode(port->typec_port,
> TYPEC_PWR_MODE_PD);
>
> can this resolve your problem?
I'm not sure. It need to have a try, then I can answer you.
But from USBPD spec, the hard_reset_count need to reset zero only when
1. At src state, pe_src_send_cap and receive GoodCRC
2. At snk state, pe_snk_evaluate_cap need to reset hard_reset_count
>
> Li Jun
> >
> > And that's why hard_reset_count is not reset to 0.
> > >
> > > Guenter
Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
Guenter Roeck 於 2020年10月5日 週一 下午11:30寫道： > > On 10/5/20 4:08 AM, Greg KH wrote: > [ ... ] > >>> What ever happened with this patch, is there still disagreement? > >>> > >> > >> Yes, there is. I wouldn't have added the conditional without reason, > >> and I am concerned that removing it entirely will open another problem. > >> Feel free to apply, though - I can't prove that my concern is valid, > >> and after all we'll get reports from the field later if it is. > > > > Ok, can I get an ack so I know who to come back to in the future if > > there are issues? :) > > > > Not from me, for the reasons I stated. I would be ok with something like: > > - if (tcpm_port_is_disconnected(port)) > + if (tcpm_port_is_disconnected(port) || > + (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2))) > > to narrow down the condition. I have tried the above comment and It doesn't work. How about to change the judgement like as below - if (tcpm_port_is_disconnected(port)) + if (tcpm_port_is_disconnected(port) || !port->vbus_present) The hard_reset_count not reset issue is following by the below order 1. VBUS off ( at the same time, cc is still detected as attached) port->attached become false and cc is not open 2. After that, cc detached. due to port->attached is false, tcpm_detach() directly return. And that's why hard_reset_count is not reset to 0. > > Guenter
Re: [PATCH] regulator: rtmv20: Update DT binding document and property name parsing
Mark Brown 於 2020年9月30日 週三 下午7:06寫道： > > On Wed, Sep 30, 2020 at 06:08:00PM +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > 1. Add vendor suffix to all proprietary properties. > > You should need a driver update to match this one? Yes, sure, I describe in the change item 4. Do you want me to divided the patch into binding document update and driver update?
Re: [PATCH v2 2/2] regulator: rtmv20: Add DT-binding document for Richtek RTMV20
Mark Brown 於 2020年9月30日 週三 下午5:43寫道： > > On Wed, Sep 30, 2020 at 10:23:49AM +0800, ChiYuan Huang wrote: > > >Due to that already merged into your regulator for-next git, may I > > send the patch to fix Rob's comment? > > Of course, yes please. OK, Thx. > > > And I also found one line need to be added into rtmv20 probe phase. > > Please check below. > > /* > > * keep in shutdown mode to minimize the current consumption > > * and also mark regcache as dirty > > */ > > + regcache_cache_only(priv->regmap, true); > > regcache_mark_dirty(priv->regmap); > > gpiod_set_value(priv->enable_gpio, 0); > > > > Can I directly merge into one that includes Rob's comment and the > > above line to be added? > > Please make it a separate patch. I'll send the separate patch by next.
Re: [PATCH v2 2/2] regulator: rtmv20: Add DT-binding document for Richtek RTMV20
Hi, Rob Ack is in below comments. Hi, Mark: Due to that already merged into your regulator for-next git, may I send the patch to fix Rob's comment? And I also found one line need to be added into rtmv20 probe phase. Please check below. /* * keep in shutdown mode to minimize the current consumption * and also mark regcache as dirty */ + regcache_cache_only(priv->regmap, true); regcache_mark_dirty(priv->regmap); gpiod_set_value(priv->enable_gpio, 0); Can I directly merge into one that includes Rob's comment and the above line to be added? Rob Herring 於 2020年9月29日 週二 下午11:06寫道： > > On Mon, Sep 28, 2020 at 03:19:44PM +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Add DT-binding document for Richtek RTMV20 > > > > Signed-off-by: ChiYuan Huang > > --- > > .../regulator/richtek,rtmv20-regulator.yaml| 168 > > + > > 1 file changed, 168 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/regulator/richtek,rtmv20-regulator.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/regulator/richtek,rtmv20-regulator.yaml > > b/Documentation/devicetree/bindings/regulator/richtek,rtmv20-regulator.yaml > > new file mode 100644 > > index ..4cb4b68 > > --- /dev/null > > +++ > > b/Documentation/devicetree/bindings/regulator/richtek,rtmv20-regulator.yaml > > @@ -0,0 +1,168 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/regulator/richtek,rtmv20-regulator.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Richtek RTMV20 laser diode regulator > > + > > +maintainers: > > + - ChiYuan Huang > > + > > +description: | > > + Richtek RTMV20 is a load switch current regulator that can supply up to > > 6A. > > + It is used to drive laser diode. There're two signals for chip controls > > + (Enable/Fail), Enable pin to turn chip on, and Fail pin as fault > > indication. > > + There're still four pins for camera control, two inputs (strobe and > > vsync), > > + the others for outputs (fsin1 and fsin2). Strobe input to start the > > current > > + supply, vsync input from IR camera, and fsin1/fsin2 output for the > > optional. > > + > > +properties: > > + compatible: > > +const: richtek,rtmv20 > > + > > + reg: > > +maxItems: 1 > > + > > + wakeup-source: true > > + > > + interrupts-extend: > > You mean interrupts-extended? > > In any case, use 'interrupts' here and the tooling allows for either. Yes, you're righ. Sorry, I key in the yaml file, line by line. It's really a typo. > > > +maxItems: 1 > > + > > + enable-gpios: > > +description: A connection of the 'enable' gpio line. > > +maxItems: 1 > > + > > + ld-pulse-delay-us: > > +description: | > > + load current pulse delay in microsecond after strobe pin pulse high. > > +$ref: "/schemas/types.yaml#/definitions/uint32" > > Don't need a type ref when you have a standard property unit suffix, so > drop. Ack, remove type ref for all properties that included unit suffix. > > This and all the following need a vendor prefix too. Is it okay to Add "richtek," prefix for all the properties except lsw regulator node? > > > +minimum: 0 > > +maximum: 10 > > +default: 0 > > + > > + ld-pulse-width-us: > > +description: | > > + Load current pulse width in microsecond after strobe pin pulse high. > > +$ref: "/schemas/types.yaml#/definitions/uint32" > > +minimum: 0 > > +maximum: 1 > > +default: 1200 > > + > > + fsin1-delay-us: > > +description: | > > + Fsin1 pulse high delay in microsecond after vsync signal pulse high. > > +$ref: "/schemas/types.yaml#/definitions/uint32" > > +minimum: 0 > > +maximum: 10 > > +default: 23000 > > + > > + fsin1-width-us: > > +description: | > > + Fsin1 pulse high width in microsecond after vsync signal pulse high. > > +$ref: "/schemas/types.yaml#/definitions/uint32" > > +minimum: 40 > > +maximum: 1 > > +default: 160 > > + > > + fsin2-delay-us: > > +description: | > > + Fsin2 pulse high delay in microsecond after vsync signal pulse high. > > +$ref:
Re: [PATCH v1 2/2] regulator: rtmv20: Add DT-binding document for Richtek RTMV20
Re-add into the mail loop list. ChiYuan Huang 於 2020年9月25日 週五 下午12:09寫道： > > Mark Brown 於 2020年9月25日 週五 上午12:24寫道： > > > > On Thu, Sep 24, 2020 at 11:04:51PM +0800, cy_huang wrote: > > > > > + enable-gpio: > > > +description: A connection of the 'enable' gpio line. > > > +maxItems: 1 > > > > -gpios. GPIO properties should always be plural even if there's only > > one GPIO. > > > > > + ld_pulse_delay: > > > > Properties should use - not _ and for all the properties specifying > > things like times you should have units so... > > > > > +description: | > > > + load current pulse delay in microsecond after strobe pin pulse > > > high. > > > +$ref: "/schemas/types.yaml#/definitions/uint32" > > > > ...add -us here. > > > Ack > > > + fsin_enable: > > > +description: Fsin function enable control. > > > +$ref: "/schemas/types.yaml#/definitions/uint32" > > > +minimum: 0 > > > +maximum: 1 > > > +default: 0 > > > > This looks like it should be a boolean property not a number. The same > > is true for most if not all of the other properties with min/max of 0/1. > Ok, just change the property as boolean type.
Re: [PATCH v1 1/2] regulator: rtmv20: Adds support for Richtek RTMV20 load switch regulator
Mark Brown 於 2020年9月25日 週五 下午7:33寫道：
>
> On Fri, Sep 25, 2020 at 12:07:50PM +0800, ChiYuan Huang wrote:
> > Mark Brown 於 2020年9月25日 週五 上午12:12寫道：
>
> Please don't take things off-list unless there is a really strong reason
> to do so. Sending things to the list ensures that everyone gets a
> chance to read and comment on things.
>
Sorry, I press the wrong button to only reply one not reply all.
Loop in the mail list again.
> > > I can't help but feel that this looks like a register cache would be a
> > > simpler way of achieving this.
>
> > This is because of enable gpio limitation. If gpio low to high, all
> > registers will be reset.
> > And enable gpio high to low will cause I2C cannot be accessed.
> > That's why we need to apply register setting after hardware enable pin
> > be pulled high.
> > The consumption current for EN L vs H is also different from 1uA vs
> > 450uA defined as typical values.
>
> That's exactly the sort of situation that regmap caches are usually used
> for, mark the device as cache only when powering off then resync after
> power on.
>
Thx, I think I catch the point.
> > > > + switch (props[i].len) {
> > > > + case RTMV20_2BYTE_ACCES:
> > > > + case RTMV20_1BYTE_ACCES:
>
> > > It feels like this should all be abstracted in the regmap with custom
> > > reg_read() and reg_write() operations - or have two regmaps for the two
> > > different register sizes. Having the register sizes and endianness
> > > handling outside of regmap code seems like an abstraction failure.
>
> > Actually, it's the consecutive register address with two byte data.
> > Lower address defined as val_h, and the higher defined as val_l.
> > So I just use cpu_to_be16 to do the transformation.
>
> Oh, so just a straight regmap would be fine.
>
Thx.
> > From the above hardware description, do you have any suggestion to
> > achieve the register cache?
>
> Look at how other devices use regcache_cache_only().
>
Thx.
> > > Don't do this, the driver should not adjust the system state unless
> > > explicitly told to do so - we don't know if any state changes are safe
> > > on a given board.
>
> > From my original coding, it's put before regulator register.
> > After regulator registered, it will follow the regulator framework to
> > do the turn on/off and the other settings.
> > I think it won't affect the system state, just to keep IC as shutdown
> > mode (1uA) after chip vendor info check.
>
> It depends if the device was enabled before the driver started, and if
> anything started powering on when the device was powered on.
Sure, I'll re-check the flow.
Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
Hi, Guenter: ChiYuan Huang 於 2020年9月6日 週日 下午11:22寫道： > > Guenter Roeck 於 2020年9月5日 週六 下午11:51寫道： > > > > On 9/4/20 6:24 PM, ChiYuan Huang wrote: > > > Guenter Roeck 於 2020年9月5日 週六 上午3:41寫道： > > >> > > >> On 9/3/20 9:21 AM, ChiYuan Huang wrote: > > >>> Guenter Roeck 於 2020年9月3日 週四 上午12:57寫道： > > >>>> > > >>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote: > > >>>>> From: ChiYuan Huang > > >>>>> > > >>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count > > >>>>> won't bt reset for some case. > > >>>>> > > >>>>> Signed-off-by: ChiYuan Huang > > >>>>> --- > > >>>>> Below's the flow. > > >>>>> > > >>>>> _tcpm_pd_vbus_off() -> run_state_machine to change state to > > >>>>> SNK_UNATTACHED > > >>>>> call tcpm_snk_detach() -> tcpm_snk_detach() -> tcpm_detach() > > >>>>> tcpm_port_is_disconnected() will be called. > > >>>>> But port->attached is still true and port->cc1=open and port->cc2=open > > >>>>> > > >>>>> It cause tcpm_port_is_disconnected return false, then > > >>>>> hard_reset_count won't be reset. > > >>>>> After that, tcpm_reset_port() is called. > > >>>>> port->attached become false. > > >>>>> > > >>>>> After that, cc now trigger cc_change event, the hard_reset_count will > > >>>>> be kept. > > >>>>> Even tcpm_detach will be called, due to port->attached is false, > > >>>>> tcpm_detach() > > >>>>> will directly return. > > >>>>> > > >>>>> CC_EVENT will only trigger drp toggling again. > > >>>>> --- > > >>>>> drivers/usb/typec/tcpm/tcpm.c | 3 +-- > > >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) > > >>>>> > > >>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c > > >>>>> b/drivers/usb/typec/tcpm/tcpm.c > > >>>>> index a48e3f90..5c73e1d 100644 > > >>>>> --- a/drivers/usb/typec/tcpm/tcpm.c > > >>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c > > >>>>> @@ -2797,8 +2797,7 @@ static void tcpm_detach(struct tcpm_port *port) > > >>>>> port->tcpc->set_bist_data(port->tcpc, false); > > >>>>> } > > >>>>> > > >>>>> - if (tcpm_port_is_disconnected(port)) > > >>>>> - port->hard_reset_count = 0; > > >>>>> + port->hard_reset_count = 0; > > >>>>> > > >>>> > > >>>> Doesn't that mean that the state machine will never enter > > >>>> error recovery ? > > >>>> > > >>> I think it does't affect the error recovery. > > >>> All error recovery seems to check pd_capable flag. > > >>> > > >>> >From my below case, it's A to C cable only. There is no USBPD contract > > >>> will be estabilished. > > >>> > > >>> This case occurred following by the below test condition > > >>> Cable -> A to C (default Rp bind to vbus) connected to PC. > > >>> 1. first time plugged in the cable with PC > > >>> It will make HARD_RESET_COUNT to be equal 2 > > >>> 2. And then plug out. At that time HARD_RESET_COUNT is till 2. > > >>> 3. next time plugged in again. > > >>> Due to hard_reset_count is still 2 , after wait_cap_timeout, the state > > >>> eventually changed to SNK_READY. > > >>> But during the state transition, no hard_reset be sent. > > >>> > > >>> Defined in the USBPD policy engine, typec transition to USBPD, all > > >>> variables must be reset included hard_reset_count. > > >>> So it expected SNK must send hard_reset again. > > >>> > > >>> The original code defined hard_reset_count must be reset only when > > >>> tcpm_port_is_disconnected. > > >>> > > >>> It doesn't make sense that it only occurred in some scenario. > > >>> If tcpm_detach is
Re: [PATCH v5 2/2] usb typec: mt6360: Add MT6360 Type-C DT binding documentation
Rob Herring 於 2020年9月15日 週二 上午2:29寫道： > > On Tue, 01 Sep 2020 10:40:42 +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Add a devicetree binding documentation for the MT6360 Type-C driver. > > > > Signed-off-by: ChiYuan Huang > > --- > > .../bindings/usb/mediatek,mt6360-tcpc.yaml | 95 > > ++ > > 1 file changed, 95 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml > > > > Reviewed-by: Rob Herring Thx.
Re: [PATCH v5 2/2] usb typec: mt6360: Add MT6360 Type-C DT binding documentation
Hi.
cy_huang 於 2020年9月1日 週二 上午10:40寫道：
>
> From: ChiYuan Huang
>
> Add a devicetree binding documentation for the MT6360 Type-C driver.
>
> Signed-off-by: ChiYuan Huang
> ---
> .../bindings/usb/mediatek,mt6360-tcpc.yaml | 95
> ++
> 1 file changed, 95 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
>
> diff --git a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> new file mode 100644
> index ..1e8e1c2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/usb/mediatek,mt6360-tcpc.yaml#";
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> +
> +title: Mediatek MT6360 Type-C Port Switch and Power Delivery controller DT
> bindings
> +
> +maintainers:
> + - ChiYuan Huang
> +
> +description: |
> + Mediatek MT6360 is a multi-functional device. It integrates charger, ADC,
> flash, RGB indicators,
> + regulators (BUCKs/LDOs), and TypeC Port Switch with Power Delivery
> controller.
> + This document only describes MT6360 Type-C Port Switch and Power Delivery
> controller.
> +
> +properties:
> + compatible:
> +enum:
> + - mediatek,mt6360-tcpc
> +
> + interrupts:
> +maxItems: 1
> +
> + interrupt-names:
> +items:
> + - const: PD_IRQB
> +
> + connector:
> +type: object
> +$ref: ../connector/usb-connector.yaml#
> +description:
> + Properties for usb c connector.
> +
> +additionalProperties: false
> +
> +required:
> + - compatible
> + - interrupts
> + - interrupt-names
> +
> +examples:
> + - |
> +#include
> +#include
> +i2c0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mt6360@34 {
> +compatible = "mediatek,mt6360";
> +reg = <0x34>;
> +tcpc {
> + compatible = "mediatek,mt6360-tcpc";
> + interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-names = "PD_IRQB";
> +
> + connector {
> +compatible = "usb-c-connector";
> +label = "USB-C";
> +data-role = "dual";
> +power-role = "dual";
> +try-power-role = "sink";
> +source-pdos = PDO_FIXED_DATA_SWAP)>;
> +sink-pdos = PDO_FIXED_DATA_SWAP)>;
> +op-sink-microwatt = <1000>;
> +
> +ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> +reg = <0>;
> +endpoint {
> + remote-endpoint = <&usb_hs>;
> +};
> + };
> + port@1 {
> +reg = <1>;
> +endpoint {
> + remote-endpoint = <&usb_ss>;
> +};
> + };
> + port@2 {
> +reg = <2>;
> +endpoint {
> + remote-endpoint = <&dp_aux>;
> +};
> + };
> +};
> + };
> +};
> + };
> +};
> +...
Any comment about the v5 dt-binding document change?
> --
> 2.7.4
>
Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
Guenter Roeck 於 2020年9月5日 週六 下午11:51寫道： > > On 9/4/20 6:24 PM, ChiYuan Huang wrote: > > Guenter Roeck 於 2020年9月5日 週六 上午3:41寫道： > >> > >> On 9/3/20 9:21 AM, ChiYuan Huang wrote: > >>> Guenter Roeck 於 2020年9月3日 週四 上午12:57寫道： > >>>> > >>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote: > >>>>> From: ChiYuan Huang > >>>>> > >>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count > >>>>> won't bt reset for some case. > >>>>> > >>>>> Signed-off-by: ChiYuan Huang > >>>>> --- > >>>>> Below's the flow. > >>>>> > >>>>> _tcpm_pd_vbus_off() -> run_state_machine to change state to > >>>>> SNK_UNATTACHED > >>>>> call tcpm_snk_detach() -> tcpm_snk_detach() -> tcpm_detach() > >>>>> tcpm_port_is_disconnected() will be called. > >>>>> But port->attached is still true and port->cc1=open and port->cc2=open > >>>>> > >>>>> It cause tcpm_port_is_disconnected return false, then hard_reset_count > >>>>> won't be reset. > >>>>> After that, tcpm_reset_port() is called. > >>>>> port->attached become false. > >>>>> > >>>>> After that, cc now trigger cc_change event, the hard_reset_count will > >>>>> be kept. > >>>>> Even tcpm_detach will be called, due to port->attached is false, > >>>>> tcpm_detach() > >>>>> will directly return. > >>>>> > >>>>> CC_EVENT will only trigger drp toggling again. > >>>>> --- > >>>>> drivers/usb/typec/tcpm/tcpm.c | 3 +-- > >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c > >>>>> b/drivers/usb/typec/tcpm/tcpm.c > >>>>> index a48e3f90..5c73e1d 100644 > >>>>> --- a/drivers/usb/typec/tcpm/tcpm.c > >>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c > >>>>> @@ -2797,8 +2797,7 @@ static void tcpm_detach(struct tcpm_port *port) > >>>>> port->tcpc->set_bist_data(port->tcpc, false); > >>>>> } > >>>>> > >>>>> - if (tcpm_port_is_disconnected(port)) > >>>>> - port->hard_reset_count = 0; > >>>>> + port->hard_reset_count = 0; > >>>>> > >>>> > >>>> Doesn't that mean that the state machine will never enter > >>>> error recovery ? > >>>> > >>> I think it does't affect the error recovery. > >>> All error recovery seems to check pd_capable flag. > >>> > >>> >From my below case, it's A to C cable only. There is no USBPD contract > >>> will be estabilished. > >>> > >>> This case occurred following by the below test condition > >>> Cable -> A to C (default Rp bind to vbus) connected to PC. > >>> 1. first time plugged in the cable with PC > >>> It will make HARD_RESET_COUNT to be equal 2 > >>> 2. And then plug out. At that time HARD_RESET_COUNT is till 2. > >>> 3. next time plugged in again. > >>> Due to hard_reset_count is still 2 , after wait_cap_timeout, the state > >>> eventually changed to SNK_READY. > >>> But during the state transition, no hard_reset be sent. > >>> > >>> Defined in the USBPD policy engine, typec transition to USBPD, all > >>> variables must be reset included hard_reset_count. > >>> So it expected SNK must send hard_reset again. > >>> > >>> The original code defined hard_reset_count must be reset only when > >>> tcpm_port_is_disconnected. > >>> > >>> It doesn't make sense that it only occurred in some scenario. > >>> If tcpm_detach is called, hard_reset count must be reset also. > >>> > >> > >> If a hard reset fails, the state machine may cycle through states > >> HARD_RESET_SEND, HARD_RESET_START, SRC_HARD_RESET_VBUS_OFF, > >> SRC_HARD_RESET_VBUS_ON back to SRC_UNATTACHED. In this state, > >> tcpm_src_detach() and with it tcpm_detach() is called. The hard > >> reset counter is incremented in HARD_RESET_SEND. If tcpm_detach() >
Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
Guenter Roeck 於 2020年9月5日 週六 上午3:41寫道： > > On 9/3/20 9:21 AM, ChiYuan Huang wrote: > > Guenter Roeck 於 2020年9月3日 週四 上午12:57寫道： > >> > >> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote: > >>> From: ChiYuan Huang > >>> > >>> Fix: If vbus event is before cc_event trigger, hard_reset_count > >>> won't bt reset for some case. > >>> > >>> Signed-off-by: ChiYuan Huang > >>> --- > >>> Below's the flow. > >>> > >>> _tcpm_pd_vbus_off() -> run_state_machine to change state to SNK_UNATTACHED > >>> call tcpm_snk_detach() -> tcpm_snk_detach() -> tcpm_detach() > >>> tcpm_port_is_disconnected() will be called. > >>> But port->attached is still true and port->cc1=open and port->cc2=open > >>> > >>> It cause tcpm_port_is_disconnected return false, then hard_reset_count > >>> won't be reset. > >>> After that, tcpm_reset_port() is called. > >>> port->attached become false. > >>> > >>> After that, cc now trigger cc_change event, the hard_reset_count will be > >>> kept. > >>> Even tcpm_detach will be called, due to port->attached is false, > >>> tcpm_detach() > >>> will directly return. > >>> > >>> CC_EVENT will only trigger drp toggling again. > >>> --- > >>> drivers/usb/typec/tcpm/tcpm.c | 3 +-- > >>> 1 file changed, 1 insertion(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > >>> index a48e3f90..5c73e1d 100644 > >>> --- a/drivers/usb/typec/tcpm/tcpm.c > >>> +++ b/drivers/usb/typec/tcpm/tcpm.c > >>> @@ -2797,8 +2797,7 @@ static void tcpm_detach(struct tcpm_port *port) > >>> port->tcpc->set_bist_data(port->tcpc, false); > >>> } > >>> > >>> - if (tcpm_port_is_disconnected(port)) > >>> - port->hard_reset_count = 0; > >>> + port->hard_reset_count = 0; > >>> > >> > >> Doesn't that mean that the state machine will never enter > >> error recovery ? > >> > > I think it does't affect the error recovery. > > All error recovery seems to check pd_capable flag. > > > >>From my below case, it's A to C cable only. There is no USBPD contract > > will be estabilished. > > > > This case occurred following by the below test condition > > Cable -> A to C (default Rp bind to vbus) connected to PC. > > 1. first time plugged in the cable with PC > > It will make HARD_RESET_COUNT to be equal 2 > > 2. And then plug out. At that time HARD_RESET_COUNT is till 2. > > 3. next time plugged in again. > > Due to hard_reset_count is still 2 , after wait_cap_timeout, the state > > eventually changed to SNK_READY. > > But during the state transition, no hard_reset be sent. > > > > Defined in the USBPD policy engine, typec transition to USBPD, all > > variables must be reset included hard_reset_count. > > So it expected SNK must send hard_reset again. > > > > The original code defined hard_reset_count must be reset only when > > tcpm_port_is_disconnected. > > > > It doesn't make sense that it only occurred in some scenario. > > If tcpm_detach is called, hard_reset count must be reset also. > > > > If a hard reset fails, the state machine may cycle through states > HARD_RESET_SEND, HARD_RESET_START, SRC_HARD_RESET_VBUS_OFF, > SRC_HARD_RESET_VBUS_ON back to SRC_UNATTACHED. In this state, > tcpm_src_detach() and with it tcpm_detach() is called. The hard > reset counter is incremented in HARD_RESET_SEND. If tcpm_detach() > resets the counter, the state machine will keep cycling through hard > resets without ever entering the error recovery state. I am not > entirely sure where the counter should be reset, but tcpm_detach() > seems to be the wrong place. This case you specified means locally error occurred. It intended to re-run the state machine from typec to USBPD. >From my understanding, hard_reset_count to be reset is reasonable. The normal stare from the state transition you specified is HARD_RESET_SEND, HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF, SRC_HARD_RESET_VBUS_ON -> received VBUS_EVENT then go to SRC_STARTUP. > > Guenter > > >> Guenter > >> > >>> tcpm_reset_port(port); > >>> } > >>> -- > >>> 2.7.4 > >>> >
Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
Guenter Roeck 於 2020年9月3日 週四 上午12:57寫道： > > On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Fix: If vbus event is before cc_event trigger, hard_reset_count > > won't bt reset for some case. > > > > Signed-off-by: ChiYuan Huang > > --- > > Below's the flow. > > > > _tcpm_pd_vbus_off() -> run_state_machine to change state to SNK_UNATTACHED > > call tcpm_snk_detach() -> tcpm_snk_detach() -> tcpm_detach() > > tcpm_port_is_disconnected() will be called. > > But port->attached is still true and port->cc1=open and port->cc2=open > > > > It cause tcpm_port_is_disconnected return false, then hard_reset_count > > won't be reset. > > After that, tcpm_reset_port() is called. > > port->attached become false. > > > > After that, cc now trigger cc_change event, the hard_reset_count will be > > kept. > > Even tcpm_detach will be called, due to port->attached is false, > > tcpm_detach() > > will directly return. > > > > CC_EVENT will only trigger drp toggling again. > > --- > > drivers/usb/typec/tcpm/tcpm.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index a48e3f90..5c73e1d 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -2797,8 +2797,7 @@ static void tcpm_detach(struct tcpm_port *port) > > port->tcpc->set_bist_data(port->tcpc, false); > > } > > > > - if (tcpm_port_is_disconnected(port)) > > - port->hard_reset_count = 0; > > + port->hard_reset_count = 0; > > > > Doesn't that mean that the state machine will never enter > error recovery ? > I think it does't affect the error recovery. All error recovery seems to check pd_capable flag. >From my below case, it's A to C cable only. There is no USBPD contract will be estabilished. This case occurred following by the below test condition Cable -> A to C (default Rp bind to vbus) connected to PC. 1. first time plugged in the cable with PC It will make HARD_RESET_COUNT to be equal 2 2. And then plug out. At that time HARD_RESET_COUNT is till 2. 3. next time plugged in again. Due to hard_reset_count is still 2 , after wait_cap_timeout, the state eventually changed to SNK_READY. But during the state transition, no hard_reset be sent. Defined in the USBPD policy engine, typec transition to USBPD, all variables must be reset included hard_reset_count. So it expected SNK must send hard_reset again. The original code defined hard_reset_count must be reset only when tcpm_port_is_disconnected. It doesn't make sense that it only occurred in some scenario. If tcpm_detach is called, hard_reset count must be reset also. > Guenter > > > tcpm_reset_port(port); > > } > > -- > > 2.7.4 > >
Re: [PATCH v4 2/2] usb typec: mt6360: Add MT6360 Type-C DT binding documentation
Chunfeng Yun 於 2020年8月31日 週一 上午11:00寫道：
>
> On Sat, 2020-08-29 at 10:49 +0800, ChiYuan Huang wrote:
> > ChiYuan Huang 於 2020年8月29日 週六 上午8:32寫道：
> > >
> > > Rob Herring 於 2020年8月29日 週六 上午6:05寫道：
> > > >
> > > > On Fri, Aug 28, 2020 at 06:30:36PM +0800, cy_huang wrote:
> > > > > From: ChiYuan Huang
> > > > >
> > > > > Add a devicetree binding documentation for the MT6360 Type-C driver.
> > > > >
> > > > > usb typec: mt6360: Rename DT binding doument from mt6360 to mt636x
> > > > >
> > > > > Signed-off-by: ChiYuan Huang
> > > > > ---
> > > > > .../bindings/usb/mediatek,mt6360-tcpc.yaml | 73
> > > > > ++
> > > > > 1 file changed, 73 insertions(+)
> > > > > create mode 100644
> > > > > Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > > > > b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > > > > new file mode 100644
> > > > > index ..9e8ab0d
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > > > > @@ -0,0 +1,73 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: "http://devicetree.org/schemas/usb/mediatek,mt6360-tcpc.yaml#";
> > > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> > > > > +
> > > > > +title: Mediatek MT6360 Type-C Port Switch and Power Delivery
> > > > > controller DT bindings
> > > > > +
> > > > > +maintainers:
> > > > > + - ChiYuan Huang
> > > > > +
> > > > > +description: |
> > > > > + Mediatek MT6360 is a multi-functional device. It integrates
> > > > > charger, ADC, flash, RGB indicators,
> > > > > + regulators (BUCKs/LDOs), and TypeC Port Switch with Power Delivery
> > > > > controller.
> > > > > + This document only describes MT6360 Type-C Port Switch and Power
> > > > > Delivery controller.
> > > > > +
> > > > > +properties:
> > > > > + compatible:
> > > > > +enum:
> > > > > + - mediatek,mt6360-tcpc
> > > > > +
> > > > > + interrupts-extended:
> > > >
> > > > Use 'interrupts'. The tooling will automatically support
> > > > 'interrupts-extended'.
> > > Okay.
> > > >
> > > > > +maxItems: 1
> > > > > +
> > > > > + interrupt-names:
> > > > > +items:
> > > > > + - const: PD_IRQB
> > > > > +
> > > > > +patternProperties:
> > > > > + "connector":
> > > > > +type: object
> > > > > +$ref: ../connector/usb-connector.yaml#
> > > > > +description:
> > > > > + Properties for usb c connector.
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +required:
> > > > > + - compatible
> > > > > + - interrupts-extended
> > > > > + - interrupt-names
> > > > > +
> > > > > +examples:
> > > > > + - |
> > > > > +#include
> > > > > +#include
> > > > > +i2c0 {
> > > > > +#address-cells = <1>;
> > > > > +#size-cells = <0>;
> > > > > +
> > > > > +mt6360@34 {
> > > > > +compatible = "mediatek,mt6360";
> > > > > +reg = <0x34>;
> > > > > +
> > > > > +tcpc {
> > > > > +compatible = "mediatek,mt6360-tcpc";
> > > > > +interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
> > > > > +interrupt-names = "PD_IRQB";
> > > > > +
> > > > > +connector {
> > > >
> > > > Where's th
Re: [PATCH v4 2/2] usb typec: mt6360: Add MT6360 Type-C DT binding documentation
ChiYuan Huang 於 2020年8月29日 週六 上午8:32寫道：
>
> Rob Herring 於 2020年8月29日 週六 上午6:05寫道：
> >
> > On Fri, Aug 28, 2020 at 06:30:36PM +0800, cy_huang wrote:
> > > From: ChiYuan Huang
> > >
> > > Add a devicetree binding documentation for the MT6360 Type-C driver.
> > >
> > > usb typec: mt6360: Rename DT binding doument from mt6360 to mt636x
> > >
> > > Signed-off-by: ChiYuan Huang
> > > ---
> > > .../bindings/usb/mediatek,mt6360-tcpc.yaml | 73
> > > ++
> > > 1 file changed, 73 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > > b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > > new file mode 100644
> > > index ..9e8ab0d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > > @@ -0,0 +1,73 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/usb/mediatek,mt6360-tcpc.yaml#";
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> > > +
> > > +title: Mediatek MT6360 Type-C Port Switch and Power Delivery controller
> > > DT bindings
> > > +
> > > +maintainers:
> > > + - ChiYuan Huang
> > > +
> > > +description: |
> > > + Mediatek MT6360 is a multi-functional device. It integrates charger,
> > > ADC, flash, RGB indicators,
> > > + regulators (BUCKs/LDOs), and TypeC Port Switch with Power Delivery
> > > controller.
> > > + This document only describes MT6360 Type-C Port Switch and Power
> > > Delivery controller.
> > > +
> > > +properties:
> > > + compatible:
> > > +enum:
> > > + - mediatek,mt6360-tcpc
> > > +
> > > + interrupts-extended:
> >
> > Use 'interrupts'. The tooling will automatically support
> > 'interrupts-extended'.
> Okay.
> >
> > > +maxItems: 1
> > > +
> > > + interrupt-names:
> > > +items:
> > > + - const: PD_IRQB
> > > +
> > > +patternProperties:
> > > + "connector":
> > > +type: object
> > > +$ref: ../connector/usb-connector.yaml#
> > > +description:
> > > + Properties for usb c connector.
> > > +
> > > +additionalProperties: false
> > > +
> > > +required:
> > > + - compatible
> > > + - interrupts-extended
> > > + - interrupt-names
> > > +
> > > +examples:
> > > + - |
> > > +#include
> > > +#include
> > > +i2c0 {
> > > +#address-cells = <1>;
> > > +#size-cells = <0>;
> > > +
> > > +mt6360@34 {
> > > +compatible = "mediatek,mt6360";
> > > +reg = <0x34>;
> > > +
> > > +tcpc {
> > > +compatible = "mediatek,mt6360-tcpc";
> > > +interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
> > > +interrupt-names = "PD_IRQB";
> > > +
> > > +connector {
> >
> > Where's the data connections? The assumption of the binding is the USB
> > (2 and 3) connections come from the parent if there's no graph to the
> > USB controller(s).
> MT6360 is only a subpmic. TypeC part only handle the CC logic to support
> USBPD.
> For the usb connection like as usbhs/usbss, it need to be handled
> by/connect to application processor side.
> LIke as connector/usb-connector.yaml decribed, it specify the port
> property to bind USB HS/SS.
>
Do i need to add the ports into the connector node for example?
Like as hs/ss/aux, to make the user know to use 6360's tcpc?
I check the style in connector/usb-connect.yaml
Do I also need to replace two space instead of one tab in the binding example?
> >
> > > +compatible = "usb-c-connector";
> > > +label = "USB-C";
> > > +data-role = "dual";
> > > +power-role = "dual";
> > > +try-power-role = "sink";
> > > +source-pdos = > > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>;
> > > +sink-pdos = > > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>;
> > > +op-sink-microwatt = <1000>;
> > > +};
> > > +};
> > > +};
> > > +};
> > > +...
> > > --
> > > 2.7.4
> > >
Re: [PATCH v4 2/2] usb typec: mt6360: Add MT6360 Type-C DT binding documentation
Rob Herring 於 2020年8月29日 週六 上午6:06寫道：
>
> On Fri, Aug 28, 2020 at 06:30:36PM +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Add a devicetree binding documentation for the MT6360 Type-C driver.
> >
> > usb typec: mt6360: Rename DT binding doument from mt6360 to mt636x
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > .../bindings/usb/mediatek,mt6360-tcpc.yaml | 73
> > ++
> > 1 file changed, 73 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > new file mode 100644
> > index ..9e8ab0d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > @@ -0,0 +1,73 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/usb/mediatek,mt6360-tcpc.yaml#";
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> > +
> > +title: Mediatek MT6360 Type-C Port Switch and Power Delivery controller DT
> > bindings
> > +
> > +maintainers:
> > + - ChiYuan Huang
> > +
> > +description: |
> > + Mediatek MT6360 is a multi-functional device. It integrates charger,
> > ADC, flash, RGB indicators,
> > + regulators (BUCKs/LDOs), and TypeC Port Switch with Power Delivery
> > controller.
> > + This document only describes MT6360 Type-C Port Switch and Power
> > Delivery controller.
> > +
> > +properties:
> > + compatible:
> > +enum:
> > + - mediatek,mt6360-tcpc
> > +
> > + interrupts-extended:
> > +maxItems: 1
> > +
> > + interrupt-names:
> > +items:
> > + - const: PD_IRQB
> > +
> > +patternProperties:
> > + "connector":
>
> Also, this is not a pattern, so should be under 'properties', and it
> doesn't need quotes.
Will be fixed in next patch.
>
> > +type: object
> > +$ref: ../connector/usb-connector.yaml#
> > +description:
> > + Properties for usb c connector.
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - interrupts-extended
> > + - interrupt-names
> > +
> > +examples:
> > + - |
> > +#include
> > +#include
> > +i2c0 {
> > +#address-cells = <1>;
> > +#size-cells = <0>;
> > +
> > +mt6360@34 {
> > +compatible = "mediatek,mt6360";
> > +reg = <0x34>;
> > +
> > +tcpc {
> > +compatible = "mediatek,mt6360-tcpc";
> > +interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
> > +interrupt-names = "PD_IRQB";
> > +
> > +connector {
> > +compatible = "usb-c-connector";
> > +label = "USB-C";
> > +data-role = "dual";
> > +power-role = "dual";
> > +try-power-role = "sink";
> > +source-pdos = > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>;
> > +sink-pdos = > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>;
> > +op-sink-microwatt = <1000>;
> > +};
> > +};
> > +};
> > +};
> > +...
> > --
> > 2.7.4
> >
Re: [PATCH v4 2/2] usb typec: mt6360: Add MT6360 Type-C DT binding documentation
Rob Herring 於 2020年8月29日 週六 上午6:05寫道：
>
> On Fri, Aug 28, 2020 at 06:30:36PM +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Add a devicetree binding documentation for the MT6360 Type-C driver.
> >
> > usb typec: mt6360: Rename DT binding doument from mt6360 to mt636x
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > .../bindings/usb/mediatek,mt6360-tcpc.yaml | 73
> > ++
> > 1 file changed, 73 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > new file mode 100644
> > index ..9e8ab0d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > @@ -0,0 +1,73 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/usb/mediatek,mt6360-tcpc.yaml#";
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> > +
> > +title: Mediatek MT6360 Type-C Port Switch and Power Delivery controller DT
> > bindings
> > +
> > +maintainers:
> > + - ChiYuan Huang
> > +
> > +description: |
> > + Mediatek MT6360 is a multi-functional device. It integrates charger,
> > ADC, flash, RGB indicators,
> > + regulators (BUCKs/LDOs), and TypeC Port Switch with Power Delivery
> > controller.
> > + This document only describes MT6360 Type-C Port Switch and Power
> > Delivery controller.
> > +
> > +properties:
> > + compatible:
> > +enum:
> > + - mediatek,mt6360-tcpc
> > +
> > + interrupts-extended:
>
> Use 'interrupts'. The tooling will automatically support
> 'interrupts-extended'.
Okay.
>
> > +maxItems: 1
> > +
> > + interrupt-names:
> > +items:
> > + - const: PD_IRQB
> > +
> > +patternProperties:
> > + "connector":
> > +type: object
> > +$ref: ../connector/usb-connector.yaml#
> > +description:
> > + Properties for usb c connector.
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - interrupts-extended
> > + - interrupt-names
> > +
> > +examples:
> > + - |
> > +#include
> > +#include
> > +i2c0 {
> > +#address-cells = <1>;
> > +#size-cells = <0>;
> > +
> > +mt6360@34 {
> > +compatible = "mediatek,mt6360";
> > +reg = <0x34>;
> > +
> > +tcpc {
> > +compatible = "mediatek,mt6360-tcpc";
> > +interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
> > +interrupt-names = "PD_IRQB";
> > +
> > +connector {
>
> Where's the data connections? The assumption of the binding is the USB
> (2 and 3) connections come from the parent if there's no graph to the
> USB controller(s).
MT6360 is only a subpmic. TypeC part only handle the CC logic to support USBPD.
For the usb connection like as usbhs/usbss, it need to be handled
by/connect to application processor side.
LIke as connector/usb-connector.yaml decribed, it specify the port
property to bind USB HS/SS.
>
> > +compatible = "usb-c-connector";
> > +label = "USB-C";
> > +data-role = "dual";
> > +power-role = "dual";
> > +try-power-role = "sink";
> > +source-pdos = > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>;
> > +sink-pdos = > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>;
> > +op-sink-microwatt = <1000>;
> > +};
> > +};
> > +};
> > +};
> > +...
> > --
> > 2.7.4
> >
Re: [PATCH v3 1/3] usb typec: mt6360: Add support for mt6360 Type-C driver
Chunfeng Yun 於 2020年8月28日 週五 下午8:11寫道：
>
> On Thu, 2020-08-27 at 19:18 +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Mediatek MT6360 is a multi-functional IC that includes USB Type-C.
> > It works with Type-C Port Controller Manager to provide USB PD
> > and USB Type-C functionalities.
> >
> > v1 to v2
> > 1. Add fix to Prevent the race condition from interrupt and tcpci port
> > unregister during module remove.
> >
> > v2 to v3
> > 1. Change comment style for the head of source code.
> > 2. No need to print error for platform_get_irq_byname.
> > 3. Fix tcpci_register_port check from IS_ERR_OR_NULL to IS_ERR.
> > 4. Rename driver/Kconfig/Makefile form mt6360 to mt636x.
> > 5. Rename DT binding documents from mt6360 to mt636x.
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > drivers/usb/typec/tcpm/Kconfig| 8 ++
> > drivers/usb/typec/tcpm/Makefile | 1 +
> > drivers/usb/typec/tcpm/tcpci_mt6360.c | 212
> > ++
> > 3 files changed, 221 insertions(+)
> > create mode 100644 drivers/usb/typec/tcpm/tcpci_mt6360.c
> >
> > diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
> > index fa3f393..58a64e1 100644
> > --- a/drivers/usb/typec/tcpm/Kconfig
> > +++ b/drivers/usb/typec/tcpm/Kconfig
> > @@ -27,6 +27,14 @@ config TYPEC_RT1711H
> > Type-C Port Controller Manager to provide USB PD and USB
> > Type-C functionalities.
> >
> > +config TYPEC_MT6360
> > + tristate "Mediatek MT6360 Type-C driver"
> > + depends on MFD_MT6360
> > + help
> > + Mediatek MT6360 is a multi-functional IC that includes
> > + USB Type-C. It works with Type-C Port Controller Manager
> > + to provide USB PD and USB Type-C functionalities.
> > +
> > endif # TYPEC_TCPCI
> >
> > config TYPEC_FUSB302
> > diff --git a/drivers/usb/typec/tcpm/Makefile
> > b/drivers/usb/typec/tcpm/Makefile
> > index a5ff6c8..7592ccb 100644
> > --- a/drivers/usb/typec/tcpm/Makefile
> > +++ b/drivers/usb/typec/tcpm/Makefile
> > @@ -5,3 +5,4 @@ obj-$(CONFIG_TYPEC_WCOVE) += typec_wcove.o
> > typec_wcove-y:= wcove.o
> > obj-$(CONFIG_TYPEC_TCPCI)+= tcpci.o
> > obj-$(CONFIG_TYPEC_RT1711H) += tcpci_rt1711h.o
> > +obj-$(CONFIG_TYPEC_MT6360) += tcpci_mt6360.o
> > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > new file mode 100644
> > index ..f1bd9e0
> > --- /dev/null
> > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > @@ -0,0 +1,212 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2020 MediaTek Inc.
> > + *
> > + * Author: ChiYuan Huang
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "tcpci.h"
> > +
> > +#define MT6360_REG_VCONNCTRL10x8C
> > +#define MT6360_REG_MODECTRL2 0x8F
> > +#define MT6360_REG_SWRESET 0xA0
> > +#define MT6360_REG_DEBCTRL1 0xA1
> > +#define MT6360_REG_DRPCTRL1 0xA2
> > +#define MT6360_REG_DRPCTRL2 0xA3
> > +#define MT6360_REG_I2CTORST 0xBF
> > +#define MT6360_REG_RXCTRL2 0xCF
> > +#define MT6360_REG_CTDCTRL2 0xEC
> > +
> > +/* MT6360_REG_VCONNCTRL1 */
> > +#define MT6360_VCONNCL_ENABLEBIT(0)
> > +/* MT6360_REG_RXCTRL2 */
> > +#define MT6360_OPEN40M_ENABLEBIT(7)
> > +/* MT6360_REG_CTDCTRL2 */
> > +#define MT6360_RPONESHOT_ENABLE BIT(6)
> > +
> > +struct mt6360_tcpc_info {
> > + struct tcpci_data tdata;
> > + struct tcpci *tcpci;
> > + struct device *dev;
> > + int irq;
> > +};
> > +
> > +static inline int mt6360_tcpc_read16(struct regmap *regmap,
> > + unsigned int reg, u16 *val)
> > +{
> > + return regmap_raw_read(regmap, reg, val, sizeof(u16));
> > +}
> > +
> > +static inline int mt6360_tcpc_write16(struct regmap *regmap,
> > + unsigned int reg, u16 val)
> > +{
> > + return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > +}
> > +
> > +static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> > +{
> > + struct regmap *regmap = tdata->regmap;
> > + int ret;
> > +
> > + ret = regmap_write(reg
Re: [PATCH v3 1/3] usb typec: mt6360: Add support for mt6360 Type-C driver
Heikki Krogerus 於 2020年8月28日 週五 下午5:39寫道： > > On Thu, Aug 27, 2020 at 10:51:43PM +0800, ChiYuan Huang wrote: > > Heikki Krogerus 於 2020年8月27日 週四 下午10:00寫道： > > > > > > On Thu, Aug 27, 2020 at 07:18:55PM +0800, cy_huang wrote: > > > > From: ChiYuan Huang > > > > > > > > Mediatek MT6360 is a multi-functional IC that includes USB Type-C. > > > > It works with Type-C Port Controller Manager to provide USB PD > > > > and USB Type-C functionalities. > > > > > > > > v1 to v2 > > > > 1. Add fix to Prevent the race condition from interrupt and tcpci port > > > > unregister during module remove. > > > > > > > > v2 to v3 > > > > 1. Change comment style for the head of source code. > > > > 2. No need to print error for platform_get_irq_byname. > > > > 3. Fix tcpci_register_port check from IS_ERR_OR_NULL to IS_ERR. > > > > 4. Rename driver/Kconfig/Makefile form mt6360 to mt636x. > > > > 5. Rename DT binding documents from mt6360 to mt636x. > > > > > > You don't place additional changelog here... > > > > > > > Signed-off-by: ChiYuan Huang > > > > --- > > > > > > You put it here, after that '---' marker: > > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format > > > > > HI Hekki: > > after reading the document, I have a little bit confused how to > > use diffstat for the changelog. > > Is there any example that make me know to write a clear > > description for the changelog? > > Picking the latest patch from linux-usb ml. with version history: > https://lore.kernel.org/linux-usb/1598083553-31896-11-git-send-email-chunfeng@mediatek.com/ > > See how the last tag line "Signed-off-by: Chunfeng Yun..." is followed > by marker "---", which then is followed by the version history (the > version history is then also ended with the marker "---", a step that > I don't think is mandatory, but commonly used and often recommended). > > That way that patch version history does not contaminate the actual > commit message. > > > thanks, > Ah, I already send the patch v4, I only add the changelog after the sign-off --- label, but forget to add --- after changelog ended Please let me resend the patch v4 to add --- label after the change log eded. > -- > heikki
Re: [PATCH v3 2/3] usb typec: mt6360: Rename driver/Kconfig/Makefile from mt6360 to mt636x
Guenter Roeck 於 2020年8月28日 週五 上午12:41寫道： > > On Thu, Aug 27, 2020 at 07:18:56PM +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > 1. Rename file form tcpci_mt6360.c to tcpci_mt636x.c > > 2. Rename internal function from mt6360 to mt636x, except the register > > definition. > > 3. Change Kconfig/Makefile from MT6360 to MT636X. > > > > Signed-off-by: ChiYuan Huang > > --- > > drivers/usb/typec/tcpm/Kconfig| 6 +- > > drivers/usb/typec/tcpm/Makefile | 2 +- > > drivers/usb/typec/tcpm/tcpci_mt6360.c | 212 > > -- > > drivers/usb/typec/tcpm/tcpci_mt636x.c | 212 > > ++ > > 4 files changed, 216 insertions(+), 216 deletions(-) > > delete mode 100644 drivers/usb/typec/tcpm/tcpci_mt6360.c > > create mode 100644 drivers/usb/typec/tcpm/tcpci_mt636x.c > > Maybe Heikki is ok with this change, but I am not, for the reasons > mentioned before. So I won't approve this patch. Note that, either > case, it should be merged with the first patch. Yes, I agree with you opinion. use 636x, the range is too large from 0 to 9, it may not all be compatible. Even though it's also possible that the part number don't have the same function. So I'm going to remove the rename patch. Do I need to add a patch named "revert"? Or just remove it. I'm not sure which way is better. It seems you all want the code change to be squashed into the first code. And the second one is the DT binding. Right? > > Guenter
Re: [PATCH v3 1/3] usb typec: mt6360: Add support for mt6360 Type-C driver
Heikki Krogerus 於 2020年8月27日 週四 下午10:00寫道： > > On Thu, Aug 27, 2020 at 07:18:55PM +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Mediatek MT6360 is a multi-functional IC that includes USB Type-C. > > It works with Type-C Port Controller Manager to provide USB PD > > and USB Type-C functionalities. > > > > v1 to v2 > > 1. Add fix to Prevent the race condition from interrupt and tcpci port > > unregister during module remove. > > > > v2 to v3 > > 1. Change comment style for the head of source code. > > 2. No need to print error for platform_get_irq_byname. > > 3. Fix tcpci_register_port check from IS_ERR_OR_NULL to IS_ERR. > > 4. Rename driver/Kconfig/Makefile form mt6360 to mt636x. > > 5. Rename DT binding documents from mt6360 to mt636x. > > You don't place additional changelog here... > > > Signed-off-by: ChiYuan Huang > > --- > > You put it here, after that '---' marker: > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format > HI Hekki: after reading the document, I have a little bit confused how to use diffstat for the changelog. Is there any example that make me know to write a clear description for the changelog? > > > drivers/usb/typec/tcpm/Kconfig| 8 ++ > > drivers/usb/typec/tcpm/Makefile | 1 + > > drivers/usb/typec/tcpm/tcpci_mt6360.c | 212 > > ++ > > 3 files changed, 221 insertions(+) > > create mode 100644 drivers/usb/typec/tcpm/tcpci_mt6360.c > > thanks, > > -- > heikki
Re: [PATCH v2 1/2] usb typec: mt6360: Add support for mt6360 Type-C driver
Guenter Roeck 於 2020年8月27日 週四 下午9:46寫道：
>
> On 8/27/20 4:32 AM, 啟原黃 wrote:
> > Chunfeng Yun 於 2020年8月27日 週四 下午4:05寫道：
> >>
> >> On Thu, 2020-08-27 at 12:50 +0800, cy_huang wrote:
> >>> From: ChiYuan Huang
> >>>
> >>> Mediatek MT6360 is a multi-functional IC that includes USB Type-C.
> >>> It works with Type-C Port Controller Manager to provide USB PD
> >>> and USB Type-C functionalities.
> >>>
> >>> Add fix to Prevent the race condition from interrupt and tcpci port
> >>> unregister
> >>> during module remove.
> >>>
> >>> Signed-off-by: ChiYuan Huang
> >>> ---
> >>> drivers/usb/typec/tcpm/Kconfig| 8 ++
> >>> drivers/usb/typec/tcpm/Makefile | 1 +
> >>> drivers/usb/typec/tcpm/tcpci_mt6360.c | 213
> >>> ++
> >> Can you avoid using special SoC name in file name?
> >> It's not clear if you later support new SoC in the driver, e.g. mt63xx?
> > Okay, I will rename it to mt636x. From our SubPMIC generation,
> > currently, naming will be 6360/62/67, etc..
>
> What if 6361 or 6365 or 6369 will require a different driver ?
> What if 6371 will, in the future, be supported by the same driver ?
> Or, for that matter, 7360 ? 6537 ?
>
> We usually try to avoid "x" in driver names because it can never be
> guaranteed that it will apply to x={0..9}. The current file name is
> just fine; it is customary to name drivers after the first chip they
> support.
>
> Guenter
Thanks. it means the current file name and description is okay
I'll start to revert the rename work.
>
> >>From our SOC roadmap, one generation can be used for one to two more years.
> > I think the name MT636x is enough.
> > For the next one, I can submit the patch to make it compatible with this
> > driver.
> > Thanks for you comment.
> >>
> >>> 3 files changed, 222 insertions(+)
> >>> create mode 100644 drivers/usb/typec/tcpm/tcpci_mt6360.c
> >>>
> >>> diff --git a/drivers/usb/typec/tcpm/Kconfig
> >>> b/drivers/usb/typec/tcpm/Kconfig
> >>> index fa3f393..58a64e1 100644
> >>> --- a/drivers/usb/typec/tcpm/Kconfig
> >>> +++ b/drivers/usb/typec/tcpm/Kconfig
> >>> @@ -27,6 +27,14 @@ config TYPEC_RT1711H
> >>> Type-C Port Controller Manager to provide USB PD and USB
> >>> Type-C functionalities.
> >>>
> >>> +config TYPEC_MT6360
> >>> + tristate "Mediatek MT6360 Type-C driver"
> >>> + depends on MFD_MT6360
> >>> + help
> >>> + Mediatek MT6360 is a multi-functional IC that includes
> >>> + USB Type-C. It works with Type-C Port Controller Manager
> >>> + to provide USB PD and USB Type-C functionalities.
> >>> +
> >>> endif # TYPEC_TCPCI
> >>>
> >>> config TYPEC_FUSB302
> >>> diff --git a/drivers/usb/typec/tcpm/Makefile
> >>> b/drivers/usb/typec/tcpm/Makefile
> >>> index a5ff6c8..7592ccb 100644
> >>> --- a/drivers/usb/typec/tcpm/Makefile
> >>> +++ b/drivers/usb/typec/tcpm/Makefile
> >>> @@ -5,3 +5,4 @@ obj-$(CONFIG_TYPEC_WCOVE) += typec_wcove.o
> >>> typec_wcove-y:= wcove.o
> >>> obj-$(CONFIG_TYPEC_TCPCI)+= tcpci.o
> >>> obj-$(CONFIG_TYPEC_RT1711H) += tcpci_rt1711h.o
> >>> +obj-$(CONFIG_TYPEC_MT6360) += tcpci_mt6360.o
> >>> diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> >>> b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> >>> new file mode 100644
> >>> index ..a381b5d
> >>> --- /dev/null
> >>> +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> >>> @@ -0,0 +1,213 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-only
> >>> +//
> >>> +// Copyright (C) 2020 MediaTek Inc.
> >>> +//
> >>> +// Author: ChiYuan Huang
> >> Use /* */ except SPDX?
> > Yes, sure.
> >>
> >>> +
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +
> >>> +#include "tcpci.h"
> >>> +
> >>> +#define MT6360_REG_VCONNCTRL10x8C
> >>> +#define MT6360_REG_MODECTRL2 0x8F
> >>> +#define MT6360
