On 01/21/2014 12:27 AM, Sebastian Reichel wrote:
On Mon, Jan 20, 2014 at 11:16:43PM +0100, Sylwester Nawrocki wrote:
On 01/20/2014 05:19 AM, Sakari Ailus wrote:
[...]
- #address-cells: Should be set to<1>.
- #size-cells : Should be set to<0>.
The ISP also exports clocks. Shouldn't you add
#clock-cells =<1>;
Ok. I already though about that possibility, but wasn't sure which
way is the cleaner one. Thanks for clarifying.
[...]
This doesn't seem to follow the common clock bindings.
I think it does follow common clock bindings at least. Clocks can
referenced with the following statement:
camera-sensor-0 {
clocks =<&isp_xclk1>;
clock-names = ...
};
Yes, sorry, I think you're right. I guess it was just #clock-cells not
being used optimally.
Instead, you could define value of #clock-cells to be 1 and allow clocks
consumers to reference the provider node in a standard way, e.g.:
This also works and probably better. I will merge clock provider
into the main omap3isp node.
[...]
endpoint subnode for serial interfaces
--------------------------------------
Required properties:
- ti,isp-interface-type : should be one of the following values
I think the interface type should be standardised at V4L2 level. We
currently do not do that, but instead do a little bit of guessing.
I'm all for such a standard property. It seems much more clear to use such
a property. And I already run into issues with deriving the bus interface
type from existing properties, please see https://linuxtv.org/patch/19937
I assume it would be fine to add a string type property like
"interface-type"
or "bus-type".
*<0> to use the phy in CSI mode
*<1> to use the phy in CCP mode
*<2> to use the phy in CCP mode, but configured for MIPI CSI2
mh... from what I understand a port can be configured to be either
CSI2 or CPP2 type. If CCP2 type is chosen the port can be configured
to be CSI1 mode instead of actually being CPP2. See
see "struct isp_ccp2_platform_data" in include/media/omap3isp.h.
But actually I made a typo above. This is what I meant:
*<0> to use the phy in MIPI CSI2 mode
*<1> to use the phy in SMIA CCP2 mode
*<2> to use the phy in SMIA CCP2 mode, but configured for MIPI CSI1
I'm not sure if this can be properly be described in a standardized
type property.
Hmm, are there any other combinations involving MIPI CSI1 ?
Couldn't we just say that there are: MIPI CSI2, SMIA CCP2 and MIPI CSI1
protocols/bus types used ?
[...]
- data-lanes: an array of physical data lane indexes. Position of an entry
determines the logical lane number, while the value of an entry indicates
physical lane, e.g. for 2-lane MIPI CSI-2 bus we could have
"data-lanes =<1 2>;", assuming the clock lane is on hardware lane 0.
This property is valid for serial busses only (e.g. MIPI CSI-2).
- clock-lanes: an array of physical clock lane indexes. Position of an entry
determines the logical lane number, while the value of an entry indicates
physical lane, e.g. for a MIPI CSI-2 bus we could have "clock-lanes =<0>;",
which places the clock lane on hardware lane 0. This property is valid for
serial busses only (e.g. MIPI CSI-2). Note that for the MIPI CSI-2 bus this
array contains only one entry.
I'd rather refer to
Documentation/devicetree/bindings/media/video-interfaces.txt than copy from
it. It's important that there's a single definition for the standard
properties. Just mentioning the property by name should be enough. What do
you think?
+1
sounds fine to me. Something like this?
- data-lanes: see [0]
- clock-lanes: see [0]
[0] Documentation/devicetree/bindings/media/video-interfaces.txt
I guess it would be fine.
[...]
camera-switch {
/*
* TODO:
* - check if the switching code is generic enough to use a
* more generic name like "gpio-camera-switch".
* - document the camera-switch binding
*/
compatible = "nokia,n900-camera-switch";
Indeed. I don't think the hardware engineers realised what kind of a long
standing issue they created for us when they chose that solution. ;)
Writing a small driver for this that exports a sub-device would probably be
the best option as this is hardly very generic. Should this be shown to the
user space or not? Probably it'd be nice to avoid showing the related sub-device
if there would be one.
Probably we should avoid exposing such a hardware detail to user space.
OTOH it would be easy to handle as a media entity through the media
controller API.
If this is exposed to the userspace, then a userspace application
"knows", that it cannot use both cameras at the same time. Otherwise
it can just react to error messages when it tries to use the second
camera.
Indeed, that's a good argument, I forgot about it for a while.
I'm still trying to get N9 support working first, the drivers are in a
better shape and there are no such hardware hacks.
gpios =<&gpio4 1>; /* 97 */
I think the binding should be defining how state of the GPIO corresponds
to state of the mux.
Obviously it should be mentioned in the n900-camera-switch binding
Documentation. This document was just the proposal for the omap3isp
node :)
Huh, I wasn't reading carefully enough! Then since it is just about the
OMAP3 ISP it might be a good idea to drop the switch from the example,
it seems unrelated.
port@0 {
switch_in: endpoint {
remote-endpoint =<&csi1_ep>;
};
switch_out1: endpoint {
remote-endpoint =<&et8ek8>;
};
switch_out2: endpoint {
remote-endpoint =<&smiapp_dfl>;
};
};
This won't work, since names of the nodes are identical they will be
combined by the dtc into a single 'endpoint' node with single
'remote-endpoint' property
- might not be exactly something that you want.
So it could be rewritten like:
right.
[...]
However, simplifying a bit, the 'endpoint' nodes are supposed to describe
the configuration of a bus interface (port) for a specific remote device.
Then what you need might be something like:
camera-switch {
compatible = "nokia,n900-camera-switch";
#address-cells =<1>;
#size-cells =<0>;
switch_in: port@0 {
reg =<0>;
endpoint {
remote-endpoint =<&csi1_ep>;
};
};
switch_out1: port@1 {
reg =<1>;
endpoint {
remote-endpoint =<&et8ek8>;
};
};
switch_out2: port@2 {
endpoint {
reg =<2>;
remote-endpoint =<&smiapp_dfl>;
};
};
};
sounds fine to me.
I'm just wondering if we need to be describing this in DT in such
detail.
Do you have an alternative suggestion for the N900's bus switch
hack?
No, not really anything better at the moment.
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
