Re: [PATCH v2 09/19] media: camms: Add core files

2017-07-03 Thread Sakari Ailus
Hi Todor,

On Mon, Jul 03, 2017 at 05:03:40PM +0300, Todor Tomov wrote:
> >> +  unsigned int i;
> >> +
> >> +  v4l2_of_parse_endpoint(node, );
> >> +
> >> +  csd->interface.csiphy_id = vep.base.port;
> >> +
> >> +  mipi_csi2 = _csi2;
> >> +  lncfg->clk.pos = mipi_csi2->clock_lane;
> >> +  lncfg->clk.pol = mipi_csi2->lane_polarities[0];
> >> +  lncfg->num_data = mipi_csi2->num_data_lanes;
> >> +
> >> +  lncfg->data = devm_kzalloc(dev, lncfg->num_data * sizeof(*lncfg->data),
> >> + GFP_KERNEL);
> >> +  if (!lncfg->data)
> >> +  return -ENOMEM;
> >> +
> >> +  for (i = 0; i < lncfg->num_data; i++) {
> >> +  lncfg->data[i].pos = mipi_csi2->data_lanes[i];
> >> +  lncfg->data[i].pol = mipi_csi2->lane_polarities[i + 1];
> >> +  }
> >> +
> >> +  of_property_read_u32(node, "qcom,settle-cnt", settle_cnt);
> > 
> > Isn't this something that depends on the CSI-2 bus speed, for instance?
> > Could you calculate it instead of putting it to DT?
> 
> Actually, after some digging into this, yes, I can calculate it. I can
> calculate the CSI-2 bus speed based on the sensor's output pixel clock
> and then calculate the settle time and this settle count value.
> So I already have the code to get the sensor's pixel clock using the
> standard v4l2 control V4L2_CID_PIXEL_RATE.

What we have currently in documentation on this is here:



I.e. both should be implemented. The link frequency is rather more relevant
for CSI-2 albeit you can derive one from the other in case of CSI-2. The
pixel rate documentation should probably be rather elsewhere.

> Now the question is what to do if the sensor driver doesn't support this
> control? Just return an error and refuse to work with this "limited"
> sensor driver?

If the sensor driver does not provide enough information to work with a
receiver, it's only fair not to proceed with streaming. That said, it might
be possible to manage with some sensible defaults in some cases but then again
you could have only some units working with this configuration. It'd be
much safer to require the information: not doing so hides the error and
makes it (more) difficult to debug.

...

