Re: [U-Boot] [PATCH] usb: kbd: Don't fail with iomux

[Simon Glass]

+Tom

On 26 September 2017 at 19:19, Rob Clark  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 
> ---
> Somehow this patch was dropped on the floor.  I don't remember
> which version # this is up to, search the list if you care.  But
> this is the latest.  I only noticed it was missing because u-boot
> crashes when you boot with usb-keyboard plugged in (at least on
> db410c) without it.  So someone please apply this patch before it
> gets lost again.
>
>  common/usb_kbd.c  | 46 +++---
>  include/console.h |  2 --
>  2 files changed, 31 insertions(+), 17 deletions(-)

Reviewed-by: Simon Glass 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: kbd: Don't fail with iomux

[Bin Meng]

On Wed, Sep 27, 2017 at 9:19 AM, Rob Clark  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 
> ---
> Somehow this patch was dropped on the floor.  I don't remember
> which version # this is up to, search the list if you care.  But
> this is the latest.  I only noticed it was missing because u-boot
> crashes when you boot with usb-keyboard plugged in (at least on
> db410c) without it.  So someone please apply this patch before it
> gets lost again.
>
>  common/usb_kbd.c  | 46 +++---
>  include/console.h |  2 --
>  2 files changed, 31 insertions(+), 17 deletions(-)
>

Reviewed-by: Bin Meng 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: kbd: Don't fail with iomux

[Peter Robinson]

On Wed, Sep 27, 2017 at 2:19 AM, Rob Clark  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 
Tested-by: Peter Robinson 

Tested on RPi3 and a couple of other devices.

> ---
> Somehow this patch was dropped on the floor.  I don't remember
> which version # this is up to, search the list if you care.  But
> this is the latest.  I only noticed it was missing because u-boot
> crashes when you boot with usb-keyboard plugged in (at least on
> db410c) without it.  So someone please apply this patch before it
> gets lost again.
>
>  common/usb_kbd.c  | 46 +++---
>  include/console.h |  2 --
>  2 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index a323d72a36..4c3ad95fca 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -516,23 +516,39 @@ static int probe_usb_keyboard(struct usb_device *dev)
> return error;
>
> stdinname = env_get("stdin");
> -#if CONFIG_IS_ENABLED(CONSOLE_MUX)
> -   error = iomux_doenv(stdin, stdinname);
> -   if (error)
> -   return error;
> -#else
> -   /* Check if this is the standard input device. */
> -   if (strcmp(stdinname, DEVNAME))
> -   return 1;
> +   if (CONFIG_IS_ENABLED(CONSOLE_MUX)) {
> +   char *devname = DEVNAME;
> +   char *newstdin = NULL;
> +   /*
> +* stdin might not be set yet.. either way, with console-
> +* mux the sensible thing to do is add ourselves to the
> +* list of stdio devices:
> +*/
> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
> +   newstdin = malloc(strlen(stdinname) +
> +   strlen(","DEVNAME) + 1);
> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
> +   stdinname = newstdin;
> +   } else if (!stdinname) {
> +   stdinname = devname;
> +   }
> +   error = iomux_doenv(stdin, stdinname);
> +   free(newstdin);
> +   if (error)
> +   return error;
> +   } else {
> +   /* Check if this is the standard input device. */
> +   if (strcmp(stdinname, DEVNAME))
> +   return 1;
>
> -   /* Reassign the console */
> -   if (overwrite_console())
> -   return 1;
> +   /* Reassign the console */
> +   if (overwrite_console())
> +   return 1;
>
> -   error = console_assign(stdin, DEVNAME);
> -   if (error)
> -   return error;
> -#endif
> +   error = console_assign(stdin, DEVNAME);
> +   if (error)
> +   return error;
> +   }
>
> return 0;
>  }
> diff --git a/include/console.h b/include/console.h
> index cea29ed6dc..7dfd36d7d1 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -57,8 +57,6 @@ int console_announce_r(void);
>  /*
>   * CONSOLE multiplexing.
>   */
> -#ifdef CONFIG_CONSOLE_MUX
>  #include 
> -#endif
>
>  #endif
> --
> 2.13.5
>
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] usb: kbd: Don't fail with iomux

