[PATCH 0/5] platform/x86: msi-ec: Add the first platform device attributes

[PATCH 0/5] platform/x86: msi-ec: Add the first platform device attributes

[Nikita Kravets]

Hi!

This patch series implements a platform driver and the first three
platform device attributes: firmware version (RO), firmware release
date (RO) and cooler boost (RW).

Cc: Aakash Singh <mail@singhaakash.dev>
Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>

Nikita Kravets (5):
  platform/x86: msi-ec: Register a platform driver
  platform/x86: msi-ec: Add fw version and release date attributes
  platform/x86: msi-ec: Filter out unsupported attributes
  platform/x86: msi-ec: Add EC bit operation functions
  platform/x86: msi-ec: Add a cooler boost attribute

 drivers/platform/x86/msi-ec.c | 194 ++++++++++++++++++++++++++++++++++
 drivers/platform/x86/msi-ec.h |   5 +
 2 files changed, 199 insertions(+)

-- 
2.42.0

[PATCH 1/5] platform/x86: msi-ec: Register a platform driver

[Nikita Kravets]

Register a platform driver for the future features.

Cc: Aakash Singh <mail@singhaakash.dev>
Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
Signed-off-by: Nikita Kravets <teackot@gmail.com>
---
 drivers/platform/x86/msi-ec.c | 44 +++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
index f26a3121092f..12c559c9eac4 100644
--- a/drivers/platform/x86/msi-ec.c
+++ b/drivers/platform/x86/msi-ec.c
@@ -818,6 +818,30 @@ static struct acpi_battery_hook battery_hook = {
 	.name = MSI_EC_DRIVER_NAME,
 };
 
+/*
+ * Sysfs platform driver
+ */
+
+static int msi_platform_probe(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static int msi_platform_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_device *msi_platform_device;
+
+static struct platform_driver msi_platform_driver = {
+	.driver = {
+		.name = MSI_EC_DRIVER_NAME,
+	},
+	.probe = msi_platform_probe,
+	.remove = msi_platform_remove,
+};
+
 /*
  * Module load/unload
  */
@@ -878,6 +902,23 @@ static int __init msi_ec_init(void)
 	if (result < 0)
 		return result;
 
+	result = platform_driver_register(&msi_platform_driver);
+	if (result < 0)
+		return result;
+
+	msi_platform_device = platform_device_alloc(MSI_EC_DRIVER_NAME, -1);
+	if (msi_platform_device == NULL) {
+		platform_driver_unregister(&msi_platform_driver);
+		return -ENOMEM;
+	}
+
+	result = platform_device_add(msi_platform_device);
+	if (result < 0) {
+		platform_device_del(msi_platform_device);
+		platform_driver_unregister(&msi_platform_driver);
+		return result;
+	}
+
 	battery_hook_register(&battery_hook);
 	return 0;
 }
@@ -885,6 +926,9 @@ static int __init msi_ec_init(void)
 static void __exit msi_ec_exit(void)
 {
 	battery_hook_unregister(&battery_hook);
+
+	platform_driver_unregister(&msi_platform_driver);
+	platform_device_del(msi_platform_device);
 }
 
 MODULE_LICENSE("GPL");
-- 
2.42.0

Re: [PATCH 1/5] platform/x86: msi-ec: Register a platform driver

[Ilpo Järvinen]

On Tue, 10 Oct 2023, Nikita Kravets wrote:

> Register a platform driver for the future features.
> 
> Cc: Aakash Singh <mail@singhaakash.dev>
> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
> Signed-off-by: Nikita Kravets <teackot@gmail.com>
> ---
>  drivers/platform/x86/msi-ec.c | 44 +++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
> index f26a3121092f..12c559c9eac4 100644
> --- a/drivers/platform/x86/msi-ec.c
> +++ b/drivers/platform/x86/msi-ec.c
> @@ -818,6 +818,30 @@ static struct acpi_battery_hook battery_hook = {
>  	.name = MSI_EC_DRIVER_NAME,
>  };
>  
> +/*
> + * Sysfs platform driver
> + */
> +
> +static int msi_platform_probe(struct platform_device *pdev)
> +{
> +	return 0;
> +}
> +
> +static int msi_platform_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}

No need to provide empty .probe() or .remove().

> +static struct platform_device *msi_platform_device;
> +
> +static struct platform_driver msi_platform_driver = {
> +	.driver = {
> +		.name = MSI_EC_DRIVER_NAME,
> +	},
> +	.probe = msi_platform_probe,
> +	.remove = msi_platform_remove,
> +};
> +
>  /*
>   * Module load/unload
>   */
> @@ -878,6 +902,23 @@ static int __init msi_ec_init(void)
>  	if (result < 0)
>  		return result;
>  
> +	result = platform_driver_register(&msi_platform_driver);
> +	if (result < 0)
> +		return result;
> +
> +	msi_platform_device = platform_device_alloc(MSI_EC_DRIVER_NAME, -1);
> +	if (msi_platform_device == NULL) {
> +		platform_driver_unregister(&msi_platform_driver);
> +		return -ENOMEM;
> +	}
> +
> +	result = platform_device_add(msi_platform_device);
> +	if (result < 0) {
> +		platform_device_del(msi_platform_device);
> +		platform_driver_unregister(&msi_platform_driver);
> +		return result;

Instead of duplicating error handling, make a proper rollback with goto 
and labels, or better yet, use the cleanup.h if you know how it works.

> +	}
> +
>  	battery_hook_register(&battery_hook);
>  	return 0;
>  }
> @@ -885,6 +926,9 @@ static int __init msi_ec_init(void)
>  static void __exit msi_ec_exit(void)
>  {
>  	battery_hook_unregister(&battery_hook);
> +
> +	platform_driver_unregister(&msi_platform_driver);
> +	platform_device_del(msi_platform_device);
>  }
>  
>  MODULE_LICENSE("GPL");
> 

-- 
 i.

Re: [PATCH 1/5] platform/x86: msi-ec: Register a platform driver

[Hans de Goede]

Hi,

