On Mon, May 16, 2022 at 13:06, AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx> 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... Thank you for taking the time to check in your downstream kernels, I really appreciate it. > > ...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. > > To stay on the safe side, though, perhaps add a comment explaining that > this driver works on that assumption? ..but that's clear, anyway, if you > actually read the code. > > From my perspective, this commit is good to go. I will keep as is for v2 series and apply your review tag. thanks again ! > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
