Re: [PATCH v2 5/9] usb: roles: Provide the switch drivers handle to the switch in the API

Heikki Krogerus
2020-03-02T07:43:09Z

On Mon, Mar 02, 2020 at 06:22:59AM +0000, Peter Chen wrote:
> On 20-02-24 13:14:40, Peter Chen wrote:
> > > */
> > > -static int cdns3_role_set(struct device *dev, enum usb_role role)
> > > +static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
> > > {
> > > - struct cdns3 *cdns = dev_get_drvdata(dev);
> > > + struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
> > > int ret = 0;
> > >
> > > pm_runtime_get_sync(cdns->dev);
> > > @@ -423,12 +423,6 @@ static int cdns3_role_set(struct device *dev, enum usb_role role)
> > > return ret;
> > > }
> > >
> > > -static const struct usb_role_switch_desc cdns3_switch_desc = {
> > > - .set = cdns3_role_set,
> > > - .get = cdns3_role_get,
> > > - .allow_userspace_control = true,
> > > -};
> > > -
> > > /**
> > > * cdns3_probe - probe for cdns3 core device
> > > * @pdev: Pointer to cdns3 core platform device
> > > @@ -437,6 +431,7 @@ static const struct usb_role_switch_desc cdns3_switch_desc = {
> > > */
> > > static int cdns3_probe(struct platform_device *pdev)
> > > {
> > > + struct usb_role_switch_desc sw_desc = { };
> > > struct device *dev = &pdev->dev;
> > > struct resource *res;
> > > struct cdns3 *cdns;
> > > @@ -529,7 +524,12 @@ static int cdns3_probe(struct platform_device *pdev)
> > > if (ret)
> > > goto err3;
> > >
> > > - cdns->role_sw = usb_role_switch_register(dev, &cdns3_switch_desc);
> > > + sw_desc.set = cdns3_role_set,
> > > + sw_desc.get = cdns3_role_get,
> > > + sw_desc.allow_userspace_control = true,
>
> Heikki, when I try to apply above, and compile, I find above issue,
> the end of code should be ";", but not ",".

OK. I'll fix that. I could have sworn that I tested that, but clearly
I have not done it.

> > > + sw_desc.driver_data = cdns;
> > > +
> > > + cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
> > > if (IS_ERR(cdns->role_sw)) {
> > > ret = PTR_ERR(cdns->role_sw);
> > > dev_warn(dev, "Unable to register Role Switch\n");
> > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > > index 52139c2a9924..ae0bdc036464 100644
> > > --- a/drivers/usb/chipidea/core.c
> > > +++ b/drivers/usb/chipidea/core.c
> > > @@ -600,9 +600,9 @@ static int ci_cable_notifier(struct notifier_block *nb, unsigned long event,
> > > return NOTIFY_DONE;
> > > }
> > >
> > > -static enum usb_role ci_usb_role_switch_get(struct device *dev)
> > > +static enum usb_role ci_usb_role_switch_get(struct usb_role_switch *sw)
> > > {
> > > - struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > + struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> > > enum usb_role role;
> > > unsigned long flags;
> > >
> > > @@ -613,9 +613,10 @@ static enum usb_role ci_usb_role_switch_get(struct device *dev)
> > > return role;
> > > }
> > >
> > > -static int ci_usb_role_switch_set(struct device *dev, enum usb_role role)
> > > +static int ci_usb_role_switch_set(struct usb_role_switch *sw,
> > > + enum usb_role role)
> > > {
> > > - struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > + struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> > > struct ci_hdrc_cable *cable = NULL;
> > > enum usb_role current_role = ci_role_to_usb_role(ci);
> > > enum ci_role ci_role = usb_role_to_ci_role(role);
> > > @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > > }
> > >
> > > if (ci_role_switch.fwnode) {
> > > + ci_role_switch.driver_data = ci;
>
> And chipidea code, better change it like cdns3's, otherwise, the
> switch desc for all controllers have the same driver_data.

OK.

thanks,

--
heikki