LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Maxim Devaev <mdevaev@gmail.com>
To: "Maciej Żenczykowski" <maze@google.com>
Cc: balbi@kernel.org, stern@rowland.harvard.edu,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	ruslan.bilovol@gmail.com, mika.westerberg@linux.intel.com,
	jj251510319013@gmail.com, linux-usb@vger.kernel.org,
	Kernel hackers <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] usb: gadget: f_hid: optional SETUP/SET_REPORT mode
Date: Fri, 20 Aug 2021 17:36:32 +0300	[thread overview]
Message-ID: <20210820173632.053b1b2d@reki> (raw)
In-Reply-To: <CANP3RGejWk7Zj2XMGGPgrGMSjqRY+ZaVFha6jG720RCSF9HEkQ@mail.gmail.com>

Maciej Żenczykowski <maze@google.com> wrote:
> perhaps better to rephrase as 'the host ceases to change the status of
> the gadget/keyboard LEDs',
> unless this is actually driven by the keyboard as opposed to the other
> way round (which is what I'd expect from AT, PS/2 keyboards).

Since I'm describing the behavior on the gadget side,
it seemed to me that the explanation about the impossibility
of receiving it was more appropriate.

> Not clear what 'not poll' means here.  Why would they (the host) need
> to poll an OUT endpoint?

Poll the IN Endpoint, fixed

> Additionally it seems like any keyboard gadget would want to default
> to the older more compatible mode?
> Or are there compatibility problems with it as well?

Yes, any keyboard should use SETUP/SET_REPORT if it wants maximum
compatibility. This mode has no problems with hosts. I suggest it
as optional only because for the last 9 years the default behavior
has been OUT Endpoint and in the place of those people who use it,
I would be upset if this was changed, since it could lead to strange
problems like lack of a queue and loss of events if f_hid is used
for data transmission, and not for emulating input devices.

> if you look down below, this isn't actually dynamic, should we just
> have hidg_interface_desc_{intout,ssreport} structs?

DYNAMIC is indirectly provided by the no_out_endpoints flag.
I preferred to avoid duplicating the code here.

> may be better to just use an if (hidg->use_out_ep) status =
> usb_assign_descriptors(...) else status = usb_assign_descriptors(...)

Yep, you're right

> maybe it would be better to use consistent naming...
> 
> hidg->no_out_endpoint = opts->no_out_endpoint
> 
> or call the option 'use_ssreport' instead of 'no_out_endpoint'
> (negatives are harder to think about)

Yea, I also thought about it and made such a name precisely
based on consistency. The rest of the code contains the out_ep
variable, so it was logical to make the use_out_ep flag.
In addition, there are no long names like *_out_endpoint anywhere.
At the same time, abbreviations are not used in configfs
and I didn't want to make a flag there that the user should change
to 0 if he does not want to use out endpoint. I would prefer
to leave it as it is, because it does not use negation logic,
and it will be easy for you to grep something like out_ep\>

> Anyway, nothing in here is particularly important, just loose thoughts.
> In general this seems pretty nice.

Thank you for the review! I will fix this and make a third version of the patch.


      reply	other threads:[~2021-08-20 14:36 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-14  3:12 Maxim Devaev
2021-08-19 21:13 ` Maciej Żenczykowski
2021-08-20 14:36   ` Maxim Devaev [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210820173632.053b1b2d@reki \
    --to=mdevaev@gmail.com \
    --cc=balbi@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jj251510319013@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=maze@google.com \
    --cc=mika.westerberg@linux.intel.com \
    --cc=ruslan.bilovol@gmail.com \
    --cc=stern@rowland.harvard.edu \
    --subject='Re: [PATCH v2] usb: gadget: f_hid: optional SETUP/SET_REPORT mode' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).