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

On Fri, 01 Sep 2023 22:01:51 +0900,
Geert Uytterhoeven wrote:
> 
> 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?...

Sorry.
This is the wrong version.
I will update this to the correct one as I will also fix other parts.

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

-- 
Yosinori Sato