Hi,
On 4/15/20 2:01 PM, Daniel Vetter wrote:
On Wed, Apr 15, 2020 at 01:39:23PM +0200, Hans de Goede wrote:
Hi,
On 4/15/20 12:22 PM, Daniel Vetter wrote:
On Wed, Apr 15, 2020 at 12:11 PM Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 4/15/20 11:52 AM, Daniel Vetter wrote:
<snip>
iv. What every SoC subsystem does:
- lcdshadow drivers register drivers
- drm drivers look them up
- if stuff isn't there yet, we delay loading with EPROBE_DEFER until
the entire thing is assembled.
That's what we're doing already for other standardized components like drm_bridge or drm_panel, and I think that's also the right approach for backlight and anything else like that. Hand-rolling our own EPROBE_DEFER handling, or some other duct-tape monsters imo just leads to real pain. Also, with EPROBE_DEFER we have one standard way of building a driver from component, which spans subsystems and is also the underlying magic that makes stuff like component.c work.
On the SoCs we have devicetree telling us what components there are, so we can wait for them to show up. The only way to figure out if the lcdshadow thing is there on a ThinkPad is asking thinkpad_acpi, or duplicating a lot of code from thinkpad_acpi. Edit: also see below for a possible solution.
Yup it sucks. I think all we can do is have a small acpi match function (which yes will duplicate some of the thinkpad_acpi driver logic) to re-create that information and give us a "should we have a lcdshadow driver for this $pci_device" answer.
Ok, so questions about this solution:
Where should that match-function live ?
An acpi_thinkpad derived match-function will only be able to answer if there is an lcdshadow device/driver for the internal panel. It will not be able to tie this info to a certain PCI device. My plan is to pass NULL as dev_name when registering the lcdshadow-device and have lcdshadow_get(dev, <connector-name>) skip device-name matching (consider everything a match) for lcdshadow-devices registered with NULL as dev_name.
So I guess in this case the mini match function should just ignore the passed in device?
Yeah I think we can't really avoid that. I also expect that we'll need ACPI and dt versions of this, and driver needs to know which one to call. Since at least in a dt world the driver knows exactly for which dt node it needs a lcdshadow driver for (with the phandle stuff), so we can be a lot more strict.
For the acpi version I'm not even sure we can do more than provide the struct device * pointer of the gpu. I think if we ever get more than 1 lcdshadow driver on acpi systems we can add more stuff later on, for now I'd just leave that out.
So maybe
acpi_lcdshadow_get(struct device *dev);
of_lcdshadow_get(struct device_node *np);
And with maybe a future plan to add some kind of enum or whatever to acpi_lcdshadow_get(). Both would return either the lcdshadow pointer, or an PTR_ERR() so that we could encode EPROBE_DEFER vs ENOENT.
Ok, note I plan to only implement the acpi version for now, I do expect some non ACPI/x86 devices to show up with his feature eventually but I believe it is best to implement this once those actually show up. Esp. since this will also involve adding some devicetree bindings for this.
We might also want a low-level lcdshadow_get() which only returns ENOENT when the driver isn't there, and which leaves "do we really need one?" to higher levels to answer.
Right, so my latest idea on that is indeed a high-level lcdshadow_get() which takes a struct device * and a connector-name and which never returns EPROBE_DEFER.
As for leaving things to the higher levels to answer, as explained in my other follow-up email I think that we should probably add a lcdshadow_probe_defer() helper for this and call that early on in the PCI-driver probe functions for the 3 major x86 GPU drivers. Does that sound ok to you?
I'd also lean towards putting lcdshadow headers/interfaces into drivers/gpu,
Ack, I think we should even make this drm specific and prefix it with drm_ so that we get drm_lcdshadow_foo as functions, just to make clear that this is maintained together with the other drm bits.
But my question about "where should this live" was mainly about the light weight match helpers you suggested to use to figure out if the device supports lcdshadow at all (and we thus should wait for a driver) or not. E.g. I can see us adding a:
drivers/gpu/drm/drm_lcdshadow.c
file for the core bits and then maybe a:
drivers/gpu/drm/drm_lcdshadow_detect.c
file with the light weight match helpers, with each helper wrapped in #if IS_ENABLED(CONFIG_THINKPAD_ACPI), etc. ?
with driver implementations all over.
Ack.
Regards,
Hans