Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fab763ac-2865-58b1-d735-51a28a76aa94@gmail.com>
Date:   Mon, 29 Aug 2022 10:22:28 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [RFC v1 1/2] HID: logitech-hidpp: Fix "Sw. Id." for HID++ 2.0
 commands
Content-Language: en-US
To:     Bastien Nocera <hadess@hadess.net>, linux-input@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        =?UTF-8?Q?Filipe_La=c3=adns?= <lains@riseup.net>,
        Nestor Lopez Casado <nlopezcasad@logitech.com>
References: <20220829134852.312548-1-hadess@hadess.net>
From:   "Peter F. Patel-Schneider" <pfpschneider@gmail.com>
In-Reply-To: <20220829134852.312548-1-hadess@hadess.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 8/29/22 09:48, Bastien Nocera wrote:
> Always set a non-zero "Sw. Id." in the lower nibble of the Function/ASE
> and Software Identifier byte in HID++ 2.0 commands.
>
> As per the "Protocol HID++2.0 essential features" section in
> https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf
> "
> Software identifier (4 bits, unsigned)
>
> A number uniquely defining the software that sends a request. The
> firmware must copy the software identifier in the response but does
> not use it in any other ways.
>
> 0 Do not use (allows to distinguish a notification from a response).
> "
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215699
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>   drivers/hid/hid-logitech-hidpp.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 86e7a38d8a9a..02f8c99672c7 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -41,6 +41,9 @@ module_param(disable_tap_to_click, bool, 0644);
>   MODULE_PARM_DESC(disable_tap_to_click,
>   	"Disable Tap-To-Click mode reporting for touchpads (only on the K400 currently).");
>   
> +/* Define a non-zero software ID to identify our own requests */
> +#define LINUX_KERNEL_SW_ID			0x06
> +
>   #define REPORT_ID_HIDPP_SHORT			0x10
>   #define REPORT_ID_HIDPP_LONG			0x11
>   #define REPORT_ID_HIDPP_VERY_LONG		0x12
> @@ -343,7 +346,7 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
>   	else
>   		message->report_id = REPORT_ID_HIDPP_LONG;
>   	message->fap.feature_index = feat_index;
> -	message->fap.funcindex_clientid = funcindex_clientid;
> +	message->fap.funcindex_clientid = funcindex_clientid | LINUX_KERNEL_SW_ID;
>   	memcpy(&message->fap.params, params, param_count);
>   
>   	ret = hidpp_send_message_sync(hidpp, message, response);


Looks good to me.  It might be better to use ID 0x01 to signifiy the "first" 
software but that is a minor quibble.


peter