Re: [PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Martyn Welch <martyn.welch@ge.com>]

Benjamin Herrenschmidt wrote:
> O
>   
>> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
>> index 48f0a00..3d169bb 100644
>> --- a/arch/powerpc/kernel/setup-common.c
>> +++ b/arch/powerpc/kernel/setup-common.c
>> @@ -94,6 +94,10 @@ struct screen_info screen_info = {
>>  	.orig_video_points = 16
>>  };
>>  
>> +/* Variables required to store legacy IO irq routing */
>> +int of_i8042_kbd_irq;
>> +int of_i8042_aux_irq;
>>     
>
> Is there a reasonable ifdef to use for the above or we don't care ?
>
>   
>>  #ifdef __DO_IRQ_CANON
>>  /* XXX should go elsewhere eventually */
>>  int ppc_do_canonicalize_irqs;
>> @@ -567,6 +571,15 @@ int check_legacy_ioport(unsigned long base_port)
>>  			np = of_find_compatible_node(NULL, NULL, "pnpPNP,f03");
>>  		if (np) {
>>  			parent = of_get_parent(np);
>> +
>> +			of_i8042_kbd_irq = irq_of_parse_and_map(parent, 0);
>> +			if (!of_i8042_kbd_irq)
>> +				of_i8042_kbd_irq = 1;
>> +
>> +			of_i8042_aux_irq = irq_of_parse_and_map(parent, 1);
>> +			if (!of_i8042_aux_irq)
>> +				of_i8042_aux_irq = 12;
>> +
>>  			of_node_put(np);
>>  			np = parent;
>>  			break;
>> diff --git a/drivers/input/serio/i8042-io.h b/drivers/input/serio/i8042-io.h
>> index 847f4aa..5d48bb6 100644
>> --- a/drivers/input/serio/i8042-io.h
>> +++ b/drivers/input/serio/i8042-io.h
>> @@ -27,6 +27,11 @@
>>  #include <asm/irq.h>
>>  #elif defined(CONFIG_SH_CAYMAN)
>>  #include <asm/irq.h>
>> +#elif defined(CONFIG_PPC)
>> +extern int of_i8042_kbd_irq;
>> +extern int of_i8042_aux_irq;
>> +# define I8042_KBD_IRQ  of_i8042_kbd_irq
>> +# define I8042_AUX_IRQ  of_i8042_aux_irq
>>  #else
>>  # define I8042_KBD_IRQ	1
>>  # define I8042_AUX_IRQ	12
>>     
>
> Now while that works, I do tend to dislike global variables like that.
>
> _Maybe_ a better approach would be to have those #define resolve to
> functions:
>
> #define I8042_KBD_IRQ	of_find_i8042_kbd_irq()
>
> Or something like that ?
>
> That means ending up having 2 functions which more/less reproduce the
> loop to find the driver in the device-tree but that's a minor
> inconvenience.
>
> Now, maybe the variables are less bloat here. What do you think ? Which
> way do you prefer ?
>
>   

Personally, I'm happy either way. If you'd prefer I implement these as
functions, I can't quickly see why that wouldn't work. I thought using
variables would be the least disruptive.

I'm also happy to change it so that the irqs are specified in the kbd
and aux nodes (I agree it would make much more sense and was how the
first revision of this patch worked).

Either way, my preferred solution is that contained in this patch,
unless those with more experience agree otherwise... ;-)

Martyn

> Cheers,
> Ben.
>
>
>   


-- 
Martyn Welch (Principal Software Engineer)   |   Registered in England and
GE Intelligent Platforms                     |   Wales (3828642) at 100
T +44(0)127322748                            |   Barbirolli Square, Manchester,
E martyn.welch@ge.com                        |   M2 3AB  VAT:GB 927559189