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

From: "Heiko Stübner" <heiko@sntech.de>
To: Robin Murphy <robin.murphy@arm.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Zhang Rui <rui.zhang@intel.com>,
	Lukasz Luba <lukasz.luba@arm.com>,
	linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	kernel@collabora.com
Subject: Re: [PATCH] thermal: rockchip: shut up GRF warning
Date: Tue, 19 Aug 2025 16:13:25 +0200	[thread overview]
Message-ID: <11865312.F0gNSz5aLb@diego> (raw)
In-Reply-To: <ciuhfx4u6xs2g43hn7nzjfmhaakfip6ndcpftuvg3kgzf52zkt@3vyvcjflmtqm>

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