Re: [PATCH] thermal: rockchip: shut up GRF warning

["Heiko Stübner" <heiko@sntech.de>]

Am Dienstag, 19. August 2025, 15:56:42 Mitteleuropäische Sommerzeit schrieb Sebastian Reichel:
> Hello Robin,
> 
> On Tue, Aug 19, 2025 at 01:42:42PM +0100, Robin Murphy wrote:
> > > +	if (thermal->chip->grf_mode != GRF_NONE) {
> > > +		thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > > +		if (IS_ERR(thermal->grf)) {
> > > +			ret = PTR_ERR(thermal->grf);
> > > +			if (thermal->chip->grf_mode == GRF_OPTIONAL)
> > > +				dev_warn(dev, "Missing rockchip,grf property\n");
> > > +			else
> > > +				return dev_err_probe(dev, ret, "Missing rockchip,grf property\n");
> > > +		}
> > > +	}
> > 
> > Nit: Does the lookup itself need to be made conditional? I think I'd
> > also agree that the "optional" mode seems suspect, so potentially it
> > could be a whole lot simpler, e.g.:
> > 
> > 	thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > 	if (IS_ERR(thermal->grf) && thermal->chip->grf_required)
> > 		return dev_err_probe(dev, PTR_ERR(thermal->grf),
> > 				     "Missing rockchip,grf property\n");
> 
> I came up with the enum, because I think most platforms having
> "optional" GRF support actually require it, so I want to keep the
> warning. At the same time I don't want to mark them as GRF required
> at this point, since that is potentially a DT ABI break. It really
> needs to be checked per platform in the TRM and/or by testing on
> real HW. With this patch we can easily handle this platform by
> platform by moving them from GRF_OPTIONAL to GRF_MANDATORY without
> affecting the unchecked platforms. Also switching a platform from
> optional to required needs to be reflected in the DT binding. So
> this involves a lot of work. I think it makes sense to carry the
> slightly complex check in the driver's probe routine for now.

I did go digging now ... there are 3 variants marked as "optional":

  rk3366_tsadc_data ->  rockchip,rk3366-tsadc
	never added any DTS

  rk3399_tsadc_data ->  rockchip,rk3399-tsadc
	commit 95c27ba7bd92 ("arm64: dts: rockchip: add thermal nodes for rk3399 SoCs")
	added the tsdadc node together with its rockchip,grf reference

   rk3568_tsadc_data   rockchip,rk3568-tsadc
	commit 1330875dc2a3 ("arm64: dts: rockchip: add rk3568 tsadc nodes")
	added the tsdadc node together with its rockchip,grf reference

So none of the platforms had ever a phase where we had the tsadc without
its grf-reference. So making the GRF mandatory for those, will not create
breakage. We could even tighten the binding to make that explicit.


Heiko