| Message ID | 20170831182004.24946-1-semen.protsenko@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] usb: gadget: g_dnl: Use serial# variable to set g_dnl_serial | expand |
On 31 August 2017 at 21:20, Sam Protsenko <semen.protsenko@linaro.org> wrote: > Since 842778a09104 commit, "fastboot devices" stopped to show correct > device serial number for TI boards, showing this line instead: > > ???????????? fastboot > > This is because serial# env variable could be set after g_dnl gadget was > initialized (e.g. by using env_set() in the board file). > > To fix this, before checking g_dnl_serial, let's re-check if we have > valid serial# value. And if so, let's set it as g_dnl_serial value. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > drivers/usb/gadget/g_dnl.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c > index 0491a0eea9..e4d0289757 100644 > --- a/drivers/usb/gadget/g_dnl.c > +++ b/drivers/usb/gadget/g_dnl.c > @@ -226,6 +226,15 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) > > g_dnl_bind_fixup(&device_desc, cdev->driver->name); > > + /* First try to obtain serial number from serial# variable */ > + if (strlen(g_dnl_serial) == 0) { > + const char *s; > + > + s = env_get("serial#"); > + if (s) > + g_dnl_set_serialnumber((char *)s); > + } > + > if (strlen(g_dnl_serial)) { > id = usb_string_id(cdev); > if (id < 0) > -- > 2.14.1 > Similar commit: [1]. But I'm not sure that implementing g_dnl_bind_fixup() in board file is the best way to fix it. [1] http://git.denx.de/?p=u-boot.git;a=commit;h=e91ead868b536d0a0dc49aa48f4d8c80c1a94b79
On 08/31/2017 08:45 PM, Sam Protsenko wrote: > On 31 August 2017 at 21:20, Sam Protsenko <semen.protsenko@linaro.org> wrote: >> Since 842778a09104 commit, "fastboot devices" stopped to show correct >> device serial number for TI boards, showing this line instead: >> >> ???????????? fastboot >> >> This is because serial# env variable could be set after g_dnl gadget was >> initialized (e.g. by using env_set() in the board file). >> >> To fix this, before checking g_dnl_serial, let's re-check if we have >> valid serial# value. And if so, let's set it as g_dnl_serial value. >> >> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >> --- >> drivers/usb/gadget/g_dnl.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c >> index 0491a0eea9..e4d0289757 100644 >> --- a/drivers/usb/gadget/g_dnl.c >> +++ b/drivers/usb/gadget/g_dnl.c >> @@ -226,6 +226,15 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) >> >> g_dnl_bind_fixup(&device_desc, cdev->driver->name); >> >> + /* First try to obtain serial number from serial# variable */ >> + if (strlen(g_dnl_serial) == 0) { >> + const char *s; >> + >> + s = env_get("serial#"); >> + if (s) >> + g_dnl_set_serialnumber((char *)s); >> + } >> + >> if (strlen(g_dnl_serial)) { >> id = usb_string_id(cdev); >> if (id < 0) >> -- >> 2.14.1 >> > > Similar commit: [1]. But I'm not sure that implementing > g_dnl_bind_fixup() in board file is the best way to fix it. > > [1] http://git.denx.de/?p=u-boot.git;a=commit;h=e91ead868b536d0a0dc49aa48f4d8c80c1a94b79 > I would opt for having g_dnl_bind_fixup() since it is generally used for tweaking board specific data to gadget subsystem (like vid, pid, serial number).
On 08/31/2017 08:20 PM, Sam Protsenko wrote: > Since 842778a09104 commit, "fastboot devices" stopped to show correct > device serial number for TI boards, showing this line instead: > > ???????????? fastboot > > This is because serial# env variable could be set after g_dnl gadget was > initialized (e.g. by using env_set() in the board file). > > To fix this, before checking g_dnl_serial, let's re-check if we have > valid serial# value. And if so, let's set it as g_dnl_serial value. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> Can't we use U_BOOT_ENV_CALLBACK() here instead and set the serial number when the environment variable is set ? > --- > drivers/usb/gadget/g_dnl.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c > index 0491a0eea9..e4d0289757 100644 > --- a/drivers/usb/gadget/g_dnl.c > +++ b/drivers/usb/gadget/g_dnl.c > @@ -226,6 +226,15 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) > > g_dnl_bind_fixup(&device_desc, cdev->driver->name); > > + /* First try to obtain serial number from serial# variable */ > + if (strlen(g_dnl_serial) == 0) { > + const char *s; > + > + s = env_get("serial#"); > + if (s) > + g_dnl_set_serialnumber((char *)s); > + } > + > if (strlen(g_dnl_serial)) { > id = usb_string_id(cdev); > if (id < 0) >
On 09/01/2017 10:19 AM, Marek Vasut wrote: > On 08/31/2017 08:20 PM, Sam Protsenko wrote: >> Since 842778a09104 commit, "fastboot devices" stopped to show correct >> device serial number for TI boards, showing this line instead: >> >> ???????????? fastboot >> >> This is because serial# env variable could be set after g_dnl gadget was >> initialized (e.g. by using env_set() in the board file). >> >> To fix this, before checking g_dnl_serial, let's re-check if we have >> valid serial# value. And if so, let's set it as g_dnl_serial value. >> >> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > Can't we use U_BOOT_ENV_CALLBACK() here instead and set the serial > number when the environment variable is set ? Hmm... This might work. However, I'm a bit concerned with having "bound" the one particular env variable ("serial#" in this case) to generic g_dnl.c code. > >> --- >> drivers/usb/gadget/g_dnl.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c >> index 0491a0eea9..e4d0289757 100644 >> --- a/drivers/usb/gadget/g_dnl.c >> +++ b/drivers/usb/gadget/g_dnl.c >> @@ -226,6 +226,15 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) >> >> g_dnl_bind_fixup(&device_desc, cdev->driver->name); >> >> + /* First try to obtain serial number from serial# variable */ >> + if (strlen(g_dnl_serial) == 0) { >> + const char *s; >> + >> + s = env_get("serial#"); >> + if (s) >> + g_dnl_set_serialnumber((char *)s); >> + } >> + >> if (strlen(g_dnl_serial)) { >> id = usb_string_id(cdev); >> if (id < 0) >> > >
On 09/01/2017 10:36 AM, Łukasz Majewski wrote: > On 09/01/2017 10:19 AM, Marek Vasut wrote: >> On 08/31/2017 08:20 PM, Sam Protsenko wrote: >>> Since 842778a09104 commit, "fastboot devices" stopped to show correct >>> device serial number for TI boards, showing this line instead: >>> >>> ???????????? fastboot >>> >>> This is because serial# env variable could be set after g_dnl gadget was >>> initialized (e.g. by using env_set() in the board file). >>> >>> To fix this, before checking g_dnl_serial, let's re-check if we have >>> valid serial# value. And if so, let's set it as g_dnl_serial value. >>> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >> >> Can't we use U_BOOT_ENV_CALLBACK() here instead and set the serial >> number when the environment variable is set ? > > Hmm... This might work. > > However, I'm a bit concerned with having "bound" the one particular env > variable ("serial#" in this case) to generic g_dnl.c code. That's a separate problem though, we already have this serial# variable in place.
On 09/01/2017 11:51 AM, Marek Vasut wrote: > On 09/01/2017 10:36 AM, Łukasz Majewski wrote: >> On 09/01/2017 10:19 AM, Marek Vasut wrote: >>> On 08/31/2017 08:20 PM, Sam Protsenko wrote: >>>> Since 842778a09104 commit, "fastboot devices" stopped to show correct >>>> device serial number for TI boards, showing this line instead: >>>> >>>> ???????????? fastboot >>>> >>>> This is because serial# env variable could be set after g_dnl gadget was >>>> initialized (e.g. by using env_set() in the board file). >>>> >>>> To fix this, before checking g_dnl_serial, let's re-check if we have >>>> valid serial# value. And if so, let's set it as g_dnl_serial value. >>>> >>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >>> >>> Can't we use U_BOOT_ENV_CALLBACK() here instead and set the serial >>> number when the environment variable is set ? >> >> Hmm... This might work. >> >> However, I'm a bit concerned with having "bound" the one particular env >> variable ("serial#" in this case) to generic g_dnl.c code. > That's a separate problem though, we already have this serial# variable > in place. Do you mean that it is the same of "fixed" variable, as "ipaddr" is for network subsytem? >
On 1 September 2017 at 11:19, Marek Vasut <marex@denx.de> wrote: > On 08/31/2017 08:20 PM, Sam Protsenko wrote: >> Since 842778a09104 commit, "fastboot devices" stopped to show correct >> device serial number for TI boards, showing this line instead: >> >> ???????????? fastboot >> >> This is because serial# env variable could be set after g_dnl gadget was >> initialized (e.g. by using env_set() in the board file). >> >> To fix this, before checking g_dnl_serial, let's re-check if we have >> valid serial# value. And if so, let's set it as g_dnl_serial value. >> >> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > Can't we use U_BOOT_ENV_CALLBACK() here instead and set the serial > number when the environment variable is set ? > Good point. I've tested this approach and it works fine. Will resend v2 shortly. Thanks for the hint! >> --- >> drivers/usb/gadget/g_dnl.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c >> index 0491a0eea9..e4d0289757 100644 >> --- a/drivers/usb/gadget/g_dnl.c >> +++ b/drivers/usb/gadget/g_dnl.c >> @@ -226,6 +226,15 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) >> >> g_dnl_bind_fixup(&device_desc, cdev->driver->name); >> >> + /* First try to obtain serial number from serial# variable */ >> + if (strlen(g_dnl_serial) == 0) { >> + const char *s; >> + >> + s = env_get("serial#"); >> + if (s) >> + g_dnl_set_serialnumber((char *)s); >> + } >> + >> if (strlen(g_dnl_serial)) { >> id = usb_string_id(cdev); >> if (id < 0) >> > > > -- > Best regards, > Marek Vasut
On 1 September 2017 at 14:58, Sam Protsenko <semen.protsenko@linaro.org> wrote: > On 1 September 2017 at 11:19, Marek Vasut <marex@denx.de> wrote: >> On 08/31/2017 08:20 PM, Sam Protsenko wrote: >>> Since 842778a09104 commit, "fastboot devices" stopped to show correct >>> device serial number for TI boards, showing this line instead: >>> >>> ???????????? fastboot >>> >>> This is because serial# env variable could be set after g_dnl gadget was >>> initialized (e.g. by using env_set() in the board file). >>> >>> To fix this, before checking g_dnl_serial, let's re-check if we have >>> valid serial# value. And if so, let's set it as g_dnl_serial value. >>> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >> >> Can't we use U_BOOT_ENV_CALLBACK() here instead and set the serial >> number when the environment variable is set ? >> > > Good point. I've tested this approach and it works fine. Will resend > v2 shortly. Thanks for the hint! > Sent v2, using U_BOOT_ENV_CALLBACK: [1]. Guys, please review. [1] https://lists.denx.de/pipermail/u-boot/2017-September/304460.html >>> --- >>> drivers/usb/gadget/g_dnl.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c >>> index 0491a0eea9..e4d0289757 100644 >>> --- a/drivers/usb/gadget/g_dnl.c >>> +++ b/drivers/usb/gadget/g_dnl.c >>> @@ -226,6 +226,15 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) >>> >>> g_dnl_bind_fixup(&device_desc, cdev->driver->name); >>> >>> + /* First try to obtain serial number from serial# variable */ >>> + if (strlen(g_dnl_serial) == 0) { >>> + const char *s; >>> + >>> + s = env_get("serial#"); >>> + if (s) >>> + g_dnl_set_serialnumber((char *)s); >>> + } >>> + >>> if (strlen(g_dnl_serial)) { >>> id = usb_string_id(cdev); >>> if (id < 0) >>> >> >> >> -- >> Best regards, >> Marek Vasut
diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c index 0491a0eea9..e4d0289757 100644 --- a/drivers/usb/gadget/g_dnl.c +++ b/drivers/usb/gadget/g_dnl.c @@ -226,6 +226,15 @@ static int g_dnl_bind(struct usb_composite_dev *cdev) g_dnl_bind_fixup(&device_desc, cdev->driver->name); + /* First try to obtain serial number from serial# variable */ + if (strlen(g_dnl_serial) == 0) { + const char *s; + + s = env_get("serial#"); + if (s) + g_dnl_set_serialnumber((char *)s); + } + if (strlen(g_dnl_serial)) { id = usb_string_id(cdev); if (id < 0)
Since 842778a09104 commit, "fastboot devices" stopped to show correct device serial number for TI boards, showing this line instead: ???????????? fastboot This is because serial# env variable could be set after g_dnl gadget was initialized (e.g. by using env_set() in the board file). To fix this, before checking g_dnl_serial, let's re-check if we have valid serial# value. And if so, let's set it as g_dnl_serial value. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/usb/gadget/g_dnl.c | 9 +++++++++ 1 file changed, 9 insertions(+)