linux-kernel - Re: [PATCH v2 1/2] usb: phy: Introduce one extcon device into usb phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Mar 2017 11:00:35 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     NeilBrown <neilb@...e.com>
Cc:     Felipe Balbi <balbi@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        USB <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linaro Kernel Mailman List <linaro-kernel@...ts.linaro.org>,
        Jun Li <jun.li@....com>, Peter Chen <peter.chen@...escale.com>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v2 1/2] usb: phy: Introduce one extcon device into usb phy

Hi,

On 29 March 2017 at 06:56, NeilBrown <neilb@...e.com> wrote:
> On Thu, Mar 23 2017, Baolin Wang wrote:
>
>> Usually usb phy need register one extcon device to get the connection
>> notifications. It will remove some duplicate code if the extcon device
>> is registered using common code instead of each phy driver having its
>> own related extcon APIs. So we add one pointer of extcon device into
>> usb phy structure, and some other helper functions to register extcon.
>>
>> Suggested-by: NeilBrown <neilb@...e.com>
>> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
>> ---
>> Changes since v1:
>>  - Fix build errors with random config.
>> ---
>>  drivers/usb/phy/Kconfig |    6 +++---
>>  drivers/usb/phy/phy.c   |   44 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/usb/phy.h |    6 ++++++
>>  3 files changed, 53 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
>> index 61cef75..c9c61a8 100644
>> --- a/drivers/usb/phy/Kconfig
>> +++ b/drivers/usb/phy/Kconfig
>> @@ -4,6 +4,7 @@
>>  menu "USB Physical Layer drivers"
>>
>>  config USB_PHY
>> +     select EXTCON
>>       def_bool n
>>
>>  #
>> @@ -116,7 +117,7 @@ config OMAP_OTG
>>
>>  config TAHVO_USB
>>       tristate "Tahvo USB transceiver driver"
>> -     depends on MFD_RETU && EXTCON
>> +     depends on MFD_RETU
>>       depends on USB_GADGET || !USB_GADGET # if USB_GADGET=m, this can't be 'y'
>>       select USB_PHY
>>       help
>> @@ -148,7 +149,6 @@ config USB_MSM_OTG
>>       depends on (USB || USB_GADGET) && (ARCH_QCOM || COMPILE_TEST)
>>       depends on USB_GADGET || !USB_GADGET # if USB_GADGET=m, this can't be 'y'
>>       depends on RESET_CONTROLLER
>> -     depends on EXTCON
>>       select USB_PHY
>>       help
>>         Enable this to support the USB OTG transceiver on Qualcomm chips. It
>> @@ -162,7 +162,7 @@ config USB_MSM_OTG
>>  config USB_QCOM_8X16_PHY
>>       tristate "Qualcomm APQ8016/MSM8916 on-chip USB PHY controller support"
>>       depends on ARCH_QCOM || COMPILE_TEST
>> -     depends on RESET_CONTROLLER && EXTCON
>> +     depends on RESET_CONTROLLER
>>       select USB_PHY
>>       select USB_ULPI_VIEWPORT
>>       help
>> diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
>> index 98f75d2..baa8b18 100644
>> --- a/drivers/usb/phy/phy.c
>> +++ b/drivers/usb/phy/phy.c
>> @@ -100,6 +100,41 @@ static int devm_usb_phy_match(struct device *dev, void *res, void *match_data)
>>       return *phy == match_data;
>>  }
>>
>> +static int usb_add_extcon(struct usb_phy *x)
>> +{
>> +     int ret;
>> +
>> +     if (of_property_read_bool(x->dev->of_node, "extcon")) {
>> +             x->edev = extcon_get_edev_by_phandle(x->dev, 0);
>
> This is not what I meant to suggest.
> This provides an extcon only for some phy devices.  I meant that every
> single usb_phy should always have an extcon.
> So phy.c should probably call extcon_dev_allocate() and
> extcon_dev_register() - or similar.

That means every usb phy acts as one extcon device, but I think usb
phy device is not one extcon device, ID/VBUS GPIOs are really extcon
devices, which means usb phy extcon device is duplicate, right?

>
> The driver then calls extcon_set_state() or similar on this extcon
> device in whatever way might be appropriate.
> For a phy driver like phy-qcom-8x16-usb it might just pass on
> events from some separate extcon device.
>
> For the (hypothetical?) device that Felipe suggests with separate
> ID and VBUS extcons, it would pass on events from multiple different
> extcons.  i.e. it would act like a multiplexer.
>
> Other drivers would do things from interrupt handlers, based on values
> read from registers.
>
> The important point is that each usb_phy doesn't get a pointer to some
> separate extcon.  Rather it has an extcon that it completely owns.  The
> name of the extcon is the same of the phy.  The extcon should be seen as
> a part of the phy, not a separate thing which provides service to the
> phy.
>
> Thanks,
> NeilBrown



-- 
Baolin.wang
Best Regards