| Message ID | 20231010172037.611063-7-teackot@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: msi-ec: Add the first platform device attributes | expand |
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.
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
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?
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
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; }
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(-)