Sorry Lukas, for forgetting these comments.
I'll take these comments to this thread as the other thread is a dead
end. I think the drm_panel_attach() (in this patch) is a better place to
add the device link than panel_bridge_attach() (the other patch).
On 21/02/18 09:52, Lukas Wunner wrote:
> On Wed, Feb 21, 2018 at 12:21:50AM +0200, Jyri Sarha wrote:
>> @@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge
*bridge)
>> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>>
>> drm_panel_detach(panel_bridge->panel);
>> +
>> + device_link_del(panel_bridge->link);
>
> No, you've set the DL_FLAG_AUTOREMOVE flag, so you'll end up removing
> the link twice, which oopses.
>
> It's either DL_FLAG_AUTOREMOVE or device_link_del(), not both.
>
Oh yes. I'll drop the device_link_del(). Now reading the code more
carefully, the link del from the driver is obviously redundant. However,
it does not cause a crash if the link is only deleted from the consumer
size.
There is still one argument for having the link del in panel detach
however: If some drm device would behave more dynamically, and could
detach a panel on the fly but stay operational still, then it would be
correct to call link del from the drm driver. But if we ever have such a
device, we can solve that issue separately.
>
>> +static int panel_bridge_link_to_master(struct panel_bridge
*panel_bridge)
>> +{
>> + struct device *mdev = panel_bridge->bridge.dev->dev;
>> + struct device *pdev = panel_bridge->panel->dev;
>> + u32 flags = DL_FLAG_AUTOREMOVE;
>> +
>> + panel_bridge->link = device_link_add(mdev, pdev, flags);
>> + if (!panel_bridge->link) {
>> + dev_err(pdev, "failed to link panel %s to %s\n",
>> + dev_name(pdev), dev_name(mdev));
>
> You're printing two instances of pdev's name in the log message,
> one should be sufficient.
>
Oh yes, I'll drop the panel device name from the format string.
> Also, you've mixed up the order: mdev is the consumer, pdev the
> supplier.
>
Yes, I noticed that myself. The new patch does not have this problem.
> (Bikeshed: The DL_FLAG_AUTOREMOVE would still fit within 80 chars
> on the line with device_link_add() and the flags variable wouldn't
> have to be declared then. Your call.)
>
This time I need the flags for having the line under 80 chars.
> Thanks,
>
> Lukas
>
Best regards,
Jyri
On 23/02/18 08:58, Lukas Wunner wrote:
> On Thu, Feb 22, 2018 at 07:46:27PM +0200, Jyri Sarha wrote:
>> Add device_link from panel device (supplier) to drm device (consumer)
>> with DL_FLAG_AUTOREMOVE when drm_panel_attach() is called. Currently
>> the master drm driver is not protected against the attached. The
>> device_link with DL_FLAG_AUTOREMOVE should make sure the drm device is
>> unbound before the panel driver becomes unavailable.
>>
>> The device_link is removed when drm_panel_detach() is called. The
>> drm_panel_detach() should be called by the panel driver it self when
>> it is removed. Otherwise the both driver are racing to delete the same
>> link.
>
> Unfortunately you haven't addressed any of my comments on the previous
> version of this patch:
>
> https://www.spinics.net/lists/dri-devel/msg166320.html
>
> Please respin with my comments addressed. Thanks,
>
> Lukas
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
