Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Andrzej Hajda]

On 04.05.2018 15:52, Peter Rosin wrote:
> If the bridge supplier is unbound, this will bring the bridge consumer
> down along with the bridge. Thus, there will no longer linger any
> dangling pointers from the bridge consumer (the drm_device) to some
> non-existent bridge supplier.
>
> Signed-off-by: Peter Rosin <p...@axentia.se>
> ---
>  drivers/gpu/drm/drm_bridge.c | 18 ++
>  include/drm/drm_bridge.h |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 78d186b6831b..0259f0a3ff27 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include 
>  
>  #include 
> +#include 
>  #include 
>  
>  #include "drm_crtc_internal.h"
> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
> struct drm_bridge *bridge,
>   if (bridge->dev)
>   return -EBUSY;
>  
> + if (encoder->dev->dev != bridge->odev) {

I wonder why device_link_add does not handle this case (self dependency)
silently as noop, as it seems to be a correct behavior.

> + bridge->link = device_link_add(encoder->dev->dev,
> +bridge->odev, 0);
> + if (!bridge->link) {
> + dev_err(bridge->odev, "failed to link bridge to %s\n",
> + dev_name(encoder->dev->dev));
> + return -EINVAL;
> + }
> + }
> +
>   bridge->dev = encoder->dev;
>   bridge->encoder = encoder;
>  
>   if (bridge->funcs->attach) {
>   ret = bridge->funcs->attach(bridge);
>   if (ret < 0) {
> + if (bridge->link)
> + device_link_del(bridge->link);
> + bridge->link = NULL;
>   bridge->dev = NULL;
>   bridge->encoder = NULL;
>   return ret;
> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>   if (bridge->funcs->detach)
>   bridge->funcs->detach(bridge);
>  
> + if (bridge->link)
> + device_link_del(bridge->link);
> + bridge->link = NULL;
> +
>   bridge->dev = NULL;
>  }
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index b656e505d11e..804189c63a4c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> + * @link: drm consumer <-> bridge supplier

Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
to the bridge" would be better.

Anyway:
Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>

 --
Regards
Andrzej

>   * @funcs: control functions
>   * @driver_private: pointer to the bridge driver's internal context
>   */
> @@ -271,6 +272,7 @@ struct drm_bridge {
>   struct drm_bridge *next;
>   struct list_head list;
>   const struct drm_bridge_timings *timings;
> + struct device_link *link;
>  
>   const struct drm_bridge_funcs *funcs;
>   void *driver_private;

Re: [PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach

[Andrzej Hajda]

On 04.05.2018 15:52, Peter Rosin wrote:
> The .odev owner device will be handy to have around.
>
> Signed-off-by: Peter Rosin <p...@axentia.se>
Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>

 --
Regards
Andrzej
> ---
>  drivers/gpu/drm/drm_bridge.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index df084db33494..78d186b6831b 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -70,6 +70,9 @@ static LIST_HEAD(bridge_list);
>   */
>  void drm_bridge_add(struct drm_bridge *bridge)
>  {
> + if (WARN_ON(!bridge->odev))
> + return;
> +
>   mutex_lock(_lock);
>   list_add_tail(>list, _list);
>   mutex_unlock(_lock);
> @@ -115,6 +118,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
> drm_bridge *bridge,
>   if (!encoder || !bridge)
>   return -EINVAL;
>  
> + if (WARN_ON(!bridge->odev))
> + return -EINVAL;
> +
>   if (previous && (!previous->dev || previous->encoder != encoder))
>   return -EINVAL;
>  

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Andrzej Hajda]

On 10.05.2018 00:21, Peter Rosin wrote:
> On 2018-05-09 17:53, Peter Rosin wrote:
>> On 2018-05-09 17:08, Andrzej Hajda wrote:
>>> On 04.05.2018 15:51, Peter Rosin wrote:
>>>> Bridge drivers can now (temporarily, in a transition phase) select if
>>>> they want to provide a full owner device or keep just providing an
>>>> of_node.
>>>>
>>>> By providing a full owner device, the bridge drivers no longer need
>>>> to provide an of_node since that node is available via the owner
>>>> device.
>>>>
>>>> When all bridge drivers provide an owner device, that will become
>>>> mandatory and the .of_node member will be removed.
>>>>
>>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>>> ---
>>>>  drivers/gpu/drm/drm_bridge.c | 3 ++-
>>>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
>>> What is the reason to put rockchip here? Shouldn't be in separate patch?
>> Because the rockchip driver peeks into the bridge struct and all the
>> changes in this patch relate to making the new .odev member optional in
>> the transition phase, when the bridge can have either a new-style odev
>> or an old style of_node.
>>
>> I guess this rockchip change could be patch 2, but it has to come first
>> after this patch and it makes no sense on its own. Hence, one patch.
>>
>> This rockchip_lvds interaction is also present in patch 24/26
>> drm/bridge: remove the .of_node member
>>
>> I can split them if you want, but I personally don't see the point.
> I had a second look, and maybe the series should start with a patch like
> this instead, so that the rockchip_lvds.c hunks can be removed from
> patch 1/26 and 24/26. That would perhaps be slightly cleaner?
>
> On the other hand, it's orthogonal and this series can be left as is
> (with the benefit of me not having to do another iteration and you all
> not having another bunch of messages to sift through). The below
> patch could easily be (adjusted and) applied later instead. Or not,
> since the right fix is to do this with the newfangled static image
> format mechanism from Jacopo Mondi, and it might be easier to just do
> it right.
>
> State your preference.

For me the current version is OK, it maybe lacks explanation why do you
need to touch rockchip, from my PoV it did not seem so obvious.
Somebody should fix rockchip to use Jacopo's approach instead of
violating abstractions, but this is another story.

With or without added missing explanation:

Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>

 --
Regards
Andrzej


>
> Cheers,
> Peter
>
> >From dee27b36a36acd271459d1489336b56132097425 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <p...@axentia.se>
> Date: Wed, 9 May 2018 23:58:24 +0200
> Subject: [PATCH] drm/rockchip: lvds: do not dig into the DT of remote bridges
>
> The driver is trying to find the needed "data-mapping" for
> interacting with the following bridge in the graphics chain.
> But, doing so is bad since it is done w/o regard of the
> compatible of the remote bridge, so the value of "data-mapping"
> might not mean what this driver assumes. It is also pointless
> since no bridge has any documented "data-mapping" DT property
> and no dts file show any undocumented use.
>
> Just remove the inappropriate code.
>
> Signed-off-by: Peter Rosin <p...@axentia.se>
> ---
>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
> b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 4bd94b167d2c..fa3f4bf9712f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct 
> device *master,
>   }
>   if (lvds->panel)
>   remote = lvds->panel->dev->of_node;
> - else
> - remote = lvds->bridge->of_node;
>   if (of_property_read_string(dev->of_node, "rockchip,output", ))
>   /* default set it as output rgb */
>   lvds->output = DISPLAY_OUTPUT_RGB;

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Andrzej Hajda]

On 04.05.2018 15:51, Peter Rosin wrote:
> Bridge drivers can now (temporarily, in a transition phase) select if
> they want to provide a full owner device or keep just providing an
> of_node.
>
> By providing a full owner device, the bridge drivers no longer need
> to provide an of_node since that node is available via the owner
> device.
>
> When all bridge drivers provide an owner device, that will become
> mandatory and the .of_node member will be removed.
>
> Signed-off-by: Peter Rosin 
> ---
>  drivers/gpu/drm/drm_bridge.c | 3 ++-
>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-

What is the reason to put rockchip here? Shouldn't be in separate patch?

>  include/drm/drm_bridge.h | 2 ++
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..3872f5379998 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
> *np)
>   mutex_lock(_lock);
>  
>   list_for_each_entry(bridge, _list, list) {
> - if (bridge->of_node == np) {
> + if ((bridge->odev && bridge->odev->of_node == np) ||
> + bridge->of_node == np) {
>   mutex_unlock(_lock);
>   return bridge;
>   }
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
> b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 4bd94b167d2c..557e0079c98d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct 
> device *master,
>   }
>   if (lvds->panel)
>   remote = lvds->panel->dev->of_node;
> - else
> + else if (lvds->bridge->of_node)
>   remote = lvds->bridge->of_node;
> + else
> + remote = lvds->bridge->odev->of_node;

I guess odev should be NULL here, or have I missed something.

Regards
Andrzej

>   if (of_property_read_string(dev->of_node, "rockchip,output", ))
>   /* default set it as output rgb */
>   lvds->output = DISPLAY_OUTPUT_RGB;
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec46979..7c17977c3537 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -254,6 +254,7 @@ struct drm_bridge_timings {
>  
>  /**
>   * struct drm_bridge - central DRM bridge control structure
> + * @odev: device that owns the bridge
>   * @dev: DRM device this bridge belongs to
>   * @encoder: encoder to which this bridge is connected
>   * @next: the next bridge in the encoder chain
> @@ -265,6 +266,7 @@ struct drm_bridge_timings {
>   * @driver_private: pointer to the bridge driver's internal context
>   */
>  struct drm_bridge {
> + struct device *odev;
>   struct drm_device *dev;
>   struct drm_encoder *encoder;
>   struct drm_bridge *next;

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Andrzej Hajda]

On 07.05.2018 15:43, Peter Rosin wrote:
> On 2018-05-07 14:59, Andrzej Hajda wrote:
>> On 04.05.2018 15:52, Peter Rosin wrote:
>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>> down along with the bridge. Thus, there will no longer linger any
>>> dangling pointers from the bridge consumer (the drm_device) to some
>>> non-existent bridge supplier.
>> I understand rationales behind this patch, but it is another step into
>> making drm_dev one big driver with subcomponents, where drm will work
>> only if every subcomponent is working/loaded.
> The step is very small IMHO. Just a device-link, which is very easy to
> remove once whatever other solution is ready.
>
>>   Do we need to go this way?
> If the drivers expect the parts to be there, and there is no other safety
> net in place if they are not, what is the (short-term) alternative?
>
>> In case of many platforms such approach results in display turned on
>> very late on boot for example due to late initialization of some
>> regulator exposed by some i2c device, which is used by hdmi bridge. And
>> this hdmi bridge is just to provide alternative(rarely used) display
>> path, the main display path would work anyway.
> This patch does not contribute to any late init and any such delay is not
> affected by this. At all.
>
>> So the main question to drm maintainers is about evolution of bridges,
>> if drm_bridges should become mandatory components of drm device or they
>> could be added/removed dynamically?
> That is a much bigger question than this patch/series. Conflating the
> two is not fair IMHO. You could run this very same argument for every
> driver that gets added, since any additional driver will just make it
> harder to make everything dynamic. Should we stop development right
> away?
>
> Besides, as long as the drm devices are in fact acting as big static
> drivers (built from smaller parts), 

not true

> this should be considered a bug-fix
> that will prevent dereference of stale pointers.
>
> Or will some other solution appear and magically make all bridges and
> drm drivers capable of dynamic reconfiguration in the next few weeks?
> Yeah, right...

You are not changing single driver, you are changing framework and it
affects all the drivers using it, being more cautious about such patches
seems quite natural.

Anyway, I have realized that since drm_bridge_detach will remove the
link, so with properly written dynamic bridge removal, your patch should
not be a blocker.

Regards
Andrzej

>
> Cheers,
> Peter
>
>> Regards
>> Andrzej
>>
>>
>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c | 18 ++
>>>  include/drm/drm_bridge.h |  2 ++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index 78d186b6831b..0259f0a3ff27 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -26,6 +26,7 @@
>>>  #include 
>>>  
>>>  #include 
>>> +#include 
>>>  #include 
>>>  
>>>  #include "drm_crtc_internal.h"
>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>>> struct drm_bridge *bridge,
>>> if (bridge->dev)
>>> return -EBUSY;
>>>  
>>> +   if (encoder->dev->dev != bridge->odev) {
>>> +   bridge->link = device_link_add(encoder->dev->dev,
>>> +  bridge->odev, 0);
>>> +   if (!bridge->link) {
>>> +   dev_err(bridge->odev, "failed to link bridge to %s\n",
>>> +   dev_name(encoder->dev->dev));
>>> +   return -EINVAL;
>>> +   }
>>> +   }
>>> +
>>> bridge->dev = encoder->dev;
>>> bridge->encoder = encoder;
>>>  
>>> if (bridge->funcs->attach) {
>>> ret = bridge->funcs->attach(bridge);
>>> if (ret < 0) {
>>> +   if (bridge->link)
>>> +   device_link_del(bridge->link);
>>> +   bridge->link = NULL;
>>> bridge->dev = NULL;
>>> bridge->encoder = NULL;
>>> return ret;
>>> @@ -159,6 +173,10 @@ void

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Andrzej Hajda]

On 07.05.2018 15:53, Daniel Vetter wrote:
> On Mon, May 07, 2018 at 02:59:45PM +0200, Andrzej Hajda wrote:
>> On 04.05.2018 15:52, Peter Rosin wrote:
>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>> down along with the bridge. Thus, there will no longer linger any
>>> dangling pointers from the bridge consumer (the drm_device) to some
>>> non-existent bridge supplier.
>> I understand rationales behind this patch, but it is another step into
>> making drm_dev one big driver with subcomponents, where drm will work
>> only if every subcomponent is working/loaded. Do we need to go this way?
>> In case of many platforms such approach results in display turned on
>> very late on boot for example due to late initialization of some
>> regulator exposed by some i2c device, which is used by hdmi bridge. And
>> this hdmi bridge is just to provide alternative(rarely used) display
>> path, the main display path would work anyway.
>>
>> So the main question to drm maintainers is about evolution of bridges,
>> if drm_bridges should become mandatory components of drm device or they
>> could be added/removed dynamically?
> This is already the case. You currently cannot hotplug a drm_bridge,
> everything must be present.

Are you sure? DRM core is changing quite fast, so maybe I have missed
something, but AFAIK core calls bridge code only if full display
pipeline is created and connector is in connected state.
So adding and removing bridges from inactive pipelines should work if
coded properly.

>  I don't think it makes sense to change that
> until we have physically hotpluggable drm_bridges in real hardware.

But kernel core already assumes that device drivers are hot-pluggable
:), even this patch is created to solve issues regarding driver hot
unplugging.

>  I
> definitely don't want to refcount stuff to work around driver load
> bonghits on DT platforms, because refcounting is way too hard to get right
> :-)

I am not sure about bridges, but I have successfully (IMO) experimented
with hot (un)plugging panel driver, see panel-samsung-s6e8aa0.c driver
and exynos_drm_dsi.c, panel driver can be safely
plugged/unplugged/replugged without any refcounting (but with help of
mipi_dsi attach/detach callbacks, which are not available for
non-mipi-dsi drivers).

Regards
Andrzej

>
> Afaik there's out-of-tree patches to solve 99% of the driver load fun on
> DT platforms, but because it's not a 100% solution it's blocked since
> forever.
> -Daniel
>
>> Regards
>> Andrzej
>>
>>
>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c | 18 ++
>>>  include/drm/drm_bridge.h |  2 ++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index 78d186b6831b..0259f0a3ff27 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -26,6 +26,7 @@
>>>  #include 
>>>  
>>>  #include 
>>> +#include 
>>>  #include 
>>>  
>>>  #include "drm_crtc_internal.h"
>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>>> struct drm_bridge *bridge,
>>> if (bridge->dev)
>>> return -EBUSY;
>>>  
>>> +   if (encoder->dev->dev != bridge->odev) {
>>> +   bridge->link = device_link_add(encoder->dev->dev,
>>> +  bridge->odev, 0);
>>> +   if (!bridge->link) {
>>> +   dev_err(bridge->odev, "failed to link bridge to %s\n",
>>> +   dev_name(encoder->dev->dev));
>>> +   return -EINVAL;
>>> +   }
>>> +   }
>>> +
>>> bridge->dev = encoder->dev;
>>> bridge->encoder = encoder;
>>>  
>>> if (bridge->funcs->attach) {
>>> ret = bridge->funcs->attach(bridge);
>>> if (ret < 0) {
>>> +   if (bridge->link)
>>> +   device_link_del(bridge->link);
>>> +   bridge->link = NULL;
>>> bridge->dev = NULL;
>>> bridge->encoder = NULL;
>>> return ret;
>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>> if (bridge->funcs->detach)
>>> bridge->fun

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Andrzej Hajda]

On 04.05.2018 15:52, Peter Rosin wrote:
> If the bridge supplier is unbound, this will bring the bridge consumer
> down along with the bridge. Thus, there will no longer linger any
> dangling pointers from the bridge consumer (the drm_device) to some
> non-existent bridge supplier.

I understand rationales behind this patch, but it is another step into
making drm_dev one big driver with subcomponents, where drm will work
only if every subcomponent is working/loaded. Do we need to go this way?
In case of many platforms such approach results in display turned on
very late on boot for example due to late initialization of some
regulator exposed by some i2c device, which is used by hdmi bridge. And
this hdmi bridge is just to provide alternative(rarely used) display
path, the main display path would work anyway.

So the main question to drm maintainers is about evolution of bridges,
if drm_bridges should become mandatory components of drm device or they
could be added/removed dynamically?

Regards
Andrzej


>
> Signed-off-by: Peter Rosin 
> ---
>  drivers/gpu/drm/drm_bridge.c | 18 ++
>  include/drm/drm_bridge.h |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 78d186b6831b..0259f0a3ff27 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include 
>  
>  #include 
> +#include 
>  #include 
>  
>  #include "drm_crtc_internal.h"
> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
> struct drm_bridge *bridge,
>   if (bridge->dev)
>   return -EBUSY;
>  
> + if (encoder->dev->dev != bridge->odev) {
> + bridge->link = device_link_add(encoder->dev->dev,
> +bridge->odev, 0);
> + if (!bridge->link) {
> + dev_err(bridge->odev, "failed to link bridge to %s\n",
> + dev_name(encoder->dev->dev));
> + return -EINVAL;
> + }
> + }
> +
>   bridge->dev = encoder->dev;
>   bridge->encoder = encoder;
>  
>   if (bridge->funcs->attach) {
>   ret = bridge->funcs->attach(bridge);
>   if (ret < 0) {
> + if (bridge->link)
> + device_link_del(bridge->link);
> + bridge->link = NULL;
>   bridge->dev = NULL;
>   bridge->encoder = NULL;
>   return ret;
> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>   if (bridge->funcs->detach)
>   bridge->funcs->detach(bridge);
>  
> + if (bridge->link)
> + device_link_del(bridge->link);
> + bridge->link = NULL;
> +
>   bridge->dev = NULL;
>  }
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index b656e505d11e..804189c63a4c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> + * @link: drm consumer <-> bridge supplier
>   * @funcs: control functions
>   * @driver_private: pointer to the bridge driver's internal context
>   */
> @@ -271,6 +272,7 @@ struct drm_bridge {
>   struct drm_bridge *next;
>   struct list_head list;
>   const struct drm_bridge_timings *timings;
> + struct device_link *link;
>  
>   const struct drm_bridge_funcs *funcs;
>   void *driver_private;

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Andrzej Hajda]

Hi Peter,

On 27.04.2018 00:31, Peter Rosin wrote:
> Hi!
>
> It was noted by Russel King [1] that bridges (not using components)
> might disappear unexpectedly if the owner of the bridge was unbound.
> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
> came up with using device links to resolve the panel issue, which
> was also my (independent) reaction to the note from Russel.
>
> This series builds up to the addition of that link in the last
> patch, but in my opinion the other 23 patches do have merit on their
> own.
>
> The last patch needs testing, while the others look trivial. That
> said, I might have missed some subtlety.

of_node is used as an identifier of the bridge in the kernel. If you
replace it with device pointer there will be potential problem with
devices having two or more bridges, how do you differentiate bridges if
the owner is the same? If I remember correctly current bridge code does
not allow to have multiple bridges in one device, but that should be
quite easy to fix if necessary. After this change it will become more
difficult.

Anyway I remember discussion that in DT world bridge should be
identified rather by of_graph port node, not by parent node as it is
now. If you want to translate this relation to device owner, you should
add also port number to have full identification of the bridge, ie pair
(owner, port_number) would be equivalent of port node.

Regards
Andrzej


>
> Cheers,
> Peter
>
> [1] https://lkml.org/lkml/2018/4/23/769
> [2] https://www.spinics.net/lists/dri-devel/msg174275.html
>
> Peter Rosin (24):
>   drm/bridge: allow optionally specifying an .owner device
>   drm/bridge: adv7511: provide an .owner device
>   drm/bridge/analogix: core: specify the .owner of the bridge
>   drm/bridge: analogix-anx78xx: provide an .owner device
>   drm/bridge: vga-dac: provide an .owner device
>   drm/bridge: lvds-encoder: provide an .owner device
>   drm/bridge: megachips-stdp-ge-b850v3-fw: provide an .owner device
>   drm/bridge: nxp-ptn3460: provide an .owner device
>   drm/bridge: panel: provide an .owner device
>   drm/bridge: ps8622: provide an .owner device
>   drm/bridge: sii902x: provide an .owner device
>   drm/bridge: sii9234: provide an .owner device
>   drm/bridge: sii8620: provide an .owner device
>   drm/bridge: synopsys: provide an .owner device for the bridges
>   drm/bridge: tc358767: provide an .owner device
>   drm/bridge: ti-tfp410: provide an .owner device
>   drm/exynos: mic: provide an .owner device for the bridge
>   drm/mediatek: hdmi: provide an .owner device for the bridge
>   drm/msm: specify the .owner of the bridges
>   drm/rcar-du: lvds: provide an .owner device for the bridge
>   drm/sti: provide an .owner device for the bridges
>   drm/bridge: remove the .of_node member
>   drm/bridge: require the .owner to be filled in on drm_bridge_attach
>   drm/bridge: establish a link between the bridge supplier and consumer
>
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |  2 +-
>  drivers/gpu/drm/bridge/analogix-anx78xx.c  |  5 +
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
>  drivers/gpu/drm/bridge/dumb-vga-dac.c  |  2 +-
>  drivers/gpu/drm/bridge/lvds-encoder.c  |  2 +-
>  .../drm/bridge/megachips-stdp-ge-b850v3-fw.c   |  2 +-
>  drivers/gpu/drm/bridge/nxp-ptn3460.c   |  2 +-
>  drivers/gpu/drm/bridge/panel.c |  4 +---
>  drivers/gpu/drm/bridge/parade-ps8622.c |  2 +-
>  drivers/gpu/drm/bridge/sii902x.c   |  2 +-
>  drivers/gpu/drm/bridge/sii9234.c   |  2 +-
>  drivers/gpu/drm/bridge/sil-sii8620.c   |  2 +-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  |  4 +---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c  |  4 +---
>  drivers/gpu/drm/bridge/tc358767.c  |  2 +-
>  drivers/gpu/drm/bridge/ti-tfp410.c |  2 +-
>  drivers/gpu/drm/drm_bridge.c   | 23 
> +-
>  drivers/gpu/drm/exynos/exynos_drm_mic.c|  2 +-
>  drivers/gpu/drm/mediatek/mtk_hdmi.c|  2 +-
>  drivers/gpu/drm/msm/dsi/dsi_manager.c  |  1 +
>  drivers/gpu/drm/msm/edp/edp_bridge.c   |  1 +
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  1 +
>  drivers/gpu/drm/rcar-du/rcar_lvds.c|  2 +-
>  drivers/gpu/drm/sti/sti_dvo.c  |  2 +-
>  drivers/gpu/drm/sti/sti_hda.c  |  1 +
>  drivers/gpu/drm/sti/sti_hdmi.c |  1 +
>  include/drm/drm_bridge.h   |  8 
>  27 files changed, 51 insertions(+), 33 deletions(-)
>

Re: [PATCH v9 0/2] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[Andrzej Hajda]

On 18.04.2018 16:40, Jacopo Mondi wrote:
> As I have another series which is based on this one + Eagle board display
> support, I'm re-sending this one to fix the small issue I pointed out in my
> reply to v8.
>
> Simon: no changes to Eagle DTS series, so the last one sent is still the good
> one.
>
> DRM: I have collected several reviewed-by tags both on driver and bindings.
> Can I send out incremental patches on this series and consider this one to
> be ready to be collected?

Queued to drm-misc-next.

Regards
Andrzej


>
> Thanks
>j
>
> v8 -> v9:
> - Put 'remote' OF node after usage not just in error path during device
>   tree parsing routine
> - Add Rob's Reviewed-by tag to the device tree bindings documentation
>
> v7 -> b8:
> - Make 'vcc' supply mandatory
> - Use 'oe' property name to describe "OE" pin
> - Minor fixes as suggested by Laurent on bindings and driver
>
> v6 -> v7:
> - Use semi-standard names for powerdown and output enable GPIOs as suggested
>   by Rob and Vladimir
> - Use 'regulator_get()' not the optional version, and list only 'vcc' as
>   requested supply
> - Addressed Laurent's review comments and removed Eagle display enablement 
> patch
>   to be sent separately
>
> v5 -> v6:
> - Drop check for CONFIG_OF as it is a Kconfig dependency
> - Add Niklas Reviewed-by tags
> - List [3/3] depenencies below commit message to ease integration
>
> v4 -> v5:
> - Fix punctuation in bindings documentation
> - Add small statement to bindings document to clarify the chip has no
>   control bus
> - Print regulator name in enable/disable routines error path
> - Add Andrzej Reviewed-by tag
>
> v3 -> v4:
> - Rename permutations of "pdwn" to just "pdwn" everywhere in the series
> - Improve power enable/disable routines as suggested by Andrzej and Sergei
> - Change "pdwn" gpio initialization to use the logical output level
> - Change Kconfig description
>
> v2 -> v3:
> - Drop support for "lvds-decoder" and make the driver THC63LVD1024 specific
> -- Rework bindings to describe multiple input/output ports
> -- Rename driver and remove "lvds-decoder" references
> -- Rework Eagle DTS to use new bindings
>
> v1 -> v2:
> - Drop support for THC63LVD1024
>
> Jacopo Mondi (2):
>   dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder
>   drm: bridge: Add thc63lvd1024 LVDS decoder driver
>
>  .../bindings/display/bridge/thine,thc63lvd1024.txt |  60 ++
>  drivers/gpu/drm/bridge/Kconfig |   6 +
>  drivers/gpu/drm/bridge/Makefile|   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c  | 205 
> +
>  4 files changed, 272 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>
> --
> 2.7.4
>
>
>
>

Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Andrzej Hajda]

On 27.03.2018 09:36, Geert Uytterhoeven wrote:
> Hi Andrzej,
>
> On Tue, Mar 27, 2018 at 9:28 AM, Andrzej Hajda <a.ha...@samsung.com> wrote:
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>>> +static void thc63_enable(struct drm_bridge *bridge)
>>>> +{
>>>> +struct thc63_dev *thc63 = to_thc63(bridge);
>>>> +struct regulator *vcc;
>>>> +int i;
>>> unsigned int i;
>> Why? You are introducing silly bug this way, see few lines below.
>>
>>>> +
>>>> +for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>>> +vcc = thc63->vccs[i];
>>>> +if (!vcc)
>>>> +continue;
>>>> +
>>>> +if (regulator_enable(vcc))
>>>> +goto error_vcc_enable;
>>>> +}
>>>> +
>>>> +if (thc63->pdwn)
>>>> +gpiod_set_value(thc63->pdwn, 0);
>>>> +
>>>> +if (thc63->oe)
>>>> +gpiod_set_value(thc63->oe, 1);
>>>> +
>>>> +return;
>>>> +
>>>> +error_vcc_enable:
>>>> +dev_err(thc63->dev, "Failed to enable regulator %s\n",
>>>> +thc63_reg_names[i]);
>>>> +
>>>> +for (i = i - 1; i >= 0; i--) {
>> Here, the loop will not end if you define i unsigned.
> True.
>
>> I know one can change the loop, to make it working with unsigned. But
>> this clearly shows that using unsigned is more risky.
>> What are advantages of unsigned vs int in this case. Are there some
>> guidelines/discussions about it?
> Some people consider signed integers harmful, as they may be converted
> silently by the compiler to the "larger" unsigned type when needed.

Wow, it sounds crazy, shall we expect gigantic patchsets, converting all
occurrences of int to "unsigned int" ? :)
I know both types have their pros and cons and can behave unexpectedly
in corner cases, but I do not see why unsigned should be preferred over
signed in general, or in this particular case.
I guess there were somewhere discussion about it, could you point me to
it if possible, to avoid unnecessary noise in this thread.

Regards
Andrzej

>
> Gr{oetje,eeting}s,
>
> Geert
>

Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Andrzej Hajda]

On 27.03.2018 08:24, Vladimir Zapolskiy wrote:
> Hi Jacopo,
>
> On 03/16/2018 05:16 PM, Jacopo Mondi wrote:
>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>> output converter.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>> ---
>>  drivers/gpu/drm/bridge/Kconfig|   6 +
>>  drivers/gpu/drm/bridge/Makefile   |   1 +
>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 
>> ++
>>  3 files changed, 262 insertions(+)
>>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index 3b99d5a..5815a20 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -92,6 +92,12 @@ config DRM_SII9234
>>It is an I2C driver, that detects connection of MHL bridge
>>and starts encapsulation of HDMI signal.
>>  
>> +config DRM_THINE_THC63LVD1024
>> +tristate "Thine THC63LVD1024 LVDS decoder bridge"
>> +depends on OF
>> +---help---
>> +  Thine THC63LVD1024 LVDS/parallel converter driver.
>> +
>>  config DRM_TOSHIBA_TC358767
>>  tristate "Toshiba TC358767 eDP bridge"
>>  depends on OF
>> diff --git a/drivers/gpu/drm/bridge/Makefile 
>> b/drivers/gpu/drm/bridge/Makefile
>> index 373eb28..fd90b16 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
>> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
>> b/drivers/gpu/drm/bridge/thc63lvd1024.c
>> new file mode 100644
>> index 000..02a54c2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>> @@ -0,0 +1,255 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
>> + *
>> + * Copyright (C) 2018 Jacopo Mondi <jacopo+rene...@jmondi.org>
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +enum {
>> +THC63_LVDS_IN0,
>> +THC63_LVDS_IN1,
>> +THC63_DIGITAL_OUT0,
>> +THC63_DIGITAL_OUT1,
>> +};
>> +
>> +static const char * const thc63_reg_names[] = {
>> +"vcc", "lvcc", "pvcc", "cvcc",
>> +};
>> +
>> +struct thc63_dev {
>> +struct device *dev;
>> +
>> +struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
>> +
>> +struct gpio_desc *pdwn;
>> +struct gpio_desc *oe;
>> +
>> +struct drm_bridge bridge;
>> +struct drm_bridge *next;
>> +};
>> +
>> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
>> +{
>> +return container_of(bridge, struct thc63_dev, bridge);
>> +}
>> +
>> +static int thc63_attach(struct drm_bridge *bridge)
>> +{
>> +struct thc63_dev *thc63 = to_thc63(bridge);
>> +
>> +return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
>> +}
>> +
>> +static void thc63_enable(struct drm_bridge *bridge)
>> +{
>> +struct thc63_dev *thc63 = to_thc63(bridge);
>> +struct regulator *vcc;
>> +int i;
> unsigned int i;

Why? You are introducing silly bug this way, see few lines below.

>
>> +
>> +for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>> +vcc = thc63->vccs[i];
>> +if (!vcc)
>> +continue;
>> +
>> +if (regulator_enable(vcc))
>> +goto error_vcc_enable;
>> +}
>> +
>> +if (thc63->pdwn)
>> +gpiod_set_value(thc63->pdwn, 0);
>> +
>> +if (thc63->oe)
>> +gpiod_set_value(thc63->oe, 1);
>> +
>> +return;
>> +
>> +error_vcc_enable:
>> +dev_err(thc63->dev, "Failed to enable regul

Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Andrzej Hajda]

On 27.03.2018 08:15, Vladimir Zapolskiy wrote:
> Hi Jacopo,
>
> On 03/27/2018 01:22 AM, Rob Herring wrote:
>> On Tue, Mar 20, 2018 at 02:43:33PM +0200, Laurent Pinchart wrote:
>>> Hi Jacopo,
>>>
>>> (CC'ing Rob)
>>>
>>> Thank you for the patch.
>>>
>>> On Friday, 16 March 2018 17:16:37 EET Jacopo Mondi wrote:
>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>>
>>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>>>> ---
>>>>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 
>>>> +++
>>>>  1 file changed, 66 insertions(+)
>>>>  create mode 100644
>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>> new file mode 100644
>>>> index 000..8225c6a
>>>> --- /dev/null
>>>> +++
>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>> @@ -0,0 +1,66 @@
>>>> +Thine Electronics THC63LVD1024 LVDS decoder
>>>> +---
>>>> +
>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
>>>> streams
>>>> +to parallel data outputs. The chip supports single/dual input/output 
>>>> modes,
>>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
>>>> outputs.
>>>> +
>>>> +Single or dual operation modes, output data mapping and DDR output modes
>>>> are
>>>> +configured through input signals and the chip does not expose any control
>>>> bus.
>>>> +
>>>> +Required properties:
>>>> +- compatible: Shall be "thine,thc63lvd1024"
>>>> +
>>>> +Optional properties:
>>>> +- vcc-supply: Power supply for TTL output and digital circuitry
>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>>>> +- lvcc-supply: Power supply for LVDS inputs
>>>> +- pvcc-supply: Power supply for PLL circuitry
>>> As explained in a comment to one of the previous versions of this series, 
>>> I'm 
>>> tempted to make vcc-supply mandatory and drop the three other power 
>>> supplies 
>>> for now, as I believe there's very little chance they will be connected to 
>>> separately controllable regulators (all supplies use the same voltage). In 
>>> the 
>>> very unlikely event that this occurs in design we need to support in the 
>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional 
>>> without breaking backward compatibility.
>> I'm okay with that.
>>
>>> Apart from that,
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>>>
>>>> +- pdwn-gpios: Power down GPIO signal. Active low
>> powerdown-gpios is the semi-standard name.
>>
> right, I've also noticed it. If possible please avoid shortenings in
> property names.

It is not shortening, it just follow pin name from decoder's datasheet.

>
>>>> +- oe-gpios: Output enable GPIO signal. Active high
>>>> +
> And this one is also a not ever met property name, please consider to
> rename it to 'enable-gpios', for instance display panels define it.


Again, it follows datasheet naming scheme. Has something changed in DT
conventions?

Regards
Andrzej

>
>>>> +The THC63LVD1024 video port connections are modeled according
>>>> +to OF graph bindings specified by
>>>> Documentation/devicetree/bindings/graph.txt
> [snip]
>
>>>> +
>>>> +  port@2{
>>>> +  reg = <2>;
>>>> +
>>>> +  lvds_dec_out_2: endpoint {
>>>> +  remote-endpoint = <_in>;
>>>> +  };
>>>> +
> Drop a surplus empty line above.
>
>>>> +  };
>>>> +
> Drop a surplus empty line above.
>
>>>> +  };
>>>> +  };
> --
> With best wishes,
> Vladimir
>
>
>

