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

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

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.

No, actually I mean adding a new sysfs attribute under ACPI device
node, just like path/hid/status/adr, etc.

Of course the attribute should be optional, depends on if the _STR
control methods exist or not.

thanks,
rui
> 
> Thanks,
> Rafael