[U-Boot] [PATCH] usb: kbd: don't fail with iomux (v3)

[Simon Glass]

Hi Rob,

On 13 August 2017 at 12:07, Rob Clark <robdclark at gmail.com> wrote:
> On Sun, Aug 13, 2017 at 11:35 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Rob,
>>
>> On 6 August 2017 at 05:58, Rob Clark <robdclark at gmail.com> wrote:
>>>
>>> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg at chromium.org> wrote:
>>> > On 4 August 2017 at 07:16, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> >> Hi Rob,
>>> >>
>>> >> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark <robdclark at gmail.com> wrote:
>>> >>> stdin might not be set, which would cause iomux_doenv() to fail
>>> >>> therefore causing probe_usb_keyboard() to fail.  Furthermore if we do
>>> >>> have iomux enabled, the sensible thing (in terms of user experience)
>>> >>> would be to simply add ourselves to the list of stdin devices.
>>> >>>
>>> >>> This fixes an issue with usbkbd on dragonboard410c with distro-
>>> >>> bootcmd, where stdin is not set (so stdinname is null).
>>> >>>
>>> >>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>> >>> ---
>>> >>> v2: address Bin's review comments
>>> >>> v3: fix fail with free()ing if usbkbd is already in stdin env variable
>>> >>>     pointed out by Simon
>>> >>>
>>> >>> (the real v3 this time)
>>> >>>
>>> >>
>>> >> As I mentioned, it's the email title, not the commit title with
>>> >> version number. If you prefer format-patch, then use
>>> >> --subject-prefix="PATCH v3"
>>> >>
>>> >>>  common/usb_kbd.c | 15 +++++++++++++++
>>> >>>  1 file changed, 15 insertions(+)
>>> >
>>> > Reviewed-by: Simon Glass <sjg at chromium.org>
>>> >
>>> > Question below
>>> >
>>> >>>
>>> >>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>>> >>> index d2d29cc98f..d71eae61ec 100644
>>> >>> --- a/common/usb_kbd.c
>>> >>> +++ b/common/usb_kbd.c
>>> >>> @@ -517,7 +517,22 @@ static int probe_usb_keyboard(struct usb_device *dev)
>>> >>>
>>> >>>         stdinname = getenv("stdin");
>>> >>>  #if CONFIG_IS_ENABLED(CONSOLE_MUX)
>>> >
>>> > Would this work:
>>> >
>>> > if (CONFIG_IS_ENABLED(CONSOLE_MUX) {
>>> >    ..
>>> > }
>>> >
>>> > The #ifdef adds a code path that is not tested, so if possible we
>>> > should try to use the compiler.
>>>
>>> I gave this a quick try.. it would require either adding an
>>> unconditional #include <iomux.h> in usb_kbd.c or dropping the #ifdef
>>> CONFIG_CONSOLE_MUX guarding the #include <iomux.h> in console.h.. not
>>> sure which is preferred?
>>
>> I don't think we should put #ifdefs around header files #includes so
>> the first option seems best. There are still some header files in
>> U-Boot which 'do stuff' like ensure that an option is set, etc. We
>> should over time fix those up.
>>
>> The #include in console.h seems wrong as well. It would be good to
>> move that to console.c
>> .
>> .>
>>> I suspect it would cause problems with -O0 but when I tried
>>> KCFLAGS=-O0 there were enough other unrelated compile errors, that I
>>> guess this isn't a legit use-case.
>>
>> Yes we have had that for a while. I don't think people use it anymore.
>>
>>>
>>> If you want I can send a v4 that uses "if (CONFIG_IS_ENABLED(...))"
>>> (or a separate patch on top.. in which case, do you mind removing the
>>> "(v3)" in $subject when you apply, or do you prefer I spam list with
>>> yet another resend?)  And in either case let me know what you prefer
>>> about iomux.h
>>
>> I think a v4 patch is best. But you should not have the version in the
>> subject there since it will end up in the commit. If you use patman it
>> will produce the patch correctly.
>>
>
> fwiw, I did already send a v4 which went with the first option..

OK. I can't seem to find it - can you please give me a patchwork link?

Thanks,
Simon