Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Krzysztof Kozlowski]

On 14/02/2023 13:10, Guenter Roeck wrote:
> On 2/14/23 00:31, Krzysztof Kozlowski wrote:
>> On 13/02/2023 21:05, 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 are using 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 an arch independent way
>>> using regmap syscon APIs. Update Kconfig accordingly to select new added
>>> dependencies and allow driver to be compile tested.
>>>
>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
>>> ---
>>>   drivers/watchdog/Kconfig      |  4 +++-
>>>   drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++-----
>>>   2 files changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index b64bc49c7..cf752ad64 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -1865,7 +1865,9 @@ config GXP_WATCHDOG
>>>   config MT7621_WDT
>>>   	tristate "Mediatek SoC watchdog"
>>>   	select WATCHDOG_CORE
>>> -	depends on SOC_MT7620 || SOC_MT7621
>>> +	select REGMAP_MMIO
>>> +	select MFD_SYSCON
>>> +	depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
>>>   	help
>>>   	  Hardware driver for the Mediatek/Ralink MT7621/8 SoC Watchdog Timer.
>>>   
>>> diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c
>>> index 40fb2c9ba..22e979bdd 100644
>>> --- a/drivers/watchdog/mt7621_wdt.c
>>> +++ b/drivers/watchdog/mt7621_wdt.c
>>> @@ -15,8 +15,8 @@
>>>   #include <linux/moduleparam.h>
>>>   #include <linux/platform_device.h>
>>>   #include <linux/mod_devicetable.h>
>>> -
>>> -#include <asm/mach-ralink/ralink_regs.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/regmap.h>
>>>   
>>>   #define SYSC_RSTSTAT			0x38
>>>   #define WDT_RST_CAUSE			BIT(1)
>>> @@ -34,6 +34,7 @@
>>>   struct mt7621_wdt_data {
>>>   	void __iomem *base;
>>>   	struct reset_control *rst;
>>> +	struct regmap *sysc;
>>>   	struct watchdog_device wdt;
>>>   };
>>>   
>>> @@ -104,9 +105,12 @@ static int mt7621_wdt_stop(struct watchdog_device *w)
>>>   	return 0;
>>>   }
>>>   
>>> -static int mt7621_wdt_bootcause(void)
>>> +static int mt7621_wdt_bootcause(struct mt7621_wdt_data *d)
>>>   {
>>> -	if (rt_sysc_r32(SYSC_RSTSTAT) & WDT_RST_CAUSE)
>>> +	u32 val;
>>> +
>>> +	regmap_read(d->sysc, SYSC_RSTSTAT, &val);
>>> +	if (val & WDT_RST_CAUSE)
>>>   		return WDIOF_CARDRESET;
>>>   
>>>   	return 0;
>>> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
>>>   	if (!drvdata)
>>>   		return -ENOMEM;
>>>   
>>> +	drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
>>> +	if (IS_ERR(drvdata->sysc))
>>> +		return PTR_ERR(drvdata->sysc);
>>
>> This should be the backup/error path for original code using syscon
>> property. Looking up by compatible is really not portable/re-usable.
>>
> 
> I really disagree here.
> 
> $ git grep syscon_regmap_lookup_by_compatible | wc
>       90     326    8940
> 
> I have not yet reviewed this code, but I do not accept this argument against it.

First, argument that bad pattern is being used is not an argument to
keep it and repeat it.

Second, we already had examples that:
1. Author used syscon_regmap_lookup_by_compatible() and assumed "we will
never add new variant/soc".
2. Then turns out that new variants are obviously added and
syscon_regmap_lookup_by_compatible() stops scaling.

Whether any new variant/compatible/platform can appear for this watchdog
- I don't know.

Third, with syscon_regmap_lookup_by_compatible() you have undocumented
(not in the binding) dependency between blocks which:
a. stops any reusability,
b. affects device links and probe ordering (simply - there is no, device
must defer probe),
c. is simply undocumented.

The usage of syscon_regmap_lookup_by_compatible() has clear drawbacks
thus new code should rather use syscon phandles which solve all of above.

Best regards,
Krzysztof