[Rob Clark]

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 
---
Somehow this patch was dropped on the floor.  I don't remember
which version # this is up to, search the list if you care.  But
this is the latest.  I only noticed it was missing because u-boot
crashes when you boot with usb-keyboard plugged in (at least on
db410c) without it.  So someone please apply this patch before it
gets lost again.

 common/usb_kbd.c  | 46 +++---
 include/console.h |  2 --
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index a323d72a36..4c3ad95fca 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -516,23 +516,39 @@ static int probe_usb_keyboard(struct usb_device *dev)
return error;
 
stdinname = env_get("stdin");
-#if CONFIG_IS_ENABLED(CONSOLE_MUX)
-   error = iomux_doenv(stdin, stdinname);
-   if (error)
-   return error;
-#else
-   /* Check if this is the standard input device. */
-   if (strcmp(stdinname, DEVNAME))
-   return 1;
+   if (CONFIG_IS_ENABLED(CONSOLE_MUX)) {
+   char *devname = DEVNAME;
+   char *newstdin = NULL;
+   /*
+* stdin might not be set yet.. either way, with console-
+* mux the sensible thing to do is add ourselves to the
+* list of stdio devices:
+*/
+   if (stdinname && !strstr(stdinname, DEVNAME)) {
+   newstdin = malloc(strlen(stdinname) +
+   strlen(","DEVNAME) + 1);
+   sprintf(newstdin, "%s,"DEVNAME, stdinname);
+   stdinname = newstdin;
+   } else if (!stdinname) {
+   stdinname = devname;
+   }
+   error = iomux_doenv(stdin, stdinname);
+   free(newstdin);
+   if (error)
+   return error;
+   } else {
+   /* Check if this is the standard input device. */
+   if (strcmp(stdinname, DEVNAME))
+   return 1;
 
-   /* Reassign the console */
-   if (overwrite_console())
-   return 1;
+   /* Reassign the console */
+   if (overwrite_console())
+   return 1;
 
-   error = console_assign(stdin, DEVNAME);
-   if (error)
-   return error;
-#endif
+   error = console_assign(stdin, DEVNAME);
+   if (error)
+   return error;
+   }
 
return 0;
 }
diff --git a/include/console.h b/include/console.h
index cea29ed6dc..7dfd36d7d1 100644
--- a/include/console.h
+++ b/include/console.h
@@ -57,8 +57,6 @@ int console_announce_r(void);
 /*
  * CONSOLE multiplexing.
  */
-#ifdef CONFIG_CONSOLE_MUX
 #include 
-#endif
 
 #endif
-- 
2.13.5

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Simon Glass]

Hi Rob,

On 13 August 2017 at 12:07, Rob Clark  wrote:
> On Sun, Aug 13, 2017 at 11:35 AM, Simon Glass  wrote:
>> Hi Rob,
>>
>> On 6 August 2017 at 05:58, Rob Clark  wrote:
>>>
>>> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass  wrote:
>>> > On 4 August 2017 at 07:16, Bin Meng  wrote:
>>> >> Hi Rob,
>>> >>
>>> >> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark  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 
>>> >>> ---
>>> >>> 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 
>>> >
>>> > 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  in usb_kbd.c or dropping the #ifdef
>>> CONFIG_CONSOLE_MUX guarding the #include  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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rob Clark]

On Sun, Aug 13, 2017 at 11:35 AM, Simon Glass  wrote:
> Hi Rob,
>
> On 6 August 2017 at 05:58, Rob Clark  wrote:
>>
>> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass  wrote:
>> > On 4 August 2017 at 07:16, Bin Meng  wrote:
>> >> Hi Rob,
>> >>
>> >> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark  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 
>> >>> ---
>> >>> 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 
>> >
>> > 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  in usb_kbd.c or dropping the #ifdef
>> CONFIG_CONSOLE_MUX guarding the #include  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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Simon Glass]

Hi Rob,

On 6 August 2017 at 05:58, Rob Clark  wrote:
>
> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass  wrote:
> > On 4 August 2017 at 07:16, Bin Meng  wrote:
> >> Hi Rob,
> >>
> >> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark  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 
> >>> ---
> >>> 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 
> >
> > 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  in usb_kbd.c or dropping the #ifdef
> CONFIG_CONSOLE_MUX guarding the #include  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.