On 10/11/23 15:00, Ilpo Järvinen wrote:
> On Tue, 10 Oct 2023, Nikita Kravets wrote:
> 
>> Register a platform driver for the future features.
>>
>> Cc: Aakash Singh <mail@singhaakash.dev>
>> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
>> Signed-off-by: Nikita Kravets <teackot@gmail.com>
>> ---
>>  drivers/platform/x86/msi-ec.c | 44 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
>> index f26a3121092f..12c559c9eac4 100644
>> --- a/drivers/platform/x86/msi-ec.c
>> +++ b/drivers/platform/x86/msi-ec.c
>> @@ -818,6 +818,30 @@ static struct acpi_battery_hook battery_hook = {
>>  	.name = MSI_EC_DRIVER_NAME,
>>  };
>>  
>> +/*
>> + * Sysfs platform driver
>> + */
>> +
>> +static int msi_platform_probe(struct platform_device *pdev)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int msi_platform_remove(struct platform_device *pdev)
>> +{
>> +	return 0;
>> +}
> 
> No need to provide empty .probe() or .remove().
> 
>> +static struct platform_device *msi_platform_device;
>> +
>> +static struct platform_driver msi_platform_driver = {
>> +	.driver = {
>> +		.name = MSI_EC_DRIVER_NAME,
>> +	},
>> +	.probe = msi_platform_probe,
>> +	.remove = msi_platform_remove,
>> +};
>> +
>>  /*
>>   * Module load/unload
>>   */
>> @@ -878,6 +902,23 @@ static int __init msi_ec_init(void)
>>  	if (result < 0)
>>  		return result;
>>  
>> +	result = platform_driver_register(&msi_platform_driver);
>> +	if (result < 0)
>> +		return result;
>> +
>> +	msi_platform_device = platform_device_alloc(MSI_EC_DRIVER_NAME, -1);
>> +	if (msi_platform_device == NULL) {
>> +		platform_driver_unregister(&msi_platform_driver);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	result = platform_device_add(msi_platform_device);
>> +	if (result < 0) {
>> +		platform_device_del(msi_platform_device);
>> +		platform_driver_unregister(&msi_platform_driver);
>> +		return result;
> 
> Instead of duplicating error handling, make a proper rollback with goto 
> and labels, or better yet, use the cleanup.h if you know how it works.

Actually it would be better for a driver like this to use
platform_create_bundle(), see e.g. the last couple of lines
from:

drivers/platform/x86/x86-android-tablets/core.c

This will do both the platform_device registration as well
as the driver registration in one go avoiding the need
for rollback on error and it will also allow probe()
and all functions only used by probe() to be marked
as __init so that they can be free-ed from memory
once msi_ec_init() has completed running.

Regards,

Hans




> 
>> +	}
>> +
>>  	battery_hook_register(&battery_hook);
>>  	return 0;
>>  }
>> @@ -885,6 +926,9 @@ static int __init msi_ec_init(void)
>>  static void __exit msi_ec_exit(void)
>>  {
>>  	battery_hook_unregister(&battery_hook);
>> +
>> +	platform_driver_unregister(&msi_platform_driver);
>> +	platform_device_del(msi_platform_device);
>>  }
>>  
>>  MODULE_LICENSE("GPL");
>>
> 

[PATCH 2/5] platform/x86: msi-ec: Add fw version and release date attributes

[Nikita Kravets]

Create a root attribute group and add the first platform device
attributes: firmware version and firmware release date. Firmware
version attribute uses an already present ec_get_firmware_version()
function. Both features are present on all supported laptops.

Cc: Aakash Singh <mail@singhaakash.dev>
Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
Signed-off-by: Nikita Kravets <teackot@gmail.com>
---
 drivers/platform/x86/msi-ec.c | 67 ++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
index 12c559c9eac4..772b230fb47e 100644
--- a/drivers/platform/x86/msi-ec.c
+++ b/drivers/platform/x86/msi-ec.c
@@ -818,17 +818,82 @@ static struct acpi_battery_hook battery_hook = {
 	.name = MSI_EC_DRIVER_NAME,
 };
 
+/*
+ * Sysfs platform device attributes
+ */
+
+static ssize_t fw_version_show(struct device *device,
+			       struct device_attribute *attr, char *buf)
+{
+	u8 rdata[MSI_EC_FW_VERSION_LENGTH + 1];
+	int result;
+
+	result = ec_get_firmware_version(rdata);
+	if (result < 0)
+		return result;
+
+	return sysfs_emit(buf, "%s\n", rdata);
+}
+
+static ssize_t fw_release_date_show(struct device *device,
+				    struct device_attribute *attr, char *buf)
+{
+	u8 rdate[MSI_EC_FW_DATE_LENGTH + 1];
+	u8 rtime[MSI_EC_FW_TIME_LENGTH + 1];
+	int result;
+	int year, month, day, hour, minute, second;
+
+	memset(rdate, 0, MSI_EC_FW_DATE_LENGTH + 1);
+	result = ec_read_seq(MSI_EC_FW_DATE_ADDRESS,
+			     rdate,
+			     MSI_EC_FW_DATE_LENGTH);
+	if (result < 0)
+		return result;
+
+	result = sscanf(rdate, "%02d%02d%04d", &month, &day, &year);
+	if (result != 3)
+		return -EINVAL;
+
+	memset(rtime, 0, MSI_EC_FW_TIME_LENGTH + 1);
+	result = ec_read_seq(MSI_EC_FW_TIME_ADDRESS,
+			     rtime,
+			     MSI_EC_FW_TIME_LENGTH);
+	if (result < 0)
+		return result;
+
+	result = sscanf(rtime, "%02d:%02d:%02d", &hour, &minute, &second);
+	if (result != 3)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%04d/%02d/%02d %02d:%02d:%02d\n", year, month, day,
+			  hour, minute, second);
+}
+
+static DEVICE_ATTR_RO(fw_version);
+static DEVICE_ATTR_RO(fw_release_date);
+
+static struct attribute *msi_root_attrs[] = {
+	&dev_attr_fw_version.attr,
+	&dev_attr_fw_release_date.attr,
+	NULL
+};
+
+static struct attribute_group msi_root_group = {
+	.attrs = msi_root_attrs,
+};
+
 /*
  * Sysfs platform driver
  */
 
 static int msi_platform_probe(struct platform_device *pdev)
 {
-	return 0;
+	return sysfs_create_group(&pdev->dev.kobj, &msi_root_group);
 }
 
 static int msi_platform_remove(struct platform_device *pdev)
 {
+	sysfs_remove_group(&pdev->dev.kobj, &msi_root_group);
 	return 0;
 }
 
-- 
2.42.0

Re: [PATCH 2/5] platform/x86: msi-ec: Add fw version and release date attributes

[Ilpo Järvinen]

On Tue, 10 Oct 2023, Nikita Kravets wrote:

> Create a root attribute group and add the first platform device
> attributes: firmware version and firmware release date. Firmware
> version attribute uses an already present ec_get_firmware_version()
> function. Both features are present on all supported laptops.
> 
> Cc: Aakash Singh <mail@singhaakash.dev>
> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
> Signed-off-by: Nikita Kravets <teackot@gmail.com>
> ---
>  drivers/platform/x86/msi-ec.c | 67 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
> index 12c559c9eac4..772b230fb47e 100644
> --- a/drivers/platform/x86/msi-ec.c
> +++ b/drivers/platform/x86/msi-ec.c
> @@ -818,17 +818,82 @@ static struct acpi_battery_hook battery_hook = {
>  	.name = MSI_EC_DRIVER_NAME,
>  };
>  
> +/*
> + * Sysfs platform device attributes
> + */
> +
> +static ssize_t fw_version_show(struct device *device,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	u8 rdata[MSI_EC_FW_VERSION_LENGTH + 1];
> +	int result;
> +
> +	result = ec_get_firmware_version(rdata);
> +	if (result < 0)
> +		return result;
> +
> +	return sysfs_emit(buf, "%s\n", rdata);
> +}
> +
> +static ssize_t fw_release_date_show(struct device *device,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	u8 rdate[MSI_EC_FW_DATE_LENGTH + 1];
> +	u8 rtime[MSI_EC_FW_TIME_LENGTH + 1];
> +	int result;
> +	int year, month, day, hour, minute, second;
> +
> +	memset(rdate, 0, MSI_EC_FW_DATE_LENGTH + 1);

sizeof(*rdate) is safer so please use it.

> +	result = ec_read_seq(MSI_EC_FW_DATE_ADDRESS,
> +			     rdate,
> +			     MSI_EC_FW_DATE_LENGTH);
> +	if (result < 0)
> +		return result;
> +
> +	result = sscanf(rdate, "%02d%02d%04d", &month, &day, &year);

There fields would naturally be %u and unsigned but see the other comment 
below before doing this change.

> +	if (result != 3)
> +		return -EINVAL;

EINVAL should be returned if the input was invalid but here the data 
itself is not okay so some other errno would be better.

> +	memset(rtime, 0, MSI_EC_FW_TIME_LENGTH + 1);

sizeof() like above.

> +	result = ec_read_seq(MSI_EC_FW_TIME_ADDRESS,
> +			     rtime,
> +			     MSI_EC_FW_TIME_LENGTH);
> +	if (result < 0)
> +		return result;
> +
> +	result = sscanf(rtime, "%02d:%02d:%02d", &hour, &minute, &second);
> +	if (result != 3)
> +		return -EINVAL;

Ditto.

> +
> +	return sysfs_emit(buf, "%04d/%02d/%02d %02d:%02d:%02d\n", year, month, day,
> +			  hour, minute, second);

It would be kind of nice to use %pt formatting here instead of custom
datetime format, however, it would either require converting to time64_t 
or using struct rtc_time. The latter would naturally have the right fields 
but they're not unsigned so my comment above about %u is not going to work 
well with it.

I'm also a bit unsure whether it's appropriate to use that struct outside 
of rtc realm. vsprintf.c seems to convert time64_t into rtc_time before 
printing though.

Hans, do you have any idea about the struct rtc_time?

> +}
> +
> +static DEVICE_ATTR_RO(fw_version);
> +static DEVICE_ATTR_RO(fw_release_date);
> +
> +static struct attribute *msi_root_attrs[] = {
> +	&dev_attr_fw_version.attr,
> +	&dev_attr_fw_release_date.attr,
> +	NULL
> +};
> +
> +static struct attribute_group msi_root_group = {
> +	.attrs = msi_root_attrs,
> +};
> +
>  /*
>   * Sysfs platform driver
>   */
>  
>  static int msi_platform_probe(struct platform_device *pdev)
>  {
> -	return 0;
> +	return sysfs_create_group(&pdev->dev.kobj, &msi_root_group);
>  }
>  
>  static int msi_platform_remove(struct platform_device *pdev)
>  {
> +	sysfs_remove_group(&pdev->dev.kobj, &msi_root_group);
>  	return 0;
>  }

Don't handle add/remove but put the attribute group pointer into the 
platform_driver.driver instead.

-- 
 i.

Re: [PATCH 2/5] platform/x86: msi-ec: Add fw version and release date attributes

[Hans de Goede]

Hi Nikita,

Great to see that you are working on upstreaming more of the
out-of-tree msi-ec functionality. Thank you for working on this.

On 10/11/23 14:41, Ilpo Järvinen wrote:
> On Tue, 10 Oct 2023, Nikita Kravets wrote:
> 
>> Create a root attribute group and add the first platform device
>> attributes: firmware version and firmware release date. Firmware
>> version attribute uses an already present ec_get_firmware_version()
>> function. Both features are present on all supported laptops.
>>
>> Cc: Aakash Singh <mail@singhaakash.dev>
>> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
>> Signed-off-by: Nikita Kravets <teackot@gmail.com>
>> ---
>>  drivers/platform/x86/msi-ec.c | 67 ++++++++++++++++++++++++++++++++++-
>>  1 file changed, 66 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
>> index 12c559c9eac4..772b230fb47e 100644
>> --- a/drivers/platform/x86/msi-ec.c
>> +++ b/drivers/platform/x86/msi-ec.c
>> @@ -818,17 +818,82 @@ static struct acpi_battery_hook battery_hook = {
>>  	.name = MSI_EC_DRIVER_NAME,
>>  };
>>  
>> +/*
>> + * Sysfs platform device attributes
>> + */
>> +
>> +static ssize_t fw_version_show(struct device *device,
>> +			       struct device_attribute *attr, char *buf)
>> +{
>> +	u8 rdata[MSI_EC_FW_VERSION_LENGTH + 1];
>> +	int result;
>> +
>> +	result = ec_get_firmware_version(rdata);
>> +	if (result < 0)
>> +		return result;
>> +
>> +	return sysfs_emit(buf, "%s\n", rdata);
>> +}
>> +
>> +static ssize_t fw_release_date_show(struct device *device,
>> +				    struct device_attribute *attr, char *buf)
>> +{
>> +	u8 rdate[MSI_EC_FW_DATE_LENGTH + 1];
>> +	u8 rtime[MSI_EC_FW_TIME_LENGTH + 1];
>> +	int result;
>> +	int year, month, day, hour, minute, second;
>> +
>> +	memset(rdate, 0, MSI_EC_FW_DATE_LENGTH + 1);
> 
> sizeof(*rdate) is safer so please use it.
> 
>> +	result = ec_read_seq(MSI_EC_FW_DATE_ADDRESS,
>> +			     rdate,
>> +			     MSI_EC_FW_DATE_LENGTH);
>> +	if (result < 0)
>> +		return result;
>> +
>> +	result = sscanf(rdate, "%02d%02d%04d", &month, &day, &year);
> 
> There fields would naturally be %u and unsigned but see the other comment 
> below before doing this change.
> 
>> +	if (result != 3)
>> +		return -EINVAL;
> 
> EINVAL should be returned if the input was invalid but here the data 
> itself is not okay so some other errno would be better.
> 
>> +	memset(rtime, 0, MSI_EC_FW_TIME_LENGTH + 1);
> 
> sizeof() like above.
> 
>> +	result = ec_read_seq(MSI_EC_FW_TIME_ADDRESS,
>> +			     rtime,
>> +			     MSI_EC_FW_TIME_LENGTH);
>> +	if (result < 0)
>> +		return result;
>> +
>> +	result = sscanf(rtime, "%02d:%02d:%02d", &hour, &minute, &second);
>> +	if (result != 3)
>> +		return -EINVAL;
> 
> Ditto.
> 
>> +
>> +	return sysfs_emit(buf, "%04d/%02d/%02d %02d:%02d:%02d\n", year, month, day,
>> +			  hour, minute, second);
> 
> It would be kind of nice to use %pt formatting here instead of custom
> datetime format, however, it would either require converting to time64_t 
> or using struct rtc_time. The latter would naturally have the right fields 
> but they're not unsigned so my comment above about %u is not going to work 
> well with it.
> 
> I'm also a bit unsure whether it's appropriate to use that struct outside 
> of rtc realm. vsprintf.c seems to convert time64_t into rtc_time before 
> printing though.
> 
> Hans, do you have any idea about the struct rtc_time?
> 
>> +}
>> +
>> +static DEVICE_ATTR_RO(fw_version);
>> +static DEVICE_ATTR_RO(fw_release_date);
>> +
>> +static struct attribute *msi_root_attrs[] = {
>> +	&dev_attr_fw_version.attr,
>> +	&dev_attr_fw_release_date.attr,
>> +	NULL
>> +};
>> +
>> +static struct attribute_group msi_root_group = {
>> +	.attrs = msi_root_attrs,
>> +};
>> +
>>  /*
>>   * Sysfs platform driver
>>   */
>>  
>>  static int msi_platform_probe(struct platform_device *pdev)
>>  {
>> -	return 0;
>> +	return sysfs_create_group(&pdev->dev.kobj, &msi_root_group);
>>  }
>>  
>>  static int msi_platform_remove(struct platform_device *pdev)
>>  {
>> +	sysfs_remove_group(&pdev->dev.kobj, &msi_root_group);
>>  	return 0;
>>  }
> 
> Don't handle add/remove but put the attribute group pointer into the 
> platform_driver.driver instead.

