Re: [PATCH v8 14/15] drm/xe/hwmon: Add CRI PCIe temperature sensor offset

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Oct 08, 2026 at 12:11:01AM +0530, Poosa, Karthik wrote:
> 
> On 07-10-2026 23:57, Rodrigo Vivi wrote:
> > On Tue, Oct 06, 2026 at 09:05:33AM +0530, Karthik Poosa wrote:
> > > Describe the Crescent Island PCIe temperature sensor at byte offset 20 in
> > > READ_THERMAL_DATA.
> > > 
> > > Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_pci.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > index 1cbd49933b2c..c7ab7b1abd25 100644
> > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > @@ -482,6 +482,7 @@ static const struct xe_device_desc cri_desc = {
> > >   	.has_i2c = true,
> > >   	.has_mbx_power_limits = true,
> > >   	.has_mbx_thermal_info = true,
> > > +	.pcie_temp_sensor_offset = 20,
> > wrong place and bad name and it should be squashed to the previous patch.
> by wrong place you mean, order here in the list alphabetically ?
> > 
> > But also, please disregard the rv-b that I gave there... seeing this again
> > this is a bad naming... I'm also bad with naming, but pcie_ as a prefix
> > seems something related to the pci spec itself, what is not the case,
> > so we need something different....
> how about .mbx_pcie_sensor_offset ?

as I told I'm bad with naming, so I asked AI and I fully agree with AI now:

"""
Recommendation: drop the info field and follow the existing TEMP_INDEX_MCTRL convention:
#define TEMP_INDEX_PCIE          10
#define BMG_G21_TEMP_INDEX_PCIE   8
#define CRI_TEMP_INDEX_PCIE      20

A small xe_hwmon_pcie_temp_index() helper would choose between them.
Because the TEMP_INDEX_ prefix already reads as "index in the thermal mailbox",
nobody will mistake it for a PCIe-spec value. This also removes the G21 special
case from xe_pci.c.

If they want to keep the info field, a name like mbx_thermal_pcie_index makes
it clear it's a mailbox thing (it pairs with has_mbx_thermal_info).

I'd still push for the local defines.
"""

So, let's go for the local defines for this. no need for the info flag.
And a single patch.

Thanks,
Rodrigo.

> > 
> > >   	.has_mert = true,
> > >   	.has_pre_prod_wa = 1,
> > >   	.has_soc_remapper_sysctrl = true,
> > > -- 
> > > 2.25.1
> > >