2020-08-11 01:50:08

by Yingjoe Chen

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

Hi,


On Mon, 2020-08-10 at 14:40 +0800, Fengping Yu wrote:
> From: "fengping.yu" <[email protected]>
>
> This patch add devicetree bindings for Mediatek matrix keypad driver.
>
> Signed-off-by: fengping.yu <[email protected]>
> ---
> .../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


2020-08-12 22:18:06

by Dmitry Torokhov

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

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" <[email protected]>
> >
> > This patch add devicetree bindings for Mediatek matrix keypad driver.
> >
> > Signed-off-by: fengping.yu <[email protected]>
> > ---
> > .../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

2020-08-13 05:04:20

by Yingjoe Chen

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

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" <[email protected]>
> > >
> > > This patch add devicetree bindings for Mediatek matrix keypad driver.
> > >
> > > Signed-off-by: fengping.yu <[email protected]>
> > > ---
> > > .../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