lkml.org 
[lkml]   [2012]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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.

>
>
>
>
>
>
>
\
 
 \ /
  Last update: 2012-07-02 07:22    [W:0.079 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site