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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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);
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 <[email protected]>
The default value of the register is all 0 so this should be good.
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
>
> 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
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 <[email protected]>
> ---
>
> 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 <[email protected]>
> + 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
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 <[email protected]>
>> ---
>>
>> 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 <[email protected]>
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;
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 <[email protected]>
> >> ---
> >>
> >> 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 <[email protected]>
>
> 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
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 <[email protected]>
>>>> ---
>>>>
>>>> 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 <[email protected]>
>> 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;
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 <[email protected]>
> ---
>
> 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
>
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 <[email protected]>
> ---
>
> 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
>
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 <[email protected]>
> > ---
> >
> > 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
> >
>
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 <[email protected]>
>> ---
>>
>> 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
>>