Message ID | 20170815220349.GA15441@amd (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi, On Wed, Aug 16, 2017 at 12:03:49AM +0200, Pavel Machek wrote: > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt > index 852041a..aade681 100644 > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt > @@ -119,6 +119,8 @@ Optional endpoint properties > as 0 (normal). This property is valid for serial busses only. > - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used > with CCP2, for instance. > +- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted > +- crc: crc is in use booleans in DT are not done with <0> and <1>, but as properties without values. True = property exists, False = property does not exist. -- Sebastian
> Hi, > > On Wed, Aug 16, 2017 at 12:03:49AM +0200, Pavel Machek wrote: > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt > > index 852041a..aade681 100644 > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt > > @@ -119,6 +119,8 @@ Optional endpoint properties > > as 0 (normal). This property is valid for serial busses only. > > - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used > > with CCP2, for instance. > > +- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted > > +- crc: crc is in use > > booleans in DT are not done with <0> and <1>, but as properties without > values. True = property exists, False = property does not exist. Well, strobe property above already uses = <0>/<1> format, as do others. Problem with "false = property does not exist" is that you don't know if it is "someone forgot to define it" or "someone made a typo" or "dts is too old to know about this property" or "the property indeed should be false"... Pavel
On Wed, Aug 16, 2017 at 10:59:15PM +0200, Pavel Machek wrote: > > Hi, > > > > On Wed, Aug 16, 2017 at 12:03:49AM +0200, Pavel Machek wrote: > > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt > > > index 852041a..aade681 100644 > > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt > > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt > > > @@ -119,6 +119,8 @@ Optional endpoint properties > > > as 0 (normal). This property is valid for serial busses only. > > > - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used > > > with CCP2, for instance. > > > +- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted > > > +- crc: crc is in use > > > > booleans in DT are not done with <0> and <1>, but as properties without > > values. True = property exists, False = property does not exist. Now that you mention it, I remember that now. > > Well, strobe property above already uses = <0>/<1> format, as do > others. > > Problem with "false = property does not exist" is that you don't know > if it is "someone forgot to define it" or "someone made a typo" or > "dts is too old to know about this property" or "the property indeed > should be false"... As this is an established practice, I think we should follow it for bool properties. We could change the existing ones, too, and leave some extra checks in place to handle old dtbs.
Hi, On Thu, Aug 17, 2017 at 12:23:44AM +0300, Sakari Ailus wrote: > On Wed, Aug 16, 2017 at 10:59:15PM +0200, Pavel Machek wrote: > > > On Wed, Aug 16, 2017 at 12:03:49AM +0200, Pavel Machek wrote: > > > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt > > > > index 852041a..aade681 100644 > > > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt > > > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt > > > > @@ -119,6 +119,8 @@ Optional endpoint properties > > > > as 0 (normal). This property is valid for serial busses only. > > > > - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used > > > > with CCP2, for instance. > > > > +- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted > > > > +- crc: crc is in use > > > > > > booleans in DT are not done with <0> and <1>, but as properties without > > > values. True = property exists, False = property does not exist. > > Now that you mention it, I remember that now. > > > Well, strobe property above already uses = <0>/<1> format, as do > > others. > > > Problem with "false = property does not exist" is that you don't know > > if it is "someone forgot to define it" or "someone made a typo" or > > "dts is too old to know about this property" or "the property indeed > > should be false"... > > As this is an established practice, I think we should follow it for bool > properties. Yes it's common practice, there is also device_property_read_bool() to get the value. > We could change the existing ones, too, and leave some extra checks in > place to handle old dtbs. The following should be downward compatible: var = read_bool(); if (var && !read_int()) var = false; Btw. DT people should be CC'd for DT binding additions/changes. -- Sebastian
Sebastian Reichel wrote: > Hi, > > On Thu, Aug 17, 2017 at 12:23:44AM +0300, Sakari Ailus wrote: >> On Wed, Aug 16, 2017 at 10:59:15PM +0200, Pavel Machek wrote: >>>> On Wed, Aug 16, 2017 at 12:03:49AM +0200, Pavel Machek wrote: >>>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt >>>>> index 852041a..aade681 100644 >>>>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt >>>>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt >>>>> @@ -119,6 +119,8 @@ Optional endpoint properties >>>>> as 0 (normal). This property is valid for serial busses only. >>>>> - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used >>>>> with CCP2, for instance. >>>>> +- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted >>>>> +- crc: crc is in use >>>> >>>> booleans in DT are not done with <0> and <1>, but as properties without >>>> values. True = property exists, False = property does not exist. >> >> Now that you mention it, I remember that now. >> >>> Well, strobe property above already uses = <0>/<1> format, as do >>> others. >> >>> Problem with "false = property does not exist" is that you don't know >>> if it is "someone forgot to define it" or "someone made a typo" or >>> "dts is too old to know about this property" or "the property indeed >>> should be false"... >> >> As this is an established practice, I think we should follow it for bool >> properties. > > Yes it's common practice, there is also device_property_read_bool() > to get the value. > >> We could change the existing ones, too, and leave some extra checks in >> place to handle old dtbs. > > The following should be downward compatible: > > var = read_bool(); > if (var && !read_int()) > var = false; > > Btw. DT people should be CC'd for DT binding additions/changes. Pavel: could you also add linux-media as well for the next version, please?
Hi! > > > Well, strobe property above already uses = <0>/<1> format, as do > > > others. > > > > > Problem with "false = property does not exist" is that you don't know > > > if it is "someone forgot to define it" or "someone made a typo" or > > > "dts is too old to know about this property" or "the property indeed > > > should be false"... > > > > As this is an established practice, I think we should follow it for bool > > properties. > > Yes it's common practice, there is also device_property_read_bool() > to get the value. > > > We could change the existing ones, too, and leave some extra checks in > > place to handle old dtbs. > > The following should be downward compatible: > > var = read_bool(); > if (var && !read_int()) > var = false; > > Btw. DT people should be CC'd for DT binding additions/changes. Ok, so for now we do crc=1 by default. Now we'd like to have crc support configurable in the dts. But if we just introduce "crc;" option, it will break old dts users. We could introduce "no-crc;" and that would work in this particular case, but will break when we want different defaults at different devices. Anyway, introducing "no-crc;" seems pretty ugly to me. I'd rather do "crc=<0>;", in a similar way we handle other options now. Best regards, Pavel
Hi Pavel, On Mon, Aug 28, 2017 at 03:59:59PM +0200, Pavel Machek wrote: > Hi! > > > > > Well, strobe property above already uses = <0>/<1> format, as do > > > > others. > > > > > > > Problem with "false = property does not exist" is that you don't know > > > > if it is "someone forgot to define it" or "someone made a typo" or > > > > "dts is too old to know about this property" or "the property indeed > > > > should be false"... > > > > > > As this is an established practice, I think we should follow it for bool > > > properties. > > > > Yes it's common practice, there is also device_property_read_bool() > > to get the value. > > > > > We could change the existing ones, too, and leave some extra checks in > > > place to handle old dtbs. > > > > The following should be downward compatible: > > > > var = read_bool(); > > if (var && !read_int()) > > var = false; > > > > Btw. DT people should be CC'd for DT binding additions/changes. > > Ok, so for now we do crc=1 by default. Now we'd like to have crc > support configurable in the dts. But if we just introduce "crc;" > option, it will break old dts users. We could introduce "no-crc;" and > that would work in this particular case, but will break when we want > different defaults at different devices. > > Anyway, introducing "no-crc;" seems pretty ugly to me. I'd rather do > "crc=<0>;", in a similar way we handle other options now. The same issue actually exists for CSI-2: the CRC is typically enabled and I think you can at least disable the check (in OMAP3 ISP). Do we have a need to disable it at the moment for any purpose? I think I've seen this was disabled somewhere but it may just as well be a thoughtlessly written configuration.
Hi On 28.08.2017 18:07, Sakari Ailus wrote: > Hi Pavel, > > On Mon, Aug 28, 2017 at 03:59:59PM +0200, Pavel Machek wrote: >> Hi! >> >>>>> Well, strobe property above already uses = <0>/<1> format, as do >>>>> others. >>>> >>>>> Problem with "false = property does not exist" is that you don't know >>>>> if it is "someone forgot to define it" or "someone made a typo" or >>>>> "dts is too old to know about this property" or "the property indeed >>>>> should be false"... >>>> >>>> As this is an established practice, I think we should follow it for bool >>>> properties. >>> >>> Yes it's common practice, there is also device_property_read_bool() >>> to get the value. >>> >>>> We could change the existing ones, too, and leave some extra checks in >>>> place to handle old dtbs. >>> >>> The following should be downward compatible: >>> >>> var = read_bool(); >>> if (var && !read_int()) >>> var = false; >>> >>> Btw. DT people should be CC'd for DT binding additions/changes. >> >> Ok, so for now we do crc=1 by default. Now we'd like to have crc >> support configurable in the dts. But if we just introduce "crc;" >> option, it will break old dts users. We could introduce "no-crc;" and >> that would work in this particular case, but will break when we want >> different defaults at different devices. >> >> Anyway, introducing "no-crc;" seems pretty ugly to me. I'd rather do >> "crc=<0>;", in a similar way we handle other options now. > > The same issue actually exists for CSI-2: the CRC is typically enabled and > I think you can at least disable the check (in OMAP3 ISP). Do we have a > need to disable it at the moment for any purpose? I think I've seen this > was disabled somewhere but it may just as well be a thoughtlessly written > configuration. > Front camera has crc disabled in Nokia kernel. I am not sure the sensor supports crc at all. Regards, Ivo -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt index 852041a..aade681 100644 --- a/Documentation/devicetree/bindings/media/video-interfaces.txt +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt @@ -119,6 +119,8 @@ Optional endpoint properties as 0 (normal). This property is valid for serial busses only. - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used with CCP2, for instance. +- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted +- crc: crc is in use Example ------- diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts index b1be53d..0c1ef5c 100644 --- a/arch/arm/boot/dts/omap3-n900.dts +++ b/arch/arm/boot/dts/omap3-n900.dts @@ -144,15 +144,6 @@ io-channel-names = "temp", "bsi", "vbat"; }; - rear_camera: camera@0 { - compatible = "linux,camera"; - - module { - model = "TCM8341MD"; - sensor = <&cam1>; - }; - }; - pwm9: dmtimer-pwm { compatible = "ti,omap-dmtimer-pwm"; #pwm-cells = <3>; @@ -192,7 +183,7 @@ clock-inv = <0>; /* Select strobe = <1> for back camera, <0> for front camera */ strobe = <1>; - crc = <0>; + crc = <1>; }; }; }; diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c index 6cb1f04..3e17d72 100644 --- a/drivers/media/platform/omap3isp/isp.c +++ b/drivers/media/platform/omap3isp/isp.c @@ -2098,12 +2098,12 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode, buscfg->bus.ccp2.strobe_clk_pol = vep.bus.mipi_csi1.clock_inv; + buscfg->bus.ccp2.crc = + vep.bus.mipi_csi1.crc; buscfg->bus.ccp2.phy_layer = vep.bus.mipi_csi1.strobe; buscfg->bus.ccp2.ccp2_mode = vep.bus_type == V4L2_MBUS_CCP2; buscfg->bus.ccp2.vp_clk_pol = 1; - - buscfg->bus.ccp2.crc = 1; } else { buscfg->bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane; diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687..ce1af2d 100644 --- a/drivers/media/v4l2-core/v4l2-fwnode.c +++ b/drivers/media/v4l2-core/v4l2-fwnode.c @@ -170,6 +170,9 @@ v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle *fwnode, if (!fwnode_property_read_u32(fwnode, "clock-lanes", &v)) bus->clock_lane = v; + if (!fwnode_property_read_u32(fwnode, "crc", &v)) + bus->crc = v; + if (bus_type == V4L2_FWNODE_BUS_TYPE_CCP2) vep->bus_type = V4L2_MBUS_CCP2; else diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h index cb34dcb..eb820dc 100644 --- a/include/media/v4l2-fwnode.h +++ b/include/media/v4l2-fwnode.h @@ -66,11 +66,13 @@ struct v4l2_fwnode_bus_parallel { * index (1) * @data_lane: the number of the data lane * @clock_lane: the number of the clock lane + * @crc: crc is in use */ struct v4l2_fwnode_bus_mipi_csi1 { bool clock_inv; bool strobe; bool lane_polarity[2]; + bool crc; unsigned char data_lane; unsigned char clock_lane; };