Re: [PATCH v4 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[Andrzej Hajda]

On 15.03.2018 11:56, Jacopo Mondi wrote:
> The R-Car V3M Eagle board includes a transparent THC63LVD1024 LVDS
> decoder, connected to the on-chip LVDS encoder output on one side
> and to HDMI encoder ADV7511w on the other one.
>
> As the decoder does not need any configuration it has been so-far
> omitted from DTS. Now that a driver is available, describe it in DT
> as well.
>
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>

Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>

 --
Regards
Andrzej
> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 33 
> +++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts 
> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index c0fd144..69f43b8 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -42,6 +42,33 @@
>   };
>   };
>   };
> +
> + thc63lvd1024: lvds-decoder {
> + compatible = "thine,thc63lvd1024";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + thc63lvd1024_in_0: endpoint {
> + remote-endpoint = <_out>;
> + };
> + };
> +
> + port@2{
> + reg = <2>;
> +
> + thc63lvd1024_out_2: endpoint {
> + remote-endpoint = <_in>;
> + };
> +
> + };
> +
> + };
> + };
>  };
>  
>   {
> @@ -98,7 +125,7 @@
>   port@0 {
>   reg = <0>;
>   adv7511_in: endpoint {
> - remote-endpoint = <_out>;
> + remote-endpoint = <_out_2>;
>   };
>   };
>  
> @@ -152,8 +179,8 @@
>  
>   ports {
>   port@1 {
> - endpoint {
> - remote-endpoint = <_in>;
> + lvds0_out: endpoint {
> + remote-endpoint = <_in_0>;
>   };
>   };
>   };

Re: [PATCH v4 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Andrzej Hajda]

thc63->vccs[i];
> + if (!vcc)
> + continue;
> +
> + if (regulator_disable(vcc))
> + dev_dbg(thc63->dev,
> + "Failed to disable regulator %u\n", i);

ditto

> + }
> +}
> +
> +struct drm_bridge_funcs thc63_bridge_func = {
> + .attach = thc63_attach,
> + .enable = thc63_enable,
> + .disable = thc63_disable,
> +};
> +
> +static int thc63_parse_dt(struct thc63_dev *thc63)
> +{
> + struct device_node *thc63_out;
> + struct device_node *remote;
> + int ret = 0;
> +
> + thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> +   THC63_DIGITAL_OUT0, -1);
> + if (!thc63_out) {
> + dev_err(thc63->dev, "Missing endpoint in Port@%u\n",
> + THC63_DIGITAL_OUT0);
> + return -ENODEV;
> + }
> +
> + remote = of_graph_get_remote_port_parent(thc63_out);
> + if (!remote) {
> + dev_err(thc63->dev, "Endpoint in Port@%u unconnected\n",
> + THC63_DIGITAL_OUT0);
> + ret = -ENODEV;
> + goto error_put_out_node;
> + }
> +
> + if (!of_device_is_available(remote)) {
> + dev_err(thc63->dev, "Port@%u remote endpoint is disabled\n",
> + THC63_DIGITAL_OUT0);
> + ret = -ENODEV;
> + goto error_put_remote_node;
> + }
> +
> + thc63->next = of_drm_find_bridge(remote);
> + if (!thc63->next)
> + ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> + of_node_put(remote);
> +error_put_out_node:
> + of_node_put(thc63_out);
> +
> + return ret;
> +}
> +
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> + if (IS_ERR(thc63->oe)) {
> + dev_err(thc63->dev, "Unable to get \"oe-gpios\"\n");
> + return PTR_ERR(thc63->oe);
> + }
> +
> + thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "pdwn",
> +       GPIOD_OUT_HIGH);
> + if (IS_ERR(thc63->pdwn)) {
> + dev_err(thc63->dev, "Unable to get \"pdwn-gpios\"\n");
> + return PTR_ERR(thc63->pdwn);
> + }
> +
> + return 0;
> +}
> +
> +static int thc63_regulator_init(struct thc63_dev *thc63)
> +{
> + struct regulator **reg;
> + int i;
> +
> + reg = >vccs[0];
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> + *reg = devm_regulator_get_optional(thc63->dev,
> +thc63_reg_names[i]);
> + if (IS_ERR(*reg)) {
> + if (PTR_ERR(*reg) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + *reg = NULL;
> + }

You could use "if (!...) continue", to avoid nesting.

With or without suggested changes:
Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>

 --
Regards
Andrzej

> + }
> +
> + return 0;
> +}
> +
> +static int thc63_probe(struct platform_device *pdev)
> +{
> + struct thc63_dev *thc63;
> + int ret;
> +
> + thc63 = devm_kzalloc(>dev, sizeof(*thc63), GFP_KERNEL);
> + if (!thc63)
> + return -ENOMEM;
> +
> + thc63->dev = >dev;
> + platform_set_drvdata(pdev, thc63);
> +
> + ret = thc63_regulator_init(thc63);
> + if (ret)
> + return ret;
> +
> + ret = thc63_gpio_init(thc63);
> + if (ret)
> + return ret;
> +
> + ret = thc63_parse_dt(thc63);
> + if (ret)
> + return ret;
> +
> + thc63->bridge.driver_private = thc63;
> + thc63->bridge.of_node = pdev->dev.of_node;
> + thc63->bridge.funcs = _bridge_func;
> +
> + drm_bridge_add(>bridge);
> +
> + return 0;
> +}
> +
> +static int thc63_remove(struct platform_device *pdev)
> +{
> + struct thc63_dev *thc63 = platform_get_drvdata(pdev);
> +
> + drm_bridge_remove(>bridge);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id thc63_match[] = {
> + { .compatible = "thine,thc63lvd1024", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, thc63_match);
> +#endif
> +
> +static struct platform_driver thc63_driver = {
> + .probe  = thc63_probe,
> + .remove = thc63_remove,
> + .driver = {
> + .name   = "thc63lvd1024",
> + .of_match_table = thc63_match,
> + },
> +};
> +module_platform_driver(thc63_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jac...@jmondi.org>");
> +MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v4 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Andrzej Hajda]

On 15.03.2018 11:56, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 63 
> ++
>  1 file changed, 63 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt 
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 000..0936bde
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,63 @@
> +Thine Electronics THC63LVD1024 LVDS decoder
> +---
> +
> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS 
> streams
> +to parallel data outputs. The chip supports single/dual input/output modes,
> +handling up to two two input LVDS stream and up to two digital CMOS/TTL 
> outputs.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024",
> +
> +Optional properties:
> +- vcc-supply: Power supply for TTL output and digital circuitry
> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> +- lvcc-supply: Power supply for LVDS inputs
> +- pvcc-supply: Power supply for PLL circuitry
> +- pdwn-gpios: Power down GPIO signal. Active low.
> +- oe-gpios: Output enable GPIO signal. Active high.

Nitpick: different lines ends differently: period, dot, nothing.
Another thing if there will be next iteration it would be good to
emphasize converter has no control bus.
Anyway:

Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>

 --
Regards
Andrzej

> +
> +The THC63LVD1024 video port connections are modeled according
> +to OF graph bindings specified by Documentation/devicetree/bindings/graph.txt
> +
> +Required video port nodes:
> +- Port@0: First LVDS input port
> +- Port@2: First digital CMOS/TTL parallel output
> +
> +Optional video port nodes:
> +- Port@1: Second LVDS input port
> +- Port@3: Second digital CMOS/TTL parallel output
> +
> +Example:
> +
> +
> + thc63lvd1024: lvds-decoder {
> + compatible = "thine,thc63lvd1024";
> +
> + vcc-supply = <_lvds_vcc>;
> + lvcc-supply = <_lvds_lvcc>;
> +
> + pdwn-gpio = < 15 GPIO_ACTIVE_LOW>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + lvds_dec_in_0: endpoint {
> + remote-endpoint = <_out>;
> + };
> + };
> +
> + port@2{
> + reg = <2>;
> +
> + lvds_dec_out_2: endpoint {
> + remote-endpoint = <_in>;
> + };
> +
> + };
> +
> + };
> + };

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Andrzej Hajda]

