From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E3113C4167D for ; Wed, 1 Nov 2023 08:45:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232517AbjKAIp0 (ORCPT ); Wed, 1 Nov 2023 04:45:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35558 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231420AbjKAIpX (ORCPT ); Wed, 1 Nov 2023 04:45:23 -0400 Received: from todd.t-8ch.de (todd.t-8ch.de [IPv6:2a01:4f8:c010:41de::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 67CAEA2; Wed, 1 Nov 2023 01:45:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=t-8ch.de; s=mail; t=1698828315; bh=dUn69NpLHm95YlSQflbcp4YFtXVEb5UF3XD5Lg1d5gg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=O/NmY7tTzxPyPRa6lTNocgFIqOJjvGDfR88RyqlmXkdEZ7KnUY4FVhnBV3jXO8kEI M2K26fSQ1MFk/XD4lCApNP3GBO+iQMfsLYdH+097QrWsdV0BaKT/kvH3dglzSYgE7e +x288rUcOvxWX4XNS3TIhOxl1Pwtol6ggbdpS4rA= Date: Wed, 1 Nov 2023 09:45:14 +0100 From: Thomas =?utf-8?Q?Wei=C3=9Fschuh?= To: Anshul Dalal Cc: linux-input@vger.kernel.org, devicetree@vger.kernel.org, Dmitry Torokhov , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Jeff LaBundy , Shuah Khan , linux-kernel-mentees@lists.linuxfoundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad Message-ID: <2241e06f-ef90-4299-97cd-cfb8fd569065@t-8ch.de> References: <20231027051819.81333-1-anshulusr@gmail.com> <20231027051819.81333-2-anshulusr@gmail.com> <9c9f6171-f879-46f5-81d2-6764257a49eb@t-8ch.de> <152d15c4-10bb-45fe-9b9c-b323535a921f@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <152d15c4-10bb-45fe-9b9c-b323535a921f@gmail.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Anshul, On 2023-11-01 09:50:36+0530, Anshul Dalal wrote: > On 10/31/23 07:53, Thomas Weißschuh wrote: > > Oct 31, 2023 03:10:50 Anshul Dalal : > >> Thanks for the review! The requested changes will be addressed in the > >> next patch version though I had a few comments below: > >> > >> On 10/27/23 11:44, Thomas Weißschuh wrote: > >>> Hi Anshul, > >>> > >>> thanks for the reworks! > >>> > >>> Some more comments inline. > >>> > >>> On 2023-10-27 10:48:11+0530, Anshul Dalal wrote: > > > > [..] > > > >>>> +struct seesaw_button_description { > >>>> +   unsigned int code; > >>>> +   unsigned int bit; > >>>> +}; > >>>> + > >>>> +static const struct seesaw_button_description seesaw_buttons[] = { > >>>> +   { > >>>> +       .code = BTN_EAST, > >>>> +       .bit = SEESAW_BUTTON_A, > >>>> +   }, > >>>> +   { > >>>> +       .code = BTN_SOUTH, > >>>> +       .bit = SEESAW_BUTTON_B, > >>>> +   }, > >>>> +   { > >>>> +       .code = BTN_NORTH, > >>>> +       .bit = SEESAW_BUTTON_X, > >>>> +   }, > >>>> +   { > >>>> +       .code = BTN_WEST, > >>>> +       .bit = SEESAW_BUTTON_Y, > >>>> +   }, > >>>> +   { > >>>> +       .code = BTN_START, > >>>> +       .bit = SEESAW_BUTTON_START, > >>>> +   }, > >>>> +   { > >>>> +       .code = BTN_SELECT, > >>>> +       .bit = SEESAW_BUTTON_SELECT, > >>>> +   }, > >>>> +}; > >>> > >>> This looks very much like a sparse keymap which can be implemented with > >>> the helpers from . > >>> > >> > >> When going through the API provided by sparse-keymap, I could only see > >> the use for sparse_keymap_report_entry here. Which leads to the > >> following refactored code: > >> > >> static const struct key_entry seesaw_buttons_new[] = { > >>     {KE_KEY, SEESAW_BUTTON_A, {BTN_SOUTH}}, > >>     {KE_KEY, SEESAW_BUTTON_B, {BTN_EAST}}, > > > > No braces I think. > > > > Since the last field in key_entry is a union, the braces seem to be > required. Indeed. To make the union more visible explicit this could be done: { KE_KEY, SEESAW_BUTTON_A, .keycode = BTN_SOUTH } > > >>     ... > >> }; > >> > >> for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) { > >>     sparse_keymap_report_entry(input, &seesaw_buttons_new[i], > >>         data.button_state & BIT(seesaw_buttons_new[i].code), > >>         false); > >> } > >> > >> I don't think this significantly improves the code unless you had some > >> other way to use the API in mind. > > > > I thought about sparse_keymap_setup() and sparse_keymap_report_event(). > > > > It does not significantly change the code but would be a standard API. > > > > Thanks for pointing me in the right direction, do you think the > following implementation of the API is acceptable for the driver. Since > I couldn't find a driver for any similar device using the API in this > manner. > > inside seesaw_probe(): > > err = sparse_keymap_setup(seesaw->input_dev, seesaw_buttons_new, NULL); > if (err) { > dev_err(&client->dev, > "failed to set up input device keymap: %d\n", err); > return err; > } Yes, and it replaces the calls to input_set_capability(). > inside seesaw_poll(): > > for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) { > if (!sparse_keymap_report_event( > input, seesaw_buttons_new[i].code, > data.button_state & BIT(seesaw_buttons_new[i].code), > false)) { > dev_err_ratelimited( > &input->dev, > "failed to report event for keycode: %d", > seesaw_buttons_new[i].keycode); > return; > } > } for_each_set_bit(i, (long *)&SEESAW_BUTTON_MASK, BITS_PER_TYPE(SEESAW_BUTTON_MASK)) sparse_keymap_report_event(input, BIT(i), data.button_state & BIT(i), false); The sparse keymap takes care of the translation. Notes: SEESAW_BUTTON_MASK is now an actual variable instead of a macro. It should be 'static const' in that case. When using the sparse keymap APIs the driver also needs to depend on INPUT_SPARSEKMAP. Thomas