diff mbox

[v4,02/10] s5p-fimc: Add device tree support for FIMC devices

Message ID 1359745771-23684-3-git-send-email-s.nawrocki@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

This patch adds support for FIMC devices instantiated from devicetree
for S5PV210 and Exynos4 SoCs. The FIMC IP features include colorspace
conversion and scaling (mem-to-mem) and parallel/MIPI CSI2 bus video
capture interface.

Multiple SoC revisions specific parameters are defined statically in
the driver and are used for both dt and non-dt. The driver's static
data is selected based on the compatible property. Previously the
platform device name was used to match driver data and a specific
SoC/IP version.

Aliases are used to determine an index of the IP which is essential
for linking FIMC IP with other entities, like MIPI-CSIS (the MIPI
CSI-2 bus frontend) or FIMC-LITE and FIMC-IS ISP.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Changes since v3:
 - added optional clock-frequency property to specify local bus
   (FIMCn_LCLK) clock frequency
---
 .../devicetree/bindings/media/soc/samsung-fimc.txt |   72 +++++++++++++++
 drivers/media/platform/s5p-fimc/fimc-capture.c     |    2 +-
 drivers/media/platform/s5p-fimc/fimc-core.c        |   93 +++++++++++++-------
 3 files changed, 135 insertions(+), 32 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-fimc.txt

Comments

Stephen Warren Feb. 6, 2013, 11:40 p.m. UTC | #1
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
> This patch adds support for FIMC devices instantiated from devicetree
> for S5PV210 and Exynos4 SoCs. The FIMC IP features include colorspace
> conversion and scaling (mem-to-mem) and parallel/MIPI CSI2 bus video
> capture interface.
> 
> Multiple SoC revisions specific parameters are defined statically in
> the driver and are used for both dt and non-dt. The driver's static
> data is selected based on the compatible property. Previously the
> platform device name was used to match driver data and a specific
> SoC/IP version.
> 
> Aliases are used to determine an index of the IP which is essential
> for linking FIMC IP with other entities, like MIPI-CSIS (the MIPI
> CSI-2 bus frontend) or FIMC-LITE and FIMC-IS ISP.

> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt

> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
> +----------------------------------------------
> +
> +The Exynos Camera subsystem comprises of multiple sub-devices that are
> +represented by separate platform devices. Some of the IPs come in different

"platform devices" is a rather Linux-centric term, and DT bindings
should be OS-agnostic. Perhaps use "device tree nodes" here?

> +variants across the SoC revisions (FIMC) and some remain mostly unchanged
> +(MIPI CSIS, FIMC-LITE).
> +
> +All those sub-subdevices are defined as parent nodes of the common device

s/parent nodes/child node/ I think?

> +For every fimc node a numbered alias should be present in the aliases node.
> +Aliases are of the form fimc<n>, where <n> is an integer (0...N) specifying
> +the IP's instance index.

Why? Isn't it up to the DT author whether they care if each fimc node is
assigned a specific identification v.s. whether identification is
assigned automatically?

> +Optional properties
> +
> + - clock-frequency - maximum FIMC local clock (LCLK) frequency

Again, I'd expect a clocks property here instead.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Feb. 8, 2013, 11:16 p.m. UTC | #2
On 02/07/2013 12:40 AM, Stephen Warren wrote:
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>
>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>> +----------------------------------------------
>> +
>> +The Exynos Camera subsystem comprises of multiple sub-devices that are
>> +represented by separate platform devices. Some of the IPs come in different
>
> "platform devices" is a rather Linux-centric term, and DT bindings
> should be OS-agnostic. Perhaps use "device tree nodes" here?

Indeed, thank you for the suggestion, I'll change it.

>> +variants across the SoC revisions (FIMC) and some remain mostly unchanged
>> +(MIPI CSIS, FIMC-LITE).
>> +
>> +All those sub-subdevices are defined as parent nodes of the common device
>
> s/parent nodes/child node/ I think?

Yeah, 'parent nodes' doesn't really make sense. Thanks for catching it.

>> +For every fimc node a numbered alias should be present in the aliases node.
>> +Aliases are of the form fimc<n>, where<n>  is an integer (0...N) specifying
>> +the IP's instance index.
>
> Why? Isn't it up to the DT author whether they care if each fimc node is
> assigned a specific identification v.s. whether identification is
> assigned automatically?

There are at least three different kinds of IPs that come in multiple
instances in an SoC. To activate data links between them each instance
needs to be clearly identified. There are also differences between
instances of same device. Hence it's important these aliases don't have
random values.

Some more details about the SoC can be found at [1]. The aliases are
also already used in the Exynos5 GScaler bindings [2] in a similar way.

>> +Optional properties
>> +
>> + - clock-frequency - maximum FIMC local clock (LCLK) frequency
>
> Again, I'd expect a clocks property here instead.

Please see my comment to patch 01/10. Analogously, I needed this clock
frequency to ensure reliable video data pipeline operation.

[1] http://tinyurl.com/anw9udm
[2] http://www.spinics.net/lists/arm-kernel/msg200036.html
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 8, 2013, 11:21 p.m. UTC | #3
On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>
>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>> +----------------------------------------------
...
>>> +For every fimc node a numbered alias should be present in the
>>> aliases node.
>>> +Aliases are of the form fimc<n>, where<n>  is an integer (0...N)
>>> specifying
>>> +the IP's instance index.
>>
>> Why? Isn't it up to the DT author whether they care if each fimc node is
>> assigned a specific identification v.s. whether identification is
>> assigned automatically?
> 
> There are at least three different kinds of IPs that come in multiple
> instances in an SoC. To activate data links between them each instance
> needs to be clearly identified. There are also differences between
> instances of same device. Hence it's important these aliases don't have
> random values.
> 
> Some more details about the SoC can be found at [1]. The aliases are
> also already used in the Exynos5 GScaler bindings [2] in a similar way.

