Re: [PATCH 2/2] ACPI/thermal: support for thermal zone description

[Zhang Rui <rui.zhang@intel.com>]

On Fri, 2017-08-18 at 16:31 -0600, Prakash, Prashanth wrote:
> Hi Rafael/Rui,
> 
> On 8/17/2017 8:14 PM, Zhang Rui wrote:
> > 
> > On Fri, 2017-08-18 at 02:09 +0200, Rafael J. Wysocki wrote:
> > > 
> > > On Wed, Aug 9, 2017 at 4:27 PM, Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > 
> > > > Hi, Prakash,
> > > > 
> > > > On Tue, 2017-08-08 at 10:01 -0600, Prakash, Prashanth wrote:
> > > > > 
> > > > > Hi Rui,
> > > > > 
> > > > > On 8/8/2017 2:23 AM, Zhang Rui wrote:
> > > > > > 
> > > > > > 
> > > > > > On Fri, 2017-07-14 at 11:48 -0600, Prashanth Prakash wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > Per ACPI 6.2 spec, platforms can optionally add a
> > > > > > > string(_STR)
> > > > > > > object within each thermal zone package which provides a
> > > > > > > user
> > > > > > > friendly name/description.
> > > > > > > 
> > > > > > > Add support to parse the string object, which will be
> > > > > > > exposed
> > > > > > > to userspace by thermal framework.
> > > > > > > 
> > > > > > is there any real request for this?
> > > > > Yes, Qualcomm server platforms adds these description
> > > > > strings.
> > > > > > 
> > > > > > 
> > > > > > _STR is a generic control method for all the ACPI devices.
> > > > > > Thus I'm wondering, if really needed, should we expose this
> > > > > > in
> > > > > > acpi
> > > > > > bus
> > > > > > instead?
> > > > > AFAIK, adding a _STR to any package was not explicitly
> > > > > allowed by
> > > > > the
> > > > > spec.
> > > > > Updates in APCI 6.2 made it legal to add an _STR object to
> > > > > thermal
> > > > > zone
> > > > > specifically, so added this support only to thermal zone.
> > > > > 
> > > > I see that _STR is stated explicitly in 11.4.14, ACPI spec 6.2,
> > > > but
> > > > according to section 6.1.10, "The _STR object evaluates to a
> > > > Unicode
> > > > string that describes the device or thermal zone. "
> > > > _STR is still a generic control method that can exist in any
> > > > other
> > > > device scope.
> > > > 
> > > > so to me, this is a optional but generic feature for all the
> > > > ACPI
> > > > devices, and we don't have a solid reason that it should be
> > > > part of
> > > > thermal sysfs I/F, thus a better solution to me is to expose
> > > > this
> > > > as an
> > > > attribute of ACPI device, and we can link to the ACPI device
> > > > from
> > > > thermal sysfs I/F in userspace.
> > > > 
> > > > what do you think, Rafael?
> > > Since you have a ->get_desc method, I don't have a big problem
> > > with
> > > the approach here.
> > > 
> > > I'm not particularly liking the "<not supported>" thing returned
> > > if
> > > _STR is not present, though.
> I will change the implementation such that if _STR object was not
> found then
> thermal_get_desc would return -ENXIO (or should it be different
> errno?).
> 
> > 
> > No, actually I mean adding a new sysfs attribute under ACPI device
> > node, just like path/hid/status/adr, etc.
> Sorry Rui, I didn't read your earlier comment correctly. Thermal
> zone's _STR is
> useful in couple of scenarios(even if ACPI device containing the
> thermal_zone
> had a _STR object):
> - When we have more than 1 thermal sensors/zones under a device then
> this will
> allow us to differentiate them

Yes I agree.
>From userspace point of view,
with you patch, userspace can get the content of _STR by
cat /sys/class/thermal/thermal_zoneX/desc

And what I mean is that, userspace can already get the same information
by
cat /sys/class/thermal/thermal_zoneX/device/description
even without the patch.


> - When we have some thermal sensors that doesn't have ACPI device
> associated
> with it. For example: a shared L3 cache, an abstract region on SoC.
> In these cases
> we can add a thermal zone object in an appropriate place in dsdt and
> the
> associated _STR will allow us to provide a user friendly
> name/description.

if the sensor is registered by native driver, I think .get_desc() is
useful.
But if you want to hack the dsdt to get it enumerated via ACPI, then my
approach still works without the patch. :)

thanks,
rui
> > 
> > Of course the attribute should be optional, depends on if the _STR
> > control methods exist or not.
> > 
> > thanks,
> > rui
> > > 
> > > Thanks,
> > > Rafael
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> Thanks for your feedback!
> 
> --
> Regards,
> Prashanth
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html