>
> BR,
> -R
>
> >>> +   char *devname = DEVNAME;
> >>> +   char *newstdin = NULL;
> >>> +   /*
> >>> +* stdin might not be set yet.. either way, with console-mux the
> >>> +* sensible thing to do is add ourselves to the list of stdio
> >>> +* devices:
> >>> +*/
> >>> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
> >>> +   newstdin = malloc(strlen(stdinname) + strlen(","DEVNAME) 
> >>> + 1);
> >>> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
> >>> +   stdinname = newstdin;
> >>> +   } else if (!stdinname) {
> >>> +   stdinname = devname;
> >>> +   }
> >>> error = iomux_doenv(stdin, stdinname);
> >>> +   free(newstdin);
> >>> if (error)
> >>> return error;
> >>>  #else
> >>> --
> >
> > Regards,
> > Simon


Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rob Clark]

On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass  wrote:
> On 4 August 2017 at 07:16, Bin Meng  wrote:
>> Hi Rob,
>>
>> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark  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 
>>> ---
>>> 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 
>
> 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  in usb_kbd.c or dropping the #ifdef
CONFIG_CONSOLE_MUX guarding the #include  in console.h.. not
sure which is preferred?

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.

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

BR,
-R

>>> +   char *devname = DEVNAME;
>>> +   char *newstdin = NULL;
>>> +   /*
>>> +* stdin might not be set yet.. either way, with console-mux the
>>> +* sensible thing to do is add ourselves to the list of stdio
>>> +* devices:
>>> +*/
>>> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
>>> +   newstdin = malloc(strlen(stdinname) + strlen(","DEVNAME) + 
>>> 1);
>>> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
>>> +   stdinname = newstdin;
>>> +   } else if (!stdinname) {
>>> +   stdinname = devname;
>>> +   }
>>> error = iomux_doenv(stdin, stdinname);
>>> +   free(newstdin);
>>> if (error)
>>> return error;
>>>  #else
>>> --
>
> Regards,
> Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Bin Meng]

Hi Rob,

On Sun, Aug 6, 2017 at 6:41 PM, Rob Clark  wrote:
> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass  wrote:
>> On 4 August 2017 at 07:16, Bin Meng  wrote:
>>> Hi Rob,
>>>
>>> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark  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 
 ---
 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 
>>
>> 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 think it would, except maybe if someone compiled w/ -O0 (unresolved
> symbols at link time).. not sure if that is something we care about?

Can you please resend with a commit title without (v3) but email title with v3?

Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rob Clark]

On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass  wrote:
> On 4 August 2017 at 07:16, Bin Meng  wrote:
>> Hi Rob,
>>
>> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark  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 
>>> ---
>>> 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 
>
> 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 think it would, except maybe if someone compiled w/ -O0 (unresolved
symbols at link time).. not sure if that is something we care about?

BR,
-R

>>> +   char *devname = DEVNAME;
>>> +   char *newstdin = NULL;
>>> +   /*
>>> +* stdin might not be set yet.. either way, with console-mux the
>>> +* sensible thing to do is add ourselves to the list of stdio
>>> +* devices:
>>> +*/
>>> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
>>> +   newstdin = malloc(strlen(stdinname) + strlen(","DEVNAME) + 
>>> 1);
>>> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
>>> +   stdinname = newstdin;
>>> +   } else if (!stdinname) {
>>> +   stdinname = devname;
>>> +   }
>>> error = iomux_doenv(stdin, stdinname);
>>> +   free(newstdin);
>>> if (error)
>>> return error;
>>>  #else
>>> --
>
> Regards,
> Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Simon Glass]

On 4 August 2017 at 07:16, Bin Meng  wrote:
> Hi Rob,
>
> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark  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 
>> ---
>> 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 

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.

>> +   char *devname = DEVNAME;
>> +   char *newstdin = NULL;
>> +   /*
>> +* stdin might not be set yet.. either way, with console-mux the
>> +* sensible thing to do is add ourselves to the list of stdio
>> +* devices:
>> +*/
>> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
>> +   newstdin = malloc(strlen(stdinname) + strlen(","DEVNAME) + 
>> 1);
>> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
>> +   stdinname = newstdin;
>> +   } else if (!stdinname) {
>> +   stdinname = devname;
>> +   }
>> error = iomux_doenv(stdin, stdinname);
>> +   free(newstdin);
>> if (error)
>> return error;
>>  #else
>> --

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Bin Meng]

