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 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? -- Regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
