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

From: Hans de Goede <hdegoede@redhat.com>
To: Chanwoo Choi <cwchoi00@gmail.com>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Mark Gross <markgross@kernel.org>,
	Andy Shevchenko <andy@infradead.org>,
	Wolfram Sang <wsa@the-dreams.de>,
	Sebastian Reichel <sre@kernel.org>,
	MyungJoo Ham <myungjoo.ham@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Ard Biesheuvel <ardb@kernel.org>
Cc: Len Brown <lenb@kernel.org>,
	linux-acpi@vger.kernel.org, Yauhen Kharuzhy <jekhor@gmail.com>,
	Tsuchiya Yuto <kitakar@gmail.com>,
	platform-driver-x86@vger.kernel.org, linux-i2c@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-efi@vger.kernel.org
Subject: Re: [PATCH v2 18/20] extcon: intel-cht-wc: Refactor cht_wc_extcon_get_charger()
Date: Wed, 17 Nov 2021 23:30:56 +0100	[thread overview]
Message-ID: <662623cd-c70b-63e6-499e-7c24b5d3e342@redhat.com> (raw)
In-Reply-To: <f84e2060-f6b7-64f9-78cd-e8ad8776ab2d@gmail.com>

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;
>>
> 
> 