Hmmm. I'd expect explicit DT properties to represent the
instance-specific "configuration", or even different compatible values.
Relying on the alias ID seems rather indirect; what if in e.g.
Exynos6/... the mapping from instance/alias ID to feature set changes.
With explicit DT properties, that'd just be a .dts change, whereas by
requiring alias IDs now, you'd need a driver change to support this.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Feb. 9, 2013, 12:05 a.m. UTC | #4
On 02/09/2013 12:21 AM, Stephen Warren wrote:
> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>
>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>> +----------------------------------------------
> ...
>>>> +For every fimc node a numbered alias should be present in the
>>>> aliases node.
>>>> +Aliases are of the form fimc<n>, where<n>   is an integer (0...N)
>>>> specifying
>>>> +the IP's instance index.
>>>
>>> Why? Isn't it up to the DT author whether they care if each fimc node is
>>> assigned a specific identification v.s. whether identification is
>>> assigned automatically?
>>
>> There are at least three different kinds of IPs that come in multiple
>> instances in an SoC. To activate data links between them each instance
>> needs to be clearly identified. There are also differences between
>> instances of same device. Hence it's important these aliases don't have
>> random values.
>>
>> Some more details about the SoC can be found at [1]. The aliases are
>> also already used in the Exynos5 GScaler bindings [2] in a similar way.
>
> Hmmm. I'd expect explicit DT properties to represent the
> instance-specific "configuration", or even different compatible values.
> Relying on the alias ID seems rather indirect; what if in e.g.
> Exynos6/... the mapping from instance/alias ID to feature set changes.
> With explicit DT properties, that'd just be a .dts change, whereas by
> requiring alias IDs now, you'd need a driver change to support this.

In the initial version of this patch series I used cell-index property,
but then Grant pointed out in some other mail thread it should be
avoided. Hence I used the node aliases.

Different compatible values might not work, when for example there
are 3 IPs out of 4 of one type and the fourth one of another type.
It wouldn't even by really different types, just quirks/little
differences between them, e.g. no data path routed to one of other IPs.

Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
MIPI-CSIS needs to be written to the FIMC.2 data input control register.
Even though MIPI-CSIS.N are same in terms of hardware structure they still
need to be distinguished as separate instances.

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 9, 2013, 12:32 a.m. UTC | #5
On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>
>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>> +----------------------------------------------
>> ...
>>>>> +For every fimc node a numbered alias should be present in the
>>>>> aliases node.
>>>>> +Aliases are of the form fimc<n>, where<n>   is an integer (0...N)
>>>>> specifying
>>>>> +the IP's instance index.
>>>>
>>>> Why? Isn't it up to the DT author whether they care if each fimc
>>>> node is
>>>> assigned a specific identification v.s. whether identification is
>>>> assigned automatically?
>>>
>>> There are at least three different kinds of IPs that come in multiple
>>> instances in an SoC. To activate data links between them each instance
>>> needs to be clearly identified. There are also differences between
>>> instances of same device. Hence it's important these aliases don't have
>>> random values.
>>>
>>> Some more details about the SoC can be found at [1]. The aliases are
>>> also already used in the Exynos5 GScaler bindings [2] in a similar way.
>>
>> Hmmm. I'd expect explicit DT properties to represent the
>> instance-specific "configuration", or even different compatible values.
>> Relying on the alias ID seems rather indirect; what if in e.g.
>> Exynos6/... the mapping from instance/alias ID to feature set changes.
>> With explicit DT properties, that'd just be a .dts change, whereas by
>> requiring alias IDs now, you'd need a driver change to support this.
> 
> In the initial version of this patch series I used cell-index property,
> but then Grant pointed out in some other mail thread it should be
> avoided. Hence I used the node aliases.

To me, using cell-index is semantically equivalent to using the alias ID.

> Different compatible values might not work, when for example there
> are 3 IPs out of 4 of one type and the fourth one of another type.
> It wouldn't even by really different types, just quirks/little
> differences between them, e.g. no data path routed to one of other IPs.

I was thinking of using feature-/quirk-oriented properties. For example,
if there's a port on 3 of the 4 devices to connect to some other IP
block, simply include a boolean property to indicate whether that port
is present. It would be in 3 of the nodes but not the 4th.

> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
> Even though MIPI-CSIS.N are same in terms of hardware structure they still
> need to be distinguished as separate instances.

Oh, so you're using the alias ID as the value to write into the FIMC.2
register for that. I'm not 100% familiar with aliases, but they seem
like a more user-oriented naming thing to me, whereas values for hooking
up intra-SoC routing are an unrelated namespace semantically, even if
the values happen to line up right now. Perhaps rather than a Boolean
property I mentioned above, use a custom property to indicate the ID
that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
similar to using cell-index, my *guess* is that Grant's objection to
using cell-index was more based on re-using cell-index for something
other than its intended purpose than pushing you to use an alias ID
rather than a property.

