On 24/01/2024 04:37, Saravana Kannan wrote: > On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxx> wrote: >> >> On 23/01/2024 18:30, Peter Griffin wrote: >>>>> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n"); >>>>> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit)) >>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >>>>> if (ret) >>>>> return ret; >>>>> >>>>> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) { >>>>> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, >>>>> - "samsung,syscon-phandle"); >>>>> - if (IS_ERR(wdt->pmureg)) >>>>> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg), >>>>> - "syscon regmap lookup failed.\n"); >>>> >>>> >>>> Continuing topic from the binding: I don't see how you handle probe >>>> deferral, suspend ordering. >>> >>> The current implementation is simply relying on exynos-pmu being >>> postcore_initcall level. >>> >>> I was just looking around for any existing Linux APIs that could be a >>> more robust solution. It looks like >>> >>> of_parse_phandle() >>> and >>> of_find_device_by_node(); >>> >>> Are often used to solve this type of probe deferral issue between >>> devices. Is that what you would recommend using? Or is there something >>> even better? >> >> I think you should keep the phandle and then set device link based on >> of_find_device_by_node(). This would actually improve the code, because >> syscon_regmap_lookup_by_phandle() does not create device links. > > I kinda agree with this. Just because we no longer use a syscon API to > find the PMU register address doesn't mean the WDT doesn't depend on > the PMU. > > However, I think we should move to a generic "syscon" property. Then I > can add support for "syscon" property to fw_devlink and then things > will just work in terms of probe ordering, suspend/resume and also > showing the dependency in DT even if you don't use the syscon APIs. > > Side note 1: > > I think we really should officially document a generic syscon DT > property similar to how we have a generic "clocks" or "dmas" property. > Then we can have a syscon_get_regmap() that's like so: > > struct regmap *syscon_get_regmap(struct device *dev) > { > return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon"); > } > > Instead of every device defining its own bespoke DT property to do the > exact same thing. I did a quick "back of the envelope" grep on this > and I get about 143 unique properties just to get the syscon regmap. > $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e > 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l > 143 Sorry, generic "syscon" property won't fly with DT maintainers, because there is no such thing as syscon in any of hardware. > > Side note 2: > > How are we making sure that it's the exynos-pmu driver that ends up > probing the PMU and not the generic syscon driver? Both of these are > platform drivers. And the exynos PMU device lists both the exynos > compatible string and the syscon property. Is it purely a link order > coincidence? initcall ordering Best regards, Krzysztof