Re: [PATCH v2 2/2] phy: omap-usb2: Adapt phy-omap-usb2 for AM437x

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3406 bytes --]

Hi,

On Tue, Oct 15, 2013 at 12:23:45PM +0530, George Cherian wrote:
> This patch adds a compatible for AM437x "ti,am43xx-usb2" to
> reuse the same phy-omap-usb2 driver.

it does more than just adding a new compatible flag.

> diff --git a/drivers/phy/phy-omap-usb2.c b/drivers/phy/phy-omap-usb2.c
> index 3e5f08c..7c0bc6c 100644
> --- a/drivers/phy/phy-omap-usb2.c
> +++ b/drivers/phy/phy-omap-usb2.c
> @@ -144,6 +144,35 @@ static struct phy_ops ops = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +#ifdef CONFIG_OF
> +static const struct usb_phy_data omap_usb2_data = {
> +	.label = "omap_usb2",
> +	.set_host = omap_usb_set_host,
> +	.set_peripheral = omap_usb_set_peripheral,
> +	.set_vbus = omap_usb_set_vbus,
> +	.start_srp = omap_usb_start_srp,
> +};
> +
> +static const struct usb_phy_data am437x_usb2_data = {
> +	.label = "am437x_usb2",
> +	.set_host = omap_usb_set_host,
> +	.set_peripheral = omap_usb_set_peripheral,
> +};
> +
> +static const struct of_device_id omap_usb2_id_table[] = {
> +	{
> +		.compatible = "ti,omap-usb2",
> +		.data = &omap_usb2_data,
> +	},
> +	{
> +		.compatible = "ti,am437x-usb2",
> +		.data = &am437x_usb2_data,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, omap_usb2_id_table);
> +#endif
> +
>  static int omap_usb2_probe(struct platform_device *pdev)
>  {
>  	struct omap_usb	*phy;
> @@ -153,9 +182,14 @@ static int omap_usb2_probe(struct platform_device *pdev)
>  	struct device_node *node = pdev->dev.of_node;
>  	struct device_node *control_node;
>  	struct platform_device *control_pdev;
> +	const struct of_device_id *of_id;
> +	struct usb_phy_data *phy_data;
> +
> +	of_id = of_match_device(of_match_ptr(omap_usb2_id_table), &pdev->dev);
>  
> -	if (!node)
> +	if (!of_id)
>  		return -EINVAL;
> +	phy_data = (struct usb_phy_data *)of_id->data;
>  
>  	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
>  	if (!phy) {
> @@ -172,7 +206,7 @@ static int omap_usb2_probe(struct platform_device *pdev)
>  	phy->dev		= &pdev->dev;
>  
>  	phy->phy.dev		= phy->dev;
> -	phy->phy.label		= "omap-usb2";
> +	phy->phy.label		= phy_data->label;

label can be set based on compatible flag:

if (of_device_is_compatible(node, "ti,omap-usb2"))
	phy->phy.label		= "omap-usb2";
else /* if (of_device_is_compatible(node, "ti,am437x-usb2")) */ /* default to newest */
	phy->phy.label		= "am437x-usb2";	/* keep name consistency */

>  	phy->phy.set_suspend	= omap_usb2_suspend;
>  	phy->phy.otg		= otg;
>  	phy->phy.type		= USB_PHY_TYPE_USB2;
> @@ -199,10 +233,10 @@ static int omap_usb2_probe(struct platform_device *pdev)
>  	phy->is_suspended	= 1;
>  	omap_control_usb_phy_power(phy->control_dev, 0);
>  
> -	otg->set_host		= omap_usb_set_host;
> -	otg->set_peripheral	= omap_usb_set_peripheral;
> -	otg->set_vbus		= omap_usb_set_vbus;
> -	otg->start_srp		= omap_usb_start_srp;
> +	otg->set_host		= phy_data->set_host;
> +	otg->set_peripheral	= phy_data->set_peripheral;
> +	otg->set_vbus		= phy_data->set_vbus;
> +	otg->start_srp		= phy_data->start_srp;

this doesn't look right. I would rather have feature flags so that you
could:

if (test_bit(&phy_data->flags, OMAP_USB2_HAS_SRP))
	otg->start_srp = omap_usb_start_srp;

and so on. Note that you might need to add __maybe_unused annotations to
those functions otherwise you'll get unused warnings.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]