After all, what happens in some later SoC where you have two different
types of module that feed into the common module, such that type A
sources have IDs 0..3 in the common module, and type B sources have IDs
4..7 in the common module - you wouldn't want to require alias ISs 4..7
for the type B DT nodes.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Feb. 9, 2013, 10:29 p.m. UTC | #6
On 02/09/2013 01:32 AM, Stephen Warren wrote:
> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>
>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>> +----------------------------------------------
>>> ...
>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>> aliases node.
>>>>>> +Aliases are of the form fimc<n>, where<n>    is an integer (0...N)
>>>>>> specifying
>>>>>> +the IP's instance index.
>>>>>
>>>>> Why? Isn't it up to the DT author whether they care if each fimc
>>>>> node is
>>>>> assigned a specific identification v.s. whether identification is
>>>>> assigned automatically?
>>>>
>>>> There are at least three different kinds of IPs that come in multiple
>>>> instances in an SoC. To activate data links between them each instance
>>>> needs to be clearly identified. There are also differences between
>>>> instances of same device. Hence it's important these aliases don't have
>>>> random values.
>>>>
>>>> Some more details about the SoC can be found at [1]. The aliases are
>>>> also already used in the Exynos5 GScaler bindings [2] in a similar way.
>>>
>>> Hmmm. I'd expect explicit DT properties to represent the
>>> instance-specific "configuration", or even different compatible values.
>>> Relying on the alias ID seems rather indirect; what if in e.g.
>>> Exynos6/... the mapping from instance/alias ID to feature set changes.
>>> With explicit DT properties, that'd just be a .dts change, whereas by
>>> requiring alias IDs now, you'd need a driver change to support this.
>>
>> In the initial version of this patch series I used cell-index property,
>> but then Grant pointed out in some other mail thread it should be
>> avoided. Hence I used the node aliases.
>
> To me, using cell-index is semantically equivalent to using the alias ID.

I can't see significant difference either. I just switched to what
seemed to be used for similar purpose.

>> Different compatible values might not work, when for example there
>> are 3 IPs out of 4 of one type and the fourth one of another type.
>> It wouldn't even by really different types, just quirks/little
>> differences between them, e.g. no data path routed to one of other IPs.
>
> I was thinking of using feature-/quirk-oriented properties. For example,
> if there's a port on 3 of the 4 devices to connect to some other IP
> block, simply include a boolean property to indicate whether that port
> is present. It would be in 3 of the nodes but not the 4th.

Yes, I could add several properties corresponding to all members of this
[3] data structure. But still it is needed to clearly identify the IP
block in a set of the hardware instances.

>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
>> Even though MIPI-CSIS.N are same in terms of hardware structure they still
>> need to be distinguished as separate instances.
>
> Oh, so you're using the alias ID as the value to write into the FIMC.2
> register for that. I'm not 100% familiar with aliases, but they seem
> like a more user-oriented naming thing to me, whereas values for hooking
> up intra-SoC routing are an unrelated namespace semantically, even if
> the values happen to line up right now. Perhaps rather than a Boolean
> property I mentioned above, use a custom property to indicate the ID
> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems

That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
blocks are immutably connected to the SoC camera physical MIPI CSI-2
interfaces, and the physical camera ports have fixed assignment to the
MIPI-CSIS devices..  This way we could drop alias ID for the MIPI-CSIS
nodes. And their instance index that is required for the top level
driver which exposes topology and the routing capabilities to user space
could be restored from the reg property value by subtracting a fixed
offset.

Similarly an instance index of FIMC-LITE could be derived from the value
of reg property in a port node that links it to FIMC-IS ISP. I have been
omitting these port/endpoint nodes because it seemed unnecessary to model
explicitly those data paths. However it feels a bit overkill to add them
just for the sake of identifying the IP block instance

Still I can't really see a possibility to drop indexes for the FIMC nodes.

> similar to using cell-index, my *guess* is that Grant's objection to
> using cell-index was more based on re-using cell-index for something
> other than its intended purpose than pushing you to use an alias ID
> rather than a property.

The comments to a patch for some other driver I was referring to can be
found at [1]. My first patch series appeared significantly later [2].
I confused things a bit, sorry about that.

I can see aliases used in bindings of multiple devices: uart, spi, sound
interfaces, gpio, ... And all bindings seem to impose some rules on how
their aliases are created.

> After all, what happens in some later SoC where you have two different
> types of module that feed into the common module, such that type A
> sources have IDs 0..3 in the common module, and type B sources have IDs
> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
> for the type B DT nodes.

There is no need to write alias ID directly into registers, and actually
it doesn't really happen. But we need to know that, for example camera A
is connected to physical MIPI CSI-2 channel 0 and to capture video with
DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to
MIPI-CSIS 0.

The system registers, where a sort of camera/display glue logic is
configured also refer to FIMC devices explicitly by ID. So the driver
as a client of the system registers block/glue logic interface needs
to pass an information which FIMC H/W instance it wants to be e.g.
attached/detached to/from some data pipeline.

I still need to design some API for the camera system registers (glue
logic) block. This would be shared by the V4L2 and the DRM FIMC driver.
I thought about a couple of function calls specific to Exynos platform
that would be called directly by related drivers.

[1] 
https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-January/012128.html
[2] http://www.spinics.net/lists/linux-media/msg48341.html
[3] 
http://lxr.linux.no/#linux+v3.7.6/drivers/media/platform/s5p-fimc/fimc-core.h#L365
[4] http://tinyurl.com/arczzuo
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Feb. 9, 2013, 10:52 p.m. UTC | #7
On 02/09/2013 11:29 PM, Sylwester Nawrocki wrote:
>
>> After all, what happens in some later SoC where you have two different
>> types of module that feed into the common module, such that type A
>> sources have IDs 0..3 in the common module, and type B sources have IDs
>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
>> for the type B DT nodes.

