On 17.04.2018 03:17, Jerry Zhang wrote:
[...]
+ cfg = &gi->control_config;
+ c = &cfg->c;
+ list_for_each_entry_safe(f, tmp, &cfg->func_list, list) {
+ if (f->req_match && f->setup) {
+ list_del(&f->list);
+ if (usb_add_function(&cfg->c, f))
+ list_add(&f->list, &cfg->func_list);
+ }
+ }
shouldn't we goto error here instead of silently ignoring error?
+
usb_ep_autoconfig_reset(cdev->gadget);
return 0;
@@ -1391,11 +1407,33 @@ static void configfs_composite_unbind(struct usb_gadget *gadget)
set_gadget_data(gadget, NULL);
}
+static int configfs_composite_setup(struct usb_gadget *gadget,
+ const struct usb_ctrlrequest *c)
+{
+ struct usb_composite_dev *cdev = get_gadget_data(gadget);
+ struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
+
+ struct config_usb_cfg *cfg = &gi->control_config;
+ struct usb_function *f;
+ int value = composite_setup(gadget, c);
I think it would be more readable if you call this function later in the
code instead of during initialization of variable.
+
+ if (value >= 0)
+ return value;
+
+ list_for_each_entry(f, &cfg->c.functions, list) {
+ if (f->req_match(f, c, false)) {
+ value = f->setup(f, c);
+ break;
+ }
+ }
+ return value;
+}
+
static const struct usb_gadget_driver configfs_driver_template = {
.bind = configfs_composite_bind,
.unbind = configfs_composite_unbind,
- .setup = composite_setup,
+ .setup = configfs_composite_setup,
.reset = composite_disconnect,
.disconnect = composite_disconnect,
@@ -1461,6 +1499,18 @@ static struct config_group *gadgets_make(
if (!gi->composite.gadget_driver.function)
goto err;
+ gi->control_config.c.label = "control_config";
+ /* composite requires some value, but it doesn't matter */
+ gi->control_config.c.bConfigurationValue = 42;
+ INIT_LIST_HEAD(&gi->control_config.func_list);
+ config_group_init_type_name(&gi->control_config.group,
+ "control_config", &gadget_config_type);
+ configfs_add_default_group(&gi->control_config.group, &gi->group);
+
+ if (usb_add_config_only(&gi->cdev, &gi->control_config.c))
+ goto err;
+ list_del(&gi->control_config.c.list);
+
I know that you are trying to reuse as much code as possible but in my
humble opinion we should make a separate config_item_type for this to
drop things related to string bMaxPower etc as all those values are then
later ignored in the kernel so it looks like it doesn't make any sense
to allow users to set them.
As main usecase for this code is FunctionFS I think that we should
consider adding some flag to FunctionFS to mark instance as only for
such purpouse. I mean sth like FFS_CONTROL_ONLY which would make
FunctionFS igore the descs (or allow to provide 0 of them) and make this
function usable only for this purpouse (disallow linking to real config
and allow only for linking to this fake one).
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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
