Re: [PATCH v2 0/3] introduce External Connector Class (extcon)

[NeilBrown]

On Wed, 14 Dec 2011 19:28:25 +0900 MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
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.

   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:-)

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.

   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.

   Whatever format is used should be documented in the
   Documentation/ABI file.

   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?



> 
> This patchset supports the usage of notifier for passing such information
> between device drivers.
> 
> Another issue is that at a single switch port, there might be multiple
> and heterogeneous cables attached at the same time. Besides the state
> (Attached or Detached) of each cable may alter independently.
> 
> In order to address such issues, Android kernel's "Switch" class seems to
> be a good basis and we have implemented "Multistate Switch Class" based on
> it. The "Switch" class code of Android kernel is GPL as well.

Do you need to update this text to remove "Multistate Switch Class" ??

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature