Re: [PATCHv3 07/15] atmel-isi: remove dependency of the soc-camera framework

[Sakari Ailus]

Hi Hans,

On Fri, Mar 10, 2017 at 12:25:54PM +0100, Hans Verkuil wrote:
> Slight problem with this. If I make this change, then the of_node_put below
> changes as well:
> 
> @@ -1193,7 +1176,7 @@ static int isi_graph_init(struct atmel_isi *isi)
>  done:
> if (ret < 0) {
> v4l2_async_notifier_unregister(>notifier);
> -   of_node_put(isi->entity.node);
> +   of_node_put(isi->entity.asd.match.of.node);
> }
> 
> And I get this compiler warning:
> 
>   CC [M]  drivers/media/platform/atmel/atmel-isi.o
> drivers/media/platform/atmel/atmel-isi.c: In function ‘isi_graph_init’:
> drivers/media/platform/atmel/atmel-isi.c:1179:15: warning: passing argument 1 
> of ‘of_node_put’ discards ‘const’ qualifier from pointer target type 
> [-Wdiscarded-qualifiers]
>of_node_put(isi->entity.asd.match.of.node);
>^~~
> In file included from drivers/media/platform/atmel/atmel-isi.c:25:0:
> ./include/linux/of.h:130:20: note: expected ‘struct device_node *’ but 
> argument is of type ‘const struct device_node *’
>  static inline void of_node_put(struct device_node *node) { }
> ^~~
> 
> 
> Any suggestions? Just keep the entity.node after all?

Yeah, makes sense, I didn't come to think of that.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCHv3 07/15] atmel-isi: remove dependency of the soc-camera framework

[Hans Verkuil]

On 10/03/17 11:39, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks! It's very nice to see one more driver converted to stand-alone one!
> 
> Speaking of which --- this seems to be the second last one. The only
> remaining one is sh_mobile_ceu_camera.c!
> 
> On Mon, Mar 06, 2017 at 03:56:08PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil 
>>
>> This patch converts the atmel-isi driver from a soc-camera driver to a driver
>> that is stand-alone.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/media/i2c/soc_camera/ov2640.c |   23 +-
>>  drivers/media/platform/soc_camera/Kconfig |3 +-
>>  drivers/media/platform/soc_camera/atmel-isi.c | 1223 
>> +++--
>>  3 files changed, 735 insertions(+), 514 deletions(-)
>>



>> +static int isi_graph_parse(struct atmel_isi *isi,
>> +struct device_node *node)
> 
> Fits on a single line.
> 
>> +{
>> +struct device_node *remote;
>> +struct device_node *ep = NULL;
>> +struct device_node *next;
>> +int ret = 0;
>> +
>> +while (1) {
>> +next = of_graph_get_next_endpoint(node, ep);
>> +if (!next)
>> +break;
> 
> You could make this a while loop condition.
> 
>> +
>> +of_node_put(ep);
> 
> ep is put by of_graph_get_next_endpoint(), no need to do it manually here.
> 
>> +ep = next;
>> +
>> +remote = of_graph_get_remote_port_parent(ep);
>> +if (!remote) {
> 
> of_node_put(ep);
> 
>> +ret = -EINVAL;
>> +break;
>> +}
>> +
>> +/* Skip entities that we have already processed. */
>> +if (remote == isi->dev->of_node) {
>> +of_node_put(remote);
>> +continue;
>> +}
>> +
>> +/* Remote node to connect */
>> +if (!isi->entity.node) {
> 
> There would have to be something wrong about the DT graph for the two above
> to happen. I guess one could just return an error if something is terribly
> wrong.
> 
> I'm thinking this from the point of view of making this function generic,
> and moving it to the framework as most drivers to something very similar,
> but I'd prefer to get the fwnode patches in first.
> 
>> +isi->entity.node = remote;
> 
> You could use entity.asd.match.of.node instead, and drop the node field.

Slight problem with this. If I make this change, then the of_node_put below
changes as well:

@@ -1193,7 +1176,7 @@ static int isi_graph_init(struct atmel_isi *isi)
 done:
if (ret < 0) {
v4l2_async_notifier_unregister(>notifier);
-   of_node_put(isi->entity.node);
+   of_node_put(isi->entity.asd.match.of.node);
}

And I get this compiler warning:

  CC [M]  drivers/media/platform/atmel/atmel-isi.o
drivers/media/platform/atmel/atmel-isi.c: In function ‘isi_graph_init’:
drivers/media/platform/atmel/atmel-isi.c:1179:15: warning: passing argument 1 
of ‘of_node_put’ discards ‘const’ qualifier from pointer target type 
[-Wdiscarded-qualifiers]
   of_node_put(isi->entity.asd.match.of.node);
   ^~~
In file included from drivers/media/platform/atmel/atmel-isi.c:25:0:
./include/linux/of.h:130:20: note: expected ‘struct device_node *’ but argument 
is of type ‘const struct device_node *’
 static inline void of_node_put(struct device_node *node) { }
^~~


Any suggestions? Just keep the entity.node after all?

Regards,

Hans

> 
>> +isi->entity.asd.match_type = V4L2_ASYNC_MATCH_OF;
>> +isi->entity.asd.match.of.node = remote;
>> +ret++;
>> +}
>> +}
>> +
>> +of_node_put(ep);
>> +
>> +return ret;
>> +}

