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]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
Changes in v2:
- add Laurent's reviewed-by tag.
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 274a6f7..0fd6bc9 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -106,24 +106,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:
@@ -131,17 +132,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]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
Changes in v2:
- Add Laurent's reviewed-by tag.
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 0fd6bc9..cb46aec 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -391,6 +391,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);
@@ -478,8 +483,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;
@@ -512,16 +515,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
After adding the format check in set_fmt(), we don't need any format check
in configure_geometry(). So make configure_geometry() as void type.
Signed-off-by: Josh Wu <[email protected]>
---
Changes in v2:
- new added patch
drivers/media/platform/soc_camera/atmel-isi.c | 39 +++++++++++++++++++++------
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index cb46aec..d0df518 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -103,17 +103,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
return readl(isi->regs + reg);
}
-static int configure_geometry(struct atmel_isi *isi, u32 width,
+static void configure_geometry(struct atmel_isi *isi, u32 width,
u32 height, u32 code)
{
u32 cfg2;
/* According to sensor's output format to set cfg2 */
switch (code) {
- /* YUV, including grey */
+ default:
+ /* Grey */
case MEDIA_BUS_FMT_Y8_1X8:
cfg2 = ISI_CFG2_GRAYSCALE;
break;
+ /* YUV */
case MEDIA_BUS_FMT_VYUY8_2X8:
cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
break;
@@ -127,8 +129,6 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
break;
/* RGB, TODO */
- default:
- return -EINVAL;
}
isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
@@ -139,8 +139,29 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
& ISI_CFG2_IM_VSIZE_MASK;
isi_writel(isi, ISI_CFG2, cfg2);
+}
- return 0;
+static bool is_supported(struct soc_camera_device *icd,
+ const struct soc_camera_format_xlate *xlate)
+{
+ bool ret = true;
+
+ switch (xlate->code) {
+ /* YUV, including grey */
+ case MEDIA_BUS_FMT_Y8_1X8:
+ case MEDIA_BUS_FMT_VYUY8_2X8:
+ case MEDIA_BUS_FMT_UYVY8_2X8:
+ case MEDIA_BUS_FMT_YVYU8_2X8:
+ case MEDIA_BUS_FMT_YUYV8_2X8:
+ break;
+ /* RGB, TODO */
+ default:
+ dev_err(icd->parent, "not supported format: %d\n",
+ xlate->code);
+ ret = false;
+ }
+
+ return ret;
}
static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -391,10 +412,8 @@ 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,
+ 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 */
@@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
if (mf->code != xlate->code)
return -EINVAL;
+ /* check with atmel-isi support format */
+ if (!is_supported(icd, xlate))
+ return -EINVAL;
+
pix->width = mf->width;
pix->height = mf->height;
pix->field = mf->field;
--
1.9.1
Hi, Laurent
Could you have time to review this patch? so that I can send a pull
request with your reviewed-by tags.
Best Regards,
Josh Wu
On 8/5/2015 11:26 AM, Josh Wu wrote:
> After adding the format check in set_fmt(), we don't need any format check
> in configure_geometry(). So make configure_geometry() as void type.
>
> Signed-off-by: Josh Wu <[email protected]>
> ---
>
> Changes in v2:
> - new added patch
>
> drivers/media/platform/soc_camera/atmel-isi.c | 39 +++++++++++++++++++++------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index cb46aec..d0df518 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -103,17 +103,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
> return readl(isi->regs + reg);
> }
>
> -static int configure_geometry(struct atmel_isi *isi, u32 width,
> +static void configure_geometry(struct atmel_isi *isi, u32 width,
> u32 height, u32 code)
> {
> u32 cfg2;
>
> /* According to sensor's output format to set cfg2 */
> switch (code) {
> - /* YUV, including grey */
> + default:
> + /* Grey */
> case MEDIA_BUS_FMT_Y8_1X8:
> cfg2 = ISI_CFG2_GRAYSCALE;
> break;
> + /* YUV */
> case MEDIA_BUS_FMT_VYUY8_2X8:
> cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
> break;
> @@ -127,8 +129,6 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
> cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
> break;
> /* RGB, TODO */
> - default:
> - return -EINVAL;
> }
>
> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> @@ -139,8 +139,29 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
> cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
> & ISI_CFG2_IM_VSIZE_MASK;
> isi_writel(isi, ISI_CFG2, cfg2);
> +}
>
> - return 0;
> +static bool is_supported(struct soc_camera_device *icd,
> + const struct soc_camera_format_xlate *xlate)
> +{
> + bool ret = true;
> +
> + switch (xlate->code) {
> + /* YUV, including grey */
> + case MEDIA_BUS_FMT_Y8_1X8:
> + case MEDIA_BUS_FMT_VYUY8_2X8:
> + case MEDIA_BUS_FMT_UYVY8_2X8:
> + case MEDIA_BUS_FMT_YVYU8_2X8:
> + case MEDIA_BUS_FMT_YUYV8_2X8:
> + break;
> + /* RGB, TODO */
> + default:
> + dev_err(icd->parent, "not supported format: %d\n",
> + xlate->code);
> + ret = false;
> + }
> +
> + return ret;
> }
>
> static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> @@ -391,10 +412,8 @@ 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,
> + 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 */
> @@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
> if (mf->code != xlate->code)
> return -EINVAL;
>
> + /* check with atmel-isi support format */
> + if (!is_supported(icd, xlate))
> + return -EINVAL;
> +
> pix->width = mf->width;
> pix->height = mf->height;
> pix->field = mf->field;
Hi Josh,
Thank you for the patch.
On Wednesday 05 August 2015 11:26:29 Josh Wu wrote:
> After adding the format check in set_fmt(), we don't need any format check
> in configure_geometry(). So make configure_geometry() as void type.
>
> Signed-off-by: Josh Wu <[email protected]>
> ---
>
> Changes in v2:
> - new added patch
>
> drivers/media/platform/soc_camera/atmel-isi.c | 39 ++++++++++++++++++------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index cb46aec..d0df518
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -103,17 +103,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
> return readl(isi->regs + reg);
> }
>
> -static int configure_geometry(struct atmel_isi *isi, u32 width,
> +static void configure_geometry(struct atmel_isi *isi, u32 width,
> u32 height, u32 code)
> {
> u32 cfg2;
>
> /* According to sensor's output format to set cfg2 */
> switch (code) {
> - /* YUV, including grey */
> + default:
> + /* Grey */
> case MEDIA_BUS_FMT_Y8_1X8:
> cfg2 = ISI_CFG2_GRAYSCALE;
> break;
> + /* YUV */
> case MEDIA_BUS_FMT_VYUY8_2X8:
> cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
> break;
> @@ -127,8 +129,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
> width, cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
> break;
> /* RGB, TODO */
> - default:
> - return -EINVAL;
> }
>
> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> @@ -139,8 +139,29 @@ static int configure_geometry(struct atmel_isi *isi,
> u32 width, cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
> & ISI_CFG2_IM_VSIZE_MASK;
> isi_writel(isi, ISI_CFG2, cfg2);
> +}
>
> - return 0;
> +static bool is_supported(struct soc_camera_device *icd,
> + const struct soc_camera_format_xlate *xlate)
> +{
> + bool ret = true;
> +
> + switch (xlate->code) {
> + /* YUV, including grey */
> + case MEDIA_BUS_FMT_Y8_1X8:
> + case MEDIA_BUS_FMT_VYUY8_2X8:
> + case MEDIA_BUS_FMT_UYVY8_2X8:
> + case MEDIA_BUS_FMT_YVYU8_2X8:
> + case MEDIA_BUS_FMT_YUYV8_2X8:
I would just return true here and false below, and remove the ret variable.
> + break;
> + /* RGB, TODO */
> + default:
> + dev_err(icd->parent, "not supported format: %d\n",
> + xlate->code);
If this can happen when userspace asks for an unsupported format I don't think
you should print an error message to the kernel log.
> + ret = false;
> + }
> +
> + return ret;
> }
>
> static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> @@ -391,10 +412,8 @@ 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,
> + 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 */
> @@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
> *icd, if (mf->code != xlate->code)
> return -EINVAL;
>
> + /* check with atmel-isi support format */
> + if (!is_supported(icd, xlate))
> + return -EINVAL;
> +
S_FMT is supposed to pick a suitable default format when the requested format
isn't supported. It shouldn't return an error.
> pix->width = mf->width;
> pix->height = mf->height;
> pix->field = mf->field;
--
Regards,
Laurent Pinchart
Hi, Laurent
Thanks for the review.
On 8/21/2015 2:30 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Wednesday 05 August 2015 11:26:29 Josh Wu wrote:
>> After adding the format check in set_fmt(), we don't need any format check
>> in configure_geometry(). So make configure_geometry() as void type.
>>
>> Signed-off-by: Josh Wu <[email protected]>
>> ---
>>
>> Changes in v2:
>> - new added patch
>>
>> drivers/media/platform/soc_camera/atmel-isi.c | 39 ++++++++++++++++++------
>> 1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>> b/drivers/media/platform/soc_camera/atmel-isi.c index cb46aec..d0df518
>> 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -103,17 +103,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>> return readl(isi->regs + reg);
>> }
>>
>> -static int configure_geometry(struct atmel_isi *isi, u32 width,
>> +static void configure_geometry(struct atmel_isi *isi, u32 width,
>> u32 height, u32 code)
>> {
>> u32 cfg2;
>>
>> /* According to sensor's output format to set cfg2 */
>> switch (code) {
>> - /* YUV, including grey */
>> + default:
>> + /* Grey */
>> case MEDIA_BUS_FMT_Y8_1X8:
>> cfg2 = ISI_CFG2_GRAYSCALE;
>> break;
>> + /* YUV */
>> case MEDIA_BUS_FMT_VYUY8_2X8:
>> cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
>> break;
>> @@ -127,8 +129,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
>> width, cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
>> break;
>> /* RGB, TODO */
>> - default:
>> - return -EINVAL;
>> }
>>
>> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> @@ -139,8 +139,29 @@ static int configure_geometry(struct atmel_isi *isi,
>> u32 width, cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>> & ISI_CFG2_IM_VSIZE_MASK;
>> isi_writel(isi, ISI_CFG2, cfg2);
>> +}
>>
>> - return 0;
>> +static bool is_supported(struct soc_camera_device *icd,
>> + const struct soc_camera_format_xlate *xlate)
>> +{
>> + bool ret = true;
>> +
>> + switch (xlate->code) {
>> + /* YUV, including grey */
>> + case MEDIA_BUS_FMT_Y8_1X8:
>> + case MEDIA_BUS_FMT_VYUY8_2X8:
>> + case MEDIA_BUS_FMT_UYVY8_2X8:
>> + case MEDIA_BUS_FMT_YVYU8_2X8:
>> + case MEDIA_BUS_FMT_YUYV8_2X8:
> I would just return true here and false below, and remove the ret variable.
Ok.
>
>> + break;
>> + /* RGB, TODO */
>> + default:
>> + dev_err(icd->parent, "not supported format: %d\n",
>> + xlate->code);
> If this can happen when userspace asks for an unsupported format I don't think
> you should print an error message to the kernel log.
ok, I will remove this.
>
>> + ret = false;
>> + }
>> +
>> + return ret;
>> }
>>
>> static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>> @@ -391,10 +412,8 @@ 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,
>> + 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 */
>> @@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
>> *icd, if (mf->code != xlate->code)
>> return -EINVAL;
>>
>> + /* check with atmel-isi support format */
>> + if (!is_supported(icd, xlate))
>> + return -EINVAL;
>> +
> S_FMT is supposed to pick a suitable default format when the requested format
> isn't supported. It shouldn't return an error.
So I will move this check to beginning of S_FMT and if format not
support, I will select a default format for it.
Best Regards,
Josh Wu
>
>> pix->width = mf->width;
>> pix->height = mf->height;
>> pix->field = mf->field;