[PATCH] USB: gadget: g_fs: code cleanup and bug fixes

From: Michal Nazarewicz
Date: Mon Jun 14 2010 - 04:43:09 EST

This commit cleans the g_fs gadget hopefully making it more
readable. This is achieved by usage of the usb_string_ids_tab()
function for bulk string IDs registration as well as
generalising provided configuration so that a single routine is
used to add each configuration and bind interfaces. As an
effect, the code is shorter and has fewer #ifdefs.

Also, a bug has been fixed where bConfigurationValue were
not affected by what configuration user chose to compile in via
Kconfig option. As an effect, RDNIS and ECM were always number
1 and the "pure" configuration was always number 2.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
---
Requires "USB: gadget: composite: usb_string_ids_*() functions added"
patch to work.

drivers/usb/gadget/Kconfig | 23 +++---
drivers/usb/gadget/g_ffs.c | 174 ++++++++++++--------------------------------
2 files changed, 60 insertions(+), 137 deletions(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index d5b65d3..d3d0e2d 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -750,6 +750,7 @@ config USB_GADGETFS
config USB_FUNCTIONFS
tristate "Function Filesystem (EXPERIMENTAL)"
depends on EXPERIMENTAL
+ select USB_FUNCTIONFS_GENERIC if !(USB_FUNCTIONFS_ETH || USB_FUNCTIONFS_RNDIS)
help
The Function Filesystem (FunctioFS) lets one create USB
composite functions in user space in the same way as GadgetFS
@@ -758,31 +759,31 @@ config USB_FUNCTIONFS
implemented in kernel space (for instance Ethernet, serial or
mass storage) and other are implemented in user space.

+ If you say "y" or "m" here you will be able what kind of
+ configurations the gadget will provide.
+
Say "y" to link the driver statically, or "m" to build
a dynamically linked module called "g_ffs".

config USB_FUNCTIONFS_ETH
- bool "Include CDC ECM (Ethernet) function"
+ bool "Include configuration with CDC ECM (Ethernet)"
depends on USB_FUNCTIONFS && NET
help
- Include an CDC ECM (Ethernet) funcion in the CDC ECM (Funcion)
- Filesystem. If you also say "y" to the RNDIS query below the
- gadget will have two configurations.
+ Include a configuration with CDC ECM funcion (Ethernet) and the
+ Funcion Filesystem.

config USB_FUNCTIONFS_RNDIS
- bool "Include RNDIS (Ethernet) function"
+ bool "Include configuration with RNDIS (Ethernet)"
depends on USB_FUNCTIONFS && NET
help
- Include an RNDIS (Ethernet) funcion in the Funcion Filesystem.
- If you also say "y" to the CDC ECM query above the gadget will
- have two configurations.
+ Include a configuration with RNDIS funcion (Ethernet) and the Filesystem.

config USB_FUNCTIONFS_GENERIC
bool "Include 'pure' configuration"
- depends on USB_FUNCTIONFS && (USB_FUNCTIONFS_ETH || USB_FUNCTIONFS_RNDIS)
+ depends on USB_FUNCTIONFS
help
- Include a configuration with FunctionFS and no Ethernet
- configuration.
+ Include a configuration with the Function Filesystem alone with
+ no Ethernet interface.

config USB_FILE_STORAGE
tristate "File-backed Storage Gadget"
diff --git a/drivers/usb/gadget/g_ffs.c b/drivers/usb/gadget/g_ffs.c
index da3a9e4..a9474f8 100644
--- a/drivers/usb/gadget/g_ffs.c
+++ b/drivers/usb/gadget/g_ffs.c
@@ -32,12 +32,13 @@
# include "u_ether.c"

static u8 gfs_hostaddr[ETH_ALEN];
-#else
-# if !defined CONFIG_USB_FUNCTIONFS_GENERIC
-# define CONFIG_USB_FUNCTIONFS_GENERIC
+# ifdef CONFIG_USB_FUNCTIONFS_ETH
+static int eth_bind_config(struct usb_configuration *c, u8 ethaddr[ETH_ALEN]);
# endif
+#else
# define gether_cleanup() do { } while (0)
# define gether_setup(gadget, hostaddr) ((int)0)
+# define gfs_hostaddr NULL
#endif

#include "f_fs.c"
@@ -107,15 +108,7 @@ static const struct usb_descriptor_header *gfs_otg_desc[] = {
enum {
GFS_STRING_MANUFACTURER_IDX,
GFS_STRING_PRODUCT_IDX,
-#ifdef CONFIG_USB_FUNCTIONFS_RNDIS
- GFS_STRING_RNDIS_CONFIG_IDX,
-#endif
-#ifdef CONFIG_USB_FUNCTIONFS_ETH
- GFS_STRING_ECM_CONFIG_IDX,
-#endif
-#ifdef CONFIG_USB_FUNCTIONFS_GENERIC
- GFS_STRING_GENERIC_CONFIG_IDX,
-#endif
+ GFS_STRING_FIRST_CONFIG_IDX,
};

static char gfs_manufacturer[50];
@@ -126,13 +119,13 @@ static struct usb_string gfs_strings[] = {
[GFS_STRING_MANUFACTURER_IDX].s = gfs_manufacturer,
[GFS_STRING_PRODUCT_IDX].s = gfs_driver_desc,
#ifdef CONFIG_USB_FUNCTIONFS_RNDIS
- [GFS_STRING_RNDIS_CONFIG_IDX].s = "FunctionFS + RNDIS",
+ { .s = "FunctionFS + RNDIS" },
#endif
#ifdef CONFIG_USB_FUNCTIONFS_ETH
- [GFS_STRING_ECM_CONFIG_IDX].s = "FunctionFS + ECM",
+ { .s = "FunctionFS + ECM" },
#endif
#ifdef CONFIG_USB_FUNCTIONFS_GENERIC
- [GFS_STRING_GENERIC_CONFIG_IDX].s = "FunctionFS",
+ { .s = "FunctionFS" },
#endif
{ } /* end of list */
};
@@ -146,59 +139,33 @@ static struct usb_gadget_strings *gfs_dev_strings[] = {
};


+
+struct gfs_configuration {
+ struct usb_configuration c;
+ int (*eth)(struct usb_configuration *c, u8 *ethaddr);
+} gfs_configurations[] = {
#ifdef CONFIG_USB_FUNCTIONFS_RNDIS
-static int gfs_do_rndis_config(struct usb_configuration *c);
-
-static struct usb_configuration gfs_rndis_config_driver = {
- .label = "FunctionFS + RNDIS",
- .bind = gfs_do_rndis_config,
- .bConfigurationValue = 1,
- /* .iConfiguration = DYNAMIC */
- .bmAttributes = USB_CONFIG_ATT_SELFPOWER,
-};
-# define gfs_add_rndis_config(cdev) \
- usb_add_config(cdev, &gfs_rndis_config_driver)
-#else
-# define gfs_add_rndis_config(cdev) 0
+ {
+ .eth = rndis_bind_config,
+ },
#endif

-
#ifdef CONFIG_USB_FUNCTIONFS_ETH
-static int gfs_do_ecm_config(struct usb_configuration *c);
-
-static struct usb_configuration gfs_ecm_config_driver = {
- .label = "FunctionFS + ECM",
- .bind = gfs_do_ecm_config,
- .bConfigurationValue = 1,
- /* .iConfiguration = DYNAMIC */
- .bmAttributes = USB_CONFIG_ATT_SELFPOWER,
-};
-# define gfs_add_ecm_config(cdev) \
- usb_add_config(cdev, &gfs_ecm_config_driver)
-#else
-# define gfs_add_ecm_config(cdev) 0
+ {
+ .eth = eth_bind_config,
+ },
#endif

-
#ifdef CONFIG_USB_FUNCTIONFS_GENERIC
-static int gfs_do_generic_config(struct usb_configuration *c);
-
-static struct usb_configuration gfs_generic_config_driver = {
- .label = "FunctionFS",
- .bind = gfs_do_generic_config,
- .bConfigurationValue = 2,
- /* .iConfiguration = DYNAMIC */
- .bmAttributes = USB_CONFIG_ATT_SELFPOWER,
-};
-# define gfs_add_generic_config(cdev) \
- usb_add_config(cdev, &gfs_generic_config_driver)
-#else
-# define gfs_add_generic_config(cdev) 0
+ {
+ },
#endif
+};


static int gfs_bind(struct usb_composite_dev *cdev);
static int gfs_unbind(struct usb_composite_dev *cdev);
+static int gfs_do_config(struct usb_configuration *c);

static struct usb_composite_driver gfs_driver = {
.name = gfs_short_name,
@@ -267,7 +234,7 @@ static int functionfs_check_dev_callback(const char *dev_name)

static int gfs_bind(struct usb_composite_dev *cdev)
{
- int ret;
+ int ret, i;

ENTER();

@@ -284,57 +251,32 @@ static int gfs_bind(struct usb_composite_dev *cdev)
snprintf(gfs_manufacturer, sizeof gfs_manufacturer, "%s %s with %s",
init_utsname()->sysname, init_utsname()->release,
cdev->gadget->name);
- ret = usb_string_id(cdev);
- if (unlikely(ret < 0))
- goto error;
- gfs_strings[GFS_STRING_MANUFACTURER_IDX].id = ret;
- gfs_dev_desc.iManufacturer = ret;
-
- ret = usb_string_id(cdev);
- if (unlikely(ret < 0))
- goto error;
- gfs_strings[GFS_STRING_PRODUCT_IDX].id = ret;
- gfs_dev_desc.iProduct = ret;
-
-#ifdef CONFIG_USB_FUNCTIONFS_RNDIS
- ret = usb_string_id(cdev);
- if (unlikely(ret < 0))
- goto error;
- gfs_strings[GFS_STRING_RNDIS_CONFIG_IDX].id = ret;
- gfs_rndis_config_driver.iConfiguration = ret;
-#endif

-#ifdef CONFIG_USB_FUNCTIONFS_ETH
- ret = usb_string_id(cdev);
+ ret = usb_string_ids_tab(cdev, gfs_strings);
if (unlikely(ret < 0))
goto error;
- gfs_strings[GFS_STRING_ECM_CONFIG_IDX].id = ret;
- gfs_ecm_config_driver.iConfiguration = ret;
-#endif

-#ifdef CONFIG_USB_FUNCTIONFS_GENERIC
- ret = usb_string_id(cdev);
- if (unlikely(ret < 0))
- goto error;
- gfs_strings[GFS_STRING_GENERIC_CONFIG_IDX].id = ret;
- gfs_generic_config_driver.iConfiguration = ret;
-#endif
+ gfs_dev_desc.iManufacturer = gfs_strings[GFS_STRING_MANUFACTURER_IDX].id;
+ gfs_dev_desc.iProduct = gfs_strings[GFS_STRING_PRODUCT_IDX].id;

ret = functionfs_bind(gfs_ffs_data, cdev);
if (unlikely(ret < 0))
goto error;

- ret = gfs_add_rndis_config(cdev);
- if (unlikely(ret < 0))
- goto error_unbind;
+ for (i = 0; i < ARRAY_SIZE(gfs_configurations); ++i) {
+ struct gfs_configuration *c = gfs_configurations + i;

- ret = gfs_add_ecm_config(cdev);
- if (unlikely(ret < 0))
- goto error_unbind;
+ ret = GFS_STRING_FIRST_CONFIG_IDX + i;
+ c->c.label = gfs_strings[ret].s;
+ c->c.iConfiguration = gfs_strings[ret].id;
+ c->c.bind = gfs_do_config;
+ c->c.bConfigurationValue = 1 + i;
+ c->c.bmAttributes = USB_CONFIG_ATT_SELFPOWER;

- ret = gfs_add_generic_config(cdev);
- if (unlikely(ret < 0))
- goto error_unbind;
+ ret = usb_add_config(cdev, &c->c);
+ if (unlikely(ret < 0))
+ goto error_unbind;
+ }

return 0;

@@ -368,10 +310,10 @@ static int gfs_unbind(struct usb_composite_dev *cdev)
}


-static int __gfs_do_config(struct usb_configuration *c,
- int (*eth)(struct usb_configuration *c, u8 *ethaddr),
- u8 *ethaddr)
+static int gfs_do_config(struct usb_configuration *c)
{
+ struct gfs_configuration *gc =
+ container_of(c, struct gfs_configuration, c);
int ret;

if (WARN_ON(!gfs_ffs_data))
@@ -382,8 +324,8 @@ static int __gfs_do_config(struct usb_configuration *c,
c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
}

- if (eth) {
- ret = eth(c, ethaddr);
+ if (gc->eth) {
+ ret = gc->eth(c, gfs_hostaddr);
if (unlikely(ret < 0))
return ret;
}
@@ -406,32 +348,12 @@ static int __gfs_do_config(struct usb_configuration *c,
return 0;
}

-#ifdef CONFIG_USB_FUNCTIONFS_RNDIS
-static int gfs_do_rndis_config(struct usb_configuration *c)
-{
- ENTER();
-
- return __gfs_do_config(c, rndis_bind_config, gfs_hostaddr);
-}
-#endif

#ifdef CONFIG_USB_FUNCTIONFS_ETH
-static int gfs_do_ecm_config(struct usb_configuration *c)
-{
- ENTER();
-
- return __gfs_do_config(c,
- can_support_ecm(c->cdev->gadget)
- ? ecm_bind_config : geth_bind_config,
- gfs_hostaddr);
-}
-#endif
-
-#ifdef CONFIG_USB_FUNCTIONFS_GENERIC
-static int gfs_do_generic_config(struct usb_configuration *c)
+static int eth_bind_config(struct usb_configuration *c, u8 ethaddr[ETH_ALEN])
{
- ENTER();
-
- return __gfs_do_config(c, NULL, NULL);
+ return can_support_ecm(c->cdev->gadget)
+ ? ecm_bind_config(c, ethaddr)
+ : geth_bind_config(c, ethaddr);
}
#endif
--
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/