Re: [PATCH v1 03/11] thermal/drivers/intel_menlow: Remove add_one_attribute

From: Rafael J. Wysocki
Date: Fri Mar 17 2023 - 14:18:35 EST

Next message: Stephen Boyd: "Re: [PATCH RFT v2 01/14] dt-bindings: clock: qcom,rpmcc: Add a way to enable unused clock cleanup"
Previous message: Isaku Yamahata: "[PATCH 2/4] KVM: x86: Add 'fault_is_private' x86 op"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Mar 13, 2023 at 1:35 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 13/03/2023 13:26, Rafael J. Wysocki wrote:
> > On Mon, Mar 13, 2023 at 11:55 AM Daniel Lezcano
> > <daniel.lezcano@xxxxxxxxxx> wrote:
> >>
> >>
> >> Hi,
> >>
> >> is this code removal acceptable ?
> >
> > I'll let you know later this week.
>
> Great, thank you

So it would be acceptable if it had no users, but that's somewhat hard
to establish.

As I wrote in a reply to the RFC version of this, I'd rather make
these attributes depend on a Kconfig option or a module parameter
before removing them completely.

> >> On 07/03/2023 14:37, Daniel Lezcano wrote:
> >>> The driver hooks the thermal framework sysfs to add some driver
> >>> specific information. A debatable approach as that may belong the
> >>> device sysfs directory, not the thermal zone directory.
> >>>
> >>> As the driver is accessing the thermal internals, we should provide at
> >>> least an API to the thermal framework to add an attribute to the
> >>> existing sysfs thermal zone entry.
> >>>
> >>> Before doing that and given the age of the driver (2008) may be it is
> >>> worth to double check if these attributes are really needed. So my
> >>> first proposal is to remove them if that does not hurt.
> >>>
> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> >>
> >>
> >>
> >>> ---
> >>> drivers/thermal/intel/intel_menlow.c | 193 ---------------------------
> >>> 1 file changed, 193 deletions(-)
> >>>
> >>> diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
> >>> index 5a6ad0552311..5a9738a93083 100644
> >>> --- a/drivers/thermal/intel/intel_menlow.c
> >>> +++ b/drivers/thermal/intel/intel_menlow.c
> >>> @@ -230,174 +230,8 @@ struct intel_menlow_attribute {
> >>> static LIST_HEAD(intel_menlow_attr_list);
> >>> static DEFINE_MUTEX(intel_menlow_attr_lock);
> >>>
> >>> -/*
> >>> - * sensor_get_auxtrip - get the current auxtrip value from sensor
> >>> - * @handle: Object handle
> >>> - * @index : GET_AUX1/GET_AUX0
> >>> - * @value : The address will be fill by the value
> >>> - */
> >>> -static int sensor_get_auxtrip(acpi_handle handle, int index,
> >>> - unsigned long long *value)
> >>> -{
> >>> - acpi_status status;
> >>> -
> >>> - if ((index != 0 && index != 1) || !value)
> >>> - return -EINVAL;
> >>> -
> >>> - status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
> >>> - NULL, value);
> >>> - if (ACPI_FAILURE(status))
> >>> - return -EIO;
> >>> -
> >>> - return 0;
> >>> -}
> >>> -
> >>> -/*
> >>> - * sensor_set_auxtrip - set the new auxtrip value to sensor
> >>> - * @handle: Object handle
> >>> - * @index : GET_AUX1/GET_AUX0
> >>> - * @value : The value will be set
> >>> - */
> >>> -static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
> >>> -{
> >>> - acpi_status status;
> >>> - union acpi_object arg = {
> >>> - ACPI_TYPE_INTEGER
> >>> - };
> >>> - struct acpi_object_list args = {
> >>> - 1, &arg
> >>> - };
> >>> - unsigned long long temp;
> >>> -
> >>> - if (index != 0 && index != 1)
> >>> - return -EINVAL;
> >>> -
> >>> - status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
> >>> - NULL, &temp);
> >>> - if (ACPI_FAILURE(status))
> >>> - return -EIO;
> >>> - if ((index && value < temp) || (!index && value > temp))
> >>> - return -EINVAL;
> >>> -
> >>> - arg.integer.value = value;
> >>> - status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
> >>> - &args, &temp);
> >>> - if (ACPI_FAILURE(status))
> >>> - return -EIO;
> >>> -
> >>> - /* do we need to check the return value of SAX0/SAX1 ? */
> >>> -
> >>> - return 0;
> >>> -}
> >>> -
> >>> -#define to_intel_menlow_attr(_attr) \
> >>> - container_of(_attr, struct intel_menlow_attribute, attr)
> >>> -
> >>> -static ssize_t aux_show(struct device *dev, struct device_attribute *dev_attr,
> >>> - char *buf, int idx)
> >>> -{
> >>> - struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> >>> - unsigned long long value;
> >>> - int result;
> >>> -
> >>> - result = sensor_get_auxtrip(attr->handle, idx, &value);
> >>> - if (result)
> >>> - return result;
> >>> -
> >>> - return sprintf(buf, "%lu", deci_kelvin_to_celsius(value));
> >>> -}
> >>> -
> >>> -static ssize_t aux0_show(struct device *dev,
> >>> - struct device_attribute *dev_attr, char *buf)
> >>> -{
> >>> - return aux_show(dev, dev_attr, buf, 0);
> >>> -}
> >>> -
> >>> -static ssize_t aux1_show(struct device *dev,
> >>> - struct device_attribute *dev_attr, char *buf)
> >>> -{
> >>> - return aux_show(dev, dev_attr, buf, 1);
> >>> -}
> >>> -
> >>> -static ssize_t aux_store(struct device *dev, struct device_attribute *dev_attr,
> >>> - const char *buf, size_t count, int idx)
> >>> -{
> >>> - struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> >>> - int value;
> >>> - int result;
> >>> -
> >>> - /*Sanity check; should be a positive integer */
> >>> - if (!sscanf(buf, "%d", &value))
> >>> - return -EINVAL;
> >>> -
> >>> - if (value < 0)
> >>> - return -EINVAL;
> >>> -
> >>> - result = sensor_set_auxtrip(attr->handle, idx,
> >>> - celsius_to_deci_kelvin(value));
> >>> - return result ? result : count;
> >>> -}
> >>> -
> >>> -static ssize_t aux0_store(struct device *dev,
> >>> - struct device_attribute *dev_attr,
> >>> - const char *buf, size_t count)
> >>> -{
> >>> - return aux_store(dev, dev_attr, buf, count, 0);
> >>> -}
> >>> -
> >>> -static ssize_t aux1_store(struct device *dev,
> >>> - struct device_attribute *dev_attr,
> >>> - const char *buf, size_t count)
> >>> -{
> >>> - return aux_store(dev, dev_attr, buf, count, 1);
> >>> -}
> >>> -
> >>> /* BIOS can enable/disable the thermal user application in dabney platform */
> >>> #define BIOS_ENABLED "\\_TZ.GSTS"
> >>> -static ssize_t bios_enabled_show(struct device *dev,
> >>> - struct device_attribute *attr, char *buf)
> >>> -{
> >>> - acpi_status status;
> >>> - unsigned long long bios_enabled;
> >>> -
> >>> - status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
> >>> - if (ACPI_FAILURE(status))
> >>> - return -ENODEV;
> >>> -
> >>> - return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
> >>> -}
> >>> -
> >>> -static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
> >>> - void *store, struct device *dev,
> >>> - acpi_handle handle)
> >>> -{
> >>> - struct intel_menlow_attribute *attr;
> >>> - int result;
> >>> -
> >>> - attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
> >>> - if (!attr)
> >>> - return -ENOMEM;
> >>> -
> >>> - sysfs_attr_init(&attr->attr.attr); /* That is consistent naming :D */
> >>> - attr->attr.attr.name = name;
> >>> - attr->attr.attr.mode = mode;
> >>> - attr->attr.show = show;
> >>> - attr->attr.store = store;
> >>> - attr->device = dev;
> >>> - attr->handle = handle;
> >>> -
> >>> - result = device_create_file(dev, &attr->attr);
> >>> - if (result) {
> >>> - kfree(attr);
> >>> - return result;
> >>> - }
> >>> -
> >>> - mutex_lock(&intel_menlow_attr_lock);
> >>> - list_add_tail(&attr->node, &intel_menlow_attr_list);
> >>> - mutex_unlock(&intel_menlow_attr_lock);
> >>> -
> >>> - return 0;
> >>> -}
> >>>
> >>> static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> >>> void *context, void **rv)
> >>> @@ -420,12 +254,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> >>> if (ACPI_FAILURE(status))
> >>> return (status == AE_NOT_FOUND) ? AE_OK : status;
> >>>
> >>> - result = intel_menlow_add_one_attribute("aux0", 0644,
> >>> - aux0_show, aux0_store,
> >>> - &thermal->device, handle);
> >>> - if (result)
> >>> - return AE_ERROR;
> >>> -
> >>> status = acpi_get_handle(handle, GET_AUX1, &dummy);
> >>> if (ACPI_FAILURE(status))
> >>> goto aux1_not_found;
> >>> @@ -434,27 +262,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> >>> if (ACPI_FAILURE(status))
> >>> goto aux1_not_found;
> >>>
> >>> - result = intel_menlow_add_one_attribute("aux1", 0644,
> >>> - aux1_show, aux1_store,
> >>> - &thermal->device, handle);
> >>> - if (result) {
> >>> - intel_menlow_unregister_sensor();
> >>> - return AE_ERROR;
> >>> - }
> >>> -
> >>> - /*
> >>> - * create the "dabney_enabled" attribute which means the user app
> >>> - * should be loaded or not
> >>> - */
> >>> -
> >>> - result = intel_menlow_add_one_attribute("bios_enabled", 0444,
> >>> - bios_enabled_show, NULL,
> >>> - &thermal->device, handle);
> >>> - if (result) {
> >>> - intel_menlow_unregister_sensor();
> >>> - return AE_ERROR;
> >>> - }
> >>> -
> >>> return AE_OK;
> >>>
> >>> aux1_not_found:
> >>
> >> --
> >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> >>
> >> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> >> <http://twitter.com/#!/linaroorg> Twitter |
> >> <http://www.linaro.org/linaro-blog/> Blog
> >>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

Next message: Stephen Boyd: "Re: [PATCH RFT v2 01/14] dt-bindings: clock: qcom,rpmcc: Add a way to enable unused clock cleanup"
Previous message: Isaku Yamahata: "[PATCH 2/4] KVM: x86: Add 'fault_is_private' x86 op"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]