| Message ID | 2d323328fba6ac55a1c3cdcefe909fad3ab0d9dc.1693444193.git.ysato@users.sourceforge.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DeviceTree support for SH7751 based boards. | expand |
On 31/08/2023 03:11, Yoshinori Sato wrote:
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
Subject: Everything is an update... Drop also full stop.
You also miss commit msgs here and other places. No, please write proper
messages explaining why you are doing it.
Best regards,
Krzysztof
Hi Sato-san, On Thu, Aug 31, 2023 at 7:22 PM Yoshinori Sato <ysato@users.sourceforge.jp> wrote: > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> 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
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 > <ysato@users.sourceforge.jp> wrote: > > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> > > 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 -- 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 --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c index beffff81c00f..de65e1c96780 100644 --- 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); - 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; + } } 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; + } + /* 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; platform_set_drvdata(pdev, tmu);
Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> --- drivers/clocksource/sh_tmu.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)