On Mon, May 27, 2024, at 3:39 AM, Ilpo Järvinen wrote:
> On Fri, 17 May 2024, Lyndon Sanche wrote:
>
>> +#include <linux/platform_profile.h>
>> +#include <linux/slab.h>
>> +#include "dell-smbios.h"
>
> Add empty line between <> and "" includes.
Agreed.
>> +enum thermal_mode_bits {
>> + DELL_BALANCED = BIT(0),
>> + DELL_COOL_BOTTOM = BIT(1),
>> + DELL_QUIET = BIT(2),
>> + DELL_PERFORMANCE = BIT(3),
>
> A few nits still to address.
>
> Can you please align these so that the values align (IIRC, I asked this
> earlier but perhaps my request was too unclear):
>
> DELL_XX = BIT(X),
> DELL_YYYYYYYYY = BIT(Y),
>
I must have missed this, I will get this corrected.
>> +static int thermal_get_supported_modes(int *supported_bits)
>> +{
>> + struct calling_interface_buffer buffer;
>> + int ret;
>> +
>> + dell_fill_request(&buffer, 0x0, 0, 0, 0);
>> + ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
>> + if (ret) {
>> + /* Thermal function not supported */
>> + if (ret == -ENXIO) {
>> + *supported_bits = 0;
>> + return 0;
>> + } else {
>
> Drop else because the previous block ends into return.
>
Agreed.
>> + return ret;
>> + }
>> + }
>
> Remove the outer if (ret) block and put the inner ones directly on the
> main level as two if () conditions.
>
Agreed.
>> + dell_fill_request(&buffer, 0x1, FIELD_PREP(DELL_ACC_SET_FIELD, acc_mode) | state, 0, 0);
>> + ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
>> + return ret;
>
> Return directly on the previous line.
>
Agreed.
>> + int ret;
>> + int supported_modes;
>> +
>> + /* If thermal commands not supported, exit without error */
>
> Fix grammar, you're perhaps missing "are".
>
Agreed.
>> + if (!dell_smbios_class_is_supported(CLASS_INFO))
>> + return 0;
>> +
>> + /* If thermal modes not supported, exit without error */
>
> Ditto.
>
> --
> i.
Thank you for your feedback. I will include this in the next patch series.
Thanks,
Lyndon
