On sam., mai 28, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > 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. Sorry for the delay. I had some long needed away time from my computer. Yes, it's a different translation for double keypad. I will send a v3 to fix single keys and make sure to send the double keypad support afterwards. > > Thanks. > > -- > Dmitry
