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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1: Type: text/plain, Size: 2793 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.

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

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

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