On 3/27/24 1:05 PM, Ilpo Järvinen wrote:
> On Tue, 26 Mar 2024, Carlos Ferreira wrote:
>
>> Hi, i have changed some of the code. How does it look now?
>>
>> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@xxxxxxxxx>
>> ---
>
> First of all, you need to make a proper submission with versioning, that
> is:
>
> - Put version into the subject: PATCH v2
> - Don't put extra stuff into changelog like the above question, if you
> need to ask something, put your question underneath the first --- line.
> - List the changes you made underneath the first --- line (see ML
> archives for examples about formatting)
Should i submit the v2 patch with the current state or with the changes suggested such as the use of the leds API?
>> drivers/platform/x86/hp/hp-wmi.c | 251 +++++++++++++++++++++++++++++--
>> 1 file changed, 241 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>> index e53660422..8108ca7e9 100644
>> --- a/drivers/platform/x86/hp/hp-wmi.c
>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>> @@ -27,6 +27,7 @@
>> #include <linux/rfkill.h>
>> #include <linux/string.h>
>> #include <linux/dmi.h>
>> +#include <linux/bitfield.h>
>
> Try to put it earlier, these should eventually be in alphabetic order
> (again, ordered by a separate patch, not this one).
You mean organizing all the imports like this?
#include <linux/acpi.h>
#include <linux/bitfield.h>
#include <linux/dmi.h>
#include <linux/hwmon.h>
#include <linux/init.h>
#include <linux/input.h>
#include <linux/input/sparse-keymap.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/platform_profile.h>
#include <linux/rfkill.h>
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/types.h>
>> MODULE_AUTHOR("Matthew Garrett <mjg59@xxxxxxxxxxxxx>");
>> MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
>> @@ -40,6 +41,10 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
>> #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
>> #define zero_if_sup(tmp) (zero_insize_support?0:sizeof(tmp)) // use when zero insize is required
>>
>> +#define FOURZONE_LIGHTING_SUPPORTED_BIT 0x01
>> +#define FOURZONE_LIGHTING_ON 228
>> +#define FOURZONE_LIGHTING_OFF 100
>> +
>> /* DMI board names of devices that should use the omen specific path for
>> * thermal profiles.
>> * This was obtained by taking a look in the windows omen command center
>> @@ -131,18 +136,36 @@ enum hp_wmi_commandtype {
>> };
>>
>> enum hp_wmi_gm_commandtype {
>> - HPWMI_FAN_SPEED_GET_QUERY = 0x11,
>> - HPWMI_SET_PERFORMANCE_MODE = 0x1A,
>> - HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
>> - HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
>> - HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
>> + HPWMI_FAN_SPEED_GET_QUERY = 0x11,
>> + HPWMI_SET_PERFORMANCE_MODE = 0x1A,
>> + HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
>> + HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
>> + HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
>> + HPWMI_GET_KEYBOARD_TYPE = 0x2B,
>> +};
>> +
>> +enum hp_wmi_fourzone_commandtype {
>> + HPWMI_SUPPORTS_LIGHTNING = 0x01,
>> + HPWMI_FOURZONE_COLOR_GET = 0x02,
>> + HPWMI_FOURZONE_COLOR_SET = 0x03,
>> + HPWMI_FOURZONE_MODE_GET = 0x04,
>> + HPWMI_FOURZONE_MODE_SET = 0x05,
>> +};
>> +
>> +enum hp_wmi_keyboardtype {
>> + HPWMI_KEYBOARD_INVALID = 0x00,
>> + HPWMI_KEYBOARD_NORMAL = 0x01,
>> + HPWMI_KEYBOARD_WITH_NUMPAD = 0x02,
>> + HPWMI_KEYBOARD_WITHOUT_NUMPAD = 0x03,
>> + HPWMI_KEYBOARD_RGB = 0x04,
>> };
>>
>> enum hp_wmi_command {
>> - HPWMI_READ = 0x01,
>> - HPWMI_WRITE = 0x02,
>> - HPWMI_ODM = 0x03,
>> - HPWMI_GM = 0x20008,
>> + HPWMI_READ = 0x01,
>> + HPWMI_WRITE = 0x02,
>> + HPWMI_ODM = 0x03,
>> + HPWMI_GM = 0x20008,
>> + HPWMI_FOURZONE = 0x20009,
>> };
>>
>> enum hp_wmi_hardware_mask {
>> @@ -652,6 +675,74 @@ static int hp_wmi_rfkill2_refresh(void)
>> return 0;
>> }
>>
>> +static int fourzone_get_colors(u32 *colors)
>> +{
>> + int ret;
>> + u8 buff[128];
>> +
>> + ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
>> + &buff, sizeof(buff), sizeof(buff));
>> + if (ret != 0)
>> + return -EINVAL;
>> +
>> + for (int i = 0; i < 4; i++) {
>> + colors[3 - i] = FIELD_PREP(GENMASK(23, 16), buff[25 + i * 3]) // r
>> + | FIELD_PREP(GENMASK(15, 8), buff[25 + i * 3 + 1]) // g
>> + | FIELD_PREP(GENMASK(7, 0), buff[25 + i * 3 + 2]); // b
>
> Those GENMASK() calls should be done in #define and only used here:
>
> #define FOURZONE_COLOR_R GENMASK(23, 16)
> #define FOURZONE_COLOR_G GENMASK(15, 8)
> #define FOURZONE_COLOR_B GENMASK(7, 0)
>
> You can then drop the comments because the code is self-explanatory.
>
> Add #include <linux/bits.h>.
>
> ...But please see my comments about multicolor below.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int fourzone_set_colors(u32 *colors)
>> +{
>> + int ret;
>> + u8 buff[128];
>> +
>> + ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
>> + &buff, sizeof(buff), sizeof(buff));
>> + if (ret != 0)
>> + return -EINVAL;
>> +
>> + for (int i = 0; i < 4; i++) {
>> + buff[25 + i * 3] = FIELD_GET(GENMASK(23, 16), colors[3 - i]); // r
>> + buff[25 + i * 3 + 1] = FIELD_GET(GENMASK(15, 8), colors[3 - i]); // g
>> + buff[25 + i * 3 + 2] = FIELD_GET(GENMASK(7, 0), colors[3 - i]); // b
>> + }
>> + return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
>> + &buff, sizeof(buff), sizeof(buff));
>> +}
>> +
>> +/*
>> + * Returns a negative number on error or 0/1 for the mode.
>> + */
>> +static int fourzone_get_mode(void)
>> +{
>> + int ret;
>> + u8 buff[4];
>
> Try to use reverse xmas tree order where possible (go through the other
> functions too).
>
>> +
>> + ret = hp_wmi_perform_query(HPWMI_FOURZONE_MODE_GET, HPWMI_FOURZONE,
>> + &buff, sizeof(buff), sizeof(buff));
>> + if (ret != 0)
>> + return ret;
>> +
>> + return buff[0] == FOURZONE_LIGHTING_ON ? 1 : 0;
>> +}
>> +
>> +/*
>> + * This device supports only two different modes:
>> + * 1 -> lights on
>> + * 0 -> lights off
>> + */
>> +static int fourzone_set_mode(u32 mode)
>> +{
>> + u8 buff[4] = {mode ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF, 0, 0, 0};
>> +
>> + return hp_wmi_perform_query(HPWMI_FOURZONE_MODE_SET, HPWMI_FOURZONE,
>> + &buff, sizeof(buff), 0);
>> +}
>> +
>> static ssize_t display_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> {
>> @@ -754,6 +845,58 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>> return count;
>> }
>>
>> +static ssize_t colors_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + int ret;
>> + u32 colors[4];
>> +
>> + ret = fourzone_get_colors(colors);
>> + if (ret != 0)
>> + return -EINVAL;
>> +
>> + return sysfs_emit(buf, "%06x %06x %06x %06x\n", colors[0], colors[1], colors[2], colors[3]);
>> +}
>> +
>> +static ssize_t colors_store(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int ret;
>> + u32 colors[4];
>> +
>> + ret = sscanf(buf, "%6x %6x %6x %6x", &colors[0], &colors[1], &colors[2], &colors[3]);
>> + if (ret != 4)
>> + return -EINVAL;
>
> I now realize this should use leds multicolor API instead.
>
>> + ret = fourzone_set_colors(colors);
>> + return ret == 0 ? count : ret;
>> +}
>> +
>> +static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + u32 ret = fourzone_get_mode();
>> +
>> + if (ret < 0)
>
> Please don't save one line like this, put the declaration on own
> line/declaration block so you can keep function call and it's error
> handling next to each other.
>
> Also, compiler should have warned you here because u32 is not the correct
> type and you're checking for < 0!
>
>> + return ret;
>> +
>> + return sysfs_emit(buf, "%d\n", ret);
>> +}
>> +
>> +static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int ret;
>> + u32 mode;
>> +
>> + ret = kstrtou32(buf, 10, &mode);
>> + if (ret)
>> + return ret;
>> +
>> + ret = fourzone_set_mode(mode);
>> + return ret == 0 ? count : ret;
>> +}
>> +
>> static int camera_shutter_input_setup(void)
>> {
>> int err;
>> @@ -781,6 +924,34 @@ static int camera_shutter_input_setup(void)
>> return err;
>> }
>>
>> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
>> +{
>> + int ret;
>> + u8 buff[128];
>> +
>> + ret = hp_wmi_perform_query(HPWMI_GET_KEYBOARD_TYPE, HPWMI_GM,
>> + &buff, sizeof(buff), sizeof(buff));
>> + if (ret != 0)
>> + return HPWMI_KEYBOARD_INVALID;
>> +
>> + /* the first byte in the response represents the keyboard type */
>> + return (enum hp_wmi_keyboardtype)(buff[0] + 1);
>> +}
>> +
>> +static ssize_t type_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + enum hp_wmi_keyboardtype type = fourzone_get_keyboard_type();
>> +
>> + if (type == HPWMI_KEYBOARD_INVALID)
>> + return -EINVAL;
>> +
>> + return sysfs_emit(buf, "%d\n", type - 1);
>
> These are always positive, right? So %u is better.
>
>> +}
>> +
>> +/*
>> + * Generic device attributes.
>> + */
>> static DEVICE_ATTR_RO(display);
>> static DEVICE_ATTR_RO(hddtemp);
>> static DEVICE_ATTR_RW(als);
>> @@ -797,7 +968,35 @@ static struct attribute *hp_wmi_attrs[] = {
>> &dev_attr_postcode.attr,
>> NULL,
>> };
>> -ATTRIBUTE_GROUPS(hp_wmi);
>> +
>> +static struct attribute_group hp_wmi_group = {
>> + .attrs = hp_wmi_attrs,
>> +};
>> +
>> +/*
>> + * Omen fourzone specific device attributes.
>> + */
>> +static DEVICE_ATTR_RW(colors);
>> +static DEVICE_ATTR_RW(mode);
>> +static DEVICE_ATTR_RO(type);
>> +
>> +static struct attribute *hp_wmi_fourzone_attrs[] = {
>> + &dev_attr_colors.attr,
>> + &dev_attr_mode.attr,
>> + &dev_attr_type.attr,
>> + NULL,
>> +};
>> +
>> +static struct attribute_group hp_wmi_fourzone_group = {
>> + .attrs = hp_wmi_fourzone_attrs,
>> + .name = "kbd-backlight",
>> +};
>> +
>> +static const struct attribute_group *hp_wmi_groups[] = {
>> + &hp_wmi_group,
>> + NULL,
>> + NULL,
>> +};
>>
>> static void hp_wmi_notify(u32 value, void *context)
>> {
>> @@ -1446,6 +1645,35 @@ static int thermal_profile_setup(void)
>> return 0;
>> }
>>
>> +static bool fourzone_supports_lighting(void)
>> +{
>> + int ret;
>> + u8 buff[128];
>> +
>> + ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
>> + &buff, sizeof(buff), sizeof(buff));
>> + if (ret != 0)
>
> < 0 ?
>