Ack to that.

Also since this adds new driver specific sysfs atrributes please
also add a Documentation/ABI/testing/sysfs-platform-msi-ec
file in the same patch documenting the new sysfs attributes.

See e.g. : Documentation/ABI/testing/sysfs-platform-asus-wmi
for what such a documentation file should look like.

And then in further patches in this series for each patch
which adds a new sysfs attribute document the attribute
in that file in the same patch.

Besides it being a good practice to have documentation this
also helps with reviewing because then your description of
how the sysfs attribute should behave can be compared to
the actual code implementing it.

Regards,

Hans

Re: [PATCH 2/5] platform/x86: msi-ec: Add fw version and release date attributes

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

Hi Hans,

You missed the one question I had for you. I put it now conviniently at 
the end of the quote block below...

On Thu, 12 Oct 2023, Hans de Goede wrote:
> 
> Great to see that you are working on upstreaming more of the
> out-of-tree msi-ec functionality. Thank you for working on this.
> 
> On 10/11/23 14:41, Ilpo Järvinen wrote:
> > On Tue, 10 Oct 2023, Nikita Kravets wrote:
> > 
> >> Create a root attribute group and add the first platform device
> >> attributes: firmware version and firmware release date. Firmware
> >> version attribute uses an already present ec_get_firmware_version()
> >> function. Both features are present on all supported laptops.
> >>
> >> Cc: Aakash Singh <mail@singhaakash.dev>
> >> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
> >> Signed-off-by: Nikita Kravets <teackot@gmail.com>
> >> ---