Hi Rob,

On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark  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 
> ---
> 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(+)
>
> 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)
> +   char *devname = DEVNAME;
> +   char *newstdin = NULL;
> +   /*
> +* stdin might not be set yet.. either way, with console-mux the
> +* sensible thing to do is add ourselves to the list of stdio
> +* devices:
> +*/
> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
> +   newstdin = malloc(strlen(stdinname) + strlen(","DEVNAME) + 1);
> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
> +   stdinname = newstdin;
> +   } else if (!stdinname) {
> +   stdinname = devname;
> +   }
> error = iomux_doenv(stdin, stdinname);
> +   free(newstdin);
> if (error)
> return error;
>  #else
> --

Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: kbd: don't fail with iomux

[Bin Meng]

Hi Rob,

On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark  wrote:
> On Fri, Aug 4, 2017 at 8:35 AM, Bin Meng  wrote:
>> Hi Rob,
>>
>> On Fri, Aug 4, 2017 at 7:52 PM, Rob Clark  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 
>>> ---
>>> v2: address Bin's review comments
>>> v3: fix fail with free()ing if usbkbd is already in stdin env variable
>>> pointed out by Simon
>>>
>>
>> Can you please indicate the version number in the email title next
>> time? It's really hard to track which version the patch is.
>
> ok, I had assumed u-boot followed linux kernel conventions of history
> below the "---" so it doesn't end up in git history.

Yes, U-Boot follows the same Linux kernel conventions with regard to
patch submission. What I was saying is not the commit title. It's the
email title, something like "[PATCH v3] usb: kbd: don't fail with
iomux"

>
>>>  common/usb_kbd.c | 15 +++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>>> index d2d29cc98f..703dd748f5 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)
>>> +   char *devname = DEVNAME;
>>> +   /*
>>> +* stdin might not be set yet.. either way, with console-mux the
>>> +* sensible thing to do is add ourselves to the list of stdio
>>> +* devices:
>>> +*/
>>> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
>>> +   char *newstdin = malloc(strlen(stdinname) + 
>>> strlen(","DEVNAME) + 1);
>>> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
>>> +   stdinname = newstdin;
>>> +   } else if (!stdinname) {
>>> +   stdinname = devname;
>>> +   }
>>> error = iomux_doenv(stdin, stdinname);
>>> +   if (stdinname != devname)
>>> +   free(stdinname);
>>> if (error)
>>> return error;
>>>  #else
>>
>> And what's the difference between this "v3" and previous "v2"?
>>
>> Previous v2 is here: http://patchwork.ozlabs.org/patch/797418/
>>
>
> Nothing because I apparently passed the old commit sha to
> git-format-patch again.  Sorry about that.  (I guess I'm doing too
> many things at once with trying to finalize a big efi_loader patchset
> in parallel with bisecting a couple remaining nonsensical travis
> failures as well as updating and resending these various other
> unrelated patches.)
>

You really should start trying patman instead of git-format-patch :)

> I'll resend the real v3 in a moment.
>

Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rob Clark]

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

 common/usb_kbd.c | 15 +++
 1 file changed, 15 insertions(+)

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)
+   char *devname = DEVNAME;
+   char *newstdin = NULL;
+   /*
+* stdin might not be set yet.. either way, with console-mux the
+* sensible thing to do is add ourselves to the list of stdio
+* devices:
+*/
+   if (stdinname && !strstr(stdinname, DEVNAME)) {
+   newstdin = malloc(strlen(stdinname) + strlen(","DEVNAME) + 1);
+   sprintf(newstdin, "%s,"DEVNAME, stdinname);
+   stdinname = newstdin;
+   } else if (!stdinname) {
+   stdinname = devname;
+   }
error = iomux_doenv(stdin, stdinname);
+   free(newstdin);
if (error)
return error;
 #else
-- 
2.13.0

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: kbd: don't fail with iomux

[Rob Clark]

On Fri, Aug 4, 2017 at 8:35 AM, Bin Meng  wrote:
> Hi Rob,
>
> On Fri, Aug 4, 2017 at 7:52 PM, Rob Clark  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 
>> ---
>> v2: address Bin's review comments
>> v3: fix fail with free()ing if usbkbd is already in stdin env variable
>> pointed out by Simon
>>
>
> Can you please indicate the version number in the email title next
> time? It's really hard to track which version the patch is.