> >> +struct camss {
> >> +  struct v4l2_device v4l2_dev;
> >> +  struct v4l2_async_notifier notifier;
> >> +  struct media_device media_dev;
> >> +  struct device *dev;
> >> +  struct csiphy_device csiphy[CAMSS_CSIPHY_NUM];
> >> +  struct csid_device csid[CAMSS_CSID_NUM];
> >> +  struct ispif_device ispif;
> >> +  struct vfe_device vfe;
> >> +  atomic_t ref_count;

If this is refcount, then you should use refcount_t instead.

-- 
Regards,

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


Re: [PATCH v2 09/19] media: camms: Add core files

2017-07-03 Thread Todor Tomov
Hello Hans,

Thank you for the review.

On 07/03/2017 02:24 PM, Hans Verkuil wrote:
> On 06/19/2017 04:48 PM, Todor Tomov wrote:
>> These files implement the platform driver code.
>>
>> Signed-off-by: Todor Tomov 
>> ---
>>   drivers/media/platform/qcom/camss-8x16/camss.c | 630 
>> +
>>   drivers/media/platform/qcom/camss-8x16/camss.h |  96 
>>   2 files changed, 726 insertions(+)
>>   create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.c
>>   create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.h
>>
>> diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c 
>> b/drivers/media/platform/qcom/camss-8x16/camss.c
>> new file mode 100644
>> index 000..a8798d1
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
>> @@ -0,0 +1,630 @@
>> +/*
>> + * camss.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - Core
>> + *
>> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>> + * Copyright (C) 2015-2016 Linaro Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> v4l2-of.h has been replaced by v4l2-fwnode.h. You need to rebase.

I'll rebase for the next version of the patchset.

> 
> Regards,
> 
> Hans

-- 
Best regards,
Todor Tomov


Re: [PATCH v2 09/19] media: camms: Add core files

2017-07-03 Thread Todor Tomov
Hi Sakari,

Thank you for the review.

On 06/29/2017 09:33 AM, Sakari Ailus wrote:
> Hi Todor,
> 
> On Mon, Jun 19, 2017 at 05:48:29PM +0300, Todor Tomov wrote:
>> These files implement the platform driver code.
>>
>> Signed-off-by: Todor Tomov 
>> ---
>>  drivers/media/platform/qcom/camss-8x16/camss.c | 630 
>> +
>>  drivers/media/platform/qcom/camss-8x16/camss.h |  96 
>>  2 files changed, 726 insertions(+)
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.c
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.h
>>
>> diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c 
>> b/drivers/media/platform/qcom/camss-8x16/camss.c
>> new file mode 100644
>> index 000..a8798d1
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
>> @@ -0,0 +1,630 @@
>> +/*
>> + * camss.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - Core
>> + *
>> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>> + * Copyright (C) 2015-2016 Linaro Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "camss.h"
>> +
>> +static struct resources csiphy_res[] = {
>> +/* CSIPHY0 */
>> +{
>> +.regulator = { NULL },
>> +.clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
>> +   "camss_ahb_clk", "csiphy0_timer_clk" },
>> +.clock_rate = { 0, 0, 0, 2 },
>> +.reg = { "csiphy0", "csiphy0_clk_mux" },
>> +.interrupt = { "csiphy0" }
>> +},
>> +
>> +/* CSIPHY1 */
>> +{
>> +.regulator = { NULL },
>> +.clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
>> +   "camss_ahb_clk", "csiphy1_timer_clk" },
>> +.clock_rate = { 0, 0, 0, 2 },
>> +.reg = { "csiphy1", "csiphy1_clk_mux" },
>> +.interrupt = { "csiphy1" }
>> +}
>> +};
>> +
>> +static struct resources csid_res[] = {
>> +/* CSID0 */
>> +{
>> +.regulator = { "vdda" },
>> +.clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
>> +   "csi0_ahb_clk", "camss_ahb_clk",
>> +   "csi0_clk", "csi0_phy_clk",
>> +   "csi0_pix_clk", "csi0_rdi_clk" },
>> +.clock_rate = { 0, 0, 0, 0, 2, 0, 0, 0 },
>> +.reg = { "csid0" },
>> +.interrupt = { "csid0" }
>> +},
>> +
>> +/* CSID1 */
>> +{
>> +.regulator = { "vdda" },
>> +.clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
>> +   "csi1_ahb_clk", "camss_ahb_clk",
>> +   "csi1_clk", "csi1_phy_clk",
>> +   "csi1_pix_clk", "csi1_rdi_clk" },
>> +.clock_rate = { 0, 0, 0, 0, 2, 0, 0, 0 },
>> +.reg = { "csid1" },
>> +.interrupt = { "csid1" }
>> +},
>> +};
>> +
>> +static struct resources_ispif ispif_res = {
>> +/* ISPIF */
>> +.clock = { "camss_top_ahb_clk", "camss_ahb_clk", "ispif_ahb_clk",
>> +   "csi0_clk", "csi0_pix_clk", "csi0_rdi_clk",
>> +   "csi1_clk", "csi1_pix_clk", "csi1_rdi_clk" },
>> +.clock_for_reset = { "camss_vfe_vfe_clk", "camss_csi_vfe_clk" },
>> +.reg = { "ispif", "csi_clk_mux" },
>> +.interrupt = "ispif"
>> +
>> +};
>> +
>> +static struct resources vfe_res = {
>> +/* VFE0 */
>> +.regulator = { NULL },
>> +.clock = { "camss_top_ahb_clk", "camss_vfe_vfe_clk",
>> +   "camss_csi_vfe_clk", "iface_clk",
>> +   "bus_clk", "camss_ahb_clk" },
>> +.clock_rate = { 0, 32000, 0, 0, 0, 0, 0, 0 },
>> +.reg = { "vfe0" },
>> +.interrupt = { "vfe0" }
>> +};
> 
> Could these be const?

Sure.

