[U-Boot] config USB_STORAGE: defconfig vs include header

[U-Boot] config USB_STORAGE: defconfig vs include header

[Stephen Warren]

Masahiro,

In patch 6e7e9294d321 "usb: add basic USB configs in Kconfig", you added 
"config USB_STORAGE" to drivers/usb/Kconfig. However, it's still just 
#defined by many include/configs/*.h rather than being defined in 
configs/*_defconfig. Is that a problem? It seems to work in practice, 
but leads people adding new boards to put the definition in 
configs/*_defconfig which then may be inconsistent with similar existing 
boards which have it defined in include/configs/*.h.

[U-Boot] config USB_STORAGE: defconfig vs include header

[Masahiro Yamada]

Hi Stephen


2016-09-08 1:15 GMT+09:00 Stephen Warren <swarren@wwwdotorg.org>:
> Masahiro,
>
> In patch 6e7e9294d321 "usb: add basic USB configs in Kconfig", you added
> "config USB_STORAGE" to drivers/usb/Kconfig. However, it's still just
> #defined by many include/configs/*.h rather than being defined in
> configs/*_defconfig. Is that a problem? It seems to work in practice, but
> leads people adding new boards to put the definition in configs/*_defconfig
> which then may be inconsistent with similar existing boards which have it
> defined in include/configs/*.h.

Once we create an entry in Kconfig,
all the defines in include/configs/*.h should be moved.





-- 
Best Regards
Masahiro Yamada

[U-Boot] config USB_STORAGE: defconfig vs include header

[Stephen Warren]

On 09/07/2016 07:29 PM, Masahiro Yamada wrote:
> Hi Stephen
>
>
> 2016-09-08 1:15 GMT+09:00 Stephen Warren <swarren@wwwdotorg.org>:
>> Masahiro,
>>
>> In patch 6e7e9294d321 "usb: add basic USB configs in Kconfig", you added
>> "config USB_STORAGE" to drivers/usb/Kconfig. However, it's still just
>> #defined by many include/configs/*.h rather than being defined in
>> configs/*_defconfig. Is that a problem? It seems to work in practice, but
>> leads people adding new boards to put the definition in configs/*_defconfig
>> which then may be inconsistent with similar existing boards which have it
>> defined in include/configs/*.h.
>
> Once we create an entry in Kconfig,
> all the defines in include/configs/*.h should be moved.

That's what I imagined. The commit above didn't do that though; are you 
planning on sending a fix?

[U-Boot] config USB_STORAGE: defconfig vs include header

[Tom Rini]

On Thu, Sep 08, 2016 at 09:58:13AM -0600, Stephen Warren wrote:
> On 09/07/2016 07:29 PM, Masahiro Yamada wrote:
> >Hi Stephen
> >
> >
> >2016-09-08 1:15 GMT+09:00 Stephen Warren <swarren@wwwdotorg.org>:
> >>Masahiro,
> >>
> >>In patch 6e7e9294d321 "usb: add basic USB configs in Kconfig", you added
> >>"config USB_STORAGE" to drivers/usb/Kconfig. However, it's still just
> >>#defined by many include/configs/*.h rather than being defined in
> >>configs/*_defconfig. Is that a problem? It seems to work in practice, but
> >>leads people adding new boards to put the definition in configs/*_defconfig
> >>which then may be inconsistent with similar existing boards which have it
> >>defined in include/configs/*.h.
> >
> >Once we create an entry in Kconfig,
> >all the defines in include/configs/*.h should be moved.
> 
> That's what I imagined. The commit above didn't do that though; are
> you planning on sending a fix?

Since those are often conflict-happy I'll take care of it.  I probably
intended to, even.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160908/8a240dd8/attachment.sig>

[U-Boot] config USB_STORAGE: defconfig vs include header

[Tom Rini]

On Thu, Sep 08, 2016 at 02:13:21PM -0400, Tom Rini wrote:
> On Thu, Sep 08, 2016 at 09:58:13AM -0600, Stephen Warren wrote:
> > On 09/07/2016 07:29 PM, Masahiro Yamada wrote:
> > >Hi Stephen
> > >
> > >
> > >2016-09-08 1:15 GMT+09:00 Stephen Warren <swarren@wwwdotorg.org>:
> > >>Masahiro,
> > >>
> > >>In patch 6e7e9294d321 "usb: add basic USB configs in Kconfig", you added
> > >>"config USB_STORAGE" to drivers/usb/Kconfig. However, it's still just
> > >>#defined by many include/configs/*.h rather than being defined in
> > >>configs/*_defconfig. Is that a problem? It seems to work in practice, but
> > >>leads people adding new boards to put the definition in configs/*_defconfig
> > >>which then may be inconsistent with similar existing boards which have it
> > >>defined in include/configs/*.h.
> > >
> > >Once we create an entry in Kconfig,
> > >all the defines in include/configs/*.h should be moved.
> > 
> > That's what I imagined. The commit above didn't do that though; are
> > you planning on sending a fix?
> 
> Since those are often conflict-happy I'll take care of it.  I probably
> intended to, even.

So, yay for moveconfig.suspicious being a thing now.  A number of boards
set CONFIG_USB_STORAGE without CONFIG_USB=y in their defconfig file, so
they lost CONFIG_USB_STORAGE in migration.  Confirming that yes, really,
nothing else is added or dropped when we set CONFIG_USB=y in the
defconfig files as well (unit testing was good, testing the world now).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160908/82cd305d/attachment.sig>

[U-Boot] config USB_STORAGE: defconfig vs include header

[Masahiro Yamada]

2016-09-09 5:17 GMT+09:00 Tom Rini <trini@konsulko.com>:
> On Thu, Sep 08, 2016 at 02:13:21PM -0400, Tom Rini wrote:
>> On Thu, Sep 08, 2016 at 09:58:13AM -0600, Stephen Warren wrote:
>> > On 09/07/2016 07:29 PM, Masahiro Yamada wrote:
>> > >Hi Stephen
>> > >
>> > >
>> > >2016-09-08 1:15 GMT+09:00 Stephen Warren <swarren@wwwdotorg.org>:
>> > >>Masahiro,
>> > >>
>> > >>In patch 6e7e9294d321 "usb: add basic USB configs in Kconfig", you added
>> > >>"config USB_STORAGE" to drivers/usb/Kconfig. However, it's still just
>> > >>#defined by many include/configs/*.h rather than being defined in
>> > >>configs/*_defconfig. Is that a problem? It seems to work in practice, but
>> > >>leads people adding new boards to put the definition in configs/*_defconfig
>> > >>which then may be inconsistent with similar existing boards which have it
>> > >>defined in include/configs/*.h.
>> > >
>> > >Once we create an entry in Kconfig,
>> > >all the defines in include/configs/*.h should be moved.
>> >
>> > That's what I imagined. The commit above didn't do that though; are
>> > you planning on sending a fix?
>>
>> Since those are often conflict-happy I'll take care of it.  I probably
>> intended to, even.
>
> So, yay for moveconfig.suspicious being a thing now.  A number of boards
> set CONFIG_USB_STORAGE without CONFIG_USB=y in their defconfig file, so
> they lost CONFIG_USB_STORAGE in migration.  Confirming that yes, really,
> nothing else is added or dropped when we set CONFIG_USB=y in the
> defconfig files as well (unit testing was good, testing the world now).

I thought so.

A little bit manual intervention
is needed for those suspicious boards.
(add CONFIGs by your own script and re-sync)

I hope this is not too painful...


Thanks for taking care of this!

-- 
Best Regards
Masahiro Yamada

[U-Boot] config USB_STORAGE: defconfig vs include header

[Tom Rini]

On Fri, Sep 09, 2016 at 10:09:56AM +0900, Masahiro Yamada wrote:
> 2016-09-09 5:17 GMT+09:00 Tom Rini <trini@konsulko.com>:
> > On Thu, Sep 08, 2016 at 02:13:21PM -0400, Tom Rini wrote:
> >> On Thu, Sep 08, 2016 at 09:58:13AM -0600, Stephen Warren wrote:
> >> > On 09/07/2016 07:29 PM, Masahiro Yamada wrote:
> >> > >Hi Stephen
> >> > >
> >> > >
> >> > >2016-09-08 1:15 GMT+09:00 Stephen Warren <swarren@wwwdotorg.org>:
> >> > >>Masahiro,
> >> > >>
> >> > >>In patch 6e7e9294d321 "usb: add basic USB configs in Kconfig", you added
> >> > >>"config USB_STORAGE" to drivers/usb/Kconfig. However, it's still just
> >> > >>#defined by many include/configs/*.h rather than being defined in
> >> > >>configs/*_defconfig. Is that a problem? It seems to work in practice, but
> >> > >>leads people adding new boards to put the definition in configs/*_defconfig
> >> > >>which then may be inconsistent with similar existing boards which have it
> >> > >>defined in include/configs/*.h.
> >> > >
> >> > >Once we create an entry in Kconfig,
> >> > >all the defines in include/configs/*.h should be moved.
> >> >
> >> > That's what I imagined. The commit above didn't do that though; are
> >> > you planning on sending a fix?
> >>
> >> Since those are often conflict-happy I'll take care of it.  I probably
> >> intended to, even.
> >
> > So, yay for moveconfig.suspicious being a thing now.  A number of boards
> > set CONFIG_USB_STORAGE without CONFIG_USB=y in their defconfig file, so
> > they lost CONFIG_USB_STORAGE in migration.  Confirming that yes, really,
> > nothing else is added or dropped when we set CONFIG_USB=y in the
> > defconfig files as well (unit testing was good, testing the world now).
> 
> I thought so.
> 
> A little bit manual intervention
> is needed for those suspicious boards.
> (add CONFIGs by your own script and re-sync)
> 
> I hope this is not too painful...

Nope, it was rather smooth and got me to notice another problem, that is
a little harder to solve "cleanly" until we have Simon's SPL series in.

> Thanks for taking care of this!

Thanks for making the tools!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160908/88e32ce7/attachment.sig>