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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sylwester,

On Wed, Jul 17, 2013 at 2:53 AM, Sylwester Nawrocki
<sylvester.nawrocki@xxxxxxxxx> 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@xxxxxxxxx>  wrote:
>>>
>>> On 05/31/2013 03:03 PM, Arun Kumar K wrote:
>
> [...]
>
>>>> Signed-off-by: Arun Kumar K<arun.kk@xxxxxxxxxxx>
>>>> ---
>>>>    .../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-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux