Re: [PATCH v6] Add tw9910 driver

[morimoto.kuninori@xxxxxxxxxxx]

Dear Guennadi

Thank you for checking patch.

> Ok, let's do it this way. We take this version as a basis, but I only 
> commit it after I get an incremental improvement patch from you:

v7 patch ?


> > +static int tw9910_stop_capture(struct soc_camera_device *icd)
> > +{
> > +	struct tw9910_priv *priv = container_of(icd, struct tw9910_priv, icd);
> > +
> > +	priv->scale = NULL;
> > +	icd->vdev->current_norm = V4L2_STD_NTSC;
> > +
> > +	tw9910_reset(priv->client);
> 
> I think, you should leave your driver and the chip configured, so, both of 
> these should go.

sorry. I can not understand about this.

Do you say that it is better ?

--------------------
static int tw9910_stop_capture(struct soc_camera_device *icd)
{
	struct tw9910_priv *priv = container_of(icd, struct tw9910_priv, icd);
	tw9910_reset(priv->client);
        return 0;
}
--------------------

> > +static unsigned long tw9910_query_bus_param(struct soc_camera_device *icd)
> > +{
> > +	struct tw9910_priv *priv = container_of(icd, struct tw9910_priv, icd);
> > +	struct soc_camera_link *icl = priv->client->dev.platform_data;
> > +	unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
> > +		SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
> > +		SOCAM_DATA_ACTIVE_HIGH | priv->info->buswidth;
> > +
> > +	return soc_camera_apply_sensor_flags(icl, flags);
> > +}

In current git (2008-12-19),
${LINUX}/include/media/soc_camera.h :: soc_camera_bus_param_compatible
doesn't check SOCAM_DATA_ACTIVE_XXX.
So I think SOCAM_DATA_ACTIVE_HIGH is not needed here.
But should I add it ? or not ?


> This is wrong too. According to the API TRY_FMT may be called at any time 
> (also during a running streaming) and should _not_ change driver's state. 
> And as you can see in soc_camera.c::soc_camera_s_fmt_vid_cap() we're not 
> holding the mutex while calling soc_camera_try_fmt_vid_cap(), so, you have 
> no guarantee in your set_fmt, that your try_fmt was last called from 
> soc_camera_s_fmt_vid_cap() or even worse that it's not being called 
> concurrently. Therefore, you have to call tw9910_select_norm() once more 
> in your set_fmt, 

OK. I could understand.

> and in try_fmt you only verify if a suitable norm can be 
> found and set pix->height and pix->width accordingly.

So I think set_fmt and try_fmt will be like this.
It's OK ?

--------------------
static const struct tw9910_scale_ctrl*
tw9910_select_norm(struct soc_camera_device *icd, struct v4l2_rect *rect)
{
...
}

static int tw9910_set_fmt( xxx )
{
        ...

	/*
	 * select suitable norm
	 */
	priv->scale = tw9910_select_norm(icd, rect);
	if (!priv->scale)
		return ret;

	....
}

static int tw9910_try_fmt( xxx )
{
	struct v4l2_rect rect;
        ...

        /*
         * check pix->field is ANY or INTERLACED
         */
         ....

	/*
	 * select suitable norm
	 */
	rect.width  = pix->width;
	rect.height = pix->height;

	scale = tw9910_select_norm(icd, &rect);
	if (!scale) 
		return -EINVAL;

	pix->width  = scale->width;
	pix->height = scale->height;

	return 0;
}
--------------------


Best regards
--
Kuninori Morimoto
 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@xxxxxxxxxx?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list