Re: [PATCH v2] media: omap3isp: fix unbalanced dma_iommu_mapping

[Suman Anna]

Hi Mauro,

On 03/21/2018 05:26 AM, Laurent Pinchart wrote:
> Hi Suman,
> 
> Thank you for the patch.
> 
> On Wednesday, 14 March 2018 17:41:36 EET Suman Anna wrote:
>> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
>> ARM DMA backend. The current code creates a dma_iommu_mapping and
>> attaches this to the ISP device, but never detaches the mapping in
>> either the probe failure paths or the driver remove path resulting
>> in an unbalanced mapping refcount and a memory leak. Fix this properly.
>>
>> Reported-by: Pavel Machek <pa...@ucw.cz>
>> Signed-off-by: Suman Anna <s-a...@ti.com>
>> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>

I don't see this patch in -next yet, can you pick up this patch at your
earliest?

Thanks,
Suman

> 
>> ---
>> v2 Changes:
>>  - Dropped the error_attach label, and returned directly from the
>>first error path (comments from Sakari)
>>  - Added Sakari's Acked-by
>> v1: https://patchwork.kernel.org/patch/10276759/
>>
>> Pavel,
>> I dropped your Tested-by from v2 since I modified the patch, can you
>> recheck the new patch again? Thanks.
>>
>> regards
>> Suman
>>
>>  drivers/media/platform/omap3isp/isp.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c
>> b/drivers/media/platform/omap3isp/isp.c index 8eb000e3d8fd..f2db5128d786
>> 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device
>> *isp)
>>
>>  static void isp_detach_iommu(struct isp_device *isp)
>>  {
>> +arm_iommu_detach_device(isp->dev);
>>  arm_iommu_release_mapping(isp->mapping);
>>  isp->mapping = NULL;
>>  }
>> @@ -1961,8 +1962,7 @@ static int isp_attach_iommu(struct isp_device *isp)
>>  mapping = arm_iommu_create_mapping(_bus_type, SZ_1G, SZ_2G);
>>  if (IS_ERR(mapping)) {
>>  dev_err(isp->dev, "failed to create ARM IOMMU mapping\n");
>> -ret = PTR_ERR(mapping);
>> -goto error;
>> +return PTR_ERR(mapping);
>>  }
>>
>>  isp->mapping = mapping;
>> @@ -1977,7 +1977,8 @@ static int isp_attach_iommu(struct isp_device *isp)
>>  return 0;
>>
>>  error:
>> -isp_detach_iommu(isp);
>> +arm_iommu_release_mapping(isp->mapping);
>> +isp->mapping = NULL;
>>  return ret;
>>  }
> 

[PATCH v2] media: omap3isp: fix unbalanced dma_iommu_mapping

[Suman Anna]

The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
ARM DMA backend. The current code creates a dma_iommu_mapping and
attaches this to the ISP device, but never detaches the mapping in
either the probe failure paths or the driver remove path resulting
in an unbalanced mapping refcount and a memory leak. Fix this properly.

Reported-by: Pavel Machek <pa...@ucw.cz>
Signed-off-by: Suman Anna <s-a...@ti.com>
Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
---
v2 Changes:
 - Dropped the error_attach label, and returned directly from the
   first error path (comments from Sakari)
 - Added Sakari's Acked-by
v1: https://patchwork.kernel.org/patch/10276759/

Pavel,
I dropped your Tested-by from v2 since I modified the patch, can you
recheck the new patch again? Thanks.

regards
Suman

 drivers/media/platform/omap3isp/isp.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index 8eb000e3d8fd..f2db5128d786 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
 
 static void isp_detach_iommu(struct isp_device *isp)
 {
+   arm_iommu_detach_device(isp->dev);
arm_iommu_release_mapping(isp->mapping);
isp->mapping = NULL;
 }
@@ -1961,8 +1962,7 @@ static int isp_attach_iommu(struct isp_device *isp)
mapping = arm_iommu_create_mapping(_bus_type, SZ_1G, SZ_2G);
if (IS_ERR(mapping)) {
dev_err(isp->dev, "failed to create ARM IOMMU mapping\n");
-   ret = PTR_ERR(mapping);
-   goto error;
+   return PTR_ERR(mapping);
}
 
isp->mapping = mapping;
@@ -1977,7 +1977,8 @@ static int isp_attach_iommu(struct isp_device *isp)
return 0;
 
 error:
-   isp_detach_iommu(isp);
+   arm_iommu_release_mapping(isp->mapping);
+   isp->mapping = NULL;
return ret;
 }
 
-- 
2.16.2

Re: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping

[Suman Anna]

Hi Sakari,

On 03/13/2018 06:14 AM, Sakari Ailus wrote:
> Hi Suman,
> 
> Thanks for the patch.
> 
> On Mon, Mar 12, 2018 at 11:52:07AM -0500, Suman Anna wrote:
>> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
>> ARM DMA backend. The current code creates a dma_iommu_mapping and
>> attaches this to the ISP device, but never detaches the mapping in
>> either the probe failure paths or the driver remove path resulting
>> in an unbalanced mapping refcount and a memory leak. Fix this properly.
>>
>> Reported-by: Pavel Machek <pa...@ucw.cz>
>> Signed-off-by: Suman Anna <s-a...@ti.com>
>> Tested-by: Pavel Machek <pa...@ucw.cz>
>> ---
>> Hi Mauro, Laurent,
>>
>> This fixes an issue reported by Pavel and discussed on this
>> thread,
>> https://marc.info/?l=linux-omap=152051945803598=2
>>
>> Posting this again to the appropriate lists.
>>
>> regards
>> Suman
>>
>>  drivers/media/platform/omap3isp/isp.c | 7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c 
>> b/drivers/media/platform/omap3isp/isp.c
>> index 8eb000e3d8fd..c7d667bfc2af 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device 
>> *isp)
>>  
>>  static void isp_detach_iommu(struct isp_device *isp)
>>  {
>> +arm_iommu_detach_device(isp->dev);
>>  arm_iommu_release_mapping(isp->mapping);
>>  isp->mapping = NULL;
>>  }
>> @@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
>>  ret = arm_iommu_attach_device(isp->dev, mapping);
>>  if (ret < 0) {
>>  dev_err(isp->dev, "failed to attach device to VA mapping\n");
>> -goto error;
>> +goto error_attach;
> 
> Instead of changing the label here, could you return immediately where the
> previous point of error handling is? No need to add another label.

Yeah, I debated about this while doing the patch, and chose to retain
the previous common return on the error paths. There are only 2 error
paths, so didn't want to mix them up. If you still prefer the mixed
style, I can post a v2.

regards
Suman

> 
> After fixing that you can add:
> 
> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> 
>>  }
>>  
>>  return 0;
>>  
>> +error_attach:
>> +arm_iommu_release_mapping(isp->mapping);
>> +isp->mapping = NULL;
>>  error:
>> -isp_detach_iommu(isp);
>>  return ret;
>>  }
>>  
>>
> 

[PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping

[Suman Anna]

The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
ARM DMA backend. The current code creates a dma_iommu_mapping and
attaches this to the ISP device, but never detaches the mapping in
either the probe failure paths or the driver remove path resulting
in an unbalanced mapping refcount and a memory leak. Fix this properly.

Reported-by: Pavel Machek <pa...@ucw.cz>
Signed-off-by: Suman Anna <s-a...@ti.com>
Tested-by: Pavel Machek <pa...@ucw.cz>
---
Hi Mauro, Laurent,

This fixes an issue reported by Pavel and discussed on this
thread,
https://marc.info/?l=linux-omap=152051945803598=2

Posting this again to the appropriate lists.

regards
Suman

 drivers/media/platform/omap3isp/isp.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index 8eb000e3d8fd..c7d667bfc2af 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1945,6 +1945,7 @@ static int isp_initialize_modules(struct isp_device *isp)
 
 static void isp_detach_iommu(struct isp_device *isp)
 {
+   arm_iommu_detach_device(isp->dev);
arm_iommu_release_mapping(isp->mapping);
isp->mapping = NULL;
 }
@@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
ret = arm_iommu_attach_device(isp->dev, mapping);
if (ret < 0) {
dev_err(isp->dev, "failed to attach device to VA mapping\n");
-   goto error;
+   goto error_attach;
}
 
return 0;
 
+error_attach:
+   arm_iommu_release_mapping(isp->mapping);
+   isp->mapping = NULL;
 error:
-   isp_detach_iommu(isp);
return ret;
 }
 
-- 
2.16.2

Re: [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO

[Suman Anna]

Hi Bjorn,

 Thanks for the patch.
>>
>> On 06/27/2017 01:43 AM, Bjorn Andersson wrote:
>>> While it's very common to use RPMSG for communicating with firmware
>>> running on these remoteprocs there is no functional dependency on RPMSG.
>>
>> This is not entirely accurate though. RPMSG is the IPC transport on
>> these remoteprocs, you seem to suggest that there are alternatives for
>> these remoteprocs. Without RPMSG, you can boot, but you will not be able
>> to talk to the remoteprocs, so I would call it a functional dependency.
>>
> 
> IMHO this functional dependency is not from the remoteproc driver but
> your system (and firmware) design. It should be possible to write
> firmware that exposes any other type of virtio device.

You may want to add this clarification to your commit description then.

> 
>>> As such RPMSG should be selected by the system integrator and not
>>> automatically by the remoteproc drivers.
>>>
>>> This does solve problems reported with circular Kconfig dependencies for
>>> Davinci and Keystone remoteproc drivers.
>>
>> The Keystone one issue shows up on linux-next (and not on 4.12-rcX) due
>> to the differing options on RESET_CONTROLLER on VIDEO_QCOM_VENUS
>> (through QCOM_SCOM).
> 
> That's probably why I didn't see this when build testing before pushing
> this.
> 
>> This can also be resolved by changing the depends on RESET_CONTROLLER
>> to a select RESET_CONTROLLER or dropping the line.
>>
> 
> Except that this would violate the general rule of "don't use 'select'
> for user-selectable options".

Yeah well, there seems to all sorts of usage w.r.t RESET_CONTROLLER and
VIRTIO. And if the ARCHs enable the ARCH_HAS_RESET_CONTROLLER, the
RESET_CONTROLLER dependencies are not even needed.

A quick grep on 4.12-rc7 yielded 20 occurrences that uses depends on and
21 for selects RESET_CONTROLLER. Similar numbers on VIRTIO are 9 and 9
(with a split between different VIRTIO drivers even).

> 
>> The davinci one is tricky though, as I did change it from using a select
>> to a depends on dependency, and obviously ppc64_defconfig is something
>> that I would not check.

Posted a cleanup series that removes the VIRTUALIZATION dependency from
REMOTEPROC and RPMSG_VIRTIO options, the latter fixes the
ppc64_defconfig issue with davinci remoteproc.

>>
> 
> While I hate the process of figuring out and enable all the dependencies
> to be able to enable a particular config, this change does reduce the
> risk of running into more of these cyclic dependencies.
> 
>> This patch definitely resolves both issues, but it is not obvious that
>> someone would also have to enable RPMSG_VIRTIO to have these remoteprocs
>> useful when looking at either of the menuconfig help.
>>
> 
> This is a common issue we have with config options and while I hate to
> add another item to the list of things that you can miss in your system
> configuration I would prefer that my Kconfig is maintainable.
> 
> This also means that most remoteproc drivers should "depends on MAILBOX"
> and not select either the framework or the specific drivers.  But let's
> try to ignore that until after the merge window.

Yeah ok, as long as you follow a consistent rule across all
remoteproc/rpmsg drivers, that's fine. In fact, other than RPMSG_VIRTIO,
the two drivers I added for this merge window do use/switches to a
depends on convention.

regards
Suman

Re: [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO

[Suman Anna]

Hi Bjorn,

Thanks for the patch.

On 06/27/2017 01:43 AM, Bjorn Andersson wrote:
> While it's very common to use RPMSG for communicating with firmware
> running on these remoteprocs there is no functional dependency on RPMSG.

This is not entirely accurate though. RPMSG is the IPC transport on
these remoteprocs, you seem to suggest that there are alternatives for
these remoteprocs. Without RPMSG, you can boot, but you will not be able
to talk to the remoteprocs, so I would call it a functional dependency.

> As such RPMSG should be selected by the system integrator and not
> automatically by the remoteproc drivers.
> 
> This does solve problems reported with circular Kconfig dependencies for
> Davinci and Keystone remoteproc drivers.

The Keystone one issue shows up on linux-next (and not on 4.12-rcX) due
to the differing options on RESET_CONTROLLER on VIDEO_QCOM_VENUS
(through QCOM_SCOM). This can also be resolved by changing the depends
on RESET_CONTROLLER to a select RESET_CONTROLLER or dropping the line.

The davinci one is tricky though, as I did change it from using a select
to a depends on dependency, and obviously ppc64_defconfig is something
that I would not check.

This patch definitely resolves both issues, but it is not obvious that
someone would also have to enable RPMSG_VIRTIO to have these remoteprocs
useful when looking at either of the menuconfig help.

regards
Suman

> 
> Signed-off-by: Bjorn Andersson 
> ---
>  drivers/media/platform/Kconfig |  2 +-
>  drivers/remoteproc/Kconfig |  4 
>  drivers/rpmsg/Kconfig  | 20 +---
>  3 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 1313cd533436..cb2f31cd0088 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -382,10 +382,10 @@ config VIDEO_STI_DELTA_DRIVER
>   tristate
>   depends on VIDEO_STI_DELTA
>   depends on VIDEO_STI_DELTA_MJPEG
> + depends on RPMSG
>   default VIDEO_STI_DELTA_MJPEG
>   select VIDEOBUF2_DMA_CONTIG
>   select V4L2_MEM2MEM_DEV
> - select RPMSG
>  
>  endif # VIDEO_STI_DELTA
>  
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index b950e6cd4ba2..3b16f422d30c 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -21,7 +21,6 @@ config OMAP_REMOTEPROC
>   depends on REMOTEPROC
>   select MAILBOX
>   select OMAP2PLUS_MBOX
> - select RPMSG_VIRTIO
>   help
> Say y here to support OMAP's remote processors (dual M3
> and DSP on OMAP4) via the remote processor framework.
> @@ -53,7 +52,6 @@ config DA8XX_REMOTEPROC
>   depends on ARCH_DAVINCI_DA8XX
>   depends on REMOTEPROC
>   depends on DMA_CMA
> - select RPMSG_VIRTIO
>   help
> Say y here to support DA8xx/OMAP-L13x remote processors via the
> remote processor framework.
> @@ -76,7 +74,6 @@ config KEYSTONE_REMOTEPROC
>   depends on ARCH_KEYSTONE
>   depends on RESET_CONTROLLER
>   depends on REMOTEPROC
> - select RPMSG_VIRTIO
>   help
> Say Y here here to support Keystone remote processors (DSP)
> via the remote processor framework.
> @@ -133,7 +130,6 @@ config ST_REMOTEPROC
>   depends on REMOTEPROC
>   select MAILBOX
>   select STI_MBOX
> - select RPMSG_VIRTIO
>   help
> Say y here to support ST's adjunct processors via the remote
> processor framework.
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 2a5d2b446de2..46f3f2431d68 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -1,8 +1,5 @@
> -menu "Rpmsg drivers"
> -
> -# RPMSG always gets selected by whoever wants it
> -config RPMSG
> - tristate
> +menuconfig RPMSG
> + tristate "Rpmsg drivers"
>  
>  config RPMSG_CHAR
>   tristate "RPMSG device interface"
> @@ -15,7 +12,7 @@ config RPMSG_CHAR
>  
>  config RPMSG_QCOM_GLINK_RPM
>   tristate "Qualcomm RPM Glink driver"
> - select RPMSG
> + depends on RPMSG
>   depends on HAS_IOMEM
>   depends on MAILBOX
>   help
> @@ -26,16 +23,17 @@ config RPMSG_QCOM_GLINK_RPM
>  config RPMSG_QCOM_SMD
>   tristate "Qualcomm Shared Memory Driver (SMD)"
>   depends on QCOM_SMEM
> - select RPMSG
> + depends on RPMSG
>   help
> Say y here to enable support for the Qualcomm Shared Memory Driver
> providing communication channels to remote processors in Qualcomm
> platforms.
>  
>  config RPMSG_VIRTIO
> - tristate
> - select RPMSG
> + tristate "Virtio remote processor messaging driver (RPMSG)"
> + depends on RPMSG
>   select VIRTIO
>   select VIRTUALIZATION
> -
> -endmenu
> + help
> +   Say y here to enable support for the Virtio remote processor
> +   messaging protocol (RPMSG).
> 

Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

[Suman Anna]

On 06/23/2017 01:59 PM, H. Nikolaus Schaller wrote:
> Hi Suman,
> 
>> Am 23.06.2017 um 20:05 schrieb Suman Anna <s-a...@ti.com>:
>>
>>>>>
>>>>> Or does it just mean that it defines the property name?
>>>>
>>>> Please read the documentation link I sent - it's in the very bottom and
>>>> should have an example.
>>>
>>> I have seen it but it does not give me a good clue how to translate that 
>>> into
>>> correct omap3isp node setup in a specific DT. Rather it raises more 
>>> questions.
>>> Maybe because I don't understand completely what it is talking about.
>>>
>>> The fundamental question is if this "assigned-clock-rates" is already
>>> handled by ov965x->clk = devm_clk_get(>dev, NULL); ?
>>>
>>> Or should we define that for the omap3isp node?
>>>
>>> Then of course we need no new code and just use the right property names.
>>> And N900, N9 camera DTs should be updated.
>>
>> Look up of_clk_set_defaults() function in drivers/clk/clk-conf.c. This
>> function gets invoked usually during clock registration, and also gets
>> called in platform_drv_probe(), so the parents and clocks do get
>> configured before your driver gets probed. So, this provides a default
>> configuration if these properties are supplied (in either clock nodes or
>> actual device nodes), and if your driver needs to change the rates at
>> runtime, then you would have to do that in the driver itself.
> 
> Ok, now I understand. Thanks!
> 
> Quite hidden, but nice feature. I would never have thought that it exists.
> Especially as there are no examples around omap3isp cameras...
> 
> And an fgrep assigned-clock-rates shows not many use cases outside CPU/SoC
> include files.
> 
> But interestingly arch/arm/boot/dts/at91sam9g25ek.dts uses it for an 
> ovti,ov2640 camera...
> 
> So it seems that we just have to write:
> 
>   ov9655@30 {
>   compatible = "ovti,ov9655";
>   reg = <0x30>;
>   clocks = < 0>;  /* cam_clka */
>   assigned-clocks = < 0>;
>   assigned-clock-rates = <2400>;
>   };

Yeah, that looks alright and should work.

regards
Suman

> 
> instead of introducing a new clock-frequency property and code to handle it.
> 
> Or do I misinterpret what "parents" and "clocks" are in this context?

Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

[Suman Anna]

Hi Nikolaus,

On 06/23/2017 10:22 AM, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 23.06.2017 um 16:57 schrieb Andreas Färber :
>>
>> Am 23.06.2017 um 16:53 schrieb H. Nikolaus Schaller:
>>> Hi Laurent,
>>>
 Am 23.06.2017 um 13:58 schrieb Laurent Pinchart 
 :

 Hi Nikolaus,

 On Friday 23 Jun 2017 12:59:24 H. Nikolaus Schaller wrote:
> Am 23.06.2017 um 12:46 schrieb Andreas Färber :
>> Am 23.06.2017 um 12:25 schrieb H. Nikolaus Schaller:
 diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt
 b/Documentation/devicetree/bindings/media/i2c/ov965x.txt new file mode
 100644
 index 000..0e0de1f
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
 @@ -0,0 +1,37 @@
 +* Omnivision OV9650/9652/9655 CMOS sensor
 +
 +The Omnivision OV965x sensor support multiple resolutions output, such
 as
 +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
 +output format.
 +
 +Required Properties:
 +- compatible: should be one of
 +  "ovti,ov9650"
 +  "ovti,ov9652"
 +  "ovti,ov9655"
 +- clocks: reference to the mclk input clock.
>>>
>>> I wonder why you have removed the clock-frequency property?
>>>
>>> In some situations the camera driver must be able to tell the clock
>>> source which frequency it wants to see.
>>
>> That's what assigned-clock-rates property is for:
>>
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/clock-b
>> indings.txt
>>
>> AFAIU clock-frequency on devices is deprecated and equivalent to having
>> a clocks property pointing to a fixed-clock, which is different from a
>> clock with varying rate.
>
> I am not sure if that helps here. The OMAP3-ISP does not have a fixed 
> clock
> rate so we can only have the driver define what it wants to see.
>
> And common practise for OMAP3-ISP based camera modules (e.g. N900, N9) is
> that they do it in the driver.
>
> Maybe ISP developers can comment?

 The OMAP3 ISP is a variable-frequency clock provider. The clock frequency 
 is 
 controlled by the clock consumer. As such, it's up to the consumer to 
 decide 
 whether to compute and request the clock rate dynamically at runtime, or 
 use 
 the assigned-clock-rates property in DT.

 Some ISPs include a clock generator, others don't. It should make no 
 difference whether the clock is provided by the ISP, by a dedicated clock 
 source in the SoC or by a discrete on-board adjustable clock source.
>>>
>>> Thanks for explaining the background.
>>>
>>> Do you have an hint or example how to use the assigned-clock-rates property 
>>> in
>>> a DT for a camera module connected to the omap3isp?
>>>
>>> Or does it just mean that it defines the property name?
>>
>> Please read the documentation link I sent - it's in the very bottom and
>> should have an example.
> 
> I have seen it but it does not give me a good clue how to translate that into
> correct omap3isp node setup in a specific DT. Rather it raises more questions.
> Maybe because I don't understand completely what it is talking about.
> 
> The fundamental question is if this "assigned-clock-rates" is already
> handled by ov965x->clk = devm_clk_get(>dev, NULL); ?
> 
> Or should we define that for the omap3isp node?
> 
> Then of course we need no new code and just use the right property names.
> And N900, N9 camera DTs should be updated.

Look up of_clk_set_defaults() function in drivers/clk/clk-conf.c. This
function gets invoked usually during clock registration, and also gets
called in platform_drv_probe(), so the parents and clocks do get
configured before your driver gets probed. So, this provides a default
configuration if these properties are supplied (in either clock nodes or
actual device nodes), and if your driver needs to change the rates at
runtime, then you would have to do that in the driver itself.

regards
Suman

Re: [PATCH 1/2] ARM: OMAP2+: Remove legacy OMAP3 ISP instantiation

[Suman Anna]

Hi Tony,

On 09/03/2015 05:34 PM, Suman Anna wrote:
> Hi Sakari,
> 
> On 07/16/2015 07:58 AM, Tony Lindgren wrote:
>> * Laurent Pinchart <laurent.pinch...@ideasonboard.com> [150716 05:57]:
>>> The OMAP3 ISP is now fully supported in DT, remove its instantiation
>>> from C code.
>>
>> Please feel to queue this along with the second patch in this series,
>> this should not cause any merge conflicts:
>>
>> Acked-by: Tony Lindgren <t...@atomide.com>
> 
> Just wondering if you have already queued this, I see the v4l changes in
> linux-next, but not this patch. Also, can you confirm if this series is
> making it into 4.3?
> 

This patch is not in 4.3-rc1, can you pick this up for the next merge
window? I am gonna send out some additional cleanup patches (removing
legacy mailbox and iommu pieces) on top on this patch. The second patch
in this series did make it.

regards
Suman

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: OMAP2+: Remove legacy OMAP3 ISP instantiation

[Suman Anna]

Hi Sakari,

On 07/16/2015 07:58 AM, Tony Lindgren wrote:
> * Laurent Pinchart  [150716 05:57]:
>> The OMAP3 ISP is now fully supported in DT, remove its instantiation
>> from C code.
> 
> Please feel to queue this along with the second patch in this series,
> this should not cause any merge conflicts:
> 
> Acked-by: Tony Lindgren 

Just wondering if you have already queued this, I see the v4l changes in
linux-next, but not this patch. Also, can you confirm if this series is
making it into 4.3?

regards
Suman

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html