Message-ID: <f02aa588-8e6b-64dc-6f98-ef1c6edc7941@redhat.com>
Date:   Sun, 28 Nov 2021 15:17:38 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v2 18/20] extcon: intel-cht-wc: Refactor
 cht_wc_extcon_get_charger()
Content-Language: en-US
To:     cwchoi00@gmail.com
Cc:     "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>, 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 list <linux-pm@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-efi@vger.kernel.org
References: <20211114170335.66994-1-hdegoede@redhat.com>
 <20211114170335.66994-19-hdegoede@redhat.com>
 <f84e2060-f6b7-64f9-78cd-e8ad8776ab2d@gmail.com>
 <662623cd-c70b-63e6-499e-7c24b5d3e342@redhat.com>
 <CAGTfZH1ndMc902R+wJXM+q+4fSJQD+RZVxaWcMvut4+9oSzqnw@mail.gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <CAGTfZH1ndMc902R+wJXM+q+4fSJQD+RZVxaWcMvut4+9oSzqnw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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