Looks nice. :]
On Wed, Nov 28 2012, Sebastian Andrzej Siewior wrote:
> |# modprobe dummy_hcd num=2
>
> |# find /sys/kernel/config/ -ls
> | 6547 0 drwxr-xr-x 5 root root 0 Nov 28 19:39 /sys/kernel/config/
> | 532 0 drwxr-xr-x 2 root root 0 Nov 28 19:39 /sys/kernel/config/dummy_udc.1
> | 531 0 drwxr-xr-x 2 root root 0 Nov 28 19:39 /sys/kernel/config/dummy_udc.0
> | 6548 0 drwxr-xr-x 4 root root 0 Nov 28 19:38 /sys/kernel/config/usb_gadget
> | 6550 0 drwxr-xr-x 2 root root 0 Nov 28 19:38 /sys/kernel/config/usb_gadget/gadgets
> | 6549 0 drwxr-xr-x 2 root root 0 Nov 28 19:38 /sys/kernel/config/usb_gadget/functions
>
> | # lsmod
> | Module Size Used by
> | dummy_hcd 20287 0
> | libcomposite 17052 0
Did dummy_hcd pulled libcomposite here?
> | # mkdir /sys/kernel/config/usb_gadget/functions/acm_one
>
> | # lsmod
> | Module Size Used by
> | f_acm 5306 0
> | u_serial 9644 1 f_acm
> | dummy_hcd 20287 0
> | libcomposite 17052 1 f_acm
>
> 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.
Either way, I think they should be grouped in a “udcs” directory.
Otherwise, this, in my opinion, pollutes configfs' root directory.
Whether they are under usb_gadget or outside, I don't have strong
opinions, even though I would see value in grouping USB gadget “stuff”
together. Than again, someone may argue about UDCs which support OTG.
> - the function name for acm is taken from the dir name. Now Michał, is
> it too much of hackery?
Again, I don't really have strong opinions (on top of which, I feel like
I'm imposing my ideas without doing any coding), but I feel that using
“function/instance” rather than “function_instance” may make things
a bit easier since the name does not have to be parsed at all.
Especially since if we choose “_” than “mass_storage” would have to
change its name, right? Maybe “.” as a separator would be better? In
your example, this would definitely look consistent with what dummy_udc
does.
PS. Ah, and in general, if anyone cares about my opinion, I hate “_” and
propose to use “-” instead whenever possible. :P
> This is a slow start. Other comments?
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> +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);
Maybe it's just me, but I don't like gotos where they can be avoided
(even though I have nothing against gotos in general):
if (!ret)
return subsys
kfree(udc_cfs);
return ERR_PTR(ret);
> +}
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--Attachment:
pgpgBlSFpHMCD.pgp
Description: PGP signature