On 14.03.2018 11:09, jacopo mondi wrote:
> Hi Andrzej,
> thanks for review
>
> On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
>> On 13.03.2018 15:30, Jacopo Mondi wrote:
>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output decoder.
>> IMO converter suits here better, but it is just suggestion.
>>
>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig|   7 +
>>>  drivers/gpu/drm/bridge/Makefile   |   1 +
>>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 
>>> ++
>>>  3 files changed, 249 insertions(+)
>>>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index 3b99d5a..facf6bd 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -92,6 +92,13 @@ config DRM_SII9234
>>>   It is an I2C driver, that detects connection of MHL bridge
>>>   and starts encapsulation of HDMI signal.
>>>
>>> +config DRM_THINE_THC63LVD1024
>>> +   tristate "Thine THC63LVD1024 LVDS decoder bridge"
>>> +   depends on OF
>>> +   select DRM_PANEL_BRIDGE
>> You don't use PANEL_BRIDGE, it should be possible to drop the select.
>>
> Ack
>
>>> +   ---help---
>>> + Thine THC63LVD1024 LVDS decoder bridge driver.
>> Decoder to what?
>> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
>> converter or bridge.
> "LVDS to digital CMOS/TTL parallel data converter" as the manual
> describes the chip?

Should work, unless we want some consistency in interface names, we have
already: parallel / DPI / RGB. I am little bit confused about relations
between them so I do not want to enforce any.


