Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-20 12:24, Russell King - ARM Linux wrote:
> On Fri, Apr 20, 2018 at 01:06:49PM +0300, Laurent Pinchart wrote:
>> Hi Peter,
>>
>> Thank you for the patch.
>>
>> On Thursday, 19 April 2018 19:27:51 EEST Peter Rosin wrote:
>>> This makes this driver work with all(?) drivers that are not
>>> componentized and instead expect to connect to a panel/bridge. That
>>> said, the only one tested is atmel_hlcdc.
>>>
>>> This hooks the relevant work function previously called by the encoder
>>> and the component also to the bridge, since the encoder goes away when
>>> connecting to the bridge interface of the driver and the equivalent of
>>> bind/unbind of the component is handled by bridge attach/detach.
>>>
>>> The lifetime requirements of a bridge and a component are slightly
>>> different, which is the reason for struct tda998x_bridge.
>>
>> Couldn't you move the allocation and initialization (tda998x_create) of the 
>> tda998x_priv structure to probe time ? I think you wouldn't need a separate 
>> structure in that case. Unless I'm mistaken there would be an added benefit 
>> of 
>> separating component and bridge initialization, resulting in the encoder not 
>> being initialized at all if the component isn't used. You wouldn't need to 
>> add 
>> a local_encoder parameter to the tda998x_init() function.
> 
> No, I don't like that idea one bit, as I've stated in the past about the
> component API.  The same (probably) goes for the bridge stuff too.
> 
> Consider the following:
> 
> Your DRM system is initialised.  You then remove a module, which results
> in the DRM system being torn down.  You re-insert the module (eg, having
> made a change to it).  The DRM system is then re-initialised.
> 
> At this point, what is the state of variables such as priv->is_on if
> you allocate the structure at probe time?
> 
> What about all the other variables in the driver private structure - are
> you sure that the driver can cope with random values from the previous
> "usage" remaining there?
> 
> At the moment, this isn't a concern for the driver because we
> dev_kzalloc() the structure in the bind callback.  Move that to the
> probe function, and the structure is no longer re-initialised each
> time, and so it retains the previous state.  The driver is not setup
> to cope with that.
> 
> So, to work around that, you would need to reinitialise _everything_
> in the structure that the driver requires, which IMHO is a very
> open to bugs (eg, if a member is missed, or added without the
> necessary re-initialisation), _especially_ when this is not a path
> that will get regular testing.
> 
> If you want to do this for a subset of data, it would be much better
> to separate them into independent structures (maybe one embedded into
> the other) so that this problem can not occur.  That way, a subset
> of the data can be memset() when bound to the rest of the DRM system
> ensuring a consistent driver state and still achieve what you're
> suggesting.

This was the exact reason I added struct tda998x_bridge. It seemed
very risky to move the tda998x_create call (or some of its meat) to
the probe function. Even if that could be done I think it should
definitely be a separate patch.

Cheers,
Peter

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-20 12:24, Russell King - ARM Linux wrote:
> On Fri, Apr 20, 2018 at 01:06:49PM +0300, Laurent Pinchart wrote:
>> Hi Peter,
>>
>> Thank you for the patch.
>>
>> On Thursday, 19 April 2018 19:27:51 EEST Peter Rosin wrote:
>>> This makes this driver work with all(?) drivers that are not
>>> componentized and instead expect to connect to a panel/bridge. That
>>> said, the only one tested is atmel_hlcdc.
>>>
>>> This hooks the relevant work function previously called by the encoder
>>> and the component also to the bridge, since the encoder goes away when
>>> connecting to the bridge interface of the driver and the equivalent of
>>> bind/unbind of the component is handled by bridge attach/detach.
>>>
>>> The lifetime requirements of a bridge and a component are slightly
>>> different, which is the reason for struct tda998x_bridge.
>>
>> Couldn't you move the allocation and initialization (tda998x_create) of the 
>> tda998x_priv structure to probe time ? I think you wouldn't need a separate 
>> structure in that case. Unless I'm mistaken there would be an added benefit 
>> of 
>> separating component and bridge initialization, resulting in the encoder not 
>> being initialized at all if the component isn't used. You wouldn't need to 
>> add 
>> a local_encoder parameter to the tda998x_init() function.
> 
> No, I don't like that idea one bit, as I've stated in the past about the
> component API.  The same (probably) goes for the bridge stuff too.
> 
> Consider the following:
> 
> Your DRM system is initialised.  You then remove a module, which results
> in the DRM system being torn down.  You re-insert the module (eg, having
> made a change to it).  The DRM system is then re-initialised.
> 
> At this point, what is the state of variables such as priv->is_on if
> you allocate the structure at probe time?
> 
> What about all the other variables in the driver private structure - are
> you sure that the driver can cope with random values from the previous
> "usage" remaining there?
> 
> At the moment, this isn't a concern for the driver because we
> dev_kzalloc() the structure in the bind callback.  Move that to the
> probe function, and the structure is no longer re-initialised each
> time, and so it retains the previous state.  The driver is not setup
> to cope with that.
> 
> So, to work around that, you would need to reinitialise _everything_
> in the structure that the driver requires, which IMHO is a very
> open to bugs (eg, if a member is missed, or added without the
> necessary re-initialisation), _especially_ when this is not a path
> that will get regular testing.
> 
> If you want to do this for a subset of data, it would be much better
> to separate them into independent structures (maybe one embedded into
> the other) so that this problem can not occur.  That way, a subset
> of the data can be memset() when bound to the rest of the DRM system
> ensuring a consistent driver state and still achieve what you're
> suggesting.

This was the exact reason I added struct tda998x_bridge. It seemed
very risky to move the tda998x_create call (or some of its meat) to
the probe function. Even if that could be done I think it should
definitely be a separate patch.

Cheers,
Peter

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-20 12:53, Russell King - ARM Linux wrote:
> On Fri, Apr 20, 2018 at 12:49:42PM +0200, Peter Rosin wrote:
>> On 2018-04-20 12:41, kbuild test robot wrote:
>>> Hi Peter,
>>>
>>> I love your patch! Yet something to improve:
>>
>> Yup, right you are!
>>
>>> [auto build test ERROR on drm/drm-next]
>>> [also build test ERROR on v4.17-rc1 next-20180420]
>>> [if your patch is applied to the wrong git tree, please drop us a note to 
>>> help improve the system]
>>>
>>> url:
>>> https://github.com/0day-ci/linux/commits/Peter-Rosin/Add-tda998x-HDMI-support-to-atmel-hlcdc/20180420-160131
>>> base:   git://people.freedesktop.org/~airlied/linux.git drm-next
>>> config: i386-randconfig-a0-201815 (attached as .config)
>>> compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
>>> reproduce:
>>> # save the attached .config to linux build tree
>>> make ARCH=i386 
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_probe':
> drivers/gpu/drm/i2c/tda998x_drv.c:1859:16: error: 'struct drm_bridge' has 
> no member named 'of_node'
>>>  bridge->bridge.of_node = dev->of_node;
>>>^
>>
>> Anybody got a better fix than this?
>>
>> #ifdef CONFIG_OF
>>  bridge->bridge.of_node = dev->of_node;
>> #endif
> 
> How about the bridge code provides a helper to do this, something like:
> 
> static inline void bridge_set_device(struct drm_bridge *bridge,
>struct device *dev)
> {
> #ifdef CONFIG_OF
>   bridge->of_node = dev->of_node;
> #endif
> }
> 
> which (a) nicely hides the firmware flavour, and (b) hides the ifdef in
> the bridge header where it belongs.  If the bridge code needs to be
> converted to fwnode in the future, at least this would be abstracted
> from the drivers.
> 

Hmm, I looked around and found numerous other #ifdefs for this, so my plan
is to just add one more. Fixing up all these ifdefs is orthogonal and can
be done later.

Cheers,
Peter

PS. I also noted that I had forgotten to remove a couple of dev_info calls
in this patch. Will remove for v4.

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-20 12:53, Russell King - ARM Linux wrote:
> On Fri, Apr 20, 2018 at 12:49:42PM +0200, Peter Rosin wrote:
>> On 2018-04-20 12:41, kbuild test robot wrote:
>>> Hi Peter,
>>>
>>> I love your patch! Yet something to improve:
>>
>> Yup, right you are!
>>
>>> [auto build test ERROR on drm/drm-next]
>>> [also build test ERROR on v4.17-rc1 next-20180420]
>>> [if your patch is applied to the wrong git tree, please drop us a note to 
>>> help improve the system]
>>>
>>> url:
>>> https://github.com/0day-ci/linux/commits/Peter-Rosin/Add-tda998x-HDMI-support-to-atmel-hlcdc/20180420-160131
>>> base:   git://people.freedesktop.org/~airlied/linux.git drm-next
>>> config: i386-randconfig-a0-201815 (attached as .config)
>>> compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
>>> reproduce:
>>> # save the attached .config to linux build tree
>>> make ARCH=i386 
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_probe':
> drivers/gpu/drm/i2c/tda998x_drv.c:1859:16: error: 'struct drm_bridge' has 
> no member named 'of_node'
>>>  bridge->bridge.of_node = dev->of_node;
>>>^
>>
>> Anybody got a better fix than this?
>>
>> #ifdef CONFIG_OF
>>  bridge->bridge.of_node = dev->of_node;
>> #endif
> 
> How about the bridge code provides a helper to do this, something like:
> 
> static inline void bridge_set_device(struct drm_bridge *bridge,
>struct device *dev)
> {
> #ifdef CONFIG_OF
>   bridge->of_node = dev->of_node;
> #endif
> }
> 
> which (a) nicely hides the firmware flavour, and (b) hides the ifdef in
> the bridge header where it belongs.  If the bridge code needs to be
> converted to fwnode in the future, at least this would be abstracted
> from the drivers.
> 

Hmm, I looked around and found numerous other #ifdefs for this, so my plan
is to just add one more. Fixing up all these ifdefs is orthogonal and can
be done later.

Cheers,
Peter

PS. I also noted that I had forgotten to remove a couple of dev_info calls
in this patch. Will remove for v4.

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Thu, Apr 19, 2018 at 06:27:51PM +0200, Peter Rosin wrote:
> This makes this driver work with all(?) drivers that are not
> componentized and instead expect to connect to a panel/bridge. That
> said, the only one tested is atmel_hlcdc.
> 
> This hooks the relevant work function previously called by the encoder
> and the component also to the bridge, since the encoder goes away when
> connecting to the bridge interface of the driver and the equivalent of
> bind/unbind of the component is handled by bridge attach/detach.
> 
> The lifetime requirements of a bridge and a component are slightly
> different, which is the reason for struct tda998x_bridge.

As we are talking about bridge stuff, the patch below is what I've had
for a while converting Armada to be able to use a bridge-based tda998x.
As you can see, it's far from satisfactory at the moment.  Specifically:

1) it assumes all bridges are TMDS bridges, because as far as I can see,
   there's no way to know any different.
2) there's no way to really know whether a missing bridge is because we
   should be using the component helpers or that the bridge device
   doesn't exist yet.

diff --git a/drivers/gpu/drm/armada/armada_drv.c 
b/drivers/gpu/drm/armada/armada_drv.c
index 262409cae8bf..854d74466dec 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -20,6 +20,127 @@
 #include 
 #include "armada_ioctlP.h"
 
