lkml.org 
[lkml]   [2013]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC] drm: i2c: add irq handler for tda998x slave encoder
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.hesselbarth@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...


\
 
 \ /
  Last update: 2013-05-19 23:01    [W:0.043 / U:0.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site