On Tue, Feb 26, 2019 at 11:29:58PM -0800, Life is hard, and then you die wrote:

On Tue, Feb 26, 2019 at 11:20:59AM +0200, Andy Shevchenko wrote:

quoted

On Thu, Feb 21, 2019 at 02:56:09AM -0800, Ronald Tschalär wrote:

quoted

quoted

+config KEYBOARD_APPLESPI
+	tristate "Apple SPI keyboard and trackpad"

quoted

+	depends on ACPI && SPI && EFI

I would rather want to see separate line for SPI...

quoted

+	depends on X86 || COMPILE_TEST

...like here

	depends on SPI

Sure. Generally, what is the criteria/rule here for splitting
conjunctions into separate 'depends'?

Rule of common sense.

For example UEFI and ACPI may have some relations, SPI and ACPI kinda
orthogonal.

quoted

+ #define DEV(applespi)           (&(applespi)->spi->dev)

quoted

quoted

+	if (memcmp(applespi->tx_status, status_ok, APPLESPI_STATUS_SIZE)) {

quoted

+		dev_warn(DEV(applespi), "Error writing to device: %*ph\n",

Hmm... DEV() is too generic name for custom macro. And frankly I don't think
it's good to have in the first place.

Yeah, I've been having trouble coming up with a better (but still
succinct) name - CORE_DEV()? RAW_DEV()? DEV_OF()? However, because
this expression is used in many places throughout the driver (mostly,
but not only, for logging statements) I feel like it's good to factor
it out. But I'll defer to your .

Please remove this macro for good. Otherwise big subsystems / drivers usually do something like

#define foo_err(...)	dev_err(...)
...

Don't know if it would help here, the driver is standalone and not so big.

quoted

quoted

+static void
+applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
+{
+	unsigned char tmp;

quoted

+	unsigned long *modifiers =
+			(unsigned long *)&keyboard_protocol->modifiers;

I would leave it on one online despite checkpatch warning (also, instead of
(unsigned long *) the (void *) might be used as a small trick).

quoted

+
+	if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
+	    !applespi_controlcodes[fnremap - 1])
+		return;
+
+	tmp = keyboard_protocol->fn_pressed;
+	keyboard_protocol->fn_pressed = test_bit(fnremap - 1, modifiers);
+	if (tmp)

quoted

+		__set_bit(fnremap - 1, modifiers);
+	else
+		__clear_bit(fnremap - 1, modifiers);

Oh, this is not good. modifiers should be really unsigned long bounary,
otherwise it is potential overflow.

Best to fix is to define them as unsigned long in the first place.

Can't do that directly, because keyboard_protocol->modifiers is a
field in the data received from the device, i.e. defined by that
protocol. Instead I could make a copy of the modifiers and pass that
around separately (i.e. in addition to the keyboard_protocol struct).

However, the implied size assertions here would basically still apply:

  MAX_MODIFIERS == sizeof(keyboard_protocol->modifiers) * 8
  ARRAY_SIZE(applespi_controlcodes) == sizeof(keyboard_protocol->modifiers) * 8

(hmm, MAX_MODIFIERS is really redundant - getting rid of it...)

Would using compiletime_assert()'s be an acceptable alternate approach
here? It would serve to both document the size constraint and to
protect against overflow due to an error in some future edit. E.g.

 applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
 {
        unsigned char tmp;
        unsigned long *modifiers = (void *)&keyboard_protocol->modifiers;
+
+       compiletime_assert(ARRAY_SIZE(applespi_controlcodes) ==
+                          sizeof_field(struct keyboard_protocol, modifiers) * 8,
+                          "applespi_controlcodes has wrong number of entries");
 
        if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
            !applespi_controlcodes[fnremap - 1])
 		return;
 
 	tmp = keyboard_protocol->fn_pressed;
 	keyboard_protocol->fn_pressed = test_bit(fnremap - 1, modifiers);
 	if (tmp)
 
 		__set_bit(fnremap - 1, modifiers);
 	else
 		__clear_bit(fnremap - 1, modifiers);
 }

Perhaps, simple

	__set_bit(b, x)		->	x |= BIT(b);
	__clear_bit(b, x)	->	x &= ~BIT(b);

?

quoted

quoted

+}

-- 
With Best Regards,
Andy Shevchenko