* Re: [PATCH] Input: goodix-berlin - Add sysfs interface for reading and writing touch IC registers
From: Hans de Goede @ 2024-05-07 12:38 UTC (permalink / raw)
To: Charles Wang, Dmitry Torokhov
Cc: hadess, Richard Hughes, linux-input, linux-kernel, neil.armstrong,
Mark Brown
In-Reply-To: <ZjocV1nvWnxr_qUI@mb-board>
Hi,
On 5/7/24 2:28 PM, Charles Wang wrote:
> Hi,
>
> On Tue, May 07, 2024 at 10:25:29AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 5/7/24 4:13 AM, Dmitry Torokhov wrote:
>>> On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 5/6/24 1:47 PM, Charles Wang wrote:
>>>>> Export a sysfs interface that would allow reading and writing touchscreen
>>>>> IC registers. With this interface many things can be done in usersapce
>>>>> such as firmware updates. An example tool that utilizes this interface
>>>>> for performing firmware updates can be found at [1].
>>>>
>>>> I'm not sure if I'm a fan of adding an interface to export raw register
>>>> access for fwupdate.
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/goodix_fwupload.c
>>>>
>>>> Contains update support for older goodix touchscreens and it is not
>>>> that big. I think it might be better to just have an in kernel fwupdate
>>>> driver for this and have a sysfs file to which to write the new firmware.
>>>>
>>>> Adding Richard Hughes, fwupd maintainer to the Cc. Richard, do you have
>>>> any input on the suggested method for firmware updating ?
>>>>
>>>> If raw register access is seen as a good solution, then I think this
>>>> should use regmap + some generic helpers (to be written) to export
>>>> regmap r/w access to userspace.
>>>
>>> I think the less code we have in kernel the better,
>>
>> Ok.
>>
>>> especially if in
>>> cases where firmware flashing is not essential for device to work (i.e.
>>> it the controller has a flash memory).
>>
>> Right the existing older goodix fw-upload is different because some
>> controllers are flash-less so they need a fw upload every boot.
>>
>>> That said IIRC Mark felt very
>>> strongly about allowing regmap writes from userspace... but maybe he
>>> softened the stance or we could have this functionality opt-in?
>>
>> Right when I was talking about generic helpers that was meant for
>> code re-use purposes. Actually exposing the regmap r/w functionality
>> to userspace is something which should be decided on a case by case
>> basis by the driver (IMHO).
>
> So what's the final conclusion, does the interface need to be modified?
I believe that the final conclusion is that the interface is fine.
Personally I think if the syfs store/show functions used for this
could be made into generic regmap helpers and then use those helpers
in the driver, so that if other drivers want similar functionality
they can re-use the show / store functions.
You should be able to use dev_get_regmap() to make the show/store
functions generic.
Regards,
Hans
>>>>> ---
>>>>> drivers/input/touchscreen/goodix_berlin.h | 2 +
>>>>> .../input/touchscreen/goodix_berlin_core.c | 52 +++++++++++++++++++
>>>>> drivers/input/touchscreen/goodix_berlin_i2c.c | 6 +++
>>>>> drivers/input/touchscreen/goodix_berlin_spi.c | 6 +++
>>>>> 4 files changed, 66 insertions(+)
>>>>>
>>>>> diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
>>>>> index 1fd77eb69..1741f2d15 100644
>>>>> --- a/drivers/input/touchscreen/goodix_berlin.h
>>>>> +++ b/drivers/input/touchscreen/goodix_berlin.h
>>>>> @@ -19,6 +19,8 @@ struct regmap;
>>>>> int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>>>>> struct regmap *regmap);
>>>>>
>>>>> +void goodix_berlin_remove(struct device *dev);
>>>>> +
>>>>> extern const struct dev_pm_ops goodix_berlin_pm_ops;
>>>>>
>>>>> #endif
>>>>> diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
>>>>> index e7b41a926..e02160841 100644
>>>>> --- a/drivers/input/touchscreen/goodix_berlin_core.c
>>>>> +++ b/drivers/input/touchscreen/goodix_berlin_core.c
>>>>> @@ -672,6 +672,44 @@ static void goodix_berlin_power_off_act(void *data)
>>>>> goodix_berlin_power_off(cd);
>>>>> }
>>>>>
>>>>> +static ssize_t goodix_berlin_registers_read(struct file *filp, struct kobject *kobj,
>>>>> + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count)
>>>>> +{
>>>>> + struct goodix_berlin_core *cd;
>>>>> + struct device *dev;
>>>>> + int error;
>>>>> +
>>>>> + dev = kobj_to_dev(kobj);
>>>>> + cd = dev_get_drvdata(dev);
>>>>> +
>>>>> + error = regmap_raw_read(cd->regmap, (unsigned int)off,
>>>>> + buf, count);
>>>>> +
>>>>> + return error ? error : count;
>>>>> +}
>>>>> +
>>>>> +static ssize_t goodix_berlin_registers_write(struct file *filp, struct kobject *kobj,
>>>>> + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count)
>>>>> +{
>>>>> + struct goodix_berlin_core *cd;
>>>>> + struct device *dev;
>>>>> + int error;
>>>>> +
>>>>> + dev = kobj_to_dev(kobj);
>>>>> + cd = dev_get_drvdata(dev);
>>>>> +
>>>>> + error = regmap_raw_write(cd->regmap, (unsigned int)off,
>>>>> + buf, count);
>>>>> +
>>>>> + return error ? error : count;
>>>>> +}
>>>>> +
>>>>> +static struct bin_attribute goodix_berlin_registers_attr = {
>>>>> + .attr = {.name = "registers", .mode = 0600},
>>>>> + .read = goodix_berlin_registers_read,
>>>>> + .write = goodix_berlin_registers_write,
>>>>> +};
>>>>> +
>>>>> int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>>>>> struct regmap *regmap)
>>>>> {
>>>>> @@ -743,6 +781,14 @@ int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>>>>>
>>>>> dev_set_drvdata(dev, cd);
>>>>>
>>>>> + error = sysfs_create_bin_file(&cd->dev->kobj,
>>>>> + &goodix_berlin_registers_attr);
>>>
>>> If we want to instantiate attributes from the driver please utilize
>>> dev_groups in respective driver structures to create and remove the
>>> attributes automatically.
>>>
>>> Thanks.
>>>
>>
>
^ permalink raw reply
* Re: [PATCH] HID: kye: Change Device Usage from Puck to Mouse
From: Jiri Kosina @ 2024-05-07 13:18 UTC (permalink / raw)
To: Milan Plžík
Cc: David Yang, linux-input, Benjamin Tissoires, linux-kernel
In-Reply-To: <CAN-R+gzMY6j=otYxDbCzYobn=FeM=4MLMrSr+scqUkHTb36qcA@mail.gmail.com>
On Tue, 7 May 2024, Milan Plžík wrote:
> no objections here. Unfortunately, I can't test the change at the moment
> (I barely remember there was a mouse that could be used with that
> tablet), but the change sounds good to me. Also, thanks a lot for
> keeping the HID drivers up-to-date even for such old hardware :)
Thanks for the prompt response. I've applied David's patch now.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 0/3] Fixes and updates to amd-sfh
From: Jiri Kosina @ 2024-05-07 13:19 UTC (permalink / raw)
To: Basavaraj Natikar; +Cc: benjamin.tissoires, linux-input, patreddy
In-Reply-To: <20240507071045.295723-1-Basavaraj.Natikar@amd.com>
On Tue, 7 May 2024, Basavaraj Natikar wrote:
> This patch series include changes for:
> - Modify and log errors based on functionality.
> - Handle "no sensors" exists in PM operations.
> - Use amd_get_c2p_val() to read the C2P register for compatibility.
>
> Basavaraj Natikar (3):
> HID: amd_sfh: Modify and log error only if case of functionality
> failures
> HID: amd_sfh: Handle "no sensors" in PM operations
> HID: amd_sfh: Use amd_get_c2p_val() to read C2P register
>
> drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 5 +----
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 17 ++++++++++++++---
> .../hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 2 +-
> 3 files changed, 16 insertions(+), 8 deletions(-)
Queued for 6.10, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property
From: Johan Hovold @ 2024-05-07 14:30 UTC (permalink / raw)
To: Linus Walleij
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Douglas Anderson, linux-input, devicetree,
linux-arm-msm, linux-kernel
In-Reply-To: <CACRpkdYZWixc1E3=Y2j0etuDS7vNY3QcqK2Qiac_KPorr7s0Ug@mail.gmail.com>
On Mon, May 06, 2024 at 08:29:40AM +0200, Linus Walleij wrote:
> + no-reset-on-power-off:
> + type: boolean
> + description:
> + Reset line is wired so that it can be left deasserted when the power
> + supply is off.
>
> To be nitpicky: *should* be left deasserted rather than *can* be left
> deasserted, right? If the behaviour is desirable but not strictly
> required.
I considered that too, but settled on the above description as it is
pure hardware description and leaving the decision to act on it up to
the OS (e.g. if support is implemented).
On the other hand, the "should" is already implied by the property name
so perhaps there's no reason not to include it also in the description:
+ no-reset-on-power-off:
+ type: boolean
+ description:
+ Reset line is wired so that it can (and should) be left
+ deasserted when the power supply is off.
And "should" (unlike "shall") still leaves room for an OS to ignore it
at the cost of increased power consumption.
Johan
^ permalink raw reply
* Re: [PATCH 2/6] input: himax_hx83112b: add regulator handling
From: Felix Kaechele @ 2024-05-07 14:33 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <95f8d63a-0343-49c3-90b6-f91efe559841@linaro.org>
Thanks, Krzysztof! Really appreciate your input, as I'm currently not a
regular contributor to the kernel.
On 2024-05-04 08:37, Krzysztof Kozlowski wrote:
> On 04/05/2024 04:04, Felix Kaechele wrote:
>> Handle regulators used on this chip family, namely AVDD and VDD. These
>> definitions are taken from the GPLv2 licensed vendor driver.
>>
>> Signed-off-by: Felix Kaechele <felix@kaechele.ca>
>> Link: https://github.com/HimaxSoftware/HX83112_Android_Driver
>> ---
>> drivers/input/touchscreen/himax_hx83112b.c | 48 ++++++++++++++++++++--
>> 1 file changed, 44 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
>> index 4f6609dcdef3..0a797789e548 100644
>> --- a/drivers/input/touchscreen/himax_hx83112b.c
>> +++ b/drivers/input/touchscreen/himax_hx83112b.c
>> @@ -19,10 +19,12 @@
>> #include <linux/interrupt.h>
>> #include <linux/kernel.h>
>> #include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>>
>> #define HIMAX_ID_83112B 0x83112b
>>
>> #define HIMAX_MAX_POINTS 10
>> +#define HIMAX_MAX_SUPPLIES 2
>>
>> #define HIMAX_REG_CFG_SET_ADDR 0x00
>> #define HIMAX_REG_CFG_INIT_READ 0x0c
>> @@ -50,6 +52,7 @@ struct himax_event {
>> static_assert(sizeof(struct himax_event) == 56);
>>
>> struct himax_ts_data {
>> + struct regulator_bulk_data supplies[HIMAX_MAX_SUPPLIES];
>> struct gpio_desc *gpiod_rst;
>> struct input_dev *input_dev;
>> struct i2c_client *client;
>> @@ -63,6 +66,11 @@ static const struct regmap_config himax_regmap_config = {
>> .val_format_endian = REGMAP_ENDIAN_LITTLE,
>> };
>>
>> +static const char *const himax_supply_names[] = {
>> + "avdd",
>> + "vdd",
>> +};
>> +
>
> That's confusing. Binding said only HX83100A family has regulators, but
> you request for everyone.
>
You're right. Looking at the vendor driver for each of models I could
see that it defined AVDD and VDD in both drivers. So I thought it could
make sense to offer it for the entire chip family, with which I meant
all HX831xx in this case.
But it seems after some more testing (and with this touch IC family
generally being a part of the display controller) the regulators are
sufficiently handled by the panel driver / bootloader for the use case
of having the touchscreen on when the display is on.
It wouldn't allow for waking the screen by using the touchscreen, and
while I'd have to go back to the original OS on the device to verify
this again, I don't remember that working on the original OS either.
So I'm thinking I may drop the whole regulator part of the patchset to
keep it smaller.
>> static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
>> {
>> int error;
>> @@ -267,7 +275,7 @@ static irqreturn_t himax_irq_handler(int irq, void *dev_id)
>>
>> static int himax_probe(struct i2c_client *client)
>> {
>> - int error;
>> + int error, i;
>> struct device *dev = &client->dev;
>> struct himax_ts_data *ts;
>>
>> @@ -290,11 +298,31 @@ static int himax_probe(struct i2c_client *client)
>> return error;
>> }
>>
>> + int num_supplies = ARRAY_SIZE(himax_supply_names);
>> +
>> + for (i = 0; i < num_supplies; i++)
>> + ts->supplies[i].supply = himax_supply_names[i];
>> +
>> + error = devm_regulator_bulk_get(dev,
>
> devm_regulator_bulk_get_enable and drop rest of the code here.
>
That's pretty neat. If I do decide to bring in regulator handling this
removes quite a bit of boilerplate.
>
>> + num_supplies,
>> + ts->supplies);
>
> Wrap it properly at 80, not one argument in one line.
>
>> + if (error) {
>> + dev_err(dev, "Failed to get supplies: %d\n", error);
>
> return dev_err_probe()
>
Same here. Thanks for the hint.
>> + return error;
>> + }
>> +
>> + error = regulator_bulk_enable(num_supplies,
>> + ts->supplies);
>> + if (error) {
>> + dev_err(dev, "Failed to enable supplies: %d\n", error);
>> + goto error_out;
>> + }
>> +
I'll be sending a v2 shortly.
Thanks again,
Felix
^ permalink raw reply
* Re: [PATCH] Input: goodix-berlin - Add sysfs interface for reading and writing touch IC registers
From: Mark Brown @ 2024-05-07 14:36 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Charles Wang, hadess, Richard Hughes, linux-input,
linux-kernel, neil.armstrong
In-Reply-To: <ZjmOUp725QTHrfcT@google.com>
[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]
On Mon, May 06, 2024 at 07:13:38PM -0700, Dmitry Torokhov wrote:
> On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote:
> > If raw register access is seen as a good solution, then I think this
> > should use regmap + some generic helpers (to be written) to export
> > regmap r/w access to userspace.
> I think the less code we have in kernel the better, especially if in
> cases where firmware flashing is not essential for device to work (i.e.
> it the controller has a flash memory). That said IIRC Mark felt very
> strongly about allowing regmap writes from userspace... but maybe he
> softened the stance or we could have this functionality opt-in?
I think unmediated raw register access is a terrible idea, you can't
safely write a driver if userspace can just go in and randomly write to
registers with no coordination with the running driver and for some
devices the kernel needs to ensure that any writes don't damage or
destabalise the system. If a driver provides an interface that looks
like raw register accesses that's of course fine (I mean, a lot of
firmware formats basically boil down to register write sequences which
is clearly fine) but it should be the driver doing that and it should be
looking at what's going on and ensure that it's joined up with the needs
of the rest of the system.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v2 6/7] arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset
From: Johan Hovold @ 2024-05-07 14:48 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Linus Walleij, Douglas Anderson, linux-input,
devicetree, linux-arm-msm, linux-kernel, Johan Hovold
In-Reply-To: <20240507144821.12275-1-johan+linaro@kernel.org>
The touch controller reset line is currently not described by the
devicetree except in the pin configuration which is used to deassert
reset.
As the reset line has an external pull up to an always-on rail there is
no need to drive the pin high so just leave it configured as an input
and disable the internal pull down.
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
index 41215567b3ae..372b35fb844f 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -977,8 +977,7 @@ int-n-pins {
reset-n-pins {
pins = "gpio99";
function = "gpio";
- output-high;
- drive-strength = <16>;
+ bias-disable;
};
};
--
2.43.2
^ permalink raw reply related
* [PATCH v2 2/7] dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M
From: Johan Hovold @ 2024-05-07 14:48 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Linus Walleij, Douglas Anderson, linux-input,
devicetree, linux-arm-msm, linux-kernel, Johan Hovold,
Krzysztof Kozlowski
In-Reply-To: <20240507144821.12275-1-johan+linaro@kernel.org>
Add a compatible string for the Elan eKTH5015M touch controller.
Judging from the current binding and commit bd3cba00dcc6 ("HID: i2c-hid:
elan: Add support for Elan eKTH6915 i2c-hid touchscreens"), eKTH5015M
appears to be compatible with eKTH6915. Notably the power-on sequence is
the same.
While at it, drop a redundant label from the example.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
.../devicetree/bindings/input/elan,ekth6915.yaml | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index c1fcf8c90c24..be84f7ed0abc 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -18,8 +18,12 @@ allOf:
properties:
compatible:
- enum:
- - elan,ekth6915
+ oneOf:
+ - items:
+ - enum:
+ - elan,ekth5015m
+ - const: elan,ekth6915
+ - const: elan,ekth6915
reg:
const: 0x10
@@ -57,8 +61,8 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
- ap_ts: touchscreen@10 {
- compatible = "elan,ekth6915";
+ touchscreen@10 {
+ compatible = "elan,ekth5015m", "elan,ekth6915";
reg = <0x10>;
interrupt-parent = <&tlmm>;
--
2.43.2
^ permalink raw reply related
* [PATCH v2 3/7] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property
From: Johan Hovold @ 2024-05-07 14:48 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Linus Walleij, Douglas Anderson, linux-input,
devicetree, linux-arm-msm, linux-kernel, Johan Hovold
In-Reply-To: <20240507144821.12275-1-johan+linaro@kernel.org>
When the power supply is shared with other peripherals the reset line
can be wired in such a way that it can remain deasserted regardless of
whether the supply is on or not.
This is important as it can be used to avoid holding the controller in
reset for extended periods of time when it remains powered, something
which can lead to increased power consumption. Leaving reset deasserted
also avoids leaking current through the reset circuitry pull-up
resistors.
Add a new 'no-reset-on-power-off' devicetree property which can be used
by the OS to determine when reset needs to be asserted on power down.
Note that this property can also be used when the supply cannot be
turned off by the OS at all.
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index be84f7ed0abc..a62916d07a08 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -36,6 +36,12 @@ properties:
reset-gpios:
description: Reset GPIO; not all touchscreens using eKTH6915 hook this up.
+ no-reset-on-power-off:
+ type: boolean
+ description:
+ Reset line is wired so that it can (and should) be left deasserted when
+ the power supply is off.
+
vcc33-supply:
description: The 3.3V supply to the touchscreen.
--
2.43.2
^ permalink raw reply related
* [PATCH v2 1/7] dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema
From: Johan Hovold @ 2024-05-07 14:48 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Linus Walleij, Douglas Anderson, linux-input,
devicetree, linux-arm-msm, linux-kernel, Johan Hovold,
Zhengqiao Xia, Krzysztof Kozlowski
In-Reply-To: <20240507144821.12275-1-johan+linaro@kernel.org>
The Ilitek ILI2901 touch screen controller was apparently incorrectly
added to the Elan eKTH6915 schema simply because it also has a reset
gpio and is currently managed by the Elan driver in Linux.
The two controllers are not related even if an unfortunate wording in
the commit message adding the Ilitek compatible made it sound like they
were.
Add a dedicated schema for the ILI2901 which does not specify the I2C
address (which is likely 0x41 rather than 0x10 as for other Ilitek touch
controllers) to avoid cluttering the Elan schema with unrelated devices
and to make it easier to find the correct schema when adding further
Ilitek controllers.
Fixes: d74ac6f60a7e ("dt-bindings: HID: i2c-hid: elan: Introduce Ilitek ili2901")
Cc: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
.../bindings/input/elan,ekth6915.yaml | 1 -
.../bindings/input/ilitek,ili2901.yaml | 66 +++++++++++++++++++
2 files changed, 66 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili2901.yaml
diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index dc4ac41f2441..c1fcf8c90c24 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -20,7 +20,6 @@ properties:
compatible:
enum:
- elan,ekth6915
- - ilitek,ili2901
reg:
const: 0x10
diff --git a/Documentation/devicetree/bindings/input/ilitek,ili2901.yaml b/Documentation/devicetree/bindings/input/ilitek,ili2901.yaml
new file mode 100644
index 000000000000..1abeec768d79
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/ilitek,ili2901.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/ilitek,ili2901.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ilitek ILI2901 touchscreen controller
+
+maintainers:
+ - Jiri Kosina <jkosina@suse.com>
+
+description:
+ Supports the Ilitek ILI2901 touchscreen controller.
+ This touchscreen controller uses the i2c-hid protocol with a reset GPIO.
+
+allOf:
+ - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+
+properties:
+ compatible:
+ enum:
+ - ilitek,ili2901
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ panel: true
+
+ reset-gpios:
+ maxItems: 1
+
+ vcc33-supply: true
+
+ vccio-supply: true
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - vcc33-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@41 {
+ compatible = "ilitek,ili2901";
+ reg = <0x41>;
+
+ interrupt-parent = <&tlmm>;
+ interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+
+ reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
+ vcc33-supply = <&pp3300_ts>;
+ };
+ };
--
2.43.2
^ permalink raw reply related
* [PATCH v2 5/7] arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
From: Johan Hovold @ 2024-05-07 14:48 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Linus Walleij, Douglas Anderson, linux-input,
devicetree, linux-arm-msm, linux-kernel, Johan Hovold, stable,
Steev Klimaszewski
In-Reply-To: <20240507144821.12275-1-johan+linaro@kernel.org>
The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay
before sending commands after having deasserted reset during power on.
Switch to the Elan specific binding so that the OS can determine the
required power-on sequence and make sure that the controller is always
detected during boot.
Note that the always-on 1.8 V supply (s10b) is not used by the
controller directly and should not be described.
Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
Cc: stable@vger.kernel.org # 6.0
Tested-by: Steev Klimaszewski <steev@kali.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
.../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index 569add4ebfab..98c1b75513be 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -653,15 +653,16 @@ &i2c4 {
status = "okay";
- /* FIXME: verify */
touchscreen@10 {
- compatible = "hid-over-i2c";
+ compatible = "elan,ekth5015m", "elan,ekth6915";
reg = <0x10>;
- hid-descr-addr = <0x1>;
interrupts-extended = <&tlmm 175 IRQ_TYPE_LEVEL_LOW>;
- vdd-supply = <&vreg_misc_3p3>;
- vddl-supply = <&vreg_s10b>;
+ reset-gpios = <&tlmm 99 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>;
+ no-reset-on-power-off;
+
+ vcc33-supply = <&vreg_misc_3p3>;
+ vccio-supply = <&vreg_misc_3p3>;
pinctrl-names = "default";
pinctrl-0 = <&ts0_default>;
@@ -1507,8 +1508,8 @@ int-n-pins {
reset-n-pins {
pins = "gpio99";
function = "gpio";
- output-high;
- drive-strength = <16>;
+ drive-strength = <2>;
+ bias-disable;
};
};
--
2.43.2
^ permalink raw reply related
* [PATCH v2 0/7] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
From: Johan Hovold @ 2024-05-07 14:48 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Linus Walleij, Douglas Anderson, linux-input,
devicetree, linux-arm-msm, linux-kernel, Johan Hovold
The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay
before sending commands after having deasserted reset during power on.
This series switches the X13s devicetree to use the Elan specific
binding so that the OS can determine the required power-on sequence and
make sure that the controller is always detected during boot. [1]
The Elan hid-i2c driver currently asserts reset unconditionally during
suspend, which does not work on the X13s where the touch controller
supply is shared with other peripherals that may remain powered. Holding
the controller in reset can increase power consumption and also leaks
current through the reset circuitry pull ups.
Note that the latter also affects X13s variants where the touchscreen is
not populated as the driver also exits probe() with reset asserted.
Fix this by adding a new 'no-reset-on-power-off' devicetree property
which can be used by the OS to determine when reset needs to be asserted
on power down and when it safe and desirable to leave it deasserted.
I tried to look for drivers that had already addressed this but it was
only after I finished implementing this that I noticed Doug's reference
to commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to
true state of the regulator"), which tried to solve a related problem.
That commit has since been reverted but ultimately resulted in commit
7607f12ba735 ("HID: i2c-hid: goodix: Add support for
"goodix,no-reset-during-suspend" property") being merged to handle the
related case where the touch controller supply is always on.
The implementation is very similar, but I decided to use the slightly
more generic 'no-reset-on-power-off' property name after considering a
number of alternatives (including trying to describe the hardware
configuration in the name). (And as this is not vendor specific, I left
out the prefix.)
Note that my X13s does not have a touchscreen, but I have done partial
verification of the implementation using that machine and the sc8280xp
CRD reference design. Bjorn has promised to help out with final
verification on an X13s with a touchscreen.
The devicetree changes are expected to go in through the Qualcomm tree
once the binding and driver updates have been merged.
Johan
[1] The reset signal is currently deasserted using the pin configuration
and the controller would be detected if probe is deferred or if user
space triggers a reprobe through sysfs.
Changes in v2
- drop redundant 'items' from binding
- include a "should" in description of 'no-reset-on-power-off' property
- always assert reset on probe
- enable elan i2c-hid driver in arm64 defconfig
Johan Hovold (7):
dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema
dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M
dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property
HID: i2c-hid: elan: fix reset suspend current leakage
arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset
arm64: defconfig: enable Elan i2c-hid driver
.../bindings/input/elan,ekth6915.yaml | 19 ++++--
.../bindings/input/ilitek,ili2901.yaml | 66 +++++++++++++++++++
arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 3 +-
.../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 15 +++--
arch/arm64/configs/defconfig | 1 +
drivers/hid/i2c-hid/i2c-hid-of-elan.c | 59 +++++++++++++----
6 files changed, 137 insertions(+), 26 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili2901.yaml
--
2.43.2
^ permalink raw reply
* [PATCH v2 4/7] HID: i2c-hid: elan: fix reset suspend current leakage
From: Johan Hovold @ 2024-05-07 14:48 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Linus Walleij, Douglas Anderson, linux-input,
devicetree, linux-arm-msm, linux-kernel, Johan Hovold, stable,
Steev Klimaszewski
In-Reply-To: <20240507144821.12275-1-johan+linaro@kernel.org>
The Elan eKTH5015M touch controller found on the Lenovo ThinkPad X13s
shares the VCC33 supply with other peripherals that may remain powered
during suspend (e.g. when enabled as wakeup sources).
The reset line is also wired so that it can be left deasserted when the
supply is off.
This is important as it avoids holding the controller in reset for
extended periods of time when it remains powered, which can lead to
increased power consumption, and also avoids leaking current through the
X13s reset circuitry during suspend (and after driver unbind).
Use the new 'no-reset-on-power-off' devicetree property to determine
when reset needs to be asserted on power down.
Notably this also avoids wasting power on machine variants without a
touchscreen for which the driver would otherwise exit probe with reset
asserted.
Fixes: bd3cba00dcc6 ("HID: i2c-hid: elan: Add support for Elan eKTH6915 i2c-hid touchscreens")
Cc: stable@vger.kernel.org # 6.0
Cc: Douglas Anderson <dianders@chromium.org>
Tested-by: Steev Klimaszewski <steev@kali.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
drivers/hid/i2c-hid/i2c-hid-of-elan.c | 59 +++++++++++++++++++++------
1 file changed, 47 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
index 5b91fb106cfc..091e37933225 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
@@ -31,6 +31,7 @@ struct i2c_hid_of_elan {
struct regulator *vcc33;
struct regulator *vccio;
struct gpio_desc *reset_gpio;
+ bool no_reset_on_power_off;
const struct elan_i2c_hid_chip_data *chip_data;
};
@@ -40,17 +41,17 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
container_of(ops, struct i2c_hid_of_elan, ops);
int ret;
+ gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
+
if (ihid_elan->vcc33) {
ret = regulator_enable(ihid_elan->vcc33);
if (ret)
- return ret;
+ goto err_deassert_reset;
}
ret = regulator_enable(ihid_elan->vccio);
- if (ret) {
- regulator_disable(ihid_elan->vcc33);
- return ret;
- }
+ if (ret)
+ goto err_disable_vcc33;
if (ihid_elan->chip_data->post_power_delay_ms)
msleep(ihid_elan->chip_data->post_power_delay_ms);
@@ -60,6 +61,15 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
return 0;
+
+err_disable_vcc33:
+ if (ihid_elan->vcc33)
+ regulator_disable(ihid_elan->vcc33);
+err_deassert_reset:
+ if (ihid_elan->no_reset_on_power_off)
+ gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0);
+
+ return ret;
}
static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
@@ -67,7 +77,14 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
struct i2c_hid_of_elan *ihid_elan =
container_of(ops, struct i2c_hid_of_elan, ops);
- gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
+ /*
+ * Do not assert reset when the hardware allows for it to remain
+ * deasserted regardless of the state of the (shared) power supply to
+ * avoid wasting power when the supply is left on.
+ */
+ if (!ihid_elan->no_reset_on_power_off)
+ gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
+
if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
@@ -79,6 +96,7 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
static int i2c_hid_of_elan_probe(struct i2c_client *client)
{
struct i2c_hid_of_elan *ihid_elan;
+ int ret;
ihid_elan = devm_kzalloc(&client->dev, sizeof(*ihid_elan), GFP_KERNEL);
if (!ihid_elan)
@@ -93,21 +111,38 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
if (IS_ERR(ihid_elan->reset_gpio))
return PTR_ERR(ihid_elan->reset_gpio);
+ ihid_elan->no_reset_on_power_off = of_property_read_bool(client->dev.of_node,
+ "no-reset-on-power-off");
+
ihid_elan->vccio = devm_regulator_get(&client->dev, "vccio");
- if (IS_ERR(ihid_elan->vccio))
- return PTR_ERR(ihid_elan->vccio);
+ if (IS_ERR(ihid_elan->vccio)) {
+ ret = PTR_ERR(ihid_elan->vccio);
+ goto err_deassert_reset;
+ }
ihid_elan->chip_data = device_get_match_data(&client->dev);
if (ihid_elan->chip_data->main_supply_name) {
ihid_elan->vcc33 = devm_regulator_get(&client->dev,
ihid_elan->chip_data->main_supply_name);
- if (IS_ERR(ihid_elan->vcc33))
- return PTR_ERR(ihid_elan->vcc33);
+ if (IS_ERR(ihid_elan->vcc33)) {
+ ret = PTR_ERR(ihid_elan->vcc33);
+ goto err_deassert_reset;
+ }
}
- return i2c_hid_core_probe(client, &ihid_elan->ops,
- ihid_elan->chip_data->hid_descriptor_address, 0);
+ ret = i2c_hid_core_probe(client, &ihid_elan->ops,
+ ihid_elan->chip_data->hid_descriptor_address, 0);
+ if (ret)
+ goto err_deassert_reset;
+
+ return 0;
+
+err_deassert_reset:
+ if (ihid_elan->no_reset_on_power_off)
+ gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0);
+
+ return ret;
}
static const struct elan_i2c_hid_chip_data elan_ekth6915_chip_data = {
--
2.43.2
^ permalink raw reply related
* [PATCH v2 7/7] arm64: defconfig: enable Elan i2c-hid driver
From: Johan Hovold @ 2024-05-07 14:48 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Linus Walleij, Douglas Anderson, linux-input,
devicetree, linux-arm-msm, linux-kernel, Johan Hovold
In-Reply-To: <20240507144821.12275-1-johan+linaro@kernel.org>
Enable the Elan i2c-hid driver which is needed for the touchscreen on
machines like the Lenovo ThinkPad X13s.
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index ac6fb3de1e3a..56fb9725d7c0 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1023,6 +1023,7 @@ CONFIG_SND_AUDIO_GRAPH_CARD2=m
CONFIG_HID_MULTITOUCH=m
CONFIG_I2C_HID_ACPI=m
CONFIG_I2C_HID_OF=m
+CONFIG_I2C_HID_OF_ELAN=m
CONFIG_USB=y
CONFIG_USB_OTG=y
CONFIG_USB_XHCI_HCD=y
--
2.43.2
^ permalink raw reply related
* Re: (subset) [PATCH v2 0/7] regulator: new API for voltage reference supplies
From: Mark Brown @ 2024-05-07 14:58 UTC (permalink / raw)
To: Liam Girdwood, Jean Delvare, Guenter Roeck, Jonathan Cameron,
Dmitry Torokhov, David Lechner
Cc: Jonathan Corbet, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, linux-doc, linux-kernel, linux-hwmon,
linux-iio, linux-staging, linux-input, Jonathan Cameron
In-Reply-To: <20240429-regulator-get-enable-get-votlage-v2-0-b1f11ab766c1@baylibre.com>
On Mon, 29 Apr 2024 18:40:08 -0500, David Lechner wrote:
> In the IIO subsystem, we noticed a pattern in many drivers where we need
> to get, enable and get the voltage of a supply that provides a reference
> voltage. In these cases, we only need the voltage and not a handle to
> the regulator. Another common pattern is for chips to have an internal
> reference voltage that is used when an external reference is not
> available. There are also a few drivers outside of IIO that do the same.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[4/7] iio: addac: ad74115: Use devm_regulator_get_enable_read_voltage()
commit: 41b94bc6d96b9b046ef08114f057dcc6c52e28b6
[5/7] iio: frequency: admv1013: Use devm_regulator_get_enable_read_voltage()
commit: 2f4bb1fa758abf4f5ee5a70ea7c2b1b8c8f7625d
[6/7] staging: iio: impedance-analyzer: ad5933: Use devm_regulator_get_enable_read_voltage()
commit: 9fcf6ef3e10b9fc605d84802058c0f30517bbaa7
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
^ permalink raw reply
* Re: [PATCH 00/18] HID: Include current HID-BPF fixes in tree
From: Benjamin Tissoires @ 2024-05-07 15:02 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Peter Hutterer, Benjamin Tissoires
Cc: linux-input, linux-kernel, linux-kselftest, Martin Sivak,
Ping Cheng, Jason Gerecke, Aaron Armstrong Skomra, Joshua Dickens
In-Reply-To: <20240410-bpf_sources-v1-0-a8bf16033ef8@kernel.org>
On Wed, 10 Apr 2024 19:19:20 +0200, Benjamin Tissoires wrote:
> When I introduced HID-BPF, I mentioned that we should ship the HID-BPF
> programs in the kernel when they are fixes so that everybody can benefit
> from them.
>
> I tried multiple times to do so but I was confronted to a tough problem:
> how can I make the kernel load them automatically?
>
> [...]
Applied to hid/hid.git (for-6.10/hid-bpf), thanks!
[01/18] HID: do not assume HAT Switch logical max < 8
https://git.kernel.org/hid/hid/c/65ad580a14e8
[02/18] HID: bpf: add first in-tree HID-BPF fix for the XPPen Artist 24
https://git.kernel.org/hid/hid/c/04b3e5ab0555
[03/18] HID: bpf: add in-tree HID-BPF fix for the XPPen Artist 16
https://git.kernel.org/hid/hid/c/e0599675a32c
[04/18] HID: bpf: add in-tree HID-BPF fix for the HP Elite Presenter Mouse
https://git.kernel.org/hid/hid/c/4e6d2a297dd5
[05/18] HID: bpf: add in-tree HID-BPF fix for the IOGear Kaliber Gaming MMOmentum mouse
https://git.kernel.org/hid/hid/c/0bc8f89f4040
[06/18] HID: bpf: add in-tree HID-BPF fix for the Wacom ArtPen
https://git.kernel.org/hid/hid/c/d9e78973921d
[07/18] HID: bpf: add in-tree HID-BPF fix for the XBox Elite 2 over Bluetooth
https://git.kernel.org/hid/hid/c/1c046d09c6ba
[08/18] HID: bpf: add in-tree HID-BPF fix for the Huion Kamvas Pro 19
https://git.kernel.org/hid/hid/c/9f1bf4c22532
[09/18] HID: bpf: add in-tree HID-BPF fix for the Raptor Mach 2
https://git.kernel.org/hid/hid/c/0cd1465cac52
[10/18] selftests/hid: import base_device.py from hid-tools
https://git.kernel.org/hid/hid/c/a7def2e51c66
[11/18] selftests/hid: add support for HID-BPF pre-loading before starting a test
https://git.kernel.org/hid/hid/c/e906463087ce
[12/18] selftests/hid: tablets: reduce the number of pen state
https://git.kernel.org/hid/hid/c/e14d88d9b8da
[13/18] selftests/hid: tablets: add a couple of XP-PEN tablets
https://git.kernel.org/hid/hid/c/03899011df4b
[14/18] selftests/hid: tablets: also check for XP-Pen offset correction
https://git.kernel.org/hid/hid/c/1b2c3caf7839
[15/18] selftests/hid: add Huion Kamvas Pro 19 tests
https://git.kernel.org/hid/hid/c/51de9ee0a6c7
[16/18] selftests/hid: import base_gamepad.py from hid-tools
https://git.kernel.org/hid/hid/c/c6b03c736a52
[17/18] selftests/hid: move the gamepads definitions in the test file
https://git.kernel.org/hid/hid/c/aa7e560454a9
[18/18] selftests/hid: add tests for the Raptor Mach 2 joystick
https://git.kernel.org/hid/hid/c/b22cbfb42c19
[19/19] selftests/hid: skip tests with HID-BPF if udev-hid-bpf is not installed
https://git.kernel.org/hid/hid/c/89ea968a9d75
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH] Input: goodix-berlin - Add sysfs interface for reading and writing touch IC registers
From: Jeff LaBundy @ 2024-05-07 15:14 UTC (permalink / raw)
To: Mark Brown
Cc: Dmitry Torokhov, Hans de Goede, Charles Wang, hadess,
Richard Hughes, linux-input, linux-kernel, neil.armstrong
In-Reply-To: <Zjo8eTQQS1LvzFgZ@finisterre.sirena.org.uk>
Hi all,
On Tue, May 07, 2024 at 11:36:41PM +0900, Mark Brown wrote:
> On Mon, May 06, 2024 at 07:13:38PM -0700, Dmitry Torokhov wrote:
> > On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote:
>
> > > If raw register access is seen as a good solution, then I think this
> > > should use regmap + some generic helpers (to be written) to export
> > > regmap r/w access to userspace.
>
> > I think the less code we have in kernel the better, especially if in
> > cases where firmware flashing is not essential for device to work (i.e.
> > it the controller has a flash memory). That said IIRC Mark felt very
> > strongly about allowing regmap writes from userspace... but maybe he
> > softened the stance or we could have this functionality opt-in?
>
> I think unmediated raw register access is a terrible idea, you can't
> safely write a driver if userspace can just go in and randomly write to
> registers with no coordination with the running driver and for some
> devices the kernel needs to ensure that any writes don't damage or
> destabalise the system. If a driver provides an interface that looks
> like raw register accesses that's of course fine (I mean, a lot of
> firmware formats basically boil down to register write sequences which
> is clearly fine) but it should be the driver doing that and it should be
> looking at what's going on and ensure that it's joined up with the needs
> of the rest of the system.
I happen to agree here; especially in the case of writing new FW to a
flash; this is a very hardware-centric and device-specific function,
which by definition belongs in a kernel driver.
For example, many devices must be placed in a bootloader mode during
the FW update, and may clamp or toggle an interrupt pin during this
mode switch. If user space initiates this sequence while the driver is
unaware of this process, it may attempt to read status registers from
an I2C address that is temporarily offline.
A much more common design pattern is for the driver to expose one W/O
sysfs attribute for accepting the FW file name, and one R/O attribute
for displaying the current FW version in flash. A small script runs at
start-up to check the version against what is stored on "disk", and if
what is stored in flash is deemed out of date, the script writes to the
W/O attribute. This is the extent of user space's involvement.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [syzbot] [input?] [usb?] WARNING in hid_output_report
From: Alan Stern @ 2024-05-07 17:35 UTC (permalink / raw)
To: syzbot; +Cc: bentiss, jikos, linux-input, linux-kernel, linux-usb,
syzkaller-bugs
In-Reply-To: <0000000000005e57b10617bc951f@google.com>
On Sun, May 05, 2024 at 03:36:27PM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 18daea77cca6 Merge tag 'for-linus' of git://git.kernel.org..
> git tree: upstream
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=159f1f17180000
> kernel config: https://syzkaller.appspot.com/x/.config?x=d2f00edef461175
> dashboard link: https://syzkaller.appspot.com/bug?extid=5186630949e3c55f0799
> compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13deb917180000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1035ae87180000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/4614372cf68b/disk-18daea77.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/e118db95ea43/vmlinux-18daea77.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/998cf091eeb5/bzImage-18daea77.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+5186630949e3c55f0799@syzkaller.appspotmail.com
>
> keytouch 0003:0926:3333.0001: implement() called with too large value 8 (n: 1)! (kworker/0:0)
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 7 at drivers/hid/hid-core.c:1451 implement drivers/hid/hid-core.c:1451 [inline]
> WARNING: CPU: 0 PID: 7 at drivers/hid/hid-core.c:1451 hid_output_report+0x548/0x760 drivers/hid/hid-core.c:1863
I suspect the WARN_ON(1) call in implement() should simply be removed.
It looks like the keytouch driver is trying to write 8 to a 1-bit field
in an output report. It probably doesn't verify the field sizes and
just assumes the device is reasonable.
Alan Stern
^ permalink raw reply
* Re: [PATCH v1 00/10] Define _GNU_SOURCE for sources using
From: Edward Liaw @ 2024-05-07 17:53 UTC (permalink / raw)
To: Muhammad Usama Anjum
Cc: Mark Brown, shuah, Jaroslav Kysela, Takashi Iwai, Jiri Kosina,
Benjamin Tissoires, Sean Christopherson, Paolo Bonzini,
Bongsu Jeon, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Jarkko Sakkinen, Dave Hansen,
Andrew Morton, linux-kernel, linux-kselftest, kernel-team,
linux-sound, linux-input, kvm, netdev, linux-rtc, linux-sgx
In-Reply-To: <be921714-b684-401e-a89a-8256df5fcb86@collabora.com>
On Tue, Apr 30, 2024 at 10:41 PM Muhammad Usama Anjum
<usama.anjum@collabora.com> wrote:
>
> Thanks for the fixes.
>
> On 5/1/24 6:59 AM, Mark Brown wrote:
> > On Tue, Apr 30, 2024 at 11:50:09PM +0000, Edward Liaw wrote:
> >> 809216233555 ("selftests/harness: remove use of LINE_MAX") introduced
> >> asprintf into kselftest_harness.h, which is a GNU extension and needs
> >> _GNU_SOURCE to either be defined prior to including headers or with the
> >> -D_GNU_SOURCE flag passed to the compiler.
> >
> > This seems like something that should be handled centrally rather than
> > having to go round and audit the users every time some update is made.
> The easiest way I could think of is to add -D_GNU_SOURCE to KHDR_HEADERS
> definition in tools/testing/selftests/Makefile. It wouldn't be obvious from
> KHDR_HEADERS name that there could be other flags in it as well though.
I'll try this approach and see. It looks like there are also some
Makefiles that don't currently include KHDR_INCLUDES.
Also, this will cause _GNU_SOURCE redefined warnings wherever #define
_GNU_SOURCE is present. Should I also delete them or wrap them with
#ifndef?
>
>
> --
> BR,
> Muhammad Usama Anjum
^ permalink raw reply
* Re: gamecube adapter driver
From: Carson @ 2024-05-07 18:04 UTC (permalink / raw)
To: Milas Robin; +Cc: dmitry.torokhov, linux-input, milas.robin
In-Reply-To: <PA4P189MB146957B28CBD2F35D6479C47FA1C2@PA4P189MB1469.EURP189.PROD.OUTLOOK.COM>
On Monday, May 6, 2024 3:32:20 PM EDT Milas Robin wrote:
> Hi,
>
> I don't think so, I didn't received any reply so I guess it is not ?
> If you have some time to review the code I would be grateful.
> As I said in the cover letter (which didn't linked correctly), this is
> my first kernel module so there might be some basic architecture notably
> on race conditions.
>
> Also due to the way I've implemented it, if it is build as a module
> it lose priority over the usb-hid generic driver as the device report
> supporting hid it.
>
> One other thing which might make the code heavier to read is properly
> detecting if the controller really support rumbles, I actually ignore the
> bit signalling it cause I don't know if the adapter could change it without
> unplugging the controller which might lead to have to recreate the device
> to switch it's rumbling capability
>
> The patch seems to have no conflict with the current input/next branch.
> If you need a branch I've uploaded it on https://github.com/Hinara/linux.git
> The branch is called ngc.
I've built the kernel but it won't boot, its likely a issue with my config but
could you rebase the patch to a newer kernel. Also I believe we are not
contacting the correct person for a merge.
https://github.com/torvalds/linux/blob/master/MAINTAINERS#L15595
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
From: Rob Herring @ 2024-05-07 18:48 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Felix Kaechele, Job Noorman, Dmitry Torokhov, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <3c2cf6f6-bf57-4fe6-9d79-5419addd6928@linaro.org>
On Sat, May 04, 2024 at 02:34:08PM +0200, Krzysztof Kozlowski wrote:
> On 04/05/2024 04:04, Felix Kaechele wrote:
> > This adds a compatible string for the Himax HX83100A touch controller
>
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> > including the AVDD and VDD supply nodes used by this chip family.
> >
> > Signed-off-by: Felix Kaechele <felix@kaechele.ca>
> > ---
> > .../bindings/input/touchscreen/himax,hx83112b.yaml | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > index f42b23d532eb..5809afedb9a2 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > @@ -15,6 +15,7 @@ allOf:
> > properties:
> > compatible:
> > enum:
> > + - himax,hx83100a
> > - himax,hx83112b
> >
> > reg:
> > @@ -26,6 +27,12 @@ properties:
> > reset-gpios:
> > maxItems: 1
> >
> > + avdd-supply:
> > + description: Analog power supply regulator
> > +
> > + vdd-supply:
> > + description: Digital power supply regulator
>
> These should not be allowed for other variant, so either you need
> allOf:if:then disallowing them (: false) or just create another binding
> file.
Or the commit message needs some explanation that the supplies also
apply to the 83112b as the existing binding has no supplies.
Rob
^ permalink raw reply
* Re: gamecube adapter driver
From: Milas Robin @ 2024-05-07 19:04 UTC (permalink / raw)
To: crange76; +Cc: dmitry.torokhov, linux-input, milas.robin
In-Reply-To: <2693037.vuYhMxLoTh@mythra>
> I've built the kernel but it won't boot, its likely a issue with my config but
> could you rebase the patch to a newer kernel.
This patch is based on next branch from the input repo:
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
The patch I submitted still compatible with their latest next branch.
Just to be sure I even updated my GitHub repository again to be based on the
latest commit from their branch, so I do not understand what do you mean by
"newer kernel" as it is up to date with the upstream branch on which it should
be merge.
Try to check in your config again ? I'm able to succesfully boot and test the
driver with a buildroot crafted image based on the input kernel tree and qemu.
If there is any issue related to boot with the driver it would be usefull at
least to get the kernel panic to debug it.
> Also I believe we are not
> contacting the correct person for a merge.
This kernel driver is not an HID driver but a USB driver, as such I don't think
the "NINTENDO HID DRIVER" maintainer is the appropriate to contact.
I might be mistaken but that also what the "get_maintainer.pl" script tells me.
^ permalink raw reply
* [dtor-input:next] BUILD SUCCESS 5128de84d8fc849400d00f7a6982711f129699ea
From: kernel test robot @ 2024-05-07 20:32 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: 5128de84d8fc849400d00f7a6982711f129699ea Input: cros_ec_keyb - remove an unused field in struct cros_ec_keyb
elapsed time: 1104m
configs tested: 164
configs skipped: 2
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allnoconfig gcc
alpha allyesconfig gcc
alpha defconfig gcc
arc allmodconfig gcc
arc allnoconfig gcc
arc allyesconfig gcc
arc defconfig gcc
arc randconfig-001-20240507 gcc
arc randconfig-002-20240507 gcc
arm allmodconfig gcc
arm allnoconfig clang
arm allyesconfig gcc
arm defconfig clang
arm randconfig-001-20240507 gcc
arm randconfig-002-20240507 clang
arm randconfig-003-20240507 gcc
arm randconfig-004-20240507 clang
arm64 allmodconfig clang
arm64 allnoconfig gcc
arm64 defconfig gcc
arm64 randconfig-001-20240507 clang
arm64 randconfig-002-20240507 clang
arm64 randconfig-003-20240507 clang
arm64 randconfig-004-20240507 clang
csky allmodconfig gcc
csky allnoconfig gcc
csky allyesconfig gcc
csky defconfig gcc
csky randconfig-001-20240507 gcc
csky randconfig-002-20240507 gcc
hexagon allmodconfig clang
hexagon allnoconfig clang
hexagon allyesconfig clang
hexagon defconfig clang
hexagon randconfig-001-20240507 clang
hexagon randconfig-002-20240507 clang
i386 allmodconfig gcc
i386 allnoconfig gcc
i386 allyesconfig gcc
i386 buildonly-randconfig-001-20240507 clang
i386 buildonly-randconfig-002-20240507 clang
i386 buildonly-randconfig-003-20240507 clang
i386 buildonly-randconfig-004-20240507 gcc
i386 buildonly-randconfig-005-20240507 gcc
i386 buildonly-randconfig-006-20240507 clang
i386 defconfig clang
i386 randconfig-001-20240507 clang
i386 randconfig-002-20240507 gcc
i386 randconfig-003-20240507 clang
i386 randconfig-004-20240507 clang
i386 randconfig-005-20240507 clang
i386 randconfig-006-20240507 clang
i386 randconfig-011-20240507 gcc
i386 randconfig-012-20240507 clang
i386 randconfig-013-20240507 clang
i386 randconfig-014-20240507 gcc
i386 randconfig-015-20240507 gcc
i386 randconfig-016-20240507 clang
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch defconfig gcc
loongarch randconfig-001-20240507 gcc
loongarch randconfig-002-20240507 gcc
m68k allmodconfig gcc
m68k allnoconfig gcc
m68k allyesconfig gcc
m68k defconfig gcc
microblaze allmodconfig gcc
microblaze allnoconfig gcc
microblaze allyesconfig gcc
microblaze defconfig gcc
mips allnoconfig gcc
mips allyesconfig gcc
nios2 allmodconfig gcc
nios2 allnoconfig gcc
nios2 allyesconfig gcc
nios2 defconfig gcc
nios2 randconfig-001-20240507 gcc
nios2 randconfig-002-20240507 gcc
openrisc allnoconfig gcc
openrisc allyesconfig gcc
openrisc defconfig gcc
parisc allmodconfig gcc
parisc allnoconfig gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc randconfig-001-20240507 gcc
parisc randconfig-002-20240507 gcc
parisc64 defconfig gcc
powerpc allmodconfig gcc
powerpc allnoconfig gcc
powerpc allyesconfig clang
powerpc randconfig-001-20240507 gcc
powerpc randconfig-002-20240507 clang
powerpc randconfig-003-20240507 clang
powerpc64 randconfig-001-20240507 gcc
powerpc64 randconfig-002-20240507 gcc
powerpc64 randconfig-003-20240507 clang
riscv allmodconfig clang
riscv allnoconfig gcc
riscv allyesconfig clang
riscv defconfig clang
riscv randconfig-001-20240507 clang
riscv randconfig-002-20240507 gcc
riscv rv32_defconfig clang
s390 allmodconfig clang
s390 allnoconfig clang
s390 allyesconfig gcc
s390 defconfig clang
s390 randconfig-001-20240507 gcc
s390 randconfig-002-20240507 clang
sh allmodconfig gcc
sh allnoconfig gcc
sh allyesconfig gcc
sh defconfig gcc
sh randconfig-001-20240507 gcc
sh randconfig-002-20240507 gcc
sparc allmodconfig gcc
sparc allnoconfig gcc
sparc defconfig gcc
sparc64 allmodconfig gcc
sparc64 allyesconfig gcc
sparc64 defconfig gcc
sparc64 randconfig-001-20240507 gcc
sparc64 randconfig-002-20240507 gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig gcc
um defconfig clang
um i386_defconfig gcc
um randconfig-001-20240507 gcc
um randconfig-002-20240507 clang
um x86_64_defconfig clang
x86_64 allnoconfig clang
x86_64 allyesconfig clang
x86_64 buildonly-randconfig-001-20240507 clang
x86_64 buildonly-randconfig-002-20240507 clang
x86_64 buildonly-randconfig-003-20240507 clang
x86_64 buildonly-randconfig-004-20240507 clang
x86_64 buildonly-randconfig-005-20240507 clang
x86_64 buildonly-randconfig-006-20240507 gcc
x86_64 defconfig gcc
x86_64 randconfig-001-20240507 gcc
x86_64 randconfig-002-20240507 clang
x86_64 randconfig-003-20240507 clang
x86_64 randconfig-004-20240507 gcc
x86_64 randconfig-005-20240507 gcc
x86_64 randconfig-006-20240507 gcc
x86_64 randconfig-011-20240507 gcc
x86_64 randconfig-012-20240507 clang
x86_64 randconfig-013-20240507 clang
x86_64 randconfig-014-20240507 clang
x86_64 randconfig-015-20240507 clang
x86_64 randconfig-016-20240507 clang
x86_64 randconfig-071-20240507 gcc
x86_64 randconfig-072-20240507 gcc
x86_64 randconfig-073-20240507 clang
x86_64 randconfig-074-20240507 clang
x86_64 randconfig-075-20240507 clang
x86_64 randconfig-076-20240507 clang
x86_64 rhel-8.3-rust clang
xtensa allnoconfig gcc
xtensa randconfig-001-20240507 gcc
xtensa randconfig-002-20240507 gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 1/1] Input: add gamecube adapter support
From: Dmitry Torokhov @ 2024-05-07 20:58 UTC (permalink / raw)
To: Milas Robin; +Cc: linux-input
In-Reply-To: <PA4P189MB14691B0AE72AC83713D8557CFA3B2@PA4P189MB1469.EURP189.PROD.OUTLOOK.COM>
Hi Milas,
On Thu, Mar 28, 2024 at 03:06:51AM +0100, Milas Robin wrote:
> Add support for the Wii U / Nintendo Switch gamecube controller adapter
Thank you for the driver, a few comments below.
>
> Signed-off-by: Milas Robin <milas.robin@live.fr>
> ---
> drivers/input/joystick/Kconfig | 20 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/gamecube-adapter.c | 607 ++++++++++++++++++++++
> 3 files changed, 628 insertions(+)
> create mode 100644 drivers/input/joystick/gamecube-adapter.c
>
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index 7755e5b454d2..18ab1f893ed0 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -422,4 +422,24 @@ config JOYSTICK_SEESAW
> To compile this driver as a module, choose M here: the module will be
> called adafruit-seesaw.
>
> +config JOYSTICK_NGC
> + tristate "Nintendo GameCube adapter support"
> + depends on USB_ARCH_HAS_HCD
> + select USB
> + help
> + Say Y here if you want to use Nintendo GameCube adapter with
> + your computer.
> + Make sure to say Y to "Joystick support" (CONFIG_INPUT_JOYDEV)
> + and/or "Event interface support" (CONFIG_INPUT_EVDEV) as well.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called gamecube_adapter.
> +
> +config JOYSTICK_NGC_FF
> + bool "Nintendo GameCube adapter rumble support"
> + depends on JOYSTICK_NGC && INPUT
> + select INPUT_FF_MEMLESS
> + help
> + Say Y here if you want to take advantage of GameCube controller rumble features.
> +
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 9976f596a920..db0f137ba57f 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
> +obj-$(CONFIG_JOYSTICK_NGC) += gamecube-adapter.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
> diff --git a/drivers/input/joystick/gamecube-adapter.c b/drivers/input/joystick/gamecube-adapter.c
> new file mode 100644
> index 000000000000..85d69f39d732
> --- /dev/null
> +++ b/drivers/input/joystick/gamecube-adapter.c
> @@ -0,0 +1,607 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2024 Milas Robin
> + *
> + * Based on the work of:
> + * Michael Lelli
> + * Dolphin Emulator project
> + */
> +
> +#include <linux/usb.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/input.h>
> +#include <linux/usb/input.h>
> +
> +/* Did not use usb-hid as it is not an hid driver */
> +#define USB_VENDOR_ID_NINTENDO 0x057e
> +#define USB_DEVICE_ID_NINTENDO_GCADAPTER 0x0337
> +
> +#define EP_IN 0x81
> +#define EP_OUT 0x02
> +
> +#define GCC_OUT_PKT_LEN 5
> +#define GCC_IN_PKT_LEN 37
> +
> +enum gamecube_status {
> + GAMECUBE_NONE,
> + GAMECUBE_WIRED = 0x10,
> + GAMECUBE_WIRELESS = 0x20,
> +};
> +
> +struct gcc_data {
> + struct ngc_data *adapter;
> + struct input_dev *input;
> + u8 no;
> + u8 status;
> + bool enable;
> +};
> +
> +struct ngc_data {
> + char phys[64];
> +
> + struct usb_device *udev;
> + struct usb_interface *intf;
> +
> + struct urb *irq_in;
> + u8 *idata;
> + dma_addr_t idata_dma;
> + spinlock_t idata_lock;
> +
> + struct urb *irq_out;
> + struct usb_anchor irq_out_anchor;
> + u8 *odata;
> + dma_addr_t odata_dma;
> + spinlock_t odata_lock; /* output data */
> + bool irq_out_active; /* we must not use an active URB */
I think all of this starting with irq_out should be under #ifdef
CONFIG_JOYSTICK_NGC_FF.
> +#ifdef CONFIG_JOYSTICK_NGC_FF
> + u8 odata_rumbles[4];
> + bool rumble_changed; /* if rumble need update*/
> +#endif
> +
> + struct gcc_data controllers[4];
Would be nice to have a #define for 4.
> + struct work_struct work; /* create/delete controller input files */
> +};
> +
> +#ifdef CONFIG_JOYSTICK_NGC_FF
> +/* Callers must hold gdata->odata_lock spinlock */
Then please use lockdep_assert_held().
> +static int ngc_queue_rumble(struct ngc_data *gdata)
> +{
> + int error;
> +
Please check irq_out_active flag here and also provide a stub for
!CONFIG_JOYSTICK_NGC_FF so you do not need to sprinkle #ifdef checks..
> + memcpy(gdata->odata + 1, gdata->odata_rumbles,
> + sizeof(gdata->odata_rumbles));
> + gdata->irq_out_active = true;
> + gdata->rumble_changed = false;
> + gdata->odata[0] = 0x11;
> + gdata->irq_out->transfer_buffer_length = 5;
> +
> + usb_anchor_urb(gdata->irq_out, &gdata->irq_out_anchor);
> + error = usb_submit_urb(gdata->irq_out, GFP_ATOMIC);
> + if (error) {
> + dev_err(&gdata->intf->dev,
> + "%s - usb_submit_urb failed with result %d\n",
> + __func__, error);
> + usb_unanchor_urb(gdata->irq_out);
> + error = -EIO;
> + }
> + return error;
> +}
> +
> +static int ngc_set_rumble_value(struct ngc_data *gdata, u8 controller, u8 value)
Since you want 0 or 1 for the value make it a "bool value".
> +{
> + unsigned long flags;
> + int error;
/
> +
> + value = !!value;
> + if (controller > 4)
> + return -EINVAL;
> +
> + spin_lock_irqsave(&gdata->odata_lock, flags);
Let's start using guard construct:
guard(spinlock_irqsave)(&gdata->odata_lock);
if (gdata->odata_rumbles[controller] == value)
return 0;
gdata->odata_rumbles[controller] = value;
gdata->rumble_changed = true;
return ngc_queue_rumble(gdata);
> + if (gdata->odata_rumbles[controller] == value) {
> + spin_unlock_irqrestore(&gdata->odata_lock, flags);
> + return 0;
> + }
> + gdata->odata_rumbles[controller] = value;
> + gdata->rumble_changed = true;
> + if (!gdata->irq_out_active)
> + error = ngc_queue_rumble(gdata);
> + spin_unlock_irqrestore(&gdata->odata_lock, flags);
> + return error;
> +}
> +
> +static int ngc_rumble_play(struct input_dev *dev, void *data,
> + struct ff_effect *eff)
> +{
> + struct gcc_data *gccdata = input_get_drvdata(dev);
> + u8 value;
> +
> + /*
> + * The gamecube controller supports only a single rumble motor so if any
> + * magnitude is set to non-zero then we start the rumble motor. If both are
> + * set to zero, we stop the rumble motor.
> + */
> +
> + if (eff->u.rumble.strong_magnitude || eff->u.rumble.weak_magnitude)
> + value = 1;
> + else
> + value = 0;
> + return ngc_set_rumble_value(gccdata->adapter, gccdata->no, value);
If value is a bool you can simply say
return ngc_set_rumble_value(gccdata->adapter, gccdata->no,
eff->u.rumble.strong_magnitude ||
eff->u.rumble.weak_magnitude);
> +}
> +static int ngc_init_ff(struct gcc_data *gccdev)
> +{
> + input_set_capability(gccdev->input, EV_FF, FF_RUMBLE);
> +
> + return input_ff_create_memless(gccdev->input, NULL, ngc_rumble_play);
> +}
> +#else
> +static int ngc_init_ff(struct gcc_data *gccdev) { return 0; }
> +#endif
> +
> +static void ngc_irq_out(struct urb *urb)
> +{
> + struct ngc_data *gdata = urb->context;
> + struct device *dev = &gdata->intf->dev;
> + int status = urb->status;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gdata->odata_lock, flags);
guard();
> +
> + switch (status) {
> + case 0:
> + /* success */
> +#ifdef CONFIG_JOYSTICK_NGC_FF
> + gdata->irq_out_active = gdata->rumble_changed;
> +#else
> + gdata->irq_out_active = false;
> +#endif
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + /* this urb is terminated, clean up */
> + dev_dbg(dev, "%s - urb shutting down with status: %d\n",
> + __func__, status);
> + gdata->irq_out_active = false;
Do you really want to send out URB again in this case?
> + break;
> +
> + default:
> + dev_dbg(dev, "%s - nonzero urb status received: %d\n",
> + __func__, status);
And here?
> + break;
> + }
> +#ifdef CONFIG_JOYSTICK_NGC_FF
> + if (gdata->irq_out_active)
> + ngc_queue_rumble(gdata);
> +#endif
> + spin_unlock_irqrestore(&gdata->odata_lock, flags);
> +}
> +
> +static int ngc_init_output(struct ngc_data *gdata,
> + struct usb_endpoint_descriptor *irq)
> +{
> + int error = -ENOMEM;
> +
> + init_usb_anchor(&gdata->irq_out_anchor);
> +
> + gdata->odata = usb_alloc_coherent(gdata->udev, GCC_OUT_PKT_LEN, GFP_KERNEL,
> + &gdata->odata_dma);
> + if (!gdata->odata)
> + return error;
> +
> + spin_lock_init(&gdata->odata_lock);
> +
> + gdata->irq_out = usb_alloc_urb(0, GFP_KERNEL);
> +
> + if (!gdata->irq_out)
> + goto err_free_coherent;
> +
> + usb_fill_int_urb(gdata->irq_out, gdata->udev,
> + usb_sndintpipe(gdata->udev, irq->bEndpointAddress),
> + gdata->odata, GCC_OUT_PKT_LEN, ngc_irq_out, gdata,
> + irq->bInterval);
> + gdata->irq_out->transfer_dma = gdata->odata_dma;
> + gdata->irq_out->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> + return 0;
> +
> +err_free_coherent:
> + usb_free_coherent(gdata->udev, GCC_OUT_PKT_LEN, gdata->odata,
> + gdata->odata_dma);
> + return error;
> +}
> +
> +static void ngc_deinit_output(struct ngc_data *gdata)
> +{
> + usb_free_urb(gdata->irq_out);
> + usb_free_coherent(gdata->udev, GCC_OUT_PKT_LEN, gdata->odata,
> + gdata->odata_dma);
> +}
> +
> +static void gcc_input(struct gcc_data *gccdata, const u8 *keys)
> +{
> + input_report_key(gccdata->input, BTN_A, !!(keys[1] & BIT(0)));
> + input_report_key(gccdata->input, BTN_B, !!(keys[1] & BIT(1)));
> + input_report_key(gccdata->input, BTN_X, !!(keys[1] & BIT(2)));
> + input_report_key(gccdata->input, BTN_Y, !!(keys[1] & BIT(3)));
> + input_report_key(gccdata->input, BTN_DPAD_LEFT, !!(keys[1] & BIT(4)));
> + input_report_key(gccdata->input, BTN_DPAD_RIGHT, !!(keys[1] & BIT(5)));
> + input_report_key(gccdata->input, BTN_DPAD_DOWN, !!(keys[1] & BIT(6)));
> + input_report_key(gccdata->input, BTN_DPAD_UP, !!(keys[1] & BIT(7)));
> +
> + input_report_key(gccdata->input, BTN_START, !!(keys[2] & BIT(0)));
> + input_report_key(gccdata->input, BTN_TR2, !!(keys[2] & BIT(1)));
> + input_report_key(gccdata->input, BTN_TR, !!(keys[2] & BIT(2)));
> + input_report_key(gccdata->input, BTN_TL, !!(keys[2] & BIT(3)));
Can we maybe have a list of mapping bits to events and loop over them.
You also do not need to normalize value for input_report_key(), it does
it for you.
> +
> + input_report_abs(gccdata->input, ABS_X, keys[3]);
> + input_report_abs(gccdata->input, ABS_Y, keys[4] ^ 0xFF);
> + input_report_abs(gccdata->input, ABS_RX, keys[5]);
> + input_report_abs(gccdata->input, ABS_RY, keys[6] ^ 0xFF);
> + input_report_abs(gccdata->input, ABS_Z, keys[7]);
> + input_report_abs(gccdata->input, ABS_RZ, keys[8]);
> +
> + input_sync(gccdata->input);
> +}
> +
> +
> +static u8 ngc_connected_type(u8 status)
> +{
> + u8 type = status & (GAMECUBE_WIRED | GAMECUBE_WIRELESS);
> +
> + switch (type) {
> + case GAMECUBE_WIRED:
> + case GAMECUBE_WIRELESS:
> + return type;
> + default:
> + return 0;
> + }
> +}
> +
> +static int ngc_controller_init(struct gcc_data *gccdev, u8 status)
> +{
> + int error;
> +
> + gccdev->input = input_allocate_device();
> + if (!gccdev->input)
> + return -ENOMEM;
> +
> + input_set_drvdata(gccdev->input, gccdev);
> + usb_to_input_id(gccdev->adapter->udev, &gccdev->input->id);
> + gccdev->input->name = "Nintendo GameCube Controller";
> + gccdev->input->phys = gccdev->adapter->phys;
> +
> + set_bit(EV_KEY, gccdev->input->evbit);
> +
> + set_bit(BTN_A, gccdev->input->keybit);
> + set_bit(BTN_B, gccdev->input->keybit);
> + set_bit(BTN_X, gccdev->input->keybit);
> + set_bit(BTN_Y, gccdev->input->keybit);
> + set_bit(BTN_DPAD_LEFT, gccdev->input->keybit);
> + set_bit(BTN_DPAD_RIGHT, gccdev->input->keybit);
> + set_bit(BTN_DPAD_DOWN, gccdev->input->keybit);
> + set_bit(BTN_DPAD_UP, gccdev->input->keybit);
> + set_bit(BTN_START, gccdev->input->keybit);
> + set_bit(BTN_TR2, gccdev->input->keybit);
> + set_bit(BTN_TR, gccdev->input->keybit);
> + set_bit(BTN_TL, gccdev->input->keybit);
> +
> + set_bit(EV_ABS, gccdev->input->evbit);
> +
> + set_bit(ABS_X, gccdev->input->absbit);
> + set_bit(ABS_Y, gccdev->input->absbit);
> + set_bit(ABS_RX, gccdev->input->absbit);
> + set_bit(ABS_RY, gccdev->input->absbit);
> + set_bit(ABS_Z, gccdev->input->absbit);
> + set_bit(ABS_RZ, gccdev->input->absbit);
These bits will be set by input_set_abs_params() below.
> +
> + input_set_abs_params(gccdev->input, ABS_X, 0, 255, 16, 16);
> + input_set_abs_params(gccdev->input, ABS_Y, 0, 255, 16, 16);
> + input_set_abs_params(gccdev->input, ABS_RX, 0, 255, 16, 16);
> + input_set_abs_params(gccdev->input, ABS_RY, 0, 255, 16, 16);
> + input_set_abs_params(gccdev->input, ABS_Z, 0, 255, 16, 0);
> + input_set_abs_params(gccdev->input, ABS_RZ, 0, 255, 16, 0);
> + error = ngc_init_ff(gccdev);
> + if (error) {
> + dev_warn(&gccdev->input->dev, "Could not create ff (skipped)");
> + goto ngc_deinit_controller;
> + }
> + error = input_register_device(gccdev->input);
> + if (error)
> + goto ngc_deinit_controller_ff;
> + gccdev->enable = true;
> + return 0;
> +ngc_deinit_controller_ff:
> + input_ff_destroy(gccdev->input);
> +ngc_deinit_controller:
> + input_free_device(gccdev->input);
> + return error;
> +}
> +
> +static void ngc_controller_update_work(struct work_struct *work)
> +{
> + int i;
> + u8 status[4];
> + bool enable[4];
> + unsigned long flags;
> + struct ngc_data *gdata = container_of(work, struct ngc_data, work);
> +
> + for (i = 0; i < 4; i++) {
> + status[i] = gdata->controllers[i].status;
> + enable[i] = ngc_connected_type(status[i]) != 0;
> + }
> +
> + for (i = 0; i < 4; i++) {
> + if (enable[i] && !gdata->controllers[i].enable) {
> + if (ngc_controller_init(&gdata->controllers[i], status[i]) != 0)
> + enable[i] = false;
> + }
> + }
> +
> + spin_lock_irqsave(&gdata->idata_lock, flags);
> + for (i = 0; i < 4; i++)
> + swap(gdata->controllers[i].enable, enable[i]);
> + spin_unlock_irqrestore(&gdata->idata_lock, flags);
> +
> + for (i = 0; i < 4; i++) {
> + if (enable[i] && !gdata->controllers[i].enable)
> + input_unregister_device(gdata->controllers[i].input);
> + }
> +}
> +
> +static void ngc_input(struct ngc_data *gdata)
> +{
> + int i;
> + unsigned long flags;
> + bool updated = false;
> +
> + for (i = 0; i < 4; i++) {
> + updated = updated ||
> + gdata->idata[1 + 9 * i] != gdata->controllers[i].status;
> + gdata->controllers[i].status = gdata->idata[1 + 9 * i];
> + }
> + if (updated)
> + schedule_work(&gdata->work);
> + spin_lock_irqsave(&gdata->idata_lock, flags);
> + for (i = 0; i < 4; i++) {
> + if (gdata->controllers[i].enable)
> + gcc_input(&gdata->controllers[i], &gdata->idata[1 + 9 * i]);
> + }
> + spin_unlock_irqrestore(&gdata->idata_lock, flags);
> +}
> +
> +static void ngc_irq_in(struct urb *urb)
> +{
> + struct ngc_data *gdata = urb->context;
> + struct usb_interface *intf = gdata->intf;
> + int error;
> +
> + switch (urb->status) {
> + case 0:
> + break;
> + case -EOVERFLOW:
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + dev_dbg(&intf->dev, "controller urb shutting down: %d\n",
> + urb->status);
> + return;
> + default:
> + dev_dbg(&intf->dev, "controller urb status: %d\n", urb->status);
> + goto exit;
> + }
> + if (gdata->irq_in->actual_length != GCC_IN_PKT_LEN)
> + dev_warn(&intf->dev, "Bad sized packet\n");
> + else if (gdata->idata[0] != 0x21)
> + dev_warn(&intf->dev, "Unknown opcode %d\n", gdata->idata[0]);
> + else
> + ngc_input(gdata);
> +exit:
> + error = usb_submit_urb(gdata->irq_in, GFP_ATOMIC);
> + if (error)
> + dev_err(&intf->dev, "controller urb failed: %d\n", error);
> +}
> +
> +static int ngc_init_input(struct ngc_data *gdata,
> + struct usb_endpoint_descriptor *irq)
> +{
> + int error = -ENOMEM;
> +
> + gdata->idata = usb_alloc_coherent(gdata->udev, GCC_IN_PKT_LEN, GFP_KERNEL,
> + &gdata->idata_dma);
> + if (!gdata->idata)
> + return error;
> +
> + gdata->irq_in = usb_alloc_urb(0, GFP_KERNEL);
> + if (!gdata->irq_in)
> + goto err_free_coherent;
> +
> + usb_fill_int_urb(gdata->irq_in, gdata->udev,
> + usb_rcvintpipe(gdata->udev, irq->bEndpointAddress),
> + gdata->idata, GCC_IN_PKT_LEN, ngc_irq_in, gdata,
> + irq->bInterval);
> + gdata->irq_in->transfer_dma = gdata->idata_dma;
> + gdata->irq_in->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> +
> + spin_lock_init(&gdata->idata_lock);
> + INIT_WORK(&gdata->work, ngc_controller_update_work);
> +
> + return 0;
> +
> +err_free_coherent:
> + usb_free_coherent(gdata->udev, GCC_IN_PKT_LEN, gdata->idata,
> + gdata->idata_dma);
> + return error;
> +
> +}
> +
> +
> +static void ngc_deinit_input(struct ngc_data *gdata)
> +{
> + usb_free_urb(gdata->irq_in);
> + usb_free_coherent(gdata->udev, GCC_IN_PKT_LEN, gdata->idata,
> + gdata->idata_dma);
> +}
> +
> +
> +static int ngc_init_irq(struct ngc_data *gdata)
> +{
> + struct usb_endpoint_descriptor *eps[] = { NULL, NULL };
> + int error;
> +
> + error = usb_find_common_endpoints(gdata->intf->cur_altsetting, NULL, NULL,
> + &eps[0], &eps[1]);
> + if (error)
> + return -ENODEV;
> + error = ngc_init_output(gdata, eps[1]);
> + if (error)
> + return error;
> + error = ngc_init_input(gdata, eps[0]);
> + if (error)
> + goto err_deinit_out;
> +#ifdef CONFIG_JOYSTICK_NGC_FF
> + memset(gdata->odata_rumbles, 0, 4);
> + gdata->rumble_changed = false;
Don't you allocate zeroed out memory?
> +#endif
> + gdata->irq_out_active = true;
> + gdata->odata[0] = 0x13;
> + gdata->irq_out->transfer_buffer_length = 1;
> +
> + error = usb_submit_urb(gdata->irq_in, GFP_KERNEL);
> + if (error)
> + goto err_deinit_in;
> +
> + usb_anchor_urb(gdata->irq_out, &gdata->irq_out_anchor);
> + error = usb_submit_urb(gdata->irq_out, GFP_ATOMIC);
> + if (error) {
> + dev_err(&gdata->intf->dev,
> + "%s - usb_submit_urb failed with result %d\n",
> + __func__, error);
> + usb_unanchor_urb(gdata->irq_out);
> + error = -EIO;
> + goto err_kill_in_urb;
> + }
> +
> + return 0;
> +err_kill_in_urb:
> + usb_kill_urb(gdata->irq_in);
> +err_deinit_in:
> + ngc_deinit_input(gdata);
> +err_deinit_out:
> + ngc_deinit_output(gdata);
> + return error;
> +}
> +
> +static void ngc_deinit_irq(struct ngc_data *gdata)
> +{
> + if (!usb_wait_anchor_empty_timeout(&gdata->irq_out_anchor, 5000)) {
> + dev_warn(&gdata->intf->dev,
> + "timed out waiting for output URB to complete, killing\n");
> + usb_kill_anchored_urbs(&gdata->irq_out_anchor);
Why can't we simply kill out URB and not dean with anchoring?
> + }
> + usb_kill_urb(gdata->irq_in);
> + /* Make sure we are done with presence work if it was scheduled */
> + flush_work(&gdata->work);
> +
> + ngc_deinit_input(gdata);
> + ngc_deinit_output(gdata);
> +}
> +
> +static void ngc_init_controllers(struct ngc_data *gdata)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(gdata->controllers); i++) {
> + gdata->controllers[i].adapter = gdata;
> + gdata->controllers[i].no = i;
> + gdata->controllers[i].status = GAMECUBE_NONE;
> + gdata->controllers[i].enable = false;
> + }
> +}
> +
> +static struct attribute *ngc_attrs[] = {
> + NULL,
> +};
> +
> +static const struct attribute_group ngc_attr_group = {
> + .attrs = ngc_attrs,
> +};
> +
> +static int ngc_init_attr(struct ngc_data *gdata)
> +{
> + return sysfs_create_group(&gdata->intf->dev.kobj, &ngc_attr_group);
> +}
> +
> +static void ngc_deinit_attr(struct ngc_data *gdata)
> +{
> + sysfs_remove_group(&gdata->intf->dev.kobj, &ngc_attr_group);
> +}
What is all this for?
> +
> +
> +static int ngc_usb_probe(struct usb_interface *iface, const struct usb_device_id *id)
> +{
> + struct usb_device *udev = interface_to_usbdev(iface);
> + struct ngc_data *gdata;
> + int error;
> +
> + gdata = kzalloc(sizeof(struct ngc_data), GFP_KERNEL);
> + if (!gdata)
> + return -ENOMEM;
> + usb_set_intfdata(iface, gdata);
> + gdata->udev = udev;
> + gdata->intf = iface;
> +
> + usb_make_path(udev, gdata->phys, sizeof(gdata->phys));
> + strlcat(gdata->phys, "/input0", sizeof(gdata->phys));
> +
> + ngc_init_controllers(gdata);
> + error = ngc_init_irq(gdata);
> + if (error)
> + goto err_free_devs;
> + error = ngc_init_attr(gdata);
> + if (error)
> + goto err_deinit_endpoints;
> + dev_info(&iface->dev, "New device registered\n");
> + return 0;
> +err_deinit_endpoints:
> + ngc_deinit_irq(gdata);
> +err_free_devs:
> + usb_set_intfdata(iface, NULL);
> + kfree(gdata);
> + return error;
> +}
> +
> +static void ngc_usb_disconnect(struct usb_interface *iface)
> +{
> + int i;
> + struct ngc_data *gdata = usb_get_intfdata(iface);
Make this first line.
> +
> + for (i = 0; i < 4; i++) {
> + if (gdata->controllers[i].enable)
> + input_unregister_device(gdata->controllers[i].input);
> + }
> + ngc_deinit_attr(gdata);
> + ngc_deinit_irq(gdata);
> + usb_set_intfdata(iface, NULL);
> + kfree(gdata);
> +}
> +
> +static const struct usb_device_id ngc_usb_devices[] = {
> + { USB_DEVICE(USB_VENDOR_ID_NINTENDO,
> + USB_DEVICE_ID_NINTENDO_GCADAPTER) },
> + {}
> +};
> +
Drop empty line.
> +MODULE_DEVICE_TABLE(usb, ngc_usb_devices);
> +
> +static struct usb_driver ngc_usb_driver = {
> + .name = "gamecube_adapter",
> + .id_table = ngc_usb_devices,
> + .probe = ngc_usb_probe,
> + .disconnect = ngc_usb_disconnect,
> +};
> +
> +module_usb_driver(ngc_usb_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Robin Milas <milas.robin@live.fr>");
> +MODULE_DESCRIPTION("Driver for GameCube adapter");
> --
> 2.44.0
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: goodix-berlin - Add sysfs interface for reading and writing touch IC registers
From: Dmitry Torokhov @ 2024-05-07 21:09 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Mark Brown, Hans de Goede, Charles Wang, hadess, Richard Hughes,
linux-input, linux-kernel, neil.armstrong
In-Reply-To: <ZjpFVGw6PgjRcZY3@nixie71>
On Tue, May 07, 2024 at 10:14:28AM -0500, Jeff LaBundy wrote:
> Hi all,
>
> On Tue, May 07, 2024 at 11:36:41PM +0900, Mark Brown wrote:
> > On Mon, May 06, 2024 at 07:13:38PM -0700, Dmitry Torokhov wrote:
> > > On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote:
> >
> > > > If raw register access is seen as a good solution, then I think this
> > > > should use regmap + some generic helpers (to be written) to export
> > > > regmap r/w access to userspace.
> >
> > > I think the less code we have in kernel the better, especially if in
> > > cases where firmware flashing is not essential for device to work (i.e.
> > > it the controller has a flash memory). That said IIRC Mark felt very
> > > strongly about allowing regmap writes from userspace... but maybe he
> > > softened the stance or we could have this functionality opt-in?
> >
> > I think unmediated raw register access is a terrible idea, you can't
> > safely write a driver if userspace can just go in and randomly write to
> > registers with no coordination with the running driver and for some
> > devices the kernel needs to ensure that any writes don't damage or
> > destabalise the system. If a driver provides an interface that looks
> > like raw register accesses that's of course fine (I mean, a lot of
> > firmware formats basically boil down to register write sequences which
> > is clearly fine) but it should be the driver doing that and it should be
> > looking at what's going on and ensure that it's joined up with the needs
> > of the rest of the system.
>
> I happen to agree here; especially in the case of writing new FW to a
> flash; this is a very hardware-centric and device-specific function,
> which by definition belongs in a kernel driver.
>
> For example, many devices must be placed in a bootloader mode during
> the FW update, and may clamp or toggle an interrupt pin during this
> mode switch. If user space initiates this sequence while the driver is
> unaware of this process, it may attempt to read status registers from
> an I2C address that is temporarily offline.
And yet we have i2c-dev and hidraw that are often successfully used to
flash the firmware, do diagnostics, etc. without encumbering the kernel.
They are more likely to work on ACPI systems because such systems have
separation between power management and function pieces (whereas on
non-ACPI systems power management is crammed into the same driver and it
is not possible to properly power up device without wiring up the rest
of it). This is something that I feel we will have to fix in the long
term.
>
> A much more common design pattern is for the driver to expose one W/O
> sysfs attribute for accepting the FW file name, and one R/O attribute
> for displaying the current FW version in flash. A small script runs at
> start-up to check the version against what is stored on "disk", and if
> what is stored in flash is deemed out of date, the script writes to the
> W/O attribute. This is the extent of user space's involvement.
This however means that code that is not used 99.9999 percent of the
time has to stay in the kernel, occupying precious memory. I agree
that if firmware update is very involved and needs precise control of
interrupts coming from the device and has certain timing restriction,
then in-kernel implementation is preferable, but in many instance
userspace updaters work just fine.
Thanks.
--
Dmitry
^ 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