* [PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
@ 2019-05-13 2:40 Yoshihiro Shimoda
2019-05-13 12:01 ` Simon Horman
0 siblings, 1 reply; 5+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-13 2:40 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, linux-renesas-soc, Yoshihiro Shimoda
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(-)
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
*/
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
2019-05-13 2:40 [PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums Yoshihiro Shimoda
@ 2019-05-13 12:01 ` Simon Horman
2019-05-15 6:57 ` Yoshihiro Shimoda
0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2019-05-13 12:01 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: gregkh, linux-usb, linux-renesas-soc
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
2019-05-13 12:01 ` Simon Horman
@ 2019-05-15 6:57 ` Yoshihiro Shimoda
2019-05-15 8:10 ` Simon Horman
2019-05-21 8:03 ` gregkh
0 siblings, 2 replies; 5+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-15 6:57 UTC (permalink / raw)
To: Simon Horman, gregkh; +Cc: linux-usb, linux-renesas-soc
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
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
2019-05-15 6:57 ` Yoshihiro Shimoda
@ 2019-05-15 8:10 ` Simon Horman
2019-05-21 8:03 ` gregkh
1 sibling, 0 replies; 5+ messages in thread
From: Simon Horman @ 2019-05-15 8:10 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: gregkh, linux-usb, linux-renesas-soc
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
> > >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
2019-05-15 6:57 ` Yoshihiro Shimoda
2019-05-15 8:10 ` Simon Horman
@ 2019-05-21 8:03 ` gregkh
1 sibling, 0 replies; 5+ messages in thread
From: gregkh @ 2019-05-21 8:03 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: Simon Horman, linux-usb, linux-renesas-soc
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!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-21 8:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13 2:40 [PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums Yoshihiro Shimoda
2019-05-13 12:01 ` Simon Horman
2019-05-15 6:57 ` Yoshihiro Shimoda
2019-05-15 8:10 ` Simon Horman
2019-05-21 8:03 ` gregkh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).