linux-kernel - Re: [DO NOT MERGE v8 08/36] clocksource: sh

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 29 May 2024 15:55:53 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Yoshinori Sato <ysato@...rs.sourceforge.jp>
Cc: linux-sh@...r.kernel.org, Damien Le Moal <dlemoal@...nel.org>,
	Niklas Cassel <cassel@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, David Airlie <airlied@...il.com>,
	Daniel Vetter <daniel@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Magnus Damm <magnus.damm@...il.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Rich Felker <dalias@...c.org>,
	John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>,
	Lee Jones <lee@...nel.org>, Helge Deller <deller@....de>,
	Heiko Stuebner <heiko.stuebner@...rry.de>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Chris Morgan <macromorgan@...mail.com>,
	Sebastian Reichel <sre@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	Masahiro Yamada <masahiroy@...nel.org>, Baoquan He <bhe@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Guenter Roeck <linux@...ck-us.net>,
	Kefeng Wang <wangkefeng.wang@...wei.com>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Azeem Shaikh <azeemshaikh38@...il.com>, Guo Ren <guoren@...nel.org>,
	Max Filippov <jcmvbkbc@...il.com>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Herve Codina <herve.codina@...tlin.com>,
	Anup Patel <apatel@...tanamicro.com>,
	Jacky Huang <ychuang3@...oton.com>,
	Hugo Villeneuve <hvilleneuve@...onoff.com>,
	Jonathan Corbet <corbet@....net>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	Sam Ravnborg <sam@...nborg.org>,
	Javier Martinez Canillas <javierm@...hat.com>,
	Sergey Shtylyov <s.shtylyov@....ru>,
	Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
	linux-ide@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
	linux-clk@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linux-pci@...r.kernel.org, linux-serial@...r.kernel.org,
	linux-fbdev@...r.kernel.org
Subject: Re: [DO NOT MERGE v8 08/36] clocksource: sh_tmu: CLOCKSOURCE support.

On Wed, May 29, 2024 at 05:00:54PM +0900, Yoshinori Sato wrote:
> Allows initialization as CLOCKSOURCE.

..

> -	dev_info(&ch->tmu->pdev->dev, "ch%u: used for %s clock events\n",
> -		 ch->index, periodic ? "periodic" : "oneshot");
> +	pr_info("%s ch%u: used for %s clock events\n",
> +		ch->tmu->name, ch->index, periodic ? "periodic" : "oneshot");

This is a step back change. We should use dev_*() if we have a device
available. And I believe this is the case (at least for the previous boards),
no?

..

> -	ch->irq = platform_get_irq(tmu->pdev, index);
> +	if (tmu->np)
> +		ch->irq = of_irq_get(tmu->np, index);
> +	else if (tmu->pdev)
> +		ch->irq = platform_get_irq(tmu->pdev, index);

I found these changes counterproductive. Instead better to have up to three
files to cover:
- the common code (library)
- the platform device support
- the pure OF support.

..

> -	res = platform_get_resource(tmu->pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&tmu->pdev->dev, "failed to get I/O memory\n");
> -		return -ENXIO;
> +	if (tmu->pdev) {
> +		res = platform_get_resource(tmu->pdev, IORESOURCE_MEM, 0);
> +		if (!res) {
> +			pr_err("sh_tmu failed to get I/O memory\n");
> +			return -ENXIO;
> +		}
> +
> +		tmu->mapbase = ioremap(res->start, resource_size(res));

devm_platform_ioremap_resource() should be good to have.
Again, consider proper splitting.

>  	}
> +	if (tmu->np)
> +		tmu->mapbase = of_iomap(tmu->np, 0);

So, how many boards are non-OF compatible? Maybe makes sense to move them to OF
and drop these platform code entirely from everywhere?

..

> +	tmu->name = dev_name(&pdev->dev);
> +	tmu->clk = clk_get(&tmu->pdev->dev, "fck");

devm_ approach can help a lot in case of platform device code.

> +	if (IS_ERR(tmu->clk)) {
> +		dev_err(&tmu->pdev->dev, "cannot get clock\n");
> +		return PTR_ERR(tmu->clk);

		return dev_err_probe() ?

> +	}

-- 
With Best Regards,
Andy Shevchenko