Re: [PATCH v2 2/4] Input: mt-matrix-keypad: Add Bosch mt matrix keypad driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 10/05/2022 16:13, Gireesh.Hiremath@in.bosch.com wrote:
> From: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>
> 
> Hi Krzysztof,
> 
>>>>> both matric_keypad.c and mt_matrix_kepad.c logically operate differently,
>>>>> my openion is not to merge both.
>>>>
>>>> IMHO from the user/system-integrator pov it is looking the same and so
>>>> one driver should be fine. To distinguish between both modes we could
>>>> add dt-property or add a new dt-compatible like "gpio-matrix-keypad-v2".
>>>>
>>>
>>> as mentioned above our keypad is not complete matrix keypad  and it will
>>> not be compatible with matrix_keypad diver. that is the reason we derived
>>> mt matrix keypad driver.
>>>
>>> to avoid confusion, we will rename the driver as bosch_mt_keypad.c
>>> if you suggest.
>>
>> Sending a new version while discussions are ongoing is not how we reach
>> consensus.
> 
> I apologize for sending new version.
> 
>>
>> Make the driver as part of matrix-keypad driver or bring real arguments
>> why it cannot be merged.
> 
> I tryied to put real hardware scenario which used in 
> Bosch Power tool measuring devices.
> Keypad schematic as below, it is reduced matrix keypad compared
> to standard matrix keypad 
> 
>                      Pin8 (gpio1 16)-----------------------
>                      Pin7 (gpio1 20)--------------------- |
>                      Pin6 (gpio1 22)------------------- | |
>                      Pin5 (gpio2 21)----------------- | | |
>                      Pin4 (ground  )--------------- | | | |
>                      Pin3 (gpio1 31)------------- | | | | |
>                      Pin2 (gpio1 23)----------- | | | | | |
>                      Pin1 (gpio1 24)--------- | | | | | | |
>                                             | | | | | | | |
>                                             | | | | | | | |
>                                             | | | | | | | |
>     |------------|---------|----------------- | | | | | | |-----------|
>     |  Button1   |         |  Button2         | | | | | |    Button3  | 
>     |      _|_   |         |   _|_            | | | | | |       _|_   | 
>     |  |--o   o--|         |--o   o-----------| | | | | |------o   o--|       
>     |  |                                      | | | | | |             |
>     |  |         |----------------------------| | | | | |             |
>     |  | Button4 |            Button5           | | | | |  Button6    |
>     |  |   _|_   |              _|_             | | | | |    _|_      |
>     |  |--o   o--|         |---o   o------------| | | | |---o   o-----|
>     |  |                   |                      | | |               |
>     |  |                   |------------------|---| | |-----------|   |
>     |  |                                      |     |             |   |
>     |  |------------------------------|       |     |---------|   |   |
>     |                                 |       |               |   |   |
>     |   Button7              Button8  |	      |    Button9    |   |   |
>     |      _|_                _|_     |	      |       _|_     |   |   |
>     |-----o   o-----|--------o   o----|       |------o   o----|   |   |
>                     |                 |                           |   |
>                     |                 |---------------------------|   |
>                     |                                                 |
>                     |-------------------------------------------------|
> 
> 
>     ____________________________________
>     | Button  | Pin activation| Keymap |
>     |----------------------------------|
>     |Button1  |	    1,6       | KEY_7  |
>     |----------------------------------|
>     |Button2  |	    1,2       | KEY_8  |
>     |----------------------------------|
>     |Button3  |	    7,8       | KEY_9  |
>     |----------------------------------|
>     |Button4  |	    2,6       | KEY_4  |
>     |----------------------------------|
>     |Button5  |	    3,4       | KEY_5  |
>     |----------------------------------|
>     |Button6  |	    6,7       | KEY_6  |
>     |----------------------------------|
>     |Button7  |	    1,8       | KEY_1  |
>     |----------------------------------|
>     |Button8  |	    6,8       | KEY_2  |
>     |----------------------------------|
>     |Button9  |	    4,5       | KEY_3  |
>     |----------------------------------|
> 				
> for Button5 and Button9 we used standard gpio_keys.c driver.
> 
> Button1,2,3,4,6,7,8 are not in standard row and column format,
> found difficulty to apply matrix keypad drive to these button.
> 
> to solve this we came with vendor specific driver like
> mt_matrix_keypad.c by taking matrix_keypad as reference.
> 
> after your review comment I felt it should named as
> bosch_keypad.c to show as vendor specific.
> 
> in this driver all gpio lines act as row as well as column,
> a key can be placed at each intersection of a unique row
> number not equal to a unique column and they are diagonally
> symmetric.
> we can skip keymap for the valid intersection of gpio and
> invalid keymap for row equal to column.
> 
> the matrix table as below for above schematic
> 
>     ------------------------------------------------------
>     |Row\Col |GPIO 0 | GPIO 1 | GPIO 2 | GPIO 3 | GPIO 4 |
>     ------------------------------------------------------
>     | GPIO 0 |  X    | KEY_9  | KEY_2  |   X    | KEY_1  |
>     ------------------------------------------------------
>     | GPIO 1 | KEY_9 |  X     | KEY_6  |   X    |  X     |
>     ------------------------------------------------------
>     | GPIO 2 | KEY_2 | KEY_6  |  X     | KEY_4  | KEY_7  |
>     ------------------------------------------------------
>     | GPIO 3 |  X    |  X     | KEY_4  |  X     | KEY_8  |
>     ------------------------------------------------------
>     | GPIO 4 | KEY_1 |  X     | KEY_7  | KEY_8  |  X     |
>     ------------------------------------------------------
>     X - invalid key
>     KEY_x - preferred key code
> 
> 
> in Device tree we avoided row and column 
> and passed gpio info as line-gpios
> 
> line-gpios = <
>           &gpio1 24 1     /*gpio_56*/
>           &gpio1 23 1     /*gpio_55*/
>           &gpio1 22 1     /*gpio_54*/
>           &gpio1 20 1     /*gpio_52*/
>           &gpio1 16 1     /*gpio_48*/
>         >;
>         linux,keymap = <
>                 0x00000000 /* row 0, col 0, KEY_RESERVED */
>                 0x0001000a /* row 0, col 1, KEY_9 */
>                 0x00020003 /* row 0, col 2, KEY_2 */
>                 0x00030000 /* row 0, col 3, KEY_RESERVED */
>                 0x00040002 /* row 0, col 4, KEY_1 */
>                 0x0100000a /* row 1, col 0, KEY_9 */
>                 0x01010000 /* row 1, col 1, KEY_RESERVED */
>                 0x01020007 /* row 1, col 2, KEY_6 */
>                 0x01030000 /* row 1, col 3, KEY_RESERVED */
>                 0x01040000 /* row 1, col 4, KEY_RESERVED */
>                 0x02000003 /* row 2, col 0, KEY_2 */
>                 0x02010007 /* row 2, col 1, KEY_6 */
>                 0x02020000 /* row 2, col 2, KEY_RESERVED */
>                 0x02030005 /* row 2, col 3, KEY_4 */
>                 0x02040008 /* row 2, col 4, KEY_7 */
>                 0x03000000 /* row 3, col 0, KEY_RESERVED */
>                 0x03010000 /* row 3, col 1, KEY_RESERVED */
>                 0x03020005 /* row 3, col 2, KEY_4 */
>                 0x03030000 /* row 3, col 3, KEY_RESERVED */
>                 0x03040009 /* row 3, col 4, KEY_8 */
>                 0x04000002 /* row 4, col 0, KEY_1 */
>                 0x04010000 /* row 4, col 1, KEY_RESERVED */
>                 0x04020008 /* row 4, col 2, KEY_7 */
>                 0x04030009 /* row 4, col 3, KEY_8 */
>                 0x04040000 /* row 4, col 4, KEY_RESERVED */
>         >;
> 
> this driver approch may be usefull for the embadded device
> which are using reduced matrix keypad

You wrote pretty long message explaining how the device works, but I
still do not see the answer to questions - why it cannot be part of
matrix keypad?

"It looks like this driver has smaller number of features than
matrix-keypad, so it should be integrated into the matrix-keypad.
matrix-keypad features are superset to this one."

"But anyway this should be just merged into matrix-keypad. It's a
simpler set of that binding."


Best regards,
Krzysztof