> Hi Joshua, > > Thanks a lot for your patch! > > It might cause conflicts with [1] and [2], so we'll need to rebase on > top of each other work at some point. > > [1] > https://lore.kernel.org/linux-input/20221226123456.14822-1-jose.exposito89@ > gmail.com/T/ [2] > https://lore.kernel.org/linux-input/20221226125454.16106-1-jose.exposito89@ > gmail.com/T/ I've seen those patches in the mailing list (and I thought they already got merged heh) so I'll keep a close eye and rebase as needed. > > Sparse nitpick: "uclogic_extra_input_mapping" can be made static. > I somehow missed that, will change. > > Is it possible to receive a report with information from both dials at > the same time? > > I'm asking because I'm trying to understand what is the meaning of the > 0x10 and 0x20 values and I wonder if they are generated when both dials > are used at the same time. > According to Aren's reference sheet he wrote up for this tablet, the 0x10 and 0x20 values are reported by the right dial (left frame: 02:f0:00:00:00:00:00:20:00:14, right frame: 02:f0:00:00:00:00:00:10:00:14). Dumping the contents of data[frame->bitmap_dial_byte] yields the same result. The left dial works like usual, 02 for left and 01 for right. As for recieving reports of the two dials turning at the same time... I can't seem to make it do that. This tablet is pretty big, and I can't turn the dials fast enough. > > You can probably reuse uclogic_params_ugee_v2_init() or at least reuse > uclogic_probe_interface() and uclogic_params_parse_ugee_v2_desc() if > for some reason we need custom logic for this tablet. > I actually looked a little into the UGEE v2 init functions to see if I could reuse anything (but to be honest, I really just skimmed) but I will take a second look to see if I can consolidate it. > > Is this value 8? In all the models I have seen so far this is indeed > the number of buttons. > > Also, what's the value of buf[6]? As you can see in > uclogic_params_parse_ugee_v2_desc(), this field is the frame type. I'd be > nice to know whether a different frame type is reported when 2 dials are > present or not. > > Could you attach the contents of the 14 bytes of "buf", please? I'd be > nice to have a look and see if we can reuse as much code as possible. > Yeah here's the 14 bytes of "buf" here: buf[0] = 000c buf[1] = 0003 buf[2] = 0030 buf[3] = 00ba buf[4] = 009a buf[5] = 0068 buf[6] = 0006 buf[7] = 0000 buf[8] = 00ff buf[9] = 001f buf[10] = 00ec buf[11] = 0009 buf[12] = 0080 buf[13] = 0072 I'm not sure you made a typo, but buf[6] in uclogic_params_parse_ugee_v2_desc is reading the button count, which reports as 6 for some reason. buf[7] is 0 though, so it appears that its still reporting as UCLOGIC_PARAMS_FRAME_BUTTONS (I could just be misunderstanding the strdescs) or the frames are completely different. > > Ideally, we should be able to handle this tablet with the other UGEE v2 > tablets. > Yeah I will consolidate this case if I manage to merge this with the other UGEE v2 tablet support. > > This array is already declared in uclogic_params_ugee_v2_init(), which, > hopefully, we will be able to reuse. Otherwise, you might be interested > in this commit (not merged yet): > https://lore.kernel.org/linux-input/20221226125454.16106-4-jose.exposito89@g > mail.com/T/#u I didn't see that array in ugee_v2_init, what I'll do is match the variables defined in your cleanup patch (I believe it's closely identical already), so it makes rebasing easier no matter what order they're merged in. > > Can't "uclogic_rdesc_ugee_v2_pen_template_arr" be used instead? > Yeah I think so, at first I didn't consider it but on closer inspection - the offsets are the same (just for some reason, out of order). I'll be looking into testing the pen using this descriptor on the second revision, maybe Pro-series and UGEE v2 aren't so different after all! > > Have a look to "uclogic_rdesc_ugee_v2_frame_dial_template_arr", I don't > know if it could be used for your tablet. > Same as above, same structure - different order (but it's all semantic, same offsets) so I'll be consolidating if it works out. > > If your tablet reports its number of buttons, UCLOGIC_RDESC_FRAME_PH_BTN > can be used here. > I'll be reviewing the descriptor data, ideally it should but I don't have high hopes (as shown above) but I'm going to be testing it more. Thank you for the review! I'll be resubmitting a second version of the patch with your suggestions and making sure the kernel test bot is happy - and hopefully there will be a lot less duplication structure-wise.
