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

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
  

Hi Arun,

On 05/31/2013 03:03 PM, Arun Kumar K wrote:

Please add at least one sentence here. All in all this patch
adds DT binding documentation for a fairly complex subsystem.

And please Cc devicetree-discuss@lists.ozlabs.org next time.

> 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 @@
> +Samsung EXYNOS SoC Camera Subsystem

Shouldn't it be, e.g.:

Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS)

Or do you intend this file to be describing also the other sub-devices,
like GScaler ?

> +-----------------------------------
> +
> +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.

> +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) ?

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

Hi Sylwester,

Thank you for the review and sorry for the delayed response.

On Fri, Jun 21, 2013 at 4:15 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi Arun,
>
> On 05/31/2013 03:03 PM, Arun Kumar K wrote:
>
> Please add at least one sentence here. All in all this patch
> adds DT binding documentation for a fairly complex subsystem.
>
> And please Cc devicetree-discuss@lists.ozlabs.org next time.
>

Ok will do that.

>
>> 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 @@
>> +Samsung EXYNOS SoC Camera Subsystem
>
>
> Shouldn't it be, e.g.:
>
> Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS)
>
> Or do you intend this file to be describing also the other sub-devices,
> like GScaler ?
>

Probably not. WIll change it to Imaging subsystem.

>
>> +-----------------------------------
>> +
>> +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.

>
>> +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.
I2C bus is used by the sensor which has its own node. May be I should
explain one of the sensor nodes over here?

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Sylwester,

On Wed, Jul 17, 2013 at 2:53 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> 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.
>

Ok. Time being I will move it to the camera node pinctrl properties.

> 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.
>

Ok that would be great. I will refer your exynos4 implementation for doing this.

>
>>>> +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.
>

Yes its going to be used  by the firmware and is done for sensor
calibration. The sensor works well even without it and so I didnt
include it in my initial patchset and kept it as a todo.

>
>> 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.
>

I havent created a dummy i2c device driver for handling the i2c clock
part and everything was handled in the fimc-is-sensor itself.
I was checking your exynos4 implementation and will try to do in similar
lines.

> 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.
>

Yes I hope it can be re-used.

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

Ok thanks.

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