* [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member @ 2017-12-22 12:36 ` Hans de Goede 2017-12-22 12:36 ` [PATCH 2/4] extcon: axp288: Remove unused platform data Hans de Goede ` (4 more replies) 0 siblings, 5 replies; 18+ messages in thread From: Hans de Goede @ 2017-12-22 12:36 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: Hans de Goede, linux-kernel Remove the unused extcon_nb struct member. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/extcon-axp288.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c index 981fba56bc18..ba185e9ebb82 100644 --- a/drivers/extcon/extcon-axp288.c +++ b/drivers/extcon/extcon-axp288.c @@ -107,7 +107,6 @@ struct axp288_extcon_info { struct gpio_desc *gpio_mux_cntl; int irq[EXTCON_IRQ_END]; struct extcon_dev *edev; - struct notifier_block extcon_nb; unsigned int previous_cable; }; -- 2.14.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] extcon: axp288: Remove unused platform data 2017-12-22 12:36 ` [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member Hans de Goede @ 2017-12-22 12:36 ` Hans de Goede 2018-01-02 0:35 ` Chanwoo Choi 2017-12-22 12:36 ` [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() Hans de Goede ` (3 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Hans de Goede @ 2017-12-22 12:36 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: Hans de Goede, linux-kernel This is not used / set anywhere in the tree. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/extcon-axp288.c | 35 +---------------------------------- include/linux/mfd/axp20x.h | 5 ----- 2 files changed, 1 insertion(+), 39 deletions(-) diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c index ba185e9ebb82..386afb7d1160 100644 --- a/drivers/extcon/extcon-axp288.c +++ b/drivers/extcon/extcon-axp288.c @@ -24,8 +24,6 @@ #include <linux/notifier.h> #include <linux/extcon-provider.h> #include <linux/regmap.h> -#include <linux/gpio.h> -#include <linux/gpio/consumer.h> #include <linux/mfd/axp20x.h> /* Power source status register */ @@ -79,11 +77,6 @@ enum axp288_extcon_reg { AXP288_BC_DET_STAT_REG = 0x2f, }; -enum axp288_mux_select { - EXTCON_GPIO_MUX_SEL_PMIC = 0, - EXTCON_GPIO_MUX_SEL_SOC, -}; - enum axp288_extcon_irq { VBUS_FALLING_IRQ = 0, VBUS_RISING_IRQ, @@ -104,7 +97,6 @@ struct axp288_extcon_info { struct device *dev; struct regmap *regmap; struct regmap_irq_chip_data *regmap_irqc; - struct gpio_desc *gpio_mux_cntl; int irq[EXTCON_IRQ_END]; struct extcon_dev *edev; unsigned int previous_cable; @@ -196,15 +188,6 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) } no_vbus: - /* - * If VBUS is absent Connect D+/D- lines to PMIC for BC - * detection. Else connect them to SOC for USB communication. - */ - if (info->gpio_mux_cntl) - gpiod_set_value(info->gpio_mux_cntl, - vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC - : EXTCON_GPIO_MUX_SEL_PMIC); - extcon_set_state_sync(info->edev, info->previous_cable, false); if (info->previous_cable == EXTCON_CHG_USB_SDP) extcon_set_state_sync(info->edev, EXTCON_USB, false); @@ -252,8 +235,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) { struct axp288_extcon_info *info; struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent); - struct axp288_extcon_pdata *pdata = pdev->dev.platform_data; - int ret, i, pirq, gpio; + int ret, i, pirq; info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); if (!info) @@ -263,8 +245,6 @@ static int axp288_extcon_probe(struct platform_device *pdev) info->regmap = axp20x->regmap; info->regmap_irqc = axp20x->regmap_irqc; info->previous_cable = EXTCON_NONE; - if (pdata) - info->gpio_mux_cntl = pdata->gpio_mux_cntl; platform_set_drvdata(pdev, info); @@ -285,19 +265,6 @@ static int axp288_extcon_probe(struct platform_device *pdev) return ret; } - /* Set up gpio control for USB Mux */ - if (info->gpio_mux_cntl) { - gpio = desc_to_gpio(info->gpio_mux_cntl); - ret = devm_gpio_request(&pdev->dev, gpio, "USB_MUX"); - if (ret < 0) { - dev_err(&pdev->dev, - "failed to request the gpio=%d\n", gpio); - return ret; - } - gpiod_direction_output(info->gpio_mux_cntl, - EXTCON_GPIO_MUX_SEL_PMIC); - } - for (i = 0; i < EXTCON_IRQ_END; i++) { pirq = platform_get_irq(pdev, i); info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq); diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h index 78dc85365c4f..080798f17ece 100644 --- a/include/linux/mfd/axp20x.h +++ b/include/linux/mfd/axp20x.h @@ -645,11 +645,6 @@ struct axp20x_dev { const struct regmap_irq_chip *regmap_irq_chip; }; -struct axp288_extcon_pdata { - /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */ - struct gpio_desc *gpio_mux_cntl; -}; - /* generic helper function for reading 9-16 bit wide regs */ static inline int axp20x_read_variable_width(struct regmap *regmap, unsigned int reg, unsigned int width) -- 2.14.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] extcon: axp288: Remove unused platform data 2017-12-22 12:36 ` [PATCH 2/4] extcon: axp288: Remove unused platform data Hans de Goede @ 2018-01-02 0:35 ` Chanwoo Choi 2018-01-02 9:16 ` Lee Jones 0 siblings, 1 reply; 18+ messages in thread From: Chanwoo Choi @ 2018-01-02 0:35 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham, Lee Jones; +Cc: linux-kernel + MFD Maintainer (Lee Jones <lee.jones@linaro.org>) Hi Hans, You better to use the scripts/get_maintainer.pl for the mailing list. I added the MFD maintainer. This patch looks good to me. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> Best Regards, Chanwoo Choi On 2017년 12월 22일 21:36, Hans de Goede wrote: > This is not used / set anywhere in the tree. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/extcon-axp288.c | 35 +---------------------------------- > include/linux/mfd/axp20x.h | 5 ----- > 2 files changed, 1 insertion(+), 39 deletions(-) > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index ba185e9ebb82..386afb7d1160 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -24,8 +24,6 @@ > #include <linux/notifier.h> > #include <linux/extcon-provider.h> > #include <linux/regmap.h> > -#include <linux/gpio.h> > -#include <linux/gpio/consumer.h> > #include <linux/mfd/axp20x.h> > > /* Power source status register */ > @@ -79,11 +77,6 @@ enum axp288_extcon_reg { > AXP288_BC_DET_STAT_REG = 0x2f, > }; > > -enum axp288_mux_select { > - EXTCON_GPIO_MUX_SEL_PMIC = 0, > - EXTCON_GPIO_MUX_SEL_SOC, > -}; > - > enum axp288_extcon_irq { > VBUS_FALLING_IRQ = 0, > VBUS_RISING_IRQ, > @@ -104,7 +97,6 @@ struct axp288_extcon_info { > struct device *dev; > struct regmap *regmap; > struct regmap_irq_chip_data *regmap_irqc; > - struct gpio_desc *gpio_mux_cntl; > int irq[EXTCON_IRQ_END]; > struct extcon_dev *edev; > unsigned int previous_cable; > @@ -196,15 +188,6 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) > } > > no_vbus: > - /* > - * If VBUS is absent Connect D+/D- lines to PMIC for BC > - * detection. Else connect them to SOC for USB communication. > - */ > - if (info->gpio_mux_cntl) > - gpiod_set_value(info->gpio_mux_cntl, > - vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC > - : EXTCON_GPIO_MUX_SEL_PMIC); > - > extcon_set_state_sync(info->edev, info->previous_cable, false); > if (info->previous_cable == EXTCON_CHG_USB_SDP) > extcon_set_state_sync(info->edev, EXTCON_USB, false); > @@ -252,8 +235,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) > { > struct axp288_extcon_info *info; > struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent); > - struct axp288_extcon_pdata *pdata = pdev->dev.platform_data; > - int ret, i, pirq, gpio; > + int ret, i, pirq; > > info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); > if (!info) > @@ -263,8 +245,6 @@ static int axp288_extcon_probe(struct platform_device *pdev) > info->regmap = axp20x->regmap; > info->regmap_irqc = axp20x->regmap_irqc; > info->previous_cable = EXTCON_NONE; > - if (pdata) > - info->gpio_mux_cntl = pdata->gpio_mux_cntl; > > platform_set_drvdata(pdev, info); > > @@ -285,19 +265,6 @@ static int axp288_extcon_probe(struct platform_device *pdev) > return ret; > } > > - /* Set up gpio control for USB Mux */ > - if (info->gpio_mux_cntl) { > - gpio = desc_to_gpio(info->gpio_mux_cntl); > - ret = devm_gpio_request(&pdev->dev, gpio, "USB_MUX"); > - if (ret < 0) { > - dev_err(&pdev->dev, > - "failed to request the gpio=%d\n", gpio); > - return ret; > - } > - gpiod_direction_output(info->gpio_mux_cntl, > - EXTCON_GPIO_MUX_SEL_PMIC); > - } > - > for (i = 0; i < EXTCON_IRQ_END; i++) { > pirq = platform_get_irq(pdev, i); > info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq); > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h > index 78dc85365c4f..080798f17ece 100644 > --- a/include/linux/mfd/axp20x.h > +++ b/include/linux/mfd/axp20x.h > @@ -645,11 +645,6 @@ struct axp20x_dev { > const struct regmap_irq_chip *regmap_irq_chip; > }; > > -struct axp288_extcon_pdata { > - /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */ > - struct gpio_desc *gpio_mux_cntl; > -}; > - > /* generic helper function for reading 9-16 bit wide regs */ > static inline int axp20x_read_variable_width(struct regmap *regmap, > unsigned int reg, unsigned int width) > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] extcon: axp288: Remove unused platform data 2018-01-02 0:35 ` Chanwoo Choi @ 2018-01-02 9:16 ` Lee Jones 2018-01-03 1:34 ` Chanwoo Choi 0 siblings, 1 reply; 18+ messages in thread From: Lee Jones @ 2018-01-02 9:16 UTC (permalink / raw) To: Chanwoo Choi; +Cc: Hans de Goede, MyungJoo Ham, linux-kernel On Tue, 02 Jan 2018, Chanwoo Choi wrote: > + MFD Maintainer (Lee Jones <lee.jones@linaro.org>) > > Hi Hans, > > You better to use the scripts/get_maintainer.pl for the mailing list. > I added the MFD maintainer. > > This patch looks good to me. > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> > > > Best Regards, > Chanwoo Choi > > On 2017년 12월 22일 21:36, Hans de Goede wrote: > > This is not used / set anywhere in the tree. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/extcon/extcon-axp288.c | 35 +---------------------------------- > > include/linux/mfd/axp20x.h | 5 ----- Acked-by: Lee Jones <lee.jones@linaro.org> -- Lee Jones Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] extcon: axp288: Remove unused platform data 2018-01-02 9:16 ` Lee Jones @ 2018-01-03 1:34 ` Chanwoo Choi 2018-01-03 9:37 ` Lee Jones 0 siblings, 1 reply; 18+ messages in thread From: Chanwoo Choi @ 2018-01-03 1:34 UTC (permalink / raw) To: Lee Jones; +Cc: Hans de Goede, MyungJoo Ham, linux-kernel Dear Lee, On 2018년 01월 02일 18:16, Lee Jones wrote: > On Tue, 02 Jan 2018, Chanwoo Choi wrote: > >> + MFD Maintainer (Lee Jones <lee.jones@linaro.org>) >> >> Hi Hans, >> >> You better to use the scripts/get_maintainer.pl for the mailing list. >> I added the MFD maintainer. >> >> This patch looks good to me. >> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >> >> >> Best Regards, >> Chanwoo Choi >> >> On 2017년 12월 22일 21:36, Hans de Goede wrote: >>> This is not used / set anywhere in the tree. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/extcon/extcon-axp288.c | 35 +---------------------------------- >>> include/linux/mfd/axp20x.h | 5 ----- > > Acked-by: Lee Jones <lee.jones@linaro.org> > I already made the immutable branch[1] for both mfd and extcon. [1] https://lkml.org/lkml/2017/12/15/133 I added the additional patch for 'include/linux/mfd/axp20x.h' If you want to need to apply it on mfd tree, please apply them of immutable branch. The following changes since commit 4fbd8d194f06c8a3fd2af1ce560ddb31f7ec8323: Linux 4.15-rc1 (2017-11-26 16:01:47 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git ib-extcon-mfd-4.16 for you to fetch changes up to 9bf317e900a19a857eb9921c9441a92e89f40415: extcon: axp288: Remove unused platform data (2018-01-03 10:11:02 +0900) ---------------------------------------------------------------- Arvind Yadav (1): extcon: axp288:: Handle return value of platform_get_irq Benson Leung (1): extcon: usbc-cros-ec: add support to notify USB type cables. Hans de Goede (2): extcon: axp288: Remove unused extcon_nb struct member extcon: axp288: Remove unused platform data drivers/extcon/extcon-axp288.c | 39 +--------- drivers/extcon/extcon-usbc-cros-ec.c | 142 ++++++++++++++++++++++++++++++++++- include/linux/mfd/axp20x.h | 5 -- include/linux/mfd/cros_ec_commands.h | 17 +++++ 4 files changed, 159 insertions(+), 44 deletions(-) -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] extcon: axp288: Remove unused platform data 2018-01-03 1:34 ` Chanwoo Choi @ 2018-01-03 9:37 ` Lee Jones 2018-01-03 9:53 ` Chanwoo Choi 0 siblings, 1 reply; 18+ messages in thread From: Lee Jones @ 2018-01-03 9:37 UTC (permalink / raw) To: Chanwoo Choi; +Cc: Hans de Goede, MyungJoo Ham, linux-kernel On Wed, 03 Jan 2018, Chanwoo Choi wrote: > Dear Lee, > > On 2018년 01월 02일 18:16, Lee Jones wrote: > > On Tue, 02 Jan 2018, Chanwoo Choi wrote: > > > >> + MFD Maintainer (Lee Jones <lee.jones@linaro.org>) > >> > >> Hi Hans, > >> > >> You better to use the scripts/get_maintainer.pl for the mailing list. > >> I added the MFD maintainer. > >> > >> This patch looks good to me. > >> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> > >> > >> > >> Best Regards, > >> Chanwoo Choi > >> > >> On 2017년 12월 22일 21:36, Hans de Goede wrote: > >>> This is not used / set anywhere in the tree. > >>> > >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>> --- > >>> drivers/extcon/extcon-axp288.c | 35 +---------------------------------- > >>> include/linux/mfd/axp20x.h | 5 ----- > > > > Acked-by: Lee Jones <lee.jones@linaro.org> > > > > I already made the immutable branch[1] for both mfd and extcon. > [1] https://lkml.org/lkml/2017/12/15/133 > > I added the additional patch for 'include/linux/mfd/axp20x.h' > If you want to need to apply it on mfd tree, please apply them of immutable branch. > > > The following changes since commit 4fbd8d194f06c8a3fd2af1ce560ddb31f7ec8323: > > Linux 4.15-rc1 (2017-11-26 16:01:47 -0800) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git ib-extcon-mfd-4.16 How is this possible? ib-extcon-mfd-4.16 is already immutable. I believe you should have created a new tag: ib-extcon-mfd-4.16-1 ... based on ib-extcon-mfd-4.16. > for you to fetch changes up to 9bf317e900a19a857eb9921c9441a92e89f40415: > > extcon: axp288: Remove unused platform data (2018-01-03 10:11:02 +0900) > > ---------------------------------------------------------------- > Arvind Yadav (1): > extcon: axp288:: Handle return value of platform_get_irq > > Benson Leung (1): > extcon: usbc-cros-ec: add support to notify USB type cables. > > Hans de Goede (2): > extcon: axp288: Remove unused extcon_nb struct member > extcon: axp288: Remove unused platform data > > drivers/extcon/extcon-axp288.c | 39 +--------- > drivers/extcon/extcon-usbc-cros-ec.c | 142 ++++++++++++++++++++++++++++++++++- > include/linux/mfd/axp20x.h | 5 -- > include/linux/mfd/cros_ec_commands.h | 17 +++++ > 4 files changed, 159 insertions(+), 44 deletions(-) > -- Lee Jones Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] extcon: axp288: Remove unused platform data 2018-01-03 9:37 ` Lee Jones @ 2018-01-03 9:53 ` Chanwoo Choi 2018-01-05 10:44 ` Lee Jones 0 siblings, 1 reply; 18+ messages in thread From: Chanwoo Choi @ 2018-01-03 9:53 UTC (permalink / raw) To: Lee Jones; +Cc: Hans de Goede, MyungJoo Ham, linux-kernel On 2018년 01월 03일 18:37, Lee Jones wrote: > On Wed, 03 Jan 2018, Chanwoo Choi wrote: > >> Dear Lee, >> >> On 2018년 01월 02일 18:16, Lee Jones wrote: >>> On Tue, 02 Jan 2018, Chanwoo Choi wrote: >>> >>>> + MFD Maintainer (Lee Jones <lee.jones@linaro.org>) >>>> >>>> Hi Hans, >>>> >>>> You better to use the scripts/get_maintainer.pl for the mailing list. >>>> I added the MFD maintainer. >>>> >>>> This patch looks good to me. >>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >>>> >>>> >>>> Best Regards, >>>> Chanwoo Choi >>>> >>>> On 2017년 12월 22일 21:36, Hans de Goede wrote: >>>>> This is not used / set anywhere in the tree. >>>>> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> --- >>>>> drivers/extcon/extcon-axp288.c | 35 +---------------------------------- >>>>> include/linux/mfd/axp20x.h | 5 ----- >>> >>> Acked-by: Lee Jones <lee.jones@linaro.org> >>> >> >> I already made the immutable branch[1] for both mfd and extcon. >> [1] https://lkml.org/lkml/2017/12/15/133 >> >> I added the additional patch for 'include/linux/mfd/axp20x.h' >> If you want to need to apply it on mfd tree, please apply them of immutable branch. >> >> >> The following changes since commit 4fbd8d194f06c8a3fd2af1ce560ddb31f7ec8323: >> >> Linux 4.15-rc1 (2017-11-26 16:01:47 -0800) >> >> are available in the git repository at: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git ib-extcon-mfd-4.16 > > How is this possible? ib-extcon-mfd-4.16 is already immutable. > > I believe you should have created a new tag: > > ib-extcon-mfd-4.16-1 > > ... based on ib-extcon-mfd-4.16. You're right. I make the new immutable branch (ib-extcon-mfd-4.16-1) as following: The following changes since commit c7eb47f9e45226571be31212f6efd4b307d3b59d: extcon: usbc-cros-ec: add support to notify USB type cables. (2017-12-15 17:21:49 +0900) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git tags/ib-extcon-mfd-4.16-1 for you to fetch changes up to 9bf317e900a19a857eb9921c9441a92e89f40415: extcon: axp288: Remove unused platform data (2018-01-03 10:11:02 +0900) ---------------------------------------------------------------- Immutable branch for both MFD and EXTCON tree. ---------------------------------------------------------------- Arvind Yadav (1): extcon: axp288:: Handle return value of platform_get_irq Hans de Goede (2): extcon: axp288: Remove unused extcon_nb struct member extcon: axp288: Remove unused platform data drivers/extcon/extcon-axp288.c | 39 ++++----------------------------------- include/linux/mfd/axp20x.h | 5 ----- 2 files changed, 4 insertions(+), 40 deletions(-) > >> for you to fetch changes up to 9bf317e900a19a857eb9921c9441a92e89f40415: >> >> extcon: axp288: Remove unused platform data (2018-01-03 10:11:02 +0900) >> >> ---------------------------------------------------------------- >> Arvind Yadav (1): >> extcon: axp288:: Handle return value of platform_get_irq >> >> Benson Leung (1): >> extcon: usbc-cros-ec: add support to notify USB type cables. >> >> Hans de Goede (2): >> extcon: axp288: Remove unused extcon_nb struct member >> extcon: axp288: Remove unused platform data >> >> drivers/extcon/extcon-axp288.c | 39 +--------- >> drivers/extcon/extcon-usbc-cros-ec.c | 142 ++++++++++++++++++++++++++++++++++- >> include/linux/mfd/axp20x.h | 5 -- >> include/linux/mfd/cros_ec_commands.h | 17 +++++ >> 4 files changed, 159 insertions(+), 44 deletions(-) >> > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] extcon: axp288: Remove unused platform data 2018-01-03 9:53 ` Chanwoo Choi @ 2018-01-05 10:44 ` Lee Jones 0 siblings, 0 replies; 18+ messages in thread From: Lee Jones @ 2018-01-05 10:44 UTC (permalink / raw) To: Chanwoo Choi; +Cc: Hans de Goede, MyungJoo Ham, linux-kernel > The following changes since commit c7eb47f9e45226571be31212f6efd4b307d3b59d: > > extcon: usbc-cros-ec: add support to notify USB type cables. (2017-12-15 17:21:49 +0900) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git tags/ib-extcon-mfd-4.16-1 > > for you to fetch changes up to 9bf317e900a19a857eb9921c9441a92e89f40415: > > extcon: axp288: Remove unused platform data (2018-01-03 10:11:02 +0900) > > ---------------------------------------------------------------- > Immutable branch for both MFD and EXTCON tree. > > ---------------------------------------------------------------- > Arvind Yadav (1): > extcon: axp288:: Handle return value of platform_get_irq > > Hans de Goede (2): > extcon: axp288: Remove unused extcon_nb struct member > extcon: axp288: Remove unused platform data > > drivers/extcon/extcon-axp288.c | 39 ++++----------------------------------- > include/linux/mfd/axp20x.h | 5 ----- > 2 files changed, 4 insertions(+), 40 deletions(-) Pulled, thanks Chanwoo. -- Lee Jones Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() 2017-12-22 12:36 ` [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member Hans de Goede 2017-12-22 12:36 ` [PATCH 2/4] extcon: axp288: Remove unused platform data Hans de Goede @ 2017-12-22 12:36 ` Hans de Goede 2018-01-02 0:54 ` Chanwoo Choi 2017-12-22 12:36 ` [PATCH 4/4] extcon: axp288: Handle reserved charger-type values better Hans de Goede ` (2 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Hans de Goede @ 2017-12-22 12:36 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: Hans de Goede, linux-kernel The axp288 extcon code depends on other drivers to do things like mux the data lines, enable/disable vbus based on the id-pin, etc. Sometimes the BIOS has not set these things up correctly resulting in the initial charger cable type detection giving a wrong result and we end up not charging or charging at only 0.5A. This commit starts a second charger-detection cycle a couple of seconds after the first one finishes, giving the other drivers time to load and do their thing. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/extcon-axp288.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c index 386afb7d1160..cc7c35c7ff02 100644 --- a/drivers/extcon/extcon-axp288.c +++ b/drivers/extcon/extcon-axp288.c @@ -1,6 +1,7 @@ /* * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver * + * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com> * Copyright (C) 2015 Intel Corporation * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com> * @@ -97,9 +98,11 @@ struct axp288_extcon_info { struct device *dev; struct regmap *regmap; struct regmap_irq_chip_data *regmap_irqc; + struct delayed_work det_work; int irq[EXTCON_IRQ_END]; struct extcon_dev *edev; unsigned int previous_cable; + bool first_detect_done; }; /* Power up/down reason string array */ @@ -137,6 +140,25 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info) regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG, clear_mask); } +static void axp288_chrg_detect_complete(struct axp288_extcon_info *info) +{ + /* + * We depend on other drivers to do things like mux the data lines, + * enable/disable vbus based on the id-pin, etc. Sometimes the BIOS has + * not set these things up correctly resulting in the initial charger + * cable type detection giving a wrong result and we end up not charging + * or charging at only 0.5A. + * + * So we schedule a second cable type detection after 2 seconds to + * give the other drivers time to load and do their thing. + */ + if (!info->first_detect_done) { + queue_delayed_work(system_wq, &info->det_work, + msecs_to_jiffies(2000)); + info->first_detect_done = true; + } +} + static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) { int ret, stat, cfg, pwr_stat; @@ -201,6 +223,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) info->previous_cable = cable; } + axp288_chrg_detect_complete(info); + return 0; dev_det_ret: @@ -222,8 +246,11 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data) return IRQ_HANDLED; } -static void axp288_extcon_enable(struct axp288_extcon_info *info) +static void axp288_extcon_det_work(struct work_struct *work) { + struct axp288_extcon_info *info = + container_of(work, struct axp288_extcon_info, det_work.work); + regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG, BC_GLOBAL_RUN, 0); /* Enable the charger detection logic */ @@ -245,6 +272,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) info->regmap = axp20x->regmap; info->regmap_irqc = axp20x->regmap_irqc; info->previous_cable = EXTCON_NONE; + INIT_DELAYED_WORK(&info->det_work, axp288_extcon_det_work); platform_set_drvdata(pdev, info); @@ -287,7 +315,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) } /* Start charger cable type detection */ - axp288_extcon_enable(info); + queue_delayed_work(system_wq, &info->det_work, 0); return 0; } -- 2.14.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() 2017-12-22 12:36 ` [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() Hans de Goede @ 2018-01-02 0:54 ` Chanwoo Choi 2018-01-02 22:44 ` Hans de Goede 0 siblings, 1 reply; 18+ messages in thread From: Chanwoo Choi @ 2018-01-02 0:54 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel Hi Hans, s/dection/detection on patch title. On 2017년 12월 22일 21:36, Hans de Goede wrote: > The axp288 extcon code depends on other drivers to do things like mux the > data lines, enable/disable vbus based on the id-pin, etc. > > Sometimes the BIOS has not set these things up correctly resulting in the > initial charger cable type detection giving a wrong result and we end up > not charging or charging at only 0.5A. > > This commit starts a second charger-detection cycle a couple of seconds > after the first one finishes, giving the other drivers time to load and > do their thing. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/extcon-axp288.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index 386afb7d1160..cc7c35c7ff02 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -1,6 +1,7 @@ > /* > * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver > * > + * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com> > * Copyright (C) 2015 Intel Corporation > * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com> > * > @@ -97,9 +98,11 @@ struct axp288_extcon_info { > struct device *dev; > struct regmap *regmap; > struct regmap_irq_chip_data *regmap_irqc; > + struct delayed_work det_work; > int irq[EXTCON_IRQ_END]; > struct extcon_dev *edev; > unsigned int previous_cable; > + bool first_detect_done; The first_detect_done is used only one time in the axp288_handle_chrg_det_event(). The other function don't use it. So, you better to define and use 'static bool first_detect_done' in the axp288_handle_chrg_det_event() instead of defining the 'first_detect_done' in the struct axp288_extcon_info. > }; > > /* Power up/down reason string array */ > @@ -137,6 +140,25 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info) > regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG, clear_mask); > } > > +static void axp288_chrg_detect_complete(struct axp288_extcon_info *info) > +{ > + /* > + * We depend on other drivers to do things like mux the data lines, > + * enable/disable vbus based on the id-pin, etc. Sometimes the BIOS has > + * not set these things up correctly resulting in the initial charger > + * cable type detection giving a wrong result and we end up not charging > + * or charging at only 0.5A. > + * > + * So we schedule a second cable type detection after 2 seconds to > + * give the other drivers time to load and do their thing. > + */ > + if (!info->first_detect_done) { > + queue_delayed_work(system_wq, &info->det_work, > + msecs_to_jiffies(2000)); > + info->first_detect_done = true; > + } > +} The axp288_chrg_detect_complete() is only used in the axp288_handle_chrg_det_event() and axp288_chrg_detect_complete() is not a complicate. I think that you don't need to make the separate function. I'd like you to add the > + > static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) > { > int ret, stat, cfg, pwr_stat; > @@ -201,6 +223,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) > info->previous_cable = cable; > } > > + axp288_chrg_detect_complete(info); As I commented, you better to add the code directly instead of separate function. > + > return 0; > > dev_det_ret: > @@ -222,8 +246,11 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data) > return IRQ_HANDLED; > } > > -static void axp288_extcon_enable(struct axp288_extcon_info *info) > +static void axp288_extcon_det_work(struct work_struct *work) > { > + struct axp288_extcon_info *info = > + container_of(work, struct axp288_extcon_info, det_work.work); > + > regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG, > BC_GLOBAL_RUN, 0); > /* Enable the charger detection logic */ > @@ -245,6 +272,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) > info->regmap = axp20x->regmap; > info->regmap_irqc = axp20x->regmap_irqc; > info->previous_cable = EXTCON_NONE; > + INIT_DELAYED_WORK(&info->det_work, axp288_extcon_det_work); > > platform_set_drvdata(pdev, info); > > @@ -287,7 +315,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) > } > > /* Start charger cable type detection */ > - axp288_extcon_enable(info); > + queue_delayed_work(system_wq, &info->det_work, 0); > > return 0; > } > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() 2018-01-02 0:54 ` Chanwoo Choi @ 2018-01-02 22:44 ` Hans de Goede 2018-01-03 0:58 ` Chanwoo Choi 0 siblings, 1 reply; 18+ messages in thread From: Hans de Goede @ 2018-01-02 22:44 UTC (permalink / raw) To: Chanwoo Choi, MyungJoo Ham; +Cc: linux-kernel Hi, On 02-01-18 01:54, Chanwoo Choi wrote: > Hi Hans, > > s/dection/detection on patch title. Thank you for all the reviews. I've fixed the typo in my personal tree. > On 2017년 12월 22일 21:36, Hans de Goede wrote: >> The axp288 extcon code depends on other drivers to do things like mux the >> data lines, enable/disable vbus based on the id-pin, etc. >> >> Sometimes the BIOS has not set these things up correctly resulting in the >> initial charger cable type detection giving a wrong result and we end up >> not charging or charging at only 0.5A. >> >> This commit starts a second charger-detection cycle a couple of seconds >> after the first one finishes, giving the other drivers time to load and >> do their thing. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/extcon/extcon-axp288.c | 32 ++++++++++++++++++++++++++++++-- >> 1 file changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c >> index 386afb7d1160..cc7c35c7ff02 100644 >> --- a/drivers/extcon/extcon-axp288.c >> +++ b/drivers/extcon/extcon-axp288.c >> @@ -1,6 +1,7 @@ >> /* >> * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver >> * >> + * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com> >> * Copyright (C) 2015 Intel Corporation >> * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com> >> * >> @@ -97,9 +98,11 @@ struct axp288_extcon_info { >> struct device *dev; >> struct regmap *regmap; >> struct regmap_irq_chip_data *regmap_irqc; >> + struct delayed_work det_work; >> int irq[EXTCON_IRQ_END]; >> struct extcon_dev *edev; >> unsigned int previous_cable; >> + bool first_detect_done; > > The first_detect_done is used only one time in the axp288_handle_chrg_det_event(). > The other function don't use it. So, you better to define and use > 'static bool first_detect_done' in the axp288_handle_chrg_det_event() > instead of defining the 'first_detect_done' in the struct axp288_extcon_info. But what if a device has 2 axp288 PMICs (unlikely I know) then only the first one to check this will do the re-detect 2 seconds later, unless they race, which is bad in itself too. In general using static function variables is a bad idea and should be avoided, it does not work when their are multiple instances of the device and it is racy. So sorry but I'm not going to make this change. > >> }; >> >> /* Power up/down reason string array */ >> @@ -137,6 +140,25 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info) >> regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG, clear_mask); >> } >> >> +static void axp288_chrg_detect_complete(struct axp288_extcon_info *info) >> +{ >> + /* >> + * We depend on other drivers to do things like mux the data lines, >> + * enable/disable vbus based on the id-pin, etc. Sometimes the BIOS has >> + * not set these things up correctly resulting in the initial charger >> + * cable type detection giving a wrong result and we end up not charging >> + * or charging at only 0.5A. >> + * >> + * So we schedule a second cable type detection after 2 seconds to >> + * give the other drivers time to load and do their thing. >> + */ >> + if (!info->first_detect_done) { >> + queue_delayed_work(system_wq, &info->det_work, >> + msecs_to_jiffies(2000)); >> + info->first_detect_done = true; >> + } >> +} > > The axp288_chrg_detect_complete() is only used in the axp288_handle_chrg_det_event() > and axp288_chrg_detect_complete() is not a complicate. I think that you don't need > to make the separate function. I'd like you to add the > >> + >> static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) >> { >> int ret, stat, cfg, pwr_stat; >> @@ -201,6 +223,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) >> info->previous_cable = cable; >> } >> >> + axp288_chrg_detect_complete(info); > > As I commented, you better to add the code directly instead of separate function. I would prefer to keep this as a separate function, that keeps the main flow of the axp288_handle_chrg_det_event function a lot more readable IMHO. axp288_handle_chrg_det_event already has a non trivial code flow adding more conditional code to it only makes it harder to read. But if you insist I can move the code inside the function for v2. Note that this will not make a difference for the code generated by the compiler, the compiler will auto-inline it anyways. > >> + >> return 0; >> >> dev_det_ret: >> @@ -222,8 +246,11 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data) >> return IRQ_HANDLED; >> } >> >> -static void axp288_extcon_enable(struct axp288_extcon_info *info) >> +static void axp288_extcon_det_work(struct work_struct *work) >> { >> + struct axp288_extcon_info *info = >> + container_of(work, struct axp288_extcon_info, det_work.work); >> + >> regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG, >> BC_GLOBAL_RUN, 0); >> /* Enable the charger detection logic */ >> @@ -245,6 +272,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) >> info->regmap = axp20x->regmap; >> info->regmap_irqc = axp20x->regmap_irqc; >> info->previous_cable = EXTCON_NONE; >> + INIT_DELAYED_WORK(&info->det_work, axp288_extcon_det_work); >> >> platform_set_drvdata(pdev, info); >> >> @@ -287,7 +315,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) >> } >> >> /* Start charger cable type detection */ >> - axp288_extcon_enable(info); >> + queue_delayed_work(system_wq, &info->det_work, 0); >> >> return 0; >> } >> > > Regards, Hans ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() 2018-01-02 22:44 ` Hans de Goede @ 2018-01-03 0:58 ` Chanwoo Choi 2018-01-03 1:04 ` Chanwoo Choi 0 siblings, 1 reply; 18+ messages in thread From: Chanwoo Choi @ 2018-01-03 0:58 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel Hi Hans, On 2018년 01월 03일 07:44, Hans de Goede wrote: > Hi, > > On 02-01-18 01:54, Chanwoo Choi wrote: >> Hi Hans, >> >> s/dection/detection on patch title. > > Thank you for all the reviews. > > I've fixed the typo in my personal tree. > >> On 2017년 12월 22일 21:36, Hans de Goede wrote: >>> The axp288 extcon code depends on other drivers to do things like mux the >>> data lines, enable/disable vbus based on the id-pin, etc. >>> >>> Sometimes the BIOS has not set these things up correctly resulting in the >>> initial charger cable type detection giving a wrong result and we end up >>> not charging or charging at only 0.5A. >>> >>> This commit starts a second charger-detection cycle a couple of seconds >>> after the first one finishes, giving the other drivers time to load and >>> do their thing. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/extcon/extcon-axp288.c | 32 ++++++++++++++++++++++++++++++-- >>> 1 file changed, 30 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c >>> index 386afb7d1160..cc7c35c7ff02 100644 >>> --- a/drivers/extcon/extcon-axp288.c >>> +++ b/drivers/extcon/extcon-axp288.c >>> @@ -1,6 +1,7 @@ >>> /* >>> * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver >>> * >>> + * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com> >>> * Copyright (C) 2015 Intel Corporation >>> * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com> >>> * >>> @@ -97,9 +98,11 @@ struct axp288_extcon_info { >>> struct device *dev; >>> struct regmap *regmap; >>> struct regmap_irq_chip_data *regmap_irqc; >>> + struct delayed_work det_work; >>> int irq[EXTCON_IRQ_END]; >>> struct extcon_dev *edev; >>> unsigned int previous_cable; >>> + bool first_detect_done; >> >> The first_detect_done is used only one time in the axp288_handle_chrg_det_event(). >> The other function don't use it. So, you better to define and use >> 'static bool first_detect_done' in the axp288_handle_chrg_det_event() >> instead of defining the 'first_detect_done' in the struct axp288_extcon_info. > > But what if a device has 2 axp288 PMICs (unlikely I know) then only the > first one to check this will do the re-detect 2 seconds later, unless they > race, which is bad in itself too. > > In general using static function variables is a bad idea and should be > avoided, it does not work when their are multiple instances of the device > and it is racy. So sorry but I'm not going to make this change. You're right. It is my mistake. Please keep your way. > >> >>> }; >>> /* Power up/down reason string array */ >>> @@ -137,6 +140,25 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info) >>> regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG, clear_mask); >>> } >>> +static void axp288_chrg_detect_complete(struct axp288_extcon_info *info) >>> +{ >>> + /* >>> + * We depend on other drivers to do things like mux the data lines, >>> + * enable/disable vbus based on the id-pin, etc. Sometimes the BIOS has >>> + * not set these things up correctly resulting in the initial charger >>> + * cable type detection giving a wrong result and we end up not charging >>> + * or charging at only 0.5A. >>> + * >>> + * So we schedule a second cable type detection after 2 seconds to >>> + * give the other drivers time to load and do their thing. >>> + */ >>> + if (!info->first_detect_done) { >>> + queue_delayed_work(system_wq, &info->det_work, >>> + msecs_to_jiffies(2000)); >>> + info->first_detect_done = true; >>> + } >>> +} >> >> The axp288_chrg_detect_complete() is only used in the axp288_handle_chrg_det_event() >> and axp288_chrg_detect_complete() is not a complicate. I think that you don't need >> to make the separate function. I'd like you to add the >> >>> + >>> static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) >>> { >>> int ret, stat, cfg, pwr_stat; >>> @@ -201,6 +223,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) >>> info->previous_cable = cable; >>> } >>> + axp288_chrg_detect_complete(info); >> >> As I commented, you better to add the code directly instead of separate function. > > I would prefer to keep this as a separate function, that keeps the main > flow of the axp288_handle_chrg_det_event function a lot more readable IMHO. > > axp288_handle_chrg_det_event already has a non trivial code flow adding more > conditional code to it only makes it harder to read. > > But if you insist I can move the code inside the function for v2. Note that > this will not make a difference for the code generated by the compiler, the > compiler will auto-inline it anyways. I didn't mention the result from compiler. I focus on the readability. On v1, the developer always check what to do in axp288_chrg_detect_complete() even if this function is used only one time like '__init'. Actually, axp288_chrg_detect_complete() is not complicate and short. But, I will not be forced because it is not a critical issue. If you want to keep the separate function, you just send v2 with only fixing the typo. > > >> >>> + >>> return 0; >>> dev_det_ret: >>> @@ -222,8 +246,11 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data) >>> return IRQ_HANDLED; >>> } >>> -static void axp288_extcon_enable(struct axp288_extcon_info *info) >>> +static void axp288_extcon_det_work(struct work_struct *work) >>> { >>> + struct axp288_extcon_info *info = >>> + container_of(work, struct axp288_extcon_info, det_work.work); >>> + >>> regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG, >>> BC_GLOBAL_RUN, 0); >>> /* Enable the charger detection logic */ >>> @@ -245,6 +272,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) >>> info->regmap = axp20x->regmap; >>> info->regmap_irqc = axp20x->regmap_irqc; >>> info->previous_cable = EXTCON_NONE; >>> + INIT_DELAYED_WORK(&info->det_work, axp288_extcon_det_work); >>> platform_set_drvdata(pdev, info); >>> @@ -287,7 +315,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) >>> } >>> /* Start charger cable type detection */ >>> - axp288_extcon_enable(info); >>> + queue_delayed_work(system_wq, &info->det_work, 0); >>> return 0; >>> } >>> >> >> > > Regards, > > Hans > > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() 2018-01-03 0:58 ` Chanwoo Choi @ 2018-01-03 1:04 ` Chanwoo Choi 0 siblings, 0 replies; 18+ messages in thread From: Chanwoo Choi @ 2018-01-03 1:04 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel Hi Hans, On 2018년 01월 03일 09:58, Chanwoo Choi wrote: > Hi Hans, > > On 2018년 01월 03일 07:44, Hans de Goede wrote: >> Hi, >> >> On 02-01-18 01:54, Chanwoo Choi wrote: >>> Hi Hans, >>> >>> s/dection/detection on patch title. >> >> Thank you for all the reviews. >> >> I've fixed the typo in my personal tree. >> >>> On 2017년 12월 22일 21:36, Hans de Goede wrote: >>>> The axp288 extcon code depends on other drivers to do things like mux the >>>> data lines, enable/disable vbus based on the id-pin, etc. >>>> >>>> Sometimes the BIOS has not set these things up correctly resulting in the >>>> initial charger cable type detection giving a wrong result and we end up >>>> not charging or charging at only 0.5A. >>>> >>>> This commit starts a second charger-detection cycle a couple of seconds >>>> after the first one finishes, giving the other drivers time to load and >>>> do their thing. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/extcon/extcon-axp288.c | 32 ++++++++++++++++++++++++++++++-- >>>> 1 file changed, 30 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c >>>> index 386afb7d1160..cc7c35c7ff02 100644 >>>> --- a/drivers/extcon/extcon-axp288.c >>>> +++ b/drivers/extcon/extcon-axp288.c >>>> @@ -1,6 +1,7 @@ >>>> /* >>>> * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver >>>> * >>>> + * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com> >>>> * Copyright (C) 2015 Intel Corporation >>>> * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com> >>>> * >>>> @@ -97,9 +98,11 @@ struct axp288_extcon_info { >>>> struct device *dev; >>>> struct regmap *regmap; >>>> struct regmap_irq_chip_data *regmap_irqc; >>>> + struct delayed_work det_work; >>>> int irq[EXTCON_IRQ_END]; >>>> struct extcon_dev *edev; >>>> unsigned int previous_cable; >>>> + bool first_detect_done; >>> >>> The first_detect_done is used only one time in the axp288_handle_chrg_det_event(). >>> The other function don't use it. So, you better to define and use >>> 'static bool first_detect_done' in the axp288_handle_chrg_det_event() >>> instead of defining the 'first_detect_done' in the struct axp288_extcon_info. >> >> But what if a device has 2 axp288 PMICs (unlikely I know) then only the >> first one to check this will do the re-detect 2 seconds later, unless they >> race, which is bad in itself too. >> >> In general using static function variables is a bad idea and should be >> avoided, it does not work when their are multiple instances of the device >> and it is racy. So sorry but I'm not going to make this change. > > You're right. It is my mistake. Please keep your way. > >> >>> >>>> }; >>>> /* Power up/down reason string array */ >>>> @@ -137,6 +140,25 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info) >>>> regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG, clear_mask); >>>> } >>>> +static void axp288_chrg_detect_complete(struct axp288_extcon_info *info) >>>> +{ >>>> + /* >>>> + * We depend on other drivers to do things like mux the data lines, >>>> + * enable/disable vbus based on the id-pin, etc. Sometimes the BIOS has >>>> + * not set these things up correctly resulting in the initial charger >>>> + * cable type detection giving a wrong result and we end up not charging >>>> + * or charging at only 0.5A. >>>> + * >>>> + * So we schedule a second cable type detection after 2 seconds to >>>> + * give the other drivers time to load and do their thing. >>>> + */ >>>> + if (!info->first_detect_done) { >>>> + queue_delayed_work(system_wq, &info->det_work, >>>> + msecs_to_jiffies(2000)); >>>> + info->first_detect_done = true; >>>> + } >>>> +} >>> >>> The axp288_chrg_detect_complete() is only used in the axp288_handle_chrg_det_event() >>> and axp288_chrg_detect_complete() is not a complicate. I think that you don't need >>> to make the separate function. I'd like you to add the >>> >>>> + >>>> static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) >>>> { >>>> int ret, stat, cfg, pwr_stat; >>>> @@ -201,6 +223,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) >>>> info->previous_cable = cable; >>>> } >>>> + axp288_chrg_detect_complete(info); >>> >>> As I commented, you better to add the code directly instead of separate function. >> >> I would prefer to keep this as a separate function, that keeps the main >> flow of the axp288_handle_chrg_det_event function a lot more readable IMHO. >> >> axp288_handle_chrg_det_event already has a non trivial code flow adding more >> conditional code to it only makes it harder to read. >> >> But if you insist I can move the code inside the function for v2. Note that >> this will not make a difference for the code generated by the compiler, the >> compiler will auto-inline it anyways. > > I didn't mention the result from compiler. I focus on the readability. > On v1, the developer always check what to do in axp288_chrg_detect_complete() > even if this function is used only one time like '__init'. > Actually, axp288_chrg_detect_complete() is not complicate and short. > > But, I will not be forced because it is not a critical issue. > If you want to keep the separate function, you just send v2 with only fixing the typo. You don't need to send v2. I'll fix the typo and apply your patch v1 As I mentioned, it is not a critical issue. Thanks. > >> >> >>> >>>> + >>>> return 0; >>>> dev_det_ret: >>>> @@ -222,8 +246,11 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data) >>>> return IRQ_HANDLED; >>>> } >>>> -static void axp288_extcon_enable(struct axp288_extcon_info *info) >>>> +static void axp288_extcon_det_work(struct work_struct *work) >>>> { >>>> + struct axp288_extcon_info *info = >>>> + container_of(work, struct axp288_extcon_info, det_work.work); >>>> + >>>> regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG, >>>> BC_GLOBAL_RUN, 0); >>>> /* Enable the charger detection logic */ >>>> @@ -245,6 +272,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) >>>> info->regmap = axp20x->regmap; >>>> info->regmap_irqc = axp20x->regmap_irqc; >>>> info->previous_cable = EXTCON_NONE; >>>> + INIT_DELAYED_WORK(&info->det_work, axp288_extcon_det_work); >>>> platform_set_drvdata(pdev, info); >>>> @@ -287,7 +315,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) >>>> } >>>> /* Start charger cable type detection */ >>>> - axp288_extcon_enable(info); >>>> + queue_delayed_work(system_wq, &info->det_work, 0); >>>> return 0; >>>> } >>>> >>> >>> >> >> Regards, >> >> Hans >> >> >> > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/4] extcon: axp288: Handle reserved charger-type values better 2017-12-22 12:36 ` [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member Hans de Goede 2017-12-22 12:36 ` [PATCH 2/4] extcon: axp288: Remove unused platform data Hans de Goede 2017-12-22 12:36 ` [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() Hans de Goede @ 2017-12-22 12:36 ` Hans de Goede 2018-01-02 0:55 ` Chanwoo Choi 2018-01-02 0:36 ` [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member Chanwoo Choi 2018-01-03 1:17 ` Chanwoo Choi 4 siblings, 1 reply; 18+ messages in thread From: Hans de Goede @ 2017-12-22 12:36 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: Hans de Goede, linux-kernel According to the data sheets all the values not handled in the switch-case are "reserved". Update the dev_warn message to reflect this and set the cable-type to EXTCON_CHG_USB_SDP (so max 500mA current draw) as safe default. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/extcon-axp288.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c index cc7c35c7ff02..d60c615f709f 100644 --- a/drivers/extcon/extcon-axp288.c +++ b/drivers/extcon/extcon-axp288.c @@ -205,8 +205,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) cable = EXTCON_CHG_USB_DCP; break; default: - dev_warn(info->dev, - "disconnect or unknown or ID event\n"); + dev_warn(info->dev, "unknown (reserved) bc detect result\n"); + cable = EXTCON_CHG_USB_SDP; } no_vbus: -- 2.14.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] extcon: axp288: Handle reserved charger-type values better 2017-12-22 12:36 ` [PATCH 4/4] extcon: axp288: Handle reserved charger-type values better Hans de Goede @ 2018-01-02 0:55 ` Chanwoo Choi 0 siblings, 0 replies; 18+ messages in thread From: Chanwoo Choi @ 2018-01-02 0:55 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel Hi Hans, On 2017년 12월 22일 21:36, Hans de Goede wrote: > According to the data sheets all the values not handled in the > switch-case are "reserved". Update the dev_warn message to reflect > this and set the cable-type to EXTCON_CHG_USB_SDP (so max 500mA > current draw) as safe default. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/extcon-axp288.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index cc7c35c7ff02..d60c615f709f 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -205,8 +205,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) > cable = EXTCON_CHG_USB_DCP; > break; > default: > - dev_warn(info->dev, > - "disconnect or unknown or ID event\n"); > + dev_warn(info->dev, "unknown (reserved) bc detect result\n"); > + cable = EXTCON_CHG_USB_SDP; > } > > no_vbus: > Looks good to me. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member 2017-12-22 12:36 ` [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member Hans de Goede ` (2 preceding siblings ...) 2017-12-22 12:36 ` [PATCH 4/4] extcon: axp288: Handle reserved charger-type values better Hans de Goede @ 2018-01-02 0:36 ` Chanwoo Choi 2018-01-03 1:17 ` Chanwoo Choi 4 siblings, 0 replies; 18+ messages in thread From: Chanwoo Choi @ 2018-01-02 0:36 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel On 2017년 12월 22일 21:36, Hans de Goede wrote: > Remove the unused extcon_nb struct member. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/extcon-axp288.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index 981fba56bc18..ba185e9ebb82 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -107,7 +107,6 @@ struct axp288_extcon_info { > struct gpio_desc *gpio_mux_cntl; > int irq[EXTCON_IRQ_END]; > struct extcon_dev *edev; > - struct notifier_block extcon_nb; > unsigned int previous_cable; > }; > > Looks good to me. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member 2017-12-22 12:36 ` [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member Hans de Goede ` (3 preceding siblings ...) 2018-01-02 0:36 ` [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member Chanwoo Choi @ 2018-01-03 1:17 ` Chanwoo Choi 2018-01-03 8:08 ` Hans de Goede 4 siblings, 1 reply; 18+ messages in thread From: Chanwoo Choi @ 2018-01-03 1:17 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel Hi Hans, On 2017년 12월 22일 21:36, Hans de Goede wrote: > Remove the unused extcon_nb struct member. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/extcon-axp288.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index 981fba56bc18..ba185e9ebb82 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -107,7 +107,6 @@ struct axp288_extcon_info { > struct gpio_desc *gpio_mux_cntl; > int irq[EXTCON_IRQ_END]; > struct extcon_dev *edev; > - struct notifier_block extcon_nb; > unsigned int previous_cable; > }; > > Applied them(patch1/2/3/4). Thanks. -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member 2018-01-03 1:17 ` Chanwoo Choi @ 2018-01-03 8:08 ` Hans de Goede 0 siblings, 0 replies; 18+ messages in thread From: Hans de Goede @ 2018-01-03 8:08 UTC (permalink / raw) To: Chanwoo Choi, MyungJoo Ham; +Cc: linux-kernel Hi, On 03-01-18 02:17, Chanwoo Choi wrote: > Hi Hans, > On 2017년 12월 22일 21:36, Hans de Goede wrote: >> Remove the unused extcon_nb struct member. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/extcon/extcon-axp288.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c >> index 981fba56bc18..ba185e9ebb82 100644 >> --- a/drivers/extcon/extcon-axp288.c >> +++ b/drivers/extcon/extcon-axp288.c >> @@ -107,7 +107,6 @@ struct axp288_extcon_info { >> struct gpio_desc *gpio_mux_cntl; >> int irq[EXTCON_IRQ_END]; >> struct extcon_dev *edev; >> - struct notifier_block extcon_nb; >> unsigned int previous_cable; >> }; >> >> > > Applied them(patch1/2/3/4). Thanks. Great, thank you. Regards, Hans ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-01-05 10:44 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20171222123627epcas1p25953168cb2b1432d2065c0068a71397f@epcas1p2.samsung.com>
2017-12-22 12:36 ` [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member Hans de Goede
2017-12-22 12:36 ` [PATCH 2/4] extcon: axp288: Remove unused platform data Hans de Goede
2018-01-02 0:35 ` Chanwoo Choi
2018-01-02 9:16 ` Lee Jones
2018-01-03 1:34 ` Chanwoo Choi
2018-01-03 9:37 ` Lee Jones
2018-01-03 9:53 ` Chanwoo Choi
2018-01-05 10:44 ` Lee Jones
2017-12-22 12:36 ` [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() Hans de Goede
2018-01-02 0:54 ` Chanwoo Choi
2018-01-02 22:44 ` Hans de Goede
2018-01-03 0:58 ` Chanwoo Choi
2018-01-03 1:04 ` Chanwoo Choi
2017-12-22 12:36 ` [PATCH 4/4] extcon: axp288: Handle reserved charger-type values better Hans de Goede
2018-01-02 0:55 ` Chanwoo Choi
2018-01-02 0:36 ` [PATCH 1/4] extcon: axp288: Remove unused extcon_nb struct member Chanwoo Choi
2018-01-03 1:17 ` Chanwoo Choi
2018-01-03 8:08 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).