diff mbox series

usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums

Message ID 1557485323-3349-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Headers show
Series usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums | expand

Commit Message

Yoshihiro Shimoda May 10, 2019, 10:48 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>
---
 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(-)

Comments

Geert Uytterhoeven May 10, 2019, 11:20 a.m. UTC | #1
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
Yoshihiro Shimoda May 10, 2019, 11:56 a.m. UTC | #2
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
diff mbox series

Patch

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
  */