Re: [PATCH v17 1/3] dt-bindings: Add bindings for Mediatek matrix keypad

Re: [PATCH v17 1/3] dt-bindings: Add bindings for Mediatek matrix keypad

[Yingjoe Chen]

Hi,


On Mon, 2020-08-10 at 14:40 +0800, Fengping Yu wrote:
> From: "fengping.yu" <fengping.yu@mediatek.com>
> 
> This patch add devicetree bindings for Mediatek matrix keypad driver.
> 
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> ---
>  .../devicetree/bindings/input/mtk-kpd.yaml    | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.yaml b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> new file mode 100644
> index 000000000000..d74dd8a6fbde

<...>


> +  keypad,num-columns:
> +    description: Number of column lines connected to the keypad controller,
> +    it is not equal to PCB columns number, instead you should add required value
> +    for each IC. If not specified, the default value is 1.
> +
> +  keypad,num-rows:
> +    description: Number of row lines connected to the keypad controller, it is
> +    not equal to PCB rows number, instead you should add required value for each IC.
> +    If not specified, the default value is 1.

Your source code can't really handle dts without rows/columns
properties. Also, the default value doesn't make any sense. No IC will
have rows or columns set to 1.

Since these are IC specified, not board specified, I think you should
just have the correct numbers in driver.

Joe.C

Re: [PATCH v17 1/3] dt-bindings: Add bindings for Mediatek matrix keypad

[Dmitry Torokhov]

Hi,

On Tue, Aug 11, 2020 at 09:47:23AM +0800, Yingjoe Chen wrote:
> Hi,
> 
> 
> On Mon, 2020-08-10 at 14:40 +0800, Fengping Yu wrote:
> > From: "fengping.yu" <fengping.yu@mediatek.com>
> > 
> > This patch add devicetree bindings for Mediatek matrix keypad driver.
> > 
> > Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> > ---
> >  .../devicetree/bindings/input/mtk-kpd.yaml    | 87 +++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.yaml b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> > new file mode 100644
> > index 000000000000..d74dd8a6fbde
> 
> <...>
> 
> 
> > +  keypad,num-columns:
> > +    description: Number of column lines connected to the keypad controller,
> > +    it is not equal to PCB columns number, instead you should add required value
> > +    for each IC. If not specified, the default value is 1.
> > +
> > +  keypad,num-rows:
> > +    description: Number of row lines connected to the keypad controller, it is
> > +    not equal to PCB rows number, instead you should add required value for each IC.
> > +    If not specified, the default value is 1.
> 
> Your source code can't really handle dts without rows/columns
> properties. Also, the default value doesn't make any sense. No IC will
> have rows or columns set to 1.
> 
> Since these are IC specified, not board specified, I think you should
> just have the correct numbers in driver.

It is actually property of board to decide how many keys it wants to
wire up. In extreme case it will be a single key, i.e. number of rows
and columns will indeed be 1.

Thanks.

-- 
Dmitry

Re: [PATCH v17 1/3] dt-bindings: Add bindings for Mediatek matrix keypad

[Dmitry Torokhov]

Hi,

On Mon, Aug 10, 2020 at 02:40:57PM +0800, Fengping Yu wrote:
> From: "fengping.yu" <fengping.yu@mediatek.com>
> 
> This patch add devicetree bindings for Mediatek matrix keypad driver.
> 
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> ---
>  .../devicetree/bindings/input/mtk-kpd.yaml    | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.yaml b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> new file mode 100644
> index 000000000000..d74dd8a6fbde
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +version: 1
> +
> +$id: http://devicetree.org/schemas/input/mtk-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek's Keypad Controller device tree bindings
> +
> +maintainer:
> +  - Fengping Yu <fengping.yu@mediatek.com>
> +
> +description: |
> +  Mediatek's Keypad controller is used to interface a SoC with a matrix-type
> +  keypad device. The keypad controller supports multiple row and column lines.
> +  A key can be placed at each intersection of a unique row and a unique column.
> +  The keypad controller can sense a key-press and key-release and report the
> +  event using a interrupt to the cpu.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: "mediatek,mt6779-keypad"
> +      - const: "mediatek,mt6873-keypad"
> +
> +  clock-names:
> +    description: Names of the clocks listed in clocks property in the same order
> +    maxItems: 1
> +    items:
> +    	- const: kpd
> +
> +  clocks:
> +    description: Must contain one entry, for the module clock
> +    refs: devicetree/bindings/clocks/clock-bindings.txt for details.
> +
> +  interrupts:
> +    description: A single interrupt specifier
> +    maxItems: 1
> +
> +  linux,keymap:
> +    description: The keymap for keys as described in the binding document
> +    refs: devicetree/bindings/input/matrix-keymap.txt
> +    minItems: 1
> +
> +  reg:
> +    description: The base address of the Keypad register bank
> +    maxItems: 1
> +
> +  wakeup-source:
> +    description: use any event on keypad as wakeup event
> +    type: boolean
> +
> +  keypad,num-columns:
> +    description: Number of column lines connected to the keypad controller,
> +    it is not equal to PCB columns number, instead you should add required value
> +    for each IC. If not specified, the default value is 1.

