Message-ID: <20ADAB092842284E95860F279283C56444C915@BGSMSX101.gar.corp.intel.com>
Date:	Tue, 20 Nov 2012 09:14:41 +0000
From:	"Tc, Jenny" <jenny.tc@...el.com>
To:	Chanwoo Choi <cw00.choi@...sung.com>
CC:	anish singh <anish198519851985@...il.com>,
	"myungjoo.ham@...sung.com" <myungjoo.ham@...sung.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"myunjoo.ham@...il.com" <myunjoo.ham@...il.com>,
	"lockwood@...roid.com" <lockwood@...roid.com>,
	"peterhuewe@....de" <peterhuewe@....de>,
	"broonie@...nsource.wolfsonmicro.com" 
	<broonie@...nsource.wolfsonmicro.com>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"lars@...afoo.de" <lars@...afoo.de>,
	"jic23@...nel.org" <jic23@...nel.org>,
	"Anton Vorontsov (cbouatmailru@...il.com)" <cbouatmailru@...il.com>,
	"Anton Vorontsov (anton.vorontsov@...aro.org)" 
	<anton.vorontsov@...aro.org>,
	"Pallala, Ramakrishna" <ramakrishna.pallala@...el.com>
Subject: RE: [PATCH] extcon : callback function to read cable property

> >>>>>>>> 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 couldn’t 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 couldn’t 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?
> 
> I don't agree about 'extcon-charger.c' because the 'extcon-class.c' driver
> support already charger cable.
> Also it is duplicate feature between 'extcon-class.c' and 'extcon-charger.c'.
> 
> Also,
> The obvious difference between the state of external cable and the state of
> host system is existence.
> I think that extcon cable state isn't changed when state of host system move
> from normal state to suspend state or opposite case. If consumer driver
> hope to know new state of host system, power_supply charging framework
> communicate with other driver or framework that provide API for checking
> state of host system.
> 
> I suggest a approach that some framework related OTG/USB need to
> implement something like below notifier block, this notifier block inform
> changed state of host system of some driver/framework that need state of
> host system.
> 
> /*
>   * Host system notifier
>   */
> #define HOST_SYSTEM_NORMAL		0x1
> #define HOST_SYSTEM_SUSPEND	0x2
> 
> extern int register_host_system_notifier(struct notifier_block *nb); extern
> void unregister_host_system_notifier(struct notifier_block *nb); int
> host_system_notify(..)
> 
> For example,
> Firstly, the power_supply charging framework check state of charger cable
> whether attached or detached cable. Second, when power_supply charging
> framework receive the changed state of host system from 'Host system
> notifier', change charging current of charger cable.

Not just SUSPEND, but we need to handle RESUME , UPDATE etc.  Also this doesn’t help us to
define a standard interface for the charger cable state/properties. IMHO it's not right
to provide different interfaces for different cables. This doesn't help us to standardize
the charger cable interface.

This thread has been running for a quite long time. Unfortunately we couldn't make an
agreement on the final solution. I would like to recap the overall requirement and
would like to propose alternate solutions. The requirements is  to
"Provide a generic interface for charger cable states and charger cable properties"

Even though extcon subsystem handles charger cable states, it's not enough to handle
all kind of charger cable states. It can handle just 2 states CONNECT/DISCONNECT.
But there are scenarios where we need to handle more than 2 states
(eg. USB SUSPEND/RESUME/UPDATE etc). Also extcon doesn't have any mechanism to
read cable properties in a generic way.  Extcon charger-cable consumer driver
implementations (eg charger-manager), defines charger cable properties statically (current in mA)
inside the consumer driver. This is not enough, since the charger cable properties may change dynamically

In existing form extcon cannot support different charger cable states and their 
properties in a generic way. Also we couldn't find a final solution on how to modify the
extcon to support these requirements. From the whole discussion what I conclude is
 * extcon is not designed to support cable properties
 * extcon is not designed to support any cable state other than CONNECT/DISCONNECT

(Feel free to correct me if my conclusion is wrong)


Considering these facts and the above discussion, what I feel is extcon subsystem cannot provide generic
interface to handle multiple charger cable states and their properties

Based on this, I would like to propose a generic interface for the charger cable notification which
can handle multiple cable states and can provide charger cable properties in a generic manner.
Since the charger cables are related to power supply subsystem, my suggestion is to have a notifier
chain inside the power_supply subsystem  and have APIs to query the charger cable properties.
This will help to standardize the charger cable handling.

Both extcon and power_supply maintainers are CCed in this thread. Please help me to identify a solution.


> 
> >
> >>
> >>>>
> >>>>> 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.
> >>
> >

Powered by blists - more mailing lists