linux-kernel - Re: [PATCH 3/3] watchdog: mt7621-wdt: avoid globals and arch dependencies

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]

Message-ID: <21af8c63-f489-8c3f-e1e3-cf976b1d20d0@linaro.org>
Date:   Fri, 10 Feb 2023 12:02:41 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Sergio Paracuellos <sergio.paracuellos@...il.com>,
        linux-watchdog@...r.kernel.org
Cc:     wim@...ux-watchdog.org, linux@...ck-us.net, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, matthias.bgg@...il.com,
        arinc.unal@...nc9.com, tsbogend@...ha.franken.de,
        p.zabel@...gutronix.de, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH 3/3] watchdog: mt7621-wdt: avoid globals and arch
 dependencies

On 10/02/2023 07:56, Sergio Paracuellos wrote:
> MT7621 SoC has a system controller node. Watchdog need to access to reset
> status register. Ralink architecture and related driver are old and from
> the beggining they ar providing some architecture dependent operations
> for accessing this shared registers through 'asm/mach-ralink/ralink_regs.h'
> header file. However this is not ideal from a driver perspective which can
> just access to the system controller registers in am arch independent way
> using regmap syscon APIs. Hence, add a new structure for driver data and
> use it along the code. This way architecture dependencies and global vars
> are not needed anymore. Update Kconfig accordingly to select new added
> dependencies and allow driver to be compile tested.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@...il.com>
> ---
>  drivers/watchdog/Kconfig      |   4 +-
>  drivers/watchdog/mt7621_wdt.c | 121 ++++++++++++++++++++++------------
>  2 files changed, 83 insertions(+), 42 deletions(-)
> 


> -
>  static int mt7621_wdt_probe(struct platform_device *pdev)
>  {
> +	struct device_node *np = pdev->dev.of_node;
>  	struct device *dev = &pdev->dev;
> -	mt7621_wdt_base = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(mt7621_wdt_base))
> -		return PTR_ERR(mt7621_wdt_base);
> +	struct watchdog_device *mt7621_wdt;
> +	struct mt7621_wdt_data *drvdata;
> +	int err;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
>  
> -	mt7621_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
> -	if (!IS_ERR(mt7621_wdt_reset))
> -		reset_control_deassert(mt7621_wdt_reset);
> +	drvdata->sysc = syscon_regmap_lookup_by_phandle(np, "ralink,sysctl");
> +	if (IS_ERR(drvdata->sysc))
> +		return PTR_ERR(drvdata->sysc);

You claim in commit title that you remove some global usage, but you add
here several new features and refactor the code significantly. You need
to split refactorings, improvements from completely new features. The
entire patch is very difficult to understand in current form.

Best regards,
Krzysztof