Re: [PATCH v1 1/2] dt-bindings: HID: i2c-hid: elan: Introduce Elan ekth6a12nay
On Jul 04 2024, Zhaoxiong Lv wrote: > The Elan ekth6a12nay touch screen chip same as Elan eKTH6915 controller > has a reset gpio. The difference is that they have different > post_power_delay_ms. > > Signed-off-by: Zhaoxiong Lv > --- > Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml > b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml > index dc4ac41f2441..0bbf9dd7636e 100644 > --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml > +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml > @@ -20,6 +20,7 @@ properties: >compatible: > enum: >- elan,ekth6915 > + - elan,ekth6a12nay >- ilitek,ili2901 This is not in v6.10-rcX, ilitek got removed and the compatible node got updated. I could amend the patch for this but I'd rather have you to do it as I can not test the change myself. So please respin the series on top of v6.10 at least, keep the various acks/rev-by and and make sure you test. Cheers, Benjamin > >reg: > -- > 2.17.1 >
Re: In kernel virtual HID devices (was Future handling of complex RGB devices on Linux v3)
On Mar 26 2024, Werner Sembach wrote: > Hi all, > > Am 26.03.24 um 16:39 schrieb Benjamin Tissoires: > > On Mar 26 2024, Werner Sembach wrote: > > > Hi all, > > > > > > Am 25.03.24 um 19:30 schrieb Hans de Goede: > > > > > > [snip] > > > > > > If the kernel already handles the custom protocol into generic HID, > > > > > > the > > > > > > work for userspace is not too hard because they can deal with a > > > > > > known > > > > > > protocol and can be cross-platform in their implementation. > > > > > > > > > > > > I'm mentioning that cross-platform because SDL used to rely on the > > > > > > input, LEDs, and other Linux peculiarities and eventually fell back > > > > > > on > > > > > > using hidraw only because it's way more easier that way. > > > > > > > > > > > > The other advantage of LampArray is that according to Microsoft's > > > > > > document, new devices are going to support it out of the box, so > > > > > > they'll > > > > > > be supported out of the box directly. > > > > > > > > > > > > Most of the time my stance is "do not add new kernel API, you'll > > > > > > regret > > > > > > it later". So in that case, given that we have a formally approved > > > > > > standard, I would suggest to use it, and consider it your API. > > > > > The only new UAPI would be the use_leds_uapi switch to turn on/off > > > > > the backwards compatibility. > > I have my reserves with such a kill switch (see below). > > > > > > Actually we don't even need that. Typically there is a single HID > > > > driver handling both keys and the backlight, so userspace cannot > > > > just unbind the HID driver since then the keys stop working. > > I don't think Werner meant unbinding the HID driver, just a toggle to > > enable/disable the basic HID core processing of LampArray. > > > > > > But with a virtual LampArray HID device the only functionality > > > > for an in kernel HID driver would be to export a basic keyboard > > > > backlight control interface for simple non per key backlight control > > > > to integrate nicely with e.g. GNOME's backlight control. > > > Don't forget that in the future there will be devices that natively > > > support > > > LampArray in their firmware, so for them it is the same device. > > Yeah, the generic LampArray support will not be able to differentiate > > "emulated" devices from native ones. > > > > > Regards, > > > > > > Werner > > > > > > > And then when OpenRGB wants to take over it can just unbind the HID > > > > driver from the HID device using existing mechanisms for that. > > Again no, it'll be too unpredicted. > > > > > > Hmm, I wonder if that will not also kill hidraw support though ... > > > > I guess getting hidraw support back might require then also manually > > > > binding the default HID input driver. Bentiss any input on this? > > To be able to talk over hidraw you need a driver to be bound, yes. But I > > had the impression that LampArray would be supported by default in > > hid-input.c, thus making this hard to remove. Having a separate driver > > will work, but as soon as the LampArray device will also export a > > multitouch touchpad, we are screwed and will have to make a choice > > between LampArray and touch... > > > > > > Background info: as discussed earlier in the thread Werner would like > > > > to have a basic driver registering a /sys/class/leds/foo::kbd_backlight/ > > > > device, since those are automatically supported by GNOME (and others) > > > > and will give basic kbd backlight brightness control in the desktop > > > > environment. This could be a simple HID driver for > > > > the hid_allocate_device()-ed virtual HID device, but userspace needs > > > > to be able to move that out of the way when it wants to take over > > > > full control of the per key lighting. > > Do we really need to entirely unregister the led class device? Can't we > > snoop on the commands and get some "mean value"? > > > > > > Regards, > > > > > > > > Hans > > > > > > > > > > > > > > > >
Re: In kernel virtual HID devices (was Future handling of complex RGB devices on Linux v3)
On Mar 26 2024, Werner Sembach wrote: > Hi all, > > Am 25.03.24 um 19:30 schrieb Hans de Goede: > > [snip] > > > > If the kernel already handles the custom protocol into generic HID, the > > > > work for userspace is not too hard because they can deal with a known > > > > protocol and can be cross-platform in their implementation. > > > > > > > > I'm mentioning that cross-platform because SDL used to rely on the > > > > input, LEDs, and other Linux peculiarities and eventually fell back on > > > > using hidraw only because it's way more easier that way. > > > > > > > > The other advantage of LampArray is that according to Microsoft's > > > > document, new devices are going to support it out of the box, so they'll > > > > be supported out of the box directly. > > > > > > > > Most of the time my stance is "do not add new kernel API, you'll regret > > > > it later". So in that case, given that we have a formally approved > > > > standard, I would suggest to use it, and consider it your API. > > > The only new UAPI would be the use_leds_uapi switch to turn on/off the > > > backwards compatibility. I have my reserves with such a kill switch (see below). > > Actually we don't even need that. Typically there is a single HID > > driver handling both keys and the backlight, so userspace cannot > > just unbind the HID driver since then the keys stop working. I don't think Werner meant unbinding the HID driver, just a toggle to enable/disable the basic HID core processing of LampArray. > > > > But with a virtual LampArray HID device the only functionality > > for an in kernel HID driver would be to export a basic keyboard > > backlight control interface for simple non per key backlight control > > to integrate nicely with e.g. GNOME's backlight control. > > Don't forget that in the future there will be devices that natively support > LampArray in their firmware, so for them it is the same device. Yeah, the generic LampArray support will not be able to differentiate "emulated" devices from native ones. > > Regards, > > Werner > > > And then when OpenRGB wants to take over it can just unbind the HID > > driver from the HID device using existing mechanisms for that. Again no, it'll be too unpredicted. > > > > Hmm, I wonder if that will not also kill hidraw support though ... > > I guess getting hidraw support back might require then also manually > > binding the default HID input driver. Bentiss any input on this? To be able to talk over hidraw you need a driver to be bound, yes. But I had the impression that LampArray would be supported by default in hid-input.c, thus making this hard to remove. Having a separate driver will work, but as soon as the LampArray device will also export a multitouch touchpad, we are screwed and will have to make a choice between LampArray and touch... > > > > Background info: as discussed earlier in the thread Werner would like > > to have a basic driver registering a /sys/class/leds/foo::kbd_backlight/ > > device, since those are automatically supported by GNOME (and others) > > and will give basic kbd backlight brightness control in the desktop > > environment. This could be a simple HID driver for > > the hid_allocate_device()-ed virtual HID device, but userspace needs > > to be able to move that out of the way when it wants to take over > > full control of the per key lighting. Do we really need to entirely unregister the led class device? Can't we snoop on the commands and get some "mean value"? > > > > Regards, > > > > Hans > > > > > > > > > > > > > > > > > The control flow for the whole system would look something like this: > > > > > > - System boots > > > > > > - Kernel driver initializes keyboard (maybe stops rainbowpuke boot > > > effects, sets brightness to a default value, or initializes a solid color) > > > > > > - systemd-backlight restores last keyboard backlight brightness > > > > > > - UPower sees sysfs leds entry and exposes it to DBus for DEs to do > > > keyboard brightness handling > > > > > > - If the user wants more control they (auto-)start OpenRGB > > > > > > - OpenRGB disables sysfs leds entry via use_leds_uapi to prevent > > > double control of the same device by UPower > > > > > > - OpenRGB directly interacts with hidraw device via LampArray API to > > > give fine granular control of the backlight > > > > > > - When OpenRGB closes it should reenable the sysfs leds entry That's where your plan falls short: if OpenRGB crashes, or is killed it will not reset that bit. Next question: is OpenRGB supposed to keep the hidraw node opened all the time or not? If it has to keep it open, we should be able to come up with a somewhat similar hack that we have with hid-steam: when the hidraw node is opened, we disable the kernel processing of LampArray. When the node is closed, we re-enable it. But that also means we have to distinguish steam/SDL from OpenRGB... I just carefully read the
Re: In kernel virtual HID devices (was Future handling of complex RGB devices on Linux v3)
On Mar 25 2024, Hans de Goede wrote: > +Cc: Bentiss, Jiri > > Hi Werner, > > On 3/20/24 12:16 PM, Werner Sembach wrote: > > Hi Hans and the others, > > > > Am 22.02.24 um 14:14 schrieb Werner Sembach: > >> Hi, > >> > >> Thanks everyone for the exhaustive feedback. And at least this thread is a > >> good comprehesive reference for the future ^^. > >> > >> To recap the hopefully final UAPI for complex RGB lighting devices: > >> > >> - By default there is a singular /sys/class/leds/* entry that treats the > >> device as if it was a single zone RGB keyboard backlight with no special > >> effects. > >> > >> - There is an accompanying misc device with the sysfs attributes "name", > >> "device_type", "firmware_version_string", "serial_number" for device > >> identification and "use_leds_uapi" that defaults to 1. > >> > >> - If set to 0 the /sys/class/leds/* entry disappears. The driver > >> should keep the last state the backlight was in active if possible. > >> > >> - If set 1 it appears again. The driver should bring it back to a > >> static 1 zone setting while avoiding flicker if possible. > >> > >> - If the device is not controllable by for example hidraw, the misc device > >> might also implement additional ioctls or sysfs attributes to allow a more > >> complex low level control for the keyboard backlight. This is will be a > >> highly vendor specific UAPI. > > So in the OpenRGB issue thread > > https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/dynamic-lighting-devices > > aka HID LampArray was mentioned. I did dismiss it because I thought that > > is only relevant for firmware, but I now stumbled upon the Virtual HID > > Framework (VHF) > > https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/virtual-hid-framework--vhf- > > and now I wonder if an equivalent exists for Linux? A quick search did not > > yield any results for me. > > Oh, interesting. I did not know about the HID LampArray API. > > About your question about virtual HID devices, there is uHID, > but as the name suggests this allows userspace to emulate a HID > device. > > In your case you want to do the emulation in kernel so that you > can translate the proprietary WMI calls to something HID LampArray > compatible. > > I guess you could do this by defining your own HID transport driver, > like how e.g. the i2c-hid code defines 1 i2c-hid parent + 1 HID > "client" for each device which talks HID over i2c in the machine. > > Bentiss, Jiri, do you have any input on this. Would something like > that be acceptable to you (just based on the concept at least) ? I just read the thread, and I think I now understand a little bit what this request is :) IMO working with the HID LampArray is the way forward. So I would suggest to convert any non-HID RGB "LED display" that we are talking about as a HID LampArray device through `hid_allocate_device()` in the kernel. Basically what you are suggesting Hans. It's just that you don't need a formal transport layer, just a child device that happens to be HID. The next question IMO is: do we want the kernel to handle such machinery? Wouldn't it be simpler to just export the HID device and let userspace talk to it through hidraw, like what OpenRGB does? If the kernel already handles the custom protocol into generic HID, the work for userspace is not too hard because they can deal with a known protocol and can be cross-platform in their implementation. I'm mentioning that cross-platform because SDL used to rely on the input, LEDs, and other Linux peculiarities and eventually fell back on using hidraw only because it's way more easier that way. The other advantage of LampArray is that according to Microsoft's document, new devices are going to support it out of the box, so they'll be supported out of the box directly. Most of the time my stance is "do not add new kernel API, you'll regret it later". So in that case, given that we have a formally approved standard, I would suggest to use it, and consider it your API. Side note to self: I really need to resurrect the hidraw revoke series so we could export those hidraw node to userspace with uaccess through logind... Cheers, Benjamin
Re: [PATCH v4 00/11] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
On Jul 27 2023, Douglas Anderson wrote:
>
> The big motivation for this patch series is mostly described in the patch
> ("drm/panel: Add a way for other devices to follow panel state"), but to
> quickly summarize here: for touchscreens that are connected to a panel we
> need the ability to power sequence the two device together. This is not a
> new need, but so far we've managed to get by through a combination of
> inefficiency, added costs, or perhaps just a little bit of brokenness.
> It's time to do better. This patch series allows us to do better.
>
> Assuming that people think this patch series looks OK, we'll have to
> figure out the right way to land it. The panel patches and i2c-hid
> patches will go through very different trees and so either we'll need
> an Ack from one side or the other or someone to create a tag for the
> other tree to pull in. This will _probably_ require the true drm-misc
> maintainers to get involved, not a lowly committer. ;-)
>
> Version 4 of this series adds a new patch that suspends i2c-hid
> devices at remove time even for non panel-followers to make things
> consistent. It also attempts to isolate the panel follower code a bit
> more as per Benjamin's feedback on v3 and adds an item to the DRM todo
> list as per Maxime's request. As per Maxime's response to my v3 cover
> letter, I added his Reviewed-by tag to all 10 patches that were part
> of v3 (but left it off of the new i2c-hid patch in v4).
>
> Version 3 of this series was a long time coming after v2. Maxime and I
> had a very long discussion trying to figure out if there was a beter
> way and in the end we didn't find one so he was OK with the series in
> general [1]. After that got resolved, I tried to resolve Benjamin's
> feedback but got stuck [2]. Eventually I made my best guess. The end
> result was a v3 that wasn't that different from v2 but that had a tiny
> bit more code split out.
>
> Version 2 of this patch series didn't change too much. At a high level:
> * I added all the forgotten "static" to functions.
> * I've hopefully made the bindings better.
> * I've integrated into fw_devlink.
> * I cleaned up a few descriptions / comments.
>
> As far as I can tell, as of v4 everyone is on the same page that this
> patch series looks like a reasonable solution to the problem and we
> just need to get all the nits fixed and figure out how to land it.
Thanks a lot for the new version. I like it much more on the HID side:
for the HID part:
Reviewed-by: Benjamin Tissoires
I wouldn't mind having this series taken from the drm tree if that is
easier. i2c-hid is a low patch rate driver, so having it updated through
DRM should not be an issue.
In that case:
Acked-by: Benjamin Tissoires
Cheers,
Benjamin
>
> [1]
> https://lore.kernel.org/r/gkwymmfkdy2p2evz22wmbwgw42ii4wnvmvu64m3bghmj2jhv7x@4mbstjxnagxd
> [2]
> https://lore.kernel.org/r/CAD=FV=vbdeombgbwhppy+5toswt64gwbhga68oxfwsno4gg...@mail.gmail.com
>
> Changes in v4:
> - Document further cleanup in the official DRM todo list.
> - ("Suspend i2c-hid devices in remove") new for v4.
> - Move panel follower alternative checks to wrapper functions.
> - Rebase atop ("Suspend i2c-hid devices in remove").
>
> Changes in v3:
> - Add is_panel_follower() as a convenience for clients.
> - Add "depends on DRM || !DRM" to Kconfig to avoid randconfig error.
> - Split more of the panel follower code out of the core.
>
> Changes in v2:
> - Move the description to the generic touchscreen.yaml.
> - Update the desc to make it clearer it's only for integrated devices.
> - Add even more text to the commit message.
> - A few comment cleanups.
> - ("Add a devlink for panel followers") new for v2.
> - i2c_hid_core_initial_power_up() is now static.
> - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
> - ihid_core_panel_prepare_work() is now static.
> - Improve documentation for smp_wmb().
>
> Douglas Anderson (11):
> dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed
> touchscreens
> drm/panel: Check for already prepared/enabled in drm_panel
> drm/panel: Add a way for other devices to follow panel state
> of: property: fw_devlink: Add a devlink for panel followers
> HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS()
> HID: i2c-hid: Rearrange probe() to power things up later
> HID: i2c-hid: Make suspend and resume into helper functions
> HID: i2c-hid: Suspend i2c-hid devices in remove
> HID: i2c-hid: Support being a panel follower
> HID: i2c-hid: Do panel follower work on the system_wq
> arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels
>
> .../bindings/input/elan,ekth6915.yaml
Re: [PATCH v3 08/10] HID: i2c-hid: Support being a panel follower
On Jul 26 2023, Doug Anderson wrote: > Hi, > > On Wed, Jul 26, 2023 at 1:57 AM Benjamin Tissoires wrote: > > > > > @@ -1143,7 +1208,14 @@ void i2c_hid_core_remove(struct i2c_client *client) > > > struct i2c_hid *ihid = i2c_get_clientdata(client); > > > struct hid_device *hid; > > > > > > - i2c_hid_core_power_down(ihid); > > > + /* > > > + * If we're a follower, the act of unfollowing will cause us to be > > > + * powered down. Otherwise we need to manually do it. > > > + */ > > > + if (ihid->is_panel_follower) > > > + drm_panel_remove_follower(>panel_follower); > > > > That part is concerning, as we are now calling hid_drv->suspend() when > > removing > > the device. It might or not have an impact (I'm not sure of it), but we > > are effectively changing the path of commands sent to the device. > > > > hid-multitouch might call a feature in ->suspend, but the remove makes > > that the physical is actually disconnected, so the function will fail, > > and I'm not sure what is happening then. > > It's not too hard to change this if we're sure we want to. I could > change how the panel follower API works, though I'd rather keep it how > it is now for symmetry. Thus, if we want to do this I'd probably just > set a boolean at the beginning of i2c_hid_core_remove() to avoid the > suspend when the panel follower API calls us back. I was more thinking on a boolean. No need to overload the API. > > That being said, are you sure you want me to do that? > > 1. My patch doesn't change the behavior of any existing hardware. It > will only do anything for hardware that indicates it needs the panel > follower logic. Presumably these people could confirm that the logic > is OK for them, though I'll also admit that it's likely not many of > them will test the remove() case. Isn't trogdor (patch 10/10) already supported? Though you should be the one making tests, so it should be fine ;) > > 2. Can you give more details about why you say that the function will > fail? The first thing that the remove() function will do is to > unfollow the panel and that can cause the suspend to happen. At the > time this code runs all the normal communications should work and so > there should be no problems calling into the suspend code. Now that I think about it more, maybe I am too biased by USB where the device remove would happened *after* the device has been physically unplugged. And this doesn't apply of course in the I2C world. > > 3. You can correct me if I'm wrong, but I'd actually argue that > calling the suspend code during remove actually fixes issues and we > should probably do it for the non-panel-follower case as well. I think > there are at least two benefits. One benefit is that if the i2c-hid > device is on a power rail that can't turn off (either an always-on or > a shared power rail) that we'll at least get the device in a low power > state before we stop managing it with this driver. The second benefit > is that it implicitly disables the interrupt and that fixes a > potential crash at remove time(). The crash in the old code I'm > imagining is: > > a) i2c_hid_core_remove() is called. > > b) We try to power down the i2c hid device, which might not do > anything if the device is on an always-on rail. > > c) We call hid_destroy_device(), which frees the hid device. > > d) An interrupt comes in before the call to free_irq() and we try to > dispatch it to the already freed hid device and crash. > > > If you agree that my reasoning makes sense, I can add a separate patch > before this one to suspend during remove. Yep, I agree with you :) Adding a separate patch would be nice, yes. Thanks! Cheers, Benjamin
Re: [PATCH v3 08/10] HID: i2c-hid: Support being a panel follower
On Jul 25 2023, Douglas Anderson wrote:
> As talked about in the patch ("drm/panel: Add a way for other devices
> to follow panel state"), we really want to keep the power states of a
> touchscreen and the panel it's attached to in sync with each other. In
> that spirit, add support to i2c-hid to be a panel follower. This will
> let the i2c-hid driver get informed when the panel is powered on and
> off. From there we can match the i2c-hid device's power state to that
> of the panel.
>
> NOTE: this patch specifically _doesn't_ use pm_runtime to keep track
> of / manage the power state of the i2c-hid device, even though my
> first instinct said that would be the way to go. Specific problems
> with using pm_runtime():
> * The initial power up couldn't happen in a runtime resume function
> since it create sub-devices and, apparently, that's not good to do
> in your resume function.
> * Managing our power state with pm_runtime meant fighting to make the
> right thing happen at system suspend to prevent the system from
> trying to resume us only to suspend us again. While this might be
> able to be solved, it added complexity.
> Overall the code without pm_runtime() ended up being smaller and
> easier to understand.
>
> Signed-off-by: Douglas Anderson
> ---
>
> Changes in v3:
> - Add "depends on DRM || !DRM" to Kconfig to avoid randconfig error.
> - Split more of the panel follower code out of the core.
>
> Changes in v2:
> - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
>
> drivers/hid/i2c-hid/Kconfig| 2 +
> drivers/hid/i2c-hid/i2c-hid-core.c | 82 +-
> 2 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
> index 3be17109301a..2bdb55203104 100644
> --- a/drivers/hid/i2c-hid/Kconfig
> +++ b/drivers/hid/i2c-hid/Kconfig
> @@ -70,5 +70,7 @@ config I2C_HID_OF_GOODIX
>
> config I2C_HID_CORE
> tristate
> + # We need to call into panel code so if DRM=m, this can't be 'y'
> + depends on DRM || !DRM
> endif
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
> b/drivers/hid/i2c-hid/i2c-hid-core.c
> index fa8a1ca43d7f..fa6d1f624342 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -38,6 +38,8 @@
> #include
> #include
>
> +#include
> +
> #include "../hid-ids.h"
> #include "i2c-hid.h"
>
> @@ -107,6 +109,8 @@ struct i2c_hid {
> struct mutexreset_lock;
>
> struct i2chid_ops *ops;
> + struct drm_panel_follower panel_follower;
> + boolis_panel_follower;
> };
>
> static const struct i2c_hid_quirks {
> @@ -1058,6 +1062,59 @@ static int i2c_hid_core_initial_power_up(struct
> i2c_hid *ihid)
> return ret;
> }
>
> +static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
> +{
> + struct i2c_hid *ihid = container_of(follower, struct i2c_hid,
> panel_follower);
> + struct hid_device *hid = ihid->hid;
> +
> + /*
> + * hid->version is set on the first power up. If it's still zero then
> + * this is the first power on so we should perform initial power up
> + * steps.
> + */
> + if (!hid->version)
> + return i2c_hid_core_initial_power_up(ihid);
> +
> + return i2c_hid_core_resume(ihid);
> +}
> +
> +static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower
> *follower)
> +{
> + struct i2c_hid *ihid = container_of(follower, struct i2c_hid,
> panel_follower);
> +
> + return i2c_hid_core_suspend(ihid);
> +}
> +
> +static const struct drm_panel_follower_funcs
> i2c_hid_core_panel_follower_funcs = {
> + .panel_prepared = i2c_hid_core_panel_prepared,
> + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> +};
> +
> +static int i2c_hid_core_register_panel_follower(struct i2c_hid *ihid)
> +{
> + struct device *dev = >client->dev;
> + int ret;
> +
> + ihid->is_panel_follower = true;
> + ihid->panel_follower.funcs = _hid_core_panel_follower_funcs;
> +
> + /*
> + * If we're not in control of our own power up/power down then we can't
> + * do the logic to manage wakeups. Give a warning if a user thought
> + * that was possible then force the capability off.
> + */
> + if (device_can_wakeup(dev)) {
> + dev_warn(dev, "Can't wakeup if following panel\n");
> + device_set_wakeup_capable(dev, false);
> + }
> +
> + ret = drm_panel_add_follower(dev, >panel_follower);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> u16 hid_descriptor_address, u32 quirks)
> {
> @@ -1119,7 +1176,15 @@ int i2c_hid_core_probe(struct i2c_client *client,
> struct i2chid_ops *ops,
> hid->bus = BUS_I2C;
> hid->initial_quirks = quirks;
>
> - ret =
Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
On Jul 25 2023, Doug Anderson wrote:
> Hi,
>
> On Mon, Jul 17, 2023 at 11:15 AM Doug Anderson wrote:
> >
> > Benjamin,
> >
> > On Mon, Jun 26, 2023 at 3:49 PM Doug Anderson wrote:
> > >
> > > Benjamin,
> > >
> > > On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires
> > > wrote:
> > > >
> > > > > +static const struct drm_panel_follower_funcs
> > > > > i2c_hid_core_panel_follower_funcs = {
> > > > > + .panel_prepared = i2c_hid_core_panel_prepared,
> > > > > + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> > > > > +};
> > > >
> > > > Can we make that above block at least behind a Kconfig?
> > > >
> > > > i2c-hid is often used for touchpads, and the notion of drm panel has
> > > > nothing to do with them. So I'd be more confident if we could disable
> > > > that code if not required.
> > >
> > > Now that other concerns are addressed, I started trying to write up a
> > > v3 and I found myself writing this as the description of the Kconfig
> > > entry:
> > >
> > > --
> > > config I2C_HID_SUPPORT_PANEL_FOLLOWER
> > > bool "Support i2c-hid devices that must be power sequenced with a panel"
> > >
> > > Say Y here if you want support for i2c-hid devices that need to
> > > coordinate power sequencing with a panel. This is typically important
> > > when you have a panel and a touchscreen that share power rails or
> > > reset GPIOs. If you say N here then the kernel will not try to honor
> > > any shared power sequencing for your hardware. In the best case,
> > > ignoring power sequencing when it's needed will draw extra power. In
> > > the worst case this will prevent your hardware from functioning or
> > > could even damage your hardware.
> > >
> > > If unsure, say Y.
> > >
> > > --
> > >
> > > I can certainly go that way, but I just wanted to truly make sure
> > > that's what we want. Specifically:
> > >
> > > 1. If we put the panel follower code behind a Kconfig then we actually
> > > have no idea if a touchscreen was intended to be a panel follower.
> > > Specifically the panel follower API is the one that detects the
> > > connection between the panel and the i2c-hid device, so without being
> > > able to call the panel follower API we have no idea that an i2c-hid
> > > device was supposed to be a panel follower.
> > >
> > > 2. It is conceivable that power sequencing a device incorrectly could
> > > truly cause hardware damage.
> > >
> > > Together, those points mean that if you turn off the Kconfig entry and
> > > then try to boot on a device that needed that Kconfig setting that you
> > > might damage hardware. I can code it up that way if you want, but it
> > > worries me...
> > >
> > >
> > > Alternatives that I can think of:
> > >
> > > a) I could change the panel follower API so that panel followers are
> > > in charge of detecting the panel that they follow. Today, that looks
> > > like:
> > >
> > >panel_np = of_parse_phandle(dev->of_node, "panel", 0);
> > >if (panel_np)
> > >/* It's a panel follower */
> > >of_node_put(panel_np);
> > >
> > > ...so we could put that code in each touchscreen driver and then fail
> > > to probe i2c-hid if we detect that we're supposed to be a panel
> > > follower but the Kconfig is turned off. The above doesn't seem
> > > massively ideal since it duplicates code. Also, one reason why I put
> > > that code in drm_panel_add_follower() is that I think this concept
> > > will eventually be needed even for non-DT cases. I don't know how to
> > > write the non-DT code right now, though...
> > >
> > >
> > > b) I could open-code detect the panel follower case but leave the
> > > actual linking to the panel follower API. AKA add to i2c-hid:
> > >
> > >if (of_property_read_bool(dev->of_node, "panel"))
> > >/* It's a panel follower */
> > >
> > > ...that's a smaller bit of code, but feels like an abstraction
> > > violation. It also would need to be updated if/when we added support
> > > for non-DT panel followers.
> > >
> > >
> > > c) I could add a "static inline" imple
Re: [PATCH 10/17] hid/picolcd: Remove flag FBINFO_FLAG_DEFAULT from fbdev driver
On Mon, Jul 10, 2023 at 3:01 PM Thomas Zimmermann wrote: > > The flag FBINFO_FLAG_DEFAULT is 0 and has no effect, as struct > fbinfo.flags has been allocated to zero by framebuffer_alloc(). So do > not set it. > > Flags should signal differences from the default values. After cleaning > up all occurences of FBINFO_FLAG_DEFAULT, the token can be removed. > > Signed-off-by: Thomas Zimmermann > Cc: "Bruno Prémont" > Cc: Jiri Kosina > Cc: Benjamin Tissoires Acked-by: Benjamin Tissoires Feel free to take this through the DRI tree (or any other that handles FB) with the rest of the series if you want. Cheers, Benjamin > --- > drivers/hid/hid-picolcd_fb.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c > index dabcd054dad9..d726aaafb146 100644 > --- a/drivers/hid/hid-picolcd_fb.c > +++ b/drivers/hid/hid-picolcd_fb.c > @@ -527,7 +527,6 @@ int picolcd_init_framebuffer(struct picolcd_data *data) > info->var = picolcdfb_var; > info->fix = picolcdfb_fix; > info->fix.smem_len = PICOLCDFB_SIZE*8; > - info->flags = FBINFO_FLAG_DEFAULT; > > fbdata = info->par; > spin_lock_init(>lock); > -- > 2.41.0 >
Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
On Thu, Jun 8, 2023 at 6:43 PM Doug Anderson wrote:
>
> Hi,
>
> On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires
> wrote:
> >
> >
> > On Jun 07 2023, Douglas Anderson wrote:
> > >
> > > As talked about in the patch ("drm/panel: Add a way for other devices
> > > to follow panel state"), we really want to keep the power states of a
> > > touchscreen and the panel it's attached to in sync with each other. In
> > > that spirit, add support to i2c-hid to be a panel follower. This will
> > > let the i2c-hid driver get informed when the panel is powered on and
> > > off. From there we can match the i2c-hid device's power state to that
> > > of the panel.
> > >
> > > NOTE: this patch specifically _doesn't_ use pm_runtime to keep track
> > > of / manage the power state of the i2c-hid device, even though my
> > > first instinct said that would be the way to go. Specific problems
> > > with using pm_runtime():
> > > * The initial power up couldn't happen in a runtime resume function
> > > since it create sub-devices and, apparently, that's not good to do
> > > in your resume function.
> > > * Managing our power state with pm_runtime meant fighting to make the
> > > right thing happen at system suspend to prevent the system from
> > > trying to resume us only to suspend us again. While this might be
> > > able to be solved, it added complexity.
> > > Overall the code without pm_runtime() ended up being smaller and
> > > easier to understand.
> >
> > Generally speaking, I'm not that happy when we need to coordinate with
> > other subsystems for bringing up resources...
>
> Yeah, I'd agree that it's not amazingly elegant. Unfortunately, I
> couldn't find any existing clean frameworks that would do what was
> needed, which is (presumably) why this problem hasn't been solved
> before. I could try to come up with a grand abstraction / new
> framework, but that doesn't seem like a great choice either unless we
> expect more users...
>
>
> > Anyway, a remark inlined (at least):
> >
> > >
> > > Signed-off-by: Douglas Anderson
> > > ---
> > >
> > > Changes in v2:
> > > - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
> > >
> > > drivers/hid/i2c-hid/i2c-hid-core.c | 82 +-
> > > 1 file changed, 81 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
> > > b/drivers/hid/i2c-hid/i2c-hid-core.c
> > > index fa8a1ca43d7f..368db3ae612f 100644
> > > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > > @@ -38,6 +38,8 @@
> > > #include
> > > #include
> > >
> > > +#include
> > > +
> > > #include "../hid-ids.h"
> > > #include "i2c-hid.h"
> > >
> > > @@ -107,6 +109,8 @@ struct i2c_hid {
> > > struct mutexreset_lock;
> > >
> > > struct i2chid_ops *ops;
> > > + struct drm_panel_follower panel_follower;
> > > + boolis_panel_follower;
> > > };
> > >
> > > static const struct i2c_hid_quirks {
> > > @@ -1058,6 +1062,34 @@ static int i2c_hid_core_initial_power_up(struct
> > > i2c_hid *ihid)
> > > return ret;
> > > }
> > >
> > > +static int i2c_hid_core_panel_prepared(struct drm_panel_follower
> > > *follower)
> > > +{
> > > + struct i2c_hid *ihid = container_of(follower, struct i2c_hid,
> > > panel_follower);
> > > + struct hid_device *hid = ihid->hid;
> > > +
> > > + /*
> > > + * hid->version is set on the first power up. If it's still zero
> > > then
> > > + * this is the first power on so we should perform initial power up
> > > + * steps.
> > > + */
> > > + if (!hid->version)
> > > + return i2c_hid_core_initial_power_up(ihid);
> > > +
> > > + return i2c_hid_core_resume(ihid);
> > > +}
> > > +
> > > +static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower
> > > *follower)
> > > +{
> > > + struct i2c_hid *ihid = container_of(follower, struct i2c_hid,
> > > panel_follower);
> > > +
> > > + return i2c_hid_core_suspend(ihid);
> > > +}
> > > +
>
Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
On Jun 07 2023, Douglas Anderson wrote:
>
> As talked about in the patch ("drm/panel: Add a way for other devices
> to follow panel state"), we really want to keep the power states of a
> touchscreen and the panel it's attached to in sync with each other. In
> that spirit, add support to i2c-hid to be a panel follower. This will
> let the i2c-hid driver get informed when the panel is powered on and
> off. From there we can match the i2c-hid device's power state to that
> of the panel.
>
> NOTE: this patch specifically _doesn't_ use pm_runtime to keep track
> of / manage the power state of the i2c-hid device, even though my
> first instinct said that would be the way to go. Specific problems
> with using pm_runtime():
> * The initial power up couldn't happen in a runtime resume function
> since it create sub-devices and, apparently, that's not good to do
> in your resume function.
> * Managing our power state with pm_runtime meant fighting to make the
> right thing happen at system suspend to prevent the system from
> trying to resume us only to suspend us again. While this might be
> able to be solved, it added complexity.
> Overall the code without pm_runtime() ended up being smaller and
> easier to understand.
Generally speaking, I'm not that happy when we need to coordinate with
other subsystems for bringing up resources...
Anyway, a remark inlined (at least):
>
> Signed-off-by: Douglas Anderson
> ---
>
> Changes in v2:
> - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
>
> drivers/hid/i2c-hid/i2c-hid-core.c | 82 +-
> 1 file changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
> b/drivers/hid/i2c-hid/i2c-hid-core.c
> index fa8a1ca43d7f..368db3ae612f 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -38,6 +38,8 @@
> #include
> #include
>
> +#include
> +
> #include "../hid-ids.h"
> #include "i2c-hid.h"
>
> @@ -107,6 +109,8 @@ struct i2c_hid {
> struct mutexreset_lock;
>
> struct i2chid_ops *ops;
> + struct drm_panel_follower panel_follower;
> + boolis_panel_follower;
> };
>
> static const struct i2c_hid_quirks {
> @@ -1058,6 +1062,34 @@ static int i2c_hid_core_initial_power_up(struct
> i2c_hid *ihid)
> return ret;
> }
>
> +static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
> +{
> + struct i2c_hid *ihid = container_of(follower, struct i2c_hid,
> panel_follower);
> + struct hid_device *hid = ihid->hid;
> +
> + /*
> + * hid->version is set on the first power up. If it's still zero then
> + * this is the first power on so we should perform initial power up
> + * steps.
> + */
> + if (!hid->version)
> + return i2c_hid_core_initial_power_up(ihid);
> +
> + return i2c_hid_core_resume(ihid);
> +}
> +
> +static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower
> *follower)
> +{
> + struct i2c_hid *ihid = container_of(follower, struct i2c_hid,
> panel_follower);
> +
> + return i2c_hid_core_suspend(ihid);
> +}
> +
> +static const struct drm_panel_follower_funcs
> i2c_hid_core_panel_follower_funcs = {
> + .panel_prepared = i2c_hid_core_panel_prepared,
> + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> +};
Can we make that above block at least behind a Kconfig?
i2c-hid is often used for touchpads, and the notion of drm panel has
nothing to do with them. So I'd be more confident if we could disable
that code if not required.
Actually, I'd be even more happier if it were in a different compilation
unit. Not necessary a different module, but at least a different file.
Cheers,
Benjamin
> +
> int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> u16 hid_descriptor_address, u32 quirks)
> {
> @@ -1119,6 +1151,41 @@ int i2c_hid_core_probe(struct i2c_client *client,
> struct i2chid_ops *ops,
> hid->bus = BUS_I2C;
> hid->initial_quirks = quirks;
>
> + /*
> + * See if we're following a panel. If drm_panel_add_follower()
> + * returns no error then we are.
> + */
> + ihid->panel_follower.funcs = _hid_core_panel_follower_funcs;
> + ret = drm_panel_add_follower(>dev, >panel_follower);
> + if (!ret) {
> + /* We're a follower. That means we'll power things up later. */
> + ihid->is_panel_follower = true;
> +
> + /*
> + * If we're not in control of our own power up/power down then
> + * we can't do the logic to manage wakeups. Give a warning if
> + * a user thought that was possible then force the capability
> + * off.
> + */
> + if (device_can_wakeup(>dev)) {
> + dev_warn(>dev, "Can't wakeup if following
> panel\n");
> +
Re: [PATCH V3 6/13] input: serio: use time_is_before_jiffies() instead of open coding it
On Tue, Feb 15, 2022 at 2:57 AM Qing Wang wrote:
>
> From: Wang Qing
>
> Use the helper function time_is_{before,after}_jiffies() to improve
> code readability.
>
> Signed-off-by: Wang Qing
> ---
Reviewed-by: Benjamin Tissoires
Cheers,
Benjamin
> drivers/input/serio/ps2-gpio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c
> index 8970b49..7834296
> --- a/drivers/input/serio/ps2-gpio.c
> +++ b/drivers/input/serio/ps2-gpio.c
> @@ -136,7 +136,7 @@ static irqreturn_t ps2_gpio_irq_rx(struct ps2_gpio_data
> *drvdata)
> if (old_jiffies == 0)
> old_jiffies = jiffies;
>
> - if ((jiffies - old_jiffies) > usecs_to_jiffies(100)) {
> + if (time_is_before_jiffies(old_jiffies + usecs_to_jiffies(100))) {
> dev_err(drvdata->dev,
> "RX: timeout, probably we missed an interrupt\n");
> goto err;
> @@ -237,7 +237,7 @@ static irqreturn_t ps2_gpio_irq_tx(struct ps2_gpio_data
> *drvdata)
> if (old_jiffies == 0)
> old_jiffies = jiffies;
>
> - if ((jiffies - old_jiffies) > usecs_to_jiffies(100)) {
> + if (time_is_before_jiffies(old_jiffies + usecs_to_jiffies(100))) {
> dev_err(drvdata->dev,
> "TX: timeout, probably we missed an interrupt\n");
> goto err;
> --
> 2.7.4
>
Re: [PATCH V3 5/13] hid: use time_is_after_jiffies() instead of open coding it
On Tue, Feb 15, 2022 at 2:56 AM Qing Wang wrote:
>
> From: Wang Qing
>
> Use the helper function time_is_{before,after}_jiffies() to improve
> code readability.
>
> Signed-off-by: Wang Qing
> Acked-by: Srinivas Pandruvada
FWIW, this one is
Acked-by: Benjamin Tissoires
Wang, is there any plan to take this series through the trivial tree
or should each maintainer take the matching patches?
Cheers,
Benjamin
> ---
> drivers/hid/intel-ish-hid/ipc/ipc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c
> b/drivers/hid/intel-ish-hid/ipc/ipc.c
> index 8ccb246..15e1423
> --- a/drivers/hid/intel-ish-hid/ipc/ipc.c
> +++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
> @@ -578,7 +578,7 @@ static void _ish_sync_fw_clock(struct ishtp_device *dev)
> static unsigned longprev_sync;
> uint64_tusec;
>
> - if (prev_sync && jiffies - prev_sync < 20 * HZ)
> + if (prev_sync && time_is_after_jiffies(prev_sync + 20 * HZ))
> return;
>
> prev_sync = jiffies;
> --
> 2.7.4
>
Re: [RESEND 00/25] Rid W=1 warnings from HID
On Thu, Apr 8, 2021 at 9:06 AM Lee Jones wrote: > > On Wed, 07 Apr 2021, Benjamin Tissoires wrote: > > > On Tue, Apr 6, 2021 at 10:56 AM Lee Jones wrote: > > > > > > On Fri, 26 Mar 2021, Lee Jones wrote: > > > > > > > This set is part of a larger effort attempting to clean-up W=1 > > > > kernel builds, which are currently overwhelmingly riddled with > > > > niggly little warnings. > > > > > > > > Lee Jones (25): > > > > HID: intel-ish-hid: Remove unused variable 'err' > > > > HID: ishtp-hid-client: Move variable to where it's actually used > > > > HID: intel-ish-hid: pci-ish: Remove unused variable 'ret' > > > > HID: intel-ish: Supply some missing param descriptions > > > > HID: intel-ish: Fix a naming disparity and a formatting error > > > > HID: usbhid: Repair a formatting issue in a struct description > > > > HID: intel-ish-hid: Fix a little doc-rot > > > > HID: usbhid: hid-pidff: Demote a couple kernel-doc abuses > > > > HID: hid-alps: Correct struct misnaming > > > > HID: intel-ish-hid: Fix potential copy/paste error > > > > HID: hid-core: Fix incorrect function name in header > > > > HID: intel-ish-hid: ipc: Correct fw_reset_work_fn() function name in > > > > header > > > > HID: ishtp-hid-client: Fix incorrect function name report_bad_packet() > > > > HID: hid-kye: Fix incorrect function name for kye_tablet_enable() > > > > HID: hid-picolcd_core: Remove unused variable 'ret' > > > > HID: hid-logitech-hidpp: Fix conformant kernel-doc header and demote > > > > abuses > > > > HID: hid-uclogic-rdesc: Kernel-doc is for functions and structs > > > > HID: hid-thrustmaster: Demote a bunch of kernel-doc abuses > > > > HID: hid-uclogic-params: Ensure function names are present and correct > > > > in kernel-doc headers > > > > HID: hid-sensor-custom: Remove unused variable 'ret' > > > > HID: wacom_sys: Demote kernel-doc abuse > > > > HID: hid-sensor-hub: Remove unused struct member 'quirks' > > > > HID: hid-sensor-hub: Move 'hsdev' description to correct struct > > > > definition > > > > HID: intel-ish-hid: ishtp-fw-loader: Fix a bunch of formatting issues > > > > HID: ishtp-hid-client: Fix 'suggest-attribute=format' compiler warning > > > > > > These have been on the list for a couple of weeks now. > > > > > > Is there anything I can do to help expedite their merge? > > > > > > I'm concerned since -rc6 has just been released. > > > > Sorry for the delay. > > > > I am currently queuing them locally and running a few tests on them. I > > don't expect anything to happen, but better be safe than anything. > > > > FWIW, I am splitting the series in 3: > > - 11 patches for intel ish are going to be queued in for-5.13/intel-ish > > - the thrustmaster one in for-5.13/thrustmaster > > - the rest (13 patches) will be sent in for-5.13/warnings. > > Sounds good to me. Thanks Benjamin. > After a few attempts at fixing my CI, I have now pushed this series as mentioned previously. Cheers, Benjamin ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND 00/25] Rid W=1 warnings from HID
On Tue, Apr 6, 2021 at 10:56 AM Lee Jones wrote: > > On Fri, 26 Mar 2021, Lee Jones wrote: > > > This set is part of a larger effort attempting to clean-up W=1 > > kernel builds, which are currently overwhelmingly riddled with > > niggly little warnings. > > > > Lee Jones (25): > > HID: intel-ish-hid: Remove unused variable 'err' > > HID: ishtp-hid-client: Move variable to where it's actually used > > HID: intel-ish-hid: pci-ish: Remove unused variable 'ret' > > HID: intel-ish: Supply some missing param descriptions > > HID: intel-ish: Fix a naming disparity and a formatting error > > HID: usbhid: Repair a formatting issue in a struct description > > HID: intel-ish-hid: Fix a little doc-rot > > HID: usbhid: hid-pidff: Demote a couple kernel-doc abuses > > HID: hid-alps: Correct struct misnaming > > HID: intel-ish-hid: Fix potential copy/paste error > > HID: hid-core: Fix incorrect function name in header > > HID: intel-ish-hid: ipc: Correct fw_reset_work_fn() function name in > > header > > HID: ishtp-hid-client: Fix incorrect function name report_bad_packet() > > HID: hid-kye: Fix incorrect function name for kye_tablet_enable() > > HID: hid-picolcd_core: Remove unused variable 'ret' > > HID: hid-logitech-hidpp: Fix conformant kernel-doc header and demote > > abuses > > HID: hid-uclogic-rdesc: Kernel-doc is for functions and structs > > HID: hid-thrustmaster: Demote a bunch of kernel-doc abuses > > HID: hid-uclogic-params: Ensure function names are present and correct > > in kernel-doc headers > > HID: hid-sensor-custom: Remove unused variable 'ret' > > HID: wacom_sys: Demote kernel-doc abuse > > HID: hid-sensor-hub: Remove unused struct member 'quirks' > > HID: hid-sensor-hub: Move 'hsdev' description to correct struct > > definition > > HID: intel-ish-hid: ishtp-fw-loader: Fix a bunch of formatting issues > > HID: ishtp-hid-client: Fix 'suggest-attribute=format' compiler warning > > These have been on the list for a couple of weeks now. > > Is there anything I can do to help expedite their merge? > > I'm concerned since -rc6 has just been released. Sorry for the delay. I am currently queuing them locally and running a few tests on them. I don't expect anything to happen, but better be safe than anything. FWIW, I am splitting the series in 3: - 11 patches for intel ish are going to be queued in for-5.13/intel-ish - the thrustmaster one in for-5.13/thrustmaster - the rest (13 patches) will be sent in for-5.13/warnings. Cheers, Benjamin ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] video: fbdev: don't print error message on framebuffer_alloc() failure
Hi Bartlomiej,
On Fri, Jun 14, 2019 at 4:52 PM Bartlomiej Zolnierkiewicz
wrote:
>
> framebuffer_alloc() can fail only on kzalloc() memory allocation
> failure and since kzalloc() will print error message in such case
> we can omit printing extra error message in drivers (which BTW is
> what the majority of framebuffer_alloc() users is doing already).
>
> Cc: "Bruno Prémont"
> Cc: Jiri Kosina
> Cc: Benjamin Tissoires
> Signed-off-by: Bartlomiej Zolnierkiewicz
> ---
> drivers/hid/hid-picolcd_fb.c |4 +---
> drivers/video/fbdev/amifb.c|4 +---
> drivers/video/fbdev/arkfb.c|4 +---
> drivers/video/fbdev/atmel_lcdfb.c |4 +---
> drivers/video/fbdev/aty/aty128fb.c |5 ++---
> drivers/video/fbdev/aty/atyfb_base.c | 10 --
> drivers/video/fbdev/aty/radeon_base.c |2 --
> drivers/video/fbdev/chipsfb.c |1 -
> drivers/video/fbdev/cirrusfb.c |5 +
> drivers/video/fbdev/da8xx-fb.c |1 -
> drivers/video/fbdev/efifb.c|1 -
> drivers/video/fbdev/grvga.c|4 +---
> drivers/video/fbdev/gxt4500.c |5 ++---
> drivers/video/fbdev/hyperv_fb.c|4 +---
> drivers/video/fbdev/i740fb.c |4 +---
> drivers/video/fbdev/imsttfb.c |5 +
> drivers/video/fbdev/intelfb/intelfbdrv.c |5 ++---
> drivers/video/fbdev/jz4740_fb.c|4 +---
> drivers/video/fbdev/mb862xx/mb862xxfbdrv.c |5 +
> drivers/video/fbdev/mbx/mbxfb.c|4 +---
> drivers/video/fbdev/omap/omapfb_main.c |2 --
> drivers/video/fbdev/omap2/omapfb/omapfb-main.c |6 +-
> drivers/video/fbdev/platinumfb.c |5 ++---
> drivers/video/fbdev/pmag-aa-fb.c |4 +---
> drivers/video/fbdev/pmag-ba-fb.c |4 +---
> drivers/video/fbdev/pmagb-b-fb.c |4 +---
> drivers/video/fbdev/pvr2fb.c |6 +-
> drivers/video/fbdev/riva/fbdev.c |1 -
> drivers/video/fbdev/s3c-fb.c |4 +---
> drivers/video/fbdev/s3fb.c |4 +---
> drivers/video/fbdev/sh_mobile_lcdcfb.c |8 ++--
> drivers/video/fbdev/sm501fb.c |4 +---
> drivers/video/fbdev/sm712fb.c |1 -
> drivers/video/fbdev/smscufx.c |4 +---
> drivers/video/fbdev/ssd1307fb.c|4 +---
> drivers/video/fbdev/sunxvr1000.c |1 -
> drivers/video/fbdev/sunxvr2500.c |1 -
> drivers/video/fbdev/sunxvr500.c|1 -
> drivers/video/fbdev/tgafb.c|4 +---
> drivers/video/fbdev/udlfb.c|4 +---
> drivers/video/fbdev/via/viafbdev.c |6 +-
> drivers/video/fbdev/vt8623fb.c |4 +---
> 42 files changed, 40 insertions(+), 123 deletions(-)
>
> Index: b/drivers/hid/hid-picolcd_fb.c
> ===
> --- a/drivers/hid/hid-picolcd_fb.c
> +++ b/drivers/hid/hid-picolcd_fb.c
> @@ -522,10 +522,8 @@ int picolcd_init_framebuffer(struct pico
> sizeof(struct fb_deferred_io) +
> sizeof(struct picolcd_fb_data) +
> PICOLCDFB_SIZE, dev);
> - if (info == NULL) {
> - dev_err(dev, "failed to allocate a framebuffer\n");
> + if (!info)
> goto err_nomem;
> - }
It would have been better to split this change as the HID and fbdev
are different trees.
However, I do not expect a conflict here (there hasn't been updates of
hid-picolcd_fb.c in a while), so feel free to take this patch through
the fbdev tree with my:
Acked-By: Benjamin Tissoires
Cheers,
Benjamin
>
> info->fbdefio = info->par;
> *info->fbdefio = picolcd_fb_defio;
> Index: b/drivers/video/fbdev/amifb.c
> ===
> --- a/drivers/video/fbdev/amifb.c
> +++ b/drivers/video/fbdev/amifb.c
> @@ -3554,10 +3554,8 @@ static int __init amifb_probe(struct pla
> custom.dmacon = DMAF_ALL | DMAF_MASTER;
>
> info = framebuffer_alloc(sizeof(struct amifb_par), >dev);
> - if (!info) {
> - dev_err(>dev, "framebuffer_alloc failed\n");
> + if (!info)
> return -ENOMEM;
> - }
>
> strcpy(info->fix.id, "Amiga ");
>
Re: [git pull] drm tree for 3.12-rc1
On Fri, Sep 6, 2013 at 2:56 AM, Linus Torvalds torva...@linux-foundation.org wrote: On Thu, Sep 5, 2013 at 4:19 PM, Linus Torvalds torva...@linux-foundation.org wrote: So I've decided I'm going to try to bisect this after all. I've done enough pulls for today anyway, I guess. Let's see if I can bisect it by just trying to boot many times each try. Ok, it's not the recent drm pull at all. I can't find a good kernel in the bunch - they all fail eventually. It may have been going in for as long as I've had this Haswell machine, and I was just lucky (and not rebooting a lot until in the merge window - and 4/5 boots work fine). It may also be user-space and have come in with the mesa update I got through yum yesterday. So there might be multiple reasons why I saw it today after the drm pull for the first time. The black screen - when it happens - happens after the fedora logo has flashed, and gdm is supposed to start up. I tried reproducing it by logging out and back in again (to restart X), but that doesn't do it. Maybe timing-related with boot or just demand-loading of binaries the first time, whatever.. Or mayby it's something special that gdm does at startup? Hi Linus, this _may_ be related to: https://bugzilla.redhat.com/show_bug.cgi?id=989763 I also have a 3rd gen Intel core GPU (so not HSW) which shut off the backlight while launching GDM (but for me, it happens at each boot). Using the brightness increase keyboard shortcut is enough for me to light up the backlight and see the session login. Not sure it will solve the problem, but it worth trying :) Cheers, Benjamin ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Dual-LVDS Acer Iconia laptop] i915/DRM issue: one screen stays off [3.2-rc4+]
Hi, On Tue, Dec 6, 2011 at 23:31, Baptiste Jonglez wrote: > On Tue, Dec 06, 2011 at 11:12:26PM +0100, Benjamin Tissoires wrote: >> Hi Baptiste, > > Hi, > >> On Tue, Dec 6, 2011 at 22:51, Baptiste Jonglez >> wrote: >> > The second screen works fine with the attached patch. It actually is >> > 6 months old but seems to have been lost in the wild... >> >> You don't have the problem of the second backlight still off? >> On our Iconia, we need to trigger a special DMI command to set it up >> (SDSS, IIRC). > > No, with the patch, it worked out-of-the-box. mmm, this is weird. We must not have the same bios or device version. We need to explicitly set the LCD2 up. > > I can even control the brightness in > `/sys/class/backlight/acpi_video0/brightness' (it affects both > displays at the same time though). But even at 0, it's still perfectly > readable. Yes, the acpi can control both brightness. But WMI can control each brightness. However, we are not sure this is what people want. So we do not plan to introduce a second brightness control. > > Maybe this a bug that got fixed recently? I've actually tried the 3.1 > kernel (and then the 3.2-rc4) because I noticed a lot of commits and > improvements in the i915 driver recently. We doubled-checked on 3.2-rc4, and we still need our patch. This is definitively a bios/hardware configuration problem. Cheers, Benjamin > >> > Thanks Benjamin! >> >> All the credits are from Ajax and somebody else on IRC I don't recall, >> really sorry. Thanks to them. >> >> Cheers, >> Benjamin. > > Regards, > Baptiste
Re: [Dual-LVDS Acer Iconia laptop] i915/DRM issue: one screen stays off [3.2-rc4+]
Hi, On Tue, Dec 6, 2011 at 23:31, Baptiste Jonglez bapti...@jonglez.org wrote: On Tue, Dec 06, 2011 at 11:12:26PM +0100, Benjamin Tissoires wrote: Hi Baptiste, Hi, On Tue, Dec 6, 2011 at 22:51, Baptiste Jonglez bapti...@jonglez.org wrote: The second screen works fine with the attached patch. It actually is 6 months old but seems to have been lost in the wild... You don't have the problem of the second backlight still off? On our Iconia, we need to trigger a special DMI command to set it up (SDSS, IIRC). No, with the patch, it worked out-of-the-box. mmm, this is weird. We must not have the same bios or device version. We need to explicitly set the LCD2 up. I can even control the brightness in `/sys/class/backlight/acpi_video0/brightness' (it affects both displays at the same time though). But even at 0, it's still perfectly readable. Yes, the acpi can control both brightness. But WMI can control each brightness. However, we are not sure this is what people want. So we do not plan to introduce a second brightness control. Maybe this a bug that got fixed recently? I've actually tried the 3.1 kernel (and then the 3.2-rc4) because I noticed a lot of commits and improvements in the i915 driver recently. We doubled-checked on 3.2-rc4, and we still need our patch. This is definitively a bios/hardware configuration problem. Cheers, Benjamin Thanks Benjamin! All the credits are from Ajax and somebody else on IRC I don't recall, really sorry. Thanks to them. Cheers, Benjamin. Regards, Baptiste ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Dual-LVDS Acer Iconia laptop] i915/DRM issue: one screen stays off [3.2-rc4+]
Hi Baptiste, On Tue, Dec 6, 2011 at 22:51, Baptiste Jonglez bapti...@jonglez.org wrote: On Mon, Dec 05, 2011 at 11:00:41AM +0800, joeyli wrote: Add Cc. to platform-driver-x86 and linux-acpi Hi Baptiste 於 日，2011-12-04 於 17:07 +0100，Baptiste Jonglez 提到： Hi, I've got a lot of troubles with a dual-LVDS Acer laptop (it doesn't have a keyboard, but two displays with touchscreens) The Intel GPU is integrated into the Core i5-480M CPU: it's a bit older than Sandybridge, as it seems to be based on the Arrandale micro-architecture. In the BIOS, both displays work fine; but as soon as the kernel boots up, the second display (i.e. the one where you usually find a keyboard) is turned off. The main display works as expected. xrandr reports two LVDS displays: LVDS1, which is connected, and LVDS2, which is marked as disconnected. No matter what I tried, I can't bring that second display up. During the boot, just after the drm is set up, the following message shows up: [drm:intel_dsm_pci_probe] *ERROR* failed to get supported _DSM functions (attached is the relevant part of dmesg [1]) The second screen works fine with the attached patch. It actually is 6 months old but seems to have been lost in the wild... You don't have the problem of the second backlight still off? On our Iconia, we need to trigger a special DMI command to set it up (SDSS, IIRC). Thanks Benjamin! All the credits are from Ajax and somebody else on IRC I don't recall, really sorry. Thanks to them. Cheers, Benjamin. There is still the issue of unhandled acer-wmi events, but it's far less incapacitating. I wonder what's the best way to report events to userspace, though (e.g. for the keyboard button, userspace might want to know when it is pressed in order to display a virtual keyboard or any other fancy stuff) Joey, if you need more logs for acer-wmi, I'll be happy to provide. Baptiste ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Dual-LVDS Acer Iconia laptop] i915/DRM issue: one screen stays off [3.2-rc4+]
Hi Baptiste, On Tue, Dec 6, 2011 at 22:51, Baptiste Jonglez wrote: > On Mon, Dec 05, 2011 at 11:00:41AM +0800, joeyli wrote: >> Add Cc. to platform-driver-x86 and linux-acpi >> >> Hi Baptiste >> >> ? ??2011-12-04 ? 17:07 +0100?Baptiste Jonglez ??? >> > Hi, >> > >> > I've got a lot of troubles with a dual-LVDS Acer laptop (it doesn't >> > have a keyboard, but two displays with touchscreens) >> > >> > The Intel GPU is integrated into the Core i5-480M CPU: it's a bit >> > older than Sandybridge, as it seems to be based on the Arrandale >> > micro-architecture. >> > >> > In the BIOS, both displays work fine; but as soon as the kernel boots >> > up, the second display (i.e. the one where you usually find a >> > keyboard) is turned off. The main display works as expected. >> > >> > xrandr reports two LVDS displays: LVDS1, which is connected, and >> > LVDS2, which is marked as "disconnected". No matter what I tried, I >> > can't bring that second display up. >> > >> > During the boot, just after the drm is set up, the following message >> > shows up: >> > >> > [drm:intel_dsm_pci_probe] *ERROR* failed to get supported _DSM functions >> > >> > (attached is the relevant part of dmesg [1]) > > The second screen works fine with the attached patch. It actually is > 6 months old but seems to have been lost in the wild... You don't have the problem of the second backlight still off? On our Iconia, we need to trigger a special DMI command to set it up (SDSS, IIRC). > > Thanks Benjamin! All the credits are from Ajax and somebody else on IRC I don't recall, really sorry. Thanks to them. Cheers, Benjamin. > > There is still the issue of unhandled acer-wmi events, but it's far > less incapacitating. I wonder what's the best way to report events to > userspace, though (e.g. for the "keyboard" button, userspace might > want to know when it is pressed in order to display a virtual keyboard > or any other fancy stuff) > > Joey, if you need more logs for acer-wmi, I'll be happy to provide. > > > Baptiste
