* [PATCH] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
@ 2019-05-10 10:48 Yoshihiro Shimoda
2019-05-10 11:20 ` Geert Uytterhoeven
0 siblings, 1 reply; 3+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-10 10:48 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>
---
This patch is related to the following Geert-san's comment.
https://marc.info/?l=linux-renesas-soc&m=155747204111858&w=2
I tested this patch on r8a7795-salvator-x board.
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..8ea5df7 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(struct renesas_usbhs_driver_param));
+ memcpy(&info->platform_callback, data->platform_callback,
+ sizeof(struct renesas_usbhs_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] 3+ messages in thread
* Re: [PATCH] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
2019-05-10 10:48 [PATCH] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums Yoshihiro Shimoda
@ 2019-05-10 11:20 ` Geert Uytterhoeven
2019-05-10 11:56 ` Yoshihiro Shimoda
0 siblings, 1 reply; 3+ messages in thread
From: Geert Uytterhoeven @ 2019-05-10 11:20 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: Greg KH, USB list, Linux-Renesas
Hi Shimoda-san,
On Fri, May 10, 2019 at 12:53 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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>
Thanks for your patch!
Looks correct to me, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
with a few suggestions/questions below...
> --- a/drivers/usb/renesas_usbhs/common.c
> +++ b/drivers/usb/renesas_usbhs/common.c
> @@ -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(struct renesas_usbhs_driver_param));
sizeof(data->param), for increased safety?
> + memcpy(&info->platform_callback, data->platform_callback,
> + sizeof(struct renesas_usbhs_platform_callback));
sizeof(data->platform_callback)?
I'm not really familiar with this driver and with the USB subsystem, but
why is this driver copying so many structs around, instead of just
keeping pointers to the originals?
Is all of that done just so .notify_hotplug() can be overridden, or am I
missing something?
Thanks again!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
2019-05-10 11:20 ` Geert Uytterhoeven
@ 2019-05-10 11:56 ` Yoshihiro Shimoda
0 siblings, 0 replies; 3+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-10 11:56 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Greg KH, USB list, Linux-Renesas
Hi Geert-san,
> From: Geert Uytterhoeven, Sent: Friday, May 10, 2019 8:20 PM
>
> Hi Shimoda-san,
>
> On Fri, May 10, 2019 at 12:53 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> 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>
>
> Thanks for your patch!
>
> Looks correct to me, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Thank you for your review!
> with a few suggestions/questions below...
>
> > --- a/drivers/usb/renesas_usbhs/common.c
> > +++ b/drivers/usb/renesas_usbhs/common.c
>
> > @@ -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(struct renesas_usbhs_driver_param));
>
> sizeof(data->param), for increased safety?
I got it. I'll fix it on v2 (maybe I'll sumbmit it in next week).
> > + memcpy(&info->platform_callback, data->platform_callback,
> > + sizeof(struct renesas_usbhs_platform_callback));
>
> sizeof(data->platform_callback)?
I'll fix it.
> I'm not really familiar with this driver and with the USB subsystem, but
> why is this driver copying so many structs around, instead of just
> keeping pointers to the originals?
# I'm also thinking if just kepping pointers are simple than copying when I made this patch though,
I don't know why because the original author is Morimoto-san :)
But, I guess:
- Some SH boards has the renesas_usbhs_platform_info data.
-- This is related to .platform_data.
--- and it will be deleted after initialization because some reasons (__initdata section?).
---- So, keeping the data, the driver copies it to own allocated memory.
I'll try to simplify the code later.
> Is all of that done just so .notify_hotplug() can be overridden, or am I
> missing something?
Yes, it seems any SH boards don't have the .notify_hotplug.
Best regards,
Yoshihiro Shimoda
> Thanks again!
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-05-10 11:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-10 10:48 [PATCH] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums Yoshihiro Shimoda
2019-05-10 11:20 ` Geert Uytterhoeven
2019-05-10 11:56 ` Yoshihiro Shimoda
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).