diff mbox series

[v2,18/20] extcon: intel-cht-wc: Refactor cht_wc_extcon_get_charger()

Message ID 20211114170335.66994-19-hdegoede@redhat.com (mailing list archive)
State Not Applicable, archived
Headers show
Series power-suppy/i2c/extcon: Fix charger setup on Xiaomi Mi Pad 2 and Lenovo Yogabook | expand

Commit Message

Hans de Goede Nov. 14, 2021, 5:03 p.m. UTC
Refactor cht_wc_extcon_get_charger() to have all the returns are in
the "switch (usbsrc)" cases.

This is a preparation patch for adding support for registering
a power_supply class device.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Chanwoo Choi Nov. 17, 2021, 7:15 a.m. UTC | #1
Hello,

I think that you need to squash it with patch21
I'm not sure that this patch is either atomic or not because
you remove the 'return EXTCON_CHG_USB_SDP/EXTCON_CHG_USB_SDP'
without explaining why it is no problem. Just mention that
pass the role to next 'switch' cases. But, before this change,
there were any reason to return the type of charger cable
before switch statement.

According to your patch description, you don't need
to make the separate patch of it. Please squash it with patch21.

On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
> Refactor cht_wc_extcon_get_charger() to have all the returns are in
> the "switch (usbsrc)" cases.
> 
> This is a preparation patch for adding support for registering
> a power_supply class device.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 119b83793123..f2b93a99a625 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -153,14 +153,15 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
>   	} while (time_before(jiffies, timeout));
>   
>   	if (status != CHT_WC_USBSRC_STS_SUCCESS) {
> -		if (ignore_errors)
> -			return EXTCON_CHG_USB_SDP; /* Save fallback */
> +		if (!ignore_errors) {
> +			if (status == CHT_WC_USBSRC_STS_FAIL)
> +				dev_warn(ext->dev, "Could not detect charger type\n");
> +			else
> +				dev_warn(ext->dev, "Timeout detecting charger type\n");
> +		}
>   
> -		if (status == CHT_WC_USBSRC_STS_FAIL)
> -			dev_warn(ext->dev, "Could not detect charger type\n");
> -		else
> -			dev_warn(ext->dev, "Timeout detecting charger type\n");
> -		return EXTCON_CHG_USB_SDP; /* Save fallback */
> +		/* Save fallback */
> +		usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
>   	}
>   
>   	usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
>
Hans de Goede Nov. 17, 2021, 10:30 p.m. UTC | #2
Hi,

On 11/17/21 08:15, Chanwoo Choi wrote:
> Hello,
> 
> I think that you need to squash it with patch21
> I'm not sure that this patch is either atomic or not because
> you remove the 'return EXTCON_CHG_USB_SDP/EXTCON_CHG_USB_SDP'
> without explaining why it is no problem. Just mention that
> pass the role to next 'switch' cases. But, before this change,
> there were any reason to return the type of charger cable
> before switch statement.

The setting of usbsrc to "CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT"
will make the following switch-case return EXTCON_CHG_USB_SDP
just as before, so there is no functional change.

> According to your patch description, you don't need
> to make the separate patch of it. Please squash it with patch21.

Having this refactoring in a separate patch makes it easier
to see what is going on in patch 21. So I'm going to keep this
as a separate patch for v3 of this series.

Thanks & Regards,

Hans



> On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
>> Refactor cht_wc_extcon_get_charger() to have all the returns are in
>> the "switch (usbsrc)" cases.
>>
>> This is a preparation patch for adding support for registering
>> a power_supply class device.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
>> index 119b83793123..f2b93a99a625 100644
>> --- a/drivers/extcon/extcon-intel-cht-wc.c
>> +++ b/drivers/extcon/extcon-intel-cht-wc.c
>> @@ -153,14 +153,15 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
>>       } while (time_before(jiffies, timeout));
>>         if (status != CHT_WC_USBSRC_STS_SUCCESS) {
>> -        if (ignore_errors)
>> -            return EXTCON_CHG_USB_SDP; /* Save fallback */
>> +        if (!ignore_errors) {
>> +            if (status == CHT_WC_USBSRC_STS_FAIL)
>> +                dev_warn(ext->dev, "Could not detect charger type\n");
>> +            else
>> +                dev_warn(ext->dev, "Timeout detecting charger type\n");
>> +        }
>>   -        if (status == CHT_WC_USBSRC_STS_FAIL)
>> -            dev_warn(ext->dev, "Could not detect charger type\n");
>> -        else
>> -            dev_warn(ext->dev, "Timeout detecting charger type\n");
>> -        return EXTCON_CHG_USB_SDP; /* Save fallback */
>> +        /* Save fallback */
>> +        usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
>>       }
>>         usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
>>
> 
>
Chanwoo Choi Nov. 19, 2021, 3:44 p.m. UTC | #3
Hi,

On Thu, Nov 18, 2021 at 7:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/17/21 08:15, Chanwoo Choi wrote:
> > Hello,
> >
> > I think that you need to squash it with patch21
> > I'm not sure that this patch is either atomic or not because
> > you remove the 'return EXTCON_CHG_USB_SDP/EXTCON_CHG_USB_SDP'
> > without explaining why it is no problem. Just mention that
> > pass the role to next 'switch' cases. But, before this change,
> > there were any reason to return the type of charger cable
> > before switch statement.
>
> The setting of usbsrc to "CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT"
> will make the following switch-case return EXTCON_CHG_USB_SDP
> just as before, so there is no functional change.
>
> > According to your patch description, you don't need
> > to make the separate patch of it. Please squash it with patch21.
>
> Having this refactoring in a separate patch makes it easier
> to see what is going on in patch 21. So I'm going to keep this
> as a separate patch for v3 of this series.

