Re: [PATCH v8 3/10] Documentation: devicetree: Update Samsung FIMC DT binding

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

Hi Mark,

Thanks for your review.

On 18/03/14 11:02, Mark Rutland wrote:
> On Tue, Mar 11, 2014 at 04:34:30PM +0000, Sylwester Nawrocki wrote:
>> > This patch documents following updates of the Exynos4 SoC camera subsystem
>> > devicetree binding:
>> > 
>> >  - addition of #clock-cells and clock-output-names properties to 'camera'
>> >    node - these are now needed so the image sensor sub-devices can reference
>> >    clocks provided by the camera host interface,
>> >  - dropped a note about required clock-frequency properties at the
>> >    image sensor nodes; the sensor devices can now control their clock
>> >    explicitly through the clk API and there is no need to require this
>> >    property in the camera host interface binding.
>> > 
>> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> > ---
>> > Changes since v7:
>> >  - dropped a note about clock-frequency property in sensor nodes.
>> > 
>> > Changes since v6:
>> >  - #clock-cells, clock-output-names documented as mandatory properties;
>> >  - renamed "cam_mclk_{a,b}" to "cam_{a,b}_clkout in the example dts,
>> >    this now matches changes in exynos4.dtsi further in the patch series;
>> >  - marked "samsung,camclk-out" property as deprecated.
>> > 
>> > Changes since v5:
>> >  - none.
>> > 
>> > Changes since v4:
>> >  - dropped a requirement of specific order of values in clocks/
>> >    clock-names properties (Mark) and reference to clock-names in
>> >    clock-output-names property description (Mark).
>> > ---
>> >  .../devicetree/bindings/media/samsung-fimc.txt     |   44 +++++++++++++-------
>> >  1 file changed, 29 insertions(+), 15 deletions(-)
>> > 
>> > diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> > index 96312f6..922d6f8 100644
>> > --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> > +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> > @@ -15,11 +15,21 @@ Common 'camera' node
>> > 
>> >  Required properties:
>> > 
>> > -- compatible	: must be "samsung,fimc", "simple-bus"
>
> While it was already the case, why is "samsung,fimc" also a simple bus?

IIRC the main reason was to have children automatically instantiated
in Linux, also "samsung,fimc" is a master node gathering the all related
camera subsystem memory mapped IP blocks.

> If it has any properties which are required for the correct use of the
> child nodes, it is _not_ a simple-bus.

Then this can be considered a separate issue, and could be addressed
in a separate patch ?

>> > -- clocks	: list of clock specifiers, corresponding to entries in
>> > -		  the clock-names property;
>> > -- clock-names	: must contain "sclk_cam0", "sclk_cam1", "pxl_async0",
>> > -		  "pxl_async1" entries, matching entries in the clocks property.
>> > +- compatible: must be "samsung,fimc", "simple-bus"
>> > +- clocks: list of clock specifiers, corresponding to entries in
>> > +  the clock-names property;
>> > +- clock-names : must contain "sclk_cam0", "sclk_cam1", "pxl_async0",
>> > +  "pxl_async1" entries, matching entries in the clocks property.
>> > +
>> > +- #clock-cells: from the common clock bindings (../clock/clock-bindings.txt),
>> > +  must be 1. A clock provider is associated with the 'camera' node and it should
>> > +  be referenced by external sensors that use clocks provided by the SoC on
>> > +  CAM_*_CLKOUT pins. The clock specifier cell stores an index of a clock.
>> > +  The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks respectively.
>> > +
>> > +- clock-output-names: from the common clock bindings, should contain names of
>> > +  clocks registered by the camera subsystem corresponding to CAM_A_CLKOUT,
>> > +  CAM_B_CLKOUT output clocks respectively.
>
> And if we're adding more stuff required by child nodes it's definitely
> not a simple-bus.
> 
> Get rid of simple-bus. Get the driver for "samsung,fimc" to probe its
> children once it has set up.
> 
> Apologies for the late reply, and sorry to have to be negative on this.

While I agree with you that the "simple-bus" might not be a best idea,
I can't see how your suggestion could be now implemented in Linux, since
AFAIK there is no API like of_platform_unpopulate(), so device objects
can be removed upon the driver module removal.

--
Regards,
Sylwester