> 
>> +
>> +/*
>> + * camss_enable_clocks - Enable multiple clocks
>> + * @nclocks: Number of clocks in clock array
>> + * @clock: Clock array
>> + * @dev: Device
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +int camss_enable_clocks(int nclocks, struct clk **clock, struct device *dev)
>> +{
>> +int ret;
>> +int i;
>> +
>> +for (i = 0; i < nclocks; i++) {
>> +ret = clk_prepare_enable(clock[i]);
>> +if (ret) {
>> +dev_err(dev, "clock enable failed\n");
>> +goto error;
>> +}
>> +}

Re: [PATCH v2 09/19] media: camms: Add core files

2017-07-03 Thread Hans Verkuil

On 06/19/2017 04:48 PM, Todor Tomov wrote:

These files implement the platform driver code.

Signed-off-by: Todor Tomov 
---
  drivers/media/platform/qcom/camss-8x16/camss.c | 630 +
  drivers/media/platform/qcom/camss-8x16/camss.h |  96 
  2 files changed, 726 insertions(+)
  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.c
  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.h

diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c 
b/drivers/media/platform/qcom/camss-8x16/camss.c
new file mode 100644
index 000..a8798d1
--- /dev/null
+++ b/drivers/media/platform/qcom/camss-8x16/camss.c
@@ -0,0 +1,630 @@
+/*
+ * camss.c
+ *
+ * Qualcomm MSM Camera Subsystem - Core
+ *
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2015-2016 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 


v4l2-of.h has been replaced by v4l2-fwnode.h. You need to rebase.

Regards,

Hans


Re: [PATCH v2 09/19] media: camms: Add core files

2017-06-29 Thread Sakari Ailus
Hi Todor,

On Mon, Jun 19, 2017 at 05:48:29PM +0300, Todor Tomov wrote:
> These files implement the platform driver code.
> 
> Signed-off-by: Todor Tomov 
> ---
>  drivers/media/platform/qcom/camss-8x16/camss.c | 630 
> +
>  drivers/media/platform/qcom/camss-8x16/camss.h |  96 
>  2 files changed, 726 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.c
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.h
> 
> diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c 
> b/drivers/media/platform/qcom/camss-8x16/camss.c
> new file mode 100644
> index 000..a8798d1
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
> @@ -0,0 +1,630 @@
> +/*
> + * camss.c
> + *
> + * Qualcomm MSM Camera Subsystem - Core
> + *
> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2015-2016 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "camss.h"
> +
> +static struct resources csiphy_res[] = {
> + /* CSIPHY0 */
> + {
> + .regulator = { NULL },
> + .clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
> +"camss_ahb_clk", "csiphy0_timer_clk" },
> + .clock_rate = { 0, 0, 0, 2 },
> + .reg = { "csiphy0", "csiphy0_clk_mux" },
> + .interrupt = { "csiphy0" }
> + },
> +
> + /* CSIPHY1 */
> + {
> + .regulator = { NULL },
> + .clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
> +"camss_ahb_clk", "csiphy1_timer_clk" },
> + .clock_rate = { 0, 0, 0, 2 },
> + .reg = { "csiphy1", "csiphy1_clk_mux" },
> + .interrupt = { "csiphy1" }
> + }
> +};
> +
> +static struct resources csid_res[] = {
> + /* CSID0 */
> + {
> + .regulator = { "vdda" },
> + .clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
> +"csi0_ahb_clk", "camss_ahb_clk",
> +"csi0_clk", "csi0_phy_clk",
> +"csi0_pix_clk", "csi0_rdi_clk" },
> + .clock_rate = { 0, 0, 0, 0, 2, 0, 0, 0 },
> + .reg = { "csid0" },
> + .interrupt = { "csid0" }
> + },
> +
> + /* CSID1 */
> + {
> + .regulator = { "vdda" },
> + .clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
> +"csi1_ahb_clk", "camss_ahb_clk",
> +"csi1_clk", "csi1_phy_clk",
> +"csi1_pix_clk", "csi1_rdi_clk" },
> + .clock_rate = { 0, 0, 0, 0, 2, 0, 0, 0 },
> + .reg = { "csid1" },
> + .interrupt = { "csid1" }
> + },
> +};
> +
> +static struct resources_ispif ispif_res = {
> + /* ISPIF */
> + .clock = { "camss_top_ahb_clk", "camss_ahb_clk", "ispif_ahb_clk",
> +"csi0_clk", "csi0_pix_clk", "csi0_rdi_clk",
> +"csi1_clk", "csi1_pix_clk", "csi1_rdi_clk" },
> + .clock_for_reset = { "camss_vfe_vfe_clk", "camss_csi_vfe_clk" },
> + .reg = { "ispif", "csi_clk_mux" },
> + .interrupt = "ispif"
> +
> +};
> +
> +static struct resources vfe_res = {
> + /* VFE0 */
> + .regulator = { NULL },
> + .clock = { "camss_top_ahb_clk", "camss_vfe_vfe_clk",
> +"camss_csi_vfe_clk", "iface_clk",
> +"bus_clk", "camss_ahb_clk" },
> + .clock_rate = { 0, 32000, 0, 0, 0, 0, 0, 0 },
> + .reg = { "vfe0" },
> + .interrupt = { "vfe0" }
> +};

Could these be const?