>
>>> +
>>>  config DRM_TOSHIBA_TC358767
>>> tristate "Toshiba TC358767 eDP bridge"
>>> depends on OF
>>> diff --git a/drivers/gpu/drm/bridge/Makefile 
>>> b/drivers/gpu/drm/bridge/Makefile
>>> index 373eb28..fd90b16 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
>>> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>>>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
>>> b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> new file mode 100644
>>> index 000..4b059c0
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,241 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
>>> + *
>>> + * Copyright (C) 2018 Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +static const char * const thc63_reg_names[] = {
>>> +   "vcc", "lvcc", "pvcc", "cvcc", };
>>> +
>>> +struct thc63_dev {
>>> +   struct device *dev;
>>> +
>>> +   struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
>>> +
>>> +   struct gpio_desc *pwdn;
>>> +   struct gpio_desc *oe;
>>> +
>>> +   struct drm_bridge bridge;
>>> +   struct drm_bridge *next;
>>> +};
>>> +
>>> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
>>> +{
>>> +   return container_of(bridge, struct thc63_dev, bridge);
>>> +}
>>> +
>>> +static int thc63_attach(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +
>>> +   return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
>>> +}
>>> +
>>> +static void thc63_enable(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +   struct regulator *vcc;
>>

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Andrzej Hajda]

