On Tue, May 24, 2022 at 09:21:28PM +0200, Mattijs Korpershoek wrote: > On dim., mai 22, 2022 at 22:42, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > > > 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@xxxxxxxxx> 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". > > That's correct and that's a good way to phrase it. > I will add that in the commit message. > > > > > 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 would prefer to have the driver's matrix_keypad (build in probe()) to > match the actual hardware. To me this seems easier to understand for > people familiar with the hardware. > > I've also tested the above snippet and it matches my expectations. > > > > > 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) > > Indeed. I plan to send out double keypad support for this driver since > that's actually needed for mt8183-pumpkin as well. > It's already in our mtk-v5.10[1] integration tree but I have not submitted > it yet. > I planned to send this a separate series to avoid burdening / have > smaller chunks to review. If that was a mistake, please let me know. > > > but I do not actually see this map layout that Mattijs drew documented > > The map layout that I draw is not directly copied from the datasheet. > It's a "translation" of the following table: > > | hardware key code | col0 | col1 | col2| > | ----------------- | -----| ---- | --- | > | row0 | 0 | 1 | 2 | > | row1 | 9 | 10 | 11 | > | row2 | 18 | 19 | 20 | > > It seems that caused more confusion than actual useful information, > sorry about that. > > > 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? > > The concern for "DTSes in the wild" that will break is a valid point. > I'm not aware of any of those. Most vendor trees i've seen don't use > this driver at all. I hope that will change at some point. > > In the end. I'd prefer to have the driver's keypad matrix match > the actual hardware. Right now we can have a 5x32 matrix which seems > absurd. Having at most an 8x8 is more reasonable. > > I'd like to send v3 with just fixing the row/column suggestion in > mt6779_keypad_irq_handler() that Dmitry suggested. > > Would that work Dmitry? OK, let's do that. Although I'd be curious to see the double keypad patches as according to the datasheets I saw the translation is different for those. Thanks. -- Dmitry
