* Re: [PATCH v3 26/42] ata: pata_ep93xx: add device tree support
From: Sergey Shtylyov @ 2023-07-20 9:45 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Dmitry Torokhov,
Arnd Bergmann, Olof Johansson, soc, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, Andy Shevchenko, Michael Peters,
Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-26-3d63a5f1103e@maquefel.me>
On 7/20/23 2:29 PM, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> - Add OF ID match table
> - Drop ep93xx_chip_revision and use soc_device_match instead
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> drivers/ata/pata_ep93xx.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index c6e043e05d43..a88824dfc5fa 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
[...]
> @@ -910,6 +912,12 @@ static struct ata_port_operations ep93xx_pata_port_ops = {
> .port_start = ep93xx_pata_port_start,
> };
>
> +static const struct soc_device_attribute ep93xx_soc_table[] = {
> + { .revision = "E1", .data = (void *)ATA_UDMA3 },
> + { .revision = "E2", .data = (void *)ATA_UDMA4 },
> + { /* sentinel */ }
> +};
> +
> static int ep93xx_pata_probe(struct platform_device *pdev)
> {
> struct ep93xx_pata_data *drv_data;
> @@ -939,7 +947,7 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
>
> drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
> if (!drv_data) {
> - err = -ENXIO;
> + err = -ENOMEM;
> goto err_rel_gpio;
> }
>
Hm, deserves its own patch. And even for this one, you should've documented it
in the patch secription...
> @@ -976,12 +984,11 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
> * so this driver supports only UDMA modes.
> */
> if (drv_data->dma_rx_channel && drv_data->dma_tx_channel) {
> - int chip_rev = ep93xx_chip_revision();
> + const struct soc_device_attribute *match;
>
> - if (chip_rev == EP93XX_CHIP_REV_E1)
> - ap->udma_mask = ATA_UDMA3;
> - else if (chip_rev == EP93XX_CHIP_REV_E2)
> - ap->udma_mask = ATA_UDMA4;
> + match = soc_device_match(ep93xx_soc_table);
> + if (match)
> + ap->udma_mask = (unsigned int) match->data;
> else
> ap->udma_mask = ATA_UDMA2;
> }
This one also looks as it could have been done separately -- before the DT
conversion?
[...]
MBR, Sergey
^ permalink raw reply
* [PATCH] dt-bindings: input: convert syna,rmi4 to DT schema
From: Krzysztof Kozlowski @ 2023-07-20 11:00 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jason A. Donenfeld, Matthias Schiffer, Vincent Huang, linux-input,
devicetree, linux-kernel
Cc: Krzysztof Kozlowski
Convert the bindings for Synaptics RMI4 bus and devices to DT schema.
Changes during conversion:
1. Add reset-gpios already used in DTS and mentioned by RMI4
specification.
2. Do not require address/size cells, because without functions
(children) they aren't really needed.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Jason, Matthias, Vincent,
I put your names as maintainers, because moderately recently you were
changing the driver. Let me know if this is okay or you prefer not to
maintain the hardware.
---
.../bindings/input/rmi4/rmi_2d_sensor.txt | 56 ----
.../bindings/input/rmi4/rmi_f01.txt | 39 ---
.../bindings/input/rmi4/rmi_i2c.txt | 61 ----
.../bindings/input/rmi4/rmi_spi.txt | 56 ----
.../devicetree/bindings/input/syna,rmi4.yaml | 271 ++++++++++++++++++
5 files changed, 271 insertions(+), 212 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_2d_sensor.txt
delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_f01.txt
delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt
create mode 100644 Documentation/devicetree/bindings/input/syna,rmi4.yaml
diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_2d_sensor.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_2d_sensor.txt
deleted file mode 100644
index 9afffbdf6e28..000000000000
--- a/Documentation/devicetree/bindings/input/rmi4/rmi_2d_sensor.txt
+++ /dev/null
@@ -1,56 +0,0 @@
-Synaptics RMI4 2D Sensor Device Binding
-
-The Synaptics RMI4 core is able to support RMI4 devices using different
-transports and different functions. This file describes the device tree
-bindings for devices which contain 2D sensors using Function 11 or
-Function 12. Complete documentation for transports and other functions
-can be found in:
-Documentation/devicetree/bindings/input/rmi4.
-
-RMI4 Function 11 and Function 12 are for 2D touch position sensing.
-Additional documentation for F11 can be found at:
-http://www.synaptics.com/sites/default/files/511-000136-01-Rev-E-RMI4-Interfacing-Guide.pdf
-
-Optional Touch Properties:
-Description in Documentation/devicetree/bindings/input/touchscreen
-- touchscreen-inverted-x
-- touchscreen-inverted-y
-- touchscreen-swapped-x-y
-- touchscreen-x-mm
-- touchscreen-y-mm
-
-Optional Properties:
-- syna,clip-x-low: Sets a minimum value for X.
-- syna,clip-y-low: Sets a minimum value for Y.
-- syna,clip-x-high: Sets a maximum value for X.
-- syna,clip-y-high: Sets a maximum value for Y.
-- syna,offset-x: Add an offset to X.
-- syna,offset-y: Add an offset to Y.
-- syna,delta-x-threshold: Set the minimum distance on the X axis required
- to generate an interrupt in reduced reporting
- mode.
-- syna,delta-y-threshold: Set the minimum distance on the Y axis required
- to generate an interrupt in reduced reporting
- mode.
-- syna,sensor-type: Set the sensor type. 1 for touchscreen 2 for touchpad.
-- syna,disable-report-mask: Mask for disabling posiiton reporting. Used to
- disable reporing absolute position data.
-- syna,rezero-wait-ms: Time in miliseconds to wait after issuing a rezero
- command.
-
-
-Example of a RMI4 I2C device with F11:
-Example:
- &i2c1 {
- rmi4-i2c-dev@2c {
- compatible = "syna,rmi4-i2c";
-
- ...
-
- rmi4-f11@11 {
- reg = <0x11>;
- touchscreen-inverted-y;
- syna,sensor-type = <2>;
- };
- };
- };
diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_f01.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_f01.txt
deleted file mode 100644
index 079cad2b6843..000000000000
--- a/Documentation/devicetree/bindings/input/rmi4/rmi_f01.txt
+++ /dev/null
@@ -1,39 +0,0 @@
-Synaptics RMI4 F01 Device Binding
-
-The Synaptics RMI4 core is able to support RMI4 devices using different
-transports and different functions. This file describes the device tree
-bindings for devices which contain Function 1. Complete documentation
-for transports and other functions can be found in:
-Documentation/devicetree/bindings/input/rmi4.
-
-Additional documentation for F01 can be found at:
-http://www.synaptics.com/sites/default/files/511-000136-01-Rev-E-RMI4-Interfacing-Guide.pdf
-
-Optional Properties:
-- syna,nosleep-mode: If set the device will run at full power without sleeping.
- nosleep has 3 modes, 0 will not change the default
- setting, 1 will disable nosleep (allow sleeping),
- and 2 will enable nosleep (disabling sleep).
-- syna,wakeup-threshold: Defines the amplitude of the disturbance to the
- background capacitance that will cause the
- device to wake from dozing.
-- syna,doze-holdoff-ms: The delay to wait after the last finger lift and the
- first doze cycle.
-- syna,doze-interval-ms: The time period that the device sleeps between finger
- activity.
-
-
-Example of a RMI4 I2C device with F01:
- Example:
- &i2c1 {
- rmi4-i2c-dev@2c {
- compatible = "syna,rmi4-i2c";
-
- ...
-
- rmi4-f01@1 {
- reg = <0x1>;
- syna,nosleep-mode = <1>;
- };
- };
- };
diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
deleted file mode 100644
index dcb012f5b3ee..000000000000
--- a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
+++ /dev/null
@@ -1,61 +0,0 @@
-Synaptics RMI4 I2C Device Binding
-
-The Synaptics RMI4 core is able to support RMI4 devices using different
-transports and different functions. This file describes the device tree
-bindings for devices using the I2C transport driver. Complete documentation
-for other transports and functions can be found in
-Documentation/devicetree/bindings/input/rmi4.
-
-Required Properties:
-- compatible: syna,rmi4-i2c
-- reg: I2C address
-- #address-cells: Set to 1 to indicate that the function child nodes
- consist of only on uint32 value.
-- #size-cells: Set to 0 to indicate that the function child nodes do not
- have a size property.
-
-Optional Properties:
-- interrupts: interrupt which the rmi device is connected to.
-See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
-
-- syna,reset-delay-ms: The number of milliseconds to wait after resetting the
- device.
-
-- syna,startup-delay-ms: The number of milliseconds to wait after powering on
- the device.
-
-- vdd-supply: VDD power supply.
-See ../regulator/regulator.txt
-
-- vio-supply: VIO power supply
-See ../regulator/regulator.txt
-
-Function Parameters:
-Parameters specific to RMI functions are contained in child nodes of the rmi device
- node. Documentation for the parameters of each function can be found in:
-Documentation/devicetree/bindings/input/rmi4/rmi_f*.txt.
-
-
-
-Example:
- &i2c1 {
- rmi4-i2c-dev@2c {
- compatible = "syna,rmi4-i2c";
- reg = <0x2c>;
- #address-cells = <1>;
- #size-cells = <0>;
- interrupt-parent = <&gpio>;
- interrupts = <4 2>;
-
- rmi4-f01@1 {
- reg = <0x1>;
- syna,nosleep-mode = <1>;
- };
-
- rmi4-f11@11 {
- reg = <0x11>;
- touchscreen-inverted-y;
- syna,sensor-type = <2>;
- };
- };
- };
diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt
deleted file mode 100644
index 632f473db65b..000000000000
--- a/Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt
+++ /dev/null
@@ -1,56 +0,0 @@
-Synaptics RMI4 SPI Device Binding
-
-The Synaptics RMI4 core is able to support RMI4 devices using different
-transports and different functions. This file describes the device tree
-bindings for devices using the SPI transport driver. Complete documentation
-for other transports and functions can be found in
-Documentation/devicetree/bindings/input/rmi4.
-
-Required Properties:
-- compatible: syna,rmi4-spi
-- reg: Chip select address for the device
-- #address-cells: Set to 1 to indicate that the function child nodes
- consist of only on uint32 value.
-- #size-cells: Set to 0 to indicate that the function child nodes do not
- have a size property.
-
-Optional Properties:
-- interrupts: interrupt which the rmi device is connected to.
-See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
-
-- spi-rx-delay-us: microsecond delay after a read transfer.
-- spi-tx-delay-us: microsecond delay after a write transfer.
-
-Function Parameters:
-Parameters specific to RMI functions are contained in child nodes of the rmi device
- node. Documentation for the parameters of each function can be found in:
-Documentation/devicetree/bindings/input/rmi4/rmi_f*.txt.
-
-
-
-Example:
- spi@7000d800 {
- rmi4-spi-dev@0 {
- compatible = "syna,rmi4-spi";
- reg = <0x0>;
- #address-cells = <1>;
- #size-cells = <0>;
- spi-max-frequency = <4000000>;
- spi-cpha;
- spi-cpol;
- interrupt-parent = <&gpio>;
- interrupts = <TEGRA_GPIO(K, 2) 0x2>;
- spi-rx-delay-us = <30>;
-
- rmi4-f01@1 {
- reg = <0x1>;
- syna,nosleep-mode = <1>;
- };
-
- rmi4-f11@11 {
- reg = <0x11>;
- touchscreen-inverted-y;
- syna,sensor-type = <2>;
- };
- };
- };
diff --git a/Documentation/devicetree/bindings/input/syna,rmi4.yaml b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
new file mode 100644
index 000000000000..286b4d52cea9
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
@@ -0,0 +1,271 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/syna,rmi4.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synaptics RMI4 compliant devices
+
+maintainers:
+ - Jason A. Donenfeld <Jason@zx2c4.com>
+ - Matthias Schiffer <matthias.schiffer@ew.tq-group.com
+ - Vincent Huang <vincent.huang@tw.synaptics.com>
+
+description: |
+ The Synaptics RMI4 (Register Mapped Interface 4) core is able to support RMI4
+ devices using different transports (I2C, SPI) and different functions (e.g.
+ Function 1, 2D sensors using Function 11 or 12).
+
+properties:
+ compatible:
+ enum:
+ - syna,rmi4-i2c
+ - syna,rmi4-spi
+
+ reg:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+ description: Active low signal
+
+ spi-cpha: true
+ spi-cpol: true
+
+ syna,reset-delay-ms:
+ description:
+ Delay to wait after resetting the device.
+
+ syna,startup-delay-ms:
+ description:
+ Delay to wait after powering on the device.
+
+ vdd-supply: true
+ vio-supply: true
+
+ rmi4-f01@1:
+ type: object
+ additionalProperties: false
+ description:
+ Function 1
+
+ properties:
+ reg:
+ maxItems: 1
+
+ syna,nosleep-mode:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1, 2]
+ description:
+ If set the device will run at full power without sleeping. nosleep
+ has 3 modes, 0 will not change the default setting, 1 will disable
+ nosleep (allow sleeping), and 2 will enable nosleep (disabling
+ sleep).
+
+ syna,wakeup-threshold:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Defines the amplitude of the disturbance to the background
+ capacitance that will cause the device to wake from dozing.
+
+ syna,doze-holdoff-ms:
+ description:
+ The delay to wait after the last finger lift and the first doze
+ cycle.
+
+ syna,doze-interval-ms:
+ description:
+ The time period that the device sleeps between finger activity.
+
+ required:
+ - reg
+
+patternProperties:
+ "^rmi4-f1[12]@1[12]$":
+ type: object
+ unevaluatedProperties: false
+ $ref: /schemas/input/touchscreen/touchscreen.yaml#
+ description:
+ RMI4 Function 11 and Function 12 are for 2D touch position sensing.
+
+ properties:
+ reg:
+ maxItems: 1
+
+ syna,clip-x-low:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Minimum value for X.
+
+ syna,clip-y-low:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Minimum value for Y.
+
+ syna,clip-x-high:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Maximum value for X.
+
+ syna,clip-y-high:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Maximum value for Y.
+
+ syna,offset-x:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Add an offset to X.
+
+ syna,offset-y:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Add an offset to Y.
+
+ syna,delta-x-threshold:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Minimum distance on the X axis required to generate an interrupt in
+ reduced reporting mode.
+
+ syna,delta-y-threshold:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Minimum distance on the Y axis required to generate an interrupt in
+ reduced reporting mode.
+
+ syna,sensor-type:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [1, 2]
+ description: |
+ Sensor type: 1 for touchscreen 2 for touchpad.
+
+ syna,disable-report-mask:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Mask for disabling posiiton reporting. Used to disable reporing
+ absolute position data.
+
+ syna,rezero-wait-ms:
+ description:
+ Time to wait after issuing a rezero command.
+
+ required:
+ - reg
+
+ "^rmi4-f[0-9a-z]+@[0-9a-z]+$":
+ type: object
+ description:
+ Other functions, not documented yet.
+
+ properties:
+ reg:
+ maxItems: 1
+
+ required:
+ - reg
+
+required:
+ - compatible
+ - reg
+
+unevaluatedProperties: false
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: syna,rmi4-i2c
+ then:
+ properties:
+ spi-rx-delay-us: false
+ spi-tx-delay-us: false
+ else:
+ properties:
+ syna,reset-delay-ms: false
+ syna,startup-delay-ms: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@20 {
+ compatible = "syna,rmi4-i2c";
+ reg = <0x20>;
+ interrupt-parent = <&gpx1>;
+ interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
+
+ syna,startup-delay-ms = <100>;
+ vdd-supply = <&tsp_vdd>;
+ vio-supply = <&ldo32_reg>;
+
+ pinctrl-0 = <&touch_irq>;
+ pinctrl-names = "default";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rmi4-f01@1 {
+ reg = <0x1>;
+ syna,nosleep-mode = <1>;
+ };
+
+ rmi4-f12@12 {
+ reg = <0x12>;
+ syna,sensor-type = <1>;
+ };
+
+ rmi4-f1a@1a {
+ reg = <0x1a>;
+ };
+ };
+ };
+
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@0 {
+ compatible = "syna,rmi4-spi";
+ reg = <0x0>;
+ interrupt-parent = <&gpx1>;
+ interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
+
+ spi-max-frequency = <4000000>;
+ spi-rx-delay-us = <30>;
+ spi-cpha;
+ spi-cpol;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rmi4-f01@1 {
+ reg = <0x1>;
+ syna,nosleep-mode = <1>;
+ };
+
+ rmi4-f11@11 {
+ reg = <0x11>;
+ touchscreen-inverted-y;
+ syna,sensor-type = <2>;
+ };
+ };
+ };
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] dt-bindings: input: convert syna,rmi4 to DT schema
From: Jason A. Donenfeld @ 2023-07-20 11:19 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Schiffer, Vincent Huang, linux-input, devicetree,
linux-kernel
In-Reply-To: <20230720110008.133359-1-krzysztof.kozlowski@linaro.org>
On Thu, Jul 20, 2023 at 01:00:08PM +0200, Krzysztof Kozlowski wrote:
> Convert the bindings for Synaptics RMI4 bus and devices to DT schema.
> Changes during conversion:
> 1. Add reset-gpios already used in DTS and mentioned by RMI4
> specification.
> 2. Do not require address/size cells, because without functions
> (children) they aren't really needed.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Jason, Matthias, Vincent,
> I put your names as maintainers, because moderately recently you were
> changing the driver. Let me know if this is okay or you prefer not to
> maintain the hardware.
I'm more of the occasional "why doesn't my new Thinkpad work?" sort of
drive-by RMI4-er, rather than an actual maintainer. I don't know a lot
about the protocol beyond what it takes to drag a laptop into behaving
right. So maybe best to leave me off the list.
Jason
^ permalink raw reply
* Re: [PATCH] dt-bindings: input: convert syna,rmi4 to DT schema
From: Krzysztof Kozlowski @ 2023-07-20 12:02 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Schiffer, Vincent Huang, linux-input, devicetree,
linux-kernel
In-Reply-To: <ZLkYWL4wMhYLRTuL@zx2c4.com>
On 20/07/2023 13:19, Jason A. Donenfeld wrote:
> On Thu, Jul 20, 2023 at 01:00:08PM +0200, Krzysztof Kozlowski wrote:
>> Convert the bindings for Synaptics RMI4 bus and devices to DT schema.
>> Changes during conversion:
>> 1. Add reset-gpios already used in DTS and mentioned by RMI4
>> specification.
>> 2. Do not require address/size cells, because without functions
>> (children) they aren't really needed.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Jason, Matthias, Vincent,
>> I put your names as maintainers, because moderately recently you were
>> changing the driver. Let me know if this is okay or you prefer not to
>> maintain the hardware.
>
> I'm more of the occasional "why doesn't my new Thinkpad work?" sort of
> drive-by RMI4-er, rather than an actual maintainer. I don't know a lot
> about the protocol beyond what it takes to drag a laptop into behaving
> right. So maybe best to leave me off the list.
If you have hardware with it and you are interested in this device
working, then it is good enough. This is maintainership only of the
bindings, not entire driver.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 12/42] dt-bindings: watchdog: Add Cirrus EP93x
From: Guenter Roeck @ 2023-07-20 13:28 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Sebastian Reichel, Thierry Reding,
Uwe Kleine-König, Mark Brown, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Vinod Koul, Miquel Raynal,
Richard Weinberger, Vignesh Raghavendra, Damien Le Moal,
Sergey Shtylyov, Dmitry Torokhov, Arnd Bergmann, Olof Johansson,
soc, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
Andy Shevchenko, Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-12-3d63a5f1103e@maquefel.me>
On 7/20/23 04:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This adds device tree bindings for the Cirrus Logic EP93xx
> watchdog block used in these SoCs.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> .../bindings/watchdog/cirrus,ep9301-wdt.yaml | 46 ++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/cirrus,ep9301-wdt.yaml b/Documentation/devicetree/bindings/watchdog/cirrus,ep9301-wdt.yaml
> new file mode 100644
> index 000000000000..d54595174a12
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/cirrus,ep9301-wdt.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/cirrus,ep9301-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic EP93xx Watchdog Timer
> +
> +maintainers:
> + - Nikita Shubin <nikita.shubin@maquefel.me>
> + - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
> +description:
> + Cirrus Logic EP93xx SoC family has it's own watchdog implementation
> +
Odd description. Isn't that true for pretty much every devicetree
bindings file, and pretty much every hardware driver ?
> +allOf:
> + - $ref: watchdog.yaml#
> +
> +properties:
> + compatible:
> + oneOf:
> + - const: cirrus,ep9301-wdt
> + - items:
> + - enum:
> + - cirrus,ep9302-wdt
> + - cirrus,ep9307-wdt
> + - cirrus,ep9312-wdt
> + - cirrus,ep9315-wdt
> + - const: cirrus,ep9301-wdt
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + watchdog@80940000 {
> + compatible = "cirrus,ep9301-wdt";
> + reg = <0x80940000 0x08>;
> + };
> +
>
^ permalink raw reply
* Re: [PATCH v3 13/42] watchdog: ep93xx: add DT support for Cirrus EP93xx
From: Guenter Roeck @ 2023-07-20 13:29 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Sebastian Reichel, Thierry Reding,
Uwe Kleine-König, Mark Brown, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Vinod Koul, Miquel Raynal,
Richard Weinberger, Vignesh Raghavendra, Damien Le Moal,
Sergey Shtylyov, Dmitry Torokhov, Arnd Bergmann, Olof Johansson,
soc, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
Andy Shevchenko, Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-13-3d63a5f1103e@maquefel.me>
On 7/20/23 04:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add OF ID match table.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/watchdog/ep93xx_wdt.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/watchdog/ep93xx_wdt.c b/drivers/watchdog/ep93xx_wdt.c
> index 59dfd7f6bf0b..af89b7bb8f66 100644
> --- a/drivers/watchdog/ep93xx_wdt.c
> +++ b/drivers/watchdog/ep93xx_wdt.c
> @@ -19,6 +19,7 @@
> */
>
> #include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/module.h>
> #include <linux/watchdog.h>
> #include <linux/io.h>
> @@ -127,9 +128,16 @@ static int ep93xx_wdt_probe(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct of_device_id ep93xx_wdt_of_ids[] = {
> + { .compatible = "cirrus,ep9301-wdt" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ep93xx_wdt_of_ids);
> +
> static struct platform_driver ep93xx_wdt_driver = {
> .driver = {
> .name = "ep93xx-wdt",
> + .of_match_table = ep93xx_wdt_of_ids,
> },
> .probe = ep93xx_wdt_probe,
> };
>
^ permalink raw reply
* Re: [PATCH v3 32/42] wdt: ts72xx: add DT support for ts72xx
From: Guenter Roeck @ 2023-07-20 13:30 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Sebastian Reichel, Thierry Reding,
Uwe Kleine-König, Mark Brown, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Vinod Koul, Miquel Raynal,
Richard Weinberger, Vignesh Raghavendra, Damien Le Moal,
Sergey Shtylyov, Dmitry Torokhov, Arnd Bergmann, Olof Johansson,
soc, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
Andy Shevchenko, Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-32-3d63a5f1103e@maquefel.me>
On 7/20/23 04:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add OF ID match table.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/watchdog/ts72xx_wdt.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/watchdog/ts72xx_wdt.c b/drivers/watchdog/ts72xx_wdt.c
> index 3d57670befe1..ac709dc31a65 100644
> --- a/drivers/watchdog/ts72xx_wdt.c
> +++ b/drivers/watchdog/ts72xx_wdt.c
> @@ -12,6 +12,7 @@
> */
>
> #include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/module.h>
> #include <linux/watchdog.h>
> #include <linux/io.h>
> @@ -160,10 +161,17 @@ static int ts72xx_wdt_probe(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct of_device_id ts72xx_wdt_of_ids[] = {
> + { .compatible = "technologic,ts7200-wdt" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ts72xx_wdt_of_ids);
> +
> static struct platform_driver ts72xx_wdt_driver = {
> .probe = ts72xx_wdt_probe,
> .driver = {
> .name = "ts72xx-wdt",
> + .of_match_table = ts72xx_wdt_of_ids,
> },
> };
>
>
^ permalink raw reply
* Re: [PATCH v3 33/42] gpio: ep93xx: add DT support for gpio-ep93xx
From: Bartosz Golaszewski @ 2023-07-20 14:49 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen, linux-arm-kernel, linux-kernel,
linux-gpio, devicetree, linux-clk, linux-rtc, linux-watchdog,
linux-pm, linux-pwm, linux-spi, netdev, dmaengine, linux-mtd,
linux-ide, linux-input, alsa-devel, Andy Shevchenko
In-Reply-To: <20230605-ep93xx-v3-33-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 10:30 AM Nikita Shubin via B4 Relay
<devnull+nikita.shubin.maquefel.me@kernel.org> wrote:
>
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add OF ID match table.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> drivers/gpio/gpio-ep93xx.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 9a25bb0caf17..c4e272a8773d 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -360,9 +360,15 @@ static int ep93xx_gpio_probe(struct platform_device *pdev)
> return devm_gpiochip_add_data(&pdev->dev, gc, egc);
> }
>
> +static const struct of_device_id ep93xx_gpio_match[] = {
> + { .compatible = "cirrus,ep9301-gpio" },
> + { /* sentinel */ }
> +};
> +
> static struct platform_driver ep93xx_gpio_driver = {
> .driver = {
> .name = "gpio-ep93xx",
> + .of_match_table = ep93xx_gpio_match,
> },
> .probe = ep93xx_gpio_probe,
> };
>
> --
> 2.39.2
>
Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
^ permalink raw reply
* Re: [PATCH v3 01/42] gpio: ep93xx: split device in multiple
From: Bartosz Golaszewski @ 2023-07-20 14:51 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen, linux-arm-kernel, linux-kernel,
linux-gpio, devicetree, linux-clk, linux-rtc, linux-watchdog,
linux-pm, linux-pwm, linux-spi, netdev, dmaengine, linux-mtd,
linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-1-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 10:29 AM Nikita Shubin via B4 Relay
<devnull+nikita.shubin.maquefel.me@kernel.org> wrote:
>
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This prepares ep93xx SOC gpio to convert into device tree driver:
> - dropped banks and legacy defines
> - split AB IRQ and make it shared
>
> We are relying on IRQ number information A, B ports have single shared
> IRQ, while F port have dedicated IRQ for each line.
>
> Also we had to split single ep93xx platform_device into multiple, one
> for each port, without this we can't do a full working transition from
> legacy platform code into device tree capable. All GPIO_LOOKUP were
> change to match new chip namings.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
^ permalink raw reply
* Re: [PATCH v3 28/42] input: keypad: ep93xx: add DT support for Cirrus EP93xx
From: Dmitry Torokhov @ 2023-07-20 17:17 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Arnd Bergmann, Olof Johansson, soc, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, Andy Shevchenko, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-28-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:28PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> - drop flags, they were not used anyway
> - add OF ID match table
> - process "autorepeat", "debounce-delay-ms", prescale from device tree
> - drop platform data usage and it's header
> - keymap goes from device tree now on
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
This is awesome, thank you!
>
> #include <linux/bits.h>
> #include <linux/module.h>
> +#include <linux/of.h>
Are you sure you need this? I think the only OF-specific structure that
is being used is of_device_id, which comes from mod_devicetable.h that
you include below.
Otherwise:
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Please feel free to merge with the rest of the series.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: nintendo: reinitialize USB Pro Controller after resuming from suspend
From: Silvan Jegen @ 2023-07-20 19:53 UTC (permalink / raw)
To: Martino Fontana; +Cc: linux-input
In-Reply-To: <20230712121042.25318-2-tinozzo123@gmail.com>
Hi
Thanks for the patch!
Some comments below.
Martino Fontana <tinozzo123@gmail.com> wrote:
> When suspending the computer, a Switch Pro Controller connected via USB will lose its internal status.
> However, because the USB connection was technically never lost, when resuming the computer, the driver will attempt to communicate with the controller as if nothing happened (and fail).
> Because of this, the user was forced to manually disconnect the controller (or to press the sync button on the controller to power it off), so that it can be re-initialized.
>
> With this patch, the controller will be automatically re-initialized after resuming from suspend.
The commit message would read nicer when wrapped around 78 chars or so.
> Fixes https://bugzilla.kernel.org/show_bug.cgi?id=216233
>
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
> ---
> drivers/hid/hid-nintendo.c | 178 ++++++++++++++++++++++---------------
> 1 file changed, 106 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 250f5d2f8..586c7f8d7 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2088,7 +2088,9 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
> struct joycon_input_report *report;
>
> req.subcmd_id = JC_SUBCMD_REQ_DEV_INFO;
> + mutex_lock(&ctlr->output_mutex);
> ret = joycon_send_subcmd(ctlr, &req, 0, HZ);
> + mutex_unlock(&ctlr->output_mutex);
> if (ret) {
> hid_err(ctlr->hdev, "Failed to get joycon info; ret=%d\n", ret);
> return ret;
> @@ -2117,6 +2119,88 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
> return 0;
> }
>
> +static int joycon_init(struct hid_device *hdev)
> +{
> + struct joycon_ctlr *ctlr = hid_get_drvdata(hdev);
> + int ret = 0;
> +
> + mutex_lock(&ctlr->output_mutex);
> + /* if handshake command fails, assume ble pro controller */
> + if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> + !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> + hid_dbg(hdev, "detected USB controller\n");
> + /* set baudrate for improved latency */
> + ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
> + if (ret) {
> + hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
> + goto err_mutex;
> + }
> + /* handshake */
> + ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> + if (ret) {
> + hid_err(hdev, "Failed handshake; ret=%d\n", ret);
> + goto err_mutex;
> + }
> + /*
> + * Set no timeout (to keep controller in USB mode).
> + * This doesn't send a response, so ignore the timeout.
> + */
> + joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> + } else if (jc_type_is_chrggrip(ctlr)) {
> + hid_err(hdev, "Failed charging grip handshake\n");
> + ret = -ETIMEDOUT;
> + goto err_mutex;
> + }
> +
> + /* get controller calibration data, and parse it */
> + ret = joycon_request_calibration(ctlr);
> + if (ret) {
> + /*
> + * We can function with default calibration, but it may be
> + * inaccurate. Provide a warning, and continue on.
> + */
> + hid_warn(hdev, "Analog stick positions may be inaccurate\n");
> + }
> +
> + /* get IMU calibration data, and parse it */
> + ret = joycon_request_imu_calibration(ctlr);
> + if (ret) {
> + /*
> + * We can function with default calibration, but it may be
> + * inaccurate. Provide a warning, and continue on.
> + */
> + hid_warn(hdev, "Unable to read IMU calibration data\n");
> + }
> +
> + /* Set the reporting mode to 0x30, which is the full report mode */
> + ret = joycon_set_report_mode(ctlr);
> + if (ret) {
> + hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
> + goto err_mutex;
> + }
> +
> + /* Enable rumble */
> + ret = joycon_enable_rumble(ctlr);
> + if (ret) {
> + hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
> + goto err_mutex;
> + }
> +
> + /* Enable the IMU */
> + ret = joycon_enable_imu(ctlr);
> + if (ret) {
> + hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
> + goto err_mutex;
> + }
> +
> + mutex_unlock(&ctlr->output_mutex);
> + return 0;
> +
> +err_mutex:
> + mutex_unlock(&ctlr->output_mutex);
> + return ret;
> +}
> +
> /* Common handler for parsing inputs */
> static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
> int size)
> @@ -2248,85 +2332,19 @@ static int nintendo_hid_probe(struct hid_device *hdev,
>
> hid_device_io_start(hdev);
>
> - /* Initialize the controller */
> - mutex_lock(&ctlr->output_mutex);
> - /* if handshake command fails, assume ble pro controller */
> - if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> - !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> - hid_dbg(hdev, "detected USB controller\n");
> - /* set baudrate for improved latency */
> - ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
> - if (ret) {
> - hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
> - goto err_mutex;
> - }
> - /* handshake */
> - ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> - if (ret) {
> - hid_err(hdev, "Failed handshake; ret=%d\n", ret);
> - goto err_mutex;
> - }
> - /*
> - * Set no timeout (to keep controller in USB mode).
> - * This doesn't send a response, so ignore the timeout.
> - */
> - joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> - } else if (jc_type_is_chrggrip(ctlr)) {
> - hid_err(hdev, "Failed charging grip handshake\n");
> - ret = -ETIMEDOUT;
> - goto err_mutex;
> - }
> -
> - /* get controller calibration data, and parse it */
> - ret = joycon_request_calibration(ctlr);
> - if (ret) {
> - /*
> - * We can function with default calibration, but it may be
> - * inaccurate. Provide a warning, and continue on.
> - */
> - hid_warn(hdev, "Analog stick positions may be inaccurate\n");
> - }
> -
> - /* get IMU calibration data, and parse it */
> - ret = joycon_request_imu_calibration(ctlr);
> - if (ret) {
> - /*
> - * We can function with default calibration, but it may be
> - * inaccurate. Provide a warning, and continue on.
> - */
> - hid_warn(hdev, "Unable to read IMU calibration data\n");
> - }
> -
> - /* Set the reporting mode to 0x30, which is the full report mode */
> - ret = joycon_set_report_mode(ctlr);
> - if (ret) {
> - hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
> - goto err_mutex;
> - }
> -
> - /* Enable rumble */
> - ret = joycon_enable_rumble(ctlr);
> - if (ret) {
> - hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
> - goto err_mutex;
> - }
> -
> - /* Enable the IMU */
> - ret = joycon_enable_imu(ctlr);
> + ret = joycon_init(hdev);
> if (ret) {
> - hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
> - goto err_mutex;
> + hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret);
> + goto err_close;
> }
>
> ret = joycon_read_info(ctlr);
> if (ret) {
> hid_err(hdev, "Failed to retrieve controller info; ret=%d\n",
> ret);
> - goto err_mutex;
> + goto err_close;
> }
>
> - mutex_unlock(&ctlr->output_mutex);
> -
> /* Initialize the leds */
> ret = joycon_leds_create(ctlr);
> if (ret) {
> @@ -2352,8 +2370,6 @@ static int nintendo_hid_probe(struct hid_device *hdev,
> hid_dbg(hdev, "probe - success\n");
> return 0;
>
> -err_mutex:
> - mutex_unlock(&ctlr->output_mutex);
> err_close:
> hid_hw_close(hdev);
> err_stop:
> @@ -2383,6 +2399,20 @@ static void nintendo_hid_remove(struct hid_device *hdev)
> hid_hw_stop(hdev);
> }
>
> +#ifdef CONFIG_PM
> +
> +static int nintendo_hid_resume(struct hid_device *hdev)
> +{
> + int ret = joycon_init(hdev);
> +
Removing this empty line would be more consistent with the rest of
the code.
> + if (ret)
> + hid_err(hdev, "Failed to restore controller after resume");
> +
> + return ret;
> +}
> +
> +#endif
> +
> static const struct hid_device_id nintendo_hid_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO,
> USB_DEVICE_ID_NINTENDO_PROCON) },
> @@ -2404,6 +2434,10 @@ static struct hid_driver nintendo_hid_driver = {
> .probe = nintendo_hid_probe,
> .remove = nintendo_hid_remove,
> .raw_event = nintendo_hid_event,
> +
> +#ifdef CONFIG_PM
> + .resume = nintendo_hid_resume,
We shouldn't mix tabs and spaces for formatting here.
Cheers,
Silvan
> +#endif
> };
> module_hid_driver(nintendo_hid_driver);
>
^ permalink raw reply
* Re: [PATCH] HID: nintendo: cleanup LED code
From: Silvan Jegen @ 2023-07-20 20:04 UTC (permalink / raw)
To: Martino Fontana; +Cc: linux-input
In-Reply-To: <20230712121433.26134-1-tinozzo123@gmail.com>
Hi!
Some comments inline below.
Martino Fontana <tinozzo123@gmail.com> wrote:
> - Only turn on the nth LED, instead of all the LEDs up to n. This better matches Nintendo consoles' behaviour, as they never turn on more than one LED (still not a complete match, see https://bugzilla.kernel.org/show_bug.cgi?id=216225)
> - Split part of `joycon_home_led_brightness_set` (which is called by hid) into `joycon_set_home_led` (which is what actually sets the LEDs), for consistency with player LEDs
> - Instead of first registering the `led_classdev`, then attempting to set the LED and unregistering the `led_classdev` if it fails, first attempt to set the LED, then register the `led_classdev` only if it succeeds (the class is still filled up in either case)
> - If setting player LEDs fails, still attempt setting the home LED (I don't know if this is actually possible, but who knows...)
> - Use `JC_NUM_LEDS` when appropriate instead of 4
> - Print return codes
Wrapping the git commit message around 78 chars or so is conventional.
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
> ---
> drivers/hid/hid-nintendo.c | 116 ++++++++++++++++++-------------------
> 1 file changed, 55 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 586c7f8d7..89631e19b 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -699,6 +699,25 @@ static int joycon_set_player_leds(struct joycon_ctlr *ctlr, u8 flash, u8 on)
> return joycon_send_subcmd(ctlr, req, 1, HZ/4);
> }
>
> +static int joycon_set_home_led(struct joycon_ctlr *ctlr, enum led_brightness brightness)
> +{
> + struct joycon_subcmd_request *req;
> + u8 buffer[sizeof(*req) + 5] = { 0 };
> + u8 *data;
> +
> + req = (struct joycon_subcmd_request *)buffer;
> + req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
> + data = req->data;
> + data[0] = 0x01;
> + data[1] = brightness << 4;
> + data[2] = brightness | (brightness << 4);
> + data[3] = 0x11;
> + data[4] = 0x11;
> +
> + hid_dbg(ctlr->hdev, "setting home led brightness\n");
> + return joycon_send_subcmd(ctlr, req, 5, HZ/4);
> +}
> +
> static int joycon_request_spi_flash_read(struct joycon_ctlr *ctlr,
> u32 start_addr, u8 size, u8 **reply)
> {
> @@ -1849,7 +1868,6 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
> int val = 0;
> int i;
> int ret;
> - int num;
>
> ctlr = hid_get_drvdata(hdev);
> if (!ctlr) {
> @@ -1857,21 +1875,10 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
> return -ENODEV;
> }
>
> - /* determine which player led this is */
> - for (num = 0; num < JC_NUM_LEDS; num++) {
> - if (&ctlr->leds[num] == led)
> - break;
> - }
> - if (num >= JC_NUM_LEDS)
> - return -EINVAL;
> + for (i = 0; i < JC_NUM_LEDS; i++)
> + val |= ctlr->leds[i].brightness << i;
>
> mutex_lock(&ctlr->output_mutex);
> - for (i = 0; i < JC_NUM_LEDS; i++) {
> - if (i == num)
> - val |= brightness << i;
> - else
> - val |= ctlr->leds[i].brightness << i;
> - }
> ret = joycon_set_player_leds(ctlr, 0, val);
> mutex_unlock(&ctlr->output_mutex);
>
> @@ -1884,9 +1891,6 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
> struct device *dev = led->dev->parent;
> struct hid_device *hdev = to_hid_device(dev);
> struct joycon_ctlr *ctlr;
> - struct joycon_subcmd_request *req;
> - u8 buffer[sizeof(*req) + 5] = { 0 };
> - u8 *data;
> int ret;
>
> ctlr = hid_get_drvdata(hdev);
> @@ -1894,25 +1898,13 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
> hid_err(hdev, "No controller data\n");
> return -ENODEV;
> }
> -
> - req = (struct joycon_subcmd_request *)buffer;
> - req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
> - data = req->data;
> - data[0] = 0x01;
> - data[1] = brightness << 4;
> - data[2] = brightness | (brightness << 4);
> - data[3] = 0x11;
> - data[4] = 0x11;
> -
> - hid_dbg(hdev, "setting home led brightness\n");
> mutex_lock(&ctlr->output_mutex);
> - ret = joycon_send_subcmd(ctlr, req, 5, HZ/4);
> + ret = joycon_set_home_led(ctlr, brightness);
> mutex_unlock(&ctlr->output_mutex);
> -
> return ret;
> }
>
> -static DEFINE_MUTEX(joycon_input_num_mutex);
> +static DEFINE_SPINLOCK(joycon_input_num_spinlock);
> static int joycon_leds_create(struct joycon_ctlr *ctlr)
> {
> struct hid_device *hdev = ctlr->hdev;
> @@ -1920,17 +1912,16 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> const char *d_name = dev_name(dev);
> struct led_classdev *led;
> char *name;
> - int ret = 0;
> + int ret;
> int i;
> - static int input_num = 1;
> + unsigned long flags;
> + int player_led_number;
> + static int input_num;
>
> - /* Set the default controller player leds based on controller number */
> - mutex_lock(&joycon_input_num_mutex);
> - mutex_lock(&ctlr->output_mutex);
> - ret = joycon_set_player_leds(ctlr, 0, 0xF >> (4 - input_num));
> - if (ret)
> - hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
> - mutex_unlock(&ctlr->output_mutex);
> + /* Set the player leds based on controller number */
> + spin_lock_irqsave(&joycon_input_num_spinlock, flags);
> + player_led_number = input_num++ % JC_NUM_LEDS;
> + spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
>
> /* configure the player LEDs */
> for (i = 0; i < JC_NUM_LEDS; i++) {
> @@ -1938,31 +1929,33 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> d_name,
> "green",
> joycon_player_led_names[i]);
> - if (!name) {
> - mutex_unlock(&joycon_input_num_mutex);
> + if (!name)
> return -ENOMEM;
> - }
>
> led = &ctlr->leds[i];
> led->name = name;
> - led->brightness = ((i + 1) <= input_num) ? 1 : 0;
> + led->brightness = (i == player_led_number) ? 1 : 0;
> led->max_brightness = 1;
> led->brightness_set_blocking =
> joycon_player_led_brightness_set;
> led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> + }
> + mutex_lock(&ctlr->output_mutex);
> + ret = joycon_set_player_leds(ctlr, 0, 0x1 << player_led_number);
> + mutex_unlock(&ctlr->output_mutex);
> + if (ret) {
> + hid_warn(hdev, "Failed to set players LEDs; ret=%d\n", ret);
> + goto home_led;
> + }
>
> + for (i = 0; i < JC_NUM_LEDS; i++) {
> + led = &ctlr->leds[i];
> ret = devm_led_classdev_register(&hdev->dev, led);
> - if (ret) {
> - hid_err(hdev, "Failed registering %s LED\n", led->name);
> - mutex_unlock(&joycon_input_num_mutex);
> - return ret;
> - }
> + if (ret)
> + hid_err(hdev, "Failed registering %s LED; ret=%d\n", led->name, ret);
> }
>
> - if (++input_num > 4)
> - input_num = 1;
> - mutex_unlock(&joycon_input_num_mutex);
> -
> +home_led:
> /* configure the home LED */
> if (jc_type_has_right(ctlr)) {
> name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
> @@ -1978,17 +1971,18 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> led->max_brightness = 0xF;
> led->brightness_set_blocking = joycon_home_led_brightness_set;
> led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> - ret = devm_led_classdev_register(&hdev->dev, led);
> - if (ret) {
> - hid_err(hdev, "Failed registering home led\n");
> - return ret;
> - }
> +
> /* Set the home LED to 0 as default state */
> - ret = joycon_home_led_brightness_set(led, 0);
> + mutex_lock(&ctlr->output_mutex);
> + ret = joycon_set_home_led(ctlr, 0);
> + mutex_unlock(&ctlr->output_mutex);
We shouldn't use any spaces for indentation here.
> if (ret) {
> - hid_warn(hdev, "Failed to set home LED default, unregistering home LED");
> - devm_led_classdev_unregister(&hdev->dev, led);
> + hid_warn(hdev, "Failed to set home LED; ret=%d\n", ret);
> + return 0;
> }
Adding an empty line here would be more consistent with the rest of
the code.
> + ret = devm_led_classdev_register(&hdev->dev, led);
> + if (ret)
> + hid_err(hdev, "Failed registering home led; ret=%d\n", ret);
In the old code we returned "ret" in this case. We probably want to do
the same here.
Cheers,
Silvan
> }
>
> return 0;
^ permalink raw reply
* [PATCH v2] HID: nintendo: reinitialize USB Pro Controller after resuming from suspend
From: Martino Fontana @ 2023-07-20 23:31 UTC (permalink / raw)
To: s.jegen; +Cc: linux-input, tinozzo123
In-Reply-To: <27ZF1I9PL9SUH.27UHPHBWRZ1LC@homearch.localdomain>
When suspending the computer, a Switch Pro Controller connected via USB will
lose its internal status. However, because the USB connection was technically
never lost, when resuming the computer, the driver will attempt to communicate
with the controller as if nothing happened (and fail).
Because of this, the user was forced to manually disconnect the controller
(or to press the sync button on the controller to power it off), so that it
can be re-initialized.
With this patch, the controller will be automatically re-initialized after
resuming from suspend.
Fixes https://bugzilla.kernel.org/show_bug.cgi?id=216233
Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
---
drivers/hid/hid-nintendo.c | 178 ++++++++++++++++++++++---------------
1 file changed, 106 insertions(+), 72 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 250f5d2f8..a5ebe857a 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -2088,7 +2088,9 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
struct joycon_input_report *report;
req.subcmd_id = JC_SUBCMD_REQ_DEV_INFO;
+ mutex_lock(&ctlr->output_mutex);
ret = joycon_send_subcmd(ctlr, &req, 0, HZ);
+ mutex_unlock(&ctlr->output_mutex);
if (ret) {
hid_err(ctlr->hdev, "Failed to get joycon info; ret=%d\n", ret);
return ret;
@@ -2117,6 +2119,88 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
return 0;
}
+static int joycon_init(struct hid_device *hdev)
+{
+ struct joycon_ctlr *ctlr = hid_get_drvdata(hdev);
+ int ret = 0;
+
+ mutex_lock(&ctlr->output_mutex);
+ /* if handshake command fails, assume ble pro controller */
+ if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
+ !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
+ hid_dbg(hdev, "detected USB controller\n");
+ /* set baudrate for improved latency */
+ ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
+ if (ret) {
+ hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
+ goto err_mutex;
+ }
+ /* handshake */
+ ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
+ if (ret) {
+ hid_err(hdev, "Failed handshake; ret=%d\n", ret);
+ goto err_mutex;
+ }
+ /*
+ * Set no timeout (to keep controller in USB mode).
+ * This doesn't send a response, so ignore the timeout.
+ */
+ joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
+ } else if (jc_type_is_chrggrip(ctlr)) {
+ hid_err(hdev, "Failed charging grip handshake\n");
+ ret = -ETIMEDOUT;
+ goto err_mutex;
+ }
+
+ /* get controller calibration data, and parse it */
+ ret = joycon_request_calibration(ctlr);
+ if (ret) {
+ /*
+ * We can function with default calibration, but it may be
+ * inaccurate. Provide a warning, and continue on.
+ */
+ hid_warn(hdev, "Analog stick positions may be inaccurate\n");
+ }
+
+ /* get IMU calibration data, and parse it */
+ ret = joycon_request_imu_calibration(ctlr);
+ if (ret) {
+ /*
+ * We can function with default calibration, but it may be
+ * inaccurate. Provide a warning, and continue on.
+ */
+ hid_warn(hdev, "Unable to read IMU calibration data\n");
+ }
+
+ /* Set the reporting mode to 0x30, which is the full report mode */
+ ret = joycon_set_report_mode(ctlr);
+ if (ret) {
+ hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
+ goto err_mutex;
+ }
+
+ /* Enable rumble */
+ ret = joycon_enable_rumble(ctlr);
+ if (ret) {
+ hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
+ goto err_mutex;
+ }
+
+ /* Enable the IMU */
+ ret = joycon_enable_imu(ctlr);
+ if (ret) {
+ hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
+ goto err_mutex;
+ }
+
+ mutex_unlock(&ctlr->output_mutex);
+ return 0;
+
+err_mutex:
+ mutex_unlock(&ctlr->output_mutex);
+ return ret;
+}
+
/* Common handler for parsing inputs */
static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
int size)
@@ -2248,85 +2332,19 @@ static int nintendo_hid_probe(struct hid_device *hdev,
hid_device_io_start(hdev);
- /* Initialize the controller */
- mutex_lock(&ctlr->output_mutex);
- /* if handshake command fails, assume ble pro controller */
- if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
- !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
- hid_dbg(hdev, "detected USB controller\n");
- /* set baudrate for improved latency */
- ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
- if (ret) {
- hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
- goto err_mutex;
- }
- /* handshake */
- ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
- if (ret) {
- hid_err(hdev, "Failed handshake; ret=%d\n", ret);
- goto err_mutex;
- }
- /*
- * Set no timeout (to keep controller in USB mode).
- * This doesn't send a response, so ignore the timeout.
- */
- joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
- } else if (jc_type_is_chrggrip(ctlr)) {
- hid_err(hdev, "Failed charging grip handshake\n");
- ret = -ETIMEDOUT;
- goto err_mutex;
- }
-
- /* get controller calibration data, and parse it */
- ret = joycon_request_calibration(ctlr);
- if (ret) {
- /*
- * We can function with default calibration, but it may be
- * inaccurate. Provide a warning, and continue on.
- */
- hid_warn(hdev, "Analog stick positions may be inaccurate\n");
- }
-
- /* get IMU calibration data, and parse it */
- ret = joycon_request_imu_calibration(ctlr);
- if (ret) {
- /*
- * We can function with default calibration, but it may be
- * inaccurate. Provide a warning, and continue on.
- */
- hid_warn(hdev, "Unable to read IMU calibration data\n");
- }
-
- /* Set the reporting mode to 0x30, which is the full report mode */
- ret = joycon_set_report_mode(ctlr);
- if (ret) {
- hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
- goto err_mutex;
- }
-
- /* Enable rumble */
- ret = joycon_enable_rumble(ctlr);
- if (ret) {
- hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
- goto err_mutex;
- }
-
- /* Enable the IMU */
- ret = joycon_enable_imu(ctlr);
+ ret = joycon_init(hdev);
if (ret) {
- hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
- goto err_mutex;
+ hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret);
+ goto err_close;
}
ret = joycon_read_info(ctlr);
if (ret) {
hid_err(hdev, "Failed to retrieve controller info; ret=%d\n",
ret);
- goto err_mutex;
+ goto err_close;
}
- mutex_unlock(&ctlr->output_mutex);
-
/* Initialize the leds */
ret = joycon_leds_create(ctlr);
if (ret) {
@@ -2352,8 +2370,6 @@ static int nintendo_hid_probe(struct hid_device *hdev,
hid_dbg(hdev, "probe - success\n");
return 0;
-err_mutex:
- mutex_unlock(&ctlr->output_mutex);
err_close:
hid_hw_close(hdev);
err_stop:
@@ -2383,6 +2399,20 @@ static void nintendo_hid_remove(struct hid_device *hdev)
hid_hw_stop(hdev);
}
+#ifdef CONFIG_PM
+
+static int nintendo_hid_resume(struct hid_device *hdev)
+{
+ int ret = joycon_init(hdev);
+
+ if (ret)
+ hid_err(hdev, "Failed to restore controller after resume");
+
+ return ret;
+}
+
+#endif
+
static const struct hid_device_id nintendo_hid_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO,
USB_DEVICE_ID_NINTENDO_PROCON) },
@@ -2404,6 +2434,10 @@ static struct hid_driver nintendo_hid_driver = {
.probe = nintendo_hid_probe,
.remove = nintendo_hid_remove,
.raw_event = nintendo_hid_event,
+
+#ifdef CONFIG_PM
+ .resume = nintendo_hid_resume,
+#endif
};
module_hid_driver(nintendo_hid_driver);
--
2.41.0
^ permalink raw reply related
* Re: [PATCH] HID: nintendo: cleanup LED code
From: Martino Fontana @ 2023-07-20 23:55 UTC (permalink / raw)
To: Silvan Jegen; +Cc: linux-input
In-Reply-To: <2KDZBNL37MH80.3RZBUHZA85LM7@homearch.localdomain>
The reason I avoid returning `ret` is that, if something other than 0
is returned, then `nintendo_hid_probe` (the function calling
`joycon_leds_create`) would fail completely. That obviously shouldn't
happen, LEDs aren't a vital piece for the gamepad functionality.
Alternatively, instead of avoiding returning anything other than 0,
`nintendo_hid_probe` could be modified by changing `if (ret)` in that
section to something else.
I'm not an expert in C, so... opinions?
Il giorno gio 20 lug 2023 alle ore 22:04 Silvan Jegen
<s.jegen@gmail.com> ha scritto:
>
> Hi!
>
> Some comments inline below.
>
> Martino Fontana <tinozzo123@gmail.com> wrote:
> > - Only turn on the nth LED, instead of all the LEDs up to n. This better matches Nintendo consoles' behaviour, as they never turn on more than one LED (still not a complete match, see https://bugzilla.kernel.org/show_bug.cgi?id=216225)
> > - Split part of `joycon_home_led_brightness_set` (which is called by hid) into `joycon_set_home_led` (which is what actually sets the LEDs), for consistency with player LEDs
> > - Instead of first registering the `led_classdev`, then attempting to set the LED and unregistering the `led_classdev` if it fails, first attempt to set the LED, then register the `led_classdev` only if it succeeds (the class is still filled up in either case)
> > - If setting player LEDs fails, still attempt setting the home LED (I don't know if this is actually possible, but who knows...)
> > - Use `JC_NUM_LEDS` when appropriate instead of 4
> > - Print return codes
>
> Wrapping the git commit message around 78 chars or so is conventional.
>
>
> > Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
> > ---
> > drivers/hid/hid-nintendo.c | 116 ++++++++++++++++++-------------------
> > 1 file changed, 55 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> > index 586c7f8d7..89631e19b 100644
> > --- a/drivers/hid/hid-nintendo.c
> > +++ b/drivers/hid/hid-nintendo.c
> > @@ -699,6 +699,25 @@ static int joycon_set_player_leds(struct joycon_ctlr *ctlr, u8 flash, u8 on)
> > return joycon_send_subcmd(ctlr, req, 1, HZ/4);
> > }
> >
> > +static int joycon_set_home_led(struct joycon_ctlr *ctlr, enum led_brightness brightness)
> > +{
> > + struct joycon_subcmd_request *req;
> > + u8 buffer[sizeof(*req) + 5] = { 0 };
> > + u8 *data;
> > +
> > + req = (struct joycon_subcmd_request *)buffer;
> > + req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
> > + data = req->data;
> > + data[0] = 0x01;
> > + data[1] = brightness << 4;
> > + data[2] = brightness | (brightness << 4);
> > + data[3] = 0x11;
> > + data[4] = 0x11;
> > +
> > + hid_dbg(ctlr->hdev, "setting home led brightness\n");
> > + return joycon_send_subcmd(ctlr, req, 5, HZ/4);
> > +}
> > +
> > static int joycon_request_spi_flash_read(struct joycon_ctlr *ctlr,
> > u32 start_addr, u8 size, u8 **reply)
> > {
> > @@ -1849,7 +1868,6 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
> > int val = 0;
> > int i;
> > int ret;
> > - int num;
> >
> > ctlr = hid_get_drvdata(hdev);
> > if (!ctlr) {
> > @@ -1857,21 +1875,10 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
> > return -ENODEV;
> > }
> >
> > - /* determine which player led this is */
> > - for (num = 0; num < JC_NUM_LEDS; num++) {
> > - if (&ctlr->leds[num] == led)
> > - break;
> > - }
> > - if (num >= JC_NUM_LEDS)
> > - return -EINVAL;
> > + for (i = 0; i < JC_NUM_LEDS; i++)
> > + val |= ctlr->leds[i].brightness << i;
> >
> > mutex_lock(&ctlr->output_mutex);
> > - for (i = 0; i < JC_NUM_LEDS; i++) {
> > - if (i == num)
> > - val |= brightness << i;
> > - else
> > - val |= ctlr->leds[i].brightness << i;
> > - }
> > ret = joycon_set_player_leds(ctlr, 0, val);
> > mutex_unlock(&ctlr->output_mutex);
> >
> > @@ -1884,9 +1891,6 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
> > struct device *dev = led->dev->parent;
> > struct hid_device *hdev = to_hid_device(dev);
> > struct joycon_ctlr *ctlr;
> > - struct joycon_subcmd_request *req;
> > - u8 buffer[sizeof(*req) + 5] = { 0 };
> > - u8 *data;
> > int ret;
> >
> > ctlr = hid_get_drvdata(hdev);
> > @@ -1894,25 +1898,13 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
> > hid_err(hdev, "No controller data\n");
> > return -ENODEV;
> > }
> > -
> > - req = (struct joycon_subcmd_request *)buffer;
> > - req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
> > - data = req->data;
> > - data[0] = 0x01;
> > - data[1] = brightness << 4;
> > - data[2] = brightness | (brightness << 4);
> > - data[3] = 0x11;
> > - data[4] = 0x11;
> > -
> > - hid_dbg(hdev, "setting home led brightness\n");
> > mutex_lock(&ctlr->output_mutex);
> > - ret = joycon_send_subcmd(ctlr, req, 5, HZ/4);
> > + ret = joycon_set_home_led(ctlr, brightness);
> > mutex_unlock(&ctlr->output_mutex);
> > -
> > return ret;
> > }
> >
> > -static DEFINE_MUTEX(joycon_input_num_mutex);
> > +static DEFINE_SPINLOCK(joycon_input_num_spinlock);
> > static int joycon_leds_create(struct joycon_ctlr *ctlr)
> > {
> > struct hid_device *hdev = ctlr->hdev;
> > @@ -1920,17 +1912,16 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> > const char *d_name = dev_name(dev);
> > struct led_classdev *led;
> > char *name;
> > - int ret = 0;
> > + int ret;
> > int i;
> > - static int input_num = 1;
> > + unsigned long flags;
> > + int player_led_number;
> > + static int input_num;
> >
> > - /* Set the default controller player leds based on controller number */
> > - mutex_lock(&joycon_input_num_mutex);
> > - mutex_lock(&ctlr->output_mutex);
> > - ret = joycon_set_player_leds(ctlr, 0, 0xF >> (4 - input_num));
> > - if (ret)
> > - hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
> > - mutex_unlock(&ctlr->output_mutex);
> > + /* Set the player leds based on controller number */
> > + spin_lock_irqsave(&joycon_input_num_spinlock, flags);
> > + player_led_number = input_num++ % JC_NUM_LEDS;
> > + spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
> >
> > /* configure the player LEDs */
> > for (i = 0; i < JC_NUM_LEDS; i++) {
> > @@ -1938,31 +1929,33 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> > d_name,
> > "green",
> > joycon_player_led_names[i]);
> > - if (!name) {
> > - mutex_unlock(&joycon_input_num_mutex);
> > + if (!name)
> > return -ENOMEM;
> > - }
> >
> > led = &ctlr->leds[i];
> > led->name = name;
> > - led->brightness = ((i + 1) <= input_num) ? 1 : 0;
> > + led->brightness = (i == player_led_number) ? 1 : 0;
> > led->max_brightness = 1;
> > led->brightness_set_blocking =
> > joycon_player_led_brightness_set;
> > led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> > + }
> > + mutex_lock(&ctlr->output_mutex);
> > + ret = joycon_set_player_leds(ctlr, 0, 0x1 << player_led_number);
> > + mutex_unlock(&ctlr->output_mutex);
> > + if (ret) {
> > + hid_warn(hdev, "Failed to set players LEDs; ret=%d\n", ret);
> > + goto home_led;
> > + }
> >
> > + for (i = 0; i < JC_NUM_LEDS; i++) {
> > + led = &ctlr->leds[i];
> > ret = devm_led_classdev_register(&hdev->dev, led);
> > - if (ret) {
> > - hid_err(hdev, "Failed registering %s LED\n", led->name);
> > - mutex_unlock(&joycon_input_num_mutex);
> > - return ret;
> > - }
> > + if (ret)
> > + hid_err(hdev, "Failed registering %s LED; ret=%d\n", led->name, ret);
> > }
> >
> > - if (++input_num > 4)
> > - input_num = 1;
> > - mutex_unlock(&joycon_input_num_mutex);
> > -
> > +home_led:
> > /* configure the home LED */
> > if (jc_type_has_right(ctlr)) {
> > name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
> > @@ -1978,17 +1971,18 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> > led->max_brightness = 0xF;
> > led->brightness_set_blocking = joycon_home_led_brightness_set;
> > led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> > - ret = devm_led_classdev_register(&hdev->dev, led);
> > - if (ret) {
> > - hid_err(hdev, "Failed registering home led\n");
> > - return ret;
> > - }
> > +
> > /* Set the home LED to 0 as default state */
> > - ret = joycon_home_led_brightness_set(led, 0);
> > + mutex_lock(&ctlr->output_mutex);
> > + ret = joycon_set_home_led(ctlr, 0);
> > + mutex_unlock(&ctlr->output_mutex);
>
> We shouldn't use any spaces for indentation here.
>
> > if (ret) {
> > - hid_warn(hdev, "Failed to set home LED default, unregistering home LED");
> > - devm_led_classdev_unregister(&hdev->dev, led);
> > + hid_warn(hdev, "Failed to set home LED; ret=%d\n", ret);
> > + return 0;
> > }
>
> Adding an empty line here would be more consistent with the rest of
> the code.
>
> > + ret = devm_led_classdev_register(&hdev->dev, led);
> > + if (ret)
> > + hid_err(hdev, "Failed registering home led; ret=%d\n", ret);
>
> In the old code we returned "ret" in this case. We probably want to do
> the same here.
>
> Cheers,
> Silvan
>
> > }
> >
> > return 0;
>
>
^ permalink raw reply
* Re: Need information to implement correct handling of the GSP 370 wireless headset
From: Rahul Rameshbabu @ 2023-07-21 2:45 UTC (permalink / raw)
To: Werner Sembach; +Cc: linux-input
In-Reply-To: <f26c2aab-7c89-d6e4-b8c1-f6740e08b034@rdorf.de>
On Fri, 07 Jul, 2023 23:22:58 +0200 Werner Sembach <werner@rdorf.de> wrote:
> Am 07.07.23 um 23:12 schrieb Werner Sembach:
>> Hi,
>>
>> The EPOS GSP 370 wireless headset has a volume control knob on the headset.
>> Turning this has 2 effects on Linux:
>>
>> - The volume gets adjusted in firmware
>>
>> - A volume up/down key event is sent to the OS and the volume is adjustet on
>> the OS level (the dongle registers both as a audio device and an usb keyboard
>> to do this)
>>
>> This double volume adjust is ofc not the intended behavior and induces all
>> kinds of wiredness, like the os is already displaying 100% volume but you can
>> still turn it higher because the firmware has not yet reached max volume.
What you will likely want to do is implement a custom USB mixer quirk
for the audio device advertised by the USB dongle. I delve into this
more in the section discussing which subsystem would be appropriate for
this.
>>
>> Running Wireshark on Windows and Linux I recognized that Windows is sending
>> "URB_CONTROL out" events to the dongle on volume adjust, which Linux doesn't,
>> these events have a paylode that seems to encode a volume value in 2 byte with
>> little endianess.
>>
>> I'm unsure if I can attach pcap file or if it will get blocked in the list so
>> i will send them as a reply.
> As I thought: The pcapng files are to big for the mailing list. When someone is
> interested let me know how I can send them (should I open a Bugzilla Ticket for
> it?).
>>
>> So my 2 questions are:
>>
>> - What are these USB "URB_CONTROL out" events? (Does not seem to be HID
>> reports?)
URB stands for USB Request Block. You are specifically working with the
USB layer here rather than the HID layer that abstracts away the lower
level transport interface.
URB in general is a system (OS) - defined format for interacting with
the core host controller driver in the OS stack. This means the
implementation of sending requests to the USB hub driver in Linux and
Windows differ. You may want to understand the specific requests made by
the Windows USB client driver for the headset and implement an
equivalent driver using the URB API of the linux kernel.
Generic Overview
* https://wiki.osdev.org/Universal_Serial_Bus#Host_Controller_Driver
Linux specific information
* https://static.lwn.net/images/pdf/LDD3/ch13.pdf
* https://docs.kernel.org/driver-api/usb/URB.html
* https://docs.kernel.org/driver-api/usb/writing_usb_driver.html
Windows specific information
* https://learn.microsoft.com/en-us/windows-hardware/drivers/usbcon/usb-control-transfer
>>
>> - In which subsystem would I want to implement them? Or should this be handled
>> in userspace?
I am guessing you would not need to add any additional support for the
USB keyboard device advertised by the dongle. It's working fine and
advertising media keys to the system as you expect. Your goal now more
likely is to send USB requests to the headset (dongle) whenever the
volume level is changed for the audio device through the system. I
assume such a "driver" (quirk) would live under sound/usb in the kernel
tree just like sound/usb/mixer_scarlett.c,
sound/usb/mixer_scarlett_gen2.c, and other usb mixer implementations in
the kernel. Specifically in sound/usb/mixer_scarlett.c, I think you will
find the add_output_ctls function to be a helpful reference for defining
a new volume control that properly controls the device firmware volume
level based on the ALSA PCM level set in the kernel by user interaction
with the userspace audio component on his/her system. You will see all
these mixer implementations then get instantiated in
sound/usb/mixer_quirks.c in the snd_usb_mixer_apply_create_quirk
function.
>>
>> Bast regards,
>>
>> Werner
>>
Might want to ask the alsa-devel mailing list about how to go about
handling the device firmware audio control as well in the linux audio
stack if you did not find this explanation suitable for getting
started/want to inquire further about the ALSA kernel architecture.
https://www.alsa-project.org/wiki/Mailing-lists
-- Rahul Rameshbabu
^ permalink raw reply
* Re: Need information to implement correct handling of the GSP 370 wireless headset
From: Werner Sembach @ 2023-07-21 8:50 UTC (permalink / raw)
To: Rahul Rameshbabu; +Cc: linux-input
In-Reply-To: <87r0p2rnhx.fsf@nvidia.com>
Am 21.07.23 um 04:45 schrieb Rahul Rameshbabu:
> On Fri, 07 Jul, 2023 23:22:58 +0200 Werner Sembach <werner@rdorf.de> wrote:
>> Am 07.07.23 um 23:12 schrieb Werner Sembach:
>>> Hi,
>>>
>>> The EPOS GSP 370 wireless headset has a volume control knob on the headset.
>>> Turning this has 2 effects on Linux:
>>>
>>> - The volume gets adjusted in firmware
>>>
>>> - A volume up/down key event is sent to the OS and the volume is adjustet on
>>> the OS level (the dongle registers both as a audio device and an usb keyboard
>>> to do this)
>>>
>>> This double volume adjust is ofc not the intended behavior and induces all
>>> kinds of wiredness, like the os is already displaying 100% volume but you can
>>> still turn it higher because the firmware has not yet reached max volume.
> What you will likely want to do is implement a custom USB mixer quirk
> for the audio device advertised by the USB dongle. I delve into this
> more in the section discussing which subsystem would be appropriate for
> this.
Thank you very much for giving me the starting points, now I only need
to find some time to work on this.
>
>>> Running Wireshark on Windows and Linux I recognized that Windows is sending
>>> "URB_CONTROL out" events to the dongle on volume adjust, which Linux doesn't,
>>> these events have a paylode that seems to encode a volume value in 2 byte with
>>> little endianess.
>>>
>>> I'm unsure if I can attach pcap file or if it will get blocked in the list so
>>> i will send them as a reply.
>> As I thought: The pcapng files are to big for the mailing list. When someone is
>> interested let me know how I can send them (should I open a Bugzilla Ticket for
>> it?).
>>> So my 2 questions are:
>>>
>>> - What are these USB "URB_CONTROL out" events? (Does not seem to be HID
>>> reports?)
> URB stands for USB Request Block. You are specifically working with the
> USB layer here rather than the HID layer that abstracts away the lower
> level transport interface.
>
> URB in general is a system (OS) - defined format for interacting with
> the core host controller driver in the OS stack. This means the
> implementation of sending requests to the USB hub driver in Linux and
> Windows differ. You may want to understand the specific requests made by
> the Windows USB client driver for the headset and implement an
> equivalent driver using the URB API of the linux kernel.
>
> Generic Overview
>
> * https://wiki.osdev.org/Universal_Serial_Bus#Host_Controller_Driver
>
> Linux specific information
>
> * https://static.lwn.net/images/pdf/LDD3/ch13.pdf
> * https://docs.kernel.org/driver-api/usb/URB.html
> * https://docs.kernel.org/driver-api/usb/writing_usb_driver.html
>
> Windows specific information
>
> * https://learn.microsoft.com/en-us/windows-hardware/drivers/usbcon/usb-control-transfer
>
>>> - In which subsystem would I want to implement them? Or should this be handled
>>> in userspace?
> I am guessing you would not need to add any additional support for the
> USB keyboard device advertised by the dongle. It's working fine and
> advertising media keys to the system as you expect. Your goal now more
> likely is to send USB requests to the headset (dongle) whenever the
> volume level is changed for the audio device through the system. I
> assume such a "driver" (quirk) would live under sound/usb in the kernel
> tree just like sound/usb/mixer_scarlett.c,
> sound/usb/mixer_scarlett_gen2.c, and other usb mixer implementations in
> the kernel. Specifically in sound/usb/mixer_scarlett.c, I think you will
> find the add_output_ctls function to be a helpful reference for defining
> a new volume control that properly controls the device firmware volume
> level based on the ALSA PCM level set in the kernel by user interaction
> with the userspace audio component on his/her system. You will see all
> these mixer implementations then get instantiated in
> sound/usb/mixer_quirks.c in the snd_usb_mixer_apply_create_quirk
> function.
>
>>> Bast regards,
>>>
>>> Werner
>>>
> Might want to ask the alsa-devel mailing list about how to go about
> handling the device firmware audio control as well in the linux audio
> stack if you did not find this explanation suitable for getting
> started/want to inquire further about the ALSA kernel architecture.
>
> https://www.alsa-project.org/wiki/Mailing-lists
I will do that when the first question undoubtly pop up once I start
reading the mixer_* source code ^^.
>
> -- Rahul Rameshbabu
^ permalink raw reply
* Re: [PATCH v3 01/42] gpio: ep93xx: split device in multiple
From: Andy Shevchenko @ 2023-07-21 13:18 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-1-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:01PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This prepares ep93xx SOC gpio to convert into device tree driver:
> - dropped banks and legacy defines
> - split AB IRQ and make it shared
>
> We are relying on IRQ number information A, B ports have single shared
> IRQ, while F port have dedicated IRQ for each line.
>
> Also we had to split single ep93xx platform_device into multiple, one
> for each port, without this we can't do a full working transition from
> legacy platform code into device tree capable. All GPIO_LOOKUP were
> change to match new chip namings.
...
> -static void ep93xx_gpio_ab_irq_handler(struct irq_desc *desc)
> +static u32 ep93xx_gpio_ab_irq_handler(struct gpio_chip *gc)
> {
> - struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> - struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> - struct irq_chip *irqchip = irq_desc_get_chip(desc);
> + struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
> unsigned long stat;
> int offset;
>
> - chained_irq_enter(irqchip, desc);
> -
> - /*
> - * Dispatch the IRQs to the irqdomain of each A and B
> - * gpiochip irqdomains depending on what has fired.
> - * The tricky part is that the IRQ line is shared
> - * between bank A and B and each has their own gpiochip.
> - */
> - stat = readb(epg->base + EP93XX_GPIO_A_INT_STATUS);
> + stat = readb(eic->base + EP93XX_INT_STATUS_OFFSET);
> for_each_set_bit(offset, &stat, 8)
> - generic_handle_domain_irq(epg->gc[0].gc.irq.domain,
> - offset);
> + generic_handle_domain_irq(gc->irq.domain, offset);
>
> - stat = readb(epg->base + EP93XX_GPIO_B_INT_STATUS);
> - for_each_set_bit(offset, &stat, 8)
> - generic_handle_domain_irq(epg->gc[1].gc.irq.domain,
> - offset);
> + return stat;
> +}
>
> - chained_irq_exit(irqchip, desc);
> +static irqreturn_t ep93xx_ab_irq_handler(int irq, void *dev_id)
> +{
> + return IRQ_RETVAL(ep93xx_gpio_ab_irq_handler(dev_id));
> }
>
> static void ep93xx_gpio_f_irq_handler(struct irq_desc *desc)
> {
> - /*
> - * map discontiguous hw irq range to continuous sw irq range:
> - *
> - * IRQ_EP93XX_GPIO{0..7}MUX -> EP93XX_GPIO_LINE_F{0..7}
> - */
> struct irq_chip *irqchip = irq_desc_get_chip(desc);
> - unsigned int irq = irq_desc_get_irq(desc);
> - int port_f_idx = (irq & 7) ^ 4; /* {20..23,48..51} -> {0..7} */
> - int gpio_irq = EP93XX_GPIO_F_IRQ_BASE + port_f_idx;
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct gpio_irq_chip *gic = &gc->irq;
> + unsigned int parent = irq_desc_get_irq(desc);
> + unsigned int i;
>
> chained_irq_enter(irqchip, desc);
> - generic_handle_irq(gpio_irq);
> + for (i = 0; i < gic->num_parents; i++)
> + if (gic->parents[i] == parent)
> + break;
> +
> + if (i < gic->num_parents)
> + generic_handle_irq(irq_find_mapping(gc->irq.domain, i));
Can we use
generic_handle_domain_irq(gc->irq.domain, i);
here as well?
> chained_irq_exit(irqchip, desc);
> }
...
> - int offset = d->irq & 7;
> + int offset = irqd_to_hwirq(d);
irq_hw_number_t ?
> irq_flow_handler_t handler;
...
> + int ret, irq, i = 0;
What do you need this assignment for?
...
> + ret = devm_request_irq(dev, irq,
> + ep93xx_ab_irq_handler,
It can be located on the previous line.
> + IRQF_SHARED, gc->label, gc);
> + if (ret)
> + return dev_err_probe(dev, ret, "error requesting IRQ : %d\n", irq);
Drop duplicating word 'error' in the message.
Space is not needed before colon.
...
> + /* TODO: replace with handle_bad_irq once we are fully hierarchical */
To be pedantic: handle_bad_irq()
> + gc->label = dev_name(&pdev->dev);
> + if (platform_irq_count(pdev) > 0) {
> + dev_dbg(&pdev->dev, "setting up irqs for %s\n", dev_name(&pdev->dev));
> + ret = ep93xx_setup_irqs(pdev, egc);
> + if (ret)
> + dev_err(&pdev->dev, "setup irqs failed for %s\n", dev_name(&pdev->dev));
What's the point to print dev name twice? Esp. taking into account
gc->label assignment above. Why not use dev_err_probe() to unify
the format of the messages from ->probe()?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 03/42] clk: ep93xx: add DT support for Cirrus EP93xx
From: Andy Shevchenko @ 2023-07-21 13:46 UTC (permalink / raw)
To: Stephen Boyd
Cc: Alessandro Zummo, Alexander Sverdlin, Alexandre Belloni,
Arnd Bergmann, Bartosz Golaszewski, Conor Dooley, Damien Le Moal,
Daniel Lezcano, David S. Miller, Dmitry Torokhov, Eric Dumazet,
Guenter Roeck, Hartley Sweeten, Jakub Kicinski, Jaroslav Kysela,
Kris Bahnsen, Krzysztof Kozlowski, Lennert Buytenhek,
Liam Girdwood, Linus Walleij, Lukasz Majewski, Mark Brown,
Michael Peters, Michael Turquette, Miquel Raynal, Nikita Shubin,
Nikita Shubin via B4 Relay, Olof Johansson, Paolo Abeni,
Richard Weinberger, Rob Herring, Russell King, Sebastian Reichel,
Sergey Shtylyov, Takashi Iwai, Thierry Reding, Thomas Gleixner,
Uwe Kleine-König, Vignesh Raghavendra, Vinod Koul,
Wim Van Sebroeck, soc, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <3fcb760c101c5f7081235290362f5c02.sboyd@kernel.org>
On Thu, Jul 20, 2023 at 04:27:45PM -0700, Stephen Boyd wrote:
> Quoting Nikita Shubin via B4 Relay (2023-07-20 04:29:03)
...
> > +static bool is_best(unsigned long rate, unsigned long now,
> > + unsigned long best)
> > +{
> > + return abs(rate - now) < abs(rate - best);
>
> Another case where we need abs_diff() so that it doesn't get confused
> when trying to do signed comparison.
Here you are: Message-Id: <20230721134235.15517-1-andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 02/42] dt-bindings: clock: Add Cirrus EP93xx
From: Krzysztof Kozlowski @ 2023-07-21 13:58 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-2-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This adds device tree bindings for the Cirrus Logic EP93xx
> clock block used in these SoCs.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> .../bindings/clock/cirrus,ep9301-clk.yaml | 46 ++++++++++++++++++++++
> include/dt-bindings/clock/cirrus,ep93xx-clock.h | 41 +++++++++++++++++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/cirrus,ep9301-clk.yaml b/Documentation/devicetree/bindings/clock/cirrus,ep9301-clk.yaml
> new file mode 100644
> index 000000000000..111e016601fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/cirrus,ep9301-clk.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/cirrus,ep9301-clk.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic ep93xx SoC's clock controller
> +
> +maintainers:
> + - Nikita Shubin <nikita.shubin@maquefel.me>
> + - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
> +properties:
> + compatible:
> + oneOf:
> + - const: cirrus,ep9301-clk
> + - items:
> + - enum:
> + - cirrus,ep9302-clk
> + - cirrus,ep9307-clk
> + - cirrus,ep9312-clk
> + - cirrus,ep9315-clk
> + - const: cirrus,ep9301-clk
> +
> + "#clock-cells":
> + const: 1
> +
> + clocks:
> + items:
> + - description: reference clock
> +
> +required:
> + - compatible
> + - "#clock-cells"
> + - clocks
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + clock-controller {
> + compatible = "cirrus,ep9301-clk";
> + #clock-cells = <1>;
> + clocks = <&xtali>;
> + };
> +...
> diff --git a/include/dt-bindings/clock/cirrus,ep93xx-clock.h b/include/dt-bindings/clock/cirrus,ep93xx-clock.h
> new file mode 100644
> index 000000000000..3cd053c0fdea
> --- /dev/null
> +++ b/include/dt-bindings/clock/cirrus,ep93xx-clock.h
Keep the same filename as bindings file.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 04/42] dt-bindings: pinctrl: Add Cirrus EP93xx
From: Krzysztof Kozlowski @ 2023-07-21 14:01 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-4-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add YAML bindings for ep93xx SoC pinctrl.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> .../bindings/pinctrl/cirrus,ep9301-pinctrl.yaml | 58 ++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml
> new file mode 100644
> index 000000000000..d5682531b0da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/cirrus,ep9301-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus ep93xx pins mux controller
> +
> +maintainers:
> + - Nikita Shubin <nikita.shubin@maquefel.me>
> + - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
> +properties:
> + compatible:
> + oneOf:
> + - const: cirrus,ep9301-pinctrl
> + - items:
> + - enum:
> + - cirrus,ep9302-pinctrl
> + - cirrus,ep9307-pinctrl
> + - cirrus,ep9312-pinctrl
> + - cirrus,ep9315-pinctrl
> + - const: cirrus,ep9301-pinctrl
> +
> +patternProperties:
> + '^pins-':
> + type: object
> + description: pin node
> + $ref: pinmux-node.yaml#
You need:
unevaluatedProperties: false
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 06/42] dt-bindings: soc: Add Cirrus EP93xx
From: Krzysztof Kozlowski @ 2023-07-21 14:04 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-6-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This adds device tree bindings for the Cirrus Logic EP93xx.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> .../bindings/arm/cirrus/ep9301-syscon.yaml | 59 ++++++++++++++++++++++
syscon goes to soc directory. Also, please add vendor prefix to the
filenames.
> .../devicetree/bindings/arm/cirrus/ep9301.yaml | 39 ++++++++++++++
> 2 files changed, 98 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/cirrus/ep9301-syscon.yaml b/Documentation/devicetree/bindings/arm/cirrus/ep9301-syscon.yaml
> new file mode 100644
> index 000000000000..77fbe1f006fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/cirrus/ep9301-syscon.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/cirrus/ep9301-syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic EP93xx Platforms System Controller
> +
> +maintainers:
> + - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> + - Nikita Shubin <nikita.shubin@maquefel.me>
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
> + - enum:
> + - cirrus,ep9302-syscon
> + - cirrus,ep9307-syscon
> + - cirrus,ep9312-syscon
> + - cirrus,ep9315-syscon
> + - const: cirrus,ep9301-syscon
> + - const: syscon
> + - const: simple-mfd
> + - items:
> + - const: cirrus,ep9301-syscon
> + - const: syscon
> + - const: simple-mfd
> +
> + reg:
> + maxItems: 1
> +
> + reboot:
> + type: object
> + properties:
> + compatible:
> + const: cirrus,ep9301-reboot
Patch introducing it should be before this one. Also, do not use
different styles for your child nodes. Your other nodes use $ref.
> +
> + clock-controller:
> + type: object
> + $ref: ../../clock/cirrus,ep9301-clk.yaml
Absolute path, so /schemas/clock/cirrus.....
> +
> + pinctrl:
> + type: object
> + $ref: ../../pinctrl/cirrus,ep9301-pinctrl.yaml
Ditto
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + syscon@80930000 {
> + compatible = "cirrus,ep9301-syscon",
> + "syscon", "simple-mfd";
> + reg = <0x80930000 0x1000>;
Incomplete example.
> + };
> diff --git a/Documentation/devicetree/bindings/arm/cirrus/ep9301.yaml b/Documentation/devicetree/bindings/arm/cirrus/ep9301.yaml
> new file mode 100644
> index 000000000000..6087784e93fb
> --- /dev/null
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 10/42] dt-bindings: rtc: Add Cirrus EP93xx
From: Krzysztof Kozlowski @ 2023-07-21 14:07 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-10-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This adds device tree bindings for the Cirrus Logic EP93xx
> RTC block used in these SoCs.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> .../devicetree/bindings/rtc/cirrus,ep9301-rtc.yaml | 40 ++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/rtc/cirrus,ep9301-rtc.yaml b/Documentation/devicetree/bindings/rtc/cirrus,ep9301-rtc.yaml
> new file mode 100644
> index 000000000000..63572c197e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/cirrus,ep9301-rtc.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/cirrus,ep9301-rtc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus EP93xx Real Time Clock controller
> +
> +maintainers:
> + - Hartley Sweeten <hsweeten@visionengravers.com>
> + - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
allOf: with $ref to rtc.yaml
> +properties:
> + compatible:
> + oneOf:
> + - const: cirrus,ep9301-rtc
> + - items:
> + - enum:
> + - cirrus,ep9302-rtc
> + - cirrus,ep9307-rtc
> + - cirrus,ep9312-rtc
> + - cirrus,ep9315-rtc
> + - const: cirrus,ep9301-rtc
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
and then unevaluatedProperties instead.
> +
> +examples:
> + - |
> + rtc@80920000 {
> + compatible = "cirrus,ep9301-rtc";
> + reg = <0x80920000 0x100>;
> + };
> +
Each of your file has a trailing blank line.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 12/42] dt-bindings: watchdog: Add Cirrus EP93x
From: Krzysztof Kozlowski @ 2023-07-21 14:08 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-12-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This adds device tree bindings for the Cirrus Logic EP93xx
Every patch:
Please do not use "This commit/patch", but imperative mood. See longer
explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 29/42] dt-bindings: rtc: Add ST M48T86
From: Krzysztof Kozlowski @ 2023-07-21 14:10 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-29-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add YAML bindings for ST M48T86 / Dallas DS12887 RTC.
>
This shouldn't really be part of this patchset. It's not part of your SoC.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 34/42] ARM: dts: add Cirrus EP93XX SoC .dtsi
From: Krzysztof Kozlowski @ 2023-07-21 14:12 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-34-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add support for Cirrus Logic EP93XX SoC's family.
>
> Co-developed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> arch/arm/boot/dts/cirrus/ep93xx.dtsi | 449 +++++++++++++++++++++++++++++++++++
> 1 file changed, 449 insertions(+)
>
> diff --git a/arch/arm/boot/dts/cirrus/ep93xx.dtsi b/arch/arm/boot/dts/cirrus/ep93xx.dtsi
> new file mode 100644
> index 000000000000..1e04f39d7b80
> --- /dev/null
> +++ b/arch/arm/boot/dts/cirrus/ep93xx.dtsi
> @@ -0,0 +1,449 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for Cirrus Logic systems EP93XX SoC
> + */
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/clock/cirrus,ep93xx-clock.h>
> +/ {
> + soc: soc {
> + compatible = "simple-bus";
> + ranges;
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + syscon: syscon@80930000 {
> + compatible = "cirrus,ep9301-syscon",
> + "syscon", "simple-mfd";
Fix alignment.
> + reg = <0x80930000 0x1000>;
> +
> + eclk: clock-controller {
> + compatible = "cirrus,ep9301-clk";
> + #clock-cells = <1>;
> + clocks = <&xtali>;
> + status = "okay";
Drop statuses when not needed.
> + };
> +
> + pinctrl: pinctrl {
> + compatible = "cirrus,ep9301-pinctrl";
> +
> + spi_default_pins: pins-spi {
> + function = "spi";
> + groups = "ssp";
> + };
> +
...
> +
> + keypad: keypad@800f0000 {
> + compatible = "cirrus,ep9307-keypad";
> + reg = <0x800f0000 0x0c>;
> + interrupt-parent = <&vic0>;
> + interrupts = <29>;
> + clocks = <&eclk EP93XX_CLK_KEYPAD>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&keypad_default_pins>;
> + linux,keymap =
No need for line break.
> + <KEY_UP>,
> + <KEY_DOWN>,
> + <KEY_VOLUMEDOWN>,
> + <KEY_HOME>,
> + <KEY_RIGHT>,
> + <KEY_LEFT>,
> + <KEY_ENTER>,
> + <KEY_VOLUMEUP>,
> + <KEY_F6>,
> + <KEY_F8>,
> + <KEY_F9>,
> + <KEY_F10>,
> + <KEY_F1>,
> + <KEY_F2>,
> + <KEY_F3>,
> + <KEY_POWER>;
> + };
> +
> + pwm0: pwm@80910000 {
> + compatible = "cirrus,ep9301-pwm";
> + reg = <0x80910000 0x10>;
> + clocks = <&eclk EP93XX_CLK_PWM>;
> + status = "disabled";
> + };
> +
> + pwm1: pwm@80910020 {
> + compatible = "cirrus,ep9301-pwm";
> + reg = <0x80910020 0x10>;
> + clocks = <&eclk EP93XX_CLK_PWM>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pwm1_default_pins>;
> + status = "disabled";
> + };
> +
> + rtc0: rtc@80920000 {
> + compatible = "cirrus,ep9301-rtc";
> + reg = <0x80920000 0x100>;
> + };
> +
> + spi0: spi@808a0000 {
> + compatible = "cirrus,ep9301-spi";
> + reg = <0x808a0000 0x18>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupt-parent = <&vic1>;
> + interrupts = <21>;
> + clocks = <&eclk EP93XX_CLK_SPI>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&spi_default_pins>;
> + status = "disabled";
> + };
> +
> + timer: timer@80810000 {
> + compatible = "cirrus,ep9301-timer";
> + reg = <0x80810000 0x100>;
> + interrupt-parent = <&vic1>;
> + interrupts = <19>;
> + };
> +
> + uart0: uart@808c0000 {
> + compatible = "arm,primecell";
This looks incomplete.
> + reg = <0x808c0000 0x1000>;
> + arm,primecell-periphid = <0x00041010>;
> + clocks = <&eclk EP93XX_CLK_UART1>, <&eclk EP93XX_CLK_UART>;
> + clock-names = "apb:uart1", "apb_pclk";
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
> + interrupt-parent = <&vic1>;
> + interrupts = <20>;
> + status = "disabled";
> + };
> +
> + uart1: uart@808d0000 {
> + compatible = "arm,primecell";
> + reg = <0x808d0000 0x1000>;
> + arm,primecell-periphid = <0x00041010>;
> + clocks = <&eclk EP93XX_CLK_UART2>, <&eclk EP93XX_CLK_UART>;
> + clock-names = "apb:uart2", "apb_pclk";
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
> + interrupt-parent = <&vic1>;
> + interrupts = <22>;
> + status = "disabled";
> + };
> +
> + uart2: uart@808b0000 {
> + compatible = "arm,primecell";
> + reg = <0x808b0000 0x1000>;
> + arm,primecell-periphid = <0x00041010>;
> + clocks = <&eclk EP93XX_CLK_UART3>, <&eclk EP93XX_CLK_UART>;
> + clock-names = "apb:uart3", "apb_pclk";
> + interrupt-parent = <&vic1>;
> + interrupts = <23>;
> + status = "disabled";
> + };
> +
> + usb0: usb@80020000 {
> + compatible = "generic-ohci";
> + reg = <0x80020000 0x10000>;
> + interrupt-parent = <&vic1>;
> + interrupts = <24>;
> + clocks = <&eclk EP93XX_CLK_USB>;
> + status = "disabled";
> + };
> +
> + watchdog0: watchdog@80940000 {
> + compatible = "cirrus,ep9301-wdt";
> + reg = <0x80940000 0x08>;
> + };
> + };
> +
> + xtali: oscillator {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <14745600>;
> + clock-output-names = "xtali";
> + };
> +
> + i2c0: i2c {
> + compatible = "i2c-gpio";
> + sda-gpios = <&gpio6 1 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> + scl-gpios = <&gpio6 0 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
Are you sure this is part of SoC? It is rather unusual... I would say
this would be the first SoC, where GPIO pins must be an I2C.
Best regards,
Krzysztof
^ 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