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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-id: <23829569.63411341203851815.JavaMail.weblogic@epml05>
Date:	Mon, 02 Jul 2012 04:37:32 +0000 (GMT)
From:	MyungJoo Ham <myungjoo.ham@...sung.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	박경민 <kyungmin.park@...sung.com>,
	"patches@...nsource.wolfsonmicro.com" 
	<patches@...nsource.wolfsonmicro.com>,
	최찬우 <cw00.choi@...sung.com>
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@...sung.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.

> 
> 
> 
>        
>   
>          
>