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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.