From: Simon Horman <horms@verge.net.au>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
Date: Mon, 13 May 2019 14:01:25 +0200 [thread overview]
Message-ID: <20190513120124.72pyg5hileurtkry@verge.net.au> (raw)
In-Reply-To: <1557715229-13102-1-git-send-email-yoshihiro.shimoda.uh@renesas.com>
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
>
next prev parent reply other threads:[~2019-05-13 12:01 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2019-05-15 6:57 ` Yoshihiro Shimoda
2019-05-15 8:10 ` Simon Horman
2019-05-21 8:03 ` gregkh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190513120124.72pyg5hileurtkry@verge.net.au \
--to=horms@verge.net.au \
--cc=gregkh@linuxfoundation.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=yoshihiro.shimoda.uh@renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).