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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