ok, I had assumed u-boot followed linux kernel conventions of history
below the "---" so it doesn't end up in git history.

>>  common/usb_kbd.c | 15 +++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>> index d2d29cc98f..703dd748f5 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)
>> +   char *devname = DEVNAME;
>> +   /*
>> +* stdin might not be set yet.. either way, with console-mux the
>> +* sensible thing to do is add ourselves to the list of stdio
>> +* devices:
>> +*/
>> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
>> +   char *newstdin = malloc(strlen(stdinname) + 
>> strlen(","DEVNAME) + 1);
>> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
>> +   stdinname = newstdin;
>> +   } else if (!stdinname) {
>> +   stdinname = devname;
>> +   }
>> error = iomux_doenv(stdin, stdinname);
>> +   if (stdinname != devname)
>> +   free(stdinname);
>> if (error)
>> return error;
>>  #else
>
> And what's the difference between this "v3" and previous "v2"?
>
> Previous v2 is here: http://patchwork.ozlabs.org/patch/797418/
>

Nothing because I apparently passed the old commit sha to
git-format-patch again.  Sorry about that.  (I guess I'm doing too
many things at once with trying to finalize a big efi_loader patchset
in parallel with bisecting a couple remaining nonsensical travis
failures as well as updating and resending these various other
unrelated patches.)

I'll resend the real v3 in a moment.

BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: kbd: don't fail with iomux

[Bin Meng]

Hi Rob,

On Fri, Aug 4, 2017 at 7:52 PM, Rob Clark  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 
> ---
> v2: address Bin's review comments
> v3: fix fail with free()ing if usbkbd is already in stdin env variable
> pointed out by Simon
>

Can you please indicate the version number in the email title next
time? It's really hard to track which version the patch is.

>  common/usb_kbd.c | 15 +++
>  1 file changed, 15 insertions(+)
>
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index d2d29cc98f..703dd748f5 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)
> +   char *devname = DEVNAME;
> +   /*
> +* stdin might not be set yet.. either way, with console-mux the
> +* sensible thing to do is add ourselves to the list of stdio
> +* devices:
> +*/
> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
> +   char *newstdin = malloc(strlen(stdinname) + 
> strlen(","DEVNAME) + 1);
> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
> +   stdinname = newstdin;
> +   } else if (!stdinname) {
> +   stdinname = devname;
> +   }
> error = iomux_doenv(stdin, stdinname);
> +   if (stdinname != devname)
> +   free(stdinname);
> if (error)
> return error;
>  #else

And what's the difference between this "v3" and previous "v2"?

Previous v2 is here: http://patchwork.ozlabs.org/patch/797418/

Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rob Clark]

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 
---
v2: address Bin's review comments
v3: fix fail with free()ing if usbkbd is already in stdin env variable
pointed out by Simon

 common/usb_kbd.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index d2d29cc98f..703dd748f5 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)
+   char *devname = DEVNAME;
+   /*
+* stdin might not be set yet.. either way, with console-mux the
+* sensible thing to do is add ourselves to the list of stdio
+* devices:
+*/
+   if (stdinname && !strstr(stdinname, DEVNAME)) {
+   char *newstdin = malloc(strlen(stdinname) + strlen(","DEVNAME) 
+ 1);
+   sprintf(newstdin, "%s,"DEVNAME, stdinname);
+   stdinname = newstdin;
+   } else if (!stdinname) {
+   stdinname = devname;
+   }
error = iomux_doenv(stdin, stdinname);
+   if (stdinname != devname)
+   free(stdinname);
if (error)
return error;
 #else
-- 
2.13.0

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: kbd: don't fail with iomux

[Bin Meng]

On Fri, Aug 4, 2017 at 3:22 AM, Rob Clark  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 
> ---
>  common/usb_kbd.c | 15 +++
>  1 file changed, 15 insertions(+)
>

Reviewed-by: Bin Meng 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: kbd: don't fail with iomux

[Rob Clark]

On Thu, Aug 3, 2017 at 3:37 PM, Simon Glass  wrote:
> Hi Rob,
>
> On 3 August 2017 at 13:22, Rob Clark  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).
>
> Is this version 2?