I forgot to add, any ID remapping could happen in the common module, if
it requires it. Type A and type B sources could have indexes 0...3 and
the common module could derive its configuration from the source ID *and*
the source type. The idea behind aliases was to identify each instance,
rather than providing an exact configuration data that the common module
could use.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 11, 2013, 9:50 p.m. UTC | #8
On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote:
> On 02/09/2013 01:32 AM, Stephen Warren wrote:
>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>
>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>>> +----------------------------------------------
>>>> ...
>>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>>> aliases node.
>>>>>>> +Aliases are of the form fimc<n>, where<n>    is an integer (0...N)
>>>>>>> specifying
>>>>>>> +the IP's instance index.
...
>>> Different compatible values might not work, when for example there
>>> are 3 IPs out of 4 of one type and the fourth one of another type.
>>> It wouldn't even by really different types, just quirks/little
>>> differences between them, e.g. no data path routed to one of other IPs.
>>
>> I was thinking of using feature-/quirk-oriented properties. For example,
>> if there's a port on 3 of the 4 devices to connect to some other IP
>> block, simply include a boolean property to indicate whether that port
>> is present. It would be in 3 of the nodes but not the 4th.
> 
> Yes, I could add several properties corresponding to all members of this
> [3] data structure. But still it is needed to clearly identify the IP
> block in a set of the hardware instances.

Why? What decisions will be made based on the identify of the IP block
instance that wouldn't be covered by DT properties that describe which
features/bugs/... the IP block instance has?

>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>>> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
>>> Even though MIPI-CSIS.N are same in terms of hardware structure they
>>> still
>>> need to be distinguished as separate instances.
>>
>> Oh, so you're using the alias ID as the value to write into the FIMC.2
>> register for that. I'm not 100% familiar with aliases, but they seem
>> like a more user-oriented naming thing to me, whereas values for hooking
>> up intra-SoC routing are an unrelated namespace semantically, even if
>> the values happen to line up right now. Perhaps rather than a Boolean
>> property I mentioned above, use a custom property to indicate the ID
>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
> 
> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
> blocks are immutably connected to the SoC camera physical MIPI CSI-2
> interfaces, and the physical camera ports have fixed assignment to the
> MIPI-CSIS devices..  This way we could drop alias ID for the MIPI-CSIS
> nodes. And their instance index that is required for the top level
> driver which exposes topology and the routing capabilities to user space
> could be restored from the reg property value by subtracting a fixed
> offset.

I suppose that would work. It feels a little indirect, and I think means
that the driver needs to go find some child node defining its end of
some link, then find the node representing the other end of the link,
then read properties out of that other node to find the value. That
seems a little unusual, but I guess it would work. I'm not sure of the
long-term implications of doing that kind of thing. You'd want to run
the idea past some DT maintainers/experts.

...
> I can see aliases used in bindings of multiple devices: uart, spi, sound
> interfaces, gpio, ... And all bindings seem to impose some rules on how
> their aliases are created.

Do you have specific examples? I really don't think the bindings should
be dictating the alias values.

>> After all, what happens in some later SoC where you have two different
>> types of module that feed into the common module, such that type A
>> sources have IDs 0..3 in the common module, and type B sources have IDs
>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
>> for the type B DT nodes.
> 
> There is no need to write alias ID directly into registers, and actually
> it doesn't really happen. But we need to know that, for example camera A
> is connected to physical MIPI CSI-2 channel 0 and to capture video with
> DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to
> MIPI-CSIS 0.

OK, so the IDs are selecting which register to write, or which mux
settings to access. That's pretty much semantically the same thing.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Feb. 12, 2013, 10:39 p.m. UTC | #9
On 02/11/2013 10:50 PM, Stephen Warren wrote:
> On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote:
>> On 02/09/2013 01:32 AM, Stephen Warren wrote:
>>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>>>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>
>>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>>>> +----------------------------------------------
>>>>> ...
>>>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>>>> aliases node.
>>>>>>>> +Aliases are of the form fimc<n>, where<n>     is an integer (0...N)
>>>>>>>> specifying
>>>>>>>> +the IP's instance index.
> ...
>>>> Different compatible values might not work, when for example there
>>>> are 3 IPs out of 4 of one type and the fourth one of another type.
>>>> It wouldn't even by really different types, just quirks/little
>>>> differences between them, e.g. no data path routed to one of other IPs.
>>>
>>> I was thinking of using feature-/quirk-oriented properties. For example,
>>> if there's a port on 3 of the 4 devices to connect to some other IP
>>> block, simply include a boolean property to indicate whether that port
>>> is present. It would be in 3 of the nodes but not the 4th.
>>
>> Yes, I could add several properties corresponding to all members of this
>> [3] data structure. But still it is needed to clearly identify the IP
>> block in a set of the hardware instances.
>
> Why? What decisions will be made based on the identify of the IP block
> instance that wouldn't be covered by DT properties that describe which
> features/bugs/... the IP block instance has?

The whole subsystem topology is exposed to user space through the Media
Controller API. Although the user space libraries/applications using
this driver are not much concerned how the hardware is represented
internally in the kernel, some properties of the media entities seen
in user space are derived from the hardware details, e.g. the media entity
names contain index of a corresponding IP block. Please see [1] for
an example of a topology exposed by the driver.

Since different H/W instances have different capabilities, user space
libraries/plugins can be coded to e.g. use one instance for video playback
post-processing and another for camera capture. The capabilities could be
also discovered with the V4L2 API to some level of detail. But still
assigning random entity names to the IP blocks has a potential of breaking
user space or causing some malfunctions.