On 13.03.2018 15:30, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output decoder.

IMO converter suits here better, but it is just suggestion.

>
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/gpu/drm/bridge/Kconfig|   7 +
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 
> ++
>  3 files changed, 249 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..facf6bd 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -92,6 +92,13 @@ config DRM_SII9234
> It is an I2C driver, that detects connection of MHL bridge
> and starts encapsulation of HDMI signal.
>  
> +config DRM_THINE_THC63LVD1024
> + tristate "Thine THC63LVD1024 LVDS decoder bridge"
> + depends on OF
> + select DRM_PANEL_BRIDGE

You don't use PANEL_BRIDGE, it should be possible to drop the select.

> + ---help---
> +   Thine THC63LVD1024 LVDS decoder bridge driver.

Decoder to what?
Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
converter or bridge.

> +
>  config DRM_TOSHIBA_TC358767
>   tristate "Toshiba TC358767 eDP bridge"
>   depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> new file mode 100644
> index 000..4b059c0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi 
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +static const char * const thc63_reg_names[] = {
> + "vcc", "lvcc", "pvcc", "cvcc", };
> +
> +struct thc63_dev {
> + struct device *dev;
> +
> + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> +
> + struct gpio_desc *pwdn;
> + struct gpio_desc *oe;
> +
> + struct drm_bridge bridge;
> + struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> +
> + return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> + vcc = thc63->vccs[i];
> + if (vcc) {
> + ret = regulator_enable(vcc);
> + if (ret)
> + goto error_vcc_enable;
> + }
> + }
> +
> + if (thc63->pwdn)
> + gpiod_set_value(thc63->pwdn, 0);
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 1);
> +
> + return;
> +
> +error_vcc_enable:
> + dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> + vcc = thc63->vccs[i];
> + if (vcc) {
> + ret = regulator_disable(vcc);
> + if (ret)
> + goto error_vcc_disable;

I think in disable path you can report an error and continue - should be
safer.

> + }
> + }
> +
> + if (thc63->pwdn)
> + gpiod_set_value(thc63->pwdn, 1);
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 0);

