> [...]
> >
> > + 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?
yeah you're right
> > +
> > 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.
ack
> > +
> > + 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.
Fair enough. I was debating between the two but chose this route to get my
patch out faster
> 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).
I'm not sure what you mean the purpose of the flag to be. Would it be
required for it to handle requests (so both ALL_CTRL and CONTROL_ONLY must
be enabled)? Since userspace already has to link the functions, this seems
more like an "are you sure?" switch as opposed to providing concrete
functionality.
Unless you mean that it wouldn't be required, but allowed in order for user
to write no descriptors. Ffs allows for pretty bare bones descriptors
already (1 speed, 1 interface, with no endpoints). If we want to allow 0
speeds to be provided we might as well allow that generally. It wouldn't be
useful in most cases, but that is similar to providing no endpoints anyway.
--Jerry
--
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
