> > >>>>>> I think that the role of extcon subsystem notify changed > > >>>>>> state(attached/detached) of cable to notifiee, but if you want to > > >>>>>> add property feature of cable, you should solve ambiguous issues. > > >>>>>> > > >>>>>> First, > > >>>>>> This patch only support the properties of charger cable but, > > >>>>>> never support property of other cable. The following structure > > >>>>>> depend on only charger cable. We can check it the following > > structure: > > >>>>>> struct extcon_chrg_cbl_props { > > >>>>>> enum extcon_chrgr_cbl_stat cable_state; > > >>>>>> unsigned long mA; > > >>>>>> }; > > >>>>>> > > >>>>>> I think that the patch have to support all of cables for property > > >> feature. > > >>>>> > > >>>>> My suggestion is to have a structure like this > > >>>>> > > >>>>> struct extcon_cablel_props { > > >>>>> unsigned int cable_state; > > >>>>> unsigned int data; > > >>>> Why can't it be float/long/double?? > > >>>>> } > > >>>>> Not all cables will have more than two states. If any cable has > > >>>>> more than two states, the above structure makes it flexible to > > >>>>> represent additional state and the data associated > > >>>>> > > >>>>>> > > >>>>>> Second, > > >>>>>> Certainly, you should define common properties of all cables and > > >>>>>> specific properties of each cable. The properties of charger > > >>>>>> cable should never be defined only. > > >>>> IMHO the extcon doesn't know anything about the cable except the > > >>>> state which is currently it is in and which also is set by the > > >>>> provider.I am unable to understand why should extcon provide more > > >>>> than what it knows?It should just limit itself to broadcasting the > > >>>> cable state and exploiting it for any other information would only > > >>>> lead to > > >> more un-necessary code. > > >>>> It should be same as IIO subsystem where the consumer and provider > > >>>> knows before hand what information they are going to share and with > > >>>> what precision and IIO core is just a way to do that.It doesn't > > >>>> know anything beyond what is given by the provider. > > >>>> Same is the case with driver core where individual driver sets it's > > >>>> own private data and the driver core just gives that private data > > >>>> back to the driver as and when it needs but parsing the private > > >>>> data in the right way is up to the individual driver. > > >>>> > > >>>> I fail to understand why is not the case here. > > >>> > > >>> The requirement is different from the IIO case. I am trying to > > >>> extend the Power Supply class to support charging in a generic way > > >> (https://lkml.org/lkml/2012/10/18/219). > > >>> The extcon consumer in this case is not a device driver. It's part > > >>> of power > > >> supply class driver itself. > > >>> I am open to any solution to get the cable properties dynamically. > > >>> Do you find a better but generic mechanism for this? > > >>> > > >>> I am trying to extend extcon just because I couldnt find any other > > >>> subsystem which gives cable notifications. IMHO, it's good if we can > > >>> avoid consumer and provider driver level dependencies just by > > >>> extending extcon. This will ensure that the same driver will work on > > >>> any > > >> platform as long as it's having the dependency only on extcon. > > >>> We shouldn't put any driver level dependency between extcon > > consumer > > >> and provider. > > >>> When we do like that, the extcon consumer is expecting the similar > > >>> implementation for the provider on all platforms. This may not be > > >>> possible > > >> Is there anything wrong in assuming similar implementation for all > > >> the providers?I think the providers know what it can provide and this > > >> may vary quite a lot.Or can we make a generic provider driver which > > >> will encompass all the randomness in the various provider > > >> drivers?This generic driver will get all the properties and since it > > >> will be generice anyone can use it to know what properties to expect.This > > will keep the extcon design intact. > > > > > > Maintainer?? > > > > I agreed about opinion of anish singh. The extcon provider driver provide > > generic > > > > ---- > > struct extcon_cablel_props { > > unsigned int cable_state; > > unsigned int data; > > } > > ---- > > You suggested upper structure and said it is only flexible to represent > > additional state, But, it is non-standard. What store real data on "unsigned int > > data"? It isn't determined and flexible. That is extcon consumer driver should > > already know type of real data or value of real data. The extcon consumer > > driver has strong dependency on extcon provider driver. In this case, if > > extcon provider driver can change data value of "unsigned int data", extcon > > consumer provider have to be modified according to extcon provider driver. > > I think it isn't correct apporach. So, I proposed that we should define > > properties for all cables. > > I couldnt find properties common for all type of cables. Alternate method I can think of is > a new driver "extcon-charger.c". This driver will register with extcon subsystem. > > The consumer and provider can use the APIs and properties exposed by this driver. This > way we can ensure that extcon design is intact and the additional charger cable state and > properties can be handled by this driver. Does that make sense? Adding another API at a device driver is something to be avoided unless it's inevitable. The state/status information required by a charger should be embedded in the data structure of the charger (e.g., regulator, power-supply-class, or charger-manager). However, we may consider bridging them via extcon anyway. We may have: enum extcon_cable_type { EXTCON_CT_REGULATOR, EXTCON_CT_PSY, EXTCON_CT_CHARGER_CB, ... }; and have the following included at struct extcon_cable: union { struct regulator *reg; struct power_supply *psy; struct charger_cable *charger_cb; ... } cable data; enum extcon_cable_type cable_type; Is there any problems with this approach? If you need to embeded "current-limit" information, the regulator should be able to provide it (current-limit regulator). If you need to embed "suspend/resume" status information in general for a specific cable type, you need to define corresponding data structure related to the cable type (class) and make it connected via extcon. > > > > > >> > > >>> and does not seems to be a scalable solution. IMHO, the extcon > > >>> should provide a mechanism to retrieve the cable properties. > > >>> Consumer drivers can use this API to get the cable properties > > >>> without knowing the provider driver implementation. This will make > > >>> the extcon drivers more > > >> scalable and reusable across multiple platforms. > > >>> > > >>>>> > > >>>>> Hope above structure would be enough to represent the common > > cable > > >>>>> state and it's data. If a cable has more than two states, then > > >>>>> extcon_update_state can be used to notify the consumer 1)Provider > > >>>>> can just toggle the "state" argument to notify the consumer that > > >>>>> cable state is changed OR > > >>>>> 2) Add one more argument like extcon_update_state(struct > > >>>>> extcon_dev *edev, u32 mask, u32 state1, u32 sate2) > > >>>> This will cause other drivers to change such as arizona. > > >>>>> If the state2 is set, then consumers can use > > >>>>> get_cable_properties() to query the cable properties. State2 need > > >>>>> to be used only if the cable need to represent more than two state > > >>>>> > > >>>>>> > > >>>>>> Third, > > >>>>>> If we finish to decide the properties for all cables, I'd like to > > >>>>>> see a example > > >>>> Why do we think that state and property is the only thing which the > > >>>> consumer want to know?I am sure there would be some more > > properties > > >>>> which would be of some interest to consumers and there is quite a > > >>>> possibility that in future we might get a patch for that also.So > > >>>> instead of that limiting it to just state is a good idea. > > >>>>>> code. > > >>>>> > > >>>>> Agreed. If we agree on the above structure, I can modify the > > >>>>> charging subsystem patch to use the same structure. > > >>>>> (https://lkml.org/lkml/2012/10/18/219). This would give a > > >>>>> reference for the > > >>>> new feature. > > >>>>> > > >>>>>> > > >>>>>> You explained following changed state of USB according to Host > > >>>>>> state on other patch. > > >>>>>> --------------------------- > > >>>>>> For USB2.0 > > >>>>>> 1) CONNECT (100mA)->UPDATE(500mA)->DISCONNECT(0mA) > > >>>>>> 2) CONNECT (100mA)->UPDATE(500mA)->HOST > > >>>> SUSPEND(2.5mA/500mA)- > > >>>>>>> DISCONNECT(0mA) > > >>>>>> 3) CONNECT (100mA)->UPDATE(500mA)->HOST > > >>>> SUSPEND(2.5mA/500mA)- > > >>>>>>> HOST RESUME(500mA)->DISCONNECT(0mA) > > >>>>>> > > >>>>>> For USB 3.0 > > >>>>>> 4) CONNECT (150mA)->UPDATE(900mA)->DISCONNECT(0mA) > > >>>>>> 5) CONNECT (150mA)->UPDATE(900mA)-> HOST > > >>>> SUSPEND(2.5mA/900mA)- > > >>>>>>> DISCONNECT(0mA) > > >>>>>> 6) CONNECT (100mA)->UPDATE(900mA)->HOST > > >>>> SUSPEND(2.5mA/900mA)- > > >>>>>>> HOST RESUME(900mA)->DISCONNECT(0mA) > > >>>>>> --------------------------- > > >>>>>> > > >>>>>> I have a question. Can the provider device driver(e.g., > > >>>>>> extcon-max77693.c/ > > >>>>>> extcon-max8997.c) detect the changed state of host? I'd like to > > >>>>>> see the example device driver that the provider device driver > > >>>>>> detect changed state of host. > > >>>>>> Could you provide example device driver? > > >>>>> > > >>>>> Good question. The OTG drivers are capable of identifying the > > >>>>> SUSPEND > > >>>> event. > > >>>>> System cannot setup SDP (USB host) charging with maximum charging > > >>>>> current - 500mA (USB2.0/ 900mA(USB 3)) without enumerating the > > USB. > > >>>>> The USB enumeration can be done only with a USB/OTG driver. IMHO > > >>>>> the above extcon drivers (extcon-max77693.c/ extcon-max8997.c) are > > >>>>> not capable of doing the USB enumeration and identifying the > > >>>>> charge current. They can just identify the charger type - > > >> SDP/DCP/CDP/ACA/AC. > > >>>>> The intelligence for USB enumeration should be inside USB/OTG > > driver. > > > > > > > > > > {.n++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I