Re: [PATCH] thermal: int340x_thermal: Add production mode attribute

[Rafael J. Wysocki]

On Tue, Jan 17, 2023 at 5:50 PM Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
>
> It is possible that system manufacturer locks further thermal tuning. In
> this case user space calibration tools should not try to adjust thermal
> configuration.
>
> Add an attribute "production_mode". This attribute is only present when
> the ACPI DCFG method is present under INT3400 device scope.

I would rewrite the above in the following way:

"It is possible that the system manufacturer locks down thermal tuning
beyond what is usually done on the given platform.  In that case user
space calibration tools should not try to adjust the thermal
configuration of the system.

To allow user space to check if that is the case, add a new sysfs
attribute "production_mode" that will be present when the ACPI DCFG
method is present under the INT3400 device object in the ACPI Namespace."

> When DCFG evaluates to non 0, user space configuration tools should exit.

It is not clear what exactly "should exit" means here.

> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> ---
>  .../driver-api/thermal/intel_dptf.rst         |  4 ++
>  .../intel/int340x_thermal/int3400_thermal.c   | 48 +++++++++++++++++++
>  2 files changed, 52 insertions(+)
>
> diff --git a/Documentation/driver-api/thermal/intel_dptf.rst b/Documentation/driver-api/thermal/intel_dptf.rst
> index 372bdb4d04c6..9a7b651ba764 100644
> --- a/Documentation/driver-api/thermal/intel_dptf.rst
> +++ b/Documentation/driver-api/thermal/intel_dptf.rst
> @@ -84,6 +84,10 @@ DPTF ACPI Drivers interface
>         https:/github.com/intel/thermal_daemon for decoding
>         thermal table.
>
> +``production_mode`` (RO)
> +       When non zero, manufacturer locked thermal configuration from

It would be slightly better to say "When different from zero, ..." IMO.

> +       further changes.
> +
>
>  ACPI Thermal Relationship table interface
>  ------------------------------------------
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index db8a6f63657d..ec5ec07ccf41 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -60,6 +60,8 @@ struct int3400_thermal_priv {
>         int odvp_count;
>         int *odvp;
>         u32 os_uuid_mask;
> +       int production_mode;
> +       bool production_mode_support;

A negative value could be used as a "no support" indicator, so the
bool field wouldn't be necessary.

>         struct odvp_attr *odvp_attrs;
>  };
>
> @@ -315,6 +317,43 @@ static int int3400_thermal_get_uuids(struct int3400_thermal_priv *priv)
>         return result;
>  }
>
> +static ssize_t production_mode_show(struct device *dev, struct device_attribute *attr,
> +                                    char *buf)
> +{
> +       struct int3400_thermal_priv *priv = dev_get_drvdata(dev);
> +
> +       return sprintf(buf, "%d\n", priv->production_mode);

sysfs_emit() ?

> +}
> +
> +static DEVICE_ATTR_RO(production_mode);
> +
> +static int production_mode_init(struct int3400_thermal_priv *priv)
> +{
> +       unsigned long long mode;
> +       acpi_status status;
> +       int ret;
> +
> +       status = acpi_evaluate_integer(priv->adev->handle, "DCFG", NULL, &mode);
> +       /* If the method is not present, this is not an error */
> +       if (ACPI_FAILURE(status))
> +               return 0;
> +
> +       ret = sysfs_create_file(&priv->pdev->dev.kobj, &dev_attr_production_mode.attr);
> +       if (ret)
> +               return ret;
> +
> +       priv->production_mode_support = true;
> +       priv->production_mode = mode;
> +
> +       return 0;
> +}
> +
> +void production_mode_exit(struct int3400_thermal_priv *priv)
> +{
> +       if (priv->production_mode_support)
> +               sysfs_remove_file(&priv->pdev->dev.kobj, &dev_attr_production_mode.attr);
> +}
> +
>  static ssize_t odvp_show(struct device *dev, struct device_attribute *attr,
>                          char *buf)
>  {
> @@ -610,8 +649,15 @@ static int int3400_thermal_probe(struct platform_device *pdev)
>         if (result)
>                 goto free_sysfs;
>
> +       result = production_mode_init(priv);
> +       if (result)
> +               goto free_notify;
> +
>         return 0;
>
> +free_notify:
> +       acpi_remove_notify_handler(priv->adev->handle, ACPI_DEVICE_NOTIFY,
> +                                  int3400_notify);
>  free_sysfs:
>         cleanup_odvp(priv);
>         if (!ZERO_OR_NULL_PTR(priv->data_vault)) {
> @@ -638,6 +684,8 @@ static int int3400_thermal_remove(struct platform_device *pdev)
>  {
>         struct int3400_thermal_priv *priv = platform_get_drvdata(pdev);
>
> +       production_mode_exit(priv);
> +
>         acpi_remove_notify_handler(
>                         priv->adev->handle, ACPI_DEVICE_NOTIFY,
>                         int3400_notify);
> --