> >> +static ssize_t fw_release_date_show(struct device *device,
> >> +				    struct device_attribute *attr, char *buf)
> >> +{
> >> +	u8 rdate[MSI_EC_FW_DATE_LENGTH + 1];
> >> +	u8 rtime[MSI_EC_FW_TIME_LENGTH + 1];
> >> +	int result;
> >> +	int year, month, day, hour, minute, second;
> >> +
> >> +	memset(rdate, 0, MSI_EC_FW_DATE_LENGTH + 1);
> > 
> > sizeof(*rdate) is safer so please use it.
> > 
> >> +	result = ec_read_seq(MSI_EC_FW_DATE_ADDRESS,
> >> +			     rdate,
> >> +			     MSI_EC_FW_DATE_LENGTH);
> >> +	if (result < 0)
> >> +		return result;
> >> +
> >> +	result = sscanf(rdate, "%02d%02d%04d", &month, &day, &year);
> > 
> > There fields would naturally be %u and unsigned but see the other comment 
> > below before doing this change.
> > 
> >> +	if (result != 3)
> >> +		return -EINVAL;
> > 
> > EINVAL should be returned if the input was invalid but here the data 
> > itself is not okay so some other errno would be better.
> > 
> >> +	memset(rtime, 0, MSI_EC_FW_TIME_LENGTH + 1);
> > 
> > sizeof() like above.
> > 
> >> +	result = ec_read_seq(MSI_EC_FW_TIME_ADDRESS,
> >> +			     rtime,
> >> +			     MSI_EC_FW_TIME_LENGTH);
> >> +	if (result < 0)
> >> +		return result;
> >> +
> >> +	result = sscanf(rtime, "%02d:%02d:%02d", &hour, &minute, &second);
> >> +	if (result != 3)
> >> +		return -EINVAL;
> > 
> > Ditto.
> > 
> >> +
> >> +	return sysfs_emit(buf, "%04d/%02d/%02d %02d:%02d:%02d\n", year, month, day,
> >> +			  hour, minute, second);
> > 
> > It would be kind of nice to use %pt formatting here instead of custom
> > datetime format, however, it would either require converting to time64_t 
> > or using struct rtc_time. The latter would naturally have the right fields 
> > but they're not unsigned so my comment above about %u is not going to work 
> > well with it.
> > 
> > I'm also a bit unsure whether it's appropriate to use that struct outside 
> > of rtc realm. vsprintf.c seems to convert time64_t into rtc_time before 
> > printing though.
> > 
> > Hans, do you have any idea about the struct rtc_time?



-- 
 i.

Re: [PATCH 2/5] platform/x86: msi-ec: Add fw version and release date attributes

[Hans de Goede]

Hi,

On 10/12/23 14:56, Ilpo Järvinen wrote:
> Hi Hans,
> 
> You missed the one question I had for you. I put it now conviniently at 
> the end of the quote block below...
> 
> On Thu, 12 Oct 2023, Hans de Goede wrote:
>>
>> Great to see that you are working on upstreaming more of the
>> out-of-tree msi-ec functionality. Thank you for working on this.
>>
>> On 10/11/23 14:41, Ilpo Järvinen wrote:
>>> On Tue, 10 Oct 2023, Nikita Kravets wrote:
>>>
>>>> Create a root attribute group and add the first platform device
>>>> attributes: firmware version and firmware release date. Firmware
>>>> version attribute uses an already present ec_get_firmware_version()
>>>> function. Both features are present on all supported laptops.
>>>>
>>>> Cc: Aakash Singh <mail@singhaakash.dev>
>>>> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
>>>> Signed-off-by: Nikita Kravets <teackot@gmail.com>
>>>> ---
> 
>>>> +static ssize_t fw_release_date_show(struct device *device,
>>>> +				    struct device_attribute *attr, char *buf)
>>>> +{
>>>> +	u8 rdate[MSI_EC_FW_DATE_LENGTH + 1];
>>>> +	u8 rtime[MSI_EC_FW_TIME_LENGTH + 1];
>>>> +	int result;
>>>> +	int year, month, day, hour, minute, second;
>>>> +
>>>> +	memset(rdate, 0, MSI_EC_FW_DATE_LENGTH + 1);
>>>
>>> sizeof(*rdate) is safer so please use it.
>>>
>>>> +	result = ec_read_seq(MSI_EC_FW_DATE_ADDRESS,
>>>> +			     rdate,
>>>> +			     MSI_EC_FW_DATE_LENGTH);
>>>> +	if (result < 0)
>>>> +		return result;
>>>> +
>>>> +	result = sscanf(rdate, "%02d%02d%04d", &month, &day, &year);
>>>
>>> There fields would naturally be %u and unsigned but see the other comment 
>>> below before doing this change.
>>>
>>>> +	if (result != 3)
>>>> +		return -EINVAL;
>>>
>>> EINVAL should be returned if the input was invalid but here the data 
>>> itself is not okay so some other errno would be better.
>>>
>>>> +	memset(rtime, 0, MSI_EC_FW_TIME_LENGTH + 1);
>>>
>>> sizeof() like above.
>>>
>>>> +	result = ec_read_seq(MSI_EC_FW_TIME_ADDRESS,
>>>> +			     rtime,
>>>> +			     MSI_EC_FW_TIME_LENGTH);
>>>> +	if (result < 0)
>>>> +		return result;
>>>> +
>>>> +	result = sscanf(rtime, "%02d:%02d:%02d", &hour, &minute, &second);
>>>> +	if (result != 3)
>>>> +		return -EINVAL;
>>>
>>> Ditto.
>>>
>>>> +
>>>> +	return sysfs_emit(buf, "%04d/%02d/%02d %02d:%02d:%02d\n", year, month, day,
>>>> +			  hour, minute, second);
>>>
>>> It would be kind of nice to use %pt formatting here instead of custom
>>> datetime format, however, it would either require converting to time64_t 
>>> or using struct rtc_time. The latter would naturally have the right fields 
>>> but they're not unsigned so my comment above about %u is not going to work 
>>> well with it.
>>>
>>> I'm also a bit unsure whether it's appropriate to use that struct outside 
>>> of rtc realm. vsprintf.c seems to convert time64_t into rtc_time before 
>>> printing though.
>>>
>>> Hans, do you have any idea about the struct rtc_time?

I don't really have any good ideas how to handle this. I agree that
using %pt might be a good idea, but then as you say the data would first
need to be converted to a struct rtc_time. All in all I think it is
probably best to stick with the DIY formatting of the time.
But I've no objections to doing the rtc_time conversion if people
think that is cleaner / better.

Regards,

Hans

Re: [PATCH 2/5] platform/x86: msi-ec: Add fw version and release date attributes

[N K]

Hi,

On Wed, Oct 18, 2023 at 5:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 10/12/23 14:56, Ilpo Järvinen wrote:
> > Hi Hans,
> >
> > You missed the one question I had for you. I put it now conviniently at
> > the end of the quote block below...
> >
> > On Thu, 12 Oct 2023, Hans de Goede wrote:
> >>
> >> Great to see that you are working on upstreaming more of the
> >> out-of-tree msi-ec functionality. Thank you for working on this.
> >>
> >> On 10/11/23 14:41, Ilpo Järvinen wrote:
> >>> On Tue, 10 Oct 2023, Nikita Kravets wrote:
> >>>
> >>>> Create a root attribute group and add the first platform device
> >>>> attributes: firmware version and firmware release date. Firmware
> >>>> version attribute uses an already present ec_get_firmware_version()
> >>>> function. Both features are present on all supported laptops.
> >>>>
> >>>> Cc: Aakash Singh <mail@singhaakash.dev>
> >>>> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
> >>>> Signed-off-by: Nikita Kravets <teackot@gmail.com>
> >>>> ---
> >
> >>>> +static ssize_t fw_release_date_show(struct device *device,
> >>>> +                              struct device_attribute *attr, char *buf)
> >>>> +{
> >>>> +  u8 rdate[MSI_EC_FW_DATE_LENGTH + 1];
> >>>> +  u8 rtime[MSI_EC_FW_TIME_LENGTH + 1];
> >>>> +  int result;
> >>>> +  int year, month, day, hour, minute, second;
> >>>> +
> >>>> +  memset(rdate, 0, MSI_EC_FW_DATE_LENGTH + 1);
> >>>
> >>> sizeof(*rdate) is safer so please use it.
> >>>
> >>>> +  result = ec_read_seq(MSI_EC_FW_DATE_ADDRESS,
> >>>> +                       rdate,
> >>>> +                       MSI_EC_FW_DATE_LENGTH);
> >>>> +  if (result < 0)
> >>>> +          return result;
> >>>> +
> >>>> +  result = sscanf(rdate, "%02d%02d%04d", &month, &day, &year);
> >>>
> >>> There fields would naturally be %u and unsigned but see the other comment
> >>> below before doing this change.
> >>>
> >>>> +  if (result != 3)
> >>>> +          return -EINVAL;
> >>>
> >>> EINVAL should be returned if the input was invalid but here the data
> >>> itself is not okay so some other errno would be better.
> >>>
> >>>> +  memset(rtime, 0, MSI_EC_FW_TIME_LENGTH + 1);
> >>>
> >>> sizeof() like above.
> >>>
> >>>> +  result = ec_read_seq(MSI_EC_FW_TIME_ADDRESS,
> >>>> +                       rtime,
> >>>> +                       MSI_EC_FW_TIME_LENGTH);
> >>>> +  if (result < 0)
> >>>> +          return result;
> >>>> +
> >>>> +  result = sscanf(rtime, "%02d:%02d:%02d", &hour, &minute, &second);
> >>>> +  if (result != 3)
> >>>> +          return -EINVAL;
> >>>
> >>> Ditto.
> >>>
> >>>> +
> >>>> +  return sysfs_emit(buf, "%04d/%02d/%02d %02d:%02d:%02d\n", year, month, day,
> >>>> +                    hour, minute, second);
> >>>
> >>> It would be kind of nice to use %pt formatting here instead of custom
> >>> datetime format, however, it would either require converting to time64_t
> >>> or using struct rtc_time. The latter would naturally have the right fields
> >>> but they're not unsigned so my comment above about %u is not going to work
> >>> well with it.
> >>>
> >>> I'm also a bit unsure whether it's appropriate to use that struct outside
> >>> of rtc realm. vsprintf.c seems to convert time64_t into rtc_time before
> >>> printing though.
> >>>
> >>> Hans, do you have any idea about the struct rtc_time?
>
> I don't really have any good ideas how to handle this. I agree that
> using %pt might be a good idea, but then as you say the data would first
> need to be converted to a struct rtc_time. All in all I think it is
> probably best to stick with the DIY formatting of the time.
> But I've no objections to doing the rtc_time conversion if people
> think that is cleaner / better.

I am working on this set of patches again, and I think using %pt and rtc_time
would be a little cleaner because rtc_time has all the required fields.
I noticed that vsprintf.c doesn't use the tm_yday and tm_wday fields,
however, is it safe to ignore them and assume they won't be used in the future?
If not, having to calculate them will probably make the code worse.

Regards,
Nikita

[PATCH 3/5] platform/x86: msi-ec: Filter out unsupported attributes

[Nikita Kravets]

Filter the attributes and only create those which are supported by the
currently loaded configuration. The filtered attributes are saved in an
attribute group to be easily created and removed.
root_attrs_support is an array of all attributes and their support
conditions. fw_version and fw_release_date are supported on all models
so their condition is always true.

Cc: Aakash Singh <mail@singhaakash.dev>
Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
Signed-off-by: Nikita Kravets <teackot@gmail.com>
---
 drivers/platform/x86/msi-ec.c | 40 +++++++++++++++++++++++++++--------
 drivers/platform/x86/msi-ec.h |  5 +++++
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
index 772b230fb47e..09472b21e093 100644
--- a/drivers/platform/x86/msi-ec.c
+++ b/drivers/platform/x86/msi-ec.c
@@ -872,22 +872,44 @@ static ssize_t fw_release_date_show(struct device *device,
 static DEVICE_ATTR_RO(fw_version);
 static DEVICE_ATTR_RO(fw_release_date);
 
-static struct attribute *msi_root_attrs[] = {
-	&dev_attr_fw_version.attr,
-	&dev_attr_fw_release_date.attr,
-	NULL
-};
-
-static struct attribute_group msi_root_group = {
-	.attrs = msi_root_attrs,
-};
+static struct attribute_group msi_root_group;
 
 /*
  * Sysfs platform driver
  */
 
+/*
+ * Copies supported attributes from `attributes` to `filtered`
+ */
+static void filter_attributes(struct attribute_support *attributes,
+			      struct attribute **filtered,
+			      size_t size)
+{
+	for (int i = 0, j = 0; i < size; i++) {
+		if (attributes[i].supported)
+			filtered[j++] = attributes[i].attribute;
+	}
+}
+
 static int msi_platform_probe(struct platform_device *pdev)
 {
+	struct attribute_support root_attrs_support[] = {
+		{
+			&dev_attr_fw_version.attr,
+			true,
+		},
+		{
+			&dev_attr_fw_release_date.attr,
+			true,
+		},
+	};
+
+	/* +1 to null-terminate the array */
+	static struct attribute *root_attrs[ARRAY_SIZE(root_attrs_support) + 1] = {0};
+
+	filter_attributes(root_attrs_support, root_attrs, ARRAY_SIZE(root_attrs_support));
+	msi_root_group.attrs = root_attrs;
+
 	return sysfs_create_group(&pdev->dev.kobj, &msi_root_group);
 }
 
diff --git a/drivers/platform/x86/msi-ec.h b/drivers/platform/x86/msi-ec.h
index be3533dc9cc6..f4198f0df5d9 100644
--- a/drivers/platform/x86/msi-ec.h
+++ b/drivers/platform/x86/msi-ec.h
@@ -119,4 +119,9 @@ struct msi_ec_conf {
 	struct msi_ec_kbd_bl_conf         kbd_bl;
 };
 
+struct attribute_support {
+	struct attribute *attribute;
+	bool supported;
+};
+
 #endif // _MSI_EC_H_
-- 
2.42.0

Re: [PATCH 3/5] platform/x86: msi-ec: Filter out unsupported attributes

[Ilpo Järvinen]

On Tue, 10 Oct 2023, Nikita Kravets wrote:

> Filter the attributes and only create those which are supported by the
> currently loaded configuration. The filtered attributes are saved in an
> attribute group to be easily created and removed.
> root_attrs_support is an array of all attributes and their support
> conditions. fw_version and fw_release_date are supported on all models
> so their condition is always true.
> 
> Cc: Aakash Singh <mail@singhaakash.dev>
> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
> Signed-off-by: Nikita Kravets <teackot@gmail.com>
> ---
>  drivers/platform/x86/msi-ec.c | 40 +++++++++++++++++++++++++++--------
>  drivers/platform/x86/msi-ec.h |  5 +++++
>  2 files changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
> index 772b230fb47e..09472b21e093 100644
> --- a/drivers/platform/x86/msi-ec.c
> +++ b/drivers/platform/x86/msi-ec.c
> @@ -872,22 +872,44 @@ static ssize_t fw_release_date_show(struct device *device,
>  static DEVICE_ATTR_RO(fw_version);
>  static DEVICE_ATTR_RO(fw_release_date);
>  
> -static struct attribute *msi_root_attrs[] = {
> -	&dev_attr_fw_version.attr,
> -	&dev_attr_fw_release_date.attr,
> -	NULL
> -};
> -
> -static struct attribute_group msi_root_group = {
> -	.attrs = msi_root_attrs,
> -};
> +static struct attribute_group msi_root_group;
>  
>  /*
>   * Sysfs platform driver
>   */
>  
> +/*
> + * Copies supported attributes from `attributes` to `filtered`
> + */
> +static void filter_attributes(struct attribute_support *attributes,
> +			      struct attribute **filtered,
> +			      size_t size)
> +{
> +	for (int i = 0, j = 0; i < size; i++) {
> +		if (attributes[i].supported)
> +			filtered[j++] = attributes[i].attribute;
> +	}

Use .is_visible in the attribute group to toggle visibility.

> +}
> +
>  static int msi_platform_probe(struct platform_device *pdev)
>  {
> +	struct attribute_support root_attrs_support[] = {
> +		{
> +			&dev_attr_fw_version.attr,
> +			true,
> +		},
> +		{
> +			&dev_attr_fw_release_date.attr,
> +			true,
> +		},
> +	};
> +
> +	/* +1 to null-terminate the array */
> +	static struct attribute *root_attrs[ARRAY_SIZE(root_attrs_support) + 1] = {0};
> +
> +	filter_attributes(root_attrs_support, root_attrs, ARRAY_SIZE(root_attrs_support));
> +	msi_root_group.attrs = root_attrs;
> +
>  	return sysfs_create_group(&pdev->dev.kobj, &msi_root_group);
>  }
>  
> diff --git a/drivers/platform/x86/msi-ec.h b/drivers/platform/x86/msi-ec.h
> index be3533dc9cc6..f4198f0df5d9 100644
> --- a/drivers/platform/x86/msi-ec.h
> +++ b/drivers/platform/x86/msi-ec.h
> @@ -119,4 +119,9 @@ struct msi_ec_conf {
>  	struct msi_ec_kbd_bl_conf         kbd_bl;
>  };
>  
> +struct attribute_support {
> +	struct attribute *attribute;
> +	bool supported;
> +};
> +
>  #endif // _MSI_EC_H_
> 

-- 
 i.

[PATCH 4/5] platform/x86: msi-ec: Add EC bit operation functions

[Nikita Kravets]

The EC of MSI laptops supports several features represented by a single
bit. Add ec_set_bit and ec_check_bit functions to operate on these bits.

Cc: Aakash Singh <mail@singhaakash.dev>
Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
Signed-off-by: Nikita Kravets <teackot@gmail.com>
---
 drivers/platform/x86/msi-ec.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
index 09472b21e093..ae73dcf01d09 100644
--- a/drivers/platform/x86/msi-ec.c
+++ b/drivers/platform/x86/msi-ec.c
@@ -699,6 +699,34 @@ static int ec_read_seq(u8 addr, u8 *buf, u8 len)
 	return 0;
 }
 
+static int ec_set_bit(u8 addr, u8 bit, bool value)
+{
+	int result;
+	u8 stored;
+
+	result = ec_read(addr, &stored);
+	if (result < 0)
+		return result;
+
+	stored ^= (-(u8) value ^ stored) & (1 << bit);
+
+	return ec_write(addr, stored);
+}
+
+static int ec_check_bit(u8 addr, u8 bit, bool *output)
+{
+	int result;
+	u8 stored;
+
+	result = ec_read(addr, &stored);
+	if (result < 0)
+		return result;
+
+	*output = (stored >> bit) & 1;
+
+	return 0;
+}
+
 static int ec_get_firmware_version(u8 buf[MSI_EC_FW_VERSION_LENGTH + 1])
 {
 	int result;
-- 
2.42.0

Re: [PATCH 4/5] platform/x86: msi-ec: Add EC bit operation functions

[Ilpo Järvinen]

On Tue, 10 Oct 2023, Nikita Kravets wrote:

> The EC of MSI laptops supports several features represented by a single
> bit. Add ec_set_bit and ec_check_bit functions to operate on these bits.
> 
> Cc: Aakash Singh <mail@singhaakash.dev>
> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
> Signed-off-by: Nikita Kravets <teackot@gmail.com>
> ---
>  drivers/platform/x86/msi-ec.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
> index 09472b21e093..ae73dcf01d09 100644
> --- a/drivers/platform/x86/msi-ec.c
> +++ b/drivers/platform/x86/msi-ec.c
> @@ -699,6 +699,34 @@ static int ec_read_seq(u8 addr, u8 *buf, u8 len)
>  	return 0;
>  }
>  
> +static int ec_set_bit(u8 addr, u8 bit, bool value)
> +{
> +	int result;
> +	u8 stored;
> +
> +	result = ec_read(addr, &stored);
> +	if (result < 0)
> +		return result;
> +
> +	stored ^= (-(u8) value ^ stored) & (1 << bit);

So first you case bool to u8 and then take negation of that unsigned 
number? ...My head is already hurting even without all the other logic.

This has to be rewritten to something that mere mortals can understand 
which doesn't explore all those odd corners of C spec. :-)

I didn't try to parse that logic through but I assuming it's the usual 
construct perhaps this could be simplified with (please be sure to check 
this throughoutly as I didn't try to understand what the original really 
does):

	bit = 1 << bit;
	stored &= ~bit;
	stored |= value ? bit : 0;

> +
> +	return ec_write(addr, stored);
> +}
> +
> +static int ec_check_bit(u8 addr, u8 bit, bool *output)
> +{
> +	int result;
> +	u8 stored;
> +
> +	result = ec_read(addr, &stored);
> +	if (result < 0)
> +		return result;
> +
> +	*output = (stored >> bit) & 1;
> +
> +	return 0;
> +}
> +
>  static int ec_get_firmware_version(u8 buf[MSI_EC_FW_VERSION_LENGTH + 1])
>  {
>  	int result;
> 

-- 
 i.

Re: [PATCH 4/5] platform/x86: msi-ec: Add EC bit operation functions

[Hans de Goede]

Hi,

On 10/11/23 14:59, Ilpo Järvinen wrote:
> On Tue, 10 Oct 2023, Nikita Kravets wrote:
> 
>> The EC of MSI laptops supports several features represented by a single
>> bit. Add ec_set_bit and ec_check_bit functions to operate on these bits.
>>
>> Cc: Aakash Singh <mail@singhaakash.dev>
>> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
>> Signed-off-by: Nikita Kravets <teackot@gmail.com>
>> ---
>>  drivers/platform/x86/msi-ec.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
>> index 09472b21e093..ae73dcf01d09 100644
>> --- a/drivers/platform/x86/msi-ec.c
>> +++ b/drivers/platform/x86/msi-ec.c
>> @@ -699,6 +699,34 @@ static int ec_read_seq(u8 addr, u8 *buf, u8 len)
>>  	return 0;
>>  }
>>  
>> +static int ec_set_bit(u8 addr, u8 bit, bool value)
>> +{
>> +	int result;
>> +	u8 stored;
>> +
>> +	result = ec_read(addr, &stored);
>> +	if (result < 0)
>> +		return result;
>> +
>> +	stored ^= (-(u8) value ^ stored) & (1 << bit);
> 
> So first you case bool to u8 and then take negation of that unsigned 
> number? ...My head is already hurting even without all the other logic.
> 
> This has to be rewritten to something that mere mortals can understand 
> which doesn't explore all those odd corners of C spec. :-)
> 
> I didn't try to parse that logic through but I assuming it's the usual 
> construct perhaps this could be simplified with (please be sure to check 
> this throughoutly as I didn't try to understand what the original really 
> does):
> 
> 	bit = 1 << bit;
> 	stored &= ~bit;
> 	stored |= value ? bit : 0;

Right instead of using a bit variable I would suggest
using the BIT(x) macro here:

 	stored &= ~BIT(bit);
 	stored |= value ? BIT(bit) : 0;

Also since you are exposing multiple userspace
entry points into the kernel this function may
race with itself, so I think you need to add
a mutex and lock this while doing the
read-modify-write to avoid 2 read-modify-write
cycles from racing with each other.

Regards,

Hans






> 
>> +
>> +	return ec_write(addr, stored);
>> +}
>> +
>> +static int ec_check_bit(u8 addr, u8 bit, bool *output)
>> +{
>> +	int result;
>> +	u8 stored;
>> +
>> +	result = ec_read(addr, &stored);
>> +	if (result < 0)
>> +		return result;
>> +
>> +	*output = (stored >> bit) & 1;
>> +
>> +	return 0;
>> +}
>> +
>>  static int ec_get_firmware_version(u8 buf[MSI_EC_FW_VERSION_LENGTH + 1])
>>  {
>>  	int result;
>>
> 

[PATCH 5/5] platform/x86: msi-ec: Add a cooler boost attribute

[Nikita Kravets]

Cooler boost increases the fan speed to improve the laptop cooling.
This is a simple on/off feature that is easy to test: if it works
you'll hear the fans spinning much faster. So far all supported models
have this feature represented by a single bit at the 0x98 EC address.
The attribute makes use of the previously added bit operation functions.

Cc: Aakash Singh <mail@singhaakash.dev>
Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
Signed-off-by: Nikita Kravets <teackot@gmail.com>
---
 drivers/platform/x86/msi-ec.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
index ae73dcf01d09..f4e770b3dda1 100644
--- a/drivers/platform/x86/msi-ec.c
+++ b/drivers/platform/x86/msi-ec.c
@@ -26,6 +26,7 @@
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
 #include <linux/string.h>
+#include <linux/kstrtox.h>
 
 #define SM_ECO_NAME		"eco"
 #define SM_COMFORT_NAME		"comfort"
@@ -850,6 +851,35 @@ static struct acpi_battery_hook battery_hook = {
  * Sysfs platform device attributes
  */
 
+static ssize_t cooler_boost_show(struct device *device,
+				 struct device_attribute *attr, char *buf)
+{
+	int result;
+	bool value;
+
+	result = ec_check_bit(conf.cooler_boost.address, conf.cooler_boost.bit, &value);
+
+	return sysfs_emit(buf, "%s\n", value ? "on" : "off");
+}
+
+static ssize_t cooler_boost_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	int result;
+	bool value;
+
+	result = kstrtobool(buf, &value);
+	if (result)
+		return result;
+
+	result = ec_set_bit(conf.cooler_boost.address, conf.cooler_boost.bit, value);
+	if (result < 0)
+		return result;
+
+	return count;
+}
+
 static ssize_t fw_version_show(struct device *device,
 			       struct device_attribute *attr, char *buf)
 {
@@ -897,6 +927,7 @@ static ssize_t fw_release_date_show(struct device *device,
 			  hour, minute, second);
 }
 
+static DEVICE_ATTR_RW(cooler_boost);
 static DEVICE_ATTR_RO(fw_version);
 static DEVICE_ATTR_RO(fw_release_date);
 
@@ -922,6 +953,10 @@ static void filter_attributes(struct attribute_support *attributes,
 static int msi_platform_probe(struct platform_device *pdev)
 {
 	struct attribute_support root_attrs_support[] = {
+		{
+			&dev_attr_cooler_boost.attr,
+			conf.cooler_boost.address != MSI_EC_ADDR_UNSUPP,
+		},
 		{
 			&dev_attr_fw_version.attr,
 			true,
-- 
2.42.0

Re: [PATCH 5/5] platform/x86: msi-ec: Add a cooler boost attribute

[Ilpo Järvinen]

On Tue, 10 Oct 2023, Nikita Kravets wrote:

> Cooler boost increases the fan speed to improve the laptop cooling.
> This is a simple on/off feature that is easy to test: if it works
> you'll hear the fans spinning much faster. So far all supported models
> have this feature represented by a single bit at the 0x98 EC address.
> The attribute makes use of the previously added bit operation functions.
> 
> Cc: Aakash Singh <mail@singhaakash.dev>
> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
> Signed-off-by: Nikita Kravets <teackot@gmail.com>
> ---
>  drivers/platform/x86/msi-ec.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
> index ae73dcf01d09..f4e770b3dda1 100644
> --- a/drivers/platform/x86/msi-ec.c
> +++ b/drivers/platform/x86/msi-ec.c
> @@ -26,6 +26,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/seq_file.h>
>  #include <linux/string.h>
> +#include <linux/kstrtox.h>
>  
>  #define SM_ECO_NAME		"eco"
>  #define SM_COMFORT_NAME		"comfort"
> @@ -850,6 +851,35 @@ static struct acpi_battery_hook battery_hook = {
>   * Sysfs platform device attributes
>   */
>  
> +static ssize_t cooler_boost_show(struct device *device,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	int result;
> +	bool value;
> +
> +	result = ec_check_bit(conf.cooler_boost.address, conf.cooler_boost.bit, &value);

Missing error handling.

> +	return sysfs_emit(buf, "%s\n", value ? "on" : "off");

str_on_off() from linux/string_choices.h.

> +}
> +
> +static ssize_t cooler_boost_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	int result;
> +	bool value;
> +
> +	result = kstrtobool(buf, &value);
> +	if (result)
> +		return result;
> +
> +	result = ec_set_bit(conf.cooler_boost.address, conf.cooler_boost.bit, value);
> +	if (result < 0)
> +		return result;
> +
> +	return count;
> +}
> +
>  static ssize_t fw_version_show(struct device *device,
>  			       struct device_attribute *attr, char *buf)
>  {
> @@ -897,6 +927,7 @@ static ssize_t fw_release_date_show(struct device *device,
>  			  hour, minute, second);
>  }
>  
> +static DEVICE_ATTR_RW(cooler_boost);
>  static DEVICE_ATTR_RO(fw_version);
>  static DEVICE_ATTR_RO(fw_release_date);
>  
> @@ -922,6 +953,10 @@ static void filter_attributes(struct attribute_support *attributes,
>  static int msi_platform_probe(struct platform_device *pdev)
>  {
>  	struct attribute_support root_attrs_support[] = {
> +		{
> +			&dev_attr_cooler_boost.attr,
> +			conf.cooler_boost.address != MSI_EC_ADDR_UNSUPP,
> +		},
>  		{
>  			&dev_attr_fw_version.attr,
>  			true,
> 

-- 
 i.