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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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.

Best regards,
Krzysztof