| Message ID | 20231010172037.611063-11-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: > 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; >
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; >> >
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;
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(+)