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

[Andre Przywara <andre.przywara@arm.com>]

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.


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?

Cheers,
Andre