* Re: [v2] Input: iqs269a - Use scope-based resource management in iqs269_parse_chan()
From: Markus Elfring @ 2024-03-04 17:48 UTC (permalink / raw)
To: Dmitry Torokhov, Jeff LaBundy, linux-input, kernel-janitors
Cc: Mattijs Korpershoek, Rob Herring, Uwe Kleine-König,
ye xingchen, LKML, Jonathan Cameron
In-Reply-To: <ZeYBTUQRAp2u3bXX@google.com>
> The extra curly braces are absolutely not needed. The for loop's body
> already defines scope, __cleanup()s should be called at the end of the body.
I present an other development opinion here.
I got the impression that the required scope should be smaller for
the adjusted local variable “ev_node” (according to the previous function implementation).
Otherwise:
How do you think about to move any source code part from the loop
into a separate function?
Regards,
Markus
^ permalink raw reply
* Re: [v2] Input: iqs269a - Use scope-based resource management in iqs269_parse_chan()
From: Dmitry Torokhov @ 2024-03-04 18:59 UTC (permalink / raw)
To: Markus Elfring
Cc: Jeff LaBundy, linux-input, kernel-janitors, Mattijs Korpershoek,
Rob Herring, Uwe Kleine-König, ye xingchen, LKML,
Jonathan Cameron
In-Reply-To: <ea3b033a-7a50-4276-9839-f6335b754c30@web.de>
On Mon, Mar 04, 2024 at 06:48:58PM +0100, Markus Elfring wrote:
> > The extra curly braces are absolutely not needed. The for loop's body
> > already defines scope, __cleanup()s should be called at the end of the body.
>
> I present an other development opinion here.
> I got the impression that the required scope should be smaller for
> the adjusted local variable “ev_node” (according to the previous function implementation).
>
> Otherwise:
> How do you think about to move any source code part from the loop
> into a separate function?
No, it should simply look like this:
diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index cd14ff9f57cf..98119c48c65f 100644
--- a/drivers/input/misc/iqs269a.c
+++ b/drivers/input/misc/iqs269a.c
@@ -557,7 +557,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
const struct fwnode_handle *ch_node)
{
struct i2c_client *client = iqs269->client;
- struct fwnode_handle *ev_node;
struct iqs269_ch_reg *ch_reg;
u16 engine_a, engine_b;
unsigned int reg, val;
@@ -734,8 +733,9 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
}
for (i = 0; i < ARRAY_SIZE(iqs269_events); i++) {
- ev_node = fwnode_get_named_child_node(ch_node,
- iqs269_events[i].name);
+ struct fwnode_handle *ev_node __free(fwnode_handle) =
+ fwnode_get_named_child_node(ch_node,
+ iqs269_events[i].name);
if (!ev_node)
continue;
@@ -744,7 +744,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
dev_err(&client->dev,
"Invalid channel %u threshold: %u\n",
reg, val);
- fwnode_handle_put(ev_node);
return -EINVAL;
}
@@ -758,7 +757,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
dev_err(&client->dev,
"Invalid channel %u hysteresis: %u\n",
reg, val);
- fwnode_handle_put(ev_node);
return -EINVAL;
}
@@ -774,7 +772,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
}
error = fwnode_property_read_u32(ev_node, "linux,code", &val);
- fwnode_handle_put(ev_node);
if (error == -EINVAL) {
continue;
} else if (error) {
Thanks.
--
Dmitry
^ permalink raw reply related
* [PATCH] HID: usbhid: hid-pidff: fix initialisation for devices that pre-play effects
From: Jules Noirant @ 2024-03-04 19:57 UTC (permalink / raw)
Cc: jkosina, bentiss, jkosina, Jules Noirant, Jiri Kosina,
Benjamin Tissoires, linux-usb, linux-input, linux-kernel
Some PID compliant devices play effects outside of the usb drivers when you
power them, for example an autocenter spring. These effects are thus
re-triggered when you reset the device through the driver init sequence,
preventing other effects to be played in some cases.
This fix makes sure all the effects currently played are stopped after
resetting the device. More specifically, it brings compatibility to the
Brunner CLS-P joystick and potentially more of their products.
Signed-off-by: Jules Noirant <jules.noirant@orange.fr>
---
drivers/hid/usbhid/hid-pidff.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index 3b4ee21cd..aade18f9e 100644
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c
@@ -109,8 +109,9 @@ static const u8 pidff_pool[] = { 0x80, 0x83, 0xa9 };
/* Special field key tables used to put special field keys into arrays */
#define PID_ENABLE_ACTUATORS 0
-#define PID_RESET 1
-static const u8 pidff_device_control[] = { 0x97, 0x9a };
+#define PID_STOP_ALL_EFFECTS 1
+#define PID_RESET 2
+static const u8 pidff_device_control[] = { 0x97, 0x99, 0x9a };
#define PID_CONSTANT 0
#define PID_RAMP 1
@@ -1157,6 +1158,10 @@ static void pidff_reset(struct pidff_device *pidff)
hid_hw_request(hid, pidff->reports[PID_DEVICE_CONTROL], HID_REQ_SET_REPORT);
hid_hw_wait(hid);
+ pidff->device_control->value[0] = pidff->control_id[PID_STOP_ALL_EFFECTS];
+ hid_hw_request(hid, pidff->reports[PID_DEVICE_CONTROL], HID_REQ_SET_REPORT);
+ hid_hw_wait(hid);
+
pidff->device_control->value[0] =
pidff->control_id[PID_ENABLE_ACTUATORS];
hid_hw_request(hid, pidff->reports[PID_DEVICE_CONTROL], HID_REQ_SET_REPORT);
--
2.40.1
^ permalink raw reply related
* Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks
From: Javier Carrasco @ 2024-03-04 20:21 UTC (permalink / raw)
To: Takashi Iwai, Javier Carrasco
Cc: Dmitry Torokhov, linux-input, linux-kernel, regressions,
Henrik Rydberg, John Horan
In-Reply-To: <874jdm17yt.wl-tiwai@suse.de>
On 04.03.24 13:45, Takashi Iwai wrote:
> On Mon, 04 Mar 2024 12:26:48 +0100,
> Javier Carrasco wrote:
>>
>> On 04.03.24 09:35, Takashi Iwai wrote:
>>> Hi,
>>>
>>> we've received a few regression reports for openSUSE Leap about the
>>> missing touchpad on Macbooks. After debugging, this turned out to be
>>> the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
>>> Input: bcm5974 - check endpoint type before starting traffic
>>>
>>> And, the same regression was confirmed on the upstream 6.8-rc6
>>> kernel.
>>>
>>> Reverting the commit above fixes the problem, the touchpad reappears.
>>>
>>> The detailed hardware info is found at:
>>> https://bugzilla.suse.com/show_bug.cgi?id=1220030
>>>
>>> Feel free to join the bugzilla above, or let me know if you need
>>> something for debugging, then I'll delegate on the bugzilla.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>
>> Hi Takashi,
>>
>> The commit adds a check to ensure that the endpoint type is interrupt.
>>
>> According to that report, the issue arose with a MacBook Pro 5.1 (no
>> button, only trackpad endpoint), so the check on the tp_ep address
>> (0x81) returns false. I assume that you see an error message
>> ("Unexpected non-int endpoint) and the probe function fails returning
>> -ENODEV.
>
> Right, there is the message.
>
>> Do you see any warning in the logs when you revert the commit? It was
>> added to prevent using wrong endpoint types, which will display the
>> following warning: "BOGUS urb xfer, pipe "some_number" != type
>> "another_number""
>
> The revert was tested on the downstream kernel, but it has also the
> check of bogus pipe, and there was no such warning, as far as I see
> the report.
>
>> I am just wondering if for some reason the check on interrupt type is
>> wrong here.
>
> I'll ask reporters to give the lsusb -v output so that we can take a
> deeper look. Also, I'm building a test kernel based on 6.8-rc7 with
> the revert, and ask reporters to test with it, just to be sure.
>
>
> thanks,
>
> Takashi
I retrieved the relevant node from the report that was uploaded a few
minutes ago:
Bus 003 Device 003: ID 05ac:0237 Apple, Inc. Internal Keyboard/Trackpad
(ISO)
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 0
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize0 8
idVendor 0x05ac Apple, Inc.
idProduct 0x0237 Internal Keyboard/Trackpad (ISO)
bcdDevice 0.77
iManufacturer 1 Apple, Inc.
iProduct 2 Apple Internal Keyboard / Trackpad
iSerial 0
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 0x0054
bNumInterfaces 3
bConfigurationValue 1
iConfiguration 0
bmAttributes 0xa0
(Bus Powered)
Remote Wakeup
MaxPower 40mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 3 Human Interface Device
bInterfaceSubClass 1 Boot Interface Subclass
bInterfaceProtocol 1 Keyboard
iInterface 3 Apple Internal Keyboard
HID Device Descriptor:
bLength 9
bDescriptorType 33
bcdHID 1.11
bCountryCode 13 International (ISO)
bNumDescriptors 1
bDescriptorType 34 Report
wDescriptorLength 156
Report Descriptors:
** UNAVAILABLE **
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x000a 1x 10 bytes
bInterval 8
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 3 Human Interface Device
bInterfaceSubClass 0
bInterfaceProtocol 0
iInterface 4 Touchpad
HID Device Descriptor:
bLength 9
bDescriptorType 33
bcdHID 1.11
bCountryCode 0 Not supported
bNumDescriptors 1
bDescriptorType 34 Report
wDescriptorLength 27
Report Descriptors:
** UNAVAILABLE **
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 2
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 2
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 3 Human Interface Device
bInterfaceSubClass 1 Boot Interface Subclass
bInterfaceProtocol 2 Mouse
iInterface 4 Touchpad
HID Device Descriptor:
bLength 9
bDescriptorType 33
bcdHID 1.11
bCountryCode 0 Not supported
bNumDescriptors 1
bDescriptorType 34 Report
wDescriptorLength 52
Report Descriptors:
** UNAVAILABLE **
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0008 1x 8 bytes
bInterval 8
Device Status: 0x0000
(Bus Powered)
There is indeed an interrupt endpoint with address 0x81, but the driver
defines bInterfaceProtocol = 2 (Mouse), and the endpoint in that
interface is 0x84:
#define BCM5974_DEVICE(prod) { \
.match_flags = (USB_DEVICE_ID_MATCH_DEVICE | \
USB_DEVICE_ID_MATCH_INT_CLASS | \
USB_DEVICE_ID_MATCH_INT_PROTOCOL), \
.idVendor = USB_VENDOR_ID_APPLE, \
.idProduct = (prod), \
.bInterfaceClass = USB_INTERFACE_CLASS_HID, \
.bInterfaceProtocol = USB_INTERFACE_PROTOCOL_MOUSE \
}
where USB_INTERFACE_PROTOCOL_MOUSE = 2.
My interpretation is that the driver is checking if the endpoint with
address 0x81 form the interface with bInterfaceProtocol = 2 (that is the
last interface of the list, the one with bInterfaceNumber = 2), but it
is not found, because its only endpoint has a different address (0x84).
Interestingly, 0x84 is the address given to the endpoint of the button
interface. The button interface should not be relevant for Macbook 5,1
(TYPE 2 in the driver), according to 43f482b48d03 ("Input: bcm5974 -
only setup button urb for TYPE1 devices").
If that is true, does anyone know why bInterfaceProtocol is always set
to USB_INTERFACE_PROTOCOL_MOUSE, and why the driver works anyway with
bEndpointAddress = 0x81 for the trackpad? The urb setup for 0x84 is only
executed for TYPE 1 devices, and the mouse interface does not have an
endpoint with address 0x81. Or am I missing something?
We could revert the patch in question, but I see no reason why checking
an expected interrupt endpoint should cause trouble. It looks like there
is something fishy going on.
Best regards,
Javier Carrasco
^ permalink raw reply
* Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
From: Karel Balej @ 2024-03-04 20:28 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Liam Girdwood, Mark Brown, devicetree, linux-kernel, linux-input,
Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <ZeTgEmjJc_VhYpLm@google.com>
Dmitry,
Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> > From: Karel Balej <balejk@matfyz.cz>
> >
> > Marvell 88PM886 PMIC provides onkey among other things. Add client
> > driver to handle it. The driver currently only provides a basic support
> > omitting additional functions found in the vendor version, such as long
> > onkey and GPIO integration.
> >
> > Signed-off-by: Karel Balej <balejk@matfyz.cz>
> > ---
> >
> > Notes:
> > RFC v3:
> > - Drop wakeup-source.
> > RFC v2:
> > - Address Dmitry's feedback:
> > - Sort includes alphabetically.
> > - Drop onkey->irq.
> > - ret -> err in irq_handler and no initialization.
> > - Break long lines and other formatting.
> > - Do not clobber platform_get_irq error.
> > - Do not set device parent manually.
> > - Use input_set_capability.
> > - Use the wakeup-source DT property.
> > - Drop of_match_table.
>
> I only said that you should not be using of_match_ptr(), but you still
> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> proper module loading support.
I removed of_match_table because I no longer need compatible for this --
there are no device tree properties and the driver is being instantiated
by the MFD driver.
Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
compiled as module? If that is the case, given what I write above, am I
correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
to use here?
Thank you, kind regards,
K. B.
^ permalink raw reply
* Re: [PATCH] Input: xpad - Add additional HyperX Controller Identifiers
From: Nguyen, Max @ 2024-03-05 0:43 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <ZeT9cbHRVe7td8WB@google.com>
On 3/3/2024 2:45 PM, Dmitry Torokhov wrote:
> Hi Max,
>
> On Fri, Mar 01, 2024 at 06:15:42PM -0800, Nguyen, Max wrote:
>> Add additional HyperX device identifiers to xpad_device and xpad_table. Cc:
>> stable@vger.kernel.org Suggested-by: Chris Toledanes<chris.toledanes@hp.com>
>> Reviewed-by: Carl Ng<carl.ng@hp.com>
>> Signed-off-by: Max Nguyen<maxwell.nguyen@hp.com>
> Your client messes up patches :(
Sorry about that... I tried to submit through a different client. I will submit through my previous method next time.
>
>> ---
>>
>> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
>> index 7c4b2a5cc1b5..a7e001a1f6dc 100644
>> --- a/drivers/input/joystick/xpad.c
>> +++ b/drivers/input/joystick/xpad.c
>> @@ -131,6 +131,11 @@ static const struct xpad_device {
>> { 0x03eb, 0xff01, "Wooting One (Legacy)", 0, XTYPE_XBOX360 },
>> { 0x03eb, 0xff02, "Wooting Two (Legacy)", 0, XTYPE_XBOX360 },
>> { 0x03f0, 0x0495, "HyperX Clutch Gladiate", 0, XTYPE_XBOXONE },
>> + { 0x03f0, 0x08B6, "HyperX Clutch Gladiate", 0, XTYPE_XBOXONE }, /* v2 */
>> + { 0x03f0, 0x07A0, "HyperX Clutch Gladiate RGB", 0, XTYPE_XBOXONE },
>> + { 0x03f0, 0x038D, "HyperX Clutch", 0, XTYPE_XBOX360 }, /* wired */
>> + { 0x03f0, 0x048D, "HyperX Clutch", 0, XTYPE_XBOX360 }, /* wireless */
>> + { 0x03f0, 0x09B4, "HyperX Clutch Tanto", 0, XTYPE_XBOXONE },
> These need to be sorted by VID/PID.
>
> I fixed it up and applied.
>
> Thanks.
Can you help explain the sort? Do the VID/PIDs need to be in sequential order?
^ permalink raw reply
* Re: [PATCH] Input: xpad - Add additional HyperX Controller Identifiers
From: Dmitry Torokhov @ 2024-03-05 1:04 UTC (permalink / raw)
To: Nguyen, Max; +Cc: linux-input
In-Reply-To: <8a2e32ad-abbe-4459-ac99-c0b8e8e2cdef@gmail.com>
On Mon, Mar 04, 2024 at 04:43:10PM -0800, Nguyen, Max wrote:
>
> On 3/3/2024 2:45 PM, Dmitry Torokhov wrote:
> > Hi Max,
> >
> > On Fri, Mar 01, 2024 at 06:15:42PM -0800, Nguyen, Max wrote:
> > > Add additional HyperX device identifiers to xpad_device and xpad_table. Cc:
> > > stable@vger.kernel.org Suggested-by: Chris Toledanes<chris.toledanes@hp.com>
> > > Reviewed-by: Carl Ng<carl.ng@hp.com>
> > > Signed-off-by: Max Nguyen<maxwell.nguyen@hp.com>
> > Your client messes up patches :(
>
> Sorry about that... I tried to submit through a different client. I will submit through my previous method next time.
>
> >
> > > ---
> > >
> > > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > > index 7c4b2a5cc1b5..a7e001a1f6dc 100644
> > > --- a/drivers/input/joystick/xpad.c
> > > +++ b/drivers/input/joystick/xpad.c
> > > @@ -131,6 +131,11 @@ static const struct xpad_device {
> > > { 0x03eb, 0xff01, "Wooting One (Legacy)", 0, XTYPE_XBOX360 },
> > > { 0x03eb, 0xff02, "Wooting Two (Legacy)", 0, XTYPE_XBOX360 },
> > > { 0x03f0, 0x0495, "HyperX Clutch Gladiate", 0, XTYPE_XBOXONE },
> > > + { 0x03f0, 0x08B6, "HyperX Clutch Gladiate", 0, XTYPE_XBOXONE }, /* v2 */
> > > + { 0x03f0, 0x07A0, "HyperX Clutch Gladiate RGB", 0, XTYPE_XBOXONE },
> > > + { 0x03f0, 0x038D, "HyperX Clutch", 0, XTYPE_XBOX360 }, /* wired */
> > > + { 0x03f0, 0x048D, "HyperX Clutch", 0, XTYPE_XBOX360 }, /* wireless */
> > > + { 0x03f0, 0x09B4, "HyperX Clutch Tanto", 0, XTYPE_XBOXONE },
> > These need to be sorted by VID/PID.
> >
> > I fixed it up and applied.
> >
> > Thanks.
>
> Can you help explain the sort? Do the VID/PIDs need to be in sequential order?
Yes, to easier see if someone is adding a duplicate, potentially with a
[slightly] different name. So 0x03f0, 0x07A0 should come before 0x03f0,
0x08B6, etc.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
From: Dmitry Torokhov @ 2024-03-05 1:10 UTC (permalink / raw)
To: Karel Balej
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Liam Girdwood, Mark Brown, devicetree, linux-kernel, linux-input,
Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <CZL8ZSZAVEBI.349BV2Y6AKIPN@gimli.ms.mff.cuni.cz>
On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> Dmitry,
>
> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> > On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> > > From: Karel Balej <balejk@matfyz.cz>
> > >
> > > Marvell 88PM886 PMIC provides onkey among other things. Add client
> > > driver to handle it. The driver currently only provides a basic support
> > > omitting additional functions found in the vendor version, such as long
> > > onkey and GPIO integration.
> > >
> > > Signed-off-by: Karel Balej <balejk@matfyz.cz>
> > > ---
> > >
> > > Notes:
> > > RFC v3:
> > > - Drop wakeup-source.
> > > RFC v2:
> > > - Address Dmitry's feedback:
> > > - Sort includes alphabetically.
> > > - Drop onkey->irq.
> > > - ret -> err in irq_handler and no initialization.
> > > - Break long lines and other formatting.
> > > - Do not clobber platform_get_irq error.
> > > - Do not set device parent manually.
> > > - Use input_set_capability.
> > > - Use the wakeup-source DT property.
> > > - Drop of_match_table.
> >
> > I only said that you should not be using of_match_ptr(), but you still
> > need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> > proper module loading support.
>
> I removed of_match_table because I no longer need compatible for this --
> there are no device tree properties and the driver is being instantiated
> by the MFD driver.
>
> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> compiled as module? If that is the case, given what I write above, am I
> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> to use here?
Yes, if uevent generated for the device is "platform:<name>" then
MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
sets it up (OF modalias or platform), but you should be able to check
the format looking at the "uevent" attribute for your device in sysfs
(/sys/devices/bus/platform/...).
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks
From: Dmitry Torokhov @ 2024-03-05 1:15 UTC (permalink / raw)
To: Javier Carrasco
Cc: Takashi Iwai, Javier Carrasco, linux-input, linux-kernel,
regressions, Henrik Rydberg, John Horan
In-Reply-To: <449417ca-aae1-4868-a96f-a99ac5d187d6@gmail.com>
On Mon, Mar 04, 2024 at 09:21:19PM +0100, Javier Carrasco wrote:
>
> There is indeed an interrupt endpoint with address 0x81, but the driver
> defines bInterfaceProtocol = 2 (Mouse), and the endpoint in that
> interface is 0x84:
>
> #define BCM5974_DEVICE(prod) { \
> .match_flags = (USB_DEVICE_ID_MATCH_DEVICE | \
> USB_DEVICE_ID_MATCH_INT_CLASS | \
> USB_DEVICE_ID_MATCH_INT_PROTOCOL), \
> .idVendor = USB_VENDOR_ID_APPLE, \
> .idProduct = (prod), \
> .bInterfaceClass = USB_INTERFACE_CLASS_HID, \
> .bInterfaceProtocol = USB_INTERFACE_PROTOCOL_MOUSE \
> }
>
> where USB_INTERFACE_PROTOCOL_MOUSE = 2.
>
>
> My interpretation is that the driver is checking if the endpoint with
> address 0x81 form the interface with bInterfaceProtocol = 2 (that is the
> last interface of the list, the one with bInterfaceNumber = 2), but it
> is not found, because its only endpoint has a different address (0x84).
>
> Interestingly, 0x84 is the address given to the endpoint of the button
> interface. The button interface should not be relevant for Macbook 5,1
> (TYPE 2 in the driver), according to 43f482b48d03 ("Input: bcm5974 -
> only setup button urb for TYPE1 devices").
>
> If that is true, does anyone know why bInterfaceProtocol is always set
> to USB_INTERFACE_PROTOCOL_MOUSE, and why the driver works anyway with
> bEndpointAddress = 0x81 for the trackpad? The urb setup for 0x84 is only
> executed for TYPE 1 devices, and the mouse interface does not have an
> endpoint with address 0x81. Or am I missing something?
The driver is naughty, it binds to the 3rd interface (bInterfaceNumber
2) but actually pokes into the 2nd interface with endpoint 0x84 without
actually claiming it. Your check expects that the endpoint belongs to
the interface that the driver binds to and thus fails.
>
> We could revert the patch in question, but I see no reason why checking
> an expected interrupt endpoint should cause trouble. It looks like there
> is something fishy going on.
Yes, the driver needs to claim both interfaces and when checking use the
right one. I will revert the patch for now given that it causes
regression and we can try fixing it again.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] Revert "Input: bcm5974 - check endpoint type before starting traffic"
From: Javier Carrasco @ 2024-03-05 5:52 UTC (permalink / raw)
To: Henrik Rydberg, Dmitry Torokhov
Cc: linux-input, linux-kernel, regressions, Javier Carrasco
This patch intended to fix an well-knonw issue in old drivers where the
endpoint type is taken for granted, which is often triggered by fuzzers.
That was the case for this driver [1], and although the fix seems to be
correct, it uncovered another issue that leads to a regression [2] if
the endpoints of the current interface are checked. The driver makes use
of endpoints that belong to a different interface rather than the one it
binds (it binds to the third interface, but also accesses an endpoint
from a different one). The driver should claim the interfaces it
requires, but that is still not the case.
Given that the regression is more severe than the issue found by
syzkaller, the best approach is reverting the patch that causes the
regression, and trying to fix the underlying problem before checking
the endpoint types again.
Note that reverting this patch will probably trigger the syzkaller bug
at some point.
[1] https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622
[2] https://lore.kernel.org/linux-input/ab9d758c-3ce9-42f6-99af-877055a589e6@leemhuis.info/T/#t
This reverts commit 2b9c3eb32a699acdd4784d6b93743271b4970899.
Fixes: b516b1b0dfcc ("Revert "Input: bcm5974 - check endpoint type before starting traffic"")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
drivers/input/mouse/bcm5974.c | 20 --------------------
1 file changed, 20 deletions(-)
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 953992b458e9..ca150618d32f 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -19,7 +19,6 @@
* Copyright (C) 2006 Nicolas Boichat (nicolas@boichat.ch)
*/
-#include "linux/usb.h"
#include <linux/kernel.h>
#include <linux/errno.h>
#include <linux/slab.h>
@@ -194,8 +193,6 @@ enum tp_type {
/* list of device capability bits */
#define HAS_INTEGRATED_BUTTON 1
-/* maximum number of supported endpoints (currently trackpad and button) */
-#define MAX_ENDPOINTS 2
/* trackpad finger data block size */
#define FSIZE_TYPE1 (14 * sizeof(__le16))
@@ -894,18 +891,6 @@ static int bcm5974_resume(struct usb_interface *iface)
return error;
}
-static bool bcm5974_check_endpoints(struct usb_interface *iface,
- const struct bcm5974_config *cfg)
-{
- u8 ep_addr[MAX_ENDPOINTS + 1] = {0};
-
- ep_addr[0] = cfg->tp_ep;
- if (cfg->tp_type == TYPE1)
- ep_addr[1] = cfg->bt_ep;
-
- return usb_check_int_endpoints(iface, ep_addr);
-}
-
static int bcm5974_probe(struct usb_interface *iface,
const struct usb_device_id *id)
{
@@ -918,11 +903,6 @@ static int bcm5974_probe(struct usb_interface *iface,
/* find the product index */
cfg = bcm5974_get_config(udev);
- if (!bcm5974_check_endpoints(iface, cfg)) {
- dev_err(&iface->dev, "Unexpected non-int endpoint\n");
- return -ENODEV;
- }
-
/* allocate memory for our device state and initialize it */
dev = kzalloc(sizeof(struct bcm5974), GFP_KERNEL);
input_dev = input_allocate_device();
---
base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
change-id: 20240305-revert_bcm5974_ep_check-37f2a6ab2714
Best regards,
--
Javier Carrasco <javier.carrasco.cruz@gmail.com>
^ permalink raw reply related
* Re: [PATCH] Revert "Input: bcm5974 - check endpoint type before starting traffic"
From: Linux regression tracking (Thorsten Leemhuis) @ 2024-03-05 6:20 UTC (permalink / raw)
To: Javier Carrasco, Henrik Rydberg, Dmitry Torokhov
Cc: linux-input, linux-kernel, regressions
In-Reply-To: <20240305-revert_bcm5974_ep_check-v1-1-db4f0422588f@gmail.com>
On 05.03.24 06:52, Javier Carrasco wrote:
> This patch intended to fix an well-knonw issue in old drivers where the
> endpoint type is taken for granted, which is often triggered by fuzzers.
>
> That was the case for this driver [1], and although the fix seems to be
> correct, it uncovered another issue that leads to a regression [2] if
> the endpoints of the current interface are checked. The driver makes use
> of endpoints that belong to a different interface rather than the one it
> binds (it binds to the third interface, but also accesses an endpoint
> from a different one). The driver should claim the interfaces it
> requires, but that is still not the case.
>
> Given that the regression is more severe than the issue found by
> syzkaller, the best approach is reverting the patch that causes the
> regression, and trying to fix the underlying problem before checking
> the endpoint types again.
>
> Note that reverting this patch will probably trigger the syzkaller bug
> at some point.
>> [1] https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622
> [2] https://lore.kernel.org/linux-input/ab9d758c-3ce9-42f6-99af-877055a589e6@leemhuis.info/T/#t
FWIW, these should be Link: or Closes tags, as explained in the docs.
And there is a better lore link. And there is the bugzilla entry as well.
Hence...
> This reverts commit 2b9c3eb32a699acdd4784d6b93743271b4970899.
>
> Fixes: b516b1b0dfcc ("Revert "Input: bcm5974 - check endpoint type before starting traffic"")
you might want to add them here like this:
Link: https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622 [1]
Link: https://lore.kernel.org/linux-input/87sf161jjc.wl-tiwai@suse.de/ [2]
Link: https://bugzilla.suse.com/show_bug.cgi?id=1220030
In an ideal world we also would add a "Reported-by: Jacopo Radice" before
the bugzilla line, as the details in the suse bugtracker apparently is
public, but it's likely better to not go down that path.
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Ciao, Thorsten
^ permalink raw reply
* Re: [PATCH] Revert "Input: bcm5974 - check endpoint type before starting traffic"
From: Javier Carrasco @ 2024-03-05 6:26 UTC (permalink / raw)
To: Linux regressions mailing list, Henrik Rydberg, Dmitry Torokhov
Cc: linux-input, linux-kernel
In-Reply-To: <1c13f4f7-fa02-4864-8621-bfd738546fc2@leemhuis.info>
On 05.03.24 07:20, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 05.03.24 06:52, Javier Carrasco wrote:
>> This patch intended to fix an well-knonw issue in old drivers where the
>> endpoint type is taken for granted, which is often triggered by fuzzers.
>>
>> That was the case for this driver [1], and although the fix seems to be
>> correct, it uncovered another issue that leads to a regression [2] if
>> the endpoints of the current interface are checked. The driver makes use
>> of endpoints that belong to a different interface rather than the one it
>> binds (it binds to the third interface, but also accesses an endpoint
>> from a different one). The driver should claim the interfaces it
>> requires, but that is still not the case.
>>
>> Given that the regression is more severe than the issue found by
>> syzkaller, the best approach is reverting the patch that causes the
>> regression, and trying to fix the underlying problem before checking
>> the endpoint types again.
>>
>> Note that reverting this patch will probably trigger the syzkaller bug
>> at some point.
>>> [1] https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622
>> [2] https://lore.kernel.org/linux-input/ab9d758c-3ce9-42f6-99af-877055a589e6@leemhuis.info/T/#t
>
> FWIW, these should be Link: or Closes tags, as explained in the docs.
> And there is a better lore link. And there is the bugzilla entry as well.
> Hence...
>
>> This reverts commit 2b9c3eb32a699acdd4784d6b93743271b4970899.
>>
>> Fixes: b516b1b0dfcc ("Revert "Input: bcm5974 - check endpoint type before starting traffic"")
>
> you might want to add them here like this:
>
> Link: https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622 [1]
> Link: https://lore.kernel.org/linux-input/87sf161jjc.wl-tiwai@suse.de/ [2]
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1220030
>
> In an ideal world we also would add a "Reported-by: Jacopo Radice" before
> the bugzilla line, as the details in the suse bugtracker apparently is
> public, but it's likely better to not go down that path.
>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> Ciao, Thorsten
Thanks for your comments, I will apply those changes right away and add
the "Repported-by:" tag as well.
Best regards,
Javier Carrasco
^ permalink raw reply
* [PATCH v2] Revert "Input: bcm5974 - check endpoint type before starting traffic"
From: Javier Carrasco @ 2024-03-05 6:38 UTC (permalink / raw)
To: Henrik Rydberg, Dmitry Torokhov
Cc: linux-input, linux-kernel, regressions, Jacopo Radice,
Javier Carrasco
This patch intended to fix an well-knonw issue in old drivers where the
endpoint type is taken for granted, which is often triggered by fuzzers.
That was the case for this driver [1], and although the fix seems to be
correct, it uncovered another issue that leads to a regression [2], if
the endpoints of the current interface are checked.
The driver makes use of endpoints that belong to a different interface
rather than the one it binds (it binds to the third interface, but also
accesses an endpoint from a different one). The driver should claim the
interfaces it requires, but that is still not the case.
Given that the regression is more severe than the issue found by
syzkaller, the best approach is reverting the patch that causes the
regression, and trying to fix the underlying problem before checking
the endpoint types again.
Note that reverting this patch will probably trigger the syzkaller bug
at some point.
This reverts commit 2b9c3eb32a699acdd4784d6b93743271b4970899.
Link: https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622 [1]
Link: https://lore.kernel.org/linux-input/87sf161jjc.wl-tiwai@suse.de/ [2]
Fixes: b516b1b0dfcc ("Revert "Input: bcm5974 - check endpoint type before starting traffic"")
Reported-by: Jacopo Radice <jacopo.radice@outlook.com>
Closes: https://bugzilla.suse.com/show_bug.cgi?id=1220030
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Changes in v2:
- Add "Reported-by", "Closes" and "Link" tags.
- Use shorter lore link.
- Link to v1: https://lore.kernel.org/r/20240305-revert_bcm5974_ep_check-v1-1-db4f0422588f@gmail.com
---
drivers/input/mouse/bcm5974.c | 20 --------------------
1 file changed, 20 deletions(-)
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 953992b458e9..ca150618d32f 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -19,7 +19,6 @@
* Copyright (C) 2006 Nicolas Boichat (nicolas@boichat.ch)
*/
-#include "linux/usb.h"
#include <linux/kernel.h>
#include <linux/errno.h>
#include <linux/slab.h>
@@ -194,8 +193,6 @@ enum tp_type {
/* list of device capability bits */
#define HAS_INTEGRATED_BUTTON 1
-/* maximum number of supported endpoints (currently trackpad and button) */
-#define MAX_ENDPOINTS 2
/* trackpad finger data block size */
#define FSIZE_TYPE1 (14 * sizeof(__le16))
@@ -894,18 +891,6 @@ static int bcm5974_resume(struct usb_interface *iface)
return error;
}
-static bool bcm5974_check_endpoints(struct usb_interface *iface,
- const struct bcm5974_config *cfg)
-{
- u8 ep_addr[MAX_ENDPOINTS + 1] = {0};
-
- ep_addr[0] = cfg->tp_ep;
- if (cfg->tp_type == TYPE1)
- ep_addr[1] = cfg->bt_ep;
-
- return usb_check_int_endpoints(iface, ep_addr);
-}
-
static int bcm5974_probe(struct usb_interface *iface,
const struct usb_device_id *id)
{
@@ -918,11 +903,6 @@ static int bcm5974_probe(struct usb_interface *iface,
/* find the product index */
cfg = bcm5974_get_config(udev);
- if (!bcm5974_check_endpoints(iface, cfg)) {
- dev_err(&iface->dev, "Unexpected non-int endpoint\n");
- return -ENODEV;
- }
-
/* allocate memory for our device state and initialize it */
dev = kzalloc(sizeof(struct bcm5974), GFP_KERNEL);
input_dev = input_allocate_device();
---
base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
change-id: 20240305-revert_bcm5974_ep_check-37f2a6ab2714
Best regards,
--
Javier Carrasco <javier.carrasco.cruz@gmail.com>
^ permalink raw reply related
* Re: [PATCH v2] Revert "Input: bcm5974 - check endpoint type before starting traffic"
From: Dmitry Torokhov @ 2024-03-05 7:41 UTC (permalink / raw)
To: Javier Carrasco, Henrik Rydberg
Cc: linux-input, linux-kernel, regressions, Jacopo Radice
In-Reply-To: <20240305-revert_bcm5974_ep_check-v2-1-925ae9b188d9@gmail.com>
On March 4, 2024 10:38:33 PM PST, Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
>This patch intended to fix an well-knonw issue in old drivers where the
>endpoint type is taken for granted, which is often triggered by fuzzers.
>
>That was the case for this driver [1], and although the fix seems to be
>correct, it uncovered another issue that leads to a regression [2], if
>the endpoints of the current interface are checked.
>
>The driver makes use of endpoints that belong to a different interface
>rather than the one it binds (it binds to the third interface, but also
>accesses an endpoint from a different one). The driver should claim the
>interfaces it requires, but that is still not the case.
>
>Given that the regression is more severe than the issue found by
>syzkaller, the best approach is reverting the patch that causes the
>regression, and trying to fix the underlying problem before checking
>the endpoint types again.
>
>Note that reverting this patch will probably trigger the syzkaller bug
>at some point.
>
>This reverts commit 2b9c3eb32a699acdd4784d6b93743271b4970899.
>
>Link: https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622 [1]
>Link: https://lore.kernel.org/linux-input/87sf161jjc.wl-tiwai@suse.de/ [2]
>
>Fixes: b516b1b0dfcc ("Revert "Input: bcm5974 - check endpoint type before starting traffic"")
This "fixes" tag looks incorrect. The patch fixes itself?
>Reported-by: Jacopo Radice <jacopo.radice@outlook.com>
>Closes: https://bugzilla.suse.com/show_bug.cgi?id=1220030
>Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>---
>Changes in v2:
>- Add "Reported-by", "Closes" and "Link" tags.
>- Use shorter lore link.
>- Link to v1: https://lore.kernel.org/r/20240305-revert_bcm5974_ep_check-v1-1-db4f0422588f@gmail.com
>---
> drivers/input/mouse/bcm5974.c | 20 --------------------
> 1 file changed, 20 deletions(-)
>
>diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
>index 953992b458e9..ca150618d32f 100644
>--- a/drivers/input/mouse/bcm5974.c
>+++ b/drivers/input/mouse/bcm5974.c
>@@ -19,7 +19,6 @@
> * Copyright (C) 2006 Nicolas Boichat (nicolas@boichat.ch)
> */
>
>-#include "linux/usb.h"
> #include <linux/kernel.h>
> #include <linux/errno.h>
> #include <linux/slab.h>
>@@ -194,8 +193,6 @@ enum tp_type {
>
> /* list of device capability bits */
> #define HAS_INTEGRATED_BUTTON 1
>-/* maximum number of supported endpoints (currently trackpad and button) */
>-#define MAX_ENDPOINTS 2
>
> /* trackpad finger data block size */
> #define FSIZE_TYPE1 (14 * sizeof(__le16))
>@@ -894,18 +891,6 @@ static int bcm5974_resume(struct usb_interface *iface)
> return error;
> }
>
>-static bool bcm5974_check_endpoints(struct usb_interface *iface,
>- const struct bcm5974_config *cfg)
>-{
>- u8 ep_addr[MAX_ENDPOINTS + 1] = {0};
>-
>- ep_addr[0] = cfg->tp_ep;
>- if (cfg->tp_type == TYPE1)
>- ep_addr[1] = cfg->bt_ep;
>-
>- return usb_check_int_endpoints(iface, ep_addr);
>-}
>-
> static int bcm5974_probe(struct usb_interface *iface,
> const struct usb_device_id *id)
> {
>@@ -918,11 +903,6 @@ static int bcm5974_probe(struct usb_interface *iface,
> /* find the product index */
> cfg = bcm5974_get_config(udev);
>
>- if (!bcm5974_check_endpoints(iface, cfg)) {
>- dev_err(&iface->dev, "Unexpected non-int endpoint\n");
>- return -ENODEV;
>- }
>-
> /* allocate memory for our device state and initialize it */
> dev = kzalloc(sizeof(struct bcm5974), GFP_KERNEL);
> input_dev = input_allocate_device();
>
>---
>base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
>change-id: 20240305-revert_bcm5974_ep_check-37f2a6ab2714
>
>Best regards,
--
Dmitry
^ permalink raw reply
* [PATCH v3] Revert "Input: bcm5974 - check endpoint type before starting traffic"
From: Javier Carrasco @ 2024-03-05 7:49 UTC (permalink / raw)
To: Henrik Rydberg, Dmitry Torokhov
Cc: linux-input, linux-kernel, regressions, Jacopo Radice,
Javier Carrasco
This patch intended to fix an well-knonw issue in old drivers where the
endpoint type is taken for granted, which is often triggered by fuzzers.
That was the case for this driver [1], and although the fix seems to be
correct, it uncovered another issue that leads to a regression [2], if
the endpoints of the current interface are checked.
The driver makes use of endpoints that belong to a different interface
rather than the one it binds (it binds to the third interface, but also
accesses an endpoint from a different one). The driver should claim the
interfaces it requires, but that is still not the case.
Given that the regression is more severe than the issue found by
syzkaller, the best approach is reverting the patch that causes the
regression, and trying to fix the underlying problem before checking
the endpoint types again.
Note that reverting this patch will probably trigger the syzkaller bug
at some point.
This reverts commit 2b9c3eb32a699acdd4784d6b93743271b4970899.
Link: https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622 [1]
Link: https://lore.kernel.org/linux-input/87sf161jjc.wl-tiwai@suse.de/ [2]
Fixes: 2b9c3eb32a69 ("Input: bcm5974 - check endpoint type before starting traffic")
Reported-by: Jacopo Radice <jacopo.radice@outlook.com>
Closes: https://bugzilla.suse.com/show_bug.cgi?id=1220030
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Changes in v3:
- Fix "Fixes:" tag.
- Link to v2: https://lore.kernel.org/r/20240305-revert_bcm5974_ep_check-v2-1-925ae9b188d9@gmail.com
Changes in v2:
- Add "Reported-by", "Closes" and "Link" tags.
- Use shorter lore link.
- Link to v1: https://lore.kernel.org/r/20240305-revert_bcm5974_ep_check-v1-1-db4f0422588f@gmail.com
---
drivers/input/mouse/bcm5974.c | 20 --------------------
1 file changed, 20 deletions(-)
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 953992b458e9..ca150618d32f 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -19,7 +19,6 @@
* Copyright (C) 2006 Nicolas Boichat (nicolas@boichat.ch)
*/
-#include "linux/usb.h"
#include <linux/kernel.h>
#include <linux/errno.h>
#include <linux/slab.h>
@@ -194,8 +193,6 @@ enum tp_type {
/* list of device capability bits */
#define HAS_INTEGRATED_BUTTON 1
-/* maximum number of supported endpoints (currently trackpad and button) */
-#define MAX_ENDPOINTS 2
/* trackpad finger data block size */
#define FSIZE_TYPE1 (14 * sizeof(__le16))
@@ -894,18 +891,6 @@ static int bcm5974_resume(struct usb_interface *iface)
return error;
}
-static bool bcm5974_check_endpoints(struct usb_interface *iface,
- const struct bcm5974_config *cfg)
-{
- u8 ep_addr[MAX_ENDPOINTS + 1] = {0};
-
- ep_addr[0] = cfg->tp_ep;
- if (cfg->tp_type == TYPE1)
- ep_addr[1] = cfg->bt_ep;
-
- return usb_check_int_endpoints(iface, ep_addr);
-}
-
static int bcm5974_probe(struct usb_interface *iface,
const struct usb_device_id *id)
{
@@ -918,11 +903,6 @@ static int bcm5974_probe(struct usb_interface *iface,
/* find the product index */
cfg = bcm5974_get_config(udev);
- if (!bcm5974_check_endpoints(iface, cfg)) {
- dev_err(&iface->dev, "Unexpected non-int endpoint\n");
- return -ENODEV;
- }
-
/* allocate memory for our device state and initialize it */
dev = kzalloc(sizeof(struct bcm5974), GFP_KERNEL);
input_dev = input_allocate_device();
---
base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
change-id: 20240305-revert_bcm5974_ep_check-37f2a6ab2714
Best regards,
--
Javier Carrasco <javier.carrasco.cruz@gmail.com>
^ permalink raw reply related
* Re: [PATCH v3 00/10] backlight: Replace struct fb_info in interfaces
From: Lee Jones @ 2024-03-05 9:36 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: andy, daniel.thompson, jingoohan1, deller, robin, javierm,
dri-devel, linux-fbdev, linux-input, linux-pwm
In-Reply-To: <20240304163220.19144-1-tzimmermann@suse.de>
On Mon, 04 Mar 2024, Thomas Zimmermann wrote:
> Backlight drivers implement struct backlight_ops.check_fb, which
> uses struct fb_info in its interface. Replace the callback with one
> that does not use fb_info.
>
> In DRM, we have several drivers that implement backlight support. By
> including <linux/backlight.h> these drivers depend on <linux/fb.h>.
> At the same time, fbdev is deprecated for new drivers and likely to
> be replaced on many systems.
>
> This patchset is part of a larger effort to implement the backlight
> code without depending on fbdev.
>
> Patch 1 makes the backlight core match backlight and framebuffer
> devices via struct fb_info.bl_dev. Patches 2 to 9 then go through
> drivers and remove unnecessary implementations of check_fb. Finally,
> patch 10 replaces the check_fb hook with controls_device, which
> uses the framebuffer's Linux device instead of the framebuffer.
>
> v3:
> * hide CONFIG_FB_BACKLIGHT behind fb_bl_device() (Lee)
> * if-else cleanups (Andy)
> * fix commit message of patch 2 (Andy)
> v2:
> * fix hid-picolcd for CONFIG_FB_BACKLIGHT=n
> * fixes to commit messages
>
> Thomas Zimmermann (10):
> backlight: Match backlight device against struct fb_info.bl_dev
> auxdisplay/ht16k33: Remove struct backlight_ops.check_fb
> hid/hid-picolcd: Fix initialization order
> hid/hid-picolcd: Remove struct backlight_ops.check_fb
> backlight/aat2870-backlight: Remove struct backlight.check_fb
> backlight/pwm-backlight: Remove struct backlight_ops.check_fb
> fbdev/sh_mobile_lcdc_fb: Remove struct backlight_ops.check_fb
> fbdev/ssd1307fb: Init backlight before registering framebuffer
> fbdev/ssd1307fb: Remove struct backlight_ops.check_fb
> backlight: Add controls_device callback to struct backlight_ops
>
> drivers/auxdisplay/ht16k33.c | 8 ------
> drivers/hid/hid-picolcd_backlight.c | 7 ------
> drivers/hid/hid-picolcd_core.c | 14 +++++------
> drivers/hid/hid-picolcd_fb.c | 6 +++++
> drivers/video/backlight/aat2870_bl.c | 7 ------
> drivers/video/backlight/backlight.c | 8 ++++--
> drivers/video/backlight/bd6107.c | 12 ++++-----
> drivers/video/backlight/gpio_backlight.c | 12 ++++-----
> drivers/video/backlight/lv5207lp.c | 12 ++++-----
> drivers/video/backlight/pwm_bl.c | 12 ---------
> drivers/video/fbdev/core/fb_backlight.c | 5 ++++
> drivers/video/fbdev/sh_mobile_lcdcfb.c | 7 ------
> drivers/video/fbdev/ssd1307fb.c | 31 +++++++++---------------
> include/linux/backlight.h | 16 ++++++------
> include/linux/fb.h | 9 +++++++
> include/linux/pwm_backlight.h | 1 -
> 16 files changed, 70 insertions(+), 97 deletions(-)
Which Acks are you missing for us to merge this?
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH 0/4] leds: trigger: Improve handling of led_trigger_event() and simplify mute audio trigger
From: Lee Jones @ 2024-03-05 9:55 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Pavel Machek, Jaroslav Kysela, Takashi Iwai, Dmitry Torokhov,
Thomas Bogendoerfer, linux-leds@vger.kernel.org, linux-sound,
open list:HID CORE LAYER, linux-mips
In-Reply-To: <208e8bcc-1f35-4095-9a70-7243fdabaf87@gmail.com>
On Sat, 02 Mar 2024, Heiner Kallweit wrote:
> On 29.02.2024 18:26, Lee Jones wrote:
> > On Tue, 13 Feb 2024, Heiner Kallweit wrote:
> >
> >> If a simple trigger is assigned to a LED, then the LED may be off until
> >> the next led_trigger_event() call. This may be an issue for simple
> >> triggers with rare led_trigger_event() calls, e.g. power supply
> >> charging indicators (drivers/power/supply/power_supply_leds.c).
> >> Therefore persist the brightness value of the last led_trigger_event()
> >> call and use this value if the trigger is assigned to a LED.
> >> This change allows to use simple triggers in more cases.
> >> As a first use case simplify handling of the mute audio trigger.
> >>
> >> This series touches few subsystems. I'd propose to handle it via
> >> the LED subsystem.
> >>
> >> Heiner Kallweit (4):
> >> leds: trigger: Store brightness set by led_trigger_event()
> >> ALSA: control-led: Integrate mute led trigger
> >> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER
> >> leds: trigger: audio: Remove this trigger
> >>
> >> arch/mips/configs/ci20_defconfig | 1 -
> >
> >> drivers/input/input-leds.c | 8 +---
> >
> > This does not apply.
> >
> > Please rebase onto v6.8-rc1.
> >
> Since v6.8-rc1 the following has been added, which is touched by
> my series:
> 698b43780ba2 ("Input: leds - set default-trigger for mute")
>
> Rebasing onto v6.8-rc1 would mean:
> - remove the change to input-leds from the series
> - resubmit this change via input subsystem
>
> This would affect bisectability, because for the time being
> input-leds would reference a config symbol that doesn't exist
> any longer.
>
> We'd be fine only if the change to input-leds is applied first.
> I think that's the best way to go, if you can't accept a series
> based on linux-next.
Then it's going to have to wait until v6.10.
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v8 3/5] mfd: cs40l50: Add support for CS40L50 core driver
From: Lee Jones @ 2024-03-05 9:58 UTC (permalink / raw)
To: James Ogletree
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Mark Brown, Jeff LaBundy,
open list:CIRRUS LOGIC HAPTIC DRIVERS,
linux-sound@vger.kernel.org, linux-input@vger.kernel.org,
devicetree@vger.kernel.org
In-Reply-To: <6DD14CBC-FAE2-4768-AD77-347229FE9AC7@cirrus.com>
On Fri, 01 Mar 2024, James Ogletree wrote:
> Hi Lee,
>
> Thanks for the review.
>
> > On Mar 1, 2024, at 3:17 AM, Lee Jones <lee@kernel.org> wrote:
> >
> > On Wed, 21 Feb 2024, James Ogletree wrote:
> >
> >> Introduce support for Cirrus Logic Device CS40L50: a
> >> haptic driver with waveform memory, integrated DSP,
> >> and closed-loop algorithms.
> >>
> >> The MFD component registers and initializes the device.
> >>
> >> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> >> ---
> >> MAINTAINERS | 2 +
> >> drivers/mfd/Kconfig | 30 ++
> >> drivers/mfd/Makefile | 4 +
> >> drivers/mfd/cs40l50-core.c | 531 ++++++++++++++++++++++++++++++++++++
> >> drivers/mfd/cs40l50-i2c.c | 69 +++++
> >> drivers/mfd/cs40l50-spi.c | 69 +++++
> >> include/linux/mfd/cs40l50.h | 142 ++++++++++
> >> 7 files changed, 847 insertions(+)
> >> create mode 100644 drivers/mfd/cs40l50-core.c
> >> create mode 100644 drivers/mfd/cs40l50-i2c.c
> >> create mode 100644 drivers/mfd/cs40l50-spi.c
> >> create mode 100644 include/linux/mfd/cs40l50.h
> >
> > Mostly fine, just a few nits.
> >
> > Assumingly this needs to go in via one tree (usually MFD).
> >
> > I can't do so until the other maintainers have provided Acks.
> >
>
> Yes, understood.
>
> >> +static const struct cs_dsp_region cs40l50_dsp_regions[] = {
> >> + { .type = WMFW_HALO_PM_PACKED, .base = CS40L50_PMEM_0 },
> >> + { .type = WMFW_HALO_XM_PACKED, .base = CS40L50_XMEM_PACKED_0 },
> >> + { .type = WMFW_HALO_YM_PACKED, .base = CS40L50_YMEM_PACKED_0 },
> >> + { .type = WMFW_ADSP2_XM, .base = CS40L50_XMEM_UNPACKED24_0 },
> >> + { .type = WMFW_ADSP2_YM, .base = CS40L50_YMEM_UNPACKED24_0 },
> >> +};
> >> +
> >> +static void cs40l50_dsp_remove(void *data)
> >> +{
> >> + cs_dsp_remove((struct cs_dsp *)data);
> >
> > Is the cast required?
> >
> > Where is this function?
>
> Seems that the cast is redundant, so I will remove.
>
> The function cs_dsp_remove() is exported from linux/firmware/cirrus/cs_dsp.h.
>
> >
> >> +}
> >> +
> >> +static const struct cs_dsp_client_ops cs40l50_client_ops;
> >
> > What's this for? Where is it used?
> >
> > In general, I'm not a fan of empty struct definitions like this.
>
> From the same cs_dsp module as mentioned above, it is a structure containing
> callbacks that gives the client driver an opportunity to respond to certain events
> during the DSP's lifecycle. It just so happens that this driver does not need to do
> anything. However, no struct definition will result in a null pointer dereference by
> cs_dsp when it checks for the callbacks.
Then check for NULL before dereferencing it?
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v3 00/10] backlight: Replace struct fb_info in interfaces
From: Thomas Zimmermann @ 2024-03-05 10:00 UTC (permalink / raw)
To: Lee Jones
Cc: andy, daniel.thompson, jingoohan1, deller, robin, javierm,
dri-devel, linux-fbdev, linux-input, linux-pwm
In-Reply-To: <20240305093632.GC5206@google.com>
Hi
Am 05.03.24 um 10:36 schrieb Lee Jones:
> On Mon, 04 Mar 2024, Thomas Zimmermann wrote:
>
>> Backlight drivers implement struct backlight_ops.check_fb, which
>> uses struct fb_info in its interface. Replace the callback with one
>> that does not use fb_info.
>>
>> In DRM, we have several drivers that implement backlight support. By
>> including <linux/backlight.h> these drivers depend on <linux/fb.h>.
>> At the same time, fbdev is deprecated for new drivers and likely to
>> be replaced on many systems.
>>
>> This patchset is part of a larger effort to implement the backlight
>> code without depending on fbdev.
>>
>> Patch 1 makes the backlight core match backlight and framebuffer
>> devices via struct fb_info.bl_dev. Patches 2 to 9 then go through
>> drivers and remove unnecessary implementations of check_fb. Finally,
>> patch 10 replaces the check_fb hook with controls_device, which
>> uses the framebuffer's Linux device instead of the framebuffer.
>>
>> v3:
>> * hide CONFIG_FB_BACKLIGHT behind fb_bl_device() (Lee)
>> * if-else cleanups (Andy)
>> * fix commit message of patch 2 (Andy)
>> v2:
>> * fix hid-picolcd for CONFIG_FB_BACKLIGHT=n
>> * fixes to commit messages
>>
>> Thomas Zimmermann (10):
>> backlight: Match backlight device against struct fb_info.bl_dev
>> auxdisplay/ht16k33: Remove struct backlight_ops.check_fb
>> hid/hid-picolcd: Fix initialization order
>> hid/hid-picolcd: Remove struct backlight_ops.check_fb
>> backlight/aat2870-backlight: Remove struct backlight.check_fb
>> backlight/pwm-backlight: Remove struct backlight_ops.check_fb
>> fbdev/sh_mobile_lcdc_fb: Remove struct backlight_ops.check_fb
>> fbdev/ssd1307fb: Init backlight before registering framebuffer
>> fbdev/ssd1307fb: Remove struct backlight_ops.check_fb
>> backlight: Add controls_device callback to struct backlight_ops
>>
>> drivers/auxdisplay/ht16k33.c | 8 ------
>> drivers/hid/hid-picolcd_backlight.c | 7 ------
>> drivers/hid/hid-picolcd_core.c | 14 +++++------
>> drivers/hid/hid-picolcd_fb.c | 6 +++++
>> drivers/video/backlight/aat2870_bl.c | 7 ------
>> drivers/video/backlight/backlight.c | 8 ++++--
>> drivers/video/backlight/bd6107.c | 12 ++++-----
>> drivers/video/backlight/gpio_backlight.c | 12 ++++-----
>> drivers/video/backlight/lv5207lp.c | 12 ++++-----
>> drivers/video/backlight/pwm_bl.c | 12 ---------
>> drivers/video/fbdev/core/fb_backlight.c | 5 ++++
>> drivers/video/fbdev/sh_mobile_lcdcfb.c | 7 ------
>> drivers/video/fbdev/ssd1307fb.c | 31 +++++++++---------------
>> include/linux/backlight.h | 16 ++++++------
>> include/linux/fb.h | 9 +++++++
>> include/linux/pwm_backlight.h | 1 -
>> 16 files changed, 70 insertions(+), 97 deletions(-)
> Which Acks are you missing for us to merge this?
Only yours.
If you decide to only merge the backlight patches, I'll merge the other
patches via other fbdev/drm trees during the next release cycle. Patch
10/10 would afterwards go into the backlight tree again.
Best regards
Thomas
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply
* [PATCH] Input: gpio_keys_polled - Suppress deferred probe error for gpio
From: Uwe Kleine-König @ 2024-03-05 10:10 UTC (permalink / raw)
To: Dmitry Torokhov, Linus Walleij, Krzysztof Kozlowski,
Andy Shevchenko
Cc: linux-input, kernel
On a PC Engines APU our admins are faced with:
$ dmesg | grep -c "gpio-keys-polled gpio-keys-polled: unable to claim gpio 0, err=-517"
261
Such a message always appears when e.g. a new USB device is plugged in.
Suppress this message which considerably clutters the kernel log for
EPROBE_DEFER (i.e. -517).
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,
there are a few other exit paths that could use dev_err_probe(), but IIRC
Dmitry isn't a fan of using dev_err_probe() where the return value cannot be
EPROBE_DEFER, so I'm only changing this one error path.
Best regards
Uwe
drivers/input/keyboard/gpio_keys_polled.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index ba00ecfbd343..b41fd1240f43 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -315,12 +315,10 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
error = devm_gpio_request_one(dev, button->gpio,
flags, button->desc ? : DRV_NAME);
- if (error) {
- dev_err(dev,
- "unable to claim gpio %u, err=%d\n",
- button->gpio, error);
- return error;
- }
+ if (error)
+ return dev_err_probe(dev, error,
+ "unable to claim gpio %u\n",
+ button->gpio);
bdata->gpiod = gpio_to_desc(button->gpio);
if (!bdata->gpiod) {
base-commit: 11afac187274a6177a7ac82997f8691c0f469e41
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] Input: gpio_keys_polled - Suppress deferred probe error for gpio
From: Linus Walleij @ 2024-03-05 10:18 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Andy Shevchenko,
linux-input, kernel
In-Reply-To: <20240305101042.10953-2-u.kleine-koenig@pengutronix.de>
On Tue, Mar 5, 2024 at 11:10 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On a PC Engines APU our admins are faced with:
>
> $ dmesg | grep -c "gpio-keys-polled gpio-keys-polled: unable to claim gpio 0, err=-517"
> 261
>
> Such a message always appears when e.g. a new USB device is plugged in.
>
> Suppress this message which considerably clutters the kernel log for
> EPROBE_DEFER (i.e. -517).
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Fair enough,
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v8 3/5] mfd: cs40l50: Add support for CS40L50 core driver
From: Charles Keepax @ 2024-03-05 10:20 UTC (permalink / raw)
To: Lee Jones
Cc: James Ogletree, James Ogletree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Mark Brown, Jeff LaBundy,
open list:CIRRUS LOGIC HAPTIC DRIVERS,
linux-sound@vger.kernel.org, linux-input@vger.kernel.org,
devicetree@vger.kernel.org
In-Reply-To: <20240305095818.GF5206@google.com>
On Tue, Mar 05, 2024 at 09:58:18AM +0000, Lee Jones wrote:
> On Fri, 01 Mar 2024, James Ogletree wrote:
> > >> +static const struct cs_dsp_client_ops cs40l50_client_ops;
> > >
> > > What's this for? Where is it used?
> > >
> > > In general, I'm not a fan of empty struct definitions like this.
> >
> > From the same cs_dsp module as mentioned above, it is a structure containing
> > callbacks that gives the client driver an opportunity to respond to certain events
> > during the DSP's lifecycle. It just so happens that this driver does not need to do
> > anything. However, no struct definition will result in a null pointer dereference by
> > cs_dsp when it checks for the callbacks.
>
> Then check for NULL before dereferencing it?
It would probably make sense to move the cs40l50_stop_core writes
into the pre_run callback, and the CLOCK_ENABLE /
cs40l50_configure_dsp stuff into the post_run callback. Which is
probably a slightly more idiomatic way to use the API of cs_dsp
and would mean some of the callbacks are initialised, thus
dodging this problem.
We could check for there being no client_ops in the cs_dsp core,
but it feels kinda redundant since there really should generally
be client ops.
Thanks,
Charles
^ permalink raw reply
* Re: [PATCH 0/4] leds: trigger: Improve handling of led_trigger_event() and simplify mute audio trigger
From: Takashi Iwai @ 2024-03-05 10:26 UTC (permalink / raw)
To: Lee Jones
Cc: Heiner Kallweit, Pavel Machek, Jaroslav Kysela, Takashi Iwai,
Dmitry Torokhov, Thomas Bogendoerfer, linux-leds@vger.kernel.org,
linux-sound, open list:HID CORE LAYER, linux-mips
In-Reply-To: <20240305095539.GE5206@google.com>
On Tue, 05 Mar 2024 10:55:39 +0100,
Lee Jones wrote:
>
> On Sat, 02 Mar 2024, Heiner Kallweit wrote:
>
> > On 29.02.2024 18:26, Lee Jones wrote:
> > > On Tue, 13 Feb 2024, Heiner Kallweit wrote:
> > >
> > >> If a simple trigger is assigned to a LED, then the LED may be off until
> > >> the next led_trigger_event() call. This may be an issue for simple
> > >> triggers with rare led_trigger_event() calls, e.g. power supply
> > >> charging indicators (drivers/power/supply/power_supply_leds.c).
> > >> Therefore persist the brightness value of the last led_trigger_event()
> > >> call and use this value if the trigger is assigned to a LED.
> > >> This change allows to use simple triggers in more cases.
> > >> As a first use case simplify handling of the mute audio trigger.
> > >>
> > >> This series touches few subsystems. I'd propose to handle it via
> > >> the LED subsystem.
> > >>
> > >> Heiner Kallweit (4):
> > >> leds: trigger: Store brightness set by led_trigger_event()
> > >> ALSA: control-led: Integrate mute led trigger
> > >> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER
> > >> leds: trigger: audio: Remove this trigger
> > >>
> > >> arch/mips/configs/ci20_defconfig | 1 -
> > >
> > >> drivers/input/input-leds.c | 8 +---
> > >
> > > This does not apply.
> > >
> > > Please rebase onto v6.8-rc1.
> > >
> > Since v6.8-rc1 the following has been added, which is touched by
> > my series:
> > 698b43780ba2 ("Input: leds - set default-trigger for mute")
> >
> > Rebasing onto v6.8-rc1 would mean:
> > - remove the change to input-leds from the series
> > - resubmit this change via input subsystem
> >
> > This would affect bisectability, because for the time being
> > input-leds would reference a config symbol that doesn't exist
> > any longer.
> >
> > We'd be fine only if the change to input-leds is applied first.
> > I think that's the best way to go, if you can't accept a series
> > based on linux-next.
>
> Then it's going to have to wait until v6.10.
Or merging via input tree?
The changes are relatively small and easy, after all.
thanks,
Takashi
^ permalink raw reply
* Re: [PATCH v3 00/10] backlight: Replace struct fb_info in interfaces
From: Lee Jones @ 2024-03-05 10:30 UTC (permalink / raw)
To: andy, daniel.thompson, jingoohan1, deller, robin, javierm,
Thomas Zimmermann
Cc: dri-devel, linux-fbdev, linux-input, linux-pwm
In-Reply-To: <20240304163220.19144-1-tzimmermann@suse.de>
On Mon, 04 Mar 2024 17:29:45 +0100, Thomas Zimmermann wrote:
> Backlight drivers implement struct backlight_ops.check_fb, which
> uses struct fb_info in its interface. Replace the callback with one
> that does not use fb_info.
>
> In DRM, we have several drivers that implement backlight support. By
> including <linux/backlight.h> these drivers depend on <linux/fb.h>.
> At the same time, fbdev is deprecated for new drivers and likely to
> be replaced on many systems.
>
> [...]
Applied, thanks!
[01/10] backlight: Match backlight device against struct fb_info.bl_dev
commit: 67716b34e1be2beb7464f9b9d0b47b2cc7dbc208
[02/10] auxdisplay/ht16k33: Remove struct backlight_ops.check_fb
commit: 747554336b46a26fcdf47e2e48044c7e175b6a5f
[03/10] hid/hid-picolcd: Fix initialization order
commit: d55b578e3b0cd6abdc52e2c34d88dd3487bb10a2
[04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
commit: 09ca774d2e87f9b086b23354b1605709fb50205f
[05/10] backlight/aat2870-backlight: Remove struct backlight.check_fb
commit: 61e837e975abcb4d278c3427d927e1cbaaed0090
[06/10] backlight/pwm-backlight: Remove struct backlight_ops.check_fb
commit: 9c2be31d2951c8dce90950db000c095330406f94
[07/10] fbdev/sh_mobile_lcdc_fb: Remove struct backlight_ops.check_fb
commit: a2a8fbdb54a78fd18850cd0b74b465657ffb1e0c
[08/10] fbdev/ssd1307fb: Init backlight before registering framebuffer
commit: 5500326bd33e52230866f50770ca822ce400a4ab
[09/10] fbdev/ssd1307fb: Remove struct backlight_ops.check_fb
commit: d1b82cc44fd8be4013538992814c45f0e55c02b4
[10/10] backlight: Add controls_device callback to struct backlight_ops
commit: 7e508af663e20e9e40003bb30e06b926c754159b
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH 0/4] leds: trigger: Improve handling of led_trigger_event() and simplify mute audio trigger
From: Lee Jones @ 2024-03-05 10:40 UTC (permalink / raw)
To: Takashi Iwai
Cc: Heiner Kallweit, Pavel Machek, Jaroslav Kysela, Takashi Iwai,
Dmitry Torokhov, Thomas Bogendoerfer, linux-leds@vger.kernel.org,
linux-sound, open list:HID CORE LAYER, linux-mips
In-Reply-To: <875xy1ynya.wl-tiwai@suse.de>
On Tue, 05 Mar 2024, Takashi Iwai wrote:
> On Tue, 05 Mar 2024 10:55:39 +0100,
> Lee Jones wrote:
> >
> > On Sat, 02 Mar 2024, Heiner Kallweit wrote:
> >
> > > On 29.02.2024 18:26, Lee Jones wrote:
> > > > On Tue, 13 Feb 2024, Heiner Kallweit wrote:
> > > >
> > > >> If a simple trigger is assigned to a LED, then the LED may be off until
> > > >> the next led_trigger_event() call. This may be an issue for simple
> > > >> triggers with rare led_trigger_event() calls, e.g. power supply
> > > >> charging indicators (drivers/power/supply/power_supply_leds.c).
> > > >> Therefore persist the brightness value of the last led_trigger_event()
> > > >> call and use this value if the trigger is assigned to a LED.
> > > >> This change allows to use simple triggers in more cases.
> > > >> As a first use case simplify handling of the mute audio trigger.
> > > >>
> > > >> This series touches few subsystems. I'd propose to handle it via
> > > >> the LED subsystem.
> > > >>
> > > >> Heiner Kallweit (4):
> > > >> leds: trigger: Store brightness set by led_trigger_event()
> > > >> ALSA: control-led: Integrate mute led trigger
> > > >> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER
> > > >> leds: trigger: audio: Remove this trigger
> > > >>
> > > >> arch/mips/configs/ci20_defconfig | 1 -
> > > >
> > > >> drivers/input/input-leds.c | 8 +---
> > > >
> > > > This does not apply.
> > > >
> > > > Please rebase onto v6.8-rc1.
> > > >
> > > Since v6.8-rc1 the following has been added, which is touched by
> > > my series:
> > > 698b43780ba2 ("Input: leds - set default-trigger for mute")
> > >
> > > Rebasing onto v6.8-rc1 would mean:
> > > - remove the change to input-leds from the series
> > > - resubmit this change via input subsystem
> > >
> > > This would affect bisectability, because for the time being
> > > input-leds would reference a config symbol that doesn't exist
> > > any longer.
> > >
> > > We'd be fine only if the change to input-leds is applied first.
> > > I think that's the best way to go, if you can't accept a series
> > > based on linux-next.
> >
> > Then it's going to have to wait until v6.10.
>
> Or merging via input tree?
> The changes are relatively small and easy, after all.
That's likely to culminate in similar merge-conflicts when further
changes are made to:
drivers/leds/led-triggers.c
drivers/leds/trigger/Kconfig
drivers/leds/trigger/Makefile
drivers/leds/trigger/ledtrig-audio.c
include/linux/leds.h
What happens if I take all but the Input change? If this doesn't cause
build-failures and the Input change will definitely land in v6.9, it
could work.
--
Lee Jones [李琼斯]
^ 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