Re: [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify.

[Greg KH <gregkh@suse.de>]

On Fri, Dec 16, 2011 at 02:38:38PM +0900, MyungJoo Ham wrote:
> On Thu, Dec 15, 2011 at 4:18 PM, Greg KH <gregkh@suse.de> wrote:
> > On Thu, Dec 15, 2011 at 02:41:38PM +0900, MyungJoo Ham wrote:
> >> On Thu, Dec 15, 2011 at 10:01 AM, Greg KH <gregkh@suse.de> wrote:
> >> >
> >> > Nice, but if we accept this, will someone also make the needed changes
> >> > to the Android userspace code to handle the user api changes that this
> >> > causes?
> >>
> >> I have no idea about how Android will react to this as I have no
> >> developmental experiences with Android.
> >> However, from the perspective of general userspace, this modification
> >> incurs path changes (/sys/class/switch/.... to /sys/class/extcon/...)
> >> only.
> >
> > Well, without such changes, any Android platform will still have to
> > include the switch code in their system, making this work a bit
> > pointless, right?
> >
> > Please look into changing this in userspace, if for no other reason than
> > to test that this kernel code works properly with the Android userspace
> > needs as well.
> 
> 1. Kernel source with Android patch has "switch" class drivers at
> /drivers/switch, which are not compatible with "extcon" class.
> 2. Android userspace access /sys/class/switch/ nodes, which are
> compatible with extcon nodes.
> 
> Not to interfere with Android kernel patches, not to be confused with
> incompatible Android switch class and its drivers, and to be
> compatible with Android userspace at the same time, we may put the
> device drivers located at Linux/drivers/extcon/ and let the class uses
> /sys/class/switch/*.

No, that's really not going to help as you changed the way the sysfs
files worked, right?

> I remember there were big no-es on using the name "switch" for these
> external connectors; however, would this userspace class name be fine?
> Or would it be fine to make the class name customizable? (let the
> device driver choose whether to be named "switch" or "extcon" or let
> the class define its location based on kernel config).
> 
> In order to get some comments from Android kernel builders, I've added
> android-kernel@googlegroups.com.

Why can't you submit patches for the userspace Android code to use this
new api instead of their sys/class/switch/ code?  If functionally it's
the same, it should be ok to do this, right?

> >> >> +                     kobject_uevent(&edev->dev->kobj, KOBJ_CHANGE);
> >> >
> >> > I really dislike using uevents, what is listening for them?  Are you
> >> > hooked into udev's event chain in userspace to properly handle this?  If
> >> > not, what is the point of sending them?
> >>
> >> It is to let userspace processes get notified for the events in extcon.
> >> Do you think sysfs_notify() would be better for this purpose?
> >
> > No, I don't think it does what you think it does :)
> >
> > What are you trying to accomplish here?  And how would sysfs_notify()
> > accomplish that?
> 
> Android has been "observing" kobject uevents. It has been observing
> the switch class kobject (&edev->dev->kobj) and that has not been
> changed from Android kernel.
> In Android, with "startObserving" method, one can let a function to be
> called with incoming UEvent.
> I haven't look into how "startObserving" is implemented. However, they
> do observe KOBJ_CHANGE at userspace. And this is not limted to
> Android.

It would be good to figure out how "observing" actually works here.

> Here, my intention is the same. It is to invoke a function in a user
> process that has registered to be invoked for a UEvent. If we are
> going to use other methods for this, we will also force userspace
> processes to change their methods to get events (at least for switch
> class and chargers) not from UEVENT.
> 
> Maybe the userspace is abusing UEVENT/KOBJ_CHANGE, though... that's
> what userspace observes to get events from kernel often.

I don't mind using the kobject change call, I just want to be very
careful about it as there aren't many users of it in the kernel at the
moment, and very little userspace code that I know of that takes
advantage of it.

If the Android framework is using it, that's fine, it's one reason to
use it, but actually determining this would be a good thing for you to
do, right?

thanks,

greg k-h