What sets the default? matrix_keymap_parse_properties() will error out
if either property is not present....

> +
> +  keypad,num-rows:
> +    description: Number of row lines connected to the keypad controller, it is
> +    not equal to PCB rows number, instead you should add required value for each IC.
> +    If not specified, the default value is 1.
> +
> +  mediatek,debounce-us:
> +    description: Debounce interval in microseconds, if not specified, the default
> +    value is 16000
> +    maximum: 256000
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - linux,keymap
> +  - clocks
> +  - clock-names
> +
> +examples:
> +  - |
> +
> +  kp@10010000 {
> +    compatible = "mediatek,mt6779-keypad";

Should we call the driver mt6779-keypad ?

> +    reg = <0 0x10010000 0 0x1000>;
> +    linux,keymap = < MATRIX_KEY(0x00, 0x00, KEY_VOLUMEDOWN) >;
> +    interrupts = <GIC_SPI 75 IRQ_TYPE_EDGE_FALLING>;
> +    clocks = <&clk26m>;
> +    clock-names = "kpd";
> +  };
> -- 
> 2.18.0

Thanks.

-- 
Dmitry

Re: [PATCH v17 1/3] dt-bindings: Add bindings for Mediatek matrix keypad

[Yingjoe Chen]

On Wed, 2020-08-12 at 15:13 -0700, Dmitry Torokhov wrote:
> Hi,
> 
> On Tue, Aug 11, 2020 at 09:47:23AM +0800, Yingjoe Chen wrote:
> > Hi,
> > 
> > 
> > On Mon, 2020-08-10 at 14:40 +0800, Fengping Yu wrote:
> > > From: "fengping.yu" <fengping.yu@mediatek.com>
> > > 
> > > This patch add devicetree bindings for Mediatek matrix keypad driver.
> > > 
> > > Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> > > ---
> > >  .../devicetree/bindings/input/mtk-kpd.yaml    | 87 +++++++++++++++++++
> > >  1 file changed, 87 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.yaml b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> > > new file mode 100644
> > > index 000000000000..d74dd8a6fbde
> > 
> > <...>
> > 
> > 
> > > +  keypad,num-columns:
> > > +    description: Number of column lines connected to the keypad controller,
> > > +    it is not equal to PCB columns number, instead you should add required value
> > > +    for each IC. If not specified, the default value is 1.
> > > +
> > > +  keypad,num-rows:
> > > +    description: Number of row lines connected to the keypad controller, it is
> > > +    not equal to PCB rows number, instead you should add required value for each IC.
> > > +    If not specified, the default value is 1.
> > 
> > Your source code can't really handle dts without rows/columns
> > properties. Also, the default value doesn't make any sense. No IC will
> > have rows or columns set to 1.
> > 
> > Since these are IC specified, not board specified, I think you should
> > just have the correct numbers in driver.
> 
> It is actually property of board to decide how many keys it wants to
> wire up. In extreme case it will be a single key, i.e. number of rows
> and columns will indeed be 1.
> 
> Thanks.
> 

From the binding "it is not equal to PCB columns number, instead you
should add required value for each IC."
Driver code use this to calculate bit position in register, which is IC
dependent.

Joe.C