lkml.org 
[lkml]   [2023]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code
From
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@gmail.com>
>> ---
>> 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.

Guenter

\
 
 \ /
  Last update: 2023-03-27 00:21    [W:0.081 / U:0.864 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site