[linux-sunxi] Re: [PATCH v4 1/2] phy-sun4i-usb: Add full support for usb0 phy / OTG

[Chanwoo Choi]

Hi,

On 06/11/2015 05:59 PM, Hans de Goede wrote:
> Hi,
> 
> On 11-06-15 10:29, Chanwoo Choi wrote:
>> Hi Hans,
>>
>> On 06/11/2015 05:21 PM, Hans de Goede wrote:
>>> Hi Chanwoo,
>>>
>>> Thanks for the quick review.
>>>
>>> On 11-06-15 10:07, Chanwoo Choi wrote:
>>>> Hi Hans,
>>>>
>>>> I add the comment about extcon-related code.
>>>>
>>>> Firstly,
>>>> I'd like you to implment the extcon driver for phy-sun4i-usb device
>>>> in drivers/extcon/ directoryby using MFD
>>>
>>> No, just no, this is not what the MFD framework is for, the usb-phy
>>> in question here is not a multifunction device. The MFD framework
>>> is intended for true multi-function devices like i2c attached
>>> PMICs which have regulators, gpios, pwm, input (power button),
>>> chargers, power-supply, etc. That is NOT the case here.
>>>
>>> Also moving this to the MFD framework would very likely requiring
>>> the devicetree binding for the usb-phy to change which we cannot
>>> do as that would break the devicetree ABI.
>>>
>>>> because there are both extcon
>>>> provider driver and extcon client driver. I think that all extcon
>>>> provider driver better to be included in drivers/extcon/ directory.
>>>> extcon_set_cable_state() function should be handled in extcon provider
>>>> driver which is incluced in drivers/extcon/ directory.
>>>
>>> I do not find this a compelling reason, there are plenty of subsystems
>>> where not all implementations of the subsystem class live in the subsystem
>>> directory, e.g. input and hwmon devices are often also found outside of
>>> the input and hwmon driver directories.
>>
>> There are difference on between input/hwmon and extcon.
>>
>> Because input and hwmon driver implement the only one type driver as provider driver.
>> But, extcon implement the two type driver of both extcon provider and extcon client driver.
>> The extcon is similiar with regulator and clock framework as resource.
>>
>> extcon provider driver to provider the event when the state of external connector is changed.
>> - devm_extcon_dev_register()
>> - e.g., almost extcon provider driver are included in 'drivers/extcon/' directory.
> 
> I understand, but that does not change my first argument, that the usb-phy is not
> a MFD device. And although it may be desirable to keep extcon provider drivers
> in the drivers/extcon, there are no technical reasons to do so.
> 
> The whole reason why Kishon asked me to start using the extcon framework is to avoid
> adding a private API to the phy-sun4i-usb code for notifying the musb-sunxi code
> about otg-id-pin status changes. Adding a separate driver for just the extcon bits
> means re-adding a private api to the phy-sun4i-usb code but this time for the
> extcon code, at which point we might just as well skip extcon and have the
> musb-sunxi glue code call directly into the phy-sun4i-usb code...
> 
> Needing a private API for a separate extcn driver actually is a good argument to
> NOT have a separate extcon driver and keep the extcon code in the phy-sun4i-usb code,
> where as I see no technical arguments in favor of a separate extcon driver.

There is one technical issue.

The extcon_set_cable_state() should be handled by extcon provider driver.
because extcon_set_cable_state() inform the extcon client driver of the event
when detecting the change of h/w line (gpio line) or register of peripheral device.

But, extcon client driver can now get the instance of extcon_dev structure
by extcon_get_edev_by_phandle() and then can change the cable state by using the extcon_set_cable_state().

I think that these issue have to be protected by framework level.

Thanks,
Chanwoo Choi