Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1210673 > unrolled thread
| Started by | Laurent Pinchart <laurent.pinchart@ideasonboard.com> |
|---|---|
| First post | 2015-08-20 20:40 +0200 |
| Last post | 2015-08-21 05:20 +0200 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v2 3/3] media: atmel-isi: add sanity check for supported formats in set_fmt() Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2015-08-20 20:40 +0200
Re: [PATCH v2 3/3] media: atmel-isi: add sanity check for supported formats in set_fmt() Josh Wu <josh.wu@atmel.com> - 2015-08-21 05:20 +0200
| From | Laurent Pinchart <laurent.pinchart@ideasonboard.com> |
|---|---|
| Date | 2015-08-20 20:40 +0200 |
| Subject | Re: [PATCH v2 3/3] media: atmel-isi: add sanity check for supported formats in set_fmt() |
| Message-ID | <pZCUN-4Dv-13@gated-at.bofh.it> |
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 <josh.wu@atmel.com>
> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Josh Wu <josh.wu@atmel.com> |
|---|---|
| Date | 2015-08-21 05:20 +0200 |
| Subject | Re: [PATCH v2 3/3] media: atmel-isi: add sanity check for supported formats in set_fmt() |
| Message-ID | <pZL21-880-5@gated-at.bofh.it> |
| In reply to | #1210673 |
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 <josh.wu@atmel.com>
>> ---
>>
>> 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web