Perhaps I should just use a custom properties like "samsung,fimc-id" ?
I tried to represent some intra-soc data routing details with our common
video interfaces bindings and it really looked like a lot of unnecessary
nodes, with 11 camera sub-device nodes required to cover a front a rear
facing camera. Some details can be just coded in the driver, especially
that newer SoCs will get a new driver anyway, since there are huge
differences between the media subsystem architecture across subsequent
SoC revisions.

[1] http://www.spinics.net/lists/linux-media/attachments/psPhA96YX70U.ps

>>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>>>> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
>>>> Even though MIPI-CSIS.N are same in terms of hardware structure they
>>>> still
>>>> need to be distinguished as separate instances.
>>>
>>> Oh, so you're using the alias ID as the value to write into the FIMC.2
>>> register for that. I'm not 100% familiar with aliases, but they seem
>>> like a more user-oriented naming thing to me, whereas values for hooking
>>> up intra-SoC routing are an unrelated namespace semantically, even if
>>> the values happen to line up right now. Perhaps rather than a Boolean
>>> property I mentioned above, use a custom property to indicate the ID
>>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
>>
>> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
>> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
>> blocks are immutably connected to the SoC camera physical MIPI CSI-2
>> interfaces, and the physical camera ports have fixed assignment to the
>> MIPI-CSIS devices..  This way we could drop alias ID for the MIPI-CSIS
>> nodes. And their instance index that is required for the top level
>> driver which exposes topology and the routing capabilities to user space
>> could be restored from the reg property value by subtracting a fixed
>> offset.
>
> I suppose that would work. It feels a little indirect, and I think means
> that the driver needs to go find some child node defining its end of
> some link, then find the node representing the other end of the link,
> then read properties out of that other node to find the value. That
> seems a little unusual, but I guess it would work. I'm not sure of the
> long-term implications of doing that kind of thing. You'd want to run
> the idea past some DT maintainers/experts.

It's a bit simpler than that. We would need only to look for the reg
property in a local port subnode. MIPI-CSIS correspond to physical MIPI
CSI-2 bus interface of an SoC, hence it has to have specific reg values
that identify each camera input interface.

csis { /* MIPI CSI-2 Slave */
     ...
     port {
         reg = <1>;  /* e.g. MIPI-CSIS.1 */
         csis_ep: endpoint {
             remote-endpoint = <&img_sensor_ep>;
         }	
     };
};

i2c-controller {
     ...
     image-sensor {  /* MIPI CSI-2 Master */
         ...
         port {
             img_sensor_ep: endpoint {
                 remote-endpoint = <&csis_ep>;
             }	
         };
     };
};

Naturally the image-sensor node represents a device external to an SoC.

> ...
>> I can see aliases used in bindings of multiple devices: uart, spi, sound
>> interfaces, gpio, ... And all bindings seem to impose some rules on how
>> their aliases are created.
>
> Do you have specific examples? I really don't think the bindings should
> be dictating the alias values.

I just grepped through the existing bindings documentation:

gpio/gpio-mxs.txt:Note: Each GPIO port should have an alias correctly 
numbered in "aliases"
gpio/gpio-mxs.txt-node.

serial/fsl-imx-uart.txt:Note: Each uart controller should have an alias 
correctly numbered
serial/fsl-imx-uart.txt:in "aliases" node.

mmc/synopsis-dw-mshc.txt:- All the MSHC controller nodes should be 
represented in the aliases node using
mmc/synopsis-dw-mshc.txt:  the following format 'mshc{n}' where n is a 
unique number for the alias.

sound/mxs-saif.txt:Note: Each SAIF controller should have an alias 
correctly numbered
sound/mxs-saif.txt:in "aliases" node.

spi/spi-samsung.txt:- All the SPI controller nodes should be represented 
in the aliases node using
spi/spi-samsung.txt:  the following format 'spi{n}' where n is a unique 
number for the alias.

tty/serial/fsl-mxs-auart.txt:Note: Each auart port should have an alias 
correctly numbered in "aliases"
tty/serial/fsl-mxs-auart.txt-node.

powerpc/fsl/mpic-msgr.txt:    An alias should be created for every 
message register block.  They are not
powerpc/fsl/mpic-msgr.txt-    required, though.  However, a particular 
implementation of this binding
powerpc/fsl/mpic-msgr.txt:    may require aliases to be present. 
Aliases are of the form
powerpc/fsl/mpic-msgr.txt-    'mpic-msgr-block<n>', where <n> is an 
integer specifying the block's number.
powerpc/fsl/mpic-msgr.txt-    Numbers shall start at 0.

I think "correctly numbered" in the above statements means there are some
specific rules on how the aliases are created, however those seem not
clearly communicated.

And there is a new patch series that allows I2C bus controller enumeration
by means of the aliases:

http://www.spinics.net/lists/arm-kernel/msg224162.html

