| Message ID | 20230210065621.598120-1-sergio.paracuellos@gmail.com |
|---|---|
| Headers | show |
| Series | watchdog: mt7621-wdt: avoid globals and arch dependencies | expand |
On 10/02/2023 07:56, Sergio Paracuellos wrote: > MT7621 SoC provides a system controller node for accessing to some registers. > Add a phandle to this node to avoid using MIPS related arch operations and I don't understand this part. You claim you add a phandle to this node, but your binding suggest you add here a phandle to other node. > includes in watchdog driver code. > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > --- > .../bindings/watchdog/mediatek,mt7621-wdt.yaml | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml > index b2b17fdf4..3c545065f 100644 > --- a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml > +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml > @@ -14,11 +14,18 @@ allOf: > > properties: > compatible: > - const: mediatek,mt7621-wdt > + items: > + - const: mediatek,mt7621-wdt > + - const: syscon > > reg: > maxItems: 1 > > + ralink,sysctl: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + phandle of syscon used to control system registers This needs to be more specific - which syscon? It also does not fit your commit msg. Best regards, Krzysztof
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
On 10/02/2023 12:22, Sergio Paracuellos wrote: > Hi Krzysztof, > > On Fri, Feb 10, 2023 at 11:59 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 10/02/2023 07:56, Sergio Paracuellos wrote: >>> MT7621 SoC provides a system controller node for accessing to some registers. >>> Add a phandle to this node to avoid using MIPS related arch operations and >> >> I don't understand this part. You claim you add a phandle to this node, >> but your binding suggest you add here a phandle to other node. > > Probably my English is not the best here :-). Yes, you are right, I > just want to add a phandle to the 'sysc' node in the current node. Then why do you need syscon compatible here? Best regards, Krzysztof
On 10/02/2023 12:35, Sergio Paracuellos wrote: >>> - 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. > > I am removing global usage and architecture dependencies using a > watchdog driver data structure so I thought the changes were easy > enough to review in this way. It seems they are not according to your > reply :). If preferred I can split this in two commits: > - Avoid globals using and introducing all the related new driver data > structure. > - Add request for regmap syscon from the phandle and remove the > architecture dependent calls and includes. > Yes, such split sounds better. Thanks. Best regards, Krzysztof
Hi all, This series make an update in the MT7621 SoC's watchdog driver code. This SoC already provides a system controller node to access reset status register needed for the watchdog. Instead of using MIPS architecture dependent operations in header 'asm/mach-ralink/ralink_regs.h' use a phandle to the system controller node and use it through regmap APIs from the code. Driver is also using some globals that are not needed at all if a driver data structure is used along the code. Hence, add all new needed stuff inside a new driver data structure. With this changes driver can be properly compile tested. Thanks in advance for reviewing this! Best regards, Sergio Paracuellos Sergio Paracuellos (3): dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers mips: dts: ralink: mt7621: add phandle to system controller node for watchdog watchdog: mt7621-wdt: avoid globals and arch dependencies .../watchdog/mediatek,mt7621-wdt.yaml | 12 +- arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +- drivers/watchdog/Kconfig | 4 +- drivers/watchdog/mt7621_wdt.c | 121 ++++++++++++------ 4 files changed, 95 insertions(+), 45 deletions(-)