[PATCH 1/2] media: atmel-isi: setup the ISI_CFG2 register directly

[PATCH 1/2] media: atmel-isi: setup the ISI_CFG2 register directly

[Josh Wu]

In the function configure_geometry(), we will setup the ISI CFG2
according to the sensor output format.

It make no sense to just read back the CFG2 register and just set part
of it.

So just set up this register directly makes things simpler.
Currently only support YUV format from camera sensor.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 9070172..8bc40ca 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -105,24 +105,25 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
 static int configure_geometry(struct atmel_isi *isi, u32 width,
 			u32 height, u32 code)
 {
-	u32 cfg2, cr;
+	u32 cfg2;
 
+	/* According to sensor's output format to set cfg2 */
 	switch (code) {
 	/* YUV, including grey */
 	case MEDIA_BUS_FMT_Y8_1X8:
-		cr = ISI_CFG2_GRAYSCALE;
+		cfg2 = ISI_CFG2_GRAYSCALE;
 		break;
 	case MEDIA_BUS_FMT_VYUY8_2X8:
-		cr = ISI_CFG2_YCC_SWAP_MODE_3;
+		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
 		break;
 	case MEDIA_BUS_FMT_UYVY8_2X8:
-		cr = ISI_CFG2_YCC_SWAP_MODE_2;
+		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2;
 		break;
 	case MEDIA_BUS_FMT_YVYU8_2X8:
-		cr = ISI_CFG2_YCC_SWAP_MODE_1;
+		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1;
 		break;
 	case MEDIA_BUS_FMT_YUYV8_2X8:
-		cr = ISI_CFG2_YCC_SWAP_DEFAULT;
+		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
 		break;
 	/* RGB, TODO */
 	default:
@@ -130,17 +131,10 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
 	}
 
 	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
-	cfg2 = isi_readl(isi, ISI_CFG2);
-	/* Set YCC swap mode */
-	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
-	cfg2 |= cr;
 	/* Set width */
-	cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK);
 	cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
 			ISI_CFG2_IM_HSIZE_MASK;
 	/* Set height */
-	cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK);
 	cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
 			& ISI_CFG2_IM_VSIZE_MASK;
 	isi_writel(isi, ISI_CFG2, cfg2);
-- 
1.9.1

[PATCH 2/2] media: atmel-isi: move configure_geometry() to start_streaming()

[Josh Wu]

As in set_fmt() function we only need to know which format is been set,
we don't need to access the ISI hardware in this moment.

So move the configure_geometry(), which access the ISI hardware, to
start_streaming() will make code more consistent and simpler.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 8bc40ca..b01086d 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -390,6 +390,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	/* Disable all interrupts */
 	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
 
+	ret = configure_geometry(isi, icd->user_width, icd->user_height,
+				icd->current_fmt->code);
+	if (ret < 0)
+		return ret;
+
 	spin_lock_irq(&isi->lock);
 	/* Clear any pending interrupt */
 	isi_readl(isi, ISI_STATUS);