Usually disable uses reverse order. Ie first disable oe, then, pwdn,
then regulators, also in reverse order.
Looks more reasonable.

> +
> + return;
> +
> +error_vcc_disable:
> + dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
> +}
> +
> +struct drm_bridge_funcs thc63_bridge_func = {
> + .attach = 

Re: [PATCH v3 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Andrzej Hajda]

On 13.03.2018 15:30, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>
> Signed-off-by: Jacopo Mondi 
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 63 
> ++
>  1 file changed, 63 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt 
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 000..5b5afcd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,63 @@
> +Thine Electronics THC63LVD1024 LVDS decoder
> +---
> +
> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS 
> streams
> +to parallel data outputs. The chip supports single/dual input/output modes,
> +handling up to two two input LVDS stream and up to two digital CMOS/TTL 
> outputs.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024",
> +
> +Optional properties:
> +- vcc-supply: Power supply for TTL output and digital circuitry
> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> +- lvcc-supply: Power supply for LVDS inputs
> +- pvcc-supply: Power supply for PLL circuitry

I wonder if it wouldn't be better to make them required (at least VCC) -
it is closer to reality.

> +- pwnd-gpios: Power down GPIO signal. Active low.

As I said before, specs[1] says about "/PDWN" pin. Is it your typo, or
different docs?
Moreover there are already bindings for THC63LVDM83D with the same
dichotomy [2].

Out of curiosity I have googled for "pwnd pin" and it looks like some
vendors use this form.
For me both forms are quite misleading: power down signal, active low,
why they couldn't call it just 'enable, active high'.

[1]: http://www.thine.co.jp/files/topics/179_ext_12_0.pdf
[2]:
https://elixir.bootlin.com/linux/v4.16-rc5/source/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt

> +- oe-gpios: Output enable GPIO signal. Active high.
> +
> +The THC63LVD1024 video port connections are modeled according
> +to OF graph bindings specified by Documentation/devicetree/bindings/graph.txt
> +
> +Required video port nodes:
> +- Port@0: First LVDS input port
> +- Port@2: First digital CMOS/TTL parallel output
> +
> +Optional video port nodes:
> +- Port@1: Second LVDS input port
> +- Port@3: Second digital CMOS/TTL parallel output
> +
> +Example:
> +
> +
> + thc63lvd1024: lvds-decoder {
> + compatible = "thine,thc63lvd1024";
> +
> + vcc-supply = <_lvds_vcc>;
> + lvcc-supply = <_lvds_lvcc>;
> +
> + pwdn-gpio = < 15 GPIO_ACTIVE_LOW>;
And here another variation :), should be pdwn-gpios.

> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + lvds_dec_in_0: endpoint {
> + remote-endpoint = <_out>;
> + };
> + };
> +
> + port@2{
> + reg = <2>;
> +
> + lvds_dec_out_2: endpoint {
> + remote-endpoint = <_in>;
> + };
> +
> + };
> +
> + };
> + };

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Andrzej Hajda]