>>> After all, what happens in some later SoC where you have two different
>>> types of module that feed into the common module, such that type A
>>> sources have IDs 0..3 in the common module, and type B sources have IDs
>>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
>>> for the type B DT nodes.
>>
>> There is no need to write alias ID directly into registers, and actually
>> it doesn't really happen. But we need to know that, for example camera A
>> is connected to physical MIPI CSI-2 channel 0 and to capture video with
>> DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to
>> MIPI-CSIS 0.
>
> OK, so the IDs are selecting which register to write, or which mux
> settings to access. That's pretty much semantically the same thing.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 13, 2013, 8:42 p.m. UTC | #10
On 02/12/2013 03:39 PM, Sylwester Nawrocki wrote:
> On 02/11/2013 10:50 PM, Stephen Warren wrote:
>> On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote:
>>> On 02/09/2013 01:32 AM, Stephen Warren wrote:
>>>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>>>>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>>
>>>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>>>>> +----------------------------------------------
>>>>>> ...
>>>>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>>>>> aliases node.
>>>>>>>>> +Aliases are of the form fimc<n>, where<n>     is an integer
>>>>>>>>> (0...N)
>>>>>>>>> specifying
>>>>>>>>> +the IP's instance index.
>> ...
>>>>> Different compatible values might not work, when for example there
>>>>> are 3 IPs out of 4 of one type and the fourth one of another type.
>>>>> It wouldn't even by really different types, just quirks/little
>>>>> differences between them, e.g. no data path routed to one of other
>>>>> IPs.
>>>>
>>>> I was thinking of using feature-/quirk-oriented properties. For
>>>> example,
>>>> if there's a port on 3 of the 4 devices to connect to some other IP
>>>> block, simply include a boolean property to indicate whether that port
>>>> is present. It would be in 3 of the nodes but not the 4th.
>>>
>>> Yes, I could add several properties corresponding to all members of this
>>> [3] data structure. But still it is needed to clearly identify the IP
>>> block in a set of the hardware instances.
>>
>> Why? What decisions will be made based on the identify of the IP block
>> instance that wouldn't be covered by DT properties that describe which
>> features/bugs/... the IP block instance has?
> 
> The whole subsystem topology is exposed to user space through the Media
> Controller API.

OK, stable user-visible names are a reasonable use for device tree. I
still don't think you should use those user-visible IDs for making any
other kind of decision though.

>>>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>>>>> MIPI-CSIS needs to be written to the FIMC.2 data input control
>>>>> register.
>>>>> Even though MIPI-CSIS.N are same in terms of hardware structure they
>>>>> still
>>>>> need to be distinguished as separate instances.
>>>>
>>>> Oh, so you're using the alias ID as the value to write into the FIMC.2
>>>> register for that. I'm not 100% familiar with aliases, but they seem
>>>> like a more user-oriented naming thing to me, whereas values for
>>>> hooking
>>>> up intra-SoC routing are an unrelated namespace semantically, even if
>>>> the values happen to line up right now. Perhaps rather than a Boolean
>>>> property I mentioned above, use a custom property to indicate the ID
>>>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this
>>>> seems
>>>
>>> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
>>> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
>>> blocks are immutably connected to the SoC camera physical MIPI CSI-2
>>> interfaces, and the physical camera ports have fixed assignment to the
>>> MIPI-CSIS devices..  This way we could drop alias ID for the MIPI-CSIS
>>> nodes. And their instance index that is required for the top level
>>> driver which exposes topology and the routing capabilities to user space
>>> could be restored from the reg property value by subtracting a fixed
>>> offset.
>>
>> I suppose that would work. It feels a little indirect, and I think means
>> that the driver needs to go find some child node defining its end of
>> some link, then find the node representing the other end of the link,
>> then read properties out of that other node to find the value. That
>> seems a little unusual, but I guess it would work. I'm not sure of the
>> long-term implications of doing that kind of thing. You'd want to run
>> the idea past some DT maintainers/experts.
> 
> It's a bit simpler than that. We would need only to look for the reg
> property in a local port subnode. MIPI-CSIS correspond to physical MIPI
> CSI-2 bus interface of an SoC, hence it has to have specific reg values
> that identify each camera input interface.

Oh I see. I guess if a device is using its own node to determine its own
identify, that's reasonable.

I thought you were talking about a situation like:

FIMC <--> XXX

where FIMC wanted to determine what ID XXX knew that particular FIMC as.

>>> I can see aliases used in bindings of multiple devices: uart, spi, sound
>>> interfaces, gpio, ... And all bindings seem to impose some rules on how
>>> their aliases are created.
>>
>> Do you have specific examples? I really don't think the bindings should
>> be dictating the alias values.
> 
> I just grepped through the existing bindings documentation:
...
> I think "correctly numbered" in the above statements means there are some
> specific rules on how the aliases are created, however those seem not
> clearly communicated.

A binding specifying that an alias must (or even should) exist for each
node seems odd to me. In the absence of an explicit rule for how to
determine the alias IDs to use, I think the rule would simply be that
the aliases must be unique?

> And there is a new patch series that allows I2C bus controller enumeration
> by means of the aliases:
> 
> http://www.spinics.net/lists/arm-kernel/msg224162.html

That's not enumerating controllers by alias (they're still enumerated by
scanning the DT nodes for buses in the normal way). The change simply
assigns the bus ID of each controller from an alias; exactly what
aliases are for.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Feb. 14, 2013, 11:03 p.m. UTC | #11
On 02/13/2013 09:42 PM, Stephen Warren wrote:
> On 02/12/2013 03:39 PM, Sylwester Nawrocki wrote:
[...]
>> The whole subsystem topology is exposed to user space through the Media
>> Controller API.
>
> OK, stable user-visible names are a reasonable use for device tree. I
> still don't think you should use those user-visible IDs for making any
> other kind of decision though.

OK, I will update the bindings so all variant details are placed in the
device tree. Then the routing information would mostly be coming from the
device specific dt properties/the common media bindings and the state of
links between the media entities, set by the user.

>> It's a bit simpler than that. We would need only to look for the reg
>> property in a local port subnode. MIPI-CSIS correspond to physical MIPI
>> CSI-2 bus interface of an SoC, hence it has to have specific reg values
>> that identify each camera input interface.
>
> Oh I see. I guess if a device is using its own node to determine its own
> identify, that's reasonable.

OK, I'm going to post an updated patch series in a week or two.

> I thought you were talking about a situation like:
>
> FIMC <--> XXX
>
> where FIMC wanted to determine what ID XXX knew that particular FIMC as.

Ah, no. Sorry for the poor explanation. FIMC are on a sort if interconnect
bus and they can be attached to a single data source, even in parallel,
and the data source entity don't even need to be fully aware of it.

>>>> I can see aliases used in bindings of multiple devices: uart, spi, sound
>>>> interfaces, gpio, ... And all bindings seem to impose some rules on how
>>>> their aliases are created.
>>>
>>> Do you have specific examples? I really don't think the bindings should
>>> be dictating the alias values.
>>
>> I just grepped through the existing bindings documentation:
> ...
>> I think "correctly numbered" in the above statements means there are some
>> specific rules on how the aliases are created, however those seem not
>> clearly communicated.
>
> A binding specifying that an alias must (or even should) exist for each
> node seems odd to me. In the absence of an explicit rule for how to
> determine the alias IDs to use, I think the rule would simply be that
> the aliases must be unique?

I guess so. Inspecting of_alias_get_id() call sites tells us that most 
drivers
just fail when alias is not present and only rarely it is not treated as an
error condition.

>> And there is a new patch series that allows I2C bus controller enumeration
>> by means of the aliases:
>>
>> http://www.spinics.net/lists/arm-kernel/msg224162.html
>
> That's not enumerating controllers by alias (they're still enumerated by
> scanning the DT nodes for buses in the normal way). The change simply
> assigns the bus ID of each controller from an alias; exactly what
> aliases are for.

OK, that clarifies a bit my understanding of the aliases.

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
new file mode 100644
index 0000000..916b2c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -0,0 +1,72 @@ 
+Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
+----------------------------------------------
+
+The Exynos Camera subsystem comprises of multiple sub-devices that are
+represented by separate platform devices. Some of the IPs come in different
+variants across the SoC revisions (FIMC) and some remain mostly unchanged
+(MIPI CSIS, FIMC-LITE).
+
+All those sub-subdevices are defined as parent nodes of the common device
+node, which also includes common properties of the whole subsystem not really
+specific to any single sub-device, like common camera port pins or external
+clocks for image sensors attached to the SoC.
+
+Common 'camera' node
+--------------------
+
+Required properties:
+
+- compatible	   : must be "samsung,fimc", "simple-bus"
+
+The 'camera' node must include at least one 'fimc' child node.
+
+
+'fimc' device nodes
+-------------------
+
+Required properties:
+
+- compatible : "samsung,s5pv210-fimc" for S5PV210,
+	       "samsung,exynos4210-fimc" for Exynos4210,
+	       "samsung,exynos4212-fimc" for Exynos4212/4412 SoCs;
+- reg	     : physical base address and size of the device memory mapped
+	       registers;
+- interrupts : FIMC interrupt to the CPU should be described here;
+
+For every fimc node a numbered alias should be present in the aliases node.
+Aliases are of the form fimc<n>, where <n> is an integer (0...N) specifying
+the IP's instance index.
+
+Optional properties
+
+ - clock-frequency - maximum FIMC local clock (LCLK) frequency
+
+Example:
+
+	aliases {
+		csis0 = &csis_0;
+		fimc0 = &fimc_0;
+	};
+
+	camera {
+		compatible = "samsung,fimc", "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		status = "okay";
+
+		fimc_0: fimc@11800000 {
+			compatible = "samsung,exynos4210-fimc";
+			reg = <0x11800000 0x1000>;
+			interrupts = <0 85 0>;
+			status = "okay";
+		};
+
+		csis_0: csis@11880000 {
+			compatible = "samsung,exynos4210-csis";
+			reg = <0x11880000 0x1000>;
+			interrupts = <0 78 0>;
+			max-data-lanes = <4>;
+		};
+	};
+
+[1] Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index f822b8e..bc84301 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -1883,7 +1883,7 @@  int fimc_initialize_capture_subdev(struct fimc_dev *fimc)
 
 	v4l2_subdev_init(sd, &fimc_subdev_ops);
 	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
