Hello Geert,
I am sorry for the late reply.
Thank you for your feedback.
> Subject: Re: [PATCH 1/3][can-next] can: rcar_can: Fix erroneous registration
>
> Hi Fabrizio,
>
> On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
> <fabrizio.castro@xxxxxxxxxxxxxx> wrote:
> > Assigning 2 to "renesas,can-clock-select" tricks the driver into
> > registering the CAN interface, even though we don't want that.
> > This patch fixes this problem and also allows for architectures
> > missing some of the clocks (e.g. RZ/G2) to behave as expected.
>
> I think the fix for the second issue is not needed (see my reply to the other
> patch).
>
> > Fixes: 862e2b6af9413b43 ("can: rcar_can: support all input clocks")
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> > Signed-off-by: Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>
> > ---
> >
> > This patch applies on linux-can-next-for-4.19-20180727
> >
> > drivers/net/can/rcar/rcar_can.c | 43 +++++++++++++++++++++++++++++++++--------
> > 1 file changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> > index 11662f4..fbd9284 100644
> > --- a/drivers/net/can/rcar/rcar_can.c
> > +++ b/drivers/net/can/rcar/rcar_can.c
> > @@ -21,9 +21,13 @@
> > #include <linux/clk.h>
> > #include <linux/can/platform/rcar_can.h>
> > #include <linux/of.h>
> > +#include <linux/of_device.h>
> >
> > #define RCAR_CAN_DRV_NAME "rcar_can"
> >
> > +#define RCAR_SUPPORTED_CLOCKS (BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
> > + BIT(CLKR_CLKEXT))
> > +
> > /* Mailbox configuration:
> > * mailbox 60 - 63 - Rx FIFO mailboxes
> > * mailbox 56 - 59 - Tx FIFO mailboxes
> > @@ -745,10 +749,12 @@ static int rcar_can_probe(struct platform_device *pdev)
> > u32 clock_select = CLKR_CLKP1;
> > int err = -ENODEV;
> > int irq;
> > + uintptr_t allowed_clks = RCAR_SUPPORTED_CLOCKS;
> >
> > if (pdev->dev.of_node) {
> > of_property_read_u32(pdev->dev.of_node,
> > "renesas,can-clock-select", &clock_select);
>
>
> > + allowed_clks = (uintptr_t)of_device_get_match_data(&pdev->dev);
> > } else {
> > pdata = dev_get_platdata(&pdev->dev);
> > if (!pdata) {
> > @@ -789,7 +795,7 @@ static int rcar_can_probe(struct platform_device *pdev)
> > goto fail_clk;
> > }
> >
> > - if (clock_select >= ARRAY_SIZE(clock_names)) {
> > + if (!(BIT(clock_select) & allowed_clks)) {
>
> Hence you can just use RCAR_SUPPORTED_CLOCKS directly,
> or better, just check clock_names[clock_select] != NULL, ...
I rather use RCAR_SUPPORTED_CLOCKS then, it's safer. I'll send a v2 for you to look at.
>
> > err = -EINVAL;
> > dev_err(&pdev->dev, "invalid CAN clock selected\n");
> > goto fail_clk;
> > @@ -899,13 +905,34 @@ static int __maybe_unused rcar_can_resume(struct device *dev)
> > static SIMPLE_DEV_PM_OPS(rcar_can_pm_ops, rcar_can_suspend, rcar_can_resume);
> >
> > static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
> > - { .compatible = "renesas,can-r8a7778" },
> > - { .compatible = "renesas,can-r8a7779" },
> > - { .compatible = "renesas,can-r8a7790" },
> > - { .compatible = "renesas,can-r8a7791" },
> > - { .compatible = "renesas,rcar-gen1-can" },
> > - { .compatible = "renesas,rcar-gen2-can" },
> > - { .compatible = "renesas,rcar-gen3-can" },
> > + {
> > + .compatible = "renesas,can-r8a7778",
> > + .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > + },
> > + {
> > + .compatible = "renesas,can-r8a7779",
> > + .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > + },
> > + {
> > + .compatible = "renesas,can-r8a7790",
> > + .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > + },
> > + {
> > + .compatible = "renesas,can-r8a7791",
> > + .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > + },
> > + {
> > + .compatible = "renesas,rcar-gen1-can",
> > + .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > + },
> > + {
> > + .compatible = "renesas,rcar-gen2-can",
> > + .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > + },
> > + {
> > + .compatible = "renesas,rcar-gen3-can",z
> > + .data = (void *)RCAR_SUPPORTED_CLOCKS,
> > + },
> > { }
>
> ... and all of the above can dropped.
>
> > };
> > MODULE_DEVICE_TABLE(of, rcar_can_of_table);
>
> BTW, why does the custom "renesas,can-clock-select" exist?
> If guess the standard "assigned-clock-parents" wasn't suitable because there's
> no actual defined clock for which you can change the parent?
>
> Why do you need manual selection? Can't the driver just pick the most suitable
> available clock, like other drivers (e.g. sh-sci) do?
Please have a look at 862e2b6af941 ("can: rcar_can: support all input clocks").
Maybe this could be improved in the future?
Thanks,
Fab
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> 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
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
