diff mbox series

[v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums

Message ID 1557715229-13102-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Mainlined
Commit be21a02a5a5ec88b12ce535ead715d4bbb173a55
Headers show
Series [v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums | expand

Commit Message

Yoshihiro Shimoda May 13, 2019, 2:40 a.m. UTC 
This patch adds a specific struct "usbhs_of_data" to add a new SoC
data easily instead of code basis in the future.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Changes from v1 [1]:
 - Use sizeof(variable) instead of sizeof(type).
 - Add Geert-san's reviewed-by (thanks!).

[1]
https://patchwork.kernel.org/patch/10938575/

 drivers/usb/renesas_usbhs/common.c | 112 +++++++++++++++++++++----------------
 drivers/usb/renesas_usbhs/common.h |   5 ++
 2 files changed, 70 insertions(+), 47 deletions(-)

Comments

Simon Horman May 13, 2019, 12:01 p.m. UTC | #1 
On Mon, May 13, 2019 at 11:40:29AM +0900, Yoshihiro Shimoda wrote:
> This patch adds a specific struct "usbhs_of_data" to add a new SoC
> data easily instead of code basis in the future.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Hi Shimoda-san,

the minor suggestion below not withstanding this looks good to me.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  Changes from v1 [1]:
>  - Use sizeof(variable) instead of sizeof(type).
>  - Add Geert-san's reviewed-by (thanks!).
> 
> [1]
> https://patchwork.kernel.org/patch/10938575/
> 
>  drivers/usb/renesas_usbhs/common.c | 112 +++++++++++++++++++++----------------
>  drivers/usb/renesas_usbhs/common.h |   5 ++
>  2 files changed, 70 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> index 249fbee..0ca89de 100644
> --- a/drivers/usb/renesas_usbhs/common.c
> +++ b/drivers/usb/renesas_usbhs/common.c
> @@ -535,53 +535,92 @@ static int usbhsc_drvcllbck_notify_hotplug(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct usbhs_of_data rcar_gen2_data = {
> +	.platform_callback = &usbhs_rcar2_ops,
> +	.param = {
> +		.type = USBHS_TYPE_RCAR_GEN2,
> +		.has_usb_dmac = 1,
> +		.pipe_configs = usbhsc_new_pipe,
> +		.pipe_size = ARRAY_SIZE(usbhsc_new_pipe),
> +	}
> +};
> +
> +static const struct usbhs_of_data rcar_gen3_data = {
> +	.platform_callback = &usbhs_rcar3_ops,
> +	.param = {
> +		.type = USBHS_TYPE_RCAR_GEN3,
> +		.has_usb_dmac = 1,
> +		.pipe_configs = usbhsc_new_pipe,
> +		.pipe_size = ARRAY_SIZE(usbhsc_new_pipe),
> +	}
> +};
> +
> +static const struct usbhs_of_data rcar_gen3_with_pll_data = {
> +	.platform_callback = &usbhs_rcar3_with_pll_ops,
> +	.param = {
> +		.type = USBHS_TYPE_RCAR_GEN3_WITH_PLL,
> +		.has_usb_dmac = 1,
> +		.pipe_configs = usbhsc_new_pipe,
> +		.pipe_size = ARRAY_SIZE(usbhsc_new_pipe),
> +	}
> +};
> +
> +static const struct usbhs_of_data rza1_data = {
> +	.platform_callback = &usbhs_rza1_ops,
> +	.param = {
> +		.type = USBHS_TYPE_RZA1,
> +		.pipe_configs = usbhsc_new_pipe,
> +		.pipe_size = ARRAY_SIZE(usbhsc_new_pipe),
> +	}
> +};
> +
>  /*
>   *		platform functions
>   */
>  static const struct of_device_id usbhs_of_match[] = {
>  	{
>  		.compatible = "renesas,usbhs-r8a774c0",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN3_WITH_PLL,
> +		.data = &rcar_gen3_with_pll_data,
>  	},
>  	{
>  		.compatible = "renesas,usbhs-r8a7790",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN2,
> +		.data = &rcar_gen2_data,
>  	},
>  	{
>  		.compatible = "renesas,usbhs-r8a7791",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN2,
> +		.data = &rcar_gen2_data,
>  	},
>  	{
>  		.compatible = "renesas,usbhs-r8a7794",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN2,
> +		.data = &rcar_gen2_data,
>  	},
>  	{
>  		.compatible = "renesas,usbhs-r8a7795",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN3,
> +		.data = &rcar_gen3_data,
>  	},
>  	{
>  		.compatible = "renesas,usbhs-r8a7796",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN3,
> +		.data = &rcar_gen3_data,
>  	},
>  	{
>  		.compatible = "renesas,usbhs-r8a77990",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN3_WITH_PLL,
> +		.data = &rcar_gen3_with_pll_data,
>  	},
>  	{
>  		.compatible = "renesas,usbhs-r8a77995",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN3_WITH_PLL,
> +		.data = &rcar_gen3_with_pll_data,
>  	},
>  	{
>  		.compatible = "renesas,rcar-gen2-usbhs",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN2,
> +		.data = &rcar_gen2_data,
>  	},
>  	{
>  		.compatible = "renesas,rcar-gen3-usbhs",
> -		.data = (void *)USBHS_TYPE_RCAR_GEN3,
> +		.data = &rcar_gen3_data,
>  	},
>  	{
>  		.compatible = "renesas,rza1-usbhs",
> -		.data = (void *)USBHS_TYPE_RZA1,
> +		.data = &rza1_data,
>  	},
>  	{ },
>  };
> @@ -591,6 +630,7 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
>  {
>  	struct renesas_usbhs_platform_info *info;
>  	struct renesas_usbhs_driver_param *dparam;
> +	const struct usbhs_of_data *data;
>  	u32 tmp;
>  	int gpio;
>  
> @@ -598,8 +638,15 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
>  	if (!info)
>  		return NULL;
>  
> +	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return NULL;
> +
>  	dparam = &info->driver_param;
> -	dparam->type = (uintptr_t)of_device_get_match_data(dev);
> +	memcpy(dparam, &data->param, sizeof(data->param));
> +	memcpy(&info->platform_callback, data->platform_callback,
> +	       sizeof(*data->platform_callback));

Can we avoid the error-proneness of calls to sizeof() as follows?

        *dparam = data->param;
        info->platform_callback = *data->platform_callback;

> +
>  	if (!of_property_read_u32(dev->of_node, "renesas,buswait", &tmp))
>  		dparam->buswait_bwait = tmp;
>  	gpio = of_get_named_gpio_flags(dev->of_node, "renesas,enable-gpio", 0,
> @@ -607,19 +654,6 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
>  	if (gpio > 0)
>  		dparam->enable_gpio = gpio;
>  
> -	if (dparam->type == USBHS_TYPE_RCAR_GEN2 ||
> -	    dparam->type == USBHS_TYPE_RCAR_GEN3 ||
> -	    dparam->type == USBHS_TYPE_RCAR_GEN3_WITH_PLL) {
> -		dparam->has_usb_dmac = 1;
> -		dparam->pipe_configs = usbhsc_new_pipe;
> -		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> -	}
> -
> -	if (dparam->type == USBHS_TYPE_RZA1) {
> -		dparam->pipe_configs = usbhsc_new_pipe;
> -		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> -	}
> -
>  	return info;
>  }
>  
> @@ -676,29 +710,13 @@ static int usbhs_probe(struct platform_device *pdev)
>  	       &info->driver_param,
>  	       sizeof(struct renesas_usbhs_driver_param));

Likewise in the (otherwise unchanged) use of memcpy above.

>  
> -	switch (priv->dparam.type) {
> -	case USBHS_TYPE_RCAR_GEN2:
> -		priv->pfunc = usbhs_rcar2_ops;
> -		break;
> -	case USBHS_TYPE_RCAR_GEN3:
> -		priv->pfunc = usbhs_rcar3_ops;
> -		break;
> -	case USBHS_TYPE_RCAR_GEN3_WITH_PLL:
> -		priv->pfunc = usbhs_rcar3_with_pll_ops;
> -		break;
> -	case USBHS_TYPE_RZA1:
> -		priv->pfunc = usbhs_rza1_ops;
> -		break;
> -	default:
> -		if (!info->platform_callback.get_id) {
> -			dev_err(&pdev->dev, "no platform callbacks");
> -			return -EINVAL;
> -		}
> -		memcpy(&priv->pfunc,
> -		       &info->platform_callback,
> -		       sizeof(struct renesas_usbhs_platform_callback));
> -		break;
> +	if (!info->platform_callback.get_id) {
> +		dev_err(&pdev->dev, "no platform callbacks");
> +		return -EINVAL;
>  	}
> +	memcpy(&priv->pfunc,
> +	       &info->platform_callback,
> +	       sizeof(struct renesas_usbhs_platform_callback));

And here too.

>  
>  	/* set driver callback functions for platform */
>  	dfunc			= &info->driver_callback;
> diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
> index 3777af8..de1a663 100644
> --- a/drivers/usb/renesas_usbhs/common.h
> +++ b/drivers/usb/renesas_usbhs/common.h
> @@ -282,6 +282,11 @@ struct usbhs_priv {
>  	struct clk *clks[2];
>  };
>  
> +struct usbhs_of_data {
> +	const struct renesas_usbhs_platform_callback	*platform_callback;
> +	const struct renesas_usbhs_driver_param		param;
> +};
> +
>  /*
>   * common
>   */
> -- 
> 2.7.4
>
Yoshihiro Shimoda May 15, 2019, 6:57 a.m. UTC | #2 
Hi Simon-san,

> From: Simon Horman, Sent: Monday, May 13, 2019 9:01 PM
> 
> On Mon, May 13, 2019 at 11:40:29AM +0900, Yoshihiro Shimoda wrote:
> > This patch adds a specific struct "usbhs_of_data" to add a new SoC
> > data easily instead of code basis in the future.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Hi Shimoda-san,
> 
> the minor suggestion below not withstanding this looks good to me.
> 
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Thank you for your review!

> > ---
> >  Changes from v1 [1]:
> >  - Use sizeof(variable) instead of sizeof(type).
> >  - Add Geert-san's reviewed-by (thanks!).
> >
> > [1]
> > https://patchwork.kernel.org/patch/10938575/
> >
> >  drivers/usb/renesas_usbhs/common.c | 112 +++++++++++++++++++++----------------
> >  drivers/usb/renesas_usbhs/common.h |   5 ++
> >  2 files changed, 70 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> > index 249fbee..0ca89de 100644
> > --- a/drivers/usb/renesas_usbhs/common.c
> > +++ b/drivers/usb/renesas_usbhs/common.c
<snip>
> > @@ -598,8 +638,15 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
> >  	if (!info)
> >  		return NULL;
> >
> > +	data = of_device_get_match_data(dev);
> > +	if (!data)
> > +		return NULL;
> > +
> >  	dparam = &info->driver_param;
> > -	dparam->type = (uintptr_t)of_device_get_match_data(dev);
> > +	memcpy(dparam, &data->param, sizeof(data->param));
> > +	memcpy(&info->platform_callback, data->platform_callback,
> > +	       sizeof(*data->platform_callback));
> 
> Can we avoid the error-proneness of calls to sizeof() as follows?
> 
>         *dparam = data->param;
>         info->platform_callback = *data->platform_callback;

Since Chris-san has submitted a patch series [1] that is based on this patch today,
I'd like to submit an incremental patch to avoid the error-proneness in the renesas_usbhs
(common.c and mod_host.c) later, if possible.

Greg-san, is it acceptable?

[1]
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=117459

Best regards,
Yoshihiro Shimoda

> > +
> >  	if (!of_property_read_u32(dev->of_node, "renesas,buswait", &tmp))
> >  		dparam->buswait_bwait = tmp;
> >  	gpio = of_get_named_gpio_flags(dev->of_node, "renesas,enable-gpio", 0,
> > @@ -607,19 +654,6 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
> >  	if (gpio > 0)
> >  		dparam->enable_gpio = gpio;
> >
> > -	if (dparam->type == USBHS_TYPE_RCAR_GEN2 ||
> > -	    dparam->type == USBHS_TYPE_RCAR_GEN3 ||
> > -	    dparam->type == USBHS_TYPE_RCAR_GEN3_WITH_PLL) {
> > -		dparam->has_usb_dmac = 1;
> > -		dparam->pipe_configs = usbhsc_new_pipe;
> > -		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> > -	}
> > -
> > -	if (dparam->type == USBHS_TYPE_RZA1) {
> > -		dparam->pipe_configs = usbhsc_new_pipe;
> > -		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> > -	}
> > -
> >  	return info;
> >  }
> >
> > @@ -676,29 +710,13 @@ static int usbhs_probe(struct platform_device *pdev)
> >  	       &info->driver_param,
> >  	       sizeof(struct renesas_usbhs_driver_param));
> 
> Likewise in the (otherwise unchanged) use of memcpy above.
> 
> >
> > -	switch (priv->dparam.type) {
> > -	case USBHS_TYPE_RCAR_GEN2:
> > -		priv->pfunc = usbhs_rcar2_ops;
> > -		break;
> > -	case USBHS_TYPE_RCAR_GEN3:
> > -		priv->pfunc = usbhs_rcar3_ops;
> > -		break;
> > -	case USBHS_TYPE_RCAR_GEN3_WITH_PLL:
> > -		priv->pfunc = usbhs_rcar3_with_pll_ops;
> > -		break;
> > -	case USBHS_TYPE_RZA1:
> > -		priv->pfunc = usbhs_rza1_ops;
> > -		break;
> > -	default:
> > -		if (!info->platform_callback.get_id) {
> > -			dev_err(&pdev->dev, "no platform callbacks");
> > -			return -EINVAL;
> > -		}
> > -		memcpy(&priv->pfunc,
> > -		       &info->platform_callback,
> > -		       sizeof(struct renesas_usbhs_platform_callback));
> > -		break;
> > +	if (!info->platform_callback.get_id) {
> > +		dev_err(&pdev->dev, "no platform callbacks");
> > +		return -EINVAL;
> >  	}
> > +	memcpy(&priv->pfunc,
> > +	       &info->platform_callback,
> > +	       sizeof(struct renesas_usbhs_platform_callback));
> 
> And here too.
> 
> >
> >  	/* set driver callback functions for platform */
> >  	dfunc			= &info->driver_callback;
> > diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
> > index 3777af8..de1a663 100644
> > --- a/drivers/usb/renesas_usbhs/common.h
> > +++ b/drivers/usb/renesas_usbhs/common.h
> > @@ -282,6 +282,11 @@ struct usbhs_priv {
> >  	struct clk *clks[2];
> >  };
> >
> > +struct usbhs_of_data {
> > +	const struct renesas_usbhs_platform_callback	*platform_callback;
> > +	const struct renesas_usbhs_driver_param		param;
> > +};
> > +
> >  /*
> >   * common
> >   */
> > --
> > 2.7.4
> >
Simon Horman May 15, 2019, 8:10 a.m. UTC | #3 
On Wed, May 15, 2019 at 06:57:17AM +0000, Yoshihiro Shimoda wrote:
> Hi Simon-san,
> 
> > From: Simon Horman, Sent: Monday, May 13, 2019 9:01 PM
> > 
> > On Mon, May 13, 2019 at 11:40:29AM +0900, Yoshihiro Shimoda wrote:
> > > This patch adds a specific struct "usbhs_of_data" to add a new SoC
> > > data easily instead of code basis in the future.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > 
> > Hi Shimoda-san,
> > 
> > the minor suggestion below not withstanding this looks good to me.
> > 
> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> 
> Thank you for your review!
> 
> > > ---
> > >  Changes from v1 [1]:
> > >  - Use sizeof(variable) instead of sizeof(type).
> > >  - Add Geert-san's reviewed-by (thanks!).
> > >
> > > [1]
> > > https://patchwork.kernel.org/patch/10938575/
> > >
> > >  drivers/usb/renesas_usbhs/common.c | 112 +++++++++++++++++++++----------------
> > >  drivers/usb/renesas_usbhs/common.h |   5 ++
> > >  2 files changed, 70 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> > > index 249fbee..0ca89de 100644
> > > --- a/drivers/usb/renesas_usbhs/common.c
> > > +++ b/drivers/usb/renesas_usbhs/common.c
> <snip>
> > > @@ -598,8 +638,15 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
> > >  	if (!info)
> > >  		return NULL;
> > >
> > > +	data = of_device_get_match_data(dev);
> > > +	if (!data)
> > > +		return NULL;
> > > +
> > >  	dparam = &info->driver_param;
> > > -	dparam->type = (uintptr_t)of_device_get_match_data(dev);
> > > +	memcpy(dparam, &data->param, sizeof(data->param));
> > > +	memcpy(&info->platform_callback, data->platform_callback,
> > > +	       sizeof(*data->platform_callback));
> > 
> > Can we avoid the error-proneness of calls to sizeof() as follows?
> > 
> >         *dparam = data->param;
> >         info->platform_callback = *data->platform_callback;
> 
> Since Chris-san has submitted a patch series [1] that is based on this patch today,
> I'd like to submit an incremental patch to avoid the error-proneness in the renesas_usbhs
> (common.c and mod_host.c) later, if possible.
> 
> Greg-san, is it acceptable?

FWIIW that sounds entirely reasonable to me as
my suggestion is a cleanup.

> [1]
> https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=117459
> 
> Best regards,
> Yoshihiro Shimoda
> 
> > > +
> > >  	if (!of_property_read_u32(dev->of_node, "renesas,buswait", &tmp))
> > >  		dparam->buswait_bwait = tmp;
> > >  	gpio = of_get_named_gpio_flags(dev->of_node, "renesas,enable-gpio", 0,
> > > @@ -607,19 +654,6 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
> > >  	if (gpio > 0)
> > >  		dparam->enable_gpio = gpio;
> > >
> > > -	if (dparam->type == USBHS_TYPE_RCAR_GEN2 ||
> > > -	    dparam->type == USBHS_TYPE_RCAR_GEN3 ||
> > > -	    dparam->type == USBHS_TYPE_RCAR_GEN3_WITH_PLL) {
> > > -		dparam->has_usb_dmac = 1;
> > > -		dparam->pipe_configs = usbhsc_new_pipe;
> > > -		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> > > -	}
> > > -
> > > -	if (dparam->type == USBHS_TYPE_RZA1) {
> > > -		dparam->pipe_configs = usbhsc_new_pipe;
> > > -		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> > > -	}
> > > -
> > >  	return info;
> > >  }
> > >
> > > @@ -676,29 +710,13 @@ static int usbhs_probe(struct platform_device *pdev)
> > >  	       &info->driver_param,
> > >  	       sizeof(struct renesas_usbhs_driver_param));
> > 
> > Likewise in the (otherwise unchanged) use of memcpy above.
> > 
> > >
> > > -	switch (priv->dparam.type) {
> > > -	case USBHS_TYPE_RCAR_GEN2:
> > > -		priv->pfunc = usbhs_rcar2_ops;
> > > -		break;
> > > -	case USBHS_TYPE_RCAR_GEN3:
> > > -		priv->pfunc = usbhs_rcar3_ops;
> > > -		break;
> > > -	case USBHS_TYPE_RCAR_GEN3_WITH_PLL:
> > > -		priv->pfunc = usbhs_rcar3_with_pll_ops;
> > > -		break;
> > > -	case USBHS_TYPE_RZA1:
> > > -		priv->pfunc = usbhs_rza1_ops;
> > > -		break;
> > > -	default:
> > > -		if (!info->platform_callback.get_id) {
> > > -			dev_err(&pdev->dev, "no platform callbacks");
> > > -			return -EINVAL;
> > > -		}
> > > -		memcpy(&priv->pfunc,
> > > -		       &info->platform_callback,
> > > -		       sizeof(struct renesas_usbhs_platform_callback));
> > > -		break;
> > > +	if (!info->platform_callback.get_id) {
> > > +		dev_err(&pdev->dev, "no platform callbacks");
> > > +		return -EINVAL;
> > >  	}
> > > +	memcpy(&priv->pfunc,
> > > +	       &info->platform_callback,
> > > +	       sizeof(struct renesas_usbhs_platform_callback));
> > 
> > And here too.
> > 
> > >
> > >  	/* set driver callback functions for platform */
> > >  	dfunc			= &info->driver_callback;
> > > diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
> > > index 3777af8..de1a663 100644
> > > --- a/drivers/usb/renesas_usbhs/common.h
> > > +++ b/drivers/usb/renesas_usbhs/common.h
> > > @@ -282,6 +282,11 @@ struct usbhs_priv {
> > >  	struct clk *clks[2];
> > >  };
> > >
> > > +struct usbhs_of_data {
> > > +	const struct renesas_usbhs_platform_callback	*platform_callback;
> > > +	const struct renesas_usbhs_driver_param		param;
> > > +};
> > > +
> > >  /*
> > >   * common
> > >   */
> > > --
> > > 2.7.4
> > >
>
Greg KH May 21, 2019, 8:03 a.m. UTC | #4 
On Wed, May 15, 2019 at 06:57:17AM +0000, Yoshihiro Shimoda wrote:
> Hi Simon-san,
> 
> > From: Simon Horman, Sent: Monday, May 13, 2019 9:01 PM
> > 
> > On Mon, May 13, 2019 at 11:40:29AM +0900, Yoshihiro Shimoda wrote:
> > > This patch adds a specific struct "usbhs_of_data" to add a new SoC
> > > data easily instead of code basis in the future.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > 
> > Hi Shimoda-san,
> > 
> > the minor suggestion below not withstanding this looks good to me.
> > 
> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> 
> Thank you for your review!
> 
> > > ---
> > >  Changes from v1 [1]:
> > >  - Use sizeof(variable) instead of sizeof(type).
> > >  - Add Geert-san's reviewed-by (thanks!).
> > >
> > > [1]
> > > https://patchwork.kernel.org/patch/10938575/
> > >
> > >  drivers/usb/renesas_usbhs/common.c | 112 +++++++++++++++++++++----------------
> > >  drivers/usb/renesas_usbhs/common.h |   5 ++
> > >  2 files changed, 70 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> > > index 249fbee..0ca89de 100644
> > > --- a/drivers/usb/renesas_usbhs/common.c
> > > +++ b/drivers/usb/renesas_usbhs/common.c
> <snip>
> > > @@ -598,8 +638,15 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
> > >  	if (!info)
> > >  		return NULL;
> > >
> > > +	data = of_device_get_match_data(dev);
> > > +	if (!data)
> > > +		return NULL;
> > > +
> > >  	dparam = &info->driver_param;
> > > -	dparam->type = (uintptr_t)of_device_get_match_data(dev);
> > > +	memcpy(dparam, &data->param, sizeof(data->param));
> > > +	memcpy(&info->platform_callback, data->platform_callback,
> > > +	       sizeof(*data->platform_callback));
> > 
> > Can we avoid the error-proneness of calls to sizeof() as follows?
> > 
> >         *dparam = data->param;
> >         info->platform_callback = *data->platform_callback;
> 
> Since Chris-san has submitted a patch series [1] that is based on this patch today,
> I'd like to submit an incremental patch to avoid the error-proneness in the renesas_usbhs
> (common.c and mod_host.c) later, if possible.
> 
> Greg-san, is it acceptable?

Fine with me!
diff mbox series

Patch

diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
index 249fbee..0ca89de 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -535,53 +535,92 @@  static int usbhsc_drvcllbck_notify_hotplug(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct usbhs_of_data rcar_gen2_data = {
+	.platform_callback = &usbhs_rcar2_ops,
+	.param = {
+		.type = USBHS_TYPE_RCAR_GEN2,
+		.has_usb_dmac = 1,
+		.pipe_configs = usbhsc_new_pipe,
+		.pipe_size = ARRAY_SIZE(usbhsc_new_pipe),
+	}
+};
+
+static const struct usbhs_of_data rcar_gen3_data = {
+	.platform_callback = &usbhs_rcar3_ops,
+	.param = {
+		.type = USBHS_TYPE_RCAR_GEN3,
+		.has_usb_dmac = 1,
+		.pipe_configs = usbhsc_new_pipe,
+		.pipe_size = ARRAY_SIZE(usbhsc_new_pipe),
+	}
+};
+
+static const struct usbhs_of_data rcar_gen3_with_pll_data = {
+	.platform_callback = &usbhs_rcar3_with_pll_ops,
+	.param = {
+		.type = USBHS_TYPE_RCAR_GEN3_WITH_PLL,
+		.has_usb_dmac = 1,
+		.pipe_configs = usbhsc_new_pipe,
+		.pipe_size = ARRAY_SIZE(usbhsc_new_pipe),
+	}
+};
+
+static const struct usbhs_of_data rza1_data = {
+	.platform_callback = &usbhs_rza1_ops,
+	.param = {
+		.type = USBHS_TYPE_RZA1,
+		.pipe_configs = usbhsc_new_pipe,
+		.pipe_size = ARRAY_SIZE(usbhsc_new_pipe),
+	}
+};
+
 /*
  *		platform functions
  */
 static const struct of_device_id usbhs_of_match[] = {
 	{
 		.compatible = "renesas,usbhs-r8a774c0",
-		.data = (void *)USBHS_TYPE_RCAR_GEN3_WITH_PLL,
+		.data = &rcar_gen3_with_pll_data,
 	},
 	{
 		.compatible = "renesas,usbhs-r8a7790",
-		.data = (void *)USBHS_TYPE_RCAR_GEN2,
+		.data = &rcar_gen2_data,
 	},
 	{
 		.compatible = "renesas,usbhs-r8a7791",
-		.data = (void *)USBHS_TYPE_RCAR_GEN2,
+		.data = &rcar_gen2_data,
 	},
 	{
 		.compatible = "renesas,usbhs-r8a7794",
-		.data = (void *)USBHS_TYPE_RCAR_GEN2,
+		.data = &rcar_gen2_data,
 	},
 	{
 		.compatible = "renesas,usbhs-r8a7795",
-		.data = (void *)USBHS_TYPE_RCAR_GEN3,
+		.data = &rcar_gen3_data,
 	},
 	{
 		.compatible = "renesas,usbhs-r8a7796",
-		.data = (void *)USBHS_TYPE_RCAR_GEN3,
+		.data = &rcar_gen3_data,
 	},
 	{
 		.compatible = "renesas,usbhs-r8a77990",
-		.data = (void *)USBHS_TYPE_RCAR_GEN3_WITH_PLL,
+		.data = &rcar_gen3_with_pll_data,
 	},
 	{
 		.compatible = "renesas,usbhs-r8a77995",
-		.data = (void *)USBHS_TYPE_RCAR_GEN3_WITH_PLL,
+		.data = &rcar_gen3_with_pll_data,
 	},
 	{
 		.compatible = "renesas,rcar-gen2-usbhs",
-		.data = (void *)USBHS_TYPE_RCAR_GEN2,
+		.data = &rcar_gen2_data,
 	},
 	{
 		.compatible = "renesas,rcar-gen3-usbhs",
-		.data = (void *)USBHS_TYPE_RCAR_GEN3,
+		.data = &rcar_gen3_data,
 	},
 	{
 		.compatible = "renesas,rza1-usbhs",
-		.data = (void *)USBHS_TYPE_RZA1,
+		.data = &rza1_data,
 	},
 	{ },
 };
@@ -591,6 +630,7 @@  static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
 {
 	struct renesas_usbhs_platform_info *info;
 	struct renesas_usbhs_driver_param *dparam;
+	const struct usbhs_of_data *data;
 	u32 tmp;
 	int gpio;
 
@@ -598,8 +638,15 @@  static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
 	if (!info)
 		return NULL;
 
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return NULL;
+
 	dparam = &info->driver_param;
-	dparam->type = (uintptr_t)of_device_get_match_data(dev);
+	memcpy(dparam, &data->param, sizeof(data->param));
+	memcpy(&info->platform_callback, data->platform_callback,
+	       sizeof(*data->platform_callback));
+
 	if (!of_property_read_u32(dev->of_node, "renesas,buswait", &tmp))
 		dparam->buswait_bwait = tmp;
 	gpio = of_get_named_gpio_flags(dev->of_node, "renesas,enable-gpio", 0,
@@ -607,19 +654,6 @@  static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
 	if (gpio > 0)
 		dparam->enable_gpio = gpio;
 
-	if (dparam->type == USBHS_TYPE_RCAR_GEN2 ||
-	    dparam->type == USBHS_TYPE_RCAR_GEN3 ||
-	    dparam->type == USBHS_TYPE_RCAR_GEN3_WITH_PLL) {
-		dparam->has_usb_dmac = 1;
-		dparam->pipe_configs = usbhsc_new_pipe;
-		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
-	}
-
-	if (dparam->type == USBHS_TYPE_RZA1) {
-		dparam->pipe_configs = usbhsc_new_pipe;
-		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
-	}
-
 	return info;
 }
 
@@ -676,29 +710,13 @@  static int usbhs_probe(struct platform_device *pdev)
 	       &info->driver_param,
 	       sizeof(struct renesas_usbhs_driver_param));
 
-	switch (priv->dparam.type) {
-	case USBHS_TYPE_RCAR_GEN2:
-		priv->pfunc = usbhs_rcar2_ops;
-		break;
-	case USBHS_TYPE_RCAR_GEN3:
-		priv->pfunc = usbhs_rcar3_ops;
-		break;
-	case USBHS_TYPE_RCAR_GEN3_WITH_PLL:
-		priv->pfunc = usbhs_rcar3_with_pll_ops;
-		break;
-	case USBHS_TYPE_RZA1:
-		priv->pfunc = usbhs_rza1_ops;
-		break;
-	default:
-		if (!info->platform_callback.get_id) {
-			dev_err(&pdev->dev, "no platform callbacks");
-			return -EINVAL;
-		}
-		memcpy(&priv->pfunc,
-		       &info->platform_callback,
-		       sizeof(struct renesas_usbhs_platform_callback));
-		break;
+	if (!info->platform_callback.get_id) {
+		dev_err(&pdev->dev, "no platform callbacks");
+		return -EINVAL;
 	}
+	memcpy(&priv->pfunc,
+	       &info->platform_callback,
+	       sizeof(struct renesas_usbhs_platform_callback));
 
 	/* set driver callback functions for platform */
 	dfunc			= &info->driver_callback;
diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
index 3777af8..de1a663 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -282,6 +282,11 @@  struct usbhs_priv {
 	struct clk *clks[2];
 };
 
+struct usbhs_of_data {
+	const struct renesas_usbhs_platform_callback	*platform_callback;
+	const struct renesas_usbhs_driver_param		param;
+};
+
 /*
  * common
  */