Message-ID: <21af8c63-f489-8c3f-e1e3-cf976b1d20d0@linaro.org>
Date:   Fri, 10 Feb 2023 12:02:41 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH 3/3] watchdog: mt7621-wdt: avoid globals and arch
 dependencies
Content-Language: en-US
To:     Sergio Paracuellos <sergio.paracuellos@gmail.com>,
        linux-watchdog@vger.kernel.org
Cc:     wim@linux-watchdog.org, linux@roeck-us.net, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
        arinc.unal@arinc9.com, tsbogend@alpha.franken.de,
        p.zabel@pengutronix.de, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, linux-mips@vger.kernel.org
References: <20230210065621.598120-1-sergio.paracuellos@gmail.com>
 <20230210065621.598120-4-sergio.paracuellos@gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230210065621.598120-4-sergio.paracuellos@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@gmail.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