Re: [PATCH v2] drm/xe/hwmon: expose fan speed

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

On Mon, Mar 10, 2025 at 07:26:38PM +0200, Raag Jadav wrote:
> On Mon, Mar 10, 2025 at 01:01:11PM -0400, Rodrigo Vivi wrote:
> > On Sat, Mar 08, 2025 at 06:12:16PM +0200, Raag Jadav wrote:
> > > On Fri, Mar 07, 2025 at 09:33:05AM -0500, Rodrigo Vivi wrote:
> > > > On Fri, Mar 07, 2025 at 06:21:12PM +0530, Raag Jadav wrote:
> > > 
> > > ...
> > > 
> > > > > @@ -454,6 +485,7 @@ static const struct hwmon_channel_info * const hwmon_info[] = {
> > > > >  	HWMON_CHANNEL_INFO(curr, HWMON_C_LABEL, HWMON_C_CRIT | HWMON_C_LABEL),
> > > > >  	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL),
> > > > >  	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT | HWMON_E_LABEL, HWMON_E_INPUT | HWMON_E_LABEL),
> > > > > +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT, HWMON_F_INPUT),
> > > > 
> > > > here as well... I believe we should not expose the fan files if !has_fan_control
> > > 
> > > I'm not sure if I'm following here. We'll still to define channel attributes
> > > to be able to let hwmon expose them at runtime based on has_fan_control flag.
> > > 
> > > One way I could think of is to do a manual sysfs_create_file(), but at that
> > > point it defeats the purpose of having hwmon.
> > 
> > Well, there might be a way. I don't believe we are the only hwmon users with this
> > case.
> > 
> > Ashutosh, Karthik, thoughts on this?
> > 
> > Have you guys seen around hwmon a way to avoid creating an entry based on the
> > platform? Like, in this case, avoid fan interfaces to appear at all on a platform
> > that doesn't support fan control?
> 
> Isn't that the whole point of ->is_visible(), and isn't that what we're already
> doing here?

Doh! right it is.

so, please move this comment to the
xe_hwmon_fan_is_visible() function.

'here as well... I believe we should not expose the fan files if !has_fan_control'

I mean, this check for the !has_fan_control should be the very first check
in that function. Then we don't need to add anywhere else apparently.

> 
> Raag