Subject: Re: [PATCH 1/1] extcon: deduplicate code in extcon_set_state_sync()
To:     Alexander Stein <alexander.stein@ew.tq-group.com>,
        MyungJoo Ham <myungjoo.ham@samsung.com>
Cc:     linux-kernel@vger.kernel.org
From:   Chanwoo Choi <cw00.choi@samsung.com>
Organization: Samsung Electronics
Message-ID: <09ea6f93-4aae-1f74-bb54-0453908a7102@samsung.com>
Date:   Thu, 16 Dec 2021 12:05:18 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101
        Thunderbird/59.0
MIME-Version: 1.0
In-Reply-To: <20211123145301.778629-1-alexander.stein@ew.tq-group.com>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <CGME20211123145318epcas1p17f6567ed89ec7f8f3ab3c007c58b1fe0@epcas1p1.samsung.com>
        <20211123145301.778629-1-alexander.stein@ew.tq-group.com>
Precedence: bulk

On 11/23/21 11:53 PM, Alexander Stein wrote:
> Finding the cable index and checking for changed status is also done
> in extcon_set_state(). So calling extcon_set_state_sync() will do these
> checks twice. Remove them and use these checks from extcon_set_state().
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> I noticed this duplicated code while debugging an extcon related issue.
> I do not know if it is allowed in the kernel to assume some behavior in
> EXPORT_SYMBOL* functions, but as the two functions mentioned are in the
> same source file it should be ok.
> In the case it is not okay to assume some behaviour,
> extcon_set_state_sync() is missing a check for !edev, like
> extcon_set_state() does.
> 
>  drivers/extcon/extcon.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index e7a9561a826d..a09e704fd0fa 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -576,19 +576,7 @@ EXPORT_SYMBOL_GPL(extcon_set_state);
>   */
>  int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id, bool state)
>  {
> -	int ret, index;
> -	unsigned long flags;
> -
> -	index = find_cable_index_by_id(edev, id);
> -	if (index < 0)
> -		return index;
> -
> -	/* Check whether the external connector's state is changed. */
> -	spin_lock_irqsave(&edev->lock, flags);
> -	ret = is_extcon_changed(edev, index, state);
> -	spin_unlock_irqrestore(&edev->lock, flags);
> -	if (!ret)
> -		return 0;
> +	int ret;
>  
>  	ret = extcon_set_state(edev, id, state);
>  	if (ret < 0)
> 

Applied it. Thanks.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics