Re: [PATCH v2 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Darren Hart]

On Thu, Jun 09, 2016 at 01:33:48PM +0200, Pali RohÃr wrote:
> On Thursday 09 June 2016 13:27:21 MichaÅ KÄpieÅ wrote:
> > > > -		case 0x10:
> > > > -			/* Keys pressed */
> > > > +		case 0x0010:
> > > > +			/* Sequence of keys pressed */
> > > > 
> > > >  			for (i = 2; i < len; ++i)
> > > > 
> > > > -				dell_wmi_process_key(buffer_entry[i]);
> > > > +				dell_wmi_process_key(0x0010, buffer_entry[i]);
> > > > 
> > > >  			break;
> > > > 
> > > > -		case 0x11:
> > > > -			for (i = 2; i < len; ++i) {
> > > > -				switch (buffer_entry[i]) {
> > > > -				case 0xfff0:
> > > > -					/* Battery unplugged */
> > > > -					pr_debug("Battery unplugged\n");
> > > > -					break;
> > > > -				case 0xfff1:
> > > > -					/* Battery inserted */
> > > > -					pr_debug("Battery inserted\n");
> > > > -					break;
> > > > -				case 0x01e1:
> > > > -				case 0x02ea:
> > > > -				case 0x02eb:
> > > > -				case 0x02ec:
> > > > -				case 0x02f6:
> > > > -					/* Keyboard backlight level changed */
> > > > -					pr_debug("Keyboard backlight level "
> > > > -						 "changed\n");
> > > > -					break;
> > > > -				default:
> > > > -					/* Unknown event */
> > > > -					pr_info("Unknown WMI event type 0x11: "
> > > > -						"0x%x\n", (int)buffer_entry[i]);
> > > > -					break;
> > > > -				}
> > > > -			}
> > > > +		case 0x0011:
> > > > +			/* Sequence of events occurred */
> > > > +			for (i = 2; i < len; ++i)
> > > > +				dell_wmi_process_key(0x0011, buffer_entry[i]);
> > > 
> > > Since this is identical to case 0x010, let's avoid the duplication
> > > of code and
> > > 
> > > handle this with a fall-through, like:
> > > 		case 0x0010:
> > > 		case 0x0011:
> > > 			/* Sequence of events occurred */
> > > 			for (i = 2; i < len; ++i)
> > > 			
> > > 				dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);
> > 
> > I believe it was Pali's intention to make a distinction between keys
> > being pressed (0x0010) and other events occuring (0x0011), so perhaps
> > a comment after the first case label could be useful?
> > 
> >     case 0x0010:
> >         /* Sequence of keys pressed; fall through */
> >     case 0x0011:
> >         /* Sequence of events occurred */
> >         for (i = 2; i < len; ++i)
> >             dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);
> > 
> > I'll leave it to Pali to decide, I'm just throwing in my two cents.
> 
> Yes, that is truth, to visually distinguish between events and keys.

The proposed comment accomplishes that without the duplication of code.

-- 
Darren Hart
Intel Open Source Technology Center