On 12.03.2018 13:30, jacopo mondi wrote:
> Hi Andrzej,
>
> On Mon, Mar 12, 2018 at 10:07:27AM +0100, Andrzej Hajda wrote:
>> On 09.03.2018 14:51, Jacopo Mondi wrote:
>>> Hello,
>>>after some discussion on the proposed bindings for generic lvds decoder 
>>> and
>>> Thine THC63LVD1024, I decided to drop the THC63 specific part and just live 
>>> with
>>> a transparent decoder that does not support any configuration from DT.
>>>
>>> Dropping THC63 support to avoid discussion on how to better implement 
>>> support
>>> for a DRM bridge with 2 input ports and focus on LVDS mode propagation 
>>> through
>>> bridges as explained in v1 cover letter (for DRM people: please see [1] as 
>>> why
>>> I find difficult to implement support for bridges with multiple input 
>>> endpoints)
>>>
>>> Same base branch as v1, with same patches for V3M Eagle applied on top.
>>> git://jmondi.org/linux v3m/v4.16-rc3/base
>>>
>>> Thanks
>>>j
>>>
>>> v1 -> v2:
>>> - Drop support for THC63LVD1024
>>>
>>> [1] I had a quick at how to model a DRM bridge with multiple input
>>> ports, and I see a blocker in how DRM identifies and matches bridges using
>>> the devices node in place of the endpoint nodes.
>>>
>>> As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
>>> a few ways to support that:
>>>  1) register 2 drm bridges from the same driver (one for each input/output 
>>> pair)
>>> but they would both be matches on the same device node when the 
>>> preceding
>>> bridge calls "of_drm_find_bridge()".
>>>  2) register a single bridge with multiple "next bridges", but when the 
>>> bridge
>>> gets attached I don't see a way on how to identify on which next bridge
>>> "drm_bridge_attach()" on, as it depends on the endpoint the current 
>>> bridge
>>> has been attached on first, and we don't have that information.
>>>  3) Register more instances of the same chip in DTS, one for each 
>>> input/output
>>> pair. They gonna share supplies and gpios, and I don't like that.
>>>
>>> I had a quick look at the currently in mainline bridges and none of them has
>>> multiple input endpoints, except for HDMI audio endpoint, which I haven't 
>>> found
>>> in use in any DTS. I guess the problem has been already debated and maybe 
>>> solved
>>> in the past, so feel free to point me to other sources.
>> I think this is is a step in wrong direction, IMHO. Your previous
>> patchset was quite OK, at least bindings, IMHO. Few things needed only
>> polishing.
>> Here we have unmanaged/transparent bridge, which is totally different,
>> what happened to regulators and gpios from previous iteration.
>> I do not have schematics of the board, but I guess respective pins of
>> the bridge must be connected somehow.
>> I think the problem you want to avoid (double bridge) should not be a
>> problem at all.
>> I suppose the most important is to have correct bindings - as they need
>> to be stable.
>> If you really must to do hacks better is to put them into driver.
>>
> I understand. The "transparent bridge" is of no actual use, but I don't see
> how the "double bridge" thing is not an issue if I were to develop
> support for the actual Thine chip.

What is exactly your configuration: single/dual input, single/dual output?
Current bindings suggests single/single, am I right? In such case you do
not need to implement dual link functionality in the driver - since you
even do not have possibility to test it.
But the bindings should support all modes of operation, or at least
should be easy to extend them in the future with backward compatibility.

>
> Please see my reply from yesterday to Archit. I still think having two
> bridges is somehow an issue...

Yes, I agree. But do we have such case? If no - no problem ATM :), if
yes - lets try to implement it and see where is the problem, as Archit
already suggested it would be just a matter of assigning bridge to port
node, instead of device node.

>
> While we clarify that, would it be fine an initial driver version for
> the actualt Thine chip with a single input support[1]? I would ditch this
> transparent bridge and resume working on a THC63LVD1024 driver from
> comments received on v1.

I think, yes: driver with only single input, and full or extend-able
bindings.

>
> Thanks
>   j
>
> [1] Single input support

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Andrzej Hajda]

On 09.03.2018 14:51, Jacopo Mondi wrote:
> Hello,
>after some discussion on the proposed bindings for generic lvds decoder and
> Thine THC63LVD1024, I decided to drop the THC63 specific part and just live 
> with
> a transparent decoder that does not support any configuration from DT.
>
> Dropping THC63 support to avoid discussion on how to better implement support
> for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
> bridges as explained in v1 cover letter (for DRM people: please see [1] as why
> I find difficult to implement support for bridges with multiple input 
> endpoints)
>
> Same base branch as v1, with same patches for V3M Eagle applied on top.
> git://jmondi.org/linux v3m/v4.16-rc3/base
>
> Thanks
>j
>
> v1 -> v2:
> - Drop support for THC63LVD1024
>
> [1] I had a quick at how to model a DRM bridge with multiple input
> ports, and I see a blocker in how DRM identifies and matches bridges using
> the devices node in place of the endpoint nodes.
>
> As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
> a few ways to support that:
>  1) register 2 drm bridges from the same driver (one for each input/output 
> pair)
> but they would both be matches on the same device node when the preceding
> bridge calls "of_drm_find_bridge()".
>  2) register a single bridge with multiple "next bridges", but when the bridge
> gets attached I don't see a way on how to identify on which next bridge
> "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
> has been attached on first, and we don't have that information.
>  3) Register more instances of the same chip in DTS, one for each input/output
> pair. They gonna share supplies and gpios, and I don't like that.
>
> I had a quick look at the currently in mainline bridges and none of them has
> multiple input endpoints, except for HDMI audio endpoint, which I haven't 
> found
> in use in any DTS. I guess the problem has been already debated and maybe 
> solved
> in the past, so feel free to point me to other sources.

I think this is is a step in wrong direction, IMHO. Your previous
patchset was quite OK, at least bindings, IMHO. Few things needed only
polishing.
Here we have unmanaged/transparent bridge, which is totally different,
what happened to regulators and gpios from previous iteration.
I do not have schematics of the board, but I guess respective pins of
the bridge must be connected somehow.
I think the problem you want to avoid (double bridge) should not be a
problem at all.
I suppose the most important is to have correct bindings - as they need
to be stable.
If you really must to do hacks better is to put them into driver.

Regards
Andrzej

>
> Jacopo Mondi (3):
>   dt-bindings: display: bridge: Document LVDS to parallel decoder
>   drm: bridge: Add LVDS decoder driver
>   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
>
>  .../bindings/display/bridge/lvds-decoder.txt   |  42 ++
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts |  31 +++-
>  drivers/gpu/drm/bridge/Kconfig |   8 ++
>  drivers/gpu/drm/bridge/Makefile|   1 +
>  drivers/gpu/drm/bridge/lvds-decoder.c  | 157 
> +
>  5 files changed, 237 insertions(+), 2 deletions(-)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
>
> --
> 2.7.4
>
>
>
>

Re: [PATCH 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Andrzej Hajda]

On 08.03.2018 16:24, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder.
>
> Signed-off-by: Jacopo Mondi 
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 59 
> ++
>  1 file changed, 59 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt 
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 000..53b6453
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,59 @@
> +THine Electronics THC63LVD1024 LVDS receiver
> +
> +
> +The THC63LVD1024 is an LVDS receiver designed to convert multiple LVDS 
> streams
> +to digital CMOS/TTL parallel data.

You say multiple streams, but bindings describe only one stream.
> +
> +Required properties:
> +- compatible: Shall be one of the following:
> +  "thine,thc63lvd1024",
> +  "lvds-decoder"
> +
> +Optional properties:
> +- supply-vcc: Power supply for TTL output and digital circuitry
> +- supply-cvcc: Power supply for TTL CLOCKOUT signal
> +- supply-lvcc: Power supply for LVDS inputs
> +- supply-pvcc: Power supply for PLL circuitry
> +- pwnd-gpio: Power down GPIO signal. Active low.

Specs [1] uses "/PDWN" name for the pin, moreover gpios suffix is preferred.

Another issue I see is two possibly contradicting conventions:
1. Properties should be named according to specs - so here it should be
named pdwn-gpios.
2. The bindings tries to be generic for lvds decoders, in such case
probably preferred name should be more generic, maybe power-gpios.

Personally I would prefer 1, in such case generic lvds-decoder driver
should look for gpio names according to compatible string.

[1]: http://www.thine.co.jp/files/topics/179_ext_12_0.pdf

> +- oe-gpio: Output enable GPIO signal. Active high.

oe-gpios

> +
> +The THC63LVD1024 has two video ports, whose connections are modeled according
> +to OF graph bindings specified by Documentation/devicetree/bindings/graph.txt
> +
> +- Port@0: LVDS input port
> +- Port@1: Digital CMOS/TTL parallel output

