Message-ID: <20111216072051.6014557b@notabene.brown>
Date:	Fri, 16 Dec 2011 07:20:51 +1100
From:	NeilBrown <neilb@...e.de>
To:	myungjoo.ham@...il.com
Cc:	myungjoo.ham@...sung.com, linux-kernel@...r.kernel.org,
	Randy Dunlap <rdunlap@...otime.net>,
	Mike Lockwood <lockwood@...roid.com>,
	Arve Hjønnevåg <arve@...roid.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Donggeun Kim <dg77.kim@...sung.com>, Greg KH <gregkh@...e.de>,
	Arnd Bergmann <arnd@...db.de>,
	Linus Walleij <linus.walleij@...aro.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Morten CHRISTIANSEN <morten.christiansen@...ricsson.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Subject: Re: [PATCH v2 0/3] introduce External Connector Class (extcon)

On Thu, 15 Dec 2011 15:36:15 +0900 MyungJoo Ham <myungjoo.ham@...sung.com>
wrote:

> On Thu, Dec 15, 2011 at 11:32 AM, NeilBrown <neilb@...e.de> wrote:
> > On Wed, 14 Dec 2011 19:28:25 +0900 MyungJoo Ham <myungjoo.ham@...sung.com>
> > wrote:
> >
> >> Note that previously, the patchset has been submitted as
> >> "Multistate Switch Class".
> >>
> >> For external connectors, which may have different types of cables attached
> >> (USB, TA, HDMI, Analog A/V, and others), we often have seperated device
> >> drivers that detect the state changes at the port and device drivers that
> >> do something according to the state changes.
> >>
> >> For example, when MAX8997-MUIC detects a Charger cable insertion, another
> >> device driver (such as MAX8903 charger, MAX8997 charger, Charger Manager,
> >> or board file) needs to set charger current limit accordingly and when
> >> MAX8997-MUIC detects a HDMI cable insertion, multimedia device drivers
> >> need to do some operations accordingly.
> >
> > Hi,
> >  a few comments...
> >
> > 1/ I still don't understand why this needs to a single device which can
> >   report multiple cables, rather than simply having multiple devices each
> >   of which reports a single cable at a time.
> >
> >   i.e. A physical 32 bit port might be represented as several 'extcon'
> >   devices each of which reports a different function: USB, HDMI, Charger,
> >   etc.
> >
> 
> Implementing what you've suggested would require another device driver
> or framework to relay notifications from the physical 32 bit port to
> several 'extcon' devices unless the physical 32 bit port has 32 IRQ
> lines or addresses. If it is implemented in a device driver, we will
> reimplement the same feature in every multi-port device drivers. If we
> make another framework, we are simply seperating extcon framework into
> two seperated frameworks.
> 
> It is because the "report" comes from a single device ("notifier":
> implementing extcon device) and multiple cables and devices using the
> cables need to listen to the "report" ("notifiee": registering
> notifier block to the extcon device). With the FSA9480 USB Switch,
> MAX8997 MUIC, or MAX77693 MUIC representing an external connector with
> multiple cables possible, it is impossible to create one device to
> handle events from one cable. We simply have only one GPIO for the
> interrupt and one I2C slave address for them. Even when we implement
> one device for one cable, we need something working as a "network
> router" between the connector(WAN port) and cables (LAN ports), which
> is "extcon" in this case.
> 
> If we implement an extcon-like device to handle each possible cable
> state and notify devices according to every cable state, then, it
> would like implementing extcon class code at each extcon-like device
> driver. We would need to implement this method (registering cable
> interest) in every extcon-like device (MUIC / USB Switch / ...)

I don't think this is nearly as complicated as you seem to be presenting it.

It is perfectly normal for a device to register multiple sub-devices.
Possibly of the same class, possibly different.  And then to manage them all.

So in your case you would have an i2c device which is the whole port and it
would register a collection of extcon devices and send notifications through
them as needed.

Some other port might be implemented by an spi device and it would register a
different set of extcon devices.  The only code overlap between the two would
be a simple 'for' loop.

Consider for example a typical 'led' driver, say leds-adp5520.c
It manages multiple LEDs.  But that doesn't mean we need a multi-led class.
We just have a 'led' class and adp5520_led_probe has a for loop to register
all the different led devices that it needs.
It holds these in an array and can do something to them whenever it needs.
(It only needs to do something to everything when shutting down, so it isn't
a perfect example but it will have to do).

So your one device driver would register a set of extcon devices, keep them
in an array and whenever something changes it would use a for loop to walk
the array and send a notification on each device.

Client ("notifiee") devices would register just with the particular
function(s) they are interested in.


