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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
