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

Hi,

On 06/11/2015 05:59 PM, Hans de Goede wrote:

Hi,

On 11-06-15 10:29, Chanwoo Choi wrote:

quoted

Hi Hans,

On 06/11/2015 05:21 PM, Hans de Goede wrote:

quoted

Hi Chanwoo,

Thanks for the quick review.

On 11-06-15 10:07, Chanwoo Choi wrote:

quoted

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.

quoted

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