> 
> >   The fact that the devices are related can be made clear by the choice of
> >   port names.
> >
> >   If these is a good reason, it needs to be included in the patch
> >   somewhere, possibly in a file in Documentation, so that when someone comes
> >   along to use this class in a year or two they can understand all the
> >   consequences of any decision they make (and so that I stop asking now:-)
> 
> With the current patchset,
> 
> extcon_register_interest(null_obj, "max8997-muic.0", "HDMI", nb_callback);
> 
> registers the notifier-block callback for a specific cable "HDMI" of
> max8997-muic.0
> 
> Do you mean to include a usage description like above in
> Documentation? If so, I'll make one under linux/Documentation/extcon/
> later.

I'm not exactly sure what I meant... Just that the justification for your
design choices weren't obvious, so I felt they needed to be documented
somehow.  I'm still hoping to change your design choices :-)


> 
> >
> > 2/ When you have multiple cable options, the 'state' sysfs file looks
> >   like e.g.
> >      USB 0, HDMI 1, DVI 1
> >
> >   Having list values in sysfs files is generally frowned upon but sometimes
> >   is necessary.  However I'm not sure this is a good format to use.
> >   There aren't a whole lot of examples to follow (because it is so frowned
> >   upon) but one option is
> >
> >      USB  [HDMI] [DVI]
> >
> >   where the selected option(s) are in square brackets.
> >   Another might be
> >
> >    USB=0
> >    HDMI=1
> >    DVI=1
> >
> >   with newlines - just like in the 'uevent' file.
> 
> Yes. This looks much better. Thanks.
> 
> >
> >   Also I think that some pairs of cables are mutually exclusive while others
> >   aren't.  This fact isn't made apparent in the 'state' file.  Maybe it
> >   should be.
> >
> 
> I think as long as the notifier (extcon class device) is the only one
> who is supposed to have the right to write and userspace cannot write
> state value, I don't think this is required. Such relation should be
> dealt in the notifier device and all the others who are "readers" do
> not need to know such detail.

It is always best to provide complete information - you never know when it
might be handy.

Suppose I wanted to write a little widget that displays an image on my
display of what sort of cable was attached.  Knowing that some were mutually
exclusive would allow for a more meaningful display.

> 
> >   Whatever format is used should be documented in the
> >   Documentation/ABI file.
> 
> Yes, I will add that one.
> 
> >
> >   Of course this problem would go away if you didn't allow multiple
> >   concurrent cables per port.
> >
> > 3/ The use of extcon_get_extcon_dev() requires that the port device be
> >   registered before the device that wants to be notified by it.  Ensuring
> >   correct ordering of device discovery is (I believe) not always easy -
> >   particularly with module loading.
> >
> >   Would it make sense to instead have one device register as a
> >   switch-provider and the other device register as a switch-listener and as
> >   soon as they both exist they get connected: a bit like how 'devices' and
> >   'drivers' can be registered in any order.
> >   Maybe the same device/driver infrastructure could be reused (an extcon
> >   bus maybe?) but I'm not really familiar enough with it to say (Greg??).
> >
> >   What I do know is that I have repeatedly tripped over the fact that you
> >   cannot use a GPIO line until the gpiochip has been registered and
> >   sometimes the device that needs it is registered before the device which
> >   provides it.  I wouldn't like to see that happening here too.
> >
> >   Are there other examples of inter-device dependencies that can be used as
> >   a model?
> 
> Yes, this has been sometimes a problem for me as well.
> 
> What if let "extcon_register_interest" returns <0 for errors, "1" if
> successfully registered, and "0" if the extcon name does not exists
> (probably will be "probed" later?) and register the requested interest
> later when the corresponding extcon is probed?
> 
> Then, I'll need to add "bool effective" in the struct
> extcon_specific_cable_nb so that the notifee (caller of
> extcon_register_interest()) my know if the interest is effectively
> registered or still pending.
> 
> Also, I may need to add an option to extcon_register_interest stating
> whether it is ok to be "pending" if extcon_name does not exist or is
> should return error if extcon_name does not exist.
> 
> Anyway, it seems that extcon_register_interest is the only one that
> needs to care this inter-device dependencies because
> extcon_register/unregister_interest are the only ones supposed to be
> called by notifee (depending devices).

I think that extcon_register_interest should never fail (except maybe with
-ENOMEM).

The "interest" can be registered even if the port doesn't exist yet.
Every time an extcon device is registered, the extcon module looks through
the list of pending registrations and completes any connection that matches.

When the connection is completed I suspect it would be appropriate to send a
notification with the initial state - that is the only way the client would
find out that the registration was complete.

It might be good to eventually make a generic library for this so that other
resource providers can support async registration.

NeilBrown


Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)

Powered by blists - more mailing lists