On Mon, 09 Jun 2025, Doug Anderson <diand...@chromium.org> wrote:
> Hi,
>
> On Fri, Jun 6, 2025 at 2:06 AM Jani Nikula <jani.nik...@intel.com> wrote:
>>
>> Use firmware node based lookups for panel followers, to make the code
>> independent of OF and device tree, and make it work also for ACPI with
>> an appropriate _DSD.
>>
>> ASL example:
>>
>> Package (0x02)
>> {
>> "panel", \_SB.PCI0.GFX0.LCD0
>> }
>>
>> Suggested-by: Heikki Krogerus <heikki.kroge...@linux.intel.com>
>> Cc: Neil Armstrong <neil.armstr...@linaro.org>
>> Cc: Jessica Zhang <jessica.zh...@oss.qualcomm.com>
>> Cc: Maxime Ripard <mrip...@kernel.org>
>> Cc: Doug Anderson <diand...@chromium.org>
>> Cc: Lee Shawn C <shawn.c....@intel.com>
>> Signed-off-by: Jani Nikula <jani.nik...@intel.com>
>> ---
>> drivers/gpu/drm/drm_panel.c | 39 +++++++++++++++++++++++++++++--------
>> 1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
>> index fee65dc65979..3eb0a615f7a9 100644
>> --- a/drivers/gpu/drm/drm_panel.c
>> +++ b/drivers/gpu/drm/drm_panel.c
>> @@ -473,17 +473,40 @@ int of_drm_get_panel_orientation(const struct
>> device_node *np,
>> EXPORT_SYMBOL(of_drm_get_panel_orientation);
>> #endif
>>
>> -static struct drm_panel *of_find_panel(struct device *follower_dev)
>> +/* Find panel by fwnode */
>> +static struct drm_panel *find_panel_by_fwnode(const struct fwnode_handle
>> *fwnode)
>
> nit: It might be worth adding a comment that says it should be
> identical to of_drm_find_panel() since that has a much richer
> kerneldoc that talks about the corner cases.
Agreed.
I'm actually wondering if it would be possible to implement
of_drm_find_panel() like this (as a follow-up change):
struct drm_panel *of_drm_find_panel(const struct device_node *np)
{
const struct fwnode_handle *fwnode = of_fwnode_handle(np);
return find_panel_by_fwnode(fwnode);
}
But I'd rather leave that out for now.
>
>> {
>> - struct device_node *panel_np;
>> struct drm_panel *panel;
>>
>> - panel_np = of_parse_phandle(follower_dev->of_node, "panel", 0);
>> - if (!panel_np)
>> + if (!fwnode_device_is_available(fwnode))
>> return ERR_PTR(-ENODEV);
>>
>> - panel = of_drm_find_panel(panel_np);
>> - of_node_put(panel_np);
>> + mutex_lock(&panel_lock);
>> +
>> + list_for_each_entry(panel, &panel_list, list) {
>> + if (dev_fwnode(panel->dev) == fwnode) {
>> + mutex_unlock(&panel_lock);
>> + return panel;
>> + }
>> + }
>> +
>> + mutex_unlock(&panel_lock);
>> +
>> + return ERR_PTR(-EPROBE_DEFER);
>> +}
>> +
>> +/* Find panel by follower device */
>> +static struct drm_panel *find_panel_by_dev(struct device *follower_dev)
>> +{
>> + struct fwnode_handle *fwnode;
>> + struct drm_panel *panel;
>> +
>> + fwnode = fwnode_find_reference(dev_fwnode(follower_dev), "panel", 0);
>> + if (IS_ERR_OR_NULL(fwnode))
>
> nit: why IS_ERR_OR_NULL() instead of IS_ERR()? The kerneldoc for
> fwnode_find_reference() doesn't mention anything about it returning a
> NULL value in any cases...
Will fix.
> Other than the nits, this looks reasonable to me.
>
> Reviewed-by: Douglas Anderson <diand...@chromium.org>
>
> I no longer have any easy access to hardware where panel-follower is
> truly necessary, but I can at least see the panel-follower calls
> getting made on sc7180-trogdor-lazor, so the of->fwnode conversion
> stuff must be working.
>
> Tested-by: Douglas Anderson <diand...@chromium.org>
Thanks for the review and testing, much appreciated!
BR,
Jani.
--
Jani Nikula, Intel
