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@xxxxxxxxxxxxxxx>
>> Cc: Jose Angel Pastrana <japp0005@xxxxxxxxxxxx>
>> Signed-off-by: Nikita Kravets <teackot@xxxxxxxxx>
>> ---
>> 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
