[RFC 1/2] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Hi Sakari.

Thanks for the review.

On 15 June 2017 at 13:59, Sakari Ailus <sakari.ailus at iki.fi> wrote:
> Hi Dave,
>
> Thanks for the set!
>
> On Wed, Jun 14, 2017 at 04:15:46PM +0100, Dave Stevenson wrote:
>> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> (known as Unicam) on BCM283x SoCs.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson at raspberrypi.org>
>> ---
>>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> new file mode 100644
>> index 0000000..cc5a451
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> @@ -0,0 +1,76 @@
>> +Broadcom BCM283x Camera Interface (Unicam)
>> +------------------------------------------
>> +
>> +The Unicam block on BCM283x SoCs is the receiver for either
>> +CSI-2 or CCP2 data from image sensors or similar devices.
>> +
>> +Required properties:
>> +===================
>> +- compatible : must be "brcm,bcm2835-unicam".
>> +- reg                : physical base address and length of the register sets for the
>> +               device.
>> +- interrupts : should contain the IRQ line for this Unicam instance.
>> +- clocks     : list of clock specifiers, corresponding to entries in
>> +               clock-names property.
>> +- clock-names        : must contain an "lp_clock" entry, matching entries
>> +               in the clocks property.
>> +
>> +Optional properties
>> +===================
>> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
>> +               This value is the maximum number supported by this instance.
>> +               Known values of 2 or 4. Default is 2.
>
> Please use "data-lanes" endpoint property instead. This is the number of
> connected physical lanes and specific to the hardware.

I'll rethink/test, but to explain what I was intending to achieve:

Registers UNICAM_DAT2 and UNICAM_DAT3 are not valid for instances of
the hardware that only have two lanes instantiated in silicon.
In the case of the whole BCM283x family, Unicam0 ony has 2 lanes
instantiated, whilst Unicam1 has the maximum 4 lanes. (Lower
resolution front cameras would connect to Unicam0, whilst the higher
resolution back cameras would go to Unicam1).

To further confuse matters, on the Pi platforms (other than the
Compute Module), it is Unicam1 that is brought out to the camera
connector but with only 2 lanes wired up.

I was intending to make it possible for the driver to avoid writing to
invalid registers, and also describe the platform limitations to allow
sanity checking.
I haven't tested against Unicam0 as yet to see what actually happens
if we try to write UNICAM_DAT2 or UNICAM_DAT3. If the hardware
silently swallows it then that requirement is null and void. I'll do
some testing tomorrow.
The second bit comes down to how friendly an error you want should
someone write an invalid DT with 'data-lanes' greater than can be
supported by the platform.

> Could you also document which endpoint properties are mandatory and which
> ones optional?

Will do, although I'm not sure there are any required properties.

>> +
>> +
>> +Unicam supports a single port node. It should contain one 'port' child node
>> +with child 'endpoint' node. Please refer to the bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +     csi1: csi at 7e801000 {
>> +             compatible = "brcm,bcm2835-unicam";
>> +             reg = <0x7e801000 0x800>,
>> +                   <0x7e802004 0x4>;
>> +             interrupts = <2 7>;
>> +             clocks = <&clocks BCM2835_CLOCK_CAM1>;
>> +             clock-names = "lp_clock";
>> +
>> +             port {
>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +
>> +                     endpoint {
>> +                             remote-endpoint = <&tc358743_0>;
>> +
>
> Extra newline. Don't you need any other properties here?

Newline done.
As above, I don't believe there are any other properties required, but
will double check. What extras were you expecting to see there?

>> +                     };
>> +             };
>> +     };
>> +
>> +     i2c0: i2c at 7e205000 {
>> +
>> +             tc358743: tc358743 at 0f {
>> +                     compatible = "toshiba,tc358743";
>> +                     reg = <0x0f>;
>> +                     status = "okay";
>> +
>> +                     clocks = <&tc358743_clk>;
>> +                     clock-names = "refclk";
>> +
>> +                     tc358743_clk: bridge-clk {
>> +                             compatible = "fixed-clock";
>> +                             #clock-cells = <0>;
>> +                             clock-frequency = <27000000>;
>> +                     };
>> +
>> +                     port {
>> +                             tc358743_0: endpoint {
>> +                                     remote-endpoint = <&csi1>;
>
> This one needs to refer to the endpoint, just as the one in the CSI-2
> receiver does.

OK.
(I'm suspecting very few drivers actually follow that link, but I'll correct).

>> +                                     clock-lanes = <0>;
>> +                                     data-lanes = <1 2 3 4>;

Oops, DT author beware! That should only have 2 lanes defined (not
that the TC358743 driver looks at it beyond checking it is non-zero).

>> +                                     clock-noncontinuous;
>> +                                     link-frequencies =
>> +                                             /bits/ 64 <297000000>;
>> +                             };
>> +                     };
>> +             };
>> +     };
>
> --
> Kind regards,
>
> Sakari Ailus
> e-mail: sakari.ailus at iki.fi     XMPP: sailus at retiisi.org.uk