Re: [RESEND RFC PATCH 07/12] clocksource: Update sh_tmu of handling.

Hi Sato-san,

On Thu, Aug 31, 2023 at 7:22 PM Yoshinori Sato
<[email protected]> wrote:
> Signed-off-by: Yoshinori Sato <[email protected]>

Thanks for your patch!

> --- a/drivers/clocksource/sh_tmu.c
> +++ b/drivers/clocksource/sh_tmu.c
> @@ -420,9 +420,6 @@ static void sh_tmu_register_clockevent(struct sh_tmu_channel *ch,
>         ced->suspend = sh_tmu_clock_event_suspend;
>         ced->resume = sh_tmu_clock_event_resume;
>
> -       dev_info(&ch->tmu->pdev->dev, "ch%u: used for clock events\n",
> -                ch->index);
> -

Why?

>         clockevents_config_and_register(ced, ch->tmu->rate, 0x300, 0xffffffff);
>
>         ret = request_irq(ch->irq, sh_tmu_interrupt,
> @@ -500,12 +497,12 @@ static int sh_tmu_parse_dt(struct sh_tmu_device *tmu)
>         tmu->model = SH_TMU;
>         tmu->num_channels = 3;
>
> -       of_property_read_u32(np, "#renesas,channels", &tmu->num_channels);
> -
> -       if (tmu->num_channels != 2 && tmu->num_channels != 3) {
> -               dev_err(&tmu->pdev->dev, "invalid number of channels %u\n",
> -                       tmu->num_channels);
> -               return -EINVAL;
> +       if (of_property_read_u32(np, "#renesas,channels", &tmu->num_channels)) {
> +               if (tmu->num_channels != 2 && tmu->num_channels != 3) {
> +                       dev_err(&tmu->pdev->dev,
> +                               "invalid number of channels %u\n", tmu->num_channels);
> +                       return -EINVAL;
> +               }

Why?
I understand TMU on SH7751 has 5 channels, so just extended the check?

Note that of_property_read_u32() returns zero on success.

>         }
>
>         return 0;
> @@ -513,7 +510,6 @@ static int sh_tmu_parse_dt(struct sh_tmu_device *tmu)
>
>  static int sh_tmu_setup(struct sh_tmu_device *tmu, struct platform_device *pdev)
>  {
> -       unsigned int i;
>         int ret;
>
>         tmu->pdev = pdev;
> @@ -535,6 +531,11 @@ static int sh_tmu_setup(struct sh_tmu_device *tmu, struct platform_device *pdev)
>                 return -ENXIO;
>         }
>
> +       if (tmu->num_channels < 2) {
> +               dev_err(&tmu->pdev->dev, "Invalid channels.\n");
> +               return -ENXIO;
> +       }
> +

Why?

>         /* Get hold of clock. */
>         tmu->clk = clk_get(&tmu->pdev->dev, "fck");
>         if (IS_ERR(tmu->clk)) {
> @@ -573,12 +574,12 @@ static int sh_tmu_setup(struct sh_tmu_device *tmu, struct platform_device *pdev)
>          * Use the first channel as a clock event device and the second channel
>          * as a clock source.
>          */
> -       for (i = 0; i < tmu->num_channels; ++i) {
> -               ret = sh_tmu_channel_setup(&tmu->channels[i], i,
> -                                          i == 0, i == 1, tmu);
> -               if (ret < 0)
> -                       goto err_unmap;
> -       }
> +       ret = sh_tmu_channel_setup(&tmu->channels[0], 0, false, true, tmu);
> +       if (ret < 0)
> +               goto err_unmap;
> +       ret = sh_tmu_channel_setup(&tmu->channels[1], 1, true, false, tmu);
> +       if (ret < 0)
> +               goto err_unmap;

Why,  oh why?...

>
>         platform_set_drvdata(pdev, tmu);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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