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

[MyungJoo Ham <myungjoo.ham@samsung.com>]

[-- 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¥