linux-kernel - Re: [RESEND PATCH 1/2] Input: mt6779-keypad

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yosex9/a7OTuC6ZK@google.com>
Date:   Sun, 22 May 2022 22:42:31 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     Mattijs Korpershoek <mkorpershoek@...libre.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Kevin Hilman <khilman@...libre.com>,
        Fabien Parent <fparent@...libre.com>,
        linux-input@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code
 mapping

On Mon, May 16, 2022 at 01:06:43PM +0200, AngeloGioacchino Del Regno wrote:
> Il 16/05/22 09:30, Mattijs Korpershoek ha scritto:
> > Hi Dmitry,
> > 
> > Thank you for your review,
> > 
> > On dim., mai 15, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@...il.com> wrote:
> > 
> > > On Fri, May 13, 2022 at 05:18:44PM +0200, Mattijs Korpershoek wrote:
> > > > In mt6779_keypad_irq_handler(), we
> > > > 1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
> > > > 2. Use that hardware code to compute columns/rows for the standard
> > > >     keyboard matrix.
> > > > 
> > > > According to the (non-public) datasheet, the
> > > > map between the hardware code and the cols/rows is:
> > > > 
> > > >          |(0)  |(1)  |(2)
> > > >      ----*-----*-----*-----
> > > >          |     |     |
> > > >          |(9)  |(10) |(11)
> > > >      ----*-----*-----*-----
> > > >          |     |     |
> > > >          |(18) |(19) |(20)
> > > >      ----*-----*-----*-----
> > > >          |     |     |
> > > > 
> > > > This brings us to another formula:
> > > > -> row = code / 9;
> > > > -> col = code % 3;
> > > 
> > > What if there are more than 3 columns?
> > That's not supported, in hardware, according to the datasheet.
> > 
> > The datasheet I have states that "The interface of MT6763 only supports
> > 3*3 single or 2*2 double, but internal ASIC still detects keys in the
> > manner of 8*8 single, and 3*3 double. The registers and key codes still
> > follows the legacy naming".
> > 
> > Should I add another patch in this series to add that limitation in the
> > probe? There are no checks done in the probe() right now.
> > 
> 
> I've just checked a downstream kernel for MT6795 and that one looks like
> being fully compatible with this driver as well... and as far as downstream
> is concerned, apparently, mt6735, 6739, 6755, 6757, 6758, 6763, 6771, 6775
> all have the same register layout and the downstream driver for these is
> always the very same one...
> 
> ...so, I don't think that there's currently any SoC that supports more than
> three columns. Besides, a fast check shows that MT8195 also has the same.
> At this point, I'd say that assuming that there are 3 columns, nor less, not
> more, is just fine.

OK, now that I looked at the datasheet I remember how it came about. The
programming (register) interface does not really care about how actual
matrix is organized, and instead has a set of bits representing keys,
from KEY0 to KEY77, arranged in 5 chunks of 15 bits split into 5 32-bit
registers. So we simply decided to use register number as row and
offset in the register as column when encoding our "matrix".

This does not match the actual keypad matrix organization, so if we want
to change this, that's fine, but then we also need to recognize that we
are skipping bits 16-31, 48-63, and so on, so to get to the right key
number we need to do something like:

	key = bit_nr / 32 * 16 + bit_nr % 32;
	row = key / 9;
	col = key % 9;

I looked at the datasheets I have and they talk about 8x8 single keypad
matrix, and 3x3 double keypad (with actual matrices either 3x3 or 2x2)
but I do not actually see this map layout that Mattijs drew  documented
anywhere though...  I also wonder if there are already existing DTSes in
the wild that will be rendered invalid by these changes. I wonder if it
would not be be better to document the existing meaning of row and
column in the driver?

Thanks.

-- 
Dmitry