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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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