If you want to keep this  patch, please remove the following description.
Instead, just mention to focus on refactor it without behavior changes.

'This is a preparation patch for adding support for registering
a power_supply class device.'

>
>
> > On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
> >> Refactor cht_wc_extcon_get_charger() to have all the returns are in
> >> the "switch (usbsrc)" cases.
> >>
> >> This is a preparation patch for adding support for registering
> >> a power_supply class device.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
> >>   1 file changed, 8 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> >> index 119b83793123..f2b93a99a625 100644
> >> --- a/drivers/extcon/extcon-intel-cht-wc.c
> >> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> >> @@ -153,14 +153,15 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
> >>       } while (time_before(jiffies, timeout));
> >>         if (status != CHT_WC_USBSRC_STS_SUCCESS) {
> >> -        if (ignore_errors)
> >> -            return EXTCON_CHG_USB_SDP; /* Save fallback */
> >> +        if (!ignore_errors) {
> >> +            if (status == CHT_WC_USBSRC_STS_FAIL)
> >> +                dev_warn(ext->dev, "Could not detect charger type\n");
> >> +            else
> >> +                dev_warn(ext->dev, "Timeout detecting charger type\n");
> >> +        }
> >>   -        if (status == CHT_WC_USBSRC_STS_FAIL)
> >> -            dev_warn(ext->dev, "Could not detect charger type\n");
> >> -        else
> >> -            dev_warn(ext->dev, "Timeout detecting charger type\n");
> >> -        return EXTCON_CHG_USB_SDP; /* Save fallback */
> >> +        /* Save fallback */
> >> +        usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
> >>       }
> >>         usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
> >>
> >
> >
>
Hans de Goede Nov. 28, 2021, 2:17 p.m. UTC | #4
Hi,

On 11/19/21 16:44, Chanwoo Choi wrote:
> Hi,
> 
> On Thu, Nov 18, 2021 at 7:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11/17/21 08:15, Chanwoo Choi wrote:
>>> Hello,
>>>
>>> I think that you need to squash it with patch21
>>> I'm not sure that this patch is either atomic or not because
>>> you remove the 'return EXTCON_CHG_USB_SDP/EXTCON_CHG_USB_SDP'
>>> without explaining why it is no problem. Just mention that
>>> pass the role to next 'switch' cases. But, before this change,
>>> there were any reason to return the type of charger cable
>>> before switch statement.
>>
>> The setting of usbsrc to "CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT"
>> will make the following switch-case return EXTCON_CHG_USB_SDP
>> just as before, so there is no functional change.
>>
>>> According to your patch description, you don't need
>>> to make the separate patch of it. Please squash it with patch21.
>>
>> Having this refactoring in a separate patch makes it easier
>> to see what is going on in patch 21. So I'm going to keep this
>> as a separate patch for v3 of this series.
> 
> If you want to keep this  patch, please remove the following description.
> Instead, just mention to focus on refactor it without behavior changes.
> 
> 'This is a preparation patch for adding support for registering
> a power_supply class device.'

Ok, done for v3 of the patch-series.

Regards,

Hans



>>> On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
>>>> Refactor cht_wc_extcon_get_charger() to have all the returns are in
>>>> the "switch (usbsrc)" cases.
>>>>
>>>> This is a preparation patch for adding support for registering
>>>> a power_supply class device.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>   drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
>>>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
>>>> index 119b83793123..f2b93a99a625 100644
>>>> --- a/drivers/extcon/extcon-intel-cht-wc.c
>>>> +++ b/drivers/extcon/extcon-intel-cht-wc.c
>>>> @@ -153,14 +153,15 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
>>>>       } while (time_before(jiffies, timeout));
>>>>         if (status != CHT_WC_USBSRC_STS_SUCCESS) {
>>>> -        if (ignore_errors)
>>>> -            return EXTCON_CHG_USB_SDP; /* Save fallback */
>>>> +        if (!ignore_errors) {
>>>> +            if (status == CHT_WC_USBSRC_STS_FAIL)
>>>> +                dev_warn(ext->dev, "Could not detect charger type\n");
>>>> +            else
>>>> +                dev_warn(ext->dev, "Timeout detecting charger type\n");
>>>> +        }
>>>>   -        if (status == CHT_WC_USBSRC_STS_FAIL)
>>>> -            dev_warn(ext->dev, "Could not detect charger type\n");
>>>> -        else
>>>> -            dev_warn(ext->dev, "Timeout detecting charger type\n");
>>>> -        return EXTCON_CHG_USB_SDP; /* Save fallback */
>>>> +        /* Save fallback */
>>>> +        usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
>>>>       }
>>>>         usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
>>>>
>>>
>>>
>>
> 
>
diff mbox series

Patch

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 119b83793123..f2b93a99a625 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -153,14 +153,15 @@  static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
 	} while (time_before(jiffies, timeout));
 
 	if (status != CHT_WC_USBSRC_STS_SUCCESS) {
-		if (ignore_errors)
-			return EXTCON_CHG_USB_SDP; /* Save fallback */
+		if (!ignore_errors) {
+			if (status == CHT_WC_USBSRC_STS_FAIL)
+				dev_warn(ext->dev, "Could not detect charger type\n");
+			else
+				dev_warn(ext->dev, "Timeout detecting charger type\n");
+		}
 
-		if (status == CHT_WC_USBSRC_STS_FAIL)
-			dev_warn(ext->dev, "Could not detect charger type\n");
-		else
-			dev_warn(ext->dev, "Timeout detecting charger type\n");
-		return EXTCON_CHG_USB_SDP; /* Save fallback */
+		/* Save fallback */
+		usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
 	}
 
 	usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;