* Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe
2019-09-16 21:15 ` [PATCH v2] " Yauhen Kharuzhy
@ 2019-09-17 6:22 ` Hans de Goede
2019-09-17 11:13 ` Andy Shevchenko
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2019-09-17 6:22 UTC (permalink / raw)
To: Yauhen Kharuzhy, linux-kernel, Chanwoo Choi, MyungJoo Ham; +Cc: Andy Shevchenko
Hi,
On 16-09-2019 23:15, Yauhen Kharuzhy wrote:
> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> PMIC at driver probing for further charger detection. This causes reset of
> USB data sessions and removing all devices from bus. If system was
> booted from Live CD or USB dongle, this makes system unusable.
>
> Check if USB ID pin is floating and re-route data lines in this case
> only, don't touch otherwise.
>
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
Patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> drivers/extcon/extcon-intel-cht-wc.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 9d32150e68db..da1886a92f75 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -338,6 +338,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
> struct cht_wc_extcon_data *ext;
> unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
> + int pwrsrc_sts, id;
> int irq, ret;
>
> irq = platform_get_irq(pdev, 0);
> @@ -387,8 +388,19 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> goto disable_sw_control;
> }
>
> - /* Route D+ and D- to PMIC for initial charger detection */
> - cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
> + ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> + if (ret) {
> + dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> + goto disable_sw_control;
> + }
> +
> + id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> +
> + /* If no USB host or device connected, route D+ and D- to PMIC for
> + * initial charger detection
> + */
> + if (id != INTEL_USB_ID_GND)
> + cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
>
> /* Get initial state */
> cht_wc_extcon_pwrsrc_event(ext);
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe
2019-09-16 21:15 ` [PATCH v2] " Yauhen Kharuzhy
2019-09-17 6:22 ` Hans de Goede
@ 2019-09-17 11:13 ` Andy Shevchenko
2019-09-17 13:25 ` Yauhen Kharuzhy
2019-09-20 8:25 ` Andy Shevchenko
2019-09-20 8:43 ` Chanwoo Choi
3 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2019-09-17 11:13 UTC (permalink / raw)
To: Yauhen Kharuzhy; +Cc: linux-kernel, Chanwoo Choi, MyungJoo Ham, Hans de Goede
On Tue, Sep 17, 2019 at 12:15:36AM +0300, Yauhen Kharuzhy wrote:
> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> PMIC at driver probing for further charger detection. This causes reset of
> USB data sessions and removing all devices from bus. If system was
> booted from Live CD or USB dongle, this makes system unusable.
>
> Check if USB ID pin is floating and re-route data lines in this case
> only, don't touch otherwise.
> + ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> + if (ret) {
> + dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> + goto disable_sw_control;
> + }
> +
> + id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
We have second implementation of this. Would it make sense to split to some
helper?
> + /* If no USB host or device connected, route D+ and D- to PMIC for
> + * initial charger detection
> + */
I'm not sure it's acceptable style of multi-line comment, but it's up to
maintainer.
> + if (id != INTEL_USB_ID_GND)
> + cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
>
> /* Get initial state */
> cht_wc_extcon_pwrsrc_event(ext);
> --
> 2.23.0.rc1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe
2019-09-17 11:13 ` Andy Shevchenko
@ 2019-09-17 13:25 ` Yauhen Kharuzhy
2019-09-18 7:17 ` Andy Shevchenko
0 siblings, 1 reply; 9+ messages in thread
From: Yauhen Kharuzhy @ 2019-09-17 13:25 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-kernel, Chanwoo Choi, MyungJoo Ham, Hans de Goede
On Tue, Sep 17, 2019 at 02:13:22PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 17, 2019 at 12:15:36AM +0300, Yauhen Kharuzhy wrote:
> > Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> > PMIC at driver probing for further charger detection. This causes reset of
> > USB data sessions and removing all devices from bus. If system was
> > booted from Live CD or USB dongle, this makes system unusable.
> >
> > Check if USB ID pin is floating and re-route data lines in this case
> > only, don't touch otherwise.
>
> > + ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> > + if (ret) {
> > + dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> > + goto disable_sw_control;
> > + }
> > +
> > + id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
>
> We have second implementation of this. Would it make sense to split to some
> helper?
Do you mean the combination of regmap_read(...CHT_WC_PWRSRC_STS,
&pwrsrc_sts) with cht_wc_extcon_get_id()?
In the cht_wc_extcon_pwrsrc_event() function the pwrsrc_sts is checked
for other bits also, so separation of PWRSRC_STS read and id calculation
to one routine will cause non-clear function calls like as
get_powersrc_and_check_id(..., &powersrc_sts, &id) which is not looks
better than current code duplication. Or we need to spend some time for
refactoring and testing of cht_wc_extcon_pwrsrc_event() code.
>
> > + /* If no USB host or device connected, route D+ and D- to PMIC for
> > + * initial charger detection
> > + */
>
> I'm not sure it's acceptable style of multi-line comment, but it's up to
> maintainer.
Argh, you are right, I fixed this in local copy but this is not
significant typo.
>
> > + if (id != INTEL_USB_ID_GND)
> > + cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
> >
> > /* Get initial state */
> > cht_wc_extcon_pwrsrc_event(ext);
> > --
> > 2.23.0.rc1
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
--
Yauhen Kharuzhy
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe
2019-09-17 13:25 ` Yauhen Kharuzhy
@ 2019-09-18 7:17 ` Andy Shevchenko
2019-09-20 7:42 ` Chanwoo Choi
0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2019-09-18 7:17 UTC (permalink / raw)
To: Yauhen Kharuzhy
Cc: Andy Shevchenko, Linux Kernel Mailing List, Chanwoo Choi,
MyungJoo Ham, Hans de Goede
On Wed, Sep 18, 2019 at 2:04 AM Yauhen Kharuzhy <jekhor@gmail.com> wrote:
>
> On Tue, Sep 17, 2019 at 02:13:22PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 17, 2019 at 12:15:36AM +0300, Yauhen Kharuzhy wrote:
> > > Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> > > PMIC at driver probing for further charger detection. This causes reset of
> > > USB data sessions and removing all devices from bus. If system was
> > > booted from Live CD or USB dongle, this makes system unusable.
> > >
> > > Check if USB ID pin is floating and re-route data lines in this case
> > > only, don't touch otherwise.
> >
> > > + ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> > > + if (ret) {
> > > + dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> > > + goto disable_sw_control;
> > > + }
> > > +
> > > + id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> >
> > We have second implementation of this. Would it make sense to split to some
> > helper?
>
> Do you mean the combination of regmap_read(...CHT_WC_PWRSRC_STS,
> &pwrsrc_sts) with cht_wc_extcon_get_id()?
Yes.
> In the cht_wc_extcon_pwrsrc_event() function the pwrsrc_sts is checked
> for other bits also, so separation of PWRSRC_STS read and id calculation
> to one routine will cause non-clear function calls like as
> get_powersrc_and_check_id(..., &powersrc_sts, &id) which is not looks
> better than current code duplication.
I see. Thanks for answer.
> Or we need to spend some time for
> refactoring and testing of cht_wc_extcon_pwrsrc_event() code.
Perhaps, In any case I'm not objecting of the current approach.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe
2019-09-18 7:17 ` Andy Shevchenko
@ 2019-09-20 7:42 ` Chanwoo Choi
0 siblings, 0 replies; 9+ messages in thread
From: Chanwoo Choi @ 2019-09-20 7:42 UTC (permalink / raw)
To: Andy Shevchenko, Yauhen Kharuzhy
Cc: Andy Shevchenko, Linux Kernel Mailing List, MyungJoo Ham, Hans de Goede
Hi Andy,
On 19. 9. 18. 오후 4:17, Andy Shevchenko wrote:
> On Wed, Sep 18, 2019 at 2:04 AM Yauhen Kharuzhy <jekhor@gmail.com> wrote:
>>
>> On Tue, Sep 17, 2019 at 02:13:22PM +0300, Andy Shevchenko wrote:
>>> On Tue, Sep 17, 2019 at 12:15:36AM +0300, Yauhen Kharuzhy wrote:
>>>> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
>>>> PMIC at driver probing for further charger detection. This causes reset of
>>>> USB data sessions and removing all devices from bus. If system was
>>>> booted from Live CD or USB dongle, this makes system unusable.
>>>>
>>>> Check if USB ID pin is floating and re-route data lines in this case
>>>> only, don't touch otherwise.
>>>
>>>> + ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
>>>> + if (ret) {
>>>> + dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
>>>> + goto disable_sw_control;
>>>> + }
>>>> +
>>>> + id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
>>>
>>> We have second implementation of this. Would it make sense to split to some
>>> helper?
>>
>> Do you mean the combination of regmap_read(...CHT_WC_PWRSRC_STS,
>> &pwrsrc_sts) with cht_wc_extcon_get_id()?
>
> Yes.
>
>> In the cht_wc_extcon_pwrsrc_event() function the pwrsrc_sts is checked
>> for other bits also, so separation of PWRSRC_STS read and id calculation
>> to one routine will cause non-clear function calls like as
>> get_powersrc_and_check_id(..., &powersrc_sts, &id) which is not looks
>> better than current code duplication.
>
> I see. Thanks for answer.
>
>> Or we need to spend some time for
>> refactoring and testing of cht_wc_extcon_pwrsrc_event() code.
>
> Perhaps, In any case I'm not objecting of the current approach.
>
If you think it is OK, could you reply with your tag?
and I'll fix the multi-line comment by myself.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe
2019-09-16 21:15 ` [PATCH v2] " Yauhen Kharuzhy
2019-09-17 6:22 ` Hans de Goede
2019-09-17 11:13 ` Andy Shevchenko
@ 2019-09-20 8:25 ` Andy Shevchenko
2019-09-20 8:43 ` Chanwoo Choi
3 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2019-09-20 8:25 UTC (permalink / raw)
To: Yauhen Kharuzhy
Cc: Linux Kernel Mailing List, Chanwoo Choi, MyungJoo Ham,
Hans de Goede, Andy Shevchenko
On Tue, Sep 17, 2019 at 10:29 AM Yauhen Kharuzhy <jekhor@gmail.com> wrote:
>
> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> PMIC at driver probing for further charger detection. This causes reset of
> USB data sessions and removing all devices from bus. If system was
> booted from Live CD or USB dongle, this makes system unusable.
>
> Check if USB ID pin is floating and re-route data lines in this case
> only, don't touch otherwise.
>
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
> drivers/extcon/extcon-intel-cht-wc.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 9d32150e68db..da1886a92f75 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -338,6 +338,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
> struct cht_wc_extcon_data *ext;
> unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
> + int pwrsrc_sts, id;
> int irq, ret;
>
> irq = platform_get_irq(pdev, 0);
> @@ -387,8 +388,19 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> goto disable_sw_control;
> }
>
> - /* Route D+ and D- to PMIC for initial charger detection */
> - cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
> + ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> + if (ret) {
> + dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> + goto disable_sw_control;
> + }
> +
> + id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> +
> + /* If no USB host or device connected, route D+ and D- to PMIC for
> + * initial charger detection
> + */
> + if (id != INTEL_USB_ID_GND)
> + cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
>
> /* Get initial state */
> cht_wc_extcon_pwrsrc_event(ext);
> --
> 2.23.0.rc1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe
2019-09-16 21:15 ` [PATCH v2] " Yauhen Kharuzhy
` (2 preceding siblings ...)
2019-09-20 8:25 ` Andy Shevchenko
@ 2019-09-20 8:43 ` Chanwoo Choi
3 siblings, 0 replies; 9+ messages in thread
From: Chanwoo Choi @ 2019-09-20 8:43 UTC (permalink / raw)
To: Yauhen Kharuzhy, linux-kernel, MyungJoo Ham
Cc: Hans de Goede, Andy Shevchenko
Hi,
On 19. 9. 17. 오전 6:15, Yauhen Kharuzhy wrote:
> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> PMIC at driver probing for further charger detection. This causes reset of
> USB data sessions and removing all devices from bus. If system was
> booted from Live CD or USB dongle, this makes system unusable.
>
> Check if USB ID pin is floating and re-route data lines in this case
> only, don't touch otherwise.
>
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
> drivers/extcon/extcon-intel-cht-wc.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 9d32150e68db..da1886a92f75 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -338,6 +338,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
> struct cht_wc_extcon_data *ext;
> unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
> + int pwrsrc_sts, id;
> int irq, ret;
>
> irq = platform_get_irq(pdev, 0);
> @@ -387,8 +388,19 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> goto disable_sw_control;
> }
>
> - /* Route D+ and D- to PMIC for initial charger detection */
> - cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
> + ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> + if (ret) {
> + dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> + goto disable_sw_control;
> + }
> +
> + id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> +
> + /* If no USB host or device connected, route D+ and D- to PMIC for
> + * initial charger detection
> + */
> + if (id != INTEL_USB_ID_GND)
> + cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
>
> /* Get initial state */
> cht_wc_extcon_pwrsrc_event(ext);
>
I edit this patch as following by myself and then applied it.
diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 9d32150e68db..771f6f4cf92e 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -338,6 +338,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
struct cht_wc_extcon_data *ext;
unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
+ int pwrsrc_sts, id;
int irq, ret;
irq = platform_get_irq(pdev, 0);
@@ -387,8 +388,19 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
goto disable_sw_control;
}
- /* Route D+ and D- to PMIC for initial charger detection */
- cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
+ ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
+ if (ret) {
+ dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
+ goto disable_sw_control;
+ }
+
+ /*
+ * If no USB host or device connected, route D+ and D- to PMIC for
+ * initial charger detection
+ */
+ id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
+ if (id != INTEL_USB_ID_GND)
+ cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply related [flat|nested] 9+ messages in thread