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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: wens@csie.org
Cc: Andre Przywara <andre.przywara@arm.com>,
	Vasily Khoruzhick <anarsoul@gmail.com>,
	Yangtao Li <tiny.windzz@gmail.com>,
	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
Subject: Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code
Date: Tue, 28 Nov 2023 15:11:29 +0100	[thread overview]
Message-ID: <9a900a1f-1ebe-4004-855e-4c9cce55c513@linaro.org> (raw)
In-Reply-To: <6356d9d1-2110-4195-805c-531bcdbeeb1f@linaro.org>

On 28/11/2023 10:13, Krzysztof Kozlowski wrote:
> On 28/11/2023 10:09, Chen-Yu Tsai wrote:
>> On Tue, Nov 28, 2023 at 5:02 PM Chen-Yu Tsai <wens@csie.org> wrote:
>>>
>>> On Tue, Nov 28, 2023 at 4:59 PM Chen-Yu Tsai <wens@csie.org> wrote:
>>>>
>>>> On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski
>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>
>>>>> On 28/11/2023 08:50, Chen-Yu Tsai wrote:
>>>>>> On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski
>>>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>>> +     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?
>>>>>>
>>>>>> Not really. This is to get a regmap exported by the device. Syscon's regmap
>>>>>> is not tied to the device at all.
>>>>>
>>>>> I am talking about open-coding existing API. Look at syscon.h.
>>>>
>>>> The underlying implementation is different.
>>>>
>>>> syscon maintains its own mapping of device nodes to regmaps, and creates
>>>> regmaps when none exist. The regmap is not tied to any struct device.
>>>> syscon basically exists outside of the driver model and one has no control
>>>> over what is exposed because it is meant for blocks that are a collection
>>>> of random stuff.
>>>
>>> My bad. I failed to realize there is a platform device driver for syscon,
>>> in addition to the existing "no struct device" implementation.
>>
>> Actually that doesn't do anything on DT platforms as of commit bdb0066df96e
>> ("mfd: syscon: Decouple syscon interface from platform devices"). All the
>> regmaps are, as I previously stated, not tied to any struct device.
> 
> 
> Sorry, it's your third reply, so I don't know what exactly you want to
> discuss.
> 
> This code open-codes existing API. Fix it.
> 
>>
>>>> Here the provider device registers the (limited) regmap it wants to provide,
>>>> tying the new regmap to itself. The consumer gets it via the dev_get_regmap()
>>>> call. The provider has a main function and isn't exposing that part of its
>>>> register map to the outside; only the random bits that were stuffed in are.
>>>>
>>>>>> With this scheme a device to could export just enough registers for the
>>>>>> consumer to use, instead of the whole address range.
>>>>>>
>>>>>> We do this in the R40 clock controller as well, which has some bits that
>>>>>> tweak the ethernet controllers RGMII delay...
>>>>>
>>>>> Not related.
>>>>
>>>> Related as in that is possibly what this code was based on, commit
>>>> 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap
>>>> from external device").
> 
> 
> How duplicating a code is related to R40 controller? Duplicating code is
> generic problem, not specific and not related to your hardware.

I think I understand now what you wanted to say - the "syscon" property
is pointing not to a syscon.

That's the mistake in the bindings - you claim it is a syscon, but it is
not and has nothing to do with syscon. Neither in the bindings nor in
the Linux drivers. This should be fixed.

Best regards,
Krzysztof