> +
> +/*
> + * camss_enable_clocks - Enable multiple clocks
> + * @nclocks: Number of clocks in clock array
> + * @clock: Clock array
> + * @dev: Device
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +int camss_enable_clocks(int nclocks, struct clk **clock, struct device *dev)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < nclocks; i++) {
> + ret = clk_prepare_enable(clock[i]);
> + if (ret) {
> + dev_err(dev, "clock enable failed\n");
> + goto error;
> + }
> + }
> +
> + return 0;
> +
> +error:
> + for (i--; i >= 0; i--)
> + clk_disable_unprepare(clock[i]);
> +
> + return ret;
> +}
> +
> +/*
> + * 

[PATCH v2 09/19] media: camms: Add core files

2017-06-19 Thread Todor Tomov
These files implement the platform driver code.

Signed-off-by: Todor Tomov 
---
 drivers/media/platform/qcom/camss-8x16/camss.c | 630 +
 drivers/media/platform/qcom/camss-8x16/camss.h |  96 
 2 files changed, 726 insertions(+)
 create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.c
 create mode 100644 drivers/media/platform/qcom/camss-8x16/camss.h

diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c 
b/drivers/media/platform/qcom/camss-8x16/camss.c
new file mode 100644
index 000..a8798d1
--- /dev/null
+++ b/drivers/media/platform/qcom/camss-8x16/camss.c
@@ -0,0 +1,630 @@
+/*
+ * camss.c
+ *
+ * Qualcomm MSM Camera Subsystem - Core
+ *
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2015-2016 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "camss.h"
+
+static struct resources csiphy_res[] = {
+   /* CSIPHY0 */
+   {
+   .regulator = { NULL },
+   .clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
+  "camss_ahb_clk", "csiphy0_timer_clk" },
+   .clock_rate = { 0, 0, 0, 2 },
+   .reg = { "csiphy0", "csiphy0_clk_mux" },
+   .interrupt = { "csiphy0" }
+   },
+
+   /* CSIPHY1 */
+   {
+   .regulator = { NULL },
+   .clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
+  "camss_ahb_clk", "csiphy1_timer_clk" },
+   .clock_rate = { 0, 0, 0, 2 },
+   .reg = { "csiphy1", "csiphy1_clk_mux" },
+   .interrupt = { "csiphy1" }
+   }
+};
+
+static struct resources csid_res[] = {
+   /* CSID0 */
+   {
+   .regulator = { "vdda" },
+   .clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
+  "csi0_ahb_clk", "camss_ahb_clk",
+  "csi0_clk", "csi0_phy_clk",
+  "csi0_pix_clk", "csi0_rdi_clk" },
+   .clock_rate = { 0, 0, 0, 0, 2, 0, 0, 0 },
+   .reg = { "csid0" },
+   .interrupt = { "csid0" }
+   },
+
+   /* CSID1 */
+   {
+   .regulator = { "vdda" },
+   .clock = { "camss_top_ahb_clk", "ispif_ahb_clk",
+  "csi1_ahb_clk", "camss_ahb_clk",
+  "csi1_clk", "csi1_phy_clk",
+  "csi1_pix_clk", "csi1_rdi_clk" },
+   .clock_rate = { 0, 0, 0, 0, 2, 0, 0, 0 },
+   .reg = { "csid1" },
+   .interrupt = { "csid1" }
+   },
+};
+
+static struct resources_ispif ispif_res = {
+   /* ISPIF */
+   .clock = { "camss_top_ahb_clk", "camss_ahb_clk", "ispif_ahb_clk",
+  "csi0_clk", "csi0_pix_clk", "csi0_rdi_clk",
+  "csi1_clk", "csi1_pix_clk", "csi1_rdi_clk" },
+   .clock_for_reset = { "camss_vfe_vfe_clk", "camss_csi_vfe_clk" },
+   .reg = { "ispif", "csi_clk_mux" },
+   .interrupt = "ispif"
+
+};
+
+static struct resources vfe_res = {
+   /* VFE0 */
+   .regulator = { NULL },
+   .clock = { "camss_top_ahb_clk", "camss_vfe_vfe_clk",
+  "camss_csi_vfe_clk", "iface_clk",
+  "bus_clk", "camss_ahb_clk" },
+   .clock_rate = { 0, 32000, 0, 0, 0, 0, 0, 0 },
+   .reg = { "vfe0" },
+   .interrupt = { "vfe0" }
+};
+
+/*
+ * camss_enable_clocks - Enable multiple clocks
+ * @nclocks: Number of clocks in clock array
+ * @clock: Clock array
+ * @dev: Device
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+int camss_enable_clocks(int nclocks, struct clk **clock, struct device *dev)
+{
+   int ret;
+   int i;
+
+   for (i = 0; i < nclocks; i++) {
+   ret = clk_prepare_enable(clock[i]);
+   if (ret) {
+   dev_err(dev, "clock enable failed\n");
+   goto error;
+   }
+   }
+
+   return 0;
+
+error:
+   for (i--; i >= 0; i--)
+   clk_disable_unprepare(clock[i]);
+
+   return ret;
+}
+
+/*
+ * camss_disable_clocks - Disable multiple clocks
+ * @nclocks: Number of clocks in clock array
+ * @clock: Clock array
+ */
+void camss_disable_clocks(int nclocks, struct clk **clock)
+{
+   int i;
+
+   for (i = nclocks - 1; i >= 0; i--)
+