* Re: [PATCH v3 4/4] Input: add Apple SPI keyboard and trackpad driver.
From: Andy Shevchenko @ 2019-03-27 9:35 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Dmitry Torokhov, Henrik Rydberg, Sergey Senozhatsky,
Steven Rostedt, Greg Kroah-Hartman, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327014807.7472-5-ronald@innovation.ch>
On Tue, Mar 26, 2019 at 06:48:07PM -0700, Ronald Tschalär wrote:
> The keyboard and trackpad on recent MacBook's (since 8,1) and
> MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> of USB, as previously. The higher level protocol is not publicly
> documented and hence has been reverse engineered. As a consequence there
> are still a number of unknown fields and commands. However, the known
> parts have been working well and received extensive testing and use.
>
> In order for this driver to work, the proper SPI drivers need to be
> loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
> for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
> reason enabling this driver in the config implies enabling the above
> drivers.
> +// SPDX-License-Identifier: GPL-2.0
According to last changes this should be GPL-2.0-only
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/crc16.h>
> +#include <linux/delay.h>
> +#include <linux/efi.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/ktime.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +#include <linux/spi/spi.h>
> +#include <linux/wait.h>
> +#include <linux/workqueue.h>
> +
> +#include <asm/barrier.h>
> +#include <asm-generic/unaligned.h>
generic?!
#include <asm/unaligned.h>
should work.
> +static const char *applespi_debug_facility(unsigned int log_mask)
> +{
> + switch (log_mask) {
> + case DBG_CMD_TP_INI:
> + return "Touchpad Initialization";
> + case DBG_CMD_BL:
> + return "Backlight Command";
> + case DBG_CMD_CL:
> + return "Caps-Lock Command";
> + case DBG_RD_KEYB:
> + return "Keyboard Event";
> + case DBG_RD_TPAD:
> + return "Touchpad Event";
> + case DBG_RD_UNKN:
> + return "Unknown Event";
> + case DBG_RD_IRQ:
> + return "Interrupt Request";
> + case DBG_RD_CRC:
> + return "Corrupted packet";
> + case DBG_TP_DIM:
> + return "Touchpad Dimensions";
> + default:
> + return "-Unrecognized log mask-";
I don't think '-' surroundings are needed, but this is rather minor. Up to you.
> + }
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3] HID: core: move Usage Page concatenation to Main item
From: Nicolas Saenz Julienne @ 2019-03-27 10:15 UTC (permalink / raw)
To: Junge, Terry, Jiri Kosina, Benjamin Tissoires
Cc: oneukum@suse.de, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <BYAPR02MB560738248E330DF46CAA199A8A5F0@BYAPR02MB5607.namprd02.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 6157 bytes --]
Hi Terry, thanks for the review!
On Tue, 2019-03-26 at 22:43 +0000, Junge, Terry wrote:
> Hi Nicolas,
>
> This patch looks good except for one comment/question below.
>
> Thanks,
> Terry
>
> On Tuesday, March 26, 2019 1:04 PM Nicolas Saenz Julienne <
> nsaenzjulienne@suse.de> wrote:
> > As seen on some USB wireless keyboards manufactured by Primax, the HID
> > parser was using some assumptions that are not always true. In this case
> > it's s
> > the fact that, inside the scope of a main item, an Usage Page will always
> > precede an Usage.
> >
> > The spec is not pretty clear as 6.2.2.7 states "Any usage that follows is
> > interpreted as a Usage ID and concatenated with the Usage Page".
> > While 6.2.2.8 states "When the parser encounters a main item it concatenates
> > the last declared Usage Page with a Usage to form a complete usage value."
> > Being somewhat contradictory it was decided to match Window's
> > implementation, which follows 6.2.2.8.
> >
> > In summary, the patch moves the Usage Page concatenation from the local
> > item parsing function to the main item parsing function.
> >
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >
> > v2->v3: - Update patch title
> >
> > v1->v2: - Add usage concatenation to hid_scan_main()
> > - Rework tests in hid-tools, making sure no-one is failing
> >
> > drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++------------
> > include/linux/hid.h | 1 +
> > 2 files changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index
> > 9993b692598f..40c836ce3248 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct
> > hid_parser *parser, unsigned type)
> > * Add a usage to the temporary parser table.
> > */
> >
> > -static int hid_add_usage(struct hid_parser *parser, unsigned usage)
> > +static int hid_add_usage(struct hid_parser *parser, unsigned usage,
> > +__u8 size)
> > {
> > if (parser->local.usage_index >= HID_MAX_USAGES) {
> > hid_err(parser->device, "usage index exceeded\n");
> > return -1;
> > }
> > parser->local.usage[parser->local.usage_index] = usage;
> > + parser->local.usage_size[parser->local.usage_index] = size;
> > parser->local.collection_index[parser->local.usage_index] =
> > parser->collection_stack_ptr ?
> > parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
> > @@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > - if (item->size <= 2)
> > - data = (parser->global.usage_page << 16) + data;
> > -
> > - return hid_add_usage(parser, data);
> > + return hid_add_usage(parser, data, item->size);
> >
> > case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
> >
> > @@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > - if (item->size <= 2)
> > - data = (parser->global.usage_page << 16) + data;
> > -
> > parser->local.usage_minimum = data;
> > return 0;
> >
> > @@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > - if (item->size <= 2)
> > - data = (parser->global.usage_page << 16) + data;
> > -
> > count = data - parser->local.usage_minimum;
> > if (count + parser->local.usage_index >= HID_MAX_USAGES) {
> > /*
> > @@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > }
> >
> > for (n = parser->local.usage_minimum; n <= data; n++)
> > - if (hid_add_usage(parser, n)) {
> > + if (hid_add_usage(parser, n, item->size)) {
> > dbg_hid("hid_add_usage failed\n");
> > return -1;
> > }
> > @@ -547,6 +539,26 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > +/*
> > + * Concatenate Usage Pages into Usages where relevant:
> > + * As per specification, 6.2.2.8: "When the parser encounters a main
> > +item it
> > + * concatenates the last declared Usage Page with a Usage to form a
> > +complete
> > + * usage value."
> > + */
> > +
> > +static void hid_concatenate_usage_page(struct hid_parser *parser) {
> > + unsigned usages;
> > + int i;
> > +
> > + usages = max_t(unsigned, parser->local.usage_index,
> > + parser->global.report_count);
>
> I don't think we need to worry about global.report_count here,
> just concatenate for the usages currently in the local queue so could
> this be simplified by removing usages and just using local.usage_index?
>
> for (i = 0; i < local.usage_index; i++)
Sounds about right.
>
> > +
> > + for (i = 0; i < usages; i++)
> > + if (parser->local.usage_size[i] <= 2)
> > + parser->local.usage[i] += parser->global.usage_page
> > << 16; }
> > +
> > /*
> > * Process a main item.
> > */
> > @@ -556,6 +568,8 @@ static int hid_parser_main(struct hid_parser *parser,
> > struct hid_item *item)
> > __u32 data;
> > int ret;
> >
> > + hid_concatenate_usage_page(parser);
> > +
> > data = item_udata(item);
> >
> > switch (item->tag) {
> > @@ -765,6 +779,8 @@ static int hid_scan_main(struct hid_parser *parser,
> > struct hid_item *item)
> > __u32 data;
> > int i;
> >
> > + hid_concatenate_usage_page(parser);
> > +
> > data = item_udata(item);
> >
> > switch (item->tag) {
> > diff --git a/include/linux/hid.h b/include/linux/hid.h index
> > f9707d1dcb58..d1fb4b678873 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -417,6 +417,7 @@ struct hid_global {
> >
> > struct hid_local {
> > unsigned usage[HID_MAX_USAGES]; /* usage array */
> > + __u8 usage_size[HID_MAX_USAGES]; /* usage size array */
> > unsigned collection_index[HID_MAX_USAGES]; /* collection index
> > array */
> > unsigned usage_index;
> > unsigned usage_minimum;
> > --
> > 2.21.0
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v3] HID: core: move Usage Page concatenation to Main item
From: Nicolas Saenz Julienne @ 2019-03-27 10:16 UTC (permalink / raw)
To: Oliver Neukum, Jiri Kosina, Benjamin Tissoires
Cc: Terry.Junge, linux-input, linux-kernel
In-Reply-To: <1553679302.14990.0.camel@suse.com>
[-- Attachment #1: Type: text/plain, Size: 702 bytes --]
Hi Oliver, thanks for the review!
On Wed, 2019-03-27 at 10:35 +0100, Oliver Neukum wrote:
> On Di, 2019-03-26 at 21:03 +0100, Nicolas Saenz Julienne wrote:
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct
> > hid_parser *parser, unsigned type)
> > * Add a usage to the temporary parser table.
> > */
> >
> > -static int hid_add_usage(struct hid_parser *parser, unsigned usage)
> > +static int hid_add_usage(struct hid_parser *parser, unsigned usage, __u8
> > size)
>
> No need to use the __u8 style inside the kernel. u8 will do.
Noted, fixed for v4.
>
> Regards
> Oliver
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v4] HID: core: move Usage Page concatenation to Main item
From: Nicolas Saenz Julienne @ 2019-03-27 10:18 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: oneukum, Terry.Junge, Nicolas Saenz Julienne, linux-input,
linux-kernel
As seen on some USB wireless keyboards manufactured by Primax, the HID
parser was using some assumptions that are not always true. In this case
it's s the fact that, inside the scope of a main item, an Usage Page
will always precede an Usage.
The spec is not pretty clear as 6.2.2.7 states "Any usage that follows
is interpreted as a Usage ID and concatenated with the Usage Page".
While 6.2.2.8 states "When the parser encounters a main item it
concatenates the last declared Usage Page with a Usage to form a
complete usage value." Being somewhat contradictory it was decided to
match Window's implementation, which follows 6.2.2.8.
In summary, the patch moves the Usage Page concatenation from the local
item parsing function to the main item parsing function.
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
v3->v4: - Use u8 instead of __u8
- Remove overly complex usage size calculation
v2->v3: - Update patch title
v1->v2: - Add usage concatenation to hid_scan_main()
- Rework tests in hid-tools, making sure no-one is failing
drivers/hid/hid-core.c | 36 ++++++++++++++++++++++++------------
include/linux/hid.h | 1 +
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9993b692598f..852bbd303d9a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct hid_parser *parser, unsigned type)
* Add a usage to the temporary parser table.
*/
-static int hid_add_usage(struct hid_parser *parser, unsigned usage)
+static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
{
if (parser->local.usage_index >= HID_MAX_USAGES) {
hid_err(parser->device, "usage index exceeded\n");
return -1;
}
parser->local.usage[parser->local.usage_index] = usage;
+ parser->local.usage_size[parser->local.usage_index] = size;
parser->local.collection_index[parser->local.usage_index] =
parser->collection_stack_ptr ?
parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
@@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
- return hid_add_usage(parser, data);
+ return hid_add_usage(parser, data, item->size);
case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
@@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
parser->local.usage_minimum = data;
return 0;
@@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
count = data - parser->local.usage_minimum;
if (count + parser->local.usage_index >= HID_MAX_USAGES) {
/*
@@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
}
for (n = parser->local.usage_minimum; n <= data; n++)
- if (hid_add_usage(parser, n)) {
+ if (hid_add_usage(parser, n, item->size)) {
dbg_hid("hid_add_usage failed\n");
return -1;
}
@@ -547,6 +539,22 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
return 0;
}
+/*
+ * Concatenate Usage Pages into Usages where relevant:
+ * As per specification, 6.2.2.8: "When the parser encounters a main item it
+ * concatenates the last declared Usage Page with a Usage to form a complete
+ * usage value."
+ */
+
+static void hid_concatenate_usage_page(struct hid_parser *parser)
+{
+ int i;
+
+ for (i = 0; i < parser->local.usage_index; i++)
+ if (parser->local.usage_size[i] <= 2)
+ parser->local.usage[i] += parser->global.usage_page << 16;
+}
+
/*
* Process a main item.
*/
@@ -556,6 +564,8 @@ static int hid_parser_main(struct hid_parser *parser, struct hid_item *item)
__u32 data;
int ret;
+ hid_concatenate_usage_page(parser);
+
data = item_udata(item);
switch (item->tag) {
@@ -765,6 +775,8 @@ static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
__u32 data;
int i;
+ hid_concatenate_usage_page(parser);
+
data = item_udata(item);
switch (item->tag) {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f9707d1dcb58..ac0c70b4ce10 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -417,6 +417,7 @@ struct hid_global {
struct hid_local {
unsigned usage[HID_MAX_USAGES]; /* usage array */
+ u8 usage_size[HID_MAX_USAGES]; /* usage size array */
unsigned collection_index[HID_MAX_USAGES]; /* collection index array */
unsigned usage_index;
unsigned usage_minimum;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 1/1] hid: hid-sensor-custom: simplify getting .driver_data
From: Simon Horman @ 2019-03-27 12:12 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-kernel, linux-renesas-soc, Jiri Kosina, Jonathan Cameron,
Srinivas Pandruvada, Benjamin Tissoires, linux-input, linux-iio
In-Reply-To: <20190319163638.31003-1-wsa+renesas@sang-engineering.com>
On Tue, Mar 19, 2019 at 05:36:38PM +0100, Wolfram Sang wrote:
> We should get 'driver_data' from 'struct device' directly. Going via
> platform_device is an unneeded step back and forth.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>
> Build tested only. buildbot is happy.
>
> drivers/hid/hid-sensor-custom.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index bb012bc032e0..462e653a7bbb 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -157,8 +157,7 @@ static int usage_id_cmp(const void *p1, const void *p2)
> static ssize_t enable_sensor_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct hid_sensor_custom *sensor_inst = platform_get_drvdata(pdev);
> + struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
>
> return sprintf(buf, "%d\n", sensor_inst->enable);
> }
> @@ -237,8 +236,7 @@ static ssize_t enable_sensor_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct hid_sensor_custom *sensor_inst = platform_get_drvdata(pdev);
> + struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
> int value;
> int ret = -EINVAL;
>
> @@ -283,8 +281,7 @@ static const struct attribute_group enable_sensor_attr_group = {
> static ssize_t show_value(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct hid_sensor_custom *sensor_inst = platform_get_drvdata(pdev);
> + struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
> struct hid_sensor_hub_attribute_info *attribute;
> int index, usage, field_index;
> char name[HID_CUSTOM_NAME_LENGTH];
> @@ -392,8 +389,7 @@ static ssize_t show_value(struct device *dev, struct device_attribute *attr,
> static ssize_t store_value(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct hid_sensor_custom *sensor_inst = platform_get_drvdata(pdev);
> + struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
> int index, field_index, usage;
> char name[HID_CUSTOM_NAME_LENGTH];
> int value;
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH 1/1] hid: hid-sensor-custom: simplify getting .driver_data
From: Jiri Kosina @ 2019-03-27 13:06 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-kernel, linux-renesas-soc, Jonathan Cameron,
Srinivas Pandruvada, Benjamin Tissoires, linux-input, linux-iio
In-Reply-To: <20190319163638.31003-1-wsa+renesas@sang-engineering.com>
On Tue, 19 Mar 2019, Wolfram Sang wrote:
> We should get 'driver_data' from 'struct device' directly. Going via
> platform_device is an unneeded step back and forth.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Applied to for-5.2/sensor, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: quirks: Fix keyboard + touchpad on Lenovo Miix 630
From: Jiri Kosina @ 2019-03-27 13:17 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: benjamin.tissoires, linux-input, bjorn.andersson, lee.jones,
linux-arm-msm, linux-kernel
In-Reply-To: <20190326165554.17266-1-jeffrey.l.hugo@gmail.com>
On Tue, 26 Mar 2019, Jeffrey Hugo wrote:
> Similar to commit edfc3722cfef ("HID: quirks: Fix keyboard + touchpad on
> Toshiba Click Mini not working"), the Lenovo Miix 630 has a combo
> keyboard/touchpad device with vid:pid of 04F3:0400, which is shared with
> Elan touchpads. The combo on the Miix 630 has an ACPI id of QTEC0001,
> which is not claimed by the elan_i2c driver, so key on that similar to
> what was done for the Toshiba Click Mini.
Applied to for-5.1/upstream-fixes, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 1/4] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.
From: Andrzej Hajda @ 2019-03-27 14:13 UTC (permalink / raw)
To: Ronald Tschalär, Dmitry Torokhov, Henrik Rydberg,
Andy Shevchenko, Sergey Senozhatsky, Steven Rostedt,
Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel,
Inki Dae, dri-devel@lists.freedesktop.org
In-Reply-To: <20190327014807.7472-2-ronald@innovation.ch>
+cc: dri-devel
On 27.03.2019 02:48, Ronald Tschalär wrote:
> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> INPUT. However, this causes problems with other drivers, in particular
> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> future commit):
>
> drivers/clk/Kconfig:9:error: recursive dependency detected!
> drivers/clk/Kconfig:9: symbol COMMON_CLK is selected by MFD_INTEL_LPSS
> drivers/mfd/Kconfig:566: symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
> drivers/mfd/Kconfig:580: symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
> drivers/input/keyboard/Kconfig:73: symbol KEYBOARD_APPLESPI depends on INPUT
> drivers/input/Kconfig:8: symbol INPUT is selected by DRM_SIL_SII8620
> drivers/gpu/drm/bridge/Kconfig:83: symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
> drivers/gpu/drm/bridge/Kconfig:1: symbol DRM_BRIDGE is selected by DRM_PL111
> drivers/gpu/drm/pl111/Kconfig:1: symbol DRM_PL111 depends on COMMON_CLK
>
> According to the docs, select should only be used for non-visible
> symbols. Furthermore almost all other references to INPUT throughout the
> kernel config are depends, not selects. Hence this change.
>
> CC: Inki Dae <inki.dae@samsung.com>
> CC: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
This is drm bridge driver, next time please cc it to dri-devel ML also.
Anyway this is not the solution we have agreed to.
Why have you abandoned the patch [1]? It needed just some minor
polishing, as I wrote in response to this mail.
[1]:
https://lore.kernel.org/lkml/20190124082423.23139-1-ronald@innovation.ch/T/#mf620df0b1583096a214d8e2e690387078583472f
Regards
Andrzej
> ---
> drivers/gpu/drm/bridge/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 2fee47b0d50b..eabedc83f25c 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
> config DRM_SIL_SII8620
> tristate "Silicon Image SII8620 HDMI/MHL bridge"
> depends on OF
> + depends on INPUT
> select DRM_KMS_HELPER
> imply EXTCON
> - select INPUT
> select RC_CORE
> help
> Silicon Image SII8620 HDMI/MHL bridge chip driver.
^ permalink raw reply
* Re: [PATCH] HID: intel-ish-hid: ISH firmware loader client driver
From: Nick Crews @ 2019-03-27 16:01 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: Rushikesh S Kadam, benjamin.tissoires, jikos, jettrink, gwendal,
linux-kernel, linux-input
In-Reply-To: <de917efc4ded94b3fbc70913207881b3a0c10b52.camel@linux.intel.com>
On Tue, Mar 26, 2019 at 8:22 PM Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Tue, 2019-03-26 at 18:39 -0600, Nick Crews wrote:
> > Hi Rushikesh, I know I've been reviewing this on Chromium, but I have
> > some more larges-scale design thoughts.
> Hi Nick.
>
> Does this fundamentally change, the way it is done here or can wait for
> subsequent revisions later?
I don't have any official stakes in this, as I'm not the maintainer or
anything, so
I'm just preaching what I think would be good design :)
I think I would like to see most of my suggestions addressed. At the
very least there
are some actual bugs (infinite loops, accessing bad memory, not
reporting all errors)
that need to get fixed. Of course I'm not the one that has to write
or test it, but I imagine
that the one large design change I proposed of where memory is
allocated shouldn't be too
hard either. I worry that "subsequent revisions" to upstream won't
happen, since it's hard enough
to get a patch accepted. Maybe that concern isn't warranted though, I
don't have that
much experience on the LKML.
Is there a really tight deadline for this? If so then I would say we
should apply what
we currently have to the Chromium tree, and upstream the final version
when it's done.
Thanks,
Nick
>
> Thanks,
> Srinivas
>
^ permalink raw reply
* [PATCH AUTOSEL 5.0 132/262] HID: intel-ish-hid: avoid binding wrong ishtp_cl_device
From: Sasha Levin @ 2019-03-27 17:59 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Hong Liu, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190327180158.10245-1-sashal@kernel.org>
From: Hong Liu <hong.liu@intel.com>
[ Upstream commit 0d28f49412405d87d3aae83da255070a46e67627 ]
When performing a warm reset in ishtp bus driver, the ishtp_cl_device
will not be removed, its fw_client still points to the already freed
ishtp_device.fw_clients array.
Later after driver finishing ishtp client enumeration, this dangling
pointer may cause driver to bind the wrong ishtp_cl_device to the new
client, causing wrong callback to be called for messages intended for
the new client.
This helps in development of firmware where frequent switching of
firmwares is required without Linux reboot.
Signed-off-by: Hong Liu <hong.liu@intel.com>
Tested-by: Hongyan Song <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 728dc6d4561a..a271d6d169b1 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -675,7 +675,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl)
spin_lock_irqsave(&cl->dev->device_list_lock, flags);
list_for_each_entry(cl_device, &cl->dev->device_list,
device_link) {
- if (cl_device->fw_client->client_id == cl->fw_client_id) {
+ if (cl_device->fw_client &&
+ cl_device->fw_client->client_id == cl->fw_client_id) {
cl->device = cl_device;
rv = 0;
break;
@@ -735,6 +736,7 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev,
spin_lock_irqsave(&ishtp_dev->device_list_lock, flags);
list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list,
device_link) {
+ cl_device->fw_client = NULL;
if (warm_reset && cl_device->reference_count)
continue;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.0 219/262] HID: intel-ish: ipc: handle PIMR before ish_wakeup also clear PISR busy_clear bit
From: Sasha Levin @ 2019-03-27 18:01 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Song Hongyan, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190327180158.10245-1-sashal@kernel.org>
From: Song Hongyan <hongyan.song@intel.com>
[ Upstream commit 2edefc056e4f0e6ec9508dd1aca2c18fa320efef ]
Host driver should handle interrupt mask register earlier than wake up ish FW
else there will be conditions when FW interrupt comes, host PIMR register still
not set ready, so move the interrupt mask setting before ish_wakeup.
Clear PISR busy_clear bit in ish_irq_handler. If not clear, there will be
conditions host driver received a busy_clear interrupt (before the busy_clear
mask bit is ready), it will return IRQ_NONE after check_generated_interrupt,
the interrupt will never be cleared, causing the DEVICE not sending following
IRQ.
Since PISR clear should not be called for the CHV device we do this change.
After the change, both ISH2HOST interrupt and busy_clear interrupt will be
considered as interrupt from ISH, busy_clear interrupt will return IRQ_HANDLED
from IPC_IS_BUSY check.
Signed-off-by: Song Hongyan <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 742191bb24c6..45e33c7ba9a6 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -91,7 +91,10 @@ static bool check_generated_interrupt(struct ishtp_device *dev)
IPC_INT_FROM_ISH_TO_HOST_CHV_AB(pisr_val);
} else {
pisr_val = ish_reg_read(dev, IPC_REG_PISR_BXT);
- interrupt_generated = IPC_INT_FROM_ISH_TO_HOST_BXT(pisr_val);
+ interrupt_generated = !!pisr_val;
+ /* only busy-clear bit is RW, others are RO */
+ if (pisr_val)
+ ish_reg_write(dev, IPC_REG_PISR_BXT, pisr_val);
}
return interrupt_generated;
@@ -839,11 +842,11 @@ int ish_hw_start(struct ishtp_device *dev)
{
ish_set_host_rdy(dev);
+ set_host_ready(dev);
+
/* After that we can enable ISH DMA operation and wakeup ISHFW */
ish_wakeup(dev);
- set_host_ready(dev);
-
/* wait for FW-initiated reset flow */
if (!dev->recvd_hw_ready)
wait_event_interruptible_timeout(dev->wait_hw_ready,
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.0 254/262] Input: soc_button_array - fix mapping of the 5th GPIO in a PNP0C40 device
From: Sasha Levin @ 2019-03-27 18:01 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Hans de Goede, Dmitry Torokhov, Sasha Levin, linux-input
In-Reply-To: <20190327180158.10245-1-sashal@kernel.org>
From: Hans de Goede <hdegoede@redhat.com>
[ Upstream commit e9eb788f9442d1b5d93efdb30c3be071ce8a22b1 ]
The Microsoft documenation for the PNP0C40 device aka the
"Windows-compatible button array" describes the 5th GpioInt listed in
the resources as: '5. Interrupt corresponding to the "Rotation Lock"
button, if supported'.
Notice this describes the 5th entry as a button while we sofar have been
mapping it to EV_SW, SW_ROTATE_LOCK. On my Point of View TAB P1006W-232
which actually comes with a rotation-lock button, the button indeed is a
button and not a slider/switch. An image search for other Windows tablets
has found 2 more models with a rotation-lock button and on both of those
it too is a push-button and not a slider/switch.
Further evidence can be found in the HUT extension HUTRR52 from Microsoft
which adds rotation lock support to the HUT, which describes 2 different
usages: "0xC9 System Display Rotation Lock Button" and
"0xCA System Display Rotation Lock Slider Switch" note that switch is seen
as a separate thing here and the non switch wording is an exact match for
the "Windows-compatible button array" spec wording.
TL;DR: our current mapping of the 5th GPIO to SW_ROTATE_LOCK is wrong
because the 5th GPIO is for a push-button not a switch.
This commit fixes this by maping the 5th GPIO to KEY_ROTATE_LOCK_TOGGLE.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/input/misc/soc_button_array.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index 23520df7650f..55cd6e0b409c 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -373,7 +373,7 @@ static struct soc_button_info soc_button_PNP0C40[] = {
{ "home", 1, EV_KEY, KEY_LEFTMETA, false, true },
{ "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false },
{ "volume_down", 3, EV_KEY, KEY_VOLUMEDOWN, true, false },
- { "rotation_lock", 4, EV_SW, SW_ROTATE_LOCK, false, false },
+ { "rotation_lock", 4, EV_KEY, KEY_ROTATE_LOCK_TOGGLE, false, false },
{ }
};
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 095/192] HID: intel-ish-hid: avoid binding wrong ishtp_cl_device
From: Sasha Levin @ 2019-03-27 18:08 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Hong Liu, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190327181025.13507-1-sashal@kernel.org>
From: Hong Liu <hong.liu@intel.com>
[ Upstream commit 0d28f49412405d87d3aae83da255070a46e67627 ]
When performing a warm reset in ishtp bus driver, the ishtp_cl_device
will not be removed, its fw_client still points to the already freed
ishtp_device.fw_clients array.
Later after driver finishing ishtp client enumeration, this dangling
pointer may cause driver to bind the wrong ishtp_cl_device to the new
client, causing wrong callback to be called for messages intended for
the new client.
This helps in development of firmware where frequent switching of
firmwares is required without Linux reboot.
Signed-off-by: Hong Liu <hong.liu@intel.com>
Tested-by: Hongyan Song <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 2623a567ffba..f546635e9ac9 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -623,7 +623,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl)
spin_lock_irqsave(&cl->dev->device_list_lock, flags);
list_for_each_entry(cl_device, &cl->dev->device_list,
device_link) {
- if (cl_device->fw_client->client_id == cl->fw_client_id) {
+ if (cl_device->fw_client &&
+ cl_device->fw_client->client_id == cl->fw_client_id) {
cl->device = cl_device;
rv = 0;
break;
@@ -683,6 +684,7 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev,
spin_lock_irqsave(&ishtp_dev->device_list_lock, flags);
list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list,
device_link) {
+ cl_device->fw_client = NULL;
if (warm_reset && cl_device->reference_count)
continue;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 162/192] HID: intel-ish: ipc: handle PIMR before ish_wakeup also clear PISR busy_clear bit
From: Sasha Levin @ 2019-03-27 18:09 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Song Hongyan, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190327181025.13507-1-sashal@kernel.org>
From: Song Hongyan <hongyan.song@intel.com>
[ Upstream commit 2edefc056e4f0e6ec9508dd1aca2c18fa320efef ]
Host driver should handle interrupt mask register earlier than wake up ish FW
else there will be conditions when FW interrupt comes, host PIMR register still
not set ready, so move the interrupt mask setting before ish_wakeup.
Clear PISR busy_clear bit in ish_irq_handler. If not clear, there will be
conditions host driver received a busy_clear interrupt (before the busy_clear
mask bit is ready), it will return IRQ_NONE after check_generated_interrupt,
the interrupt will never be cleared, causing the DEVICE not sending following
IRQ.
Since PISR clear should not be called for the CHV device we do this change.
After the change, both ISH2HOST interrupt and busy_clear interrupt will be
considered as interrupt from ISH, busy_clear interrupt will return IRQ_HANDLED
from IPC_IS_BUSY check.
Signed-off-by: Song Hongyan <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index bfbca7ec54ce..e00b9dbe220f 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -91,7 +91,10 @@ static bool check_generated_interrupt(struct ishtp_device *dev)
IPC_INT_FROM_ISH_TO_HOST_CHV_AB(pisr_val);
} else {
pisr_val = ish_reg_read(dev, IPC_REG_PISR_BXT);
- interrupt_generated = IPC_INT_FROM_ISH_TO_HOST_BXT(pisr_val);
+ interrupt_generated = !!pisr_val;
+ /* only busy-clear bit is RW, others are RO */
+ if (pisr_val)
+ ish_reg_write(dev, IPC_REG_PISR_BXT, pisr_val);
}
return interrupt_generated;
@@ -843,11 +846,11 @@ int ish_hw_start(struct ishtp_device *dev)
{
ish_set_host_rdy(dev);
+ set_host_ready(dev);
+
/* After that we can enable ISH DMA operation and wakeup ISHFW */
ish_wakeup(dev);
- set_host_ready(dev);
-
/* wait for FW-initiated reset flow */
if (!dev->recvd_hw_ready)
wait_event_interruptible_timeout(dev->wait_hw_ready,
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 187/192] Input: soc_button_array - fix mapping of the 5th GPIO in a PNP0C40 device
From: Sasha Levin @ 2019-03-27 18:10 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Hans de Goede, Dmitry Torokhov, Sasha Levin, linux-input
In-Reply-To: <20190327181025.13507-1-sashal@kernel.org>
From: Hans de Goede <hdegoede@redhat.com>
[ Upstream commit e9eb788f9442d1b5d93efdb30c3be071ce8a22b1 ]
The Microsoft documenation for the PNP0C40 device aka the
"Windows-compatible button array" describes the 5th GpioInt listed in
the resources as: '5. Interrupt corresponding to the "Rotation Lock"
button, if supported'.
Notice this describes the 5th entry as a button while we sofar have been
mapping it to EV_SW, SW_ROTATE_LOCK. On my Point of View TAB P1006W-232
which actually comes with a rotation-lock button, the button indeed is a
button and not a slider/switch. An image search for other Windows tablets
has found 2 more models with a rotation-lock button and on both of those
it too is a push-button and not a slider/switch.
Further evidence can be found in the HUT extension HUTRR52 from Microsoft
which adds rotation lock support to the HUT, which describes 2 different
usages: "0xC9 System Display Rotation Lock Button" and
"0xCA System Display Rotation Lock Slider Switch" note that switch is seen
as a separate thing here and the non switch wording is an exact match for
the "Windows-compatible button array" spec wording.
TL;DR: our current mapping of the 5th GPIO to SW_ROTATE_LOCK is wrong
because the 5th GPIO is for a push-button not a switch.
This commit fixes this by maping the 5th GPIO to KEY_ROTATE_LOCK_TOGGLE.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/input/misc/soc_button_array.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index 23520df7650f..55cd6e0b409c 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -373,7 +373,7 @@ static struct soc_button_info soc_button_PNP0C40[] = {
{ "home", 1, EV_KEY, KEY_LEFTMETA, false, true },
{ "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false },
{ "volume_down", 3, EV_KEY, KEY_VOLUMEDOWN, true, false },
- { "rotation_lock", 4, EV_SW, SW_ROTATE_LOCK, false, false },
+ { "rotation_lock", 4, EV_KEY, KEY_ROTATE_LOCK_TOGGLE, false, false },
{ }
};
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 063/123] HID: intel-ish-hid: avoid binding wrong ishtp_cl_device
From: Sasha Levin @ 2019-03-27 18:15 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Hong Liu, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190327181628.15899-1-sashal@kernel.org>
From: Hong Liu <hong.liu@intel.com>
[ Upstream commit 0d28f49412405d87d3aae83da255070a46e67627 ]
When performing a warm reset in ishtp bus driver, the ishtp_cl_device
will not be removed, its fw_client still points to the already freed
ishtp_device.fw_clients array.
Later after driver finishing ishtp client enumeration, this dangling
pointer may cause driver to bind the wrong ishtp_cl_device to the new
client, causing wrong callback to be called for messages intended for
the new client.
This helps in development of firmware where frequent switching of
firmwares is required without Linux reboot.
Signed-off-by: Hong Liu <hong.liu@intel.com>
Tested-by: Hongyan Song <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 2623a567ffba..f546635e9ac9 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -623,7 +623,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl)
spin_lock_irqsave(&cl->dev->device_list_lock, flags);
list_for_each_entry(cl_device, &cl->dev->device_list,
device_link) {
- if (cl_device->fw_client->client_id == cl->fw_client_id) {
+ if (cl_device->fw_client &&
+ cl_device->fw_client->client_id == cl->fw_client_id) {
cl->device = cl_device;
rv = 0;
break;
@@ -683,6 +684,7 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev,
spin_lock_irqsave(&ishtp_dev->device_list_lock, flags);
list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list,
device_link) {
+ cl_device->fw_client = NULL;
if (warm_reset && cl_device->reference_count)
continue;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 103/123] HID: intel-ish: ipc: handle PIMR before ish_wakeup also clear PISR busy_clear bit
From: Sasha Levin @ 2019-03-27 18:16 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Song Hongyan, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190327181628.15899-1-sashal@kernel.org>
From: Song Hongyan <hongyan.song@intel.com>
[ Upstream commit 2edefc056e4f0e6ec9508dd1aca2c18fa320efef ]
Host driver should handle interrupt mask register earlier than wake up ish FW
else there will be conditions when FW interrupt comes, host PIMR register still
not set ready, so move the interrupt mask setting before ish_wakeup.
Clear PISR busy_clear bit in ish_irq_handler. If not clear, there will be
conditions host driver received a busy_clear interrupt (before the busy_clear
mask bit is ready), it will return IRQ_NONE after check_generated_interrupt,
the interrupt will never be cleared, causing the DEVICE not sending following
IRQ.
Since PISR clear should not be called for the CHV device we do this change.
After the change, both ISH2HOST interrupt and busy_clear interrupt will be
considered as interrupt from ISH, busy_clear interrupt will return IRQ_HANDLED
from IPC_IS_BUSY check.
Signed-off-by: Song Hongyan <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 9a60ec13cb10..a3106fcc2253 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -91,7 +91,10 @@ static bool check_generated_interrupt(struct ishtp_device *dev)
IPC_INT_FROM_ISH_TO_HOST_CHV_AB(pisr_val);
} else {
pisr_val = ish_reg_read(dev, IPC_REG_PISR_BXT);
- interrupt_generated = IPC_INT_FROM_ISH_TO_HOST_BXT(pisr_val);
+ interrupt_generated = !!pisr_val;
+ /* only busy-clear bit is RW, others are RO */
+ if (pisr_val)
+ ish_reg_write(dev, IPC_REG_PISR_BXT, pisr_val);
}
return interrupt_generated;
@@ -843,11 +846,11 @@ int ish_hw_start(struct ishtp_device *dev)
{
ish_set_host_rdy(dev);
+ set_host_ready(dev);
+
/* After that we can enable ISH DMA operation and wakeup ISHFW */
ish_wakeup(dev);
- set_host_ready(dev);
-
/* wait for FW-initiated reset flow */
if (!dev->recvd_hw_ready)
wait_event_interruptible_timeout(dev->wait_hw_ready,
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.9 44/87] HID: intel-ish-hid: avoid binding wrong ishtp_cl_device
From: Sasha Levin @ 2019-03-27 18:19 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Hong Liu, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190327182040.17444-1-sashal@kernel.org>
From: Hong Liu <hong.liu@intel.com>
[ Upstream commit 0d28f49412405d87d3aae83da255070a46e67627 ]
When performing a warm reset in ishtp bus driver, the ishtp_cl_device
will not be removed, its fw_client still points to the already freed
ishtp_device.fw_clients array.
Later after driver finishing ishtp client enumeration, this dangling
pointer may cause driver to bind the wrong ishtp_cl_device to the new
client, causing wrong callback to be called for messages intended for
the new client.
This helps in development of firmware where frequent switching of
firmwares is required without Linux reboot.
Signed-off-by: Hong Liu <hong.liu@intel.com>
Tested-by: Hongyan Song <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 256521509d20..0de18c76f8d4 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -628,7 +628,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl)
spin_lock_irqsave(&cl->dev->device_list_lock, flags);
list_for_each_entry(cl_device, &cl->dev->device_list,
device_link) {
- if (cl_device->fw_client->client_id == cl->fw_client_id) {
+ if (cl_device->fw_client &&
+ cl_device->fw_client->client_id == cl->fw_client_id) {
cl->device = cl_device;
rv = 0;
break;
@@ -688,6 +689,7 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev,
spin_lock_irqsave(&ishtp_dev->device_list_lock, flags);
list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list,
device_link) {
+ cl_device->fw_client = NULL;
if (warm_reset && cl_device->reference_count)
continue;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.9 72/87] HID: intel-ish: ipc: handle PIMR before ish_wakeup also clear PISR busy_clear bit
From: Sasha Levin @ 2019-03-27 18:20 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Song Hongyan, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190327182040.17444-1-sashal@kernel.org>
From: Song Hongyan <hongyan.song@intel.com>
[ Upstream commit 2edefc056e4f0e6ec9508dd1aca2c18fa320efef ]
Host driver should handle interrupt mask register earlier than wake up ish FW
else there will be conditions when FW interrupt comes, host PIMR register still
not set ready, so move the interrupt mask setting before ish_wakeup.
Clear PISR busy_clear bit in ish_irq_handler. If not clear, there will be
conditions host driver received a busy_clear interrupt (before the busy_clear
mask bit is ready), it will return IRQ_NONE after check_generated_interrupt,
the interrupt will never be cleared, causing the DEVICE not sending following
IRQ.
Since PISR clear should not be called for the CHV device we do this change.
After the change, both ISH2HOST interrupt and busy_clear interrupt will be
considered as interrupt from ISH, busy_clear interrupt will return IRQ_HANDLED
from IPC_IS_BUSY check.
Signed-off-by: Song Hongyan <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 0c9ac4d5d850..41d44536aa15 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -92,7 +92,10 @@ static bool check_generated_interrupt(struct ishtp_device *dev)
IPC_INT_FROM_ISH_TO_HOST_CHV_AB(pisr_val);
} else {
pisr_val = ish_reg_read(dev, IPC_REG_PISR_BXT);
- interrupt_generated = IPC_INT_FROM_ISH_TO_HOST_BXT(pisr_val);
+ interrupt_generated = !!pisr_val;
+ /* only busy-clear bit is RW, others are RO */
+ if (pisr_val)
+ ish_reg_write(dev, IPC_REG_PISR_BXT, pisr_val);
}
return interrupt_generated;
@@ -795,11 +798,11 @@ int ish_hw_start(struct ishtp_device *dev)
{
ish_set_host_rdy(dev);
+ set_host_ready(dev);
+
/* After that we can enable ISH DMA operation and wakeup ISHFW */
ish_wakeup(dev);
- set_host_ready(dev);
-
/* wait for FW-initiated reset flow */
if (!dev->recvd_hw_ready)
wait_event_interruptible_timeout(dev->wait_hw_ready,
--
2.19.1
^ permalink raw reply related
* Re: [PATCH v3 4/4] Input: add Apple SPI keyboard and trackpad driver.
From: Greg Kroah-Hartman @ 2019-03-27 18:45 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Ronald Tschalär, Dmitry Torokhov, Henrik Rydberg,
Sergey Senozhatsky, Steven Rostedt, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327093530.GH9224@smile.fi.intel.com>
On Wed, Mar 27, 2019 at 11:35:30AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2019 at 06:48:07PM -0700, Ronald Tschalär wrote:
> > The keyboard and trackpad on recent MacBook's (since 8,1) and
> > MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> > of USB, as previously. The higher level protocol is not publicly
> > documented and hence has been reverse engineered. As a consequence there
> > are still a number of unknown fields and commands. However, the known
> > parts have been working well and received extensive testing and use.
> >
> > In order for this driver to work, the proper SPI drivers need to be
> > loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
> > for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
> > reason enabling this driver in the config implies enabling the above
> > drivers.
>
> > +// SPDX-License-Identifier: GPL-2.0
>
> According to last changes this should be GPL-2.0-only
What "last changes"? "GPL-2.0" is a totally valid SPDX identifier for
the kernel. Don't buy into the "-only" prefix crud that the newer SPDX
version adopted for crazy reasons. The in-kernel documentation lists
the valid identifiers and the version of SPDX we are currently using.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v3 4/4] Input: add Apple SPI keyboard and trackpad driver.
From: Steven Rostedt @ 2019-03-27 19:15 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Andy Shevchenko, Ronald Tschalär, Dmitry Torokhov,
Henrik Rydberg, Sergey Senozhatsky, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327184526.GA11095@kroah.com>
On Wed, 27 Mar 2019 19:45:26 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > +// SPDX-License-Identifier: GPL-2.0
> >
> > According to last changes this should be GPL-2.0-only
>
> What "last changes"? "GPL-2.0" is a totally valid SPDX identifier for
> the kernel. Don't buy into the "-only" prefix crud that the newer SPDX
> version adopted for crazy reasons. The in-kernel documentation lists
> the valid identifiers and the version of SPDX we are currently using.
According to: LICENSES/preferred/GPL-2.0
> SPDX-URL: https://spdx.org/licenses/GPL-2.0.html
> Usage-Guide:
> To use this license in source code, put one of the following SPDX
> tag/value pairs into a comment according to the placement
> guidelines in the licensing rules documentation.
> For 'GNU General Public License (GPL) version 2 only' use:
> SPDX-License-Identifier: GPL-2.0
> or
> SPDX-License-Identifier: GPL-2.0-only
> For 'GNU General Public License (GPL) version 2 or any later version' use:
> SPDX-License-Identifier: GPL-2.0+
> or
> SPDX-License-Identifier: GPL-2.0-or-later
So, GPL-2.0 is the same as GPL-2.0-only. Changing it from one to the
other doesn't make any difference.
-- Steve
^ permalink raw reply
* Re: [PATCH v3 4/4] Input: add Apple SPI keyboard and trackpad driver.
From: Andy Shevchenko @ 2019-03-27 19:22 UTC (permalink / raw)
To: Greg Kroah-Hartman, Bartosz Golaszewski
Cc: Ronald Tschalär, Dmitry Torokhov, Henrik Rydberg,
Sergey Senozhatsky, Steven Rostedt, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327184526.GA11095@kroah.com>
On Wed, Mar 27, 2019 at 07:45:26PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Mar 27, 2019 at 11:35:30AM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 26, 2019 at 06:48:07PM -0700, Ronald Tschalär wrote:
> > > The keyboard and trackpad on recent MacBook's (since 8,1) and
> > > MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> > > of USB, as previously. The higher level protocol is not publicly
> > > documented and hence has been reverse engineered. As a consequence there
> > > are still a number of unknown fields and commands. However, the known
> > > parts have been working well and received extensive testing and use.
> > >
> > > In order for this driver to work, the proper SPI drivers need to be
> > > loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
> > > for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
> > > reason enabling this driver in the config implies enabling the above
> > > drivers.
> >
> > > +// SPDX-License-Identifier: GPL-2.0
> >
> > According to last changes this should be GPL-2.0-only
>
> What "last changes"? "GPL-2.0" is a totally valid SPDX identifier for
> the kernel. Don't buy into the "-only" prefix crud that the newer SPDX
> version adopted for crazy reasons. The in-kernel documentation lists
> the valid identifiers and the version of SPDX we are currently using.
Thanks for this clarification.
// offtopic
Bartosz, so, it seems my first mail has been a correct one: in-kernel
documentation clarifies things for kernel.
// offtopic
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: add GPIO controllable vibrator
From: Rob Herring @ 2019-03-27 19:43 UTC (permalink / raw)
To: Luca Weiss
Cc: Dmitry Torokhov, Mark Rutland,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
In-Reply-To: <20190302141132.21160-1-luca@z3ntu.xyz>
On Sat, Mar 02, 2019 at 03:11:30PM +0100, Luca Weiss wrote:
> Provide a simple driver for GPIO controllable vibrators.
> It will be used by the Fairphone 2.
>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> .../devicetree/bindings/input/gpio-vibrator.txt | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/gpio-vibrator.txt
>
> diff --git a/Documentation/devicetree/bindings/input/gpio-vibrator.txt b/Documentation/devicetree/bindings/input/gpio-vibrator.txt
> new file mode 100644
> index 000000000000..9e2e9acf497b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/gpio-vibrator.txt
> @@ -0,0 +1,16 @@
> +* GPIO vibrator device tree bindings
> +
> +Registers a GPIO device as vibrator, where the vibration motor just has the capability to turn on or off. If the device is connected to a pwm, you should use the pwm-vibrator driver instead.
Need to wrap line.
> +
> +Required properties:
> +- compatible: should contain "gpio-vibrator"
> +- enable-gpios: Should contain a GPIO handle
> +- vcc-supply: Phandle for the regulator supplying power
This should probably be optional. There may not be a s/w controllable
supply.
> +
> +Example from Fairphone 2:
> +
> +vibrator {
> + compatible = "gpio-vibrator";
> + enable-gpios = <&msmgpio 86 GPIO_ACTIVE_HIGH>;
> + vcc-supply = <&pm8941_l18>;
> +};
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH v3 1/4] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.
From: Life is hard, and then you die @ 2019-03-28 0:07 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Rafael J. Wysocki, Lukas Wunner, Federico Lorenzi, linux-input,
linux-kernel, Inki Dae, dri-devel@lists.freedesktop.org
In-Reply-To: <9fd8af5b-20ed-5dca-7d5c-98f2926b9b0c@samsung.com>
Hi Andrzej,
On Wed, Mar 27, 2019 at 03:13:37PM +0100, Andrzej Hajda wrote:
> +cc: dri-devel
>
> On 27.03.2019 02:48, Ronald Tschalär wrote:
> > commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> > sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> > INPUT. However, this causes problems with other drivers, in particular
> > an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> > future commit):
> >
> > drivers/clk/Kconfig:9:error: recursive dependency detected!
> > drivers/clk/Kconfig:9: symbol COMMON_CLK is selected by MFD_INTEL_LPSS
> > drivers/mfd/Kconfig:566: symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
> > drivers/mfd/Kconfig:580: symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
> > drivers/input/keyboard/Kconfig:73: symbol KEYBOARD_APPLESPI depends on INPUT
> > drivers/input/Kconfig:8: symbol INPUT is selected by DRM_SIL_SII8620
> > drivers/gpu/drm/bridge/Kconfig:83: symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
> > drivers/gpu/drm/bridge/Kconfig:1: symbol DRM_BRIDGE is selected by DRM_PL111
> > drivers/gpu/drm/pl111/Kconfig:1: symbol DRM_PL111 depends on COMMON_CLK
> >
> > According to the docs, select should only be used for non-visible
> > symbols. Furthermore almost all other references to INPUT throughout the
> > kernel config are depends, not selects. Hence this change.
> >
> > CC: Inki Dae <inki.dae@samsung.com>
> > CC: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
>
> This is drm bridge driver, next time please cc it to dri-devel ML also.
Ok. Though as noted in the cover letter, the patch here is meant as a
placeholder till the real thing being discussed on dri-devel is
finalized. I was trying to avoid cross-posting too much, hence the
separate submission on dri-devel.
> Anyway this is not the solution we have agreed to.
>
> Why have you abandoned the patch [1]? It needed just some minor
> polishing, as I wrote in response to this mail.
>
> [1]:
> https://lore.kernel.org/lkml/20190124082423.23139-1-ronald@innovation.ch/T/#mf620df0b1583096a214d8e2e690387078583472f
It seems your mail client doesn't like me :-) I got neither of your
responses. Sorry, I should've checked the archives when I didn't hear
anything. In any case thank you for your review, and I will update
that patch and send out a new version shortly.
Cheers,
Ronald
> > ---
> > drivers/gpu/drm/bridge/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 2fee47b0d50b..eabedc83f25c 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
> > config DRM_SIL_SII8620
> > tristate "Silicon Image SII8620 HDMI/MHL bridge"
> > depends on OF
> > + depends on INPUT
> > select DRM_KMS_HELPER
> > imply EXTCON
> > - select INPUT
> > select RC_CORE
> > help
> > Silicon Image SII8620 HDMI/MHL bridge chip driver.
>
>
^ permalink raw reply
* Re: [PATCH v3 4/4] Input: add Apple SPI keyboard and trackpad driver.
From: Life is hard, and then you die @ 2019-03-28 0:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Dmitry Torokhov, Henrik Rydberg, Sergey Senozhatsky,
Steven Rostedt, Greg Kroah-Hartman, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327093530.GH9224@smile.fi.intel.com>
On Wed, Mar 27, 2019 at 11:35:30AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2019 at 06:48:07PM -0700, Ronald Tschalär wrote:
> > The keyboard and trackpad on recent MacBook's (since 8,1) and
> > MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> > of USB, as previously. The higher level protocol is not publicly
> > documented and hence has been reverse engineered. As a consequence there
> > are still a number of unknown fields and commands. However, the known
> > parts have been working well and received extensive testing and use.
[snip]
> > +#include <asm/barrier.h>
>
> > +#include <asm-generic/unaligned.h>
>
> generic?!
>
> #include <asm/unaligned.h>
> should work.
Yes, you're right. I brought myself up-to-speed now on the difference
between the two.
> > +static const char *applespi_debug_facility(unsigned int log_mask)
> > +{
> > + switch (log_mask) {
> > + case DBG_CMD_TP_INI:
> > + return "Touchpad Initialization";
> > + case DBG_CMD_BL:
> > + return "Backlight Command";
> > + case DBG_CMD_CL:
> > + return "Caps-Lock Command";
> > + case DBG_RD_KEYB:
> > + return "Keyboard Event";
> > + case DBG_RD_TPAD:
> > + return "Touchpad Event";
> > + case DBG_RD_UNKN:
> > + return "Unknown Event";
> > + case DBG_RD_IRQ:
> > + return "Interrupt Request";
> > + case DBG_RD_CRC:
> > + return "Corrupted packet";
> > + case DBG_TP_DIM:
> > + return "Touchpad Dimensions";
> > + default:
>
> > + return "-Unrecognized log mask-";
>
> I don't think '-' surroundings are needed, but this is rather minor. Up to you.
I've used that to distinguish an error value from normal values; but
that's not an idiom used in the kernel AFAICT, so I'll remove them.
Thanks.
Cheers,
Ronald
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox