Em Wed, 18 Feb 2015 22:48:04 +0000
"Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx> escreveu:
> Hi Mauro,
>
> Thanks for the patch.
Thanks for the review.
>
> On Fri, Feb 13, 2015 at 10:57 PM, Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxxxxxxx> wrote:
> > Instead of keeping the media controller entity not initialized,
> > fill it and create the pads for cx25840.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
> >
> > diff --git a/drivers/media/i2c/cx25840/cx25840-core.c b/drivers/media/i2c/cx25840/cx25840-core.c
> > index 573e08826b9b..bdb5bb6b58da 100644
> > --- a/drivers/media/i2c/cx25840/cx25840-core.c
> > +++ b/drivers/media/i2c/cx25840/cx25840-core.c
> > @@ -5137,6 +5137,9 @@ static int cx25840_probe(struct i2c_client *client,
> > int default_volume;
> > u32 id;
> > u16 device_id;
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > + int ret;
> > +#endif
> >
> > /* Check if the adapter supports the needed features */
> > if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > @@ -5178,6 +5181,21 @@ static int cx25840_probe(struct i2c_client *client,
> >
> > sd = &state->sd;
> > v4l2_i2c_subdev_init(sd, client, &cx25840_ops);
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > + /* TODO: need to represent analog inputs too */
> > + state->pads[0].flags = MEDIA_PAD_FL_SINK; /* Tuner or input */
> > + state->pads[1].flags = MEDIA_PAD_FL_SOURCE; /* Video */
> > + state->pads[2].flags = MEDIA_PAD_FL_SOURCE; /* VBI */
> Macros for 0,1,2 would make it more readable.
I was in doubt, on weather use a macro or not for it. I ended by
deciding to not use because the code shouldn't assume a particular order
for the pads. Also, I'm not sure if is there a way to "taint" a PAD for
VBI or Video, or if it is worth or not do do it.
So, the comments there are more a reminder than anything else.
>
> > + sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_DECODER;
> > +
> > + ret = media_entity_init(&sd->entity, ARRAY_SIZE(state->pads),
> > + state->pads, 0);
> > + if (ret < 0) {
> > + v4l_info(client, "failed to initialize media entity!\n");
> > + kfree(state);
> not needed as state is allocated using devm_kzalloc()
>
> > + return -ENODEV;
> return ret instead ?
Yeah, both comments make sense. I'll write a patch changing it.
>
> > + }
> > +#endif
> >
> > switch (id) {
> > case CX23885_AV:
> > diff --git a/drivers/media/i2c/cx25840/cx25840-core.h b/drivers/media/i2c/cx25840/cx25840-core.h
> > index 37bc04217c44..17b409f55445 100644
> > --- a/drivers/media/i2c/cx25840/cx25840-core.h
> > +++ b/drivers/media/i2c/cx25840/cx25840-core.h
> > @@ -64,6 +64,9 @@ struct cx25840_state {
> > wait_queue_head_t fw_wait; /* wake up when the fw load is finished */
> > struct work_struct fw_work; /* work entry for fw load */
> > struct cx25840_ir_state *ir_state;
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > + struct media_pad pads[3];
> Macro for 3 ?
Not much sense, as this is used only here. Ok, if we're adding an
enum (or defines) for the pads, then it makes sense to have an alias
for the number of pads.
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
