Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Andre Przywara <andre.przywara@arm.com>
Cc: Vasily Khoruzhick <anarsoul@gmail.com>,
	Yangtao Li <tiny.windzz@gmail.com>, Chen-Yu Tsai <wens@csie.org>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Samuel Holland <samuel@sholland.org>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Zhang Rui <rui.zhang@intel.com>,
	Lukasz Luba <lukasz.luba@arm.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Martin Botka <martin.botka@somainline.org>,
	Bob McChesney <bob@electricworry.net>,
	linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@lists.linux.dev, Icenowy Zheng <icenowy@aosc.io>
Subject: Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code
Date: Tue, 28 Nov 2023 15:48:18 +0100	[thread overview]
Message-ID: <4e90608e-aca5-4b57-be76-350ad54f9e7c@linaro.org> (raw)
In-Reply-To: <20231128143309.38a4ce61@donnerap.manchester.arm.com>

On 28/11/2023 15:33, Andre Przywara wrote:
> On Tue, 28 Nov 2023 08:43:32 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> Hi,
> 
>> On 28/11/2023 01:58, Andre Przywara wrote:
>>>  
>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
>>> +{
>>> +	struct device_node *syscon_node;
>>> +	struct platform_device *syscon_pdev;
>>> +	struct regmap *regmap = NULL;
>>> +
>>> +	syscon_node = of_parse_phandle(node, "syscon", 0);  
>>
>> Nope. For the 100th time, this cannot be generic.
> 
> OK. Shall this name refer to the required functionality (temperature
> offset fix) or to the target syscon node (like allwinner,misc-syscon).
> The problem is that this is really a syscon, as in: "random collection of
> bits that we didn't know where else to put in", so "syscon" alone actually
> says it all.

Every syscon is a "random collection of bits...", but not every "random
collection of bits..." is a syscon.

Your target device does not implement syscon nodes. Your Linux
implementation does not use it as syscon. Therefore if something does
not look like syscon and does not behave like syscon, it is not a syscon.

I looked at the bit and this is SRAM, not syscon. I am sorry, but it is
something entirely different and we have a binding for it: "sram", I think.

> 
> 
> And btw: it would have been about the same effort (and more helpful!) to
> type:
> 
> "This cannot be generic, please check writing-bindings.rst."    ;-)
> 
>>
>>> +	if (!syscon_node)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	syscon_pdev = of_find_device_by_node(syscon_node);
>>> +	if (!syscon_pdev) {
>>> +		/* platform device might not be probed yet */
>>> +		regmap = ERR_PTR(-EPROBE_DEFER);
>>> +		goto out_put_node;
>>> +	}
>>> +
>>> +	/* If no regmap is found then the other device driver is at fault */
>>> +	regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
>>> +	if (!regmap)
>>> +		regmap = ERR_PTR(-EINVAL);  
>>
>> Aren't you open-coding existing API to get regmap from syscon?
> 
> That's a good point, I lifted that code from sun8i-emac.c, where we have
> the exact same problem. 
> Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT
> node to have "syscon" in its compatible string list, which we
> don't have. We actually explicitly dropped this for the A64 (with
> 1f1f5183981d70bf0950), and never added this for later SoCs in the first place.
> I guess we could add it back, and it would work for this case here (tested
> that), but then cannot replace the sun8i-emac.c code, because that would
> break older DTs.
> So is there any chance we can drop the requirement for "syscon" in the
> compatible string list, in the implementation of
> syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different
> prototype? Or is there another existing API that does this already?

I must correct myself: I was wrong. You are not open-coding, because as
pointed out, this is not a phandle to syscon (even if you call it like
"syscon").

The code is fine, maybe except missing links (needs double checking,
because maybe regmap creates links?). The DT binding and DTS needs
fixing, because it is not a syscon.

Best regards,
Krzysztof