According to specs it has two lvds input and two parallel output ports,
maybe it would be good to describe all here.

Regards
Andrzej

> +
> +Example:
> +---
> +
> + lvds_decoder: decoder-0 {
> + compatible = "thine,thc63lvd1024";
> +
> + vcc-supply = <_lvds_vcc>;
> + lvcc-supply = <_lvds_lvcc>;
> +
> + pwdn-gpio = < 15 GPIO_ACTIVE_LOW>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + lvds_dec_in: endpoint {
> + remote-endpoint = <_out>;
> + };
> + };
> +
> + port@1{
> + reg = <1>;
> +
> + lvds_dec_out: endpoint {
> + remote-endpoint = <_in>;
> + };
> +
> + };
> +
> + };
> + };

Re: [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar

[Andrzej Hajda]

On 04.12.2017 13:36, Wolfram Sang wrote:
> When implementing bus recovery for the i2c-rcar driver, two problems were
> encountered: 1) When reading the SDA bit, not the SDA status was returned but
> the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to
> consider the bus free again. SCL/SDA high is not enough, and there is no other
> way known to reset the internal logic otherwise.
>
> The obvious solution to just send STOP after recovery makes sense for the
> generic case, too, IMO. If we made a device release SDA again, and are about
> start a new transfer using START, then we should terminate the previous state
> properly with STOP. This may help with some devices and shouldn't create any
> drawback AFAICS.
>
> For this, we need to introduce a 'set_sda' callback to the recovery
> infrastructure. Because of this and some other minor recovery core changes, I
> CCed a few people working on I2C bus recovery recently. The first five patches
> may be interesting for you, so your input is greatly appreciated. Also, 
> testing
> the new features with GPIO based recovery would be awesome to have.
>
> This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure
> is documented here:
>
> https://elinux.org/Tests:I2C-bus-recovery
>
> A branch is available here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
> renesas/topic/rcar-i2c-recovery
>
> Please let me know what you think.

I do not feel myself experienced I2C developer, so please be merciful if
I write stupid things :)

>From what I see the whole recovery infrastructure partially duplicates
i2c_bit_algo/i2c_algo_bit_data algorithm, and GPIO recovery duplicates
i2c-gpio driver.
Wouldn't be better to somehow reuse existing code? For example by adding
recovery callback in i2c_algorithm, and call i2c-gpio::recovery or
i2c_bit_algo::recovery from rcar-i2c-recovery (in this particular case).

I am not sure how much work it requires, and if it is worth it. Please
consider it as suggestion.

Regards
Andrzej

Re: [PATCH] v4l2-drm-example: Make the exporter selectable

[Andrzej Hajda]

Hi Laurent,

On 15.11.2017 10:00, Laurent Pinchart wrote:
> The new -e command line option allows selecting the exporter between the
> V4L2 and DRM side. DRM is used as the exporter by default.
>
> Signed-off-by: Laurent Pinchart 
> ---
>  v4l2-drm-example/dmabuf-sharing.c | 99 
> +++
>  1 file changed, 89 insertions(+), 10 deletions(-)
>
> This patch is against the master branch of
> git://git.infradead.org/users/kmpark/public-apps and is available in my clone
> tree at git://git.ideasonboard.org/samsung-public-apps.git.
>
> Andrzej, if the patch is acceptable, could you merge it upstream ?

Sylwester merged it to devel branch of
https://git.linuxtv.org/snawrocki/samsung-utils.git

The branch git://git.infradead.org/users/kmpark/public-apps is obsolete
and beyond our control.


Regards
Andrzej

>
> diff --git a/v4l2-drm-example/dmabuf-sharing.c 
> b/v4l2-drm-example/dmabuf-sharing.c
> index 5e1fb6a8f0c3..e2f1a4228af8 100644
> --- a/v4l2-drm-example/dmabuf-sharing.c
> +++ b/v4l2-drm-example/dmabuf-sharing.c
> @@ -69,6 +69,11 @@ static inline int warn(const char *file, int line, const 
> char *fmt, ...)
>  #define WARN_ON(cond, ...) \
>   ((cond) ? warn(__FILE__, __LINE__, __VA_ARGS__) : 0)
>  
> +enum dmabuf_exporter {
> + DMABUF_EXPORTER_DRM = 0,
> + DMABUF_EXPORTER_V4L2,
> +};
> +
>  struct setup {
>   char module[32];
>   int conId;
> @@ -85,6 +90,7 @@ struct setup {
>   unsigned int use_compose : 1;
>   struct v4l2_rect crop;
>   struct v4l2_rect compose;
> + enum dmabuf_exporter exporter;
>  };
>  
>  struct drm_device {
> @@ -105,10 +111,12 @@ struct drm_device {
>   unsigned int height;
>  
>   struct v4l2_rect compose;
> + int export;
>  };
>  
>  struct v4l2_device {
>   const char *devname;
> + enum v4l2_memory memory;
>   int fd;
>  
>   struct v4l2_pix_format format;
> @@ -149,6 +157,7 @@ static void usage(char *name)
>  
>   fprintf(stderr, "\nGeneric options:\n\n");
>   fprintf(stderr, "\t-b buffer_count\tset number of buffers\n");
> + fprintf(stderr, "\t-e \tset the exporter ('v4l2' or 
> 'drm')\n");
>   fprintf(stderr, "\t-h\tshow this help\n");
>  }
>  
> @@ -170,13 +179,21 @@ static int parse_args(int argc, char *argv[], struct 
> setup *s)
>  
>   strcpy(s->video, "/dev/video0");
>  
> - while ((c = getopt(argc, argv, "b:F:f:hi:M:o:p:S:s:t:")) != -1) {
> + while ((c = getopt(argc, argv, "b:e:F:f:hi:M:o:p:S:s:t:")) != -1) {
>   switch (c) {
>   case 'b':
>   ret = sscanf(optarg, "%u", >buffer_count);
>   if (WARN_ON(ret != 1, "incorrect buffer count\n"))
>   return -1;
>   break;
> + case 'e':
> + if (strcmp(optarg, "v4l2") == 0)
> + s->exporter = DMABUF_EXPORTER_V4L2;
> + else if (strcmp(optarg, "drm") == 0)
> + s->exporter = DMABUF_EXPORTER_DRM;
> + else if (WARN_ON(1, ""))
> + return -1;
> + break;
>   case 'F':
>   if (WARN_ON(strlen(optarg) != 4, "invalid fourcc\n"))
>   return -1;
> @@ -284,13 +301,49 @@ fail_prime:
>  
>  fail_gem:
>   memset(_destroy, 0, sizeof gem_destroy);
> - gem_destroy.handle = b->bo_handle,
> + gem_destroy.handle = b->bo_handle;
>   ret = ioctl(dev->fd, DRM_IOCTL_MODE_DESTROY_DUMB, _destroy);
>   WARN_ON(ret, "DESTROY_DUMB failed: %s\n", ERRSTR);
>  
>   return -1;
>  }
>  
> +static int drm_buffer_import(struct drm_device *dev, struct buffer *b,
> +  const struct v4l2_pix_format *fmt)
> +{
> + struct drm_prime_handle prime;
> + struct drm_gem_close gem_close;
> + int ret;
> +
> + memset(, 0, sizeof prime);
> + prime.fd = b->dbuf_fd;
> + ret = ioctl(dev->fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, );
> + if (WARN_ON(ret, "PRIME_FD_TO_HANDLE failed: %s\n", ERRSTR))
> + return -1;
> + b->bo_handle = prime.handle;
> +
> + uint32_t offsets[4] = { 0 };
> + uint32_t pitches[4] = { fmt->bytesperline };
> + uint32_t bo_handles[4] = { b->bo_handle };
> + unsigned int fourcc = dev->format;
> + if (!fourcc)
> + fourcc = fmt->pixelformat;
> + ret = drmModeAddFB2(dev->fd, fmt->width, fmt->height, fourcc, 
> bo_handles,
> + pitches, offsets, >fb_handle, 0);
> + if (WARN_ON(ret, "drmModeAddFB2 failed: %s\n", ERRSTR))
> + goto fail_gem;
> +
> + return 0;
> +
> +fail_gem:
> + memset(_close, 0, sizeof gem_close);
> + gem_close.handle = b->bo_handle;
> + ret = ioctl(dev->fd, DRM_IOCTL_GEM_CLOSE, _close);
> + WARN_ON(ret, "GEM_CLOSE failed: %s\n", ERRSTR);
> +
> + return -1;
> +}
> +
>  static int