@@ -477,8 +482,6 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
 static int isi_camera_set_fmt(struct soc_camera_device *icd,
 			      struct v4l2_format *f)
 {
-	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
-	struct atmel_isi *isi = ici->priv;
 	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
 	const struct soc_camera_format_xlate *xlate;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
@@ -511,16 +514,6 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
 	if (mf->code != xlate->code)
 		return -EINVAL;
 
-	/* Enable PM and peripheral clock before operate isi registers */
-	pm_runtime_get_sync(ici->v4l2_dev.dev);
-
-	ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
-
-	pm_runtime_put(ici->v4l2_dev.dev);
-
-	if (ret < 0)
-		return ret;
-
 	pix->width		= mf->width;
 	pix->height		= mf->height;
 	pix->field		= mf->field;
-- 
1.9.1

Re: [PATCH 2/2] media: atmel-isi: move configure_geometry() to start_streaming()

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Wednesday 17 June 2015 18:39:39 Josh Wu wrote:
> As in set_fmt() function we only need to know which format is been set,
> we don't need to access the ISI hardware in this moment.
> 
> So move the configure_geometry(), which access the ISI hardware, to
> start_streaming() will make code more consistent and simpler.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index 8bc40ca..b01086d
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -390,6 +390,11 @@ static int start_streaming(struct vb2_queue *vq,
> unsigned int count) /* Disable all interrupts */
>  	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> 
> +	ret = configure_geometry(isi, icd->user_width, icd->user_height,
> +				icd->current_fmt->code);

I would also make configure_geometry a void function, as the only failure case 
really can't occur.

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	if (ret < 0)
> +		return ret;
> +
>  	spin_lock_irq(&isi->lock);
>  	/* Clear any pending interrupt */
>  	isi_readl(isi, ISI_STATUS);
> @@ -477,8 +482,6 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
> static int isi_camera_set_fmt(struct soc_camera_device *icd,
>  			      struct v4l2_format *f)
>  {
> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> -	struct atmel_isi *isi = ici->priv;
>  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>  	const struct soc_camera_format_xlate *xlate;
>  	struct v4l2_pix_format *pix = &f->fmt.pix;
> @@ -511,16 +514,6 @@ static int isi_camera_set_fmt(struct soc_camera_device
> *icd, if (mf->code != xlate->code)
>  		return -EINVAL;
> 
> -	/* Enable PM and peripheral clock before operate isi registers */
> -	pm_runtime_get_sync(ici->v4l2_dev.dev);
> -
> -	ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
> -
> -	pm_runtime_put(ici->v4l2_dev.dev);
> -
> -	if (ret < 0)
> -		return ret;
> -
>  	pix->width		= mf->width;
>  	pix->height		= mf->height;
>  	pix->field		= mf->field;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: atmel-isi: move configure_geometry() to start_streaming()

[Josh Wu]

HI, Laurent

On 7/31/2015 10:37 PM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Wednesday 17 June 2015 18:39:39 Josh Wu wrote:
>> As in set_fmt() function we only need to know which format is been set,
>> we don't need to access the ISI hardware in this moment.
>>
>> So move the configure_geometry(), which access the ISI hardware, to
>> start_streaming() will make code more consistent and simpler.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 17 +++++------------
>>   1 file changed, 5 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>> b/drivers/media/platform/soc_camera/atmel-isi.c index 8bc40ca..b01086d
>> 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -390,6 +390,11 @@ static int start_streaming(struct vb2_queue *vq,
>> unsigned int count) /* Disable all interrupts */
>>   	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>>
>> +	ret = configure_geometry(isi, icd->user_width, icd->user_height,
>> +				icd->current_fmt->code);
> I would also make configure_geometry a void function, as the only failure case
> really can't occur.

I think this case can be reached if user require a RGB565 format to 
capture and sensor also support RGB565 format.
As atmel-isi driver will provide RGB565 support via the pass-through 
mode (maybe we need re-consider this part).

So that will cause the configure_geometry() returns an error since it 
found the bus format is not Y8 or YUV422.

In my opinion, we should not change configure_geometry()'s return type, 
until we add a insanity format check before we call configure_geometry() 
in future.


>
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Thanks for the review.


Best Regards,
Josh Wu
>
>> +	if (ret < 0)
>> +		return ret;
>> +
>>   	spin_lock_irq(&isi->lock);
>>   	/* Clear any pending interrupt */
>>   	isi_readl(isi, ISI_STATUS);
>> @@ -477,8 +482,6 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
>> static int isi_camera_set_fmt(struct soc_camera_device *icd,
>>   			      struct v4l2_format *f)
>>   {
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -	struct atmel_isi *isi = ici->priv;
>>   	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>>   	const struct soc_camera_format_xlate *xlate;
>>   	struct v4l2_pix_format *pix = &f->fmt.pix;
>> @@ -511,16 +514,6 @@ static int isi_camera_set_fmt(struct soc_camera_device
>> *icd, if (mf->code != xlate->code)
>>   		return -EINVAL;
>>
>> -	/* Enable PM and peripheral clock before operate isi registers */
>> -	pm_runtime_get_sync(ici->v4l2_dev.dev);
>> -
>> -	ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
>> -
>> -	pm_runtime_put(ici->v4l2_dev.dev);
>> -
>> -	if (ret < 0)
>> -		return ret;
>> -
>>   	pix->width		= mf->width;
>>   	pix->height		= mf->height;
>>   	pix->field		= mf->field;

Re: [PATCH 2/2] media: atmel-isi: move configure_geometry() to start_streaming()

[Laurent Pinchart]

Hi Josh,

On Monday 03 August 2015 11:56:01 Josh Wu wrote:
> On 7/31/2015 10:37 PM, Laurent Pinchart wrote:
> > On Wednesday 17 June 2015 18:39:39 Josh Wu wrote:
> >> As in set_fmt() function we only need to know which format is been set,
> >> we don't need to access the ISI hardware in this moment.
> >> 
> >> So move the configure_geometry(), which access the ISI hardware, to
> >> start_streaming() will make code more consistent and simpler.
> >> 
> >> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> >> ---
> >> 
> >>   drivers/media/platform/soc_camera/atmel-isi.c | 17 +++++------------
> >>   1 file changed, 5 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> >> b/drivers/media/platform/soc_camera/atmel-isi.c index 8bc40ca..b01086d
> >> 100644
> >> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> >> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> >> @@ -390,6 +390,11 @@ static int start_streaming(struct vb2_queue *vq,
> >> unsigned int count) /* Disable all interrupts */
> >> 
> >>   	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> >> 
> >> +	ret = configure_geometry(isi, icd->user_width, icd->user_height,
> >> +				icd->current_fmt->code);
> > 
> > I would also make configure_geometry a void function, as the only failure
> > case really can't occur.
> 
> I think this case can be reached if user require a RGB565 format to
> capture and sensor also support RGB565 format.
> As atmel-isi driver will provide RGB565 support via the pass-through
> mode (maybe we need re-consider this part).
> 
> So that will cause the configure_geometry() returns an error since it
> found the bus format is not Y8 or YUV422.
> 
> In my opinion, we should not change configure_geometry()'s return type,
> until we add a insanity format check before we call configure_geometry()
> in future.

It will really confuse the user if S_FMT accepts a format but STREAMON fails 
due to the format being unsupported. Could that be fixed by defaulting to a 
known supported format in S_FMT if the requested format isn't support ? You 
could then remove the error check in configure_geometry().

> > Apart from that,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks for the review.
> 
> Best Regards,
> Josh Wu
> 
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> 
> >>   	spin_lock_irq(&isi->lock);
> >>   	/* Clear any pending interrupt */
> >>   	isi_readl(isi, ISI_STATUS);
> >> 
> >> @@ -477,8 +482,6 @@ static int isi_camera_init_videobuf(struct vb2_queue
> >> *q, static int isi_camera_set_fmt(struct soc_camera_device *icd,
> >> 
> >>   			      struct v4l2_format *f)
> >>   
> >>   {
> >> 
> >> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> >> -	struct atmel_isi *isi = ici->priv;
> >> 
> >>   	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> >>   	const struct soc_camera_format_xlate *xlate;
> >>   	struct v4l2_pix_format *pix = &f->fmt.pix;
> >> 
> >> @@ -511,16 +514,6 @@ static int isi_camera_set_fmt(struct
> >> soc_camera_device
> >> *icd, if (mf->code != xlate->code)
> >> 
> >>   		return -EINVAL;
> >> 
> >> -	/* Enable PM and peripheral clock before operate isi registers */
> >> -	pm_runtime_get_sync(ici->v4l2_dev.dev);
> >> -
> >> -	ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
> >> -
> >> -	pm_runtime_put(ici->v4l2_dev.dev);
> >> -
> >> -	if (ret < 0)
> >> -		return ret;
> >> -
> >> 
> >>   	pix->width		= mf->width;
> >>   	pix->height		= mf->height;
> >>   	pix->field		= mf->field;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: atmel-isi: move configure_geometry() to start_streaming()

[Josh Wu]

Hi, Laurent

On 8/3/2015 9:27 PM, Laurent Pinchart wrote:
> Hi Josh,
>
> On Monday 03 August 2015 11:56:01 Josh Wu wrote:
>> On 7/31/2015 10:37 PM, Laurent Pinchart wrote:
>>> On Wednesday 17 June 2015 18:39:39 Josh Wu wrote:
>>>> As in set_fmt() function we only need to know which format is been set,
>>>> we don't need to access the ISI hardware in this moment.
>>>>
>>>> So move the configure_geometry(), which access the ISI hardware, to
>>>> start_streaming() will make code more consistent and simpler.
>>>>
>>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>>> ---
>>>>
>>>>    drivers/media/platform/soc_camera/atmel-isi.c | 17 +++++------------
>>>>    1 file changed, 5 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> b/drivers/media/platform/soc_camera/atmel-isi.c index 8bc40ca..b01086d
>>>> 100644
>>>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>>>> @@ -390,6 +390,11 @@ static int start_streaming(struct vb2_queue *vq,
>>>> unsigned int count) /* Disable all interrupts */
>>>>
>>>>    	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>>>>
>>>> +	ret = configure_geometry(isi, icd->user_width, icd->user_height,
>>>> +				icd->current_fmt->code);
>>> I would also make configure_geometry a void function, as the only failure
>>> case really can't occur.
>> I think this case can be reached if user require a RGB565 format to
>> capture and sensor also support RGB565 format.
>> As atmel-isi driver will provide RGB565 support via the pass-through
>> mode (maybe we need re-consider this part).
>>
>> So that will cause the configure_geometry() returns an error since it
>> found the bus format is not Y8 or YUV422.
>>
>> In my opinion, we should not change configure_geometry()'s return type,
>> until we add a insanity format check before we call configure_geometry()
>> in future.
> It will really confuse the user if S_FMT accepts a format but STREAMON fails
> due to the format being unsupported. Could that be fixed by defaulting to a
> known supported format in S_FMT if the requested format isn't support ?

yes, it's the right way to go.

> You
> could then remove the error check in configure_geometry().

So I will send a v2 patches, which will add one more patch to add 
insanity check on the S_FMT and remove the error check code in 
configure_geometry().

And for this patch in v2, I will add your reviewed-by tag. Is that Okay 
for you?

Best Regards,
Josh Wu

>>> Apart from that,
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Thanks for the review.
>>
>> Best Regards,
>> Josh Wu
>>
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>>
>>>>    	spin_lock_irq(&isi->lock);
>>>>    	/* Clear any pending interrupt */
>>>>    	isi_readl(isi, ISI_STATUS);
>>>>
>>>> @@ -477,8 +482,6 @@ static int isi_camera_init_videobuf(struct vb2_queue
>>>> *q, static int isi_camera_set_fmt(struct soc_camera_device *icd,
>>>>
>>>>    			      struct v4l2_format *f)
>>>>    
>>>>    {
>>>>
>>>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>>>> -	struct atmel_isi *isi = ici->priv;
>>>>
>>>>    	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>>>>    	const struct soc_camera_format_xlate *xlate;
>>>>    	struct v4l2_pix_format *pix = &f->fmt.pix;
>>>>
>>>> @@ -511,16 +514,6 @@ static int isi_camera_set_fmt(struct
>>>> soc_camera_device
>>>> *icd, if (mf->code != xlate->code)
>>>>
>>>>    		return -EINVAL;
>>>>
>>>> -	/* Enable PM and peripheral clock before operate isi registers */
>>>> -	pm_runtime_get_sync(ici->v4l2_dev.dev);
>>>> -
>>>> -	ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
>>>> -
>>>> -	pm_runtime_put(ici->v4l2_dev.dev);
>>>> -
>>>> -	if (ret < 0)
>>>> -		return ret;
>>>> -
>>>>
>>>>    	pix->width		= mf->width;
>>>>    	pix->height		= mf->height;
>>>>    	pix->field		= mf->field;

Re: [PATCH 2/2] media: atmel-isi: move configure_geometry() to start_streaming()

[Guennadi Liakhovetski]

Hi Josh,

On Wed, 17 Jun 2015, Josh Wu wrote:

> As in set_fmt() function we only need to know which format is been set,
> we don't need to access the ISI hardware in this moment.
> 
> So move the configure_geometry(), which access the ISI hardware, to
> start_streaming() will make code more consistent and simpler.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index 8bc40ca..b01086d 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -390,6 +390,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  	/* Disable all interrupts */
>  	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>  
> +	ret = configure_geometry(isi, icd->user_width, icd->user_height,
> +				icd->current_fmt->code);
> +	if (ret < 0)
> +		return ret;

No. Firstly, you'd have to pm_runtime_put() here if you fail. Secondly I 
think it's better to fail earlier - at S_FMT, than here. Not accessing the 
hardware in S_FMT is a good idea, but I'd at least do all the checking 
there. So, maybe add a "u32 cfg2_cr" field to struct atmel_isi, calculate 
it in S_FMT but only write to the hardware in start_streaming()?

Thanks
Guennadi

> +
>  	spin_lock_irq(&isi->lock);
>  	/* Clear any pending interrupt */
>  	isi_readl(isi, ISI_STATUS);
> @@ -477,8 +482,6 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
>  static int isi_camera_set_fmt(struct soc_camera_device *icd,
>  			      struct v4l2_format *f)
>  {
> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> -	struct atmel_isi *isi = ici->priv;
>  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>  	const struct soc_camera_format_xlate *xlate;
>  	struct v4l2_pix_format *pix = &f->fmt.pix;
> @@ -511,16 +514,6 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
>  	if (mf->code != xlate->code)
>  		return -EINVAL;
>  
> -	/* Enable PM and peripheral clock before operate isi registers */
> -	pm_runtime_get_sync(ici->v4l2_dev.dev);
> -
> -	ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
> -
> -	pm_runtime_put(ici->v4l2_dev.dev);
> -
> -	if (ret < 0)
> -		return ret;
> -
>  	pix->width		= mf->width;
>  	pix->height		= mf->height;
>  	pix->field		= mf->field;
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] media: atmel-isi: move configure_geometry() to start_streaming()

[Guennadi Liakhovetski]

Yep, I see the thread and updates to this patch now, please, ignore this 
mail, sorry.

Thanks
Guennadi

On Sun, 30 Aug 2015, Guennadi Liakhovetski wrote:

> Hi Josh,
> 
> On Wed, 17 Jun 2015, Josh Wu wrote:
> 
> > As in set_fmt() function we only need to know which format is been set,
> > we don't need to access the ISI hardware in this moment.
> > 
> > So move the configure_geometry(), which access the ISI hardware, to
> > start_streaming() will make code more consistent and simpler.
> > 
> > Signed-off-by: Josh Wu <josh.wu@atmel.com>
> > ---
> > 
> >  drivers/media/platform/soc_camera/atmel-isi.c | 17 +++++------------
> >  1 file changed, 5 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> > index 8bc40ca..b01086d 100644
> > --- a/drivers/media/platform/soc_camera/atmel-isi.c
> > +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> > @@ -390,6 +390,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >  	/* Disable all interrupts */
> >  	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> >  
> > +	ret = configure_geometry(isi, icd->user_width, icd->user_height,
> > +				icd->current_fmt->code);
> > +	if (ret < 0)
> > +		return ret;
> 
> No. Firstly, you'd have to pm_runtime_put() here if you fail. Secondly I 
> think it's better to fail earlier - at S_FMT, than here. Not accessing the 
> hardware in S_FMT is a good idea, but I'd at least do all the checking 
> there. So, maybe add a "u32 cfg2_cr" field to struct atmel_isi, calculate 
> it in S_FMT but only write to the hardware in start_streaming()?
> 
> Thanks
> Guennadi
> 
> > +
> >  	spin_lock_irq(&isi->lock);
> >  	/* Clear any pending interrupt */
> >  	isi_readl(isi, ISI_STATUS);
> > @@ -477,8 +482,6 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
> >  static int isi_camera_set_fmt(struct soc_camera_device *icd,
> >  			      struct v4l2_format *f)
> >  {
> > -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > -	struct atmel_isi *isi = ici->priv;
> >  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> >  	const struct soc_camera_format_xlate *xlate;
> >  	struct v4l2_pix_format *pix = &f->fmt.pix;
> > @@ -511,16 +514,6 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
> >  	if (mf->code != xlate->code)
> >  		return -EINVAL;
> >  
> > -	/* Enable PM and peripheral clock before operate isi registers */
> > -	pm_runtime_get_sync(ici->v4l2_dev.dev);
> > -
> > -	ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
> > -
> > -	pm_runtime_put(ici->v4l2_dev.dev);
> > -
> > -	if (ret < 0)
> > -		return ret;
> > -
> >  	pix->width		= mf->width;
> >  	pix->height		= mf->height;
> >  	pix->field		= mf->field;
> > -- 
> > 1.9.1
> > 
> 

Re: [PATCH 1/2] media: atmel-isi: setup the ISI_CFG2 register directly

[Josh Wu]

Hi, list

Ping..., any feedback for this series?

Best Regards,
Josh Wu

On 6/17/2015 6:39 PM, Josh Wu wrote:
> In the function configure_geometry(), we will setup the ISI CFG2
> according to the sensor output format.
>
> It make no sense to just read back the CFG2 register and just set part
> of it.
>
> So just set up this register directly makes things simpler.
> Currently only support YUV format from camera sensor.
>
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
>
>   drivers/media/platform/soc_camera/atmel-isi.c | 20 +++++++-------------
>   1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index 9070172..8bc40ca 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -105,24 +105,25 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>   static int configure_geometry(struct atmel_isi *isi, u32 width,
>   			u32 height, u32 code)
>   {
> -	u32 cfg2, cr;
> +	u32 cfg2;
>   
> +	/* According to sensor's output format to set cfg2 */
>   	switch (code) {
>   	/* YUV, including grey */
>   	case MEDIA_BUS_FMT_Y8_1X8:
> -		cr = ISI_CFG2_GRAYSCALE;
> +		cfg2 = ISI_CFG2_GRAYSCALE;
>   		break;
>   	case MEDIA_BUS_FMT_VYUY8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_MODE_3;
> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
>   		break;
>   	case MEDIA_BUS_FMT_UYVY8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_MODE_2;
> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2;
>   		break;
>   	case MEDIA_BUS_FMT_YVYU8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_MODE_1;
> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1;
>   		break;
>   	case MEDIA_BUS_FMT_YUYV8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_DEFAULT;
> +		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
>   		break;
>   	/* RGB, TODO */
>   	default:
> @@ -130,17 +131,10 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
>   	}
>   
>   	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> -
> -	cfg2 = isi_readl(isi, ISI_CFG2);
> -	/* Set YCC swap mode */
> -	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
> -	cfg2 |= cr;
>   	/* Set width */
> -	cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK);
>   	cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
>   			ISI_CFG2_IM_HSIZE_MASK;
>   	/* Set height */
> -	cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK);
>   	cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>   			& ISI_CFG2_IM_VSIZE_MASK;
>   	isi_writel(isi, ISI_CFG2, cfg2);

Re: [PATCH 1/2] media: atmel-isi: setup the ISI_CFG2 register directly

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Wednesday 17 June 2015 18:39:38 Josh Wu wrote:
> In the function configure_geometry(), we will setup the ISI CFG2
> according to the sensor output format.
> 
> It make no sense to just read back the CFG2 register and just set part
> of it.
> 
> So just set up this register directly makes things simpler.
> Currently only support YUV format from camera sensor.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>

The default value of the register is all 0 so this should be good.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index 9070172..8bc40ca
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -105,24 +105,25 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>  static int configure_geometry(struct atmel_isi *isi, u32 width,
>  			u32 height, u32 code)
>  {
> -	u32 cfg2, cr;
> +	u32 cfg2;
> 
> +	/* According to sensor's output format to set cfg2 */
>  	switch (code) {
>  	/* YUV, including grey */
>  	case MEDIA_BUS_FMT_Y8_1X8:
> -		cr = ISI_CFG2_GRAYSCALE;
> +		cfg2 = ISI_CFG2_GRAYSCALE;
>  		break;
>  	case MEDIA_BUS_FMT_VYUY8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_MODE_3;
> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
>  		break;
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_MODE_2;
> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2;
>  		break;
>  	case MEDIA_BUS_FMT_YVYU8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_MODE_1;
> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1;
>  		break;
>  	case MEDIA_BUS_FMT_YUYV8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_DEFAULT;
> +		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
>  		break;
>  	/* RGB, TODO */
>  	default:
> @@ -130,17 +131,10 @@ static int configure_geometry(struct atmel_isi *isi,
> u32 width, }
> 
>  	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> -
> -	cfg2 = isi_readl(isi, ISI_CFG2);
> -	/* Set YCC swap mode */
> -	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
> -	cfg2 |= cr;
>  	/* Set width */
> -	cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK);
>  	cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
>  			ISI_CFG2_IM_HSIZE_MASK;
>  	/* Set height */
> -	cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK);
>  	cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>  			& ISI_CFG2_IM_VSIZE_MASK;
>  	isi_writel(isi, ISI_CFG2, cfg2);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: atmel-isi: setup the ISI_CFG2 register directly

[Guennadi Liakhovetski]

Hi Josh,

On Wed, 17 Jun 2015, Josh Wu wrote:

> In the function configure_geometry(), we will setup the ISI CFG2
> according to the sensor output format.
> 
> It make no sense to just read back the CFG2 register and just set part
> of it.
> 
> So just set up this register directly makes things simpler.

Simpler doesn't necessarily mean better or more correct:) There are other 
fields in that register and currently the driver preserves them, with this 
patch you overwrite them with 0. 0 happens to be the reset value of that 
register. So, you should at least mention that in this patch description, 
just saying "simpler" doesn't convince me. So, at least I'd modify that, I 
can do that myself. But in general I'm not even sure why this patch is 
needed. Yes, currently those fields of that register are unused, so, we 
can assume they stay at their reset values. But firstly the hardware can 
change and at some point the reset value can change, or those other fields 
will get set indirectly by something. Or the driver will change at some 
point to support more fields of that register and then this code will have 
to be changed again. So, I'd ask you again - do you really want this 
patch? If you insist - I'll take it, but I'd add the "reset value" 
reasoning. Otherwise maybe just drop it?

Thanks
Guennadi

> Currently only support YUV format from camera sensor.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index 9070172..8bc40ca 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -105,24 +105,25 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>  static int configure_geometry(struct atmel_isi *isi, u32 width,
>  			u32 height, u32 code)
>  {
> -	u32 cfg2, cr;
> +	u32 cfg2;
>  
> +	/* According to sensor's output format to set cfg2 */
>  	switch (code) {
>  	/* YUV, including grey */
>  	case MEDIA_BUS_FMT_Y8_1X8:
> -		cr = ISI_CFG2_GRAYSCALE;
> +		cfg2 = ISI_CFG2_GRAYSCALE;
>  		break;
>  	case MEDIA_BUS_FMT_VYUY8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_MODE_3;
> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
>  		break;
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_MODE_2;
> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2;
>  		break;
>  	case MEDIA_BUS_FMT_YVYU8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_MODE_1;
> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1;
>  		break;
>  	case MEDIA_BUS_FMT_YUYV8_2X8:
> -		cr = ISI_CFG2_YCC_SWAP_DEFAULT;
> +		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
>  		break;
>  	/* RGB, TODO */
>  	default:
> @@ -130,17 +131,10 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
>  	}
>  
>  	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> -
> -	cfg2 = isi_readl(isi, ISI_CFG2);
> -	/* Set YCC swap mode */
> -	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
> -	cfg2 |= cr;
>  	/* Set width */
> -	cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK);
>  	cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
>  			ISI_CFG2_IM_HSIZE_MASK;
>  	/* Set height */
> -	cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK);
>  	cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>  			& ISI_CFG2_IM_VSIZE_MASK;
>  	isi_writel(isi, ISI_CFG2, cfg2);
> -- 
> 1.9.1
> 

Re: [PATCH 1/2] media: atmel-isi: setup the ISI_CFG2 register directly

[Josh Wu]

Hi, Guennadi

Thanks for the review.

On 8/30/2015 4:48 PM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Wed, 17 Jun 2015, Josh Wu wrote:
>
>> In the function configure_geometry(), we will setup the ISI CFG2
>> according to the sensor output format.
>>
>> It make no sense to just read back the CFG2 register and just set part
>> of it.
>>
>> So just set up this register directly makes things simpler.
> Simpler doesn't necessarily mean better or more correct:) There are other
> fields in that register and currently the driver preserves them, with this
> patch you overwrite them with 0. 0 happens to be the reset value of that
> register. So, you should at least mention that in this patch description,
> just saying "simpler" doesn't convince me.

Correct, I should mention that the reset value (0) of cfg2 means the YUV 
mode in the commit message.
To use YUV mode we need to clear COL_SPACE (bit 15 of CFG2) and since 
the reset value is 0, so in the code, I didn't need do anything.

> So, at least I'd modify that, I
> can do that myself. But in general I'm not even sure why this patch is
> needed. Yes, currently those fields of that register are unused, so, we
> can assume they stay at their reset values. But firstly the hardware can
> change and at some point the reset value can change, or those other fields
> will get set indirectly by something. Or the driver will change at some
> point to support more fields of that register and then this code will have
> to be changed again.

I understand your concern.
maybe a better solution is explicitly set the COL_SPACE (bit 15) to 0 
for the YUV formats. like:

#define ISI_CFG2_COL_SPACE_YUV        (0 << 15)

case MEDIA_BUS_FMT_YVYU8_2X8:
     cfg2 = ISI_CFG2_COL_SPACE_YUV | ISI_CFG2_YCC_SWAP_MODE_1;
     break;

And above modifications can be sent with RGB format support patch in future.

> So, I'd ask you again - do you really want this
> patch?

yes, this patch is needed. And in future i will add the RGB format settings.

> If you insist - I'll take it, but I'd add the "reset value"
> reasoning.

That would be great, thank you very much.

Best Regards,
Josh Wu

> Otherwise maybe just drop it?
>
> Thanks
> Guennadi
>
>> Currently only support YUV format from camera sensor.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 20 +++++++-------------
>>   1 file changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index 9070172..8bc40ca 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -105,24 +105,25 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>>   static int configure_geometry(struct atmel_isi *isi, u32 width,
>>   			u32 height, u32 code)
>>   {
>> -	u32 cfg2, cr;
>> +	u32 cfg2;
>>   
>> +	/* According to sensor's output format to set cfg2 */
>>   	switch (code) {
>>   	/* YUV, including grey */
>>   	case MEDIA_BUS_FMT_Y8_1X8:
>> -		cr = ISI_CFG2_GRAYSCALE;
>> +		cfg2 = ISI_CFG2_GRAYSCALE;
>>   		break;
>>   	case MEDIA_BUS_FMT_VYUY8_2X8:
>> -		cr = ISI_CFG2_YCC_SWAP_MODE_3;
>> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
>>   		break;
>>   	case MEDIA_BUS_FMT_UYVY8_2X8:
>> -		cr = ISI_CFG2_YCC_SWAP_MODE_2;
>> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2;
>>   		break;
>>   	case MEDIA_BUS_FMT_YVYU8_2X8:
>> -		cr = ISI_CFG2_YCC_SWAP_MODE_1;
>> +		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1;
>>   		break;
>>   	case MEDIA_BUS_FMT_YUYV8_2X8:
>> -		cr = ISI_CFG2_YCC_SWAP_DEFAULT;
>> +		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
>>   		break;
>>   	/* RGB, TODO */
>>   	default:
>> @@ -130,17 +131,10 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
>>   	}
>>   
>>   	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> -
>> -	cfg2 = isi_readl(isi, ISI_CFG2);
>> -	/* Set YCC swap mode */
>> -	cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
>> -	cfg2 |= cr;
>>   	/* Set width */
>> -	cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK);
>>   	cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
>>   			ISI_CFG2_IM_HSIZE_MASK;
>>   	/* Set height */
>> -	cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK);
>>   	cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>>   			& ISI_CFG2_IM_VSIZE_MASK;
>>   	isi_writel(isi, ISI_CFG2, cfg2);
>> -- 
>> 1.9.1
>>