Re: Re: [PATCH] Extcon: Arizona: Add driver for Wolfson Arizona class devices

Re: Re: [PATCH] Extcon: Arizona: Add driver for Wolfson Arizona class devices

[MyungJoo Ham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 3637 bytes --]

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.

> 
> 
> 
>        
>   
>          
> 
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH] Extcon: Arizona: Add driver for Wolfson Arizona class devices

[MyungJoo Ham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 1115 bytes --]

Hello Mark,

Though extcon-arizona.c itself does not seem to have problems, when I add the patches from your repository (http://git.kernel.org/?p=linux/kernel/git/broonie/misc.git;a=commit;h=3afbac957e3c59037a4ecaf19d68f6c8104299fc), I get compiler errors if MFD_ARIZONA is enabled in order to enable EXTCON_ARIZONA. (No other Arizona-related devices are enabled; I don't have any of them anyway) This is not an issue w/ extcon-arizona.c, but I just wanted to let you know.

  CC      drivers/extcon/extcon-arizona.o
  LD      drivers/extcon/built-in.o
  CC      drivers/mfd/wm5102-tables.o
  CC      drivers/mfd/max8997-irq.o
drivers/mfd/wm5102-tables.c:601:2: error: unknown field ¡®wake_base¡¯ specified in initializer
make[2]: *** [drivers/mfd/wm5102-tables.o] Error 1
make[2]: *** Waiting for unfinished jobs....
  CC      drivers/staging/staging.o
make[1]: *** [drivers/mfd] Error 2
make[1]: *** Waiting for unfinished jobs....


Cheers!
MyungJoo.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH] Extcon: Arizona: Add driver for Wolfson Arizona class devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Tue, Jul 03, 2012 at 07:20:38AM +0000, MyungJoo Ham wrote:

You should fix your mailer to word wrap within paragraphs, I've reflowed
your text for legibility.

> Though extcon-arizona.c itself does not seem to have problems, when I
> add the patches from your repository
> (http://git.kernel.org/?p=linux/kernel/git/broonie/misc.git;a=commit;h=3afbac957e3c59037a4ecaf19d68f6c8104299fc),

Please provide human readable descriptions of things!

> I get compiler errors if MFD_ARIZONA is enabled in order to enable
> EXTCON_ARIZONA. (No other Arizona-related devices are enabled; I don't
> have any of them anyway) This is not an issue w/ extcon-arizona.c, but
> I just wanted to let you know.

Note the cover letter for the series...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Re: [PATCH] Extcon: Arizona: Add driver for Wolfson Arizona class devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On Mon, Jul 02, 2012 at 04:37:31AM +0000, MyungJoo Ham wrote:

> 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.

If you're maintaining this tree could you please include it in -next so
things get integration tested?  I guess some coordination with Greg is
required, there's a bunch of patches already queued in his tree for
extcon.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]