From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752464AbcLSG1u (ORCPT ); Mon, 19 Dec 2016 01:27:50 -0500 Received: from mailout3.samsung.com ([203.254.224.33]:47476 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750793AbcLSG1t (ORCPT ); Mon, 19 Dec 2016 01:27:49 -0500 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 X-AuditID: cbfee61a-f79bd6d000000fc6-29-58577de34a18 Content-transfer-encoding: 8BIT Message-id: <58577DE3.7070606@samsung.com> Date: Mon, 19 Dec 2016 15:27:47 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Hans de Goede , MyungJoo Ham Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH 4/8] extcon: axp288: Fix possibly reporting 2 cables in state true References: <20161219001313.13402-1-hdegoede@redhat.com> <20161219001313.13402-4-hdegoede@redhat.com> In-reply-to: <20161219001313.13402-4-hdegoede@redhat.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprPIsWRmVeSWpSXmKPExsVy+t9jAd3HteERBq2fhS3eHJ/OZHF51xw2 i9uNK9gcmD3e77vK5tG3ZRWjx+dNcgHMUW42GamJKalFCql5yfkpmXnptkqhIW66FkoKeYm5 qbZKEbq+IUFKCmWJOaVAnpEBGnBwDnAPVtK3S3DLmHZ4FntBs0jFhnXcDYyLBboYOTkkBEwk XnY9ZYOwxSQu3FsPZHNxCAksZZRYvuoaO0iCV0BQ4sfkeyxdjBwczALyEkcuZYOEmQXUJSbN W8QMYgsJPGCU+NnnDVGuJXHxwDywmSwCqhJrG++C1bABxfe/uAEW5xdQlLj64zEjyEhRgQiJ 7hOVIGERgQCJn6f62SHGK0j8ureJFcQWBirpObaPGeK03YwSl5adAktwClhKnD29jnECo+As JJfOQrh0FpJLFzAyr2KUSC1ILihOSs81zEst1ytOzC0uzUvXS87P3cQIjpxnUjsYD+5yP8Qo wMGoxMNb8D4sQog1say4MvcQowQHs5IIr0h5eIQQb0piZVVqUX58UWlOavEhRlOgXycyS4km 5wOjOq8k3tDE3MTc2MDC3NLSxEhJnLdx9rNwIYH0xJLU7NTUgtQimD4mDk6pBsaNa1qkuH7t n3hszUK784Hr5/9ydv5Xevh8RfHEBws+Zxzrzn6hKaUhJCUo/XH5qqNzV5/+zfh+6YqeiM/M ry6fEyjJj7kQWCE1P//e9OA1Uuvcjjko5fEk9951rb2TE/Fw5oIt9Tn1CfzGb46tWrjJdtMd i5xCnrQuL2ELFq0mldc5z8/PSniqxFKckWioxVxUnAgA2zcSVrICAAA= X-MTR: 20000000000000000@CPGS Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Hans, This patch looks good to me. But I have one comment when setting the previous_cable in probe(). On 2016년 12월 19일 09:13, Hans de Goede wrote: > When the charger type changes from e.g. SDP to CDP, without Vbus being > seen as low in between axp288_handle_chrg_det_event would set the state > for the new cable type to true, without clearing the state of the > previous cable type to false. > > This commit fixes this and also gets rid of the function local static > cable variable, properly storing all drv state in the axp288_extcon_info > struct. > > Signed-off-by: Hans de Goede > --- > drivers/extcon/extcon-axp288.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > index 43b3637..ded0bd9 100644 > --- a/drivers/extcon/extcon-axp288.c > +++ b/drivers/extcon/extcon-axp288.c > @@ -115,6 +115,7 @@ struct axp288_extcon_info { > int irq[EXTCON_IRQ_END]; > struct extcon_dev *edev; > struct notifier_block extcon_nb; > + unsigned int previous_cable; > }; > > /* Power up/down reason string array */ > @@ -154,9 +155,9 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info) > > static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) > { > - static unsigned int cable; > int ret, stat, cfg, pwr_stat; > u8 chrg_type; > + unsigned int cable = info->previous_cable; > bool vbus_attach = false; > > ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, &pwr_stat); > @@ -212,7 +213,11 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info) > vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC > : EXTCON_GPIO_MUX_SEL_PMIC); > > - extcon_set_state_sync(info->edev, cable, vbus_attach); > + extcon_set_state_sync(info->edev, info->previous_cable, false); > + if (vbus_attach) { > + extcon_set_state_sync(info->edev, cable, vbus_attach); > + info->previous_cable = cable; > + } > > return 0; > > @@ -263,6 +268,7 @@ static int axp288_extcon_probe(struct platform_device *pdev) > info->dev = &pdev->dev; > info->regmap = axp20x->regmap; > info->regmap_irqc = axp20x->regmap_irqc; > + info->previous_cable = axp288_extcon_cables[0]; I think that you better to use "EXTCON_NONE" instead of "axp288_extcon_cables[0]" as following: info->previous_cable = EXTCON_NONE; > if (pdata) > info->gpio_mux_cntl = pdata->gpio_mux_cntl; > > -- Regards, Chanwoo Choi