yes, this is the actual v2 (note the fake v2 I sent earlier today)

>>
>> Signed-off-by: Rob Clark 
>> ---
>>  common/usb_kbd.c | 15 +++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>> index d2d29cc98f..703dd748f5 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)
>> +   char *devname = DEVNAME;
>> +   /*
>> +* stdin might not be set yet.. either way, with console-mux the
>> +* sensible thing to do is add ourselves to the list of stdio
>> +* devices:
>> +*/
>> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
>> +   char *newstdin = malloc(strlen(stdinname) + 
>> strlen(","DEVNAME) + 1);
>> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
>> +   stdinname = newstdin;
>> +   } else if (!stdinname) {
>> +   stdinname = devname;
>> +   }
>> error = iomux_doenv(stdin, stdinname);
>> +   if (stdinname != devname)
>> +   free(stdinname);
>
> Are you sure that free() will work? It looks like it is intended to
> free newstdin. I think it would be better to have a boolean for
> whether to free(), instead.

oh, that's true.. on v1 you would always go down one or the other leg
of the if/else, which is no longer the case

BR,
-R

>> if (error)
>> return error;
>>  #else
>> --
>> 2.13.0
>>
>
> Regards,
> Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: kbd: don't fail with iomux

[Simon Glass]

Hi Rob,

On 3 August 2017 at 13:22, Rob Clark  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).

Is this version 2?

>
> Signed-off-by: Rob Clark 
> ---
>  common/usb_kbd.c | 15 +++
>  1 file changed, 15 insertions(+)
>
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index d2d29cc98f..703dd748f5 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)
> +   char *devname = DEVNAME;
> +   /*
> +* stdin might not be set yet.. either way, with console-mux the
> +* sensible thing to do is add ourselves to the list of stdio
> +* devices:
> +*/
> +   if (stdinname && !strstr(stdinname, DEVNAME)) {
> +   char *newstdin = malloc(strlen(stdinname) + 
> strlen(","DEVNAME) + 1);
> +   sprintf(newstdin, "%s,"DEVNAME, stdinname);
> +   stdinname = newstdin;
> +   } else if (!stdinname) {
> +   stdinname = devname;
> +   }
> error = iomux_doenv(stdin, stdinname);
> +   if (stdinname != devname)
> +   free(stdinname);

Are you sure that free() will work? It looks like it is intended to
free newstdin. I think it would be better to have a boolean for
whether to free(), instead.

> if (error)
> return error;
>  #else
> --
> 2.13.0
>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rob Clark]

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 
---
 common/usb_kbd.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index d2d29cc98f..703dd748f5 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)
+   char *devname = DEVNAME;
+   /*
+* stdin might not be set yet.. either way, with console-mux the
+* sensible thing to do is add ourselves to the list of stdio
+* devices:
+*/
+   if (stdinname && !strstr(stdinname, DEVNAME)) {
+   char *newstdin = malloc(strlen(stdinname) + strlen(","DEVNAME) 
+ 1);
+   sprintf(newstdin, "%s,"DEVNAME, stdinname);
+   stdinname = newstdin;
+   } else if (!stdinname) {
+   stdinname = devname;
+   }
error = iomux_doenv(stdin, stdinname);
+   if (stdinname != devname)
+   free(stdinname);
if (error)
return error;
 #else
-- 
2.13.0

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rob Clark]

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.

Signed-off-by: Rob Clark 
---
 common/usb_kbd.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index d2d29cc..72cf78a 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -517,7 +517,21 @@ static int probe_usb_keyboard(struct usb_device *dev)
 
stdinname = getenv("stdin");
 #if CONFIG_IS_ENABLED(CONSOLE_MUX)
+   char *devname = DEVNAME;
+   /* stdin might not be set yet.. either way, with console-mux the
+* sensible thing to do is add ourselves to the list of stdio
+* devices:
+*/
+   if (stdinname) {
+   char *newstdin = malloc(strlen(stdinname) + strlen(","DEVNAME) 
+ 1);
+   sprintf(newstdin, "%s,"DEVNAME, stdinname);
+   stdinname = newstdin;
+   } else {
+   stdinname = devname;
+   }
error = iomux_doenv(stdin, stdinname);
+   if (stdinname != devname)
+   free(stdinname);
if (error)
return error;
 #else
-- 
2.9.4

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot