Re: [PATCH v1 09/11] DT: Add documentation for exynos4-is 'flashes' property

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

Hello,

On 25/03/15 09:52, Jacek Anaszewski wrote:
> On 03/25/2015 02:06 AM, Sakari Ailus wrote:
>> On Fri, Mar 20, 2015 at 04:03:29PM +0100, Jacek Anaszewski wrote:
>>> This patch adds a description of 'flashes' property
>>> to the samsung-fimc.txt.
>>>
>>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> ---
>>>   .../devicetree/bindings/media/samsung-fimc.txt     |    8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>>> index 922d6f8..cb0e263 100644
>>> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
>>> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>>> @@ -40,6 +40,13 @@ should be inactive. For the "active-a" state the camera port A must be activated
>>>   and the port B deactivated and for the state "active-b" it should be the other
>>>   way around.
>>>
>>> +Optional properties:
>>> +
>>> +- flashes - Array of phandles to the flash LEDs that can be controlled by the
>>> +	    sub-devices contained in this media device. Flash LED is
>>> +	    represented by a child node of a flash LED device
>>
>> This should be in
>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>>
>> Should flash devices be associated with sensors somehow rather than ISPs?
>> That's how they commonly are arranged, however that doesn't limit placing
>> them in silly places.
>>
>> I'm not necessarily saying the flashes-property should be present in
>> sensor's DT nodes, but it'd be good to be able to make the association if
>> it's there.

IMHO 'flashes' is a misleading name, these are simply high brightness LEDs
which can work as camera flash or auxiliary light for camcording, in context of
a camera device.

The led DT nodes which the entries of above flashes property is pointing to
have a text label, which presumably could be used to associate a LED device
with an image sensor. That said, I think we should allow a property as above
'flashes' be placed in aggregate camera node and also in sensor device node.
I think it should be left to the bridge/ISP binding to choose one option or
the other.

For now I would propose to rename the "flashes" property to "samsung,leds" or
"leds" and leave it in "camera" node.

> I know of a SoC, which drives the flash from its on-chip ISP. The GPIO
> connected to the flash controller's external strobe pin can be
> configured so that the signal is routed to it from the ISP or from
> CPU (for software strobe mode).
> 
> I think that Sylwester could say more in this subject.
> 
> 
>>> +	    (see Documentation/devicetree/bindings/leds/common.txt).
>>> +
>>>   The 'camera' node must include at least one 'fimc' child node.
>>>
>>>
>>> @@ -166,6 +173,7 @@ Example:
>>>   		clock-output-names = "cam_a_clkout", "cam_b_clkout";
>>>   		pinctrl-names = "default";
>>>   		pinctrl-0 = <&cam_port_a_clk_active>;
>>> +		flashes = <&camera_flash>, <&system_torch>;
>>>   		status = "okay";
>>>   		#address-cells = <1>;
>>>   		#size-cells = <1>;
>>
>> There will be other kind of devices that have somewhat similar relationship.
>> They just haven't been defined yet. Lens controllers or EEPROM for instance.
>> The two are an integral part of a module, something which is not modelled in
>> DT in any way, but perhaps should be.

Indeed, I'd say it belongs to a particular image sensor (camera module) binding
to describe each its physical subdevices, i.e. if a pointer to lens or EEPROM
is needed in the main module DT node.

> Do you suggest using more generic name than 'flashes'?


-- 
Regards,
Sylwester