-	snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->pdev->id);
+	snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->id);
 
 	fimc->vid_cap.sd_pads[FIMC_SD_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
 	fimc->vid_cap.sd_pads[FIMC_SD_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c b/drivers/media/platform/s5p-fimc/fimc-core.c
index 29f7bb7..07ca0e0 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.c
+++ b/drivers/media/platform/s5p-fimc/fimc-core.c
@@ -21,6 +21,8 @@ 
 #include <linux/pm_runtime.h>
 #include <linux/list.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
 #include <media/v4l2-ioctl.h>
@@ -863,45 +865,57 @@  static int fimc_m2m_resume(struct fimc_dev *fimc)
 	return 0;
 }
 
+static const struct of_device_id fimc_of_match[];
+
 static int fimc_probe(struct platform_device *pdev)
 {
-	const struct fimc_drvdata *drv_data = fimc_get_drvdata(pdev);
-	struct s5p_platform_fimc *pdata;
+	struct fimc_drvdata *drv_data = NULL;
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id;
+	u32 lclk_freq = 0;
 	struct fimc_dev *fimc;
 	struct resource *res;
 	int ret = 0;
 
-	if (pdev->id >= drv_data->num_entities) {
-		dev_err(&pdev->dev, "Invalid platform device id: %d\n",
-			pdev->id);
-		return -EINVAL;
-	}
-
-	fimc = devm_kzalloc(&pdev->dev, sizeof(*fimc), GFP_KERNEL);
+	fimc = devm_kzalloc(dev, sizeof(*fimc), GFP_KERNEL);
 	if (!fimc)
 		return -ENOMEM;
 
-	fimc->id = pdev->id;
+	if (dev->of_node) {
+		of_id = of_match_node(fimc_of_match, dev->of_node);
+		if (of_id)
+			drv_data = (struct fimc_drvdata *)of_id->data;
+		fimc->id = of_alias_get_id(dev->of_node, "fimc");
+
+		of_property_read_u32(dev->of_node, "clock-frequency",
+							&lclk_freq);
+	} else {
+		drv_data = fimc_get_drvdata(pdev);
+		fimc->id = pdev->id;
+	}
+
+	if (!drv_data || fimc->id < 0 || fimc->id >= drv_data->num_entities) {
+		dev_err(dev, "Invalid driver data or device index (%d)\n",
+			fimc->id);
+		return -EINVAL;
+	}
 
 	fimc->variant = drv_data->variant[fimc->id];
 	fimc->pdev = pdev;
-	pdata = pdev->dev.platform_data;
-	fimc->pdata = pdata;
-
 	init_waitqueue_head(&fimc->irq_queue);
 	spin_lock_init(&fimc->slock);
 	mutex_init(&fimc->lock);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	fimc->regs = devm_request_and_ioremap(&pdev->dev, res);
+	fimc->regs = devm_request_and_ioremap(dev, res);
 	if (fimc->regs == NULL) {
-		dev_err(&pdev->dev, "Failed to obtain io memory\n");
+		dev_err(dev, "Failed to obtain io memory\n");
 		return -ENOENT;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (res == NULL) {
-		dev_err(&pdev->dev, "Failed to get IRQ resource\n");
+		dev_err(dev, "Failed to get IRQ resource\n");
 		return -ENXIO;
 	}
 
@@ -909,7 +923,10 @@  static int fimc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = clk_set_rate(fimc->clock[CLK_BUS], drv_data->lclk_frequency);
+	if (lclk_freq == 0)
+		lclk_freq = drv_data->lclk_frequency;
+
+	ret = clk_set_rate(fimc->clock[CLK_BUS], lclk_freq);
 	if (ret < 0)
 		return ret;
 
@@ -917,10 +934,10 @@  static int fimc_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	ret = devm_request_irq(&pdev->dev, res->start, fimc_irq_handler,
-			       0, dev_name(&pdev->dev), fimc);
+	ret = devm_request_irq(dev, res->start, fimc_irq_handler,
+			       0, dev_name(dev), fimc);
 	if (ret) {
-		dev_err(&pdev->dev, "failed to install irq (%d)\n", ret);
+		dev_err(dev, "failed to install irq (%d)\n", ret);
 		goto err_clk;
 	}
 
@@ -929,27 +946,26 @@  static int fimc_probe(struct platform_device *pdev)
 		goto err_clk;
 
 	platform_set_drvdata(pdev, fimc);
-	pm_runtime_enable(&pdev->dev);
-	ret = pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		goto err_sd;
 	/* Initialize contiguous memory allocator */
-	fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+	fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
 	if (IS_ERR(fimc->alloc_ctx)) {
 		ret = PTR_ERR(fimc->alloc_ctx);
 		goto err_pm;
 	}
 
-	dev_dbg(&pdev->dev, "FIMC.%d registered successfully\n", fimc->id);
+	dev_dbg(dev, "FIMC.%d registered successfully\n", fimc->id);
 
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 	return 0;
 err_pm:
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 err_sd:
 	fimc_unregister_capture_subdev(fimc);
 err_clk:
-	clk_disable(fimc->clock[CLK_BUS]);
 	fimc_clk_put(fimc);
 	return ret;
 }
@@ -1258,10 +1274,24 @@  static const struct platform_device_id fimc_driver_ids[] = {
 		.name		= "exynos4x12-fimc",
 		.driver_data	= (unsigned long)&fimc_drvdata_exynos4x12,
 	},
-	{},
+	{ },
 };
 MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
 
+static const struct of_device_id fimc_of_match[] = {
+	{
+		.compatible = "samsung,s5pv210-fimc",
+		.data = &fimc_drvdata_s5pv210,
+	}, {
+		.compatible = "samsung,exynos4210-fimc",
+		.data = &fimc_drvdata_exynos4210,
+	}, {
+		.compatible = "samsung,exynos4212-fimc",
+		.data = &fimc_drvdata_exynos4x12,
+	},
+	{ /* sentinel */ },
+};
+
 static const struct dev_pm_ops fimc_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(fimc_suspend, fimc_resume)
 	SET_RUNTIME_PM_OPS(fimc_runtime_suspend, fimc_runtime_resume, NULL)
@@ -1272,9 +1302,10 @@  static struct platform_driver fimc_driver = {
 	.remove		= fimc_remove,
 	.id_table	= fimc_driver_ids,
 	.driver = {
-		.name	= FIMC_MODULE_NAME,
-		.owner	= THIS_MODULE,
-		.pm     = &fimc_pm_ops,
+		.of_match_table = fimc_of_match,
+		.name		= FIMC_MODULE_NAME,
+		.owner		= THIS_MODULE,
+		.pm     	= &fimc_pm_ops,
 	}
 };