Hi Sakari On 16 June 2017 at 10:57, Sakari Ailus <sakari.ailus@xxxxxx> wrote: > Hi Dave, > > On Thu, Jun 15, 2017 at 05:15:04PM +0100, Dave Stevenson wrote: >> Hi Sakari. >> >> Thanks for the review. >> >> On 15 June 2017 at 13:59, Sakari Ailus <sakari.ailus@xxxxxx> 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@xxxxxxxxxxxxxxx> >> >> --- >> >> .../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. > > This information should be present in the DT. I.e. the data-lanes property. > > v4l2_fwnode_endpoint_alloc_parse() can obtain that from DT, among other > things (I haven't checked the second patch yet). I was interpreting data-lanes as being a function of the CSI-2 source only, not the CSI-2 sink. I haven't seen any examples where it has been it has been set on a sink, but if that is valid then it sounds like a sensible thing to do. ... OK, I'd missed it in video_interfaces.txt where it has been set for the sh-mobile-csi2 endpoint (not that there appears to be a driver advertising sh-mobile-csi2 as a compatible string anymore - it was removed in 4.9). It sounds like adopting a sink endpoint property of "data-lanes" is reasonable. Make it optional with the driver adopting a default "<1 2>" and you'll cover 99% of the cases. On Compute Modules it can be overridden to "<1 2 3 4>" for Unicam1. Selecting more lanes on Unicam0 is just an error that the DT author has to sort out. >> >> 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. > > I'd expect things not to work if there's a grave error in DT. I guess you > could assume something but the fact is that you know you don't know. >> >> > 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. > > data-lanes, among other things? Looking through struct v4l2_fwnode_bus_mipi_csi2 for the parsed result of v4l2_fwnode_endpoint_parse_csi_bus: - V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK / V4L2_MBUS_CSI2_CONTINUOUS_CLOCK are the only flags set. The clock is generated by the CSI2 source, so not relevant to configure it on the sink. - data_lane has been discussed above. Lane reordering is not supported on the hardware so only the number of entries is of any importance, although consecutive lanes can be checked. If a default is adopted by the driver, then it is not mandatory. - the clock lane is on a dedicated pair of pins, so it will always be <0>. Little point in checking it. - num_data_lanes comes out of data-lane. Let the driver adopt a default. - lane polarity inversion is not supported by the hardware, so it will always be an array of 0's. Little point in checking. "data-lanes" can be mandatory if you think that is of any benefit, but personally I don't see it. The other properties give no benefit, so I think I am correct with only remote-endpoint being present. Thanks, Dave
