Messages in this thread | | | From | Pawel Laszczak <> | Subject | RE: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver | Date | Wed, 10 Jul 2019 08:25:08 +0000 |
| |
Hi Felipe
>>> + >>> +static int cdns3_idle_init(struct cdns3 *cdns) >>> +{ >>> + struct cdns3_role_driver *rdrv; >>> + >>> + rdrv = devm_kzalloc(cdns->dev, sizeof(*rdrv), GFP_KERNEL); >>> + if (!rdrv) >>> + return -ENOMEM; >>> + >>> + rdrv->start = cdns3_idle_role_start; >>> + rdrv->stop = cdns3_idle_role_stop; >>> + rdrv->state = CDNS3_ROLE_STATE_INACTIVE; >>> + rdrv->suspend = NULL; >>> + rdrv->resume = NULL; >>> + rdrv->name = "idle"; >> >>why don't you use the mux framework for this? This looks a bit fishy >>too. Why do you have your own driver registration structure for your >>driver only? >> > >I assume you mean interface defined in include/linux/usb/role.h. >It's quite new framework and probably was added after I've start implementing cdns3 driver. >At first glance it's look that I could use it.
I've started integrating driver with role switch framework.
Even if I use role switch interface , I still need this internal driver registration. It's convenient to use fallowing structure.
struct cdns3_role_driver { int (*start)(struct cdns3 *cdns); void (*stop)(struct cdns3 *cdns); int (*suspend)(struct cdns3 *cdns, bool do_wakeup); int (*resume)(struct cdns3 *cdns, bool hibernated); const char *name; #define CDNS3_ROLE_STATE_INACTIVE 0 #define CDNS3_ROLE_STATE_ACTIVE 1 int state; };
Driver can supports: only Host, only Device or DRD - depending on configuration.
If current configuration support only Host then driver assigns: rdrv_host->start = __cdns3_host_init; rdrv_host->stop = cdns3_host_exit;
cdns->roles[CDNS3_ROLE_HOST] = rdrv_host cdns->roles[CDNS3_ROLE_GADGET = NULL; if support only Device then: rdrv_dev->start = __cdns3_ gadget _init; rdrv_dev->stop = cdns3_ gadget _exit; cdns->roles[CDNS3_ROLE_HOST] = NULL; for DRD: cdns->roles[CDNS3_ROLE_HOST = rdrv_host; cdns->roles[CDNS3_ROLE_GADGET] = rdrv_dev;
So for DRD we will have both filled, but for only Device or Host we will have filled single element of array.
With such array we can easily start/stop role by if (!cdns->roles[role]) not supported by configuration. else cdns->roles[role]->start / cdns->roles[role]->stop
I don't need any extra: switch instruction or #ifdef statement.
The name cdns3_role_driver can be misleading. Driver doesn't register the driver but rather the interface to Device/Host.
Maybe I should change this name to cdns3_role_interface or cdns3_role_action ?
Now I have my private enum: enum cdns3_roles { CDNS3_ROLE_IDLE = 0, CDNS3_ROLE_HOST, CDNS3_ROLE_GADGET, CDNS3_ROLE_END, };
I think I could replace it with usb_role. I need one extra state for IDLE but Instead of CDNS3_ROLE_IDLE I can use USB_ROLE_NONE.
It should little simplify the driver and improve readability.
Do you have any comments or suggestion ?
Cheers, Pawel
> >>> + >>> + cdns->roles[CDNS3_ROLE_IDLE] = rdrv; >>> + >>> + return 0; >>> +} >>> +
| |