On 11/29/2012 12:19 PM, Andrzej Pietrasiewicz wrote:
Now:
- the UDC (dummy_udc.[01]) is not within /usb_gadget/ folder where we
have /gadgets/ and /functions/ subfolder. This is what
spear13xx_pcie_gadget does as well. Is this okay or do we want to have
them enumerated below /usb_gadget/ as well? If so we got to convince
Joel to take Andrzej's patch for that.
I would want udcs under /usb_gadget. This way all the usb gadget-related
stuff is grouped into one configfs subsystem. I am also not sure,
but I would just guess that creating symlinks between different
configfs subsystems can be tricky.
I'm on it.
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
new file mode 100644 index 0000000..536c122
--- /dev/null
+++ b/drivers/usb/gadget/configfs.c
@@ -0,0 +1,189 @@
+#include<linux/configfs.h>
+#include<linux/module.h>
+#include<linux/slab.h>
+#include<linux/device.h>
+#include<linux/usb/composite.h>
+
+static struct config_group *gadget_core_register(
+ struct config_group *group,
+ const char *name)
+{
+ pr_err("%s() %s\n", __func__, name);
+ return ERR_PTR(-EINVAL);
+}
+
I would use a name which is more configfs-specific, e.g.
The whole file should be configfs specific.
+static struct config_group *make_gagdet_core(
or
+static struct config_group *gadget_core_make(
but the former seems more natural to me, though.
>
All group/item operations are never called directly so there
is no need to make the name more meaningful to a
hypothetical user.
Instead, these operations names are used only from a
configfs-specific code in an assignment, so it is better
for them to have names meaningful to configfs-specific code.
In other words: their user is a configfs-specific code
so the names should be friendly to the actual user.
Let me change this then…
+static void gadget_core_deregister(
+ struct config_group *group,
+ struct config_item *item)
+{
+ pr_err("%s()\n", __func__);
+}
+
+static void drop_gadget_core(
And in general, what are the above two functions for
other than failing and printing an error? If you use
them in order to have _something_ in make_group and drop_item
then don't - group operations _can_ be NULL in which case
the mkdir just fails with -EPERM.
Yes but this is work in progress. I have a stub already.
+static struct configfs_group_operations gadget_core_group_ops = {
+ .make_group =&gadget_core_register,
+ .drop_item =&gadget_core_deregister,
+};
+
+static struct config_item_type gadget_core_item = {
+ .ct_group_ops =&gadget_core_group_ops,
+ .ct_owner = THIS_MODULE,
+};
+
This is confusing. You call something which is a config_item_type
an item, which is similar to a config_item, which is in turn
a completely different thing than config_item_type. I would go for
+static struct config_item_type gadget_core_type = {
+static struct config_group *gadget_create_gadget(
+ struct config_group *group,
+ const char *name)
+{
+ pr_err("%s() %s\n", __func__, name);
+ return ERR_PTR(-EINVAL);
+}
+
+static struct config_group *make_gadget(
+static void gadget_destroy_gadget(
+ struct config_group *group,
+ struct config_item *item)
+{
+ pr_err("%s()\n", __func__);
+}
+
+static struct config_group *drop_gadget(
+static struct configfs_group_operations gadget_attach = {
+ .make_group =&gadget_create_gadget,
+ .drop_item =&gadget_destroy_gadget,
+};
+
Why not
+static struct configfs_group_operations gadget_group_ops = {
?
This would be consistent with gadget_core_group_ops.
+static struct config_item_type gadget_item = {
+ .ct_group_ops =&gadget_attach,
+ .ct_owner = THIS_MODULE,
+};
+
+static struct config_item_type gadget_type = {
+static struct config_group gadget_group = {
+ .cg_item = {
+ .ci_namebuf = "gadgets",
+ .ci_type =&gadget_item,
+ .ci_type =&gadget_type,
+ },
+};
+
+#define MAX_NAME_LEN 40
+static struct config_group *gadget_load_func(
+ struct config_group *group,
+ const char *name)
+static struct config_group *make_func(
but see below
+{
+ struct usb_function *f;
+ char buf[MAX_NAME_LEN];
+ char *func_name;
+ char *instance_name;
+ int ret;
+
+ ret = snprintf(buf, MAX_NAME_LEN, "%s", name);
+ if (ret>= MAX_NAME_LEN)
+ ERR_PTR(-ENAMETOOLONG);
+
+ func_name = buf;
+ instance_name = strstr(func_name, "_");
+ if (!instance_name) {
+ pr_err("Unable to locate _ in FUNC_INSTANCE\n");
+ return ERR_PTR(-EINVAL);
+ }
+ *instance_name = '\0';
+ instance_name++;
+
+ pr_err("%s() try to get %s for %s\n", __func__, func_name,
instance_name);
+ f = usb_get_function(func_name);
+ usb_put_function(f);
This kind of code belongs to other places, e.g. usb_functions.c.
You get the function only to put it the very next line which indicates
that this wants to be called from somewhere else.
Again, this is early code. Later we do more with it.
I think there should be one configfs subsystem for all the gadget
stuff, but the config_groups/config_items with their components
should live in files which deal with their corresponding entities.
Yes. That makes sense if we put them as part of their individual
configuration.
"gadgets" and "functions" can still be implemented as default groups,
but _their_ make_group& co can be provided in other files.
So you say once we have
/usb_gadge/functions/acm.bla/
then acm.bla's make_group should belong to acm? If so then yes, that
makes sense. Same goes I think for the UDCs.
+ pr_err("%s() %s\n", __func__,
+ IS_ERR(f) ? "failed" : "good");
+ return ERR_PTR(-EINVAL);
+}
+
+static void gadget_unload_func(
+static void drop_func(
but see above
+ struct config_group *group,
+ struct config_item *item)
+{
+ pr_err("%s()\n", __func__);
+}
+
Any other use of the function than printing an error?
again, early code and I have a stub to fill out later.
+static struct configfs_group_operations gadget_attach_func = {
+static struct configfs_group_operations func_ops = {
+ .make_group =&gadget_load_func,
+ .drop_item =&gadget_unload_func,
+};
+
+static struct config_item_type gadget_func_item = {
+static struct config_item_type func_type = {
+ .ct_group_ops =&gadget_attach_func,
+ .ct_owner = THIS_MODULE,
+};
+
+static struct config_group func_group = {
+ .cg_item = {
+ .ci_namebuf = "functions",
+ .ci_type =&gadget_func_item,
+ },
+};
+
+static struct config_group *root_groups[] = {
+ &func_group,
+ &gadget_group,
+ NULL
+};
+
+static struct configfs_subsystem gadget_subsys = {
+ .su_group = {
+ .cg_item = {
+ .ci_namebuf = "usb_gadget",
+ .ci_type =&gadget_core_item,
+ },
+ .default_groups = root_groups,
+ },
+ .su_mutex = __MUTEX_INITIALIZER(gadget_subsys.su_mutex),
+};
+
+struct udc_configfs {
+ struct configfs_subsystem subsys;
+};
+
Like I said, I would want udcs in the same subsystem as gadgets.
+struct configfs_subsystem *udc_add_configfs(struct device *dev) {
+ struct udc_configfs *udc_cfs;
+ struct configfs_subsystem *subsys;
+ int ret;
+
+ udc_cfs = kzalloc(sizeof(struct udc_configfs), GFP_KERNEL);
+ if (!udc_cfs)
+ return ERR_PTR(-ENOMEM);
+
+ subsys =&udc_cfs->subsys;
+ config_group_init_type_name(&subsys->su_group,
+ kobject_name(&dev->kobj),
+ &gadget_core_item);
+ ret = configfs_register_subsystem(subsys);
+ if (ret)
+ goto err;
+ return subsys;
+err:
+ kfree(udc_cfs);
+ return ERR_PTR(ret);
+}
+
+void udc_del_configfs(struct configfs_subsystem *subsys) {
+ struct udc_configfs *udc_cfs;
+
+ udc_cfs = container_of(subsys, struct udc_configfs, subsys);
+
+ configfs_unregister_subsystem(subsys);
+ kfree(udc_cfs);
+}
+
+int __init gadget_cfs_init(void)
+{
+ int ret;
+
+ config_group_init(&gadget_subsys.su_group);
+ config_group_init(&func_group);
+ config_group_init(&gadget_group);
+
+ ret = configfs_register_subsystem(&gadget_subsys);
+ return ret;
Perhaps:
+ return configfs_register_subsystem(&gadget_subsys);
since ret is not used for anything else?
+}
+
+void __exit gadget_cfs_exit(void)
+{
+ configfs_unregister_subsystem(&gadget_subsys);
+}
diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index 4d90a80..2c52b82 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -26,6 +26,7 @@
#include<linux/usb/ch9.h>
#include<linux/usb/gadget.h>
+#include<linux/configfs.h>
/**
* struct usb_udc - describes one usb device controller @@ -42,6 +43,7 @@
struct usb_udc {
struct usb_gadget *gadget;
struct device dev;
struct list_head list;
+ struct configfs_subsystem *subsys;
};
static struct class *udc_class;
@@ -231,6 +233,7 @@ int usb_add_gadget_udc(struct device *parent, struct
usb_gadget *gadget)
if (ret)
goto err3;
+ udc->subsys = udc_add_configfs(&udc->dev);
mutex_unlock(&udc_lock);
return 0;
@@ -305,6 +308,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
usb_gadget_remove_driver(udc);
kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
+ udc_del_configfs(udc->subsys);
device_unregister(&udc->dev);
}
EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
@@ -516,8 +520,17 @@ static int __init usb_udc_init(void) }
subsys_initcall(usb_udc_init);
+static int __init usb_udc_init_mod(void) {
+ if (IS_ERR(udc_class))
+ return PTR_ERR(udc_class);
+ return gadget_cfs_init();
+}
+module_init(usb_udc_init_mod);
+
static void __exit usb_udc_exit(void)
{
+ gadget_cfs_exit();
class_destroy(udc_class);
}
module_exit(usb_udc_exit);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index
0af6569..4d45962 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -883,6 +883,11 @@ extern void usb_del_gadget_udc(struct usb_gadget
*gadget);
/*-----------------------------------------------------------------------
--*/
+struct configfs_subsystem *udc_add_configfs(struct device *dev); void
+udc_del_configfs(struct configfs_subsystem *subsys); int
+gadget_cfs_init(void); void __exit gadget_cfs_exit(void);
+
/* utility to simplify dealing with string descriptors */
/**
--
1.7.10.4
I like the udc part from
+struct configfs_subsystem *udc_add_configfs(struct device *dev) {
onwards since this was missing (taking into account that there should
be one configfs subsystem).
But the gadget, function, interface, endpoint config groups and handling
thereof is already implemented in usb_functions.c. Why not use them?
This is as I said early code. I will get to this later. You and Michał
want things to remain under /usb-gadgets/ folder. So I want to get this
simple things done first. Once every one agrees on them I move on.
I have something simple for configfs to mkdir via configs API. Now I
need rmmdir. Once I have that, I throw at Joel and look how it goes.
After that I try rename all function names as you suggested before
moving on. Then we could have a v2 of that.
Then I should kick Felipe to say something to the function interface I
posted since the complete code depends on that.
Andrzej
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
