Messages in this thread | | | Date | Mon, 02 Jul 2012 04:37:32 +0000 (GMT) | From | MyungJoo Ham <> | Subject | Re: Re: [PATCH] Extcon: Arizona: Add driver for Wolfson Arizona class devices |
| |
On Thu, Jun 28, 2012 at 02:08:10AM +0000, MyungJoo Ham wrote: > > > I only have some performance concerns that may be ignored > > if you don't care of it for this device. > > To be honest I think if we ever care about performance with extcon we've > got a serious problem - cable insertion shouldn't be happening too > quickly and obviously the userspace API has all the same issues.
Yes.. the performance part was not a serious concern (so it may be ignored for now).
The only "real" concern was the cable name part and I'll add a patch fixing related drivers (including this) after fixing the class file.
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
I'll put this to git http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/extcon-for-next after testing with the MFD part of Arizona drivers.
Thanks
MyungJoo.
> > > > +#define ARIZONA_CABLE_MECHANICAL "Mechanical" > > > +#define ARIZONA_CABLE_HEADPHONE "Headphone" > > > +#define ARIZONA_CABLE_HEADSET "Headset" > > > > +static const char *arizona_cable[] = { > > > + ARIZONA_CABLE_MECHANICAL, > > > + ARIZONA_CABLE_HEADSET, > > > + ARIZONA_CABLE_HEADPHONE, > > > + NULL, > > > +}; > > > For ARIZONA_CABLE_HEADPHONE and ARIZONA_CABLE_MECHANICAL, you can > > use extcon_cable_name[EXTCON_HEADPHONE_OUT] and > > extcon_cable_name[EXTCON_MECHANICAL]. > > > It appears that I need to rephrase line 38-41 of extcon_class.c. Anyway, > > it is not recommended to import the whole list. However, it is strongly > > recommended to reuse the corresponding entries from the list. > > That's what I initially wanted to do but there's real usability problems > fishing the values out of the array, the obvious method does things like > this: > > drivers/extcon/extcon-arizona.c:62: error: initializer element is not constant > drivers/extcon/extcon-arizona.c:62: error: (near initialization for 'arizona_cable[0]') > > for example and you don't want the driver to end up looking like line > noise. Perhaps there's some simple way of doing it that didn't occur to > me but there aren't any examples in tree.
Hmm... ok. I'll fix it and apply to corresponding drivers later.
> > > Anyway, the HEADSET appears to be a pair of HEADPHONE and MIC. > > You may replace HEADSET with MIC in arizona_cable and remove exclusive[] > > and regard HEADPHONE | MIC as "HEADSET". > > This was done following the example of the Android switch API which > defines these as separate cable types. Cable type is probably the wrong > name here but it's a bit late now... > > > > + /* If we got a high impedence we should have a headset, report it. */ > > > + if (info->detecting && (val & 0x400)) { > > > + ret = extcon_set_cable_state(&info->edev, > > > + ARIZONA_CABLE_HEADSET, true); > > > You may use extcon_set_cable_state_ for the performance > > as you already know the index of HEADSET. Or extcon_update_state(); > > I didn't use set_cable_state_ as the _ makes it look like > extcon_set_cable_state() is the intended call, obviously almost every > driver will have the indexes known. If there's much preferenced here > I'd expect the main function to take the numbers as argument and then > have extcon_set_cable_state_by_name() or something. > > extcon_update_state() is a bit annoying to use as you need defines for > both indexes and bits or you need shifting so the code looks ugly.
Whichever is fine. This is not a critical part.
> > > > > > >
| |