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

[Rob Clark]

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..

BR,
-R