Messages in this thread | | | Date | Tue, 18 Mar 2014 11:25:34 +0100 | From | Gabriel Fernandez <> | Subject | Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver |
| |
Hi Lee,
On 03/14/2014 11:42 AM, Lee Jones wrote: >>> 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? >> ok i change the order > I'm not saying they are in the wrong order, I'm just asking. Who wrote > the patch? Has it changed since? Sorry... I wrote the patch, then Guiseppe has changed and tested, and I re-modifiedit >>>> +- linux,keymap: The keymap for keys as described in the binding document >>>> + devicetree/bindings/input/matrix-keymap.txt. >>>> + >>>> +- keypad,num-rows: Number of row lines connected to the keypad controller. >>>> + >>>> +- keypad,num-columns: Number of column lines connected to the keypad >>>> + controller. >>>> + >>>> +- st,debounce_us: Debouncing interval time in microseconds >>> I'm sure there will be a shared binding for de-bounce. >>> >>> If not, there certainly should be. >> you want to refer to "debounce-interval" ? > That sounds more generic, but if it's not documented as such, then > please consider doing so. > >>> +Example: >>> + >>> +keyscan: keyscan@fe4b0000 { >>> + compatible = "st,keypad"; >>> Is there any way we can make this more specific to _this_ IP? >> for my knowledge this IP is the same for stixxxx platform. > So st,stix-keypad, or st,sti4x-keypad? > > I'm just thinking about future proofing the architecture. What if ST > released stj which has a different keypad IP? After discussing internally with st "st,sti-keyscan" is better
>>>> +struct keyscan_priv { >>>> + void __iomem *base; >>>> + int irq; >>>> + struct clk *clk; >>>> + struct input_dev *input_dev; >>>> + struct keypad_platform_data *config; >>>> + unsigned int last_state; >>>> + u32 keycodes[ST_KEYSCAN_MAXKEYS]; >>> Seems odd to limit this. Can't the information come from DT >>> i.e. keypad,num-rows and keypad,num-columns? >>> >> i 'll rename 'num_out_pads' into 'n_rows' and 'num_in_pads' into >> 'n_cols' > That's not quite what I meant, I mean can't ST_KEY_MAXKEYS be more > dynamic and be obtained from (keypad,num-rows * keypad,num-columns)? ok
| |