| Message ID | 1472741552-16777-1-git-send-email-tharvey@gateworks.com |
|---|---|
| State | Superseded |
| Headers | show |
On 2016-09-01 16:52, Tim Harvey wrote: > clean up usb gadget support: > - remove unnecessary kmod-usb-lib-composite > - make kmod-usb-gadget a proper dependency vs a selection > - rename modules so that they match standard linux kernel module name > and properly indent underneath usb-gadget in menuconfig > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > package/kernel/linux/modules/usb.mk | 48 +++++++++++++++---------------------- > 1 file changed, 19 insertions(+), 29 deletions(-) > > diff --git a/package/kernel/linux/modules/usb.mk b/package/kernel/linux/modules/usb.mk > index 52c28c9..24b5d7b 100644 > --- a/package/kernel/linux/modules/usb.mk > +++ b/package/kernel/linux/modules/usb.mk > @@ -219,20 +219,6 @@ endef > > $(eval $(call KernelPackage,usb-gadget)) > > -define KernelPackage/usb-lib-composite > - TITLE:=USB lib composite > - KCONFIG:=CONFIG_USB_LIBCOMPOSITE > - DEPENDS:=+kmod-usb-gadget +kmod-fs-configfs > - FILES:=$(LINUX_DIR)/drivers/usb/gadget/libcomposite.ko > - AUTOLOAD:=$(call AutoLoad,50,libcomposite) > - $(call AddDepends/usb) > -endef > - > -define KernelPackage/usb-lib-composite/description > - Lib Composite > -endef > - > -$(eval $(call KernelPackage,usb-lib-composite)) > > define KernelPackage/usb-ehci-debug-gadget > TITLE:=USB EHCI debug port Gadget support > @@ -252,14 +238,15 @@ endef > > $(eval $(call KernelPackage,usb-ehci-debug-gadget)) > > -define KernelPackage/usb-eth-gadget > - TITLE:=USB Ethernet Gadget support > +define KernelPackage/usb-g_ether Please don't use _ in package names, it is used to to separate the package name from the version in .ipk files. Also, I think the new name is less clear than the old one. > + TITLE:=USB Ethernet Gadget > KCONFIG:= \ > CONFIG_USB_ETH \ > CONFIG_USB_ETH_RNDIS=y \ > CONFIG_USB_ETH_EEM=n > - DEPENDS:=+kmod-usb-gadget +kmod-usb-lib-composite > + DEPENDS:=kmod-usb-gadget +kmod-fs-configfs > FILES:= \ > + $(LINUX_DIR)/drivers/usb/gadget/libcomposite.ko \ Adding libcomposite.ko to multiple packages is a really bad idea. Why did you prefer that over having proper dependencies in place? - Felix
On Sat, Sep 3, 2016 at 5:00 AM, Felix Fietkau <nbd@nbd.name> wrote: > On 2016-09-01 16:52, Tim Harvey wrote: >> clean up usb gadget support: >> - remove unnecessary kmod-usb-lib-composite >> - make kmod-usb-gadget a proper dependency vs a selection >> - rename modules so that they match standard linux kernel module name >> and properly indent underneath usb-gadget in menuconfig >> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >> --- >> package/kernel/linux/modules/usb.mk | 48 +++++++++++++++---------------------- >> 1 file changed, 19 insertions(+), 29 deletions(-) >> >> diff --git a/package/kernel/linux/modules/usb.mk b/package/kernel/linux/modules/usb.mk >> index 52c28c9..24b5d7b 100644 >> --- a/package/kernel/linux/modules/usb.mk >> +++ b/package/kernel/linux/modules/usb.mk >> @@ -219,20 +219,6 @@ endef >> >> $(eval $(call KernelPackage,usb-gadget)) >> >> -define KernelPackage/usb-lib-composite >> - TITLE:=USB lib composite >> - KCONFIG:=CONFIG_USB_LIBCOMPOSITE >> - DEPENDS:=+kmod-usb-gadget +kmod-fs-configfs >> - FILES:=$(LINUX_DIR)/drivers/usb/gadget/libcomposite.ko >> - AUTOLOAD:=$(call AutoLoad,50,libcomposite) >> - $(call AddDepends/usb) >> -endef >> - >> -define KernelPackage/usb-lib-composite/description >> - Lib Composite >> -endef >> - >> -$(eval $(call KernelPackage,usb-lib-composite)) >> >> define KernelPackage/usb-ehci-debug-gadget >> TITLE:=USB EHCI debug port Gadget support >> @@ -252,14 +238,15 @@ endef >> >> $(eval $(call KernelPackage,usb-ehci-debug-gadget)) >> >> -define KernelPackage/usb-eth-gadget >> - TITLE:=USB Ethernet Gadget support >> +define KernelPackage/usb-g_ether > Please don't use _ in package names, it is used to to separate the > package name from the version in .ipk files. > Also, I think the new name is less clear than the old one. I was trying to stick with the kernel module names but as they have _ in them, I will remove this part of the patch. > >> + TITLE:=USB Ethernet Gadget >> KCONFIG:= \ >> CONFIG_USB_ETH \ >> CONFIG_USB_ETH_RNDIS=y \ >> CONFIG_USB_ETH_EEM=n >> - DEPENDS:=+kmod-usb-gadget +kmod-usb-lib-composite >> + DEPENDS:=kmod-usb-gadget +kmod-fs-configfs >> FILES:= \ >> + $(LINUX_DIR)/drivers/usb/gadget/libcomposite.ko \ > Adding libcomposite.ko to multiple packages is a really bad idea. > Why did you prefer that over having proper dependencies in place? I don't believe I added any deps - I removed libcomposite from the menu selection and included it directly in the gadgets that require it because as far as I know there is no point in having libcomposite.ko without the g_* drivers that use it. On a related note, what do you think about replacing all gadget drivers (g_*) with usb_f_* modules using CONFIG_USB_CONFIG_FS allowing userspace configuration of gadgets? I can see an OpenWrt init script that builds composite gadgets based off of UCI config options. I'm not sure what the future holds for the various g_* modules as you can re-create those easily in userspace now. For details and examples see: - http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/usb/gadget_configfs.txt - http://trac.gateworks.com/wiki/linux/OTG#configfs Tim
On 2016-09-06 18:57, Tim Harvey wrote: > On Sat, Sep 3, 2016 at 5:00 AM, Felix Fietkau <nbd@nbd.name> wrote: >> Adding libcomposite.ko to multiple packages is a really bad idea. >> Why did you prefer that over having proper dependencies in place? > > I don't believe I added any deps - I removed libcomposite from the > menu selection and included it directly in the gadgets that require it > because as far as I know there is no point in having libcomposite.ko > without the g_* drivers that use it. Adding libcomposite.ko to multiple packages is exactly the problem. That way you make it impossible to cleanly install multiple gadget modules. And besides, duplicating the same files in multiple packages is rather ugly. > On a related note, what do you think about replacing all gadget > drivers (g_*) with usb_f_* modules using CONFIG_USB_CONFIG_FS allowing > userspace configuration of gadgets? I can see an OpenWrt init script > that builds composite gadgets based off of UCI config options. I'm not > sure what the future holds for the various g_* modules as you can > re-create those easily in userspace now. Makes sense to me, but I don't know much about this stuff. - Felix
On Tue, Sep 6, 2016 at 1:15 PM, Felix Fietkau <nbd@nbd.name> wrote: > On 2016-09-06 18:57, Tim Harvey wrote: >> On Sat, Sep 3, 2016 at 5:00 AM, Felix Fietkau <nbd@nbd.name> wrote: >>> Adding libcomposite.ko to multiple packages is a really bad idea. >>> Why did you prefer that over having proper dependencies in place? >> >> I don't believe I added any deps - I removed libcomposite from the >> menu selection and included it directly in the gadgets that require it >> because as far as I know there is no point in having libcomposite.ko >> without the g_* drivers that use it. > Adding libcomposite.ko to multiple packages is exactly the problem. > That way you make it impossible to cleanly install multiple gadget > modules. And besides, duplicating the same files in multiple packages is > rather ugly. What I was attempting to accomplish here was make it such that the various 'gadget' drivers appear in a sub-menu of kmod-usb-gadget because the way it is now kmod-usb-eth-gadget and kmod-usb-serial-gadget are in completely different places and don't appear until you select kmod-usb-lib-composite (how is a user supposed to know that they need to select those first?). I'm not sure why adding libcomposite.ko to multiple packages would create a problem... it just gets installed once. You know way more about the openwrt/lede kbuild system so please feel free to school me on the way to properly make these menu's less confusing. It seems to me a user should be able to say 'Add gadget support' which then expands a sub-menu with the various available gadgets. > >> On a related note, what do you think about replacing all gadget >> drivers (g_*) with usb_f_* modules using CONFIG_USB_CONFIG_FS allowing >> userspace configuration of gadgets? I can see an OpenWrt init script >> that builds composite gadgets based off of UCI config options. I'm not >> sure what the future holds for the various g_* modules as you can >> re-create those easily in userspace now. > Makes sense to me, but I don't know much about this stuff. > Ok, I'll leave that for a later task after the gadget module selection is cleaned up and a few more basic modules are added. Tim
On 2016-09-06 22:50, Tim Harvey wrote: > On Tue, Sep 6, 2016 at 1:15 PM, Felix Fietkau <nbd@nbd.name> wrote: >> On 2016-09-06 18:57, Tim Harvey wrote: >>> On Sat, Sep 3, 2016 at 5:00 AM, Felix Fietkau <nbd@nbd.name> wrote: >>>> Adding libcomposite.ko to multiple packages is a really bad idea. >>>> Why did you prefer that over having proper dependencies in place? >>> >>> I don't believe I added any deps - I removed libcomposite from the >>> menu selection and included it directly in the gadgets that require it >>> because as far as I know there is no point in having libcomposite.ko >>> without the g_* drivers that use it. >> Adding libcomposite.ko to multiple packages is exactly the problem. >> That way you make it impossible to cleanly install multiple gadget >> modules. And besides, duplicating the same files in multiple packages is >> rather ugly. > > What I was attempting to accomplish here was make it such that the > various 'gadget' drivers appear in a sub-menu of kmod-usb-gadget > because the way it is now kmod-usb-eth-gadget and > kmod-usb-serial-gadget are in completely different places and don't > appear until you select kmod-usb-lib-composite (how is a user supposed > to know that they need to select those first?). I'm not sure why > adding libcomposite.ko to multiple packages would create a problem... > it just gets installed once. opkg really doesn't like it when one package overwrites files from another - understandably. > You know way more about the openwrt/lede > kbuild system so please feel free to school me on the way to properly > make these menu's less confusing. > > It seems to me a user should be able to say 'Add gadget support' which > then expands a sub-menu with the various available gadgets. Yes, you can do that. But you don't need to touch kmod-usb-libcomposite for it... The parts that remove it are completely unnecessary. - Felix
diff --git a/package/kernel/linux/modules/usb.mk b/package/kernel/linux/modules/usb.mk index 52c28c9..24b5d7b 100644 --- a/package/kernel/linux/modules/usb.mk +++ b/package/kernel/linux/modules/usb.mk @@ -219,20 +219,6 @@ endef $(eval $(call KernelPackage,usb-gadget)) -define KernelPackage/usb-lib-composite - TITLE:=USB lib composite - KCONFIG:=CONFIG_USB_LIBCOMPOSITE - DEPENDS:=+kmod-usb-gadget +kmod-fs-configfs - FILES:=$(LINUX_DIR)/drivers/usb/gadget/libcomposite.ko - AUTOLOAD:=$(call AutoLoad,50,libcomposite) - $(call AddDepends/usb) -endef - -define KernelPackage/usb-lib-composite/description - Lib Composite -endef - -$(eval $(call KernelPackage,usb-lib-composite)) define KernelPackage/usb-ehci-debug-gadget TITLE:=USB EHCI debug port Gadget support @@ -252,14 +238,15 @@ endef $(eval $(call KernelPackage,usb-ehci-debug-gadget)) -define KernelPackage/usb-eth-gadget - TITLE:=USB Ethernet Gadget support +define KernelPackage/usb-g_ether + TITLE:=USB Ethernet Gadget KCONFIG:= \ CONFIG_USB_ETH \ CONFIG_USB_ETH_RNDIS=y \ CONFIG_USB_ETH_EEM=n - DEPENDS:=+kmod-usb-gadget +kmod-usb-lib-composite + DEPENDS:=kmod-usb-gadget +kmod-fs-configfs FILES:= \ + $(LINUX_DIR)/drivers/usb/gadget/libcomposite.ko \ $(LINUX_DIR)/drivers/usb/gadget/function/u_ether.ko \ $(LINUX_DIR)/drivers/usb/gadget/function/usb_f_ecm.ko \ $(LINUX_DIR)/drivers/usb/gadget/function/usb_f_ecm_subset.ko \ @@ -269,18 +256,19 @@ define KernelPackage/usb-eth-gadget $(call AddDepends/usb) endef -define KernelPackage/usb-eth-gadget/description +define KernelPackage/usb-g_ether/description Kernel support for USB Ethernet Gadget endef -$(eval $(call KernelPackage,usb-eth-gadget)) +$(eval $(call KernelPackage,usb-g_ether)) -define KernelPackage/usb-serial-gadget - TITLE:=USB Serial Gadget support +define KernelPackage/usb-g_serial + TITLE:=USB Serial Gadget KCONFIG:=CONFIG_USB_G_SERIAL - DEPENDS:=+kmod-usb-gadget +kmod-usb-lib-composite + DEPENDS:=kmod-usb-gadget +kmod-fs-configfs FILES:= \ + $(LINUX_DIR)/drivers/usb/gadget/libcomposite.ko \ $(LINUX_DIR)/drivers/usb/gadget/function/u_serial.ko \ $(LINUX_DIR)/drivers/usb/gadget/function/usb_f_acm.ko \ $(LINUX_DIR)/drivers/usb/gadget/function/usb_f_obex.ko \ @@ -290,28 +278,30 @@ define KernelPackage/usb-serial-gadget $(call AddDepends/usb) endef -define KernelPackage/usb-serial-gadget/description +define KernelPackage/usb-g_serial/description Kernel support for USB Serial Gadget. endef -$(eval $(call KernelPackage,usb-serial-gadget)) +$(eval $(call KernelPackage,usb-g_serial)) + -define KernelPackage/usb-mass-storage-gadget - TITLE:=USB Mass Storage support +define KernelPackage/usb-g_mass-storage + TITLE:=USB Mass Storage KCONFIG:=CONFIG_USB_MASS_STORAGE - DEPENDS:=+kmod-usb-gadget +kmod-usb-lib-composite + DEPENDS:=kmod-usb-gadget +kmod-fs-configfs FILES:= \ + $(LINUX_DIR)/drivers/usb/gadget/libcomposite.ko \ $(LINUX_DIR)/drivers/usb/gadget/function/usb_f_mass_storage.ko \ $(LINUX_DIR)/drivers/usb/gadget/legacy/g_mass_storage.ko AUTOLOAD:=$(call AutoLoad,52,usb_f_mass_storage g_mass_storage) $(call AddDepends/usb) endef -define KernelPackage/usb-mass-storage-gadget/description +define KernelPackage/usb-g_mass-storage/description Kernel support for USB Gadget Mass Storage endef -$(eval $(call KernelPackage,usb-mass-storage-gadget)) +$(eval $(call KernelPackage,usb-g_mass-storage)) define KernelPackage/usb-uhci
clean up usb gadget support: - remove unnecessary kmod-usb-lib-composite - make kmod-usb-gadget a proper dependency vs a selection - rename modules so that they match standard linux kernel module name and properly indent underneath usb-gadget in menuconfig Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- package/kernel/linux/modules/usb.mk | 48 +++++++++++++++---------------------- 1 file changed, 19 insertions(+), 29 deletions(-)