Re: [RFC PATCH 0/3] introduce: Multistate Switch Class

[NeilBrown]

On Mon, 28 Nov 2011 08:08:36 +0900 Greg KH <gregkh@xxxxxxx> wrote:

> On Sun, Nov 27, 2011 at 11:43:39PM +0100, Linus Walleij wrote:
> > Hi MyungJoo, Arnd
> > 
> > On Fri, Nov 25, 2011 at 3:02 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > > On Thursday 24 November 2011, MyungJoo Ham wrote:
> > >> For switch ports, which may have different types of cables
> > >> (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.
> > >>
> > >> 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.
> > >
> > > How does this relate to the new "pinmux" subsystem that Linus Walleij
> > > maintains? Would it be useful to integrate your driver into pinmux
> > > instead of starting a new subsystem?
> > 
> > Looks unrelated to pinmux but very useful.
> > 
> > And the uevent scheme from Arve seems like it's doing the
> > right thing to me, but see below on relation to <linux/input.h>.
> > 
> > The GPIO part has to be reviewed by Grant though.
> > 
> > Our charger code also needs to
> > know when the USB cable is inserted, currently we have a simple
> > cross-call in a header file:
> > static void ab8500_charger_usb_state_changed(u8 bm_usb_state, u16 mA);
> > 
> > So there is certainly a growing need of subsystems that
> > need to notify each other about things that are happening,
> > the need comes naturally from mobile ICs I think, and
> > I also think they all have one or another custom mechanism
> > in place already.
> > 
> > But - and now we need Dmitry to check the concepts:
> > 
> > Some use this stuff from <linux/input.h> to talk to userspace
> > though the input subsystem:
> > 
> > /*
> >  * Switch events
> >  */
> > 
> > #define SW_LID                  0x00  /* set = lid shut */
> > #define SW_TABLET_MODE          0x01  /* set = tablet mode */
> > #define SW_HEADPHONE_INSERT     0x02  /* set = inserted */
> > #define SW_RFKILL_ALL           0x03  /* rfkill master switch, type "any"
> >                                          set = radio enabled */
> > #define SW_RADIO                SW_RFKILL_ALL   /* deprecated */
> > #define SW_MICROPHONE_INSERT    0x04  /* set = inserted */
> > #define SW_DOCK                 0x05  /* set = plugged into dock */
> > #define SW_LINEOUT_INSERT       0x06  /* set = inserted */
> > #define SW_JACK_PHYSICAL_INSERT 0x07  /* set = mechanical switch set */
> > #define SW_VIDEOOUT_INSERT      0x08  /* set = inserted */
> > #define SW_CAMERA_LENS_COVER    0x09  /* set = lens covered */
> > #define SW_KEYPAD_SLIDE         0x0a  /* set = keypad slide out */
> > #define SW_FRONT_PROXIMITY      0x0b  /* set = front proximity sensor active */
> > #define SW_ROTATE_LOCK          0x0c  /* set = rotate locked/disabled */
> > #define SW_LINEIN_INSERT        0x0d  /* set = inserted */
> > #define SW_MAX                  0x0f
> > #define SW_CNT                  (SW_MAX+1)
> > 
> > These are *also* switches. You could very well add USB
> > cable insertion to the list above.
> > 
> > So for the userspace part it seems to me that we need to make
> > up our mind about this stuff: is it going to be through input or
> > uevent like in this patch? Or ?both??
> 
> Input please, uevent is not for things like switches that are "common",
> but for things that are "uncommon" and don't happen often.

Hi Greg,

 I don't find that argument at all convincing.  "common" is not an objective
 measure that we can all see the same way, and I don't even think it is
 relevant.

 Consider the difference between plugging in a USB-headphone set and an
 analogue mini-phone headphone set.  To the user they are the same thing.
 They should be assumed to be as common as each other.  Yet plugging in the
 USB headphone set will create a new Audio device while and so generate a
 uevent, while plugging in the analogue headset (currently) won't and will
 just send an input event.

 So they are different, not because of expected usage but simply because of
 internal implementation details.

 I think you could make a case that plugging in an analogue headset is in
 fact adding a new audio device (or enabling one that was present but
 inactive) and so deserves a uevent.
 Similarly plugging in a USB-charge makes a significant capability-change to
 the usb-gadget device so an 'change' uevent seems appropriate.

 If a switch enables or disables a device, then the state-change of the
 switch itself doesn't deserve a uevent, but the device that it
 enables/disables/changes-capability-of does.

 How the signal gets from the switch to the device when they are separate
 things is less clear, and I think it is the main focus of the proposed patch.

 Input might be a suitable vehicle for switch signalling, but if it was I
 would argue against trying to list every possible switch with its own switch
 code.  Rather there should be a generic 'SW_SWITCH' and different input
 devices each had a single switch but each did different things.  So an
 action would not be guided by the name of the switch event, but by the name
 of the device that the switch event came from.

 My own thought is that this deserves a new device class which allows easy
 hook-up of in-kernel signalling using notifications and which can be
 exported as input devices in much the same way the 'gpio' devices can be
 exported via gpio_keys.  The switch could also optionally be exported through
 sysfs much like gpio can be exported through sysfs.

 So I think I agree with your conclusion, but not with your argument :-)

NeilBrown

Attachment: signature.asc
Description: PGP signature