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:
> > On Thu, Feb 21, 2019 at 02:56:09AM -0800, Ronald Tschalär wrote:
> > > +config KEYBOARD_APPLESPI
> > > + tristate "Apple SPI keyboard and trackpad"
> >
> > > + depends on ACPI && SPI && EFI
> >
> > I would rather want to see separate line for SPI...
> >
> > > + 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.
> > + #define DEV(applespi) (&(applespi)->spi->dev)
> > > + if (memcmp(applespi->tx_status, status_ok, APPLESPI_STATUS_SIZE)) {
> >
> > > + 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.
> > > +static void
> > > +applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
> > > +{
> > > + unsigned char tmp;
> >
> > > + 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).
> >
> > > +
> > > + 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);
> >
> > 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);
?
> > > +}
--
With Best Regards,
Andy Shevchenko
