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