Messages in this thread | | | Date | Mon, 10 Mar 2014 08:28:59 -0700 | From | Dmitry Torokhov <> | Subject | Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver |
| |
Hi Lee,
On Mon, Mar 10, 2014 at 11:48:19AM +0000, Lee Jones wrote: > Hi Gabi, > > Sorry for the delay. It was a hectic week last week. > > As promised: > > > This patch adds ST Keyscan driver to use the keypad hw a subset > > of ST boards provide. Specific board setup will be put in the > > given dt. > > > > Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@st.com> > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > > Are you sure these are in the correct order? > > What is the history of this commit? > > > --- > > .../devicetree/bindings/input/st-keypad.txt | 50 ++++ > > This should be submitted as a seperate patch.
Why do we have such requirement? To me it would make more sense to add binding documentation in the same commit as the code that uses these bindings.
[...]
> > + > > + error = matrix_keypad_parse_of_params(dev, &pdata->num_out_pads, > > + &pdata->num_in_pads); > > + if (error) { > > + dev_err(dev, "failed to parse keypad params\n"); > > + return error; > > Nit: It's pretty unusual to use this for a standard error handling > variable. Consider 'ret' or 'err' as a replacement.
I like "error", in fact there are a lot of these in input. I use "error" for data that is only returned from error path and "retval" when the same variable is returned in both success and error paths.
> > + > > + input_dev->id.bustype = BUS_HOST; > > + input_dev->id.vendor = 0x0001; > > + input_dev->id.product = 0x0001; > > + input_dev->id.version = 0x0100; > > Any chance we can #define these?
Even better would be not use 0x0001 as vendor as there (unfortunately) quite a few other drivers use it already. Either omit or chose something else. Does ST have PCI or USB VID assigned?
Thanks.
-- Dmitry
| |