Re: [RFC v2 01/10] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Arun,

On 07/09/2013 01:08 PM, Arun Kumar K wrote:
> On Fri, Jun 21, 2013 at 4:15 AM, Sylwester Nawrocki
> <sylvester.nawrocki@gmail.com>  wrote:
>> On 05/31/2013 03:03 PM, Arun Kumar K wrote:
[...]
>>> Signed-off-by: Arun Kumar K<arun.kk@samsung.com>
>>> ---
>>>    .../devicetree/bindings/media/exynos5-fimc-is.txt  |   41
>>> ++++++++++++++++++++
>>>    1 file changed, 41 insertions(+)
>>>    create mode 100644
>>> Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
>>> b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
>>> new file mode 100644
>>> index 0000000..9fd4646
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
>>> @@ -0,0 +1,41 @@
[...]
>>> +-----------------------------------
>>> +
>>> +The camera subsystem on Samsung Exynos5 SoC has some changes relative
>>> +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and
>>> +FIMC-LITE IPs but has a much improved version of FIMC-IS which can
>>> +handle sensor controls and camera post-processing operations. The
>>> +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many
>>> +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two
>>> +dedicated scalers (SCC and SCP).
>>> +
>>> +fimc-is node
>>> +------------
>>> +
>>> +Required properties:
>>> +
>>> +- compatible        : must be "samsung,exynos5250-fimc-is"
>>> +- reg               : physical base address and size of the memory mapped
>>> +                      registers
>>> +- interrupt-parent  : Parent interrupt controller
>>> +- interrupts        : fimc-is interrupt to the parent combiner
>>> +- clocks            : list of clock specifiers, corresponding to entries
>>> in
>>> +                      clock-names property;
>>> +- clock-names       : must contain "isp", "mcu_isp", "isp_div0",
>>> "isp_div1",
>>> +                      "isp_divmpwm", "mcu_isp_div0", "mcu_isp_div1"
>>> entries,
>>> +                      matching entries in the clocks property.
>>> +
>>> +
>>> +Board specific properties:
>>> +
>>> +- pinctrl-names    : pinctrl names for camera port pinmux control, at
>>> least
>>> +                    "default" needs to be specified.
>>> +- pinctrl-0...N           : pinctrl properties corresponding to
>>> pinctrl-names
>>
>>
>> What pins exactly are supposed to be covered by these properties ? For what
>> devices ? Aren't the camera port pins supposed to be specified at the common
>> 'camera' node ? I believe the camera ports are not specific to the FIMC-IS.
>
> These are for the sensor controls (especially clock lines).
> I think I should move these to the sensor node.

This doesn't sound right either. These pins are not a property of an 
external
image sensor device, they are specific to the AP SoC. So IMO these pinctrl
properties belong to some SoC's internal device node.

I think we could add a clock provider for the sclk_cam clocks and then the
pinmux of those clock outputs could be configurable from with the clock 
ops.
E.g. we set the pinumx into CAM_?_CLKOUT function only when a clock is 
enabled.
Disabling a clock would put CLKOUT pin pinmux e.g. into input with pull 
down
state. This would ensure proper CLKOUT pin configuration when image 
sensor is
suspended or entirely powered off. I'm working on something like this for
exynos4.

>>> +pmu subnode
>>> +-----------
>>> +
>>> +Required properties:
>>> + - reg : should contain PMU physical base address and size of the memory
>>> +         mapped registers.
>>
>>
>> What about other devices, like ISP I2C, SPI ? Don't you want to list at
>> least
>> the ones currently used (I2C bus controllers) ?
>>
>
> The present driver doesnt make use of the SPI bus as its used only
> for sensor calibration which is not yet added.

Is it only going to be used by the Cortex-A5 firmware, similarly to the
I2C bus ? If so then it is likely not needed to specify it here right now.
But I believe for complete H/W description we should reserve a possibility
to add those various peripheral device nodes here.

> I2C bus is used by the sensor which has its own node. May be I should
> explain one of the sensor nodes over here?

I would describe at least I2C bus controller node and add a note that image
sensor nodes can be specified there.

Also, it would be good to start adding separate sensor drivers for each
sensor, like s5k6a3, s5k4e3, etc. Ideally we should not have duplicated
fimc-is-sensor.[ch] that would handle various sensors, i.e. same set of
sensors to drivers/media/platform/exynos4-is and drivers/media/platform/
exynos5-is.

After you post the next iteration of this series I could have a look how
it could be done.

--
Thanks,
Sylwester