[RFC] drm: i2c: add irq handler for tda998x slave encoder

[Sebastian Hesselbarth]

On 05/19/2013 10:45 PM, Russell King - ARM Linux wrote:
> On Sun, May 19, 2013 at 06:49:22PM +0200, Sebastian Hesselbarth wrote:
>> This adds an irq handler for HPD to the tda998x slave encoder driver
>> to trigger HPD change instead of polling. The gpio connected to int
>> pin of tda998x is passed through platform_data of the i2c client. As
>> HPD will ultimately cause EDID read and that will raise an
>> EDID_READ_DONE interrupt, the irq handling is done threaded with a
>> workqueue to notify drm backend of HPD events.
>>
>> Signed-off-by: Sebastian Hesselbarth
>
> Okay, I think I get this..  Some comments:
>
>> +static irqreturn_t tda998x_irq_thread(int irq, void *data)
>> +{
>> +struct drm_encoder *encoder = data;
>> +struct tda998x_priv *priv;
>> +uint8_t sta, cec, hdmi, lev;
>> +
>> +if (!encoder)
>> +return IRQ_HANDLED;
>
> This won't ever be NULL.  The IRQ layer stores the pointer you passed
> in request_threaded_irq() and that pointer will continue to point at
> that memory until the IRQ is freed.  The only way it could be NULL is
> if you register using a NULL pointer.

Russell,

thanks for the comments. Of course, encoder can't be NULL here and I'll
remove that check.

> ...
>> +if (priv->irq<  0) {
>> +for (i = 100; i>  0; i--) {
>> +uint8_t val = reg_read(encoder, REG_INT_FLAGS_2);
>
> IRQ 0 is the cross-arch "no interrupt" number.  So just use !priv->irq
> here and encourage anyone who uses -1 or NO_IRQ to fix their stuff. :)

Ok, but gpio 0 still is a cross-arch valid gpio? ;)

>> +struct tda998x_priv *priv = to_tda998x_priv(encoder);
>> +
>> +/* announce polling if no irq is available */
>> +if (priv->irq<  0)
>
> Same here.
>
>> @@ -1122,7 +1197,9 @@ tda998x_encoder_init(struct i2c_client *client,
>>
>>  priv->current_page = 0;
>>  priv->cec = i2c_new_dummy(client->adapter, 0x34);
>> +priv->irq = -EINVAL;
>
> And just initialize to zero.  (it's allocated by kzalloc already right?
> So that shouldn't be necessary.)
>
>> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
>> index 41f799f..1838703 100644
>> --- a/include/drm/i2c/tda998x.h
>> +++ b/include/drm/i2c/tda998x.h
>> @@ -20,4 +20,8 @@ struct tda998x_encoder_params {
>>  int swap_f, mirr_f;
>>   };
>>
>> +struct tda998x_platform_data {
>> +int int_gpio;
>> +};
>> +
>
> Should be combined with tda998x_encoder_params - the platform data is
> really supposed to be passed to set_config - see this in drm_encoder_slave.c:
>
>   * If @info->platform_data is non-NULL it will be used as the initial
>   * slave config.
> ...
>  err = encoder_drv->encoder_init(client, dev, encoder);
>  if (err)
>  goto fail_unregister;
>
>  if (info->platform_data)
>  encoder->slave_funcs->set_config(>base,
>   info->platform_data);
>
> So any platform data set will be passed into the set_config function...

Sure, I'll put that into params. Will rebase my v1 PATCH on v3.10-rc1
and that will create tda998x.h.

But actually, this all raises the ultimate "how to deal with DT in drm"
question: Currently, drm i2c slave encoders are registered within drm
API which doesn't work well with DT nodes for those encoders.

DT i2c adapters will register an i2c client and drm will try again in
drm_i2c_encoder_init. Registering the i2c client again will cause it
to fail because the i2c address is busy.

Now, in drm_i2c_encoder_init we have access to the board_info struct
that already offers .of_node which we could exploit here to not
register another i2c client but try to get that already registered
client or fall back to current behavior if NULL. of_node then could
be set by the master encoder from e.g.
"marvell,slave-encoder = <>;".

Or (and that is what I'd prefer) make use of the always empty i2c
slave encoder _probe() as for any other i2c client. And hook up drm
to a probed i2c client. That will also allow for the i2c client
provide functions for other APIs, e.g. alsa.

I am willing to dig into this, but would like to have a general
opinion of David, Rob, and you.

Sebastian

[RFC] drm: i2c: add irq handler for tda998x slave encoder

[Rob Clark]

On Mon, May 20, 2013 at 6:53 AM, Sebastian Hesselbarth
 wrote:
> On 05/19/2013 10:45 PM, Russell King - ARM Linux wrote:
>>
>> On Sun, May 19, 2013 at 06:49:22PM +0200, Sebastian Hesselbarth wrote:
>>>
>>> This adds an irq handler for HPD to the tda998x slave encoder driver
>>> to trigger HPD change instead of polling. The gpio connected to int
>>> pin of tda998x is passed through platform_data of the i2c client. As
>>> HPD will ultimately cause EDID read and that will raise an
>>> EDID_READ_DONE interrupt, the irq handling is done threaded with a
>>> workqueue to notify drm backend of HPD events.
>>>
>>> Signed-off-by: Sebastian Hesselbarth
>>
>>
>> Okay, I think I get this..  Some comments:
>>
>>> +static irqreturn_t tda998x_irq_thread(int irq, void *data)
>>> +{
>>> +   struct drm_encoder *encoder = data;
>>> +   struct tda998x_priv *priv;
>>> +   uint8_t sta, cec, hdmi, lev;
>>> +
>>> +   if (!encoder)
>>> +   return IRQ_HANDLED;
>>
>>
>> This won't ever be NULL.  The IRQ layer stores the pointer you passed
>> in request_threaded_irq() and that pointer will continue to point at
>> that memory until the IRQ is freed.  The only way it could be NULL is
>> if you register using a NULL pointer.
>
>
> Russell,
>
> thanks for the comments. Of course, encoder can't be NULL here and I'll
> remove that check.
>
>
>> ...
>>>
>>> +   if (priv->irq<  0) {
>>> +   for (i = 100; i>  0; i--) {
>>> +   uint8_t val = reg_read(encoder, REG_INT_FLAGS_2);
>>
>>
>> IRQ 0 is the cross-arch "no interrupt" number.  So just use !priv->irq
>> here and encourage anyone who uses -1 or NO_IRQ to fix their stuff. :)
>
>
> Ok, but gpio 0 still is a cross-arch valid gpio? ;)
>
>
>>> +   struct tda998x_priv *priv = to_tda998x_priv(encoder);
>>> +
>>> +   /* announce polling if no irq is available */
>>> +   if (priv->irq<  0)
>>
>>
>> Same here.
>>
>>> @@ -1122,7 +1197,9 @@ tda998x_encoder_init(struct i2c_client *client,
>>>
>>> priv->current_page = 0;
>>> priv->cec = i2c_new_dummy(client->adapter, 0x34);
>>> +   priv->irq = -EINVAL;
>>
>>
>> And just initialize to zero.  (it's allocated by kzalloc already right?
>> So that shouldn't be necessary.)
>>
>>> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
>>> index 41f799f..1838703 100644
>>> --- a/include/drm/i2c/tda998x.h
>>> +++ b/include/drm/i2c/tda998x.h
>>> @@ -20,4 +20,8 @@ struct tda998x_encoder_params {
>>> int swap_f, mirr_f;
>>>   };
>>>
>>> +struct tda998x_platform_data {
>>> +   int int_gpio;
>>> +};
>>> +
>>
>>
>> Should be combined with tda998x_encoder_params - the platform data is
>> really supposed to be passed to set_config - see this in
>> drm_encoder_slave.c:
>>
>>   * If @info->platform_data is non-NULL it will be used as the initial
>>   * slave config.
>> ...
>>  err = encoder_drv->encoder_init(client, dev, encoder);
>>  if (err)
>>  goto fail_unregister;
>>
>>  if (info->platform_data)
>>  encoder->slave_funcs->set_config(>base,
>>   info->platform_data);
>>
>> So any platform data set will be passed into the set_config function...
>
>
> Sure, I'll put that into params. Will rebase my v1 PATCH on v3.10-rc1
> and that will create tda998x.h.
>
> But actually, this all raises the ultimate "how to deal with DT in drm"
> question: Currently, drm i2c slave encoders are registered within drm
> API which doesn't work well with DT nodes for those encoders.
>
> DT i2c adapters will register an i2c client and drm will try again in
> drm_i2c_encoder_init. Registering the i2c client again will cause it
> to fail because the i2c address is busy.
>
> Now, in drm_i2c_encoder_init we have access to the board_info struct
> that already offers .of_node which we could exploit here to not
> register another i2c client but try to get that already registered
> client or fall back to current behavior if NULL. of_node then could
> be set by the master encoder from e.g.
> "marvell,slave-encoder = <>;".
>
> Or (and that is what I'd prefer) make use of the always empty i2c
> slave encoder _probe() as for any other i2c client. And hook up drm
> to a probed i2c client. That will also allow for the i2c client
> provide functions for other APIs, e.g. alsa.
>
> I am willing to dig into this, but would like to have a general
> opinion of David, Rob, and you.

I thing we probably want to re-visit the current way the i2c encoder
slave stuff works, to make for easier support of other sorts of
encoders, and possibly a starting point for shared panel drivers.
I've kinda been waiting to see how the common display/panel framework
stuff plays out, it's also possible that this would replace the i2c
encoder slave stuff.

BR,
-R

> Sebastian

Re: [RFC] drm: i2c: add irq handler for tda998x slave encoder

[Russell King - ARM Linux]

On Sun, May 19, 2013 at 06:49:22PM +0200, Sebastian Hesselbarth wrote:
 This adds an irq handler for HPD to the tda998x slave encoder driver
 to trigger HPD change instead of polling. The gpio connected to int
 pin of tda998x is passed through platform_data of the i2c client. As
 HPD will ultimately cause EDID read and that will raise an
 EDID_READ_DONE interrupt, the irq handling is done threaded with a
 workqueue to notify drm backend of HPD events.
 
 Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com

Okay, I think I get this..  Some comments:

 +static irqreturn_t tda998x_irq_thread(int irq, void *data)
 +{
 + struct drm_encoder *encoder = data;
 + struct tda998x_priv *priv;
 + uint8_t sta, cec, hdmi, lev;
 +
 + if (!encoder)
 + return IRQ_HANDLED;

This won't ever be NULL.  The IRQ layer stores the pointer you passed
in request_threaded_irq() and that pointer will continue to point at
that memory until the IRQ is freed.  The only way it could be NULL is
if you register using a NULL pointer.

...
 + if (priv-irq  0) {
 + for (i = 100; i  0; i--) {
 + uint8_t val = reg_read(encoder, REG_INT_FLAGS_2);

IRQ 0 is the cross-arch no interrupt number.  So just use !priv-irq
here and encourage anyone who uses -1 or NO_IRQ to fix their stuff. :)

 + struct tda998x_priv *priv = to_tda998x_priv(encoder);
 +
 + /* announce polling if no irq is available */
 + if (priv-irq  0)

Same here.

 @@ -1122,7 +1197,9 @@ tda998x_encoder_init(struct i2c_client *client,
  
   priv-current_page = 0;
   priv-cec = i2c_new_dummy(client-adapter, 0x34);
 + priv-irq = -EINVAL;

And just initialize to zero.  (it's allocated by kzalloc already right?
So that shouldn't be necessary.)

 diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
 index 41f799f..1838703 100644
 --- a/include/drm/i2c/tda998x.h
 +++ b/include/drm/i2c/tda998x.h
 @@ -20,4 +20,8 @@ struct tda998x_encoder_params {
   int swap_f, mirr_f;
  };
  
 +struct tda998x_platform_data {
 + int int_gpio;
 +};
 +

Should be combined with tda998x_encoder_params - the platform data is
really supposed to be passed to set_config - see this in drm_encoder_slave.c:

 * If @info-platform_data is non-NULL it will be used as the initial
 * slave config.
...
err = encoder_drv-encoder_init(client, dev, encoder);
if (err)
goto fail_unregister;

if (info-platform_data)
encoder-slave_funcs-set_config(encoder-base,
 info-platform_data);

So any platform data set will be passed into the set_config function...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: i2c: add irq handler for tda998x slave encoder

[Sebastian Hesselbarth]

On 05/19/2013 10:45 PM, Russell King - ARM Linux wrote:

On Sun, May 19, 2013 at 06:49:22PM +0200, Sebastian Hesselbarth wrote:

This adds an irq handler for HPD to the tda998x slave encoder driver
to trigger HPD change instead of polling. The gpio connected to int
pin of tda998x is passed through platform_data of the i2c client. As
HPD will ultimately cause EDID read and that will raise an
EDID_READ_DONE interrupt, the irq handling is done threaded with a
workqueue to notify drm backend of HPD events.

Signed-off-by: Sebastian Hesselbarthsebastian.hesselba...@gmail.com


Okay, I think I get this..  Some comments:


+static irqreturn_t tda998x_irq_thread(int irq, void *data)
+{
+   struct drm_encoder *encoder = data;
+   struct tda998x_priv *priv;
+   uint8_t sta, cec, hdmi, lev;
+
+   if (!encoder)
+   return IRQ_HANDLED;


This won't ever be NULL.  The IRQ layer stores the pointer you passed
in request_threaded_irq() and that pointer will continue to point at
that memory until the IRQ is freed.  The only way it could be NULL is
if you register using a NULL pointer.


Russell,

thanks for the comments. Of course, encoder can't be NULL here and I'll
remove that check.


...

+   if (priv-irq  0) {
+   for (i = 100; i  0; i--) {
+   uint8_t val = reg_read(encoder, REG_INT_FLAGS_2);


IRQ 0 is the cross-arch no interrupt number.  So just use !priv-irq
here and encourage anyone who uses -1 or NO_IRQ to fix their stuff. :)


Ok, but gpio 0 still is a cross-arch valid gpio? ;)


+   struct tda998x_priv *priv = to_tda998x_priv(encoder);
+
+   /* announce polling if no irq is available */
+   if (priv-irq  0)


Same here.


@@ -1122,7 +1197,9 @@ tda998x_encoder_init(struct i2c_client *client,

priv-current_page = 0;
priv-cec = i2c_new_dummy(client-adapter, 0x34);
+   priv-irq = -EINVAL;


And just initialize to zero.  (it's allocated by kzalloc already right?
So that shouldn't be necessary.)


diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 41f799f..1838703 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -20,4 +20,8 @@ struct tda998x_encoder_params {
int swap_f, mirr_f;
  };

+struct tda998x_platform_data {
+   int int_gpio;
+};
+


Should be combined with tda998x_encoder_params - the platform data is
really supposed to be passed to set_config - see this in drm_encoder_slave.c:

  * If @info-platform_data is non-NULL it will be used as the initial
  * slave config.
...
 err = encoder_drv-encoder_init(client, dev, encoder);
 if (err)
 goto fail_unregister;

 if (info-platform_data)
 encoder-slave_funcs-set_config(encoder-base,
  info-platform_data);

So any platform data set will be passed into the set_config function...


Sure, I'll put that into params. Will rebase my v1 PATCH on v3.10-rc1
and that will create tda998x.h.

But actually, this all raises the ultimate how to deal with DT in drm
question: Currently, drm i2c slave encoders are registered within drm
API which doesn't work well with DT nodes for those encoders.

DT i2c adapters will register an i2c client and drm will try again in
drm_i2c_encoder_init. Registering the i2c client again will cause it
to fail because the i2c address is busy.

Now, in drm_i2c_encoder_init we have access to the board_info struct
that already offers .of_node which we could exploit here to not
register another i2c client but try to get that already registered
client or fall back to current behavior if NULL. of_node then could
be set by the master encoder from e.g.
marvell,slave-encoder = tda998x;.

Or (and that is what I'd prefer) make use of the always empty i2c
slave encoder _probe() as for any other i2c client. And hook up drm
to a probed i2c client. That will also allow for the i2c client
provide functions for other APIs, e.g. alsa.

I am willing to dig into this, but would like to have a general
opinion of David, Rob, and you.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: i2c: add irq handler for tda998x slave encoder

[Rob Clark]

On Mon, May 20, 2013 at 6:53 AM, Sebastian Hesselbarth
sebastian.hesselba...@gmail.com wrote:
 On 05/19/2013 10:45 PM, Russell King - ARM Linux wrote:

 On Sun, May 19, 2013 at 06:49:22PM +0200, Sebastian Hesselbarth wrote:

 This adds an irq handler for HPD to the tda998x slave encoder driver
 to trigger HPD change instead of polling. The gpio connected to int
 pin of tda998x is passed through platform_data of the i2c client. As
 HPD will ultimately cause EDID read and that will raise an
 EDID_READ_DONE interrupt, the irq handling is done threaded with a
 workqueue to notify drm backend of HPD events.

 Signed-off-by: Sebastian Hesselbarthsebastian.hesselba...@gmail.com


 Okay, I think I get this..  Some comments:

 +static irqreturn_t tda998x_irq_thread(int irq, void *data)
 +{
 +   struct drm_encoder *encoder = data;
 +   struct tda998x_priv *priv;
 +   uint8_t sta, cec, hdmi, lev;
 +
 +   if (!encoder)
 +   return IRQ_HANDLED;


 This won't ever be NULL.  The IRQ layer stores the pointer you passed
 in request_threaded_irq() and that pointer will continue to point at
 that memory until the IRQ is freed.  The only way it could be NULL is
 if you register using a NULL pointer.


 Russell,

 thanks for the comments. Of course, encoder can't be NULL here and I'll
 remove that check.


 ...

 +   if (priv-irq  0) {
 +   for (i = 100; i  0; i--) {
 +   uint8_t val = reg_read(encoder, REG_INT_FLAGS_2);


 IRQ 0 is the cross-arch no interrupt number.  So just use !priv-irq
 here and encourage anyone who uses -1 or NO_IRQ to fix their stuff. :)


 Ok, but gpio 0 still is a cross-arch valid gpio? ;)


 +   struct tda998x_priv *priv = to_tda998x_priv(encoder);
 +
 +   /* announce polling if no irq is available */
 +   if (priv-irq  0)


 Same here.

 @@ -1122,7 +1197,9 @@ tda998x_encoder_init(struct i2c_client *client,

 priv-current_page = 0;
 priv-cec = i2c_new_dummy(client-adapter, 0x34);
 +   priv-irq = -EINVAL;


 And just initialize to zero.  (it's allocated by kzalloc already right?
 So that shouldn't be necessary.)

 diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
 index 41f799f..1838703 100644
 --- a/include/drm/i2c/tda998x.h
 +++ b/include/drm/i2c/tda998x.h
 @@ -20,4 +20,8 @@ struct tda998x_encoder_params {
 int swap_f, mirr_f;
   };

 +struct tda998x_platform_data {
 +   int int_gpio;
 +};
 +


 Should be combined with tda998x_encoder_params - the platform data is
 really supposed to be passed to set_config - see this in
 drm_encoder_slave.c:

   * If @info-platform_data is non-NULL it will be used as the initial
   * slave config.
 ...
  err = encoder_drv-encoder_init(client, dev, encoder);
  if (err)
  goto fail_unregister;

  if (info-platform_data)
  encoder-slave_funcs-set_config(encoder-base,
   info-platform_data);

 So any platform data set will be passed into the set_config function...


 Sure, I'll put that into params. Will rebase my v1 PATCH on v3.10-rc1
 and that will create tda998x.h.

 But actually, this all raises the ultimate how to deal with DT in drm
 question: Currently, drm i2c slave encoders are registered within drm
 API which doesn't work well with DT nodes for those encoders.

 DT i2c adapters will register an i2c client and drm will try again in
 drm_i2c_encoder_init. Registering the i2c client again will cause it
 to fail because the i2c address is busy.

 Now, in drm_i2c_encoder_init we have access to the board_info struct
 that already offers .of_node which we could exploit here to not
 register another i2c client but try to get that already registered
 client or fall back to current behavior if NULL. of_node then could
 be set by the master encoder from e.g.
 marvell,slave-encoder = tda998x;.

 Or (and that is what I'd prefer) make use of the always empty i2c
 slave encoder _probe() as for any other i2c client. And hook up drm
 to a probed i2c client. That will also allow for the i2c client
 provide functions for other APIs, e.g. alsa.

 I am willing to dig into this, but would like to have a general
 opinion of David, Rob, and you.

I thing we probably want to re-visit the current way the i2c encoder
slave stuff works, to make for easier support of other sorts of
encoders, and possibly a starting point for shared panel drivers.
I've kinda been waiting to see how the common display/panel framework
stuff plays out, it's also possible that this would replace the i2c
encoder slave stuff.

BR,
-R

 Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC] drm: i2c: add irq handler for tda998x slave encoder

[Russell King - ARM Linux]

On Sun, May 19, 2013 at 06:49:22PM +0200, Sebastian Hesselbarth wrote:
> This adds an irq handler for HPD to the tda998x slave encoder driver
> to trigger HPD change instead of polling. The gpio connected to int
> pin of tda998x is passed through platform_data of the i2c client. As
> HPD will ultimately cause EDID read and that will raise an
> EDID_READ_DONE interrupt, the irq handling is done threaded with a
> workqueue to notify drm backend of HPD events.
> 
> Signed-off-by: Sebastian Hesselbarth 

Okay, I think I get this..  Some comments:

> +static irqreturn_t tda998x_irq_thread(int irq, void *data)
> +{
> + struct drm_encoder *encoder = data;
> + struct tda998x_priv *priv;
> + uint8_t sta, cec, hdmi, lev;
> +
> + if (!encoder)
> + return IRQ_HANDLED;

This won't ever be NULL.  The IRQ layer stores the pointer you passed
in request_threaded_irq() and that pointer will continue to point at
that memory until the IRQ is freed.  The only way it could be NULL is
if you register using a NULL pointer.

...
> + if (priv->irq < 0) {
> + for (i = 100; i > 0; i--) {
> + uint8_t val = reg_read(encoder, REG_INT_FLAGS_2);

IRQ 0 is the cross-arch "no interrupt" number.  So just use !priv->irq
here and encourage anyone who uses -1 or NO_IRQ to fix their stuff. :)

> + struct tda998x_priv *priv = to_tda998x_priv(encoder);
> +
> + /* announce polling if no irq is available */
> + if (priv->irq < 0)

Same here.

> @@ -1122,7 +1197,9 @@ tda998x_encoder_init(struct i2c_client *client,
>  
>   priv->current_page = 0;
>   priv->cec = i2c_new_dummy(client->adapter, 0x34);
> + priv->irq = -EINVAL;

And just initialize to zero.  (it's allocated by kzalloc already right?
So that shouldn't be necessary.)

> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
> index 41f799f..1838703 100644
> --- a/include/drm/i2c/tda998x.h
> +++ b/include/drm/i2c/tda998x.h
> @@ -20,4 +20,8 @@ struct tda998x_encoder_params {
>   int swap_f, mirr_f;
>  };
>  
> +struct tda998x_platform_data {
> + int int_gpio;
> +};
> +

Should be combined with tda998x_encoder_params - the platform data is
really supposed to be passed to set_config - see this in drm_encoder_slave.c:

 * If @info->platform_data is non-NULL it will be used as the initial
 * slave config.
...
err = encoder_drv->encoder_init(client, dev, encoder);
if (err)
goto fail_unregister;

if (info->platform_data)
encoder->slave_funcs->set_config(>base,
 info->platform_data);

So any platform data set will be passed into the set_config function...

[RFC] drm: i2c: add irq handler for tda998x slave encoder

[Sebastian Hesselbarth]

This adds an irq handler for HPD to the tda998x slave encoder driver
to trigger HPD change instead of polling. The gpio connected to int
pin of tda998x is passed through platform_data of the i2c client. As
HPD will ultimately cause EDID read and that will raise an
EDID_READ_DONE interrupt, the irq handling is done threaded with a
workqueue to notify drm backend of HPD events.

Signed-off-by: Sebastian Hesselbarth 
---
Cc: David Airlie 
Cc: Russell King 
Cc: Rob Clark 
Cc: Daniel Vetter
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org

This patch currently is based on top of Russell King's Dove DRM driver.
I only have DT (plus the patches to get the above running) I post it as
an RFC first. To rebase it on top of v3.10-rc1, I would just have to
create include/drm/i2c/tda998x.h without rmk's initial include.

@Russell: TDA19988 int line on Cubox is on mpp27.

Sebastian
---
 drivers/gpu/drm/i2c/tda998x_drv.c |  134 +
 include/drm/i2c/tda998x.h |4 ++
 2 files changed, 125 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 8ffb844..d71b9d8 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -16,8 +16,10 @@
  */


-
+#include 
 #include 
+#include 
+#include 

 #include 
 #include 
@@ -29,6 +31,7 @@

 struct tda998x_priv {
struct i2c_client *cec;
+   struct drm_encoder *encoder;
uint16_t rev;
uint8_t current_page;
int dpms;
@@ -37,6 +40,10 @@ struct tda998x_priv {
u8 vip_cntrl_1;
u8 vip_cntrl_2;
struct tda998x_encoder_params params;
+   wait_queue_head_t wq_edid;
+   bool wq_edid_done;
+   struct work_struct work;
+   int irq;
 };

 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)->slave_priv)
@@ -285,14 +292,19 @@ struct tda998x_priv {

 /* CEC registers: (not paged)
  */
+#define REG_CEC_INTSTATUS 0xee/* read */
+# define CEC_INTSTATUS_HDMI   (1 << 1)
+# define CEC_INTSTATUS_CEC(1 << 0)
 #define REG_CEC_FRO_IM_CLK_CTRL   0xfb/* read/write */
 # define CEC_FRO_IM_CLK_CTRL_GHOST_DIS (1 << 7)
 # define CEC_FRO_IM_CLK_CTRL_ENA_OTP   (1 << 6)
 # define CEC_FRO_IM_CLK_CTRL_IMCLK_SEL (1 << 1)
 # define CEC_FRO_IM_CLK_CTRL_FRO_DIV   (1 << 0)
+#define REG_CEC_RXSHPDINTENA  0xfc/* read/write */
+#define REG_CEC_RXSHPDINT 0xfd/* read */
 #define REG_CEC_RXSHPDLEV 0xfe/* read */
-# define CEC_RXSHPDLEV_RXSENS (1 << 0)
-# define CEC_RXSHPDLEV_HPD(1 << 1)
+# define CEC_RXSHPD_RXSENS(1 << 0)
+# define CEC_RXSHPD_HPD   (1 << 1)

 #define REG_CEC_ENAMODS   0xff/* read/write */
 # define CEC_ENAMODS_DIS_FRO  (1 << 6)
@@ -666,6 +678,54 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, 
void *params)
tda998x_configure_audio(encoder, p);
 }

+static irqreturn_t tda998x_irq_thread(int irq, void *data)
+{
+   struct drm_encoder *encoder = data;
+   struct tda998x_priv *priv;
+   uint8_t sta, cec, hdmi, lev;
+
+   if (!encoder)
+   return IRQ_HANDLED;
+
+   priv = to_tda998x_priv(encoder);
+   if (!priv)
+   return IRQ_HANDLED;
+
+   sta  = cec_read(encoder, REG_CEC_INTSTATUS);
+   cec  = cec_read(encoder, REG_CEC_RXSHPDINT);
+   lev  = cec_read(encoder, REG_CEC_RXSHPDLEV);
+   hdmi = reg_read(encoder, REG_INT_FLAGS_2);
+
+   /* clear all interrupts */
+   cec_write(encoder, REG_CEC_RXSHPDINTENA, 0x00);
+
+   if (sta & CEC_INTSTATUS_HDMI) {
+   /* schedule EDID read on active HPD */
+   if ((cec & CEC_RXSHPD_HPD) && (lev & CEC_RXSHPD_HPD))
+   schedule_work(>work);
+
+   /* wake up thread waiting on EDID read done */
+   if (hdmi & INT_FLAGS_2_EDID_BLK_RD) {
+   priv->wq_edid_done = true;
+   wake_up(>wq_edid);
+   }
+   };
+
+   /* re-enable HPD interrupts */
+   cec_write(encoder, REG_CEC_RXSHPDINTENA, CEC_RXSHPD_HPD);
+
+   return IRQ_HANDLED;
+}
+
+static void tda998x_hpd_worker(struct work_struct *work)
+{
+   struct tda998x_priv *priv =
+   container_of(work, struct tda998x_priv, work);
+
+   if (priv->encoder && priv->encoder->dev)
+   drm_helper_hpd_irq_event(priv->encoder->dev);
+}
+
 static void
 tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
@@ -878,19 +938,17 @@ tda998x_encoder_detect(struct drm_encoder *encoder,
  struct drm_connector *connector)
 {
uint8_t val = cec_read(encoder, REG_CEC_RXSHPDLEV);
-   return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
+   return (val & CEC_RXSHPD_HPD) ?