Re: [PATCHv3 07/15] atmel-isi: remove dependency of the soc-camera framework

[Hans Verkuil]

On 10/03/17 11:39, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks! It's very nice to see one more driver converted to stand-alone one!
> 
> Speaking of which --- this seems to be the second last one. The only
> remaining one is sh_mobile_ceu_camera.c!
> 
> On Mon, Mar 06, 2017 at 03:56:08PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil 
>>
>> This patch converts the atmel-isi driver from a soc-camera driver to a driver
>> that is stand-alone.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/media/i2c/soc_camera/ov2640.c |   23 +-
>>  drivers/media/platform/soc_camera/Kconfig |3 +-
>>  drivers/media/platform/soc_camera/atmel-isi.c | 1223 
>> +++--
>>  3 files changed, 735 insertions(+), 514 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c 
>> b/drivers/media/i2c/soc_camera/ov2640.c
>> index 56de18263359..b9a0069f5b33 100644
>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> 
> Should this part go to a different patch?

Oops, yes. How did this end up here?

Split this off as a separate ov2640 patch.

> 
>> @@ -794,10 +794,11 @@ static int ov2640_set_params(struct i2c_client 
>> *client, u32 *width, u32 *height,
>>  dev_dbg(>dev, "%s: Selected cfmt YUYV (YUV422)", 
>> __func__);
>>  selected_cfmt_regs = ov2640_yuyv_regs;
>>  break;
>> -default:
>>  case MEDIA_BUS_FMT_UYVY8_2X8:
>> +default:
>>  dev_dbg(>dev, "%s: Selected cfmt UYVY", __func__);
>>  selected_cfmt_regs = ov2640_uyvy_regs;
>> +break;
>>  }
>>  
>>  /* reset hardware */
>> @@ -865,17 +866,7 @@ static int ov2640_get_fmt(struct v4l2_subdev *sd,
>>  mf->width   = priv->win->width;
>>  mf->height  = priv->win->height;
>>  mf->code= priv->cfmt_code;
>> -
>> -switch (mf->code) {
>> -case MEDIA_BUS_FMT_RGB565_2X8_BE:
>> -case MEDIA_BUS_FMT_RGB565_2X8_LE:
>> -mf->colorspace = V4L2_COLORSPACE_SRGB;
>> -break;
>> -default:
>> -case MEDIA_BUS_FMT_YUYV8_2X8:
>> -case MEDIA_BUS_FMT_UYVY8_2X8:
>> -mf->colorspace = V4L2_COLORSPACE_JPEG;
>> -}
>> +mf->colorspace  = V4L2_COLORSPACE_SRGB;
>>  mf->field   = V4L2_FIELD_NONE;
>>  
>>  return 0;
>> @@ -897,17 +888,17 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
>>  ov2640_select_win(>width, >height);
>>  
>>  mf->field   = V4L2_FIELD_NONE;
>> +mf->colorspace  = V4L2_COLORSPACE_SRGB;
>>  
>>  switch (mf->code) {
>>  case MEDIA_BUS_FMT_RGB565_2X8_BE:
>>  case MEDIA_BUS_FMT_RGB565_2X8_LE:
>> -mf->colorspace = V4L2_COLORSPACE_SRGB;
>> +case MEDIA_BUS_FMT_YUYV8_2X8:
>> +case MEDIA_BUS_FMT_UYVY8_2X8:
>>  break;
>>  default:
>>  mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
>> -case MEDIA_BUS_FMT_YUYV8_2X8:
>> -case MEDIA_BUS_FMT_UYVY8_2X8:
>> -mf->colorspace = V4L2_COLORSPACE_JPEG;
>> +break;
>>  }
>>  
>>  if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> diff --git a/drivers/media/platform/soc_camera/Kconfig 
>> b/drivers/media/platform/soc_camera/Kconfig
>> index 86d74788544f..a37ec91b026e 100644
>> --- a/drivers/media/platform/soc_camera/Kconfig
>> +++ b/drivers/media/platform/soc_camera/Kconfig
>> @@ -29,9 +29,8 @@ config VIDEO_SH_MOBILE_CEU
>>  
>>  config VIDEO_ATMEL_ISI
>>  tristate "ATMEL Image Sensor Interface (ISI) support"
>> -depends on VIDEO_DEV && SOC_CAMERA
>> +depends on VIDEO_V4L2 && OF && HAS_DMA
>>  depends on ARCH_AT91 || COMPILE_TEST
>> -depends on HAS_DMA
>>  select VIDEOBUF2_DMA_CONTIG
>>  ---help---
>>This module makes the ATMEL Image Sensor Interface available
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c 
>> b/drivers/media/platform/soc_camera/atmel-isi.c
>> index 46de657c3e6d..a837b94281ef 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> ...
>> @@ -457,60 +431,83 @@ static void buffer_queue(struct vb2_buffer *vb)
>>  if (vb2_is_streaming(vb->vb2_queue))
>>  start_dma(isi, buf);
>>  }
>> -spin_unlock_irqrestore(>lock, flags);
>> +spin_unlock_irqrestore(>irqlock, flags);
>>  }
>>  
>>  static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>  {
>> -struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> -struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -struct atmel_isi *isi = ici->priv;
>> +struct atmel_isi *isi = vb2_get_drv_priv(vq);
>> +struct frame_buffer *buf, *node;
>>  int ret;
>>  
>> -pm_runtime_get_sync(ici->v4l2_dev.dev);
>> +/* Enable stream on the sub device */
>> +ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
>> +if (ret && ret != -ENOIOCTLCMD) {
>> +

Re: [PATCHv3 07/15] atmel-isi: remove dependency of the soc-camera framework

[Sakari Ailus]

Hi Hans,

Thanks! It's very nice to see one more driver converted to stand-alone one!

Speaking of which --- this seems to be the second last one. The only
remaining one is sh_mobile_ceu_camera.c!

On Mon, Mar 06, 2017 at 03:56:08PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> This patch converts the atmel-isi driver from a soc-camera driver to a driver
> that is stand-alone.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  drivers/media/i2c/soc_camera/ov2640.c |   23 +-
>  drivers/media/platform/soc_camera/Kconfig |3 +-
>  drivers/media/platform/soc_camera/atmel-isi.c | 1223 
> +++--
>  3 files changed, 735 insertions(+), 514 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c 
> b/drivers/media/i2c/soc_camera/ov2640.c
> index 56de18263359..b9a0069f5b33 100644
> --- a/drivers/media/i2c/soc_camera/ov2640.c
> +++ b/drivers/media/i2c/soc_camera/ov2640.c

Should this part go to a different patch?

> @@ -794,10 +794,11 @@ static int ov2640_set_params(struct i2c_client *client, 
> u32 *width, u32 *height,
>   dev_dbg(>dev, "%s: Selected cfmt YUYV (YUV422)", 
> __func__);
>   selected_cfmt_regs = ov2640_yuyv_regs;
>   break;
> - default:
>   case MEDIA_BUS_FMT_UYVY8_2X8:
> + default:
>   dev_dbg(>dev, "%s: Selected cfmt UYVY", __func__);
>   selected_cfmt_regs = ov2640_uyvy_regs;
> + break;
>   }
>  
>   /* reset hardware */
> @@ -865,17 +866,7 @@ static int ov2640_get_fmt(struct v4l2_subdev *sd,
>   mf->width   = priv->win->width;
>   mf->height  = priv->win->height;
>   mf->code= priv->cfmt_code;
> -
> - switch (mf->code) {
> - case MEDIA_BUS_FMT_RGB565_2X8_BE:
> - case MEDIA_BUS_FMT_RGB565_2X8_LE:
> - mf->colorspace = V4L2_COLORSPACE_SRGB;
> - break;
> - default:
> - case MEDIA_BUS_FMT_YUYV8_2X8:
> - case MEDIA_BUS_FMT_UYVY8_2X8:
> - mf->colorspace = V4L2_COLORSPACE_JPEG;
> - }
> + mf->colorspace  = V4L2_COLORSPACE_SRGB;
>   mf->field   = V4L2_FIELD_NONE;
>  
>   return 0;
> @@ -897,17 +888,17 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
>   ov2640_select_win(>width, >height);
>  
>   mf->field   = V4L2_FIELD_NONE;
> + mf->colorspace  = V4L2_COLORSPACE_SRGB;
>  
>   switch (mf->code) {
>   case MEDIA_BUS_FMT_RGB565_2X8_BE:
>   case MEDIA_BUS_FMT_RGB565_2X8_LE:
> - mf->colorspace = V4L2_COLORSPACE_SRGB;
> + case MEDIA_BUS_FMT_YUYV8_2X8:
> + case MEDIA_BUS_FMT_UYVY8_2X8:
>   break;
>   default:
>   mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
> - case MEDIA_BUS_FMT_YUYV8_2X8:
> - case MEDIA_BUS_FMT_UYVY8_2X8:
> - mf->colorspace = V4L2_COLORSPACE_JPEG;
> + break;
>   }
>  
>   if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> diff --git a/drivers/media/platform/soc_camera/Kconfig 
> b/drivers/media/platform/soc_camera/Kconfig
> index 86d74788544f..a37ec91b026e 100644
> --- a/drivers/media/platform/soc_camera/Kconfig
> +++ b/drivers/media/platform/soc_camera/Kconfig
> @@ -29,9 +29,8 @@ config VIDEO_SH_MOBILE_CEU
>  
>  config VIDEO_ATMEL_ISI
>   tristate "ATMEL Image Sensor Interface (ISI) support"
> - depends on VIDEO_DEV && SOC_CAMERA
> + depends on VIDEO_V4L2 && OF && HAS_DMA
>   depends on ARCH_AT91 || COMPILE_TEST
> - depends on HAS_DMA
>   select VIDEOBUF2_DMA_CONTIG
>   ---help---
> This module makes the ATMEL Image Sensor Interface available
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c 
> b/drivers/media/platform/soc_camera/atmel-isi.c
> index 46de657c3e6d..a837b94281ef 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
...
> @@ -457,60 +431,83 @@ static void buffer_queue(struct vb2_buffer *vb)
>   if (vb2_is_streaming(vb->vb2_queue))
>   start_dma(isi, buf);
>   }
> - spin_unlock_irqrestore(>lock, flags);
> + spin_unlock_irqrestore(>irqlock, flags);
>  }
>  
>  static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  {
> - struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> - struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> - struct atmel_isi *isi = ici->priv;
> + struct atmel_isi *isi = vb2_get_drv_priv(vq);
> + struct frame_buffer *buf, *node;
>   int ret;
>  
> - pm_runtime_get_sync(ici->v4l2_dev.dev);
> + /* Enable stream on the sub device */
> + ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
> + if (ret && ret != -ENOIOCTLCMD) {
> + v4l2_err(>v4l2_dev, "stream on failed in subdev\n");

You mostly use dev_*() functions to print infromation in the driver. How
about using them consistently? There are few other cases of 

[PATCHv3 07/15] atmel-isi: remove dependency of the soc-camera framework

[Hans Verkuil]

From: Hans Verkuil 

This patch converts the atmel-isi driver from a soc-camera driver to a driver
that is stand-alone.

Signed-off-by: Hans Verkuil 
---
 drivers/media/i2c/soc_camera/ov2640.c |   23 +-
 drivers/media/platform/soc_camera/Kconfig |3 +-
 drivers/media/platform/soc_camera/atmel-isi.c | 1223 +++--
 3 files changed, 735 insertions(+), 514 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov2640.c 
b/drivers/media/i2c/soc_camera/ov2640.c
index 56de18263359..b9a0069f5b33 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -794,10 +794,11 @@ static int ov2640_set_params(struct i2c_client *client, 
u32 *width, u32 *height,
dev_dbg(>dev, "%s: Selected cfmt YUYV (YUV422)", 
__func__);
selected_cfmt_regs = ov2640_yuyv_regs;
break;
-   default:
case MEDIA_BUS_FMT_UYVY8_2X8:
+   default:
dev_dbg(>dev, "%s: Selected cfmt UYVY", __func__);
selected_cfmt_regs = ov2640_uyvy_regs;
+   break;
}
 
/* reset hardware */
@@ -865,17 +866,7 @@ static int ov2640_get_fmt(struct v4l2_subdev *sd,
mf->width   = priv->win->width;
mf->height  = priv->win->height;
mf->code= priv->cfmt_code;
-
-   switch (mf->code) {
-   case MEDIA_BUS_FMT_RGB565_2X8_BE:
-   case MEDIA_BUS_FMT_RGB565_2X8_LE:
-   mf->colorspace = V4L2_COLORSPACE_SRGB;
-   break;
-   default:
-   case MEDIA_BUS_FMT_YUYV8_2X8:
-   case MEDIA_BUS_FMT_UYVY8_2X8:
-   mf->colorspace = V4L2_COLORSPACE_JPEG;
-   }
+   mf->colorspace  = V4L2_COLORSPACE_SRGB;
mf->field   = V4L2_FIELD_NONE;
 
return 0;
@@ -897,17 +888,17 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
ov2640_select_win(>width, >height);
 
mf->field   = V4L2_FIELD_NONE;
+   mf->colorspace  = V4L2_COLORSPACE_SRGB;
 
switch (mf->code) {
case MEDIA_BUS_FMT_RGB565_2X8_BE:
case MEDIA_BUS_FMT_RGB565_2X8_LE:
-   mf->colorspace = V4L2_COLORSPACE_SRGB;
+   case MEDIA_BUS_FMT_YUYV8_2X8:
+   case MEDIA_BUS_FMT_UYVY8_2X8:
break;
default:
mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
-   case MEDIA_BUS_FMT_YUYV8_2X8:
-   case MEDIA_BUS_FMT_UYVY8_2X8:
-   mf->colorspace = V4L2_COLORSPACE_JPEG;
+   break;
}
 
if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
diff --git a/drivers/media/platform/soc_camera/Kconfig 
b/drivers/media/platform/soc_camera/Kconfig
index 86d74788544f..a37ec91b026e 100644
--- a/drivers/media/platform/soc_camera/Kconfig
+++ b/drivers/media/platform/soc_camera/Kconfig
@@ -29,9 +29,8 @@ config VIDEO_SH_MOBILE_CEU
 
 config VIDEO_ATMEL_ISI
tristate "ATMEL Image Sensor Interface (ISI) support"
-   depends on VIDEO_DEV && SOC_CAMERA
+   depends on VIDEO_V4L2 && OF && HAS_DMA
depends on ARCH_AT91 || COMPILE_TEST
-   depends on HAS_DMA
select VIDEOBUF2_DMA_CONTIG
---help---
  This module makes the ATMEL Image Sensor Interface available
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c 
b/drivers/media/platform/soc_camera/atmel-isi.c
index 46de657c3e6d..a837b94281ef 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -22,18 +22,22 @@
 #include 
 #include 
 #include 
-
-#include 
-#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
 #include 
 #include 
+#include 
 
 #include "atmel-isi.h"
 
-#define MAX_BUFFER_NUM 32
 #define MAX_SUPPORT_WIDTH  2048
 #define MAX_SUPPORT_HEIGHT 2048
-#define VID_LIMIT_BYTES(16 * 1024 * 1024)
 #define MIN_FRAME_RATE 15
 #define FRAME_INTERVAL_MILLI_SEC   (1000 / MIN_FRAME_RATE)
 
@@ -65,9 +69,37 @@ struct frame_buffer {
struct list_head list;
 };
 
+struct isi_graph_entity {
+   struct device_node *node;
+
+   struct v4l2_async_subdev asd;
+   struct v4l2_subdev *subdev;
+};
+
+/*
+ * struct isi_format - ISI media bus format information
+ * @fourcc:Fourcc code for this format
+ * @mbus_code: V4L2 media bus format code.
+ * @bpp:   Bytes per pixel (when stored in memory)
+ * @swap:  Byte swap configuration value
+ * @support:   Indicates format supported by subdev
+ * @skip:  Skip duplicate format supported by subdev
+ */
+struct isi_format {
+   u32 fourcc;
+   u32 mbus_code;
+   u8  bpp;
+   u32 swap;
+
+   boolsupport;
+   boolskip;
+};
+
+
 struct atmel_isi {
/* Protects the access of variables shared with the ISR */
-   spinlock_t  lock;
+