+static const struct drm_encoder_helper_funcs dummy_encoder_helper_funcs = {
+};
+
+static const struct drm_encoder_funcs dummy_encoder_funcs = {
+   .destroy = drm_encoder_cleanup,
+};
+
+static int dummy_encoder_add(struct device *dev, struct drm_device *drm,
+struct drm_bridge *bridge, int type, u32 crtcs)
+{
+   struct drm_encoder *enc;
+   int ret;
+
+   enc = devm_kzalloc(dev, sizeof(*enc), GFP_KERNEL);
+   if (!enc)
+   return -ENOMEM;
+
+   drm_encoder_helper_add(enc, _encoder_helper_funcs);
+   ret = drm_encoder_init(drm, enc, _encoder_funcs, type, NULL);
+   if (ret)
+   return ret;
+
+   enc->possible_crtcs = crtcs;
+   enc->bridge = bridge;
+   bridge->encoder = enc;
+
+   ret = drm_bridge_attach(enc, bridge, NULL);
+   if (ret)
+   dev_err(dev, "drm_bridge_attach() failed: %d\n", ret);
+
+   return ret;
+}
+
+static int hack_of_add_crtc_encoders(struct drm_device *drm,
+struct device_node *port)
+{
+   struct device *dev = drm->dev;
+   struct device_node *ep, *remote;
+   struct drm_bridge *bridge;
+   u32 crtcs;
+   int ret;
+
+   for_each_child_of_node(port, ep) {
+   remote = of_graph_get_remote_port_parent(ep);
+   if (!remote || !of_device_is_available(remote) ||
+   !of_device_is_available(remote->parent)) {
+   of_node_put(remote);
+   continue;
+   }
+
+   bridge = of_drm_find_bridge(remote);
+dev_info(dev, "found remote %s => bridge %p\n", of_node_full_name(remote), 
bridge);
+   if (!bridge) {
+   of_node_put(remote);
+   continue;
+   }
+
+   crtcs = drm_of_find_possible_crtcs(drm, remote);
+   /* If no CRTCs were found, fall back to our old behaviour */
+   if (crtcs == 0) {
+   dev_warn(dev, "Falling back to first CRTC\n");
+   crtcs = 1 << 0;
+   }
+
+   ret = dummy_encoder_add(dev, drm, bridge, DRM_MODE_ENCODER_TMDS,
+   crtcs);
+   if (ret) {
+   dev_err(dev, "drm_bridge_attach() failed: %d\n", ret);
+   of_node_put(ep);
+   return ret;
+   }
+   }
+
+   return 0;
+}
+
+static int hack_create_encoders(struct drm_device *drm)
+{
+   struct device *dev = drm->dev;
+   struct device_node *port = NULL;
+   int i, ret = 0;
+
+   if (dev->of_node) {
+   for (i = 0; ; i++) {
+   port = of_parse_phandle(dev->of_node, "ports", i);
+   if (!port)
+   break;
+
+   if (of_device_is_available(port->parent))
+   ret = hack_of_add_crtc_encoders(drm, port);
+
+   of_node_put(port);
+
+   if (ret)
+   break;
+   }
+   } else if (dev->platform_data) {
+   const char **devices = dev->platform_data;
+   struct device *d;
+
+   for (i = 0; devices[i]; i++) {
+   d = bus_find_device_by_name(_bus_type, NULL,
+   devices[i]);
+   if (d && d->of_node) {
+   

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Thu, Apr 19, 2018 at 06:27:51PM +0200, Peter Rosin wrote:
> This makes this driver work with all(?) drivers that are not
> componentized and instead expect to connect to a panel/bridge. That
> said, the only one tested is atmel_hlcdc.
> 
> This hooks the relevant work function previously called by the encoder
> and the component also to the bridge, since the encoder goes away when
> connecting to the bridge interface of the driver and the equivalent of
> bind/unbind of the component is handled by bridge attach/detach.
> 
> The lifetime requirements of a bridge and a component are slightly
> different, which is the reason for struct tda998x_bridge.

As we are talking about bridge stuff, the patch below is what I've had
for a while converting Armada to be able to use a bridge-based tda998x.
As you can see, it's far from satisfactory at the moment.  Specifically:

1) it assumes all bridges are TMDS bridges, because as far as I can see,
   there's no way to know any different.
2) there's no way to really know whether a missing bridge is because we
   should be using the component helpers or that the bridge device
   doesn't exist yet.

diff --git a/drivers/gpu/drm/armada/armada_drv.c 
b/drivers/gpu/drm/armada/armada_drv.c
index 262409cae8bf..854d74466dec 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -20,6 +20,127 @@
 #include 
 #include "armada_ioctlP.h"
 
+static const struct drm_encoder_helper_funcs dummy_encoder_helper_funcs = {
+};
+
+static const struct drm_encoder_funcs dummy_encoder_funcs = {
+   .destroy = drm_encoder_cleanup,
+};
+
+static int dummy_encoder_add(struct device *dev, struct drm_device *drm,
+struct drm_bridge *bridge, int type, u32 crtcs)
+{
+   struct drm_encoder *enc;
+   int ret;
+
+   enc = devm_kzalloc(dev, sizeof(*enc), GFP_KERNEL);
+   if (!enc)
+   return -ENOMEM;
+
+   drm_encoder_helper_add(enc, _encoder_helper_funcs);
+   ret = drm_encoder_init(drm, enc, _encoder_funcs, type, NULL);
+   if (ret)
+   return ret;
+
+   enc->possible_crtcs = crtcs;
+   enc->bridge = bridge;
+   bridge->encoder = enc;
+
+   ret = drm_bridge_attach(enc, bridge, NULL);
+   if (ret)
+   dev_err(dev, "drm_bridge_attach() failed: %d\n", ret);
+
+   return ret;
+}
+
+static int hack_of_add_crtc_encoders(struct drm_device *drm,
+struct device_node *port)
+{
+   struct device *dev = drm->dev;
+   struct device_node *ep, *remote;
+   struct drm_bridge *bridge;
+   u32 crtcs;
+   int ret;
+
+   for_each_child_of_node(port, ep) {
+   remote = of_graph_get_remote_port_parent(ep);
+   if (!remote || !of_device_is_available(remote) ||
+   !of_device_is_available(remote->parent)) {
+   of_node_put(remote);
+   continue;
+   }
+
+   bridge = of_drm_find_bridge(remote);
+dev_info(dev, "found remote %s => bridge %p\n", of_node_full_name(remote), 
bridge);
+   if (!bridge) {
+   of_node_put(remote);
+   continue;
+   }
+
+   crtcs = drm_of_find_possible_crtcs(drm, remote);
+   /* If no CRTCs were found, fall back to our old behaviour */
+   if (crtcs == 0) {
+   dev_warn(dev, "Falling back to first CRTC\n");
+   crtcs = 1 << 0;
+   }
+
+   ret = dummy_encoder_add(dev, drm, bridge, DRM_MODE_ENCODER_TMDS,
+   crtcs);
+   if (ret) {
+   dev_err(dev, "drm_bridge_attach() failed: %d\n", ret);
+   of_node_put(ep);
+   return ret;
+   }
+   }
+
+   return 0;
+}
+
+static int hack_create_encoders(struct drm_device *drm)
+{
+   struct device *dev = drm->dev;
+   struct device_node *port = NULL;
+   int i, ret = 0;
+
+   if (dev->of_node) {
+   for (i = 0; ; i++) {
+   port = of_parse_phandle(dev->of_node, "ports", i);
+   if (!port)
+   break;
+
+   if (of_device_is_available(port->parent))
+   ret = hack_of_add_crtc_encoders(drm, port);
+
+   of_node_put(port);
+
+   if (ret)
+   break;
+   }
+   } else if (dev->platform_data) {
+   const char **devices = dev->platform_data;
+   struct device *d;
+
+   for (i = 0; devices[i]; i++) {
+   d = bus_find_device_by_name(_bus_type, NULL,
+   devices[i]);
+   if (d && d->of_node) {
+   

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Fri, Apr 20, 2018 at 12:49:42PM +0200, Peter Rosin wrote:
> On 2018-04-20 12:41, kbuild test robot wrote:
> > Hi Peter,
> > 
> > I love your patch! Yet something to improve:
> 
> Yup, right you are!
> 
> > [auto build test ERROR on drm/drm-next]
> > [also build test ERROR on v4.17-rc1 next-20180420]
> > [if your patch is applied to the wrong git tree, please drop us a note to 
> > help improve the system]
> > 
> > url:
> > https://github.com/0day-ci/linux/commits/Peter-Rosin/Add-tda998x-HDMI-support-to-atmel-hlcdc/20180420-160131
> > base:   git://people.freedesktop.org/~airlied/linux.git drm-next
> > config: i386-randconfig-a0-201815 (attached as .config)
> > compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
> > reproduce:
> > # save the attached .config to linux build tree
> > make ARCH=i386 
> > 
> > All errors (new ones prefixed by >>):
> > 
> >drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_probe':
> >>> drivers/gpu/drm/i2c/tda998x_drv.c:1859:16: error: 'struct drm_bridge' has 
> >>> no member named 'of_node'
> >  bridge->bridge.of_node = dev->of_node;
> >^
> 
> Anybody got a better fix than this?
> 
> #ifdef CONFIG_OF
>   bridge->bridge.of_node = dev->of_node;
> #endif

How about the bridge code provides a helper to do this, something like:

static inline void bridge_set_device(struct drm_bridge *bridge,
 struct device *dev)
{
#ifdef CONFIG_OF
bridge->of_node = dev->of_node;
#endif
}

which (a) nicely hides the firmware flavour, and (b) hides the ifdef in
the bridge header where it belongs.  If the bridge code needs to be
converted to fwnode in the future, at least this would be abstracted
from the drivers.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Fri, Apr 20, 2018 at 12:49:42PM +0200, Peter Rosin wrote:
> On 2018-04-20 12:41, kbuild test robot wrote:
> > Hi Peter,
> > 
> > I love your patch! Yet something to improve:
> 
> Yup, right you are!
> 
> > [auto build test ERROR on drm/drm-next]
> > [also build test ERROR on v4.17-rc1 next-20180420]
> > [if your patch is applied to the wrong git tree, please drop us a note to 
> > help improve the system]
> > 
> > url:
> > https://github.com/0day-ci/linux/commits/Peter-Rosin/Add-tda998x-HDMI-support-to-atmel-hlcdc/20180420-160131
> > base:   git://people.freedesktop.org/~airlied/linux.git drm-next
> > config: i386-randconfig-a0-201815 (attached as .config)
> > compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
> > reproduce:
> > # save the attached .config to linux build tree
> > make ARCH=i386 
> > 
> > All errors (new ones prefixed by >>):
> > 
> >drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_probe':
> >>> drivers/gpu/drm/i2c/tda998x_drv.c:1859:16: error: 'struct drm_bridge' has 
> >>> no member named 'of_node'
> >  bridge->bridge.of_node = dev->of_node;
> >^
> 
> Anybody got a better fix than this?
> 
> #ifdef CONFIG_OF
>   bridge->bridge.of_node = dev->of_node;
> #endif

How about the bridge code provides a helper to do this, something like:

static inline void bridge_set_device(struct drm_bridge *bridge,
 struct device *dev)
{
#ifdef CONFIG_OF
bridge->of_node = dev->of_node;
#endif
}

which (a) nicely hides the firmware flavour, and (b) hides the ifdef in
the bridge header where it belongs.  If the bridge code needs to be
converted to fwnode in the future, at least this would be abstracted
from the drivers.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-20 12:41, kbuild test robot wrote:
> Hi Peter,
> 
> I love your patch! Yet something to improve:

Yup, right you are!

> [auto build test ERROR on drm/drm-next]
> [also build test ERROR on v4.17-rc1 next-20180420]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Peter-Rosin/Add-tda998x-HDMI-support-to-atmel-hlcdc/20180420-160131
> base:   git://people.freedesktop.org/~airlied/linux.git drm-next
> config: i386-randconfig-a0-201815 (attached as .config)
> compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_probe':
>>> drivers/gpu/drm/i2c/tda998x_drv.c:1859:16: error: 'struct drm_bridge' has 
>>> no member named 'of_node'
>  bridge->bridge.of_node = dev->of_node;
>^

Anybody got a better fix than this?

#ifdef CONFIG_OF
bridge->bridge.of_node = dev->of_node;
#endif

Cheers,
Peter

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

On 2018-04-20 12:41, kbuild test robot wrote:
> Hi Peter,
> 
> I love your patch! Yet something to improve:

Yup, right you are!

> [auto build test ERROR on drm/drm-next]
> [also build test ERROR on v4.17-rc1 next-20180420]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Peter-Rosin/Add-tda998x-HDMI-support-to-atmel-hlcdc/20180420-160131
> base:   git://people.freedesktop.org/~airlied/linux.git drm-next
> config: i386-randconfig-a0-201815 (attached as .config)
> compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_probe':
>>> drivers/gpu/drm/i2c/tda998x_drv.c:1859:16: error: 'struct drm_bridge' has 
>>> no member named 'of_node'
>  bridge->bridge.of_node = dev->of_node;
>^

Anybody got a better fix than this?

#ifdef CONFIG_OF
bridge->bridge.of_node = dev->of_node;
#endif

Cheers,
Peter

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[kbuild test robot]

Hi Peter,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Peter-Rosin/Add-tda998x-HDMI-support-to-atmel-hlcdc/20180420-160131
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: i386-randconfig-a0-201815 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_probe':
>> drivers/gpu/drm/i2c/tda998x_drv.c:1859:16: error: 'struct drm_bridge' has no 
>> member named 'of_node'
 bridge->bridge.of_node = dev->of_node;
   ^

vim +1859 drivers/gpu/drm/i2c/tda998x_drv.c

  1838  
  1839  static int
  1840  tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
  1841  {
  1842  struct device *dev = >dev;
  1843  struct tda998x_bridge *bridge;
  1844  int ret;
  1845  
  1846  if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
  1847  dev_warn(>dev, "adapter does not support 
I2C\n");
  1848  return -EIO;
  1849  }
  1850  
  1851  bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
  1852  if (!bridge)
  1853  return -ENOMEM;
  1854  
  1855  bridge->dev = dev;
  1856  dev_set_drvdata(dev, bridge);
  1857  
  1858  bridge->bridge.funcs = _bridge_funcs;
> 1859  bridge->bridge.of_node = dev->of_node;
  1860  drm_bridge_add(>bridge);
  1861  
  1862  ret = component_add(dev, _ops);
  1863  
  1864  if (ret)
  1865  drm_bridge_remove(>bridge);
  1866  
  1867  return ret;
  1868  }
  1869  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[kbuild test robot]

Hi Peter,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Peter-Rosin/Add-tda998x-HDMI-support-to-atmel-hlcdc/20180420-160131
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: i386-randconfig-a0-201815 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_probe':
>> drivers/gpu/drm/i2c/tda998x_drv.c:1859:16: error: 'struct drm_bridge' has no 
>> member named 'of_node'
 bridge->bridge.of_node = dev->of_node;
   ^

vim +1859 drivers/gpu/drm/i2c/tda998x_drv.c

  1838  
  1839  static int
  1840  tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
  1841  {
  1842  struct device *dev = >dev;
  1843  struct tda998x_bridge *bridge;
  1844  int ret;
  1845  
  1846  if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
  1847  dev_warn(>dev, "adapter does not support 
I2C\n");
  1848  return -EIO;
  1849  }
  1850  
  1851  bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
  1852  if (!bridge)
  1853  return -ENOMEM;
  1854  
  1855  bridge->dev = dev;
  1856  dev_set_drvdata(dev, bridge);
  1857  
  1858  bridge->bridge.funcs = _bridge_funcs;
> 1859  bridge->bridge.of_node = dev->of_node;
  1860  drm_bridge_add(>bridge);
  1861  
  1862  ret = component_add(dev, _ops);
  1863  
  1864  if (ret)
  1865  drm_bridge_remove(>bridge);
  1866  
  1867  return ret;
  1868  }
  1869  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Fri, Apr 20, 2018 at 01:06:49PM +0300, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patch.
> 
> On Thursday, 19 April 2018 19:27:51 EEST Peter Rosin wrote:
> > This makes this driver work with all(?) drivers that are not
> > componentized and instead expect to connect to a panel/bridge. That
> > said, the only one tested is atmel_hlcdc.
> > 
> > This hooks the relevant work function previously called by the encoder
> > and the component also to the bridge, since the encoder goes away when
> > connecting to the bridge interface of the driver and the equivalent of
> > bind/unbind of the component is handled by bridge attach/detach.
> > 
> > The lifetime requirements of a bridge and a component are slightly
> > different, which is the reason for struct tda998x_bridge.
> 
> Couldn't you move the allocation and initialization (tda998x_create) of the 
> tda998x_priv structure to probe time ? I think you wouldn't need a separate 
> structure in that case. Unless I'm mistaken there would be an added benefit 
> of 
> separating component and bridge initialization, resulting in the encoder not 
> being initialized at all if the component isn't used. You wouldn't need to 
> add 
> a local_encoder parameter to the tda998x_init() function.

No, I don't like that idea one bit, as I've stated in the past about the
component API.  The same (probably) goes for the bridge stuff too.

Consider the following:

Your DRM system is initialised.  You then remove a module, which results
in the DRM system being torn down.  You re-insert the module (eg, having
made a change to it).  The DRM system is then re-initialised.

At this point, what is the state of variables such as priv->is_on if
you allocate the structure at probe time?

What about all the other variables in the driver private structure - are
you sure that the driver can cope with random values from the previous
"usage" remaining there?

At the moment, this isn't a concern for the driver because we
dev_kzalloc() the structure in the bind callback.  Move that to the
probe function, and the structure is no longer re-initialised each
time, and so it retains the previous state.  The driver is not setup
to cope with that.

So, to work around that, you would need to reinitialise _everything_
in the structure that the driver requires, which IMHO is a very
open to bugs (eg, if a member is missed, or added without the
necessary re-initialisation), _especially_ when this is not a path
that will get regular testing.

If you want to do this for a subset of data, it would be much better
to separate them into independent structures (maybe one embedded into
the other) so that this problem can not occur.  That way, a subset
of the data can be memset() when bound to the rest of the DRM system
ensuring a consistent driver state and still achieve what you're
suggesting.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Fri, Apr 20, 2018 at 01:06:49PM +0300, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patch.
> 
> On Thursday, 19 April 2018 19:27:51 EEST Peter Rosin wrote:
> > This makes this driver work with all(?) drivers that are not
> > componentized and instead expect to connect to a panel/bridge. That
> > said, the only one tested is atmel_hlcdc.
> > 
> > This hooks the relevant work function previously called by the encoder
> > and the component also to the bridge, since the encoder goes away when
> > connecting to the bridge interface of the driver and the equivalent of
> > bind/unbind of the component is handled by bridge attach/detach.
> > 
> > The lifetime requirements of a bridge and a component are slightly
> > different, which is the reason for struct tda998x_bridge.
> 
> Couldn't you move the allocation and initialization (tda998x_create) of the 
> tda998x_priv structure to probe time ? I think you wouldn't need a separate 
> structure in that case. Unless I'm mistaken there would be an added benefit 
> of 
> separating component and bridge initialization, resulting in the encoder not 
> being initialized at all if the component isn't used. You wouldn't need to 
> add 
> a local_encoder parameter to the tda998x_init() function.

No, I don't like that idea one bit, as I've stated in the past about the
component API.  The same (probably) goes for the bridge stuff too.

Consider the following:

Your DRM system is initialised.  You then remove a module, which results
in the DRM system being torn down.  You re-insert the module (eg, having
made a change to it).  The DRM system is then re-initialised.

At this point, what is the state of variables such as priv->is_on if
you allocate the structure at probe time?

What about all the other variables in the driver private structure - are
you sure that the driver can cope with random values from the previous
"usage" remaining there?

At the moment, this isn't a concern for the driver because we
dev_kzalloc() the structure in the bind callback.  Move that to the
probe function, and the structure is no longer re-initialised each
time, and so it retains the previous state.  The driver is not setup
to cope with that.

So, to work around that, you would need to reinitialise _everything_
in the structure that the driver requires, which IMHO is a very
open to bugs (eg, if a member is missed, or added without the
necessary re-initialisation), _especially_ when this is not a path
that will get regular testing.

If you want to do this for a subset of data, it would be much better
to separate them into independent structures (maybe one embedded into
the other) so that this problem can not occur.  That way, a subset
of the data can be memset() when bound to the rest of the DRM system
ensuring a consistent driver state and still achieve what you're
suggesting.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Thursday, 19 April 2018 19:27:51 EEST Peter Rosin wrote:
> This makes this driver work with all(?) drivers that are not
> componentized and instead expect to connect to a panel/bridge. That
> said, the only one tested is atmel_hlcdc.
> 
> This hooks the relevant work function previously called by the encoder
> and the component also to the bridge, since the encoder goes away when
> connecting to the bridge interface of the driver and the equivalent of
> bind/unbind of the component is handled by bridge attach/detach.
> 
> The lifetime requirements of a bridge and a component are slightly
> different, which is the reason for struct tda998x_bridge.

Couldn't you move the allocation and initialization (tda998x_create) of the 
tda998x_priv structure to probe time ? I think you wouldn't need a separate 
structure in that case. Unless I'm mistaken there would be an added benefit of 
separating component and bridge initialization, resulting in the encoder not 
being initialized at all if the component isn't used. You wouldn't need to add 
a local_encoder parameter to the tda998x_init() function.

> Signed-off-by: Peter Rosin 
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 157 ---
>  1 file changed, 137 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c index 9c78f7bde49c..012dee61d817 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -36,6 +36,14 @@ struct tda998x_audio_port {
>   u8 config;  /* AP value */
>  };
> 
> +struct tda998x_priv;
> +
> +struct tda998x_bridge {
> + struct tda998x_priv *priv;
> + struct device *dev;
> + struct drm_bridge bridge;
> +};
> +
>  struct tda998x_priv {
>   struct i2c_client *cec;
>   struct i2c_client *hdmi;
> @@ -63,6 +71,8 @@ struct tda998x_priv {
>   wait_queue_head_t edid_delay_waitq;
>   bool edid_delay_active;
> 
> + struct tda998x_bridge *bridge;
> + bool local_encoder;
>   struct drm_encoder encoder;
>   struct drm_connector connector;
> 
> @@ -75,6 +85,9 @@ struct tda998x_priv {
>  #define enc_to_tda998x_priv(x) \
>   container_of(x, struct tda998x_priv, encoder)
> 
> +#define bridge_to_tda998x_bridge(x) \
> + container_of(x, struct tda998x_bridge, bridge)
> +
>  /* The TDA9988 series of devices use a paged register scheme.. to simplify
>   * things we encode the page # in upper bits of the register #.  To read/
>   * write a given register, we need to make sure CURPAGE register is set
> @@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev,
> void *data, struct hdmi_codec_daifmt *daifmt,
>  struct hdmi_codec_params *params)
>  {
> - struct tda998x_priv *priv = dev_get_drvdata(dev);
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> + struct tda998x_priv *priv = bridge->priv;
>   int i, ret;
>   struct tda998x_audio_params audio = {
>   .sample_width = params->sample_width,
> @@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev,
> void *data,
> 
>  static void tda998x_audio_shutdown(struct device *dev, void *data)
>  {
> - struct tda998x_priv *priv = dev_get_drvdata(dev);
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> + struct tda998x_priv *priv = bridge->priv;
> 
>   mutex_lock(>audio_mutex);
> 
> @@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev,
> void *data)
> 
>  int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
> {
> - struct tda998x_priv *priv = dev_get_drvdata(dev);
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> + struct tda998x_priv *priv = bridge->priv;
> 
>   mutex_lock(>audio_mutex);
> 
> @@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void
> *data, bool enable) static int tda998x_audio_get_eld(struct device *dev,
> void *data,
>uint8_t *buf, size_t len)
>  {
> - struct tda998x_priv *priv = dev_get_drvdata(dev);
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> + struct tda998x_priv *priv = bridge->priv;
> 
>   mutex_lock(>audio_mutex);
>   memcpy(buf, priv->connector.eld,
> @@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector
> *connector) {
>   struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
> 
> - return >encoder;
> + if (priv->local_encoder)
> + return >encoder;
> + else
> + return priv->bridge->bridge.encoder;
>  }
> 
>  static
> @@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv, struct drm_device *drm)
>  {
>   struct drm_connector *connector = >connector;
> + struct drm_encoder *encoder;
>   int ret;
> 
>   connector->interlace_allowed = 1;
> @@ -1156,7 

Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Thursday, 19 April 2018 19:27:51 EEST Peter Rosin wrote:
> This makes this driver work with all(?) drivers that are not
> componentized and instead expect to connect to a panel/bridge. That
> said, the only one tested is atmel_hlcdc.
> 
> This hooks the relevant work function previously called by the encoder
> and the component also to the bridge, since the encoder goes away when
> connecting to the bridge interface of the driver and the equivalent of
> bind/unbind of the component is handled by bridge attach/detach.
> 
> The lifetime requirements of a bridge and a component are slightly
> different, which is the reason for struct tda998x_bridge.

Couldn't you move the allocation and initialization (tda998x_create) of the 
tda998x_priv structure to probe time ? I think you wouldn't need a separate 
structure in that case. Unless I'm mistaken there would be an added benefit of 
separating component and bridge initialization, resulting in the encoder not 
being initialized at all if the component isn't used. You wouldn't need to add 
a local_encoder parameter to the tda998x_init() function.

> Signed-off-by: Peter Rosin 
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 157 ---
>  1 file changed, 137 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c index 9c78f7bde49c..012dee61d817 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -36,6 +36,14 @@ struct tda998x_audio_port {
>   u8 config;  /* AP value */
>  };
> 
> +struct tda998x_priv;
> +
> +struct tda998x_bridge {
> + struct tda998x_priv *priv;
> + struct device *dev;
> + struct drm_bridge bridge;
> +};
> +
>  struct tda998x_priv {
>   struct i2c_client *cec;
>   struct i2c_client *hdmi;
> @@ -63,6 +71,8 @@ struct tda998x_priv {
>   wait_queue_head_t edid_delay_waitq;
>   bool edid_delay_active;
> 
> + struct tda998x_bridge *bridge;
> + bool local_encoder;
>   struct drm_encoder encoder;
>   struct drm_connector connector;
> 
> @@ -75,6 +85,9 @@ struct tda998x_priv {
>  #define enc_to_tda998x_priv(x) \
>   container_of(x, struct tda998x_priv, encoder)
> 
> +#define bridge_to_tda998x_bridge(x) \
> + container_of(x, struct tda998x_bridge, bridge)
> +
>  /* The TDA9988 series of devices use a paged register scheme.. to simplify
>   * things we encode the page # in upper bits of the register #.  To read/
>   * write a given register, we need to make sure CURPAGE register is set
> @@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev,
> void *data, struct hdmi_codec_daifmt *daifmt,
>  struct hdmi_codec_params *params)
>  {
> - struct tda998x_priv *priv = dev_get_drvdata(dev);
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> + struct tda998x_priv *priv = bridge->priv;
>   int i, ret;
>   struct tda998x_audio_params audio = {
>   .sample_width = params->sample_width,
> @@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev,
> void *data,
> 
>  static void tda998x_audio_shutdown(struct device *dev, void *data)
>  {
> - struct tda998x_priv *priv = dev_get_drvdata(dev);
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> + struct tda998x_priv *priv = bridge->priv;
> 
>   mutex_lock(>audio_mutex);
> 
> @@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev,
> void *data)
> 
>  int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
> {
> - struct tda998x_priv *priv = dev_get_drvdata(dev);
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> + struct tda998x_priv *priv = bridge->priv;
> 
>   mutex_lock(>audio_mutex);
> 
> @@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void
> *data, bool enable) static int tda998x_audio_get_eld(struct device *dev,
> void *data,
>uint8_t *buf, size_t len)
>  {
> - struct tda998x_priv *priv = dev_get_drvdata(dev);
> + struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> + struct tda998x_priv *priv = bridge->priv;
> 
>   mutex_lock(>audio_mutex);
>   memcpy(buf, priv->connector.eld,
> @@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector
> *connector) {
>   struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
> 
> - return >encoder;
> + if (priv->local_encoder)
> + return >encoder;
> + else
> + return priv->bridge->bridge.encoder;
>  }
> 
>  static
> @@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv, struct drm_device *drm)
>  {
>   struct drm_connector *connector = >connector;
> + struct drm_encoder *encoder;
>   int ret;
> 
>   connector->interlace_allowed = 1;
> @@ -1156,7 +1177,8 @@ static int 

[PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

This makes this driver work with all(?) drivers that are not
componentized and instead expect to connect to a panel/bridge. That
said, the only one tested is atmel_hlcdc.

This hooks the relevant work function previously called by the encoder
and the component also to the bridge, since the encoder goes away when
connecting to the bridge interface of the driver and the equivalent of
bind/unbind of the component is handled by bridge attach/detach.

The lifetime requirements of a bridge and a component are slightly
different, which is the reason for struct tda998x_bridge.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 157 +-
 1 file changed, 137 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9c78f7bde49c..012dee61d817 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -36,6 +36,14 @@ struct tda998x_audio_port {
u8 config;  /* AP value */
 };
 
+struct tda998x_priv;
+
+struct tda998x_bridge {
+   struct tda998x_priv *priv;
+   struct device *dev;
+   struct drm_bridge bridge;
+};
+
 struct tda998x_priv {
struct i2c_client *cec;
struct i2c_client *hdmi;
@@ -63,6 +71,8 @@ struct tda998x_priv {
wait_queue_head_t edid_delay_waitq;
bool edid_delay_active;
 
+   struct tda998x_bridge *bridge;
+   bool local_encoder;
struct drm_encoder encoder;
struct drm_connector connector;
 
@@ -75,6 +85,9 @@ struct tda998x_priv {
 #define enc_to_tda998x_priv(x) \
container_of(x, struct tda998x_priv, encoder)
 
+#define bridge_to_tda998x_bridge(x) \
+   container_of(x, struct tda998x_bridge, bridge)
+
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
  * write a given register, we need to make sure CURPAGE register is set
@@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev, void 
*data,
   struct hdmi_codec_daifmt *daifmt,
   struct hdmi_codec_params *params)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
int i, ret;
struct tda998x_audio_params audio = {
.sample_width = params->sample_width,
@@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev, void 
*data,
 
 static void tda998x_audio_shutdown(struct device *dev, void *data)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
 
@@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev, void 
*data)
 
 int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
 
@@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void 
*data, bool enable)
 static int tda998x_audio_get_eld(struct device *dev, void *data,
 uint8_t *buf, size_t len)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
memcpy(buf, priv->connector.eld,
@@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector 
*connector)
 {
struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
 
-   return >encoder;
+   if (priv->local_encoder)
+   return >encoder;
+   else
+   return priv->bridge->bridge.encoder;
 }
 
 static
@@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv 
*priv,
  struct drm_device *drm)
 {
struct drm_connector *connector = >connector;
+   struct drm_encoder *encoder;
int ret;
 
connector->interlace_allowed = 1;
@@ -1156,7 +1177,8 @@ static int tda998x_connector_init(struct tda998x_priv 
*priv,
if (ret)
return ret;
 
-   drm_mode_connector_attach_encoder(>connector, >encoder);
+   encoder = tda998x_connector_best_encoder(>connector);
+   drm_mode_connector_attach_encoder(>connector, encoder);
 
return 0;
 }
@@ -1668,8 +1690,10 @@ static void tda998x_set_config(struct tda998x_priv *priv,
priv->audio_params = p->audio_params;
 }
 
-static int tda998x_init(struct device *dev, struct drm_device *drm)
+static int tda998x_init(struct device *dev, struct drm_device *drm,

[PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

This makes this driver work with all(?) drivers that are not
componentized and instead expect to connect to a panel/bridge. That
said, the only one tested is atmel_hlcdc.

This hooks the relevant work function previously called by the encoder
and the component also to the bridge, since the encoder goes away when
connecting to the bridge interface of the driver and the equivalent of
bind/unbind of the component is handled by bridge attach/detach.

The lifetime requirements of a bridge and a component are slightly
different, which is the reason for struct tda998x_bridge.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 157 +-
 1 file changed, 137 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9c78f7bde49c..012dee61d817 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -36,6 +36,14 @@ struct tda998x_audio_port {
u8 config;  /* AP value */
 };
 
+struct tda998x_priv;
+
+struct tda998x_bridge {
+   struct tda998x_priv *priv;
+   struct device *dev;
+   struct drm_bridge bridge;
+};
+
 struct tda998x_priv {
struct i2c_client *cec;
struct i2c_client *hdmi;
@@ -63,6 +71,8 @@ struct tda998x_priv {
wait_queue_head_t edid_delay_waitq;
bool edid_delay_active;
 
+   struct tda998x_bridge *bridge;
+   bool local_encoder;
struct drm_encoder encoder;
struct drm_connector connector;
 
@@ -75,6 +85,9 @@ struct tda998x_priv {
 #define enc_to_tda998x_priv(x) \
container_of(x, struct tda998x_priv, encoder)
 
+#define bridge_to_tda998x_bridge(x) \
+   container_of(x, struct tda998x_bridge, bridge)
+
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
  * write a given register, we need to make sure CURPAGE register is set
@@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev, void 
*data,
   struct hdmi_codec_daifmt *daifmt,
   struct hdmi_codec_params *params)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
int i, ret;
struct tda998x_audio_params audio = {
.sample_width = params->sample_width,
@@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev, void 
*data,
 
 static void tda998x_audio_shutdown(struct device *dev, void *data)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
 
@@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev, void 
*data)
 
 int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
 
@@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void 
*data, bool enable)
 static int tda998x_audio_get_eld(struct device *dev, void *data,
 uint8_t *buf, size_t len)
 {
-   struct tda998x_priv *priv = dev_get_drvdata(dev);
+   struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+   struct tda998x_priv *priv = bridge->priv;
 
mutex_lock(>audio_mutex);
memcpy(buf, priv->connector.eld,
@@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector 
*connector)
 {
struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
 
-   return >encoder;
+   if (priv->local_encoder)
+   return >encoder;
+   else
+   return priv->bridge->bridge.encoder;
 }
 
 static
@@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv 
*priv,
  struct drm_device *drm)
 {
struct drm_connector *connector = >connector;
+   struct drm_encoder *encoder;
int ret;
 
connector->interlace_allowed = 1;
@@ -1156,7 +1177,8 @@ static int tda998x_connector_init(struct tda998x_priv 
*priv,
if (ret)
return ret;
 
-   drm_mode_connector_attach_encoder(>connector, >encoder);
+   encoder = tda998x_connector_best_encoder(>connector);
+   drm_mode_connector_attach_encoder(>connector, encoder);
 
return 0;
 }
@@ -1668,8 +1690,10 @@ static void tda998x_set_config(struct tda998x_priv *priv,
priv->audio_params = p->audio_params;
 }
 
-static int tda998x_init(struct device *dev, struct drm_device *drm)
+static int tda998x_init(struct device *dev, struct drm_device *drm,
+