Re: [PATCH RFC] media: rc: OF: Add Generic bindings for remote-control
On 10/02/2013 11:33 AM, Mauro Carvalho Chehab wrote: ... Well, from userspace PoV, it should have just one devnode for each TX/RX. I'm fine with that. So, if the device has N TX and/or RX simultaneous connections, it should be exposing N device nodes, and the DT should for it should have N entries, one for each. DT is based on the actual HW construction, not how a particular OS wants to expose that HW through its APIs. If there is a single HW block, there should be a single DT node, even if that HW block supports multiple channels. In some circumstances, it might make sense for the single top-level node that represents the HW-block to have child nodes that represent the channels, depending on what exactly the HW is doing and whether this level of detail is useful in DT. I would qualify this as rare though. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] media: st-rc: Add ST remote control driver
On 09/24/2013 02:08 AM, Srinivas KANDAGATLA wrote: Thanks Stephen, On 23/09/13 21:40, Stephen Warren wrote: On 09/19/2013 02:59 AM, Srinivas KANDAGATLA wrote: This patch adds support to ST RC driver, which is basically a IR/UHF receiver and transmitter. This IP (IRB) is common across all the ST parts for settop box platforms. IRB is embedded in ST COMMS IP block. It supports both Rx Tx functionality. In this driver adds only Rx functionality via LIRC codec. diff --git a/Documentation/devicetree/bindings/media/st-rc.txt b/Documentation/devicetree/bindings/media/st-rc.txt + - rx-mode: can be infrared or uhf. rx-mode should be present iff the + rx pins are wired up. + - tx-mode: should be infrared. tx-mode should be present iff the tx + pins are wired up. Should those property names be prefixed with st,; I assume they're specific to this binding rather than something generic that applies to all IR controller bindings? If you expect them to be generic, it's fine. Officially these bindings are not specified in ePAPR specs Well, there are plenty of properties we now consider generic that aren't in ePAPR... but I see no reason for not having these properties as generic ones. Are you ok with that? I suppose that infrared-vs-uhf is a concept that's probably common enough across any similar HW device, so it may make sense for these properties to be generic. If we do intend them to be generic, I'd suggest they be defined in some generic binding document though; perhaps something like bindings/media/ir.txt or bindings/media/remote-control.txt? That way, a HW-specific binding isn't the only place where a supposedly generic property is defined. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] media: st-rc: Add ST remote control driver
On 09/19/2013 02:59 AM, Srinivas KANDAGATLA wrote: This patch adds support to ST RC driver, which is basically a IR/UHF receiver and transmitter. This IP (IRB) is common across all the ST parts for settop box platforms. IRB is embedded in ST COMMS IP block. It supports both Rx Tx functionality. In this driver adds only Rx functionality via LIRC codec. diff --git a/Documentation/devicetree/bindings/media/st-rc.txt b/Documentation/devicetree/bindings/media/st-rc.txt +Required properties: + - compatible: should contain st,comms-irb. + - reg: base physical address of the controller and length of memory + mapped region. Nits: The indentation is a little odd here; I'd expect the - to be in column 1, at least that's how most other binding docs are written. Not a big deal though. It'd be nice to indent the continuation lines (e.g. mapped region) a bit more so it's easier to see where each new entry starts. There are two spaces in mapped region. + - rx-mode: can be infrared or uhf. rx-mode should be present iff the + rx pins are wired up. + - tx-mode: should be infrared. tx-mode should be present iff the tx + pins are wired up. Should those property names be prefixed with st,; I assume they're specific to this binding rather than something generic that applies to all IR controller bindings? If you expect them to be generic, it's fine. + - clocks : phandle with clock-specifier pair. For what clock? -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] media: i2c: adv7343: fix the DT binding properties
On 09/23/2013 05:50 AM, Laurent Pinchart wrote: On Monday 23 September 2013 08:18:52 Prabhakar Lad wrote: On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote: On 09/20/2013 10:11 AM, Prabhakar Lad wrote: OK I will, just send out a fix up patch which fixes the mismatch between names for the rc-cycle, and later send out a patch which removes the platform data usage for next release with proper DT bindings. I think the binding need to be fully corrected now, I just meant to not touch the board file, i.e. leave non-dt support unchanged. Ok I'm OK with making regulator properties as optional, But still it would change the meaning of what DT is, we know that the VDD/VDD_IO .. etc pins are required properties (but still making them as optional) :-( I think there might several devices where this situation may arise so just thinking of a alternative solution. say we have property 'software-regulator' which takes true/false(0/1) If set to true we make the regulators as required property or else we assume it is handled and ignore it ? I don't think this is a good idea. You would have to add a similar platform data flag for non-dt, it doesn't sound right. I can see two options here: 1. Make the regulator properties mandatory and, e.g. define a fixed voltage GPIO regulator in DT with an empty 'gpio' property. Then pass a phandle to that regulator in the adv7343 *-supply properties. For non-dt similarly a fixed voltage regulator(s) and voltage supplies would need to be defined in the board files. 2. Make the properties optional and use (devm_)regulator_get_optional() calls in the driver (a recently added function). I must admit I don't fully understand description of this function, it currently looks pretty much same as (devm_)regulator_get(). Thus the driver would need to be handling regulator supplies only when non ERR_PTR() is returned from regulator_get_optional() and otherwise assume a non critical error. There is already quite a few example occurrences of regulator_get_optional() usage. Thanks for pointing it I'll choose option 2 and post the patch. Isn't regulator_get_optional() intended for devices that can have supplies unconnected in normal use ? I believe so, yes. The ADV7343 supplies are mandatory from a hardware point of view, so I think we should use regulator_get(). Otherwise the driver won't be able to tell the difference between a regulator that isn't present yet (for instance because the regulator device/driver hasn't been probed yet), which should result in deferred probing, and an always-on regulator that has been left out. So I think you want to make the supply properties mandatory in DT (since some form of supply is mandatory in HW), yet make the driver support broken DTs which don't have those properties, by error-checking the return value from regulator_get(). You might want to put a note into DT saying that a previous version of the binding didn't require those supply properties, so they may be missing from older DTs. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] media: i2c: adv7343: fix the DT binding properties
On 09/13/2013 11:23 PM, Prabhakar Lad wrote: Hi Stephen, This patch should have been marked as RFC. Thanks for the review. On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 09/13/2013 05:57 AM, Prabhakar Lad wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com This patch fixes the DT binding properties of adv7343 decoder. The pdata which was being read from the DT property, is removed as this can done internally in the driver using cable detection register. This patch also removes the pdata of ADV7343 which was passed from DA850 machine. diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt Required Properties : - compatible: Must be adi,adv7343 +- reg: I2C device address. +- vddio-supply: I/O voltage supply. +- vddcore-supply: core voltage supply. +- vaa-supply: Analog power supply. +- pvdd-supply: PLL power supply. Old DTs won't contain those properties. This breaks the DT ABI if those properties are required. Is that acceptable? As of now adv7343 via DT binding is not enabled in any platforms so this wont break any DT ABI. Well, if the binding has already been written, it technically already is an ABI. Perhaps the binding can be fixed if it isn't in use yet, but this is definitely not the correct approach to DT. If it is, I think we should document that older versions of the binding didn't require those properties, so they may in fact be missing. I note that this patch doesn't actually update the driver to regulator_get() anything. Shouldn't it? As of now the driver isn’t enabling/accepting the regulators, so should I add those in DT properties or not ? The binding should describe the HW, not what the driver does/doesn't yet do. I wrote the above because it looked like the driver was broken, not to encourage you to remove properties from the binding. How does the driver work if it doesn't enable the required regulators though, I wonder? I suppose the boards this driver has been tested on all must used fixed (non-SW-controlled) regulators. Optional Properties : -- adi,power-mode-sleep-mode: on enable the current consumption is reduced to - micro ampere level. All DACs and the internal PLL - circuit are disabled. -- adi,power-mode-pll-ctrl: PLL and oversampling control. This control allows -internal PLL 1 circuit to be powered down and the -oversampling to be switched off. -- ad,adv7343-power-mode-dac: array configuring the power on/off DAC's 1..6, - 0 = OFF and 1 = ON, Default value when this - property is not specified is 0 0 0 0 0 0. -- ad,adv7343-sd-config-dac-out: array configure SD DAC Output's 1 and 2, 0 = OFF - and 1 = ON, Default value when this property is - not specified is 0 0. At a very quick glance, it's not really clear why those properties are being removed. They seem like HW configuration, so might be fine to put into DT. What replaces these? Yes these were HW configuration but, its now internally handled in the driver. The 'ad,adv7343-power-mode-dac' property which enabled the DAC's 1..6 , so now in the driver by default all the DAC's are enabled by default and enable unconnected DAC auto power down. Similarly 'ad,adv7343-sd-config-dac-out' property enabled SD DAC's 1..2 but now is enabled by reading the CABLE DETECT register which tells the status of DAC1/2. OK, that's probably fine for the two properties you mentioned (you didn't describe two of them...). Some more discussion on why SW doesn't need these options might be useful in the patch description. Note that the discussion should be written for software in general (i.e. any OS's driver), and not for Linux's specific driver, since DT is not tied to any one OS. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] media: i2c: adv7343: fix the DT binding properties
On 09/13/2013 05:57 AM, Prabhakar Lad wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com This patch fixes the DT binding properties of adv7343 decoder. The pdata which was being read from the DT property, is removed as this can done internally in the driver using cable detection register. This patch also removes the pdata of ADV7343 which was passed from DA850 machine. diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt Required Properties : - compatible: Must be adi,adv7343 +- reg: I2C device address. +- vddio-supply: I/O voltage supply. +- vddcore-supply: core voltage supply. +- vaa-supply: Analog power supply. +- pvdd-supply: PLL power supply. Old DTs won't contain those properties. This breaks the DT ABI if those properties are required. Is that acceptable? If it is, I think we should document that older versions of the binding didn't require those properties, so they may in fact be missing. I note that this patch doesn't actually update the driver to regulator_get() anything. Shouldn't it? Optional Properties : -- adi,power-mode-sleep-mode: on enable the current consumption is reduced to - micro ampere level. All DACs and the internal PLL - circuit are disabled. -- adi,power-mode-pll-ctrl: PLL and oversampling control. This control allows -internal PLL 1 circuit to be powered down and the -oversampling to be switched off. -- ad,adv7343-power-mode-dac: array configuring the power on/off DAC's 1..6, - 0 = OFF and 1 = ON, Default value when this - property is not specified is 0 0 0 0 0 0. -- ad,adv7343-sd-config-dac-out: array configure SD DAC Output's 1 and 2, 0 = OFF - and 1 = ON, Default value when this property is - not specified is 0 0. At a very quick glance, it's not really clear why those properties are being removed. They seem like HW configuration, so might be fine to put into DT. What replaces these? -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] s5p-jpeg: Add initial device tree support for S5PV210/Exynos4210 SoCs
On 08/23/2013 10:01 AM, Sylwester Nawrocki wrote: On 08/18/2013 10:14 PM, Sylwester Nawrocki wrote: This patch enables the JPEG codec on S5PV210 and Exynos4210 SoCs. There are some differences in newer versions of the JPEG codec IP on SoCs like Exynos4x12 and Exynos5 series and support for them will be added in subsequent patches. Cc: Andrzej Pietrasiewicz andrze...@samsung.com Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com Could a DT maintainer review/Ack the binding in this patch ? The binding looks reasonable to me, so, Acked-by: Stephen Warren swar...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] s5k5baf: add camera sensor driver
On 08/21/2013 08:41 AM, Andrzej Hajda wrote: Driver for Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP. The driver exposes the sensor as two V4L2 subdevices: - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format, no controls. - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200, pre/post ISP cropping, downscaling via selection API, controls. The binding, Acked-by: Stephen Warren swar...@nvidia.com (although it would be great if another DT binding maintainer gave it a quick look-over to make sure I didn't miss anything!) -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] s5k5baf: add camera sensor driver
On 08/20/2013 10:03 AM, Andrzej Hajda wrote: Driver for Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP. The driver exposes the sensor as two V4L2 subdevices: - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format, no controls. - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200, pre/post ISP cropping, downscaling via selection API, controls. diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt +Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP + + +Required properties: + +- compatible : samsung,s5k5baf; +- reg : I2C slave address of the sensor; +- vdda-supply : analog power supply 2.8V (2.6V to 3.0V); +- vddreg-supply: regulator input power supply 1.8V (1.7V to 1.9V) + or 2.8V (2.6V to 3.0); +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) + or 2.8V (2.5V to 3.1V); +- stbyn-gpios : GPIO connected to STDBYN pin; +- rstn-gpios : GPIO connected to RSTN pin; +- clocks : the sensor's master clock specifier (from the common + clock bindings); +- clock-names : must be mclk; That all looks sane. +Optional properties: + +- clock-frequency : master clock frequency in Hz; if this property is + not specified default 24 MHz value will be used. I /think/ the explanation you gave Mark on this property makes sense. However, it's not clear from the description what this does; in many other cases a clock-frequency describes a fixed/actual input clock rate to a device, rather than a frequency which the device believes it should operate at, and hence the driver should request. Perhaps the following would describe this: - clock-frequency : The frequency at which the mclk clock should be configured to operate, in Hz. If this property is not specified default 24 MHz value will be used. To me, this more strongly implies that the user of the node should configure the clock, rather than the property reporting the rate at which the clock is already configured to operate. I think the rest of the binding doc (below this point) seems reasonable too. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On 08/19/2013 11:25 AM, Sylwester Nawrocki wrote: On 08/19/2013 03:25 PM, Pawel Moll wrote: On Mon, 2013-08-19 at 14:18 +0100, Andrzej Hajda wrote: +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt @@ -0,0 +1,51 @@ +Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP +- + +Required properties: + +- compatible : samsung,s5k5baf; +- reg: I2C slave address of the sensor; +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V); +- vddreg-supply : regulator input power supply 1.8V (1.7V to 1.9V) +or 2.8V (2.6V to 3.0); +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) +or 2.8V (2.5V to 3.1V); +- gpios : GPIOs connected to STDBYN and RSTN pins, +in order: STBYN, RSTN; You probably want to use the [name-]gpios convention here (see Documentation/devicetree/bindings/gpio/gpio.txt), so something like stbyn-gpios and rstn-gpios. Unless using multiple named properties is really preferred over a single gpios property I would like to keep the single property containing a list of GPIOs. ... Yes, a separate property for each type of GPIO is typical. Multiple entries in the same property are allowed if they're used for the same purpose/type, whereas here they're clearly different things. Inconsistent with (some) other properties, admittedly... -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On 08/19/2013 04:53 PM, Tomasz Figa wrote: On Monday 19 of August 2013 16:30:45 Stephen Warren wrote: On 08/19/2013 11:25 AM, Sylwester Nawrocki wrote: On 08/19/2013 03:25 PM, Pawel Moll wrote: On Mon, 2013-08-19 at 14:18 +0100, Andrzej Hajda wrote: +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt @@ -0,0 +1,51 @@ +Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP +- + +Required properties: + +- compatible : samsung,s5k5baf; +- reg: I2C slave address of the sensor; +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V); +- vddreg-supply : regulator input power supply 1.8V (1.7V to 1.9V) +or 2.8V (2.6V to 3.0); +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) +or 2.8V (2.5V to 3.1V); +- gpios : GPIOs connected to STDBYN and RSTN pins, +in order: STBYN, RSTN; You probably want to use the [name-]gpios convention here (see Documentation/devicetree/bindings/gpio/gpio.txt), so something like stbyn-gpios and rstn-gpios. Unless using multiple named properties is really preferred over a single gpios property I would like to keep the single property containing a list of GPIOs. ... Yes, a separate property for each type of GPIO is typical. Multiple entries in the same property are allowed if they're used for the same purpose/type, whereas here they're clearly different things. Inconsistent with (some) other properties, admittedly... I'm not really convinced about the superiority of named gpio properties over a single gpios property with multiple entries in this case. I'd say it's more just a matter of preference. See the clock or interrupt bindings. They all specify all the clocks and interrupts in single property, without any differentiation based on their purposes. Also keep in mind that original GPIO bindings used only a single gpios property and was only extended to allow named ones. Well, it's not so much about what's best, but just being consistent with what's already there. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
On 08/05/2013 04:37 PM, Sylwester Nawrocki wrote: On 08/05/2013 06:53 PM, Stephen Warren wrote: On 08/03/2013 03:41 PM, Sylwester Nawrocki wrote: On 08/02/2013 05:02 PM, Arun Kumar K wrote: The patch adds the DT binding documentation for Samsung Exynos5 SoC series imaging subsystem (FIMC-IS). diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt new file mode 100644 index 000..49a373a --- /dev/null +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt @@ -0,0 +1,52 @@ +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS) +-- + +The camera subsystem on Samsung Exynos5 SoC has some changes relative +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and +FIMC-LITE IPs but has a much improved version of FIMC-IS which can +handle sensor controls and camera post-processing operations. The +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two +dedicated scalers (SCC and SCP). So there are a lot of blocks mentioned there, yet the binding doesn't seem to describe most of it. Is the binding complete? Thanks for the review Stephen. No, the binding certainly isn't complete, it doesn't describe the all available IP blocks. There are separate MMIO address regions for each ... So while we could list all the devices, we decided not to do so. Because it is not needed by the current software and we may miss some details for case where the whole subsystem is controlled by the host CPU (however such scenario is extremely unlikely AFAICT) which then would be impossible or hard to change. Yes, that's probably a good approach. I guess we should list all available devices, similarly as it's done in Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt. And then should they just be disabled through the status property if they are not needed in the Linux driver ? I guess it is more sensible than marking them as optional and then not listing them in dts at all ? If you can define complete bindings for those nodes, it might make sense to do that. If the devices are perhaps complex to represent and hence you might not be able to come up with complete bindings for them right now, it may indeed be better to simply not mention the devices you don't care about for now. +pmu subnode +--- + +Required properties: + - reg : should contain PMU physical base address and size of the memory + mapped registers. I think you need a compatible value for this. How else is the node identified? The node name probably should not be used for identification. Of course the node name is currently used for identification. There is no compatible property because this pmu node is used to get hold of only part of the Power Management Unit registers, specific to the FIMC-IS. The PMU has more registers that also other drivers would be interested in, e.g. clocks or USB. I believe the correct way to solve this is for there to be a standalone PMU node at the appropriate location in DT, and for the FIMC bindings to reference that other node by phandle. Right now, the FIMC driver SW can manually follow the phandle, look at the reg property, and map that itself. Later down the road, you could instantiate a true PMU driver, and have the FIMC driver look up that driver, and call APIs on it. This change can be made without requiring any changes to the DT binding. That way, you aren't introducing a fake PMU node into the FIMC bindings just to satisfy internal Linux driver details. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] ARM: Exynos: replace custom MFC reserved memory handling with generic code
On 08/05/2013 06:26 AM, Marek Szyprowski wrote: MFC driver use custom bindings for managing reserved memory. Those bindings are not really specific to MFC device and no even well discussed. They can be easily replaced with generic, platform independent code for handling reserved and contiguous memory. Two additional child devices for each memory port (AXI master) are introduced to let one assign some properties to each of them. Later one can also use them to assign properties related to SYSMMU controllers, which can be used to manage the limited dma window provided by those memory ports. diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt +The MFC device is connected to system bus with two memory ports (AXI +masters) for better performance. Those memory ports are modelled as +separate child devices, so one can assign some properties to them (like +memory region for dma buffer allocation or sysmmu controller). + Required properties: - compatible : value should be either one among the following (a) samsung,mfc-v5 for MFC v5 present in Exynos4 SoCs (b) samsung,mfc-v6 for MFC v6 present in Exynos5 SoCs + and additionally simple-bus to correctly initialize child + devices for memory ports (AXI masters) simple-bus is wrong here; the child nodes aren't independent entities that can be instantiated separately from their parent and without depending on their parent. - - samsung,mfc-r : Base address of the first memory bank used by MFC - for DMA contiguous memory allocation and its size. - - - samsung,mfc-l : Base address of the second memory bank used by MFC - for DMA contiguous memory allocation and its size. These properties shouldn't be removed, but simply marked deprecated. The driver will need to continue to support them so that old DTs work with new kernels. The binding must therefore continue to document them so that the old DT content still makes sense. +Two child nodes must be defined for MFC device. Their names must be +following: memport-r and memport-l (right and left). Required Node names shouldn't have semantic meaning. +properties: + - compatible : value should be samsung,memport There's no need to define compatible properties for things which aren't separate devices. + - dma-memory-region : optional property with a phandle to respective memory + region (see devicetree/bindings/memory.txt), if no region + is defined, sysmmu controller must be used for managing + limited dma window of each memory port. What's the benefit of putting this property in a sub-node; surely you can put the property in the main MFC node yet follow the same conceptual schema for its content as a dma-memory-region node? -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] ARM: Exynos: replace custom MFC reserved memory handling with generic code
On 08/08/2013 03:19 PM, Tomasz Figa wrote: Hi Stephen, On Thursday 08 of August 2013 15:00:52 Stephen Warren wrote: On 08/05/2013 06:26 AM, Marek Szyprowski wrote: MFC driver use custom bindings for managing reserved memory. Those bindings are not really specific to MFC device and no even well discussed. They can be easily replaced with generic, platform independent code for handling reserved and contiguous memory. Two additional child devices for each memory port (AXI master) are introduced to let one assign some properties to each of them. Later one can also use them to assign properties related to SYSMMU controllers, which can be used to manage the limited dma window provided by those memory ports. diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt +The MFC device is connected to system bus with two memory ports (AXI +masters) for better performance. Those memory ports are modelled as +separate child devices, so one can assign some properties to them (like +memory region for dma buffer allocation or sysmmu controller). + Required properties: - compatible : value should be either one among the following (a) samsung,mfc-v5 for MFC v5 present in Exynos4 SoCs (b) samsung,mfc-v6 for MFC v6 present in Exynos5 SoCs + and additionally simple-bus to correctly initialize child + devices for memory ports (AXI masters) simple-bus is wrong here; the child nodes aren't independent entities that can be instantiated separately from their parent and without depending on their parent. I fully agree, but I can't think of anything better. Could you suggest a solution that would cover our use case: The MFC IP has two separate bus masters (called here and there memports). On some SoCs, each master must use different memory bank to meet memory bandwidth requirements for higher bitrate video streams. This can be seen as MFC having two DMA subdevices, which have different DMA windows. On Linux, this is handled by binding two appropriate CMA memory regions to the memports, so the driver can do DMA allocations on behalf of particular memport and get appropriate memory for it. I don't see what that has to do with simple-bus. Whatever parses the node of the MFC can directly read from any contained property or child node; there's no need to try and get the core DT tree parser to enumerate the children. If you actually need a struct platform_device for each of these child nodes (which sounds wrong, but I'm not familiar with the code), then simply have the driver call of_platform_populate() itself at the appropriate time. But that's not going to work well unless the child nodes have compatible values, which doesn't seem right given their purpose. - - samsung,mfc-r : Base address of the first memory bank used by MFC - for DMA contiguous memory allocation and its size. - - - samsung,mfc-l : Base address of the second memory bank used by MFC - for DMA contiguous memory allocation and its size. These properties shouldn't be removed, but simply marked deprecated. The driver will need to continue to support them so that old DTs work with new kernels. The binding must therefore continue to document them so that the old DT content still makes sense. I tend to disagree on this. For Samsung platforms we've been trying to avoid DT bindings changes as much as possible, but I'd rather say that device tree was coupled with kernel version it came from, so Samsung- specific bindings haven't been fully stabilized yet, especially since we are still at discussion stage when it's about defining processes for binding staging and stabilization. Well, that's why everyone is shouting at ARM for abusing DT... I would rather see this patch as part of work on Samsung DT binding janitoring or maybe even sanitizing in some cases, like this one, when the old (and IMHO bad) MFC binding was introduced without proper review. I don't think we want to support this kind of brokenness anymore, especially considering the hacks which would be required by such support (see original implementation of this binding and code required in board file). +Two child nodes must be defined for MFC device. Their names must be +following: memport-r and memport-l (right and left). Required Node names shouldn't have semantic meaning. What about bus-master-0 and bus-master-1? Just bus-master for each might make sense. Use reg properties to differentiate the two? -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] ARM: Exynos: replace custom MFC reserved memory handling with generic code
On 08/08/2013 04:10 PM, Tomasz Figa wrote: On Thursday 08 of August 2013 15:47:19 Stephen Warren wrote: On 08/08/2013 03:19 PM, Tomasz Figa wrote: On Thursday 08 of August 2013 15:00:52 Stephen Warren wrote: On 08/05/2013 06:26 AM, Marek Szyprowski wrote: MFC driver use custom bindings for managing reserved memory. Those bindings are not really specific to MFC device and no even well discussed. They can be easily replaced with generic, platform independent code for handling reserved and contiguous memory. Two additional child devices for each memory port (AXI master) are introduced to let one assign some properties to each of them. Later one can also use them to assign properties related to SYSMMU controllers, which can be used to manage the limited dma window provided by those memory ports. ... +Two child nodes must be defined for MFC device. Their names must be +following: memport-r and memport-l (right and left). Required Node names shouldn't have semantic meaning. What about bus-master-0 and bus-master-1? Just bus-master for each might make sense. Use reg properties to differentiate the two? What this reg property would mean in this case? My understanding of reg property was that it should be used for real addresses or IDs and for all other cases node names should be suffixed with -ID. Presumably it would hold the ID of the HW block as defined in the documentation. Or, it could be somewhat arbitrary. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
On 08/03/2013 03:41 PM, Sylwester Nawrocki wrote: On 08/02/2013 05:02 PM, Arun Kumar K wrote: The patch adds the DT binding documentation for Samsung Exynos5 SoC series imaging subsystem (FIMC-IS). diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt new file mode 100644 index 000..49a373a --- /dev/null +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt @@ -0,0 +1,52 @@ +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS) +-- + +The camera subsystem on Samsung Exynos5 SoC has some changes relative +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and +FIMC-LITE IPs but has a much improved version of FIMC-IS which can +handle sensor controls and camera post-processing operations. The +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two +dedicated scalers (SCC and SCP). So there are a lot of blocks mentioned there, yet the binding doesn't seem to describe most of it. Is the binding complete? +pmu subnode +--- + +Required properties: + - reg : should contain PMU physical base address and size of the memory + mapped registers. I think you need a compatible value for this. How else is the node identified? The node name probably should not be used for identification. + +i2c-isp (ISP I2C bus controller) nodes +-- + +Required properties: + +- compatible: should be samsung,exynos4212-i2c-isp for Exynos4212, + Exynos4412 and Exynos5250 SoCs; +- reg: physical base address and length of the registers set; +- clocks: must contain gate clock specifier for this controller; +- clock-names: must contain i2c_isp entry. + +For the above nodes it is required to specify a pinctrl state named default, Is above nodes both pmu, i2c-isp? It might make sense to be more explicit re: which nodes this comment applies to. +according to the pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt. + +Device tree nodes of the image sensors' controlled directly by the FIMC-IS s/'// ? +firmware must be child nodes of their corresponding ISP I2C bus controller node. +The data link of these image sensors must be specified using the common video +interfaces bindings, defined in video-interfaces.txt. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On 07/19/2013 12:36 AM, Kishon Vijay Abraham I wrote: Hi, On Friday 19 July 2013 11:59 AM, Greg KH wrote: On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote: Hi, On Friday 19 July 2013 11:13 AM, Greg KH wrote: On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote: + ret = dev_set_name(phy-dev, %s.%d, dev_name(dev), id); Your naming is odd, no phy anywhere in it? You rely on the sender to never send a duplicate name.id pair? Why not create your own ids based on the number of phys in the system, like almost all other classes and subsystems do? hmm.. some PHY drivers use the id they provide to perform some of their internal operation as in [1] (This is used only if a single PHY provider implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO to give the PHY drivers an option to use auto id. [1] - http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html No, who cares about the id? No one outside of the phy core ever should, because you pass back the only pointer that they really do care about, if they need to do anything with the device. Use that, and then you can hmm.. ok. rip out all of the search for a phy by a string logic, as that's not Actually this is needed for non-dt boot case. In the case of dt boot, we use a phandle by which the controller can get a reference to the phy. But in the case of non-dt boot, the controller can get a reference to the phy only by label. I don't understand. They registered the phy, and got back a pointer to it. Why can't they save it in their local structure to use it again later if needed? They should never have to ask for the device, as the One is a *PHY provider* driver which is a driver for some PHY device. This will use phy_create to create the phy. The other is a *PHY consumer* driver which might be any controller driver (can be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot). device id might be unknown if there are multiple devices in the system. I agree with you on the device id part. That need not be known to the PHY driver. How does a consumer know which label to use in a non-dt system if there are multiple PHYs in the system? That should be passed using platform data. I don't think that's right. That's just like passing clock names in platform data, which I believe is frowned upon. Instead, why not take the approach that e.g. regulators have taken? Each driver defines the names of the objects that it requires. There is a table (registered by a board file) that has lookup key (device name, regulator name), and the output is the regulator device (or global regulator name). This is almost the same way that DT works, except that in DT, the table is represented as properties in the DT. The DT binding defines the names of those properties, or the strings that must be in the foo-names properties, just like a driver in non-DT Linux is going to define the names it expects to be provided with. That way, you don't need platform data to define the names. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] [media] Add common video interfaces OF bindings documentation
On 03/26/2013 09:58 AM, Sylwester Nawrocki wrote: From: Guennadi Liakhovetski g.liakhovet...@gmx.de This patch adds a document describing common OF bindings for video capture, output and video processing devices. It is curently mainly focused on video capture devices, with data busses defined by standards such as ITU-R BT.656 or MIPI-CSI2. It also documents a method of describing data links between devices. Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com Acked-by: Stephen Warren swar...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
On 02/12/2013 03:39 PM, Sylwester Nawrocki wrote: On 02/11/2013 10:50 PM, Stephen Warren wrote: On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote: On 02/09/2013 01:32 AM, Stephen Warren wrote: On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote: On 02/09/2013 12:21 AM, Stephen Warren wrote: On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: On 02/07/2013 12:40 AM, Stephen Warren wrote: diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) +-- ... +For every fimc node a numbered alias should be present in the aliases node. +Aliases are of the form fimcn, wheren is an integer (0...N) specifying +the IP's instance index. ... Different compatible values might not work, when for example there are 3 IPs out of 4 of one type and the fourth one of another type. It wouldn't even by really different types, just quirks/little differences between them, e.g. no data path routed to one of other IPs. I was thinking of using feature-/quirk-oriented properties. For example, if there's a port on 3 of the 4 devices to connect to some other IP block, simply include a boolean property to indicate whether that port is present. It would be in 3 of the nodes but not the 4th. Yes, I could add several properties corresponding to all members of this [3] data structure. But still it is needed to clearly identify the IP block in a set of the hardware instances. Why? What decisions will be made based on the identify of the IP block instance that wouldn't be covered by DT properties that describe which features/bugs/... the IP block instance has? The whole subsystem topology is exposed to user space through the Media Controller API. OK, stable user-visible names are a reasonable use for device tree. I still don't think you should use those user-visible IDs for making any other kind of decision though. Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the MIPI-CSIS needs to be written to the FIMC.2 data input control register. Even though MIPI-CSIS.N are same in terms of hardware structure they still need to be distinguished as separate instances. Oh, so you're using the alias ID as the value to write into the FIMC.2 register for that. I'm not 100% familiar with aliases, but they seem like a more user-oriented naming thing to me, whereas values for hooking up intra-SoC routing are an unrelated namespace semantically, even if the values happen to line up right now. Perhaps rather than a Boolean property I mentioned above, use a custom property to indicate the ID that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP blocks are immutably connected to the SoC camera physical MIPI CSI-2 interfaces, and the physical camera ports have fixed assignment to the MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS nodes. And their instance index that is required for the top level driver which exposes topology and the routing capabilities to user space could be restored from the reg property value by subtracting a fixed offset. I suppose that would work. It feels a little indirect, and I think means that the driver needs to go find some child node defining its end of some link, then find the node representing the other end of the link, then read properties out of that other node to find the value. That seems a little unusual, but I guess it would work. I'm not sure of the long-term implications of doing that kind of thing. You'd want to run the idea past some DT maintainers/experts. It's a bit simpler than that. We would need only to look for the reg property in a local port subnode. MIPI-CSIS correspond to physical MIPI CSI-2 bus interface of an SoC, hence it has to have specific reg values that identify each camera input interface. Oh I see. I guess if a device is using its own node to determine its own identify, that's reasonable. I thought you were talking about a situation like: FIMC -- XXX where FIMC wanted to determine what ID XXX knew that particular FIMC as. I can see aliases used in bindings of multiple devices: uart, spi, sound interfaces, gpio, ... And all bindings seem to impose some rules on how their aliases are created. Do you have specific examples? I really don't think the bindings should be dictating the alias values. I just grepped through the existing bindings documentation: ... I think correctly numbered in the above statements means there are some specific rules on how the aliases are created, however those seem not clearly communicated. A binding specifying that an alias must (or even should) exist for each node seems odd to me. In the absence of an explicit
Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote: On 02/09/2013 01:32 AM, Stephen Warren wrote: On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote: On 02/09/2013 12:21 AM, Stephen Warren wrote: On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: On 02/07/2013 12:40 AM, Stephen Warren wrote: diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) +-- ... +For every fimc node a numbered alias should be present in the aliases node. +Aliases are of the form fimcn, wherenis an integer (0...N) specifying +the IP's instance index. ... Different compatible values might not work, when for example there are 3 IPs out of 4 of one type and the fourth one of another type. It wouldn't even by really different types, just quirks/little differences between them, e.g. no data path routed to one of other IPs. I was thinking of using feature-/quirk-oriented properties. For example, if there's a port on 3 of the 4 devices to connect to some other IP block, simply include a boolean property to indicate whether that port is present. It would be in 3 of the nodes but not the 4th. Yes, I could add several properties corresponding to all members of this [3] data structure. But still it is needed to clearly identify the IP block in a set of the hardware instances. Why? What decisions will be made based on the identify of the IP block instance that wouldn't be covered by DT properties that describe which features/bugs/... the IP block instance has? Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the MIPI-CSIS needs to be written to the FIMC.2 data input control register. Even though MIPI-CSIS.N are same in terms of hardware structure they still need to be distinguished as separate instances. Oh, so you're using the alias ID as the value to write into the FIMC.2 register for that. I'm not 100% familiar with aliases, but they seem like a more user-oriented naming thing to me, whereas values for hooking up intra-SoC routing are an unrelated namespace semantically, even if the values happen to line up right now. Perhaps rather than a Boolean property I mentioned above, use a custom property to indicate the ID that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP blocks are immutably connected to the SoC camera physical MIPI CSI-2 interfaces, and the physical camera ports have fixed assignment to the MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS nodes. And their instance index that is required for the top level driver which exposes topology and the routing capabilities to user space could be restored from the reg property value by subtracting a fixed offset. I suppose that would work. It feels a little indirect, and I think means that the driver needs to go find some child node defining its end of some link, then find the node representing the other end of the link, then read properties out of that other node to find the value. That seems a little unusual, but I guess it would work. I'm not sure of the long-term implications of doing that kind of thing. You'd want to run the idea past some DT maintainers/experts. ... I can see aliases used in bindings of multiple devices: uart, spi, sound interfaces, gpio, ... And all bindings seem to impose some rules on how their aliases are created. Do you have specific examples? I really don't think the bindings should be dictating the alias values. After all, what happens in some later SoC where you have two different types of module that feed into the common module, such that type A sources have IDs 0..3 in the common module, and type B sources have IDs 4..7 in the common module - you wouldn't want to require alias ISs 4..7 for the type B DT nodes. There is no need to write alias ID directly into registers, and actually it doesn't really happen. But we need to know that, for example camera A is connected to physical MIPI CSI-2 channel 0 and to capture video with DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to MIPI-CSIS 0. OK, so the IDs are selecting which register to write, or which mux settings to access. That's pretty much semantically the same thing. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: On 02/07/2013 12:40 AM, Stephen Warren wrote: diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) +-- ... +For every fimc node a numbered alias should be present in the aliases node. +Aliases are of the form fimcn, wheren is an integer (0...N) specifying +the IP's instance index. Why? Isn't it up to the DT author whether they care if each fimc node is assigned a specific identification v.s. whether identification is assigned automatically? There are at least three different kinds of IPs that come in multiple instances in an SoC. To activate data links between them each instance needs to be clearly identified. There are also differences between instances of same device. Hence it's important these aliases don't have random values. Some more details about the SoC can be found at [1]. The aliases are also already used in the Exynos5 GScaler bindings [2] in a similar way. Hmmm. I'd expect explicit DT properties to represent the instance-specific configuration, or even different compatible values. Relying on the alias ID seems rather indirect; what if in e.g. Exynos6/... the mapping from instance/alias ID to feature set changes. With explicit DT properties, that'd just be a .dts change, whereas by requiring alias IDs now, you'd need a driver change to support this. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/10] s5p-csis: Add device tree support
On 02/08/2013 03:29 PM, Sylwester Nawrocki wrote: On 02/07/2013 12:36 AM, Stephen Warren wrote: On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote: s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC device. This patch support for binding the driver to the MIPI-CSIS devices instantiated from device tree and for parsing all SoC and board specific properties. diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt +Optional properties: + +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default +value when this property is not specified is 166 MHz; Shouldn't this be a clocks property, so that the driver can call clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it? Hi Stephen, Thanks for your review! I also use clocks and clock-names property, but didn't specify it here, because the Exynos SoCs clocks driver is not in the mainline yet. I'm a bit sympathetic to this, since I've been trying to push Tegra towards the common clock framework and describing clock connectivity in DT, before adding new drivers that need clocks, specifically to avoid this kind of situation. The problem here is that if this gets merged now, then the clock driver comes along later, you'll have to change this binding definition to account for it, and DT bindings aren't supposed to be changed... Do you have any clock driver at all upstream yet? Or, could you add a dummy clock driver to satisfy the driver's clkl_get() needs? If you do, you can always set up some AUXDATA so that clk_get() works for your driver right now, and then remove that once the complete clock driver is in place with full device tree support. That's how we've ended up handling this for Tegra drivers. Anyway, this is all mainly food-for-thought rather than an objection to the patch; I'd leave that to Grant/Rob if applicable. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote: On 02/09/2013 12:21 AM, Stephen Warren wrote: On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: On 02/07/2013 12:40 AM, Stephen Warren wrote: diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) +-- ... +For every fimc node a numbered alias should be present in the aliases node. +Aliases are of the form fimcn, wheren is an integer (0...N) specifying +the IP's instance index. Why? Isn't it up to the DT author whether they care if each fimc node is assigned a specific identification v.s. whether identification is assigned automatically? There are at least three different kinds of IPs that come in multiple instances in an SoC. To activate data links between them each instance needs to be clearly identified. There are also differences between instances of same device. Hence it's important these aliases don't have random values. Some more details about the SoC can be found at [1]. The aliases are also already used in the Exynos5 GScaler bindings [2] in a similar way. Hmmm. I'd expect explicit DT properties to represent the instance-specific configuration, or even different compatible values. Relying on the alias ID seems rather indirect; what if in e.g. Exynos6/... the mapping from instance/alias ID to feature set changes. With explicit DT properties, that'd just be a .dts change, whereas by requiring alias IDs now, you'd need a driver change to support this. In the initial version of this patch series I used cell-index property, but then Grant pointed out in some other mail thread it should be avoided. Hence I used the node aliases. To me, using cell-index is semantically equivalent to using the alias ID. Different compatible values might not work, when for example there are 3 IPs out of 4 of one type and the fourth one of another type. It wouldn't even by really different types, just quirks/little differences between them, e.g. no data path routed to one of other IPs. I was thinking of using feature-/quirk-oriented properties. For example, if there's a port on 3 of the 4 devices to connect to some other IP block, simply include a boolean property to indicate whether that port is present. It would be in 3 of the nodes but not the 4th. Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the MIPI-CSIS needs to be written to the FIMC.2 data input control register. Even though MIPI-CSIS.N are same in terms of hardware structure they still need to be distinguished as separate instances. Oh, so you're using the alias ID as the value to write into the FIMC.2 register for that. I'm not 100% familiar with aliases, but they seem like a more user-oriented naming thing to me, whereas values for hooking up intra-SoC routing are an unrelated namespace semantically, even if the values happen to line up right now. Perhaps rather than a Boolean property I mentioned above, use a custom property to indicate the ID that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems similar to using cell-index, my *guess* is that Grant's objection to using cell-index was more based on re-using cell-index for something other than its intended purpose than pushing you to use an alias ID rather than a property. After all, what happens in some later SoC where you have two different types of module that feed into the common module, such that type A sources have IDs 0..3 in the common module, and type B sources have IDs 4..7 in the common module - you wouldn't want to require alias ISs 4..7 for the type B DT nodes. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/10] s5p-csis: Add device tree support
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote: s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC device. This patch support for binding the driver to the MIPI-CSIS devices instantiated from device tree and for parsing all SoC and board specific properties. diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt +Optional properties: + +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default + value when this property is not specified is 166 MHz; Shouldn't this be a clocks property, so that the driver can call clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it? Other than that this binding seems fine to me at a quick glance. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote: This patch adds support for FIMC devices instantiated from devicetree for S5PV210 and Exynos4 SoCs. The FIMC IP features include colorspace conversion and scaling (mem-to-mem) and parallel/MIPI CSI2 bus video capture interface. Multiple SoC revisions specific parameters are defined statically in the driver and are used for both dt and non-dt. The driver's static data is selected based on the compatible property. Previously the platform device name was used to match driver data and a specific SoC/IP version. Aliases are used to determine an index of the IP which is essential for linking FIMC IP with other entities, like MIPI-CSIS (the MIPI CSI-2 bus frontend) or FIMC-LITE and FIMC-IS ISP. diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) +-- + +The Exynos Camera subsystem comprises of multiple sub-devices that are +represented by separate platform devices. Some of the IPs come in different platform devices is a rather Linux-centric term, and DT bindings should be OS-agnostic. Perhaps use device tree nodes here? +variants across the SoC revisions (FIMC) and some remain mostly unchanged +(MIPI CSIS, FIMC-LITE). + +All those sub-subdevices are defined as parent nodes of the common device s/parent nodes/child node/ I think? +For every fimc node a numbered alias should be present in the aliases node. +Aliases are of the form fimcn, where n is an integer (0...N) specifying +the IP's instance index. Why? Isn't it up to the DT author whether they care if each fimc node is assigned a specific identification v.s. whether identification is assigned automatically? +Optional properties + + - clock-frequency - maximum FIMC local clock (LCLK) frequency Again, I'd expect a clocks property here instead. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 05/10] s5p-fimc: Add device tree based sensors registration
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote: The sensor (I2C and/or SPI client) devices are instantiated by their corresponding control bus drivers. Since the I2C client's master clock is often provided by a video bus receiver (host interface) or other than I2C/SPI controller device, the drivers of those client devices are not accessing hardware in their driver's probe() callback. Instead, after enabling clock, the host driver calls back into a sub-device when it wants to activate them. This pattern is used by some in-tree drivers and this patch also uses it for DT case. This patch is intended as a first step for adding device tree support to the S5P/Exynos SoC camera drivers. The second one is adding support for asynchronous sub-devices registration and clock control from sub-device driver level. The bindings shall not change when asynchronous probing support is added. diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt +The sensor device nodes should be added as their control bus controller I think as should be to? +(e.g. I2C0) child nodes and linked to a port node in the csis or parallel-ports +node, using common the common video interfaces bindings, i.e. port/endpoint +node pairs. The implementation of this binding requires clock-frequency +property to be present in the sensor device nodes. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 06/10] s5p-fimc: Use pinctrl API for camera ports configuration
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote: Before the camera ports can be used the pinmux needs to be configured properly. This patch adds a function to set the camera ports pinctrl to a default state within the media driver's probe(). The camera port(s) are then configured for the video bus operation. diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt +- pinctrl-names: pinctrl names for camera port pinmux control, at least + default needs to be specified. +- pinctrl-0...N : pinctrl properties corresponding to pinctrl-names + A reference to the binding document describing the pin control bindings would be appropriate here. Given that reference, I'm not sure if spelling out the property names makes sense since it feels a little like duplication; an alternative might be simply: The pinctrl bindings defined in ../../../pinctrl/pinctrl-bindings.txt must be used to define a pinctrl state named default. However, this isn't a big deal; it's fine either way. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] drm/exynos: Add device tree based discovery support for G2D
On 01/31/2013 06:27 PM, Inki Dae wrote:
Hi Kukjin,
-Original Message-
From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
ow...@vger.kernel.org] On Behalf Of Kukjin Kim
Sent: Friday, February 01, 2013 9:15 AM
To: 'Sylwester Nawrocki'; 'Inki Dae'
Cc: 'Sachin Kamat'; linux-media@vger.kernel.org; dri-
de...@lists.freedesktop.org; devicetree-disc...@lists.ozlabs.org;
patc...@linaro.org; s.nawro...@samsung.com
Subject: RE: [PATCH 2/2] drm/exynos: Add device tree based discovery
support for G2D
Sylwester Nawrocki wrote:
Hi Inki,
Hi Sylwester and Inki,
On 01/31/2013 02:30 AM, Inki Dae wrote:
-Original Message-
From: Sylwester Nawrocki [mailto:sylvester.nawro...@gmail.com]
Sent: Thursday, January 31, 2013 5:51 AM
To: Inki Dae
Cc: Sachin Kamat; linux-media@vger.kernel.org; dri-
de...@lists.freedesktop.org; devicetree-disc...@lists.ozlabs.org;
patc...@linaro.org; s.nawro...@samsung.com
Subject: Re: [PATCH 2/2] drm/exynos: Add device tree based discovery
support for G2D
On 01/30/2013 09:50 AM, Inki Dae wrote:
+static const struct of_device_id exynos_g2d_match[] = {
+ { .compatible = samsung,g2d-v41 },
not only Exynos5 and also Exyno4 has the g2d gpu and drm-based g2d
driver shoud support for all Exynos SoCs. How about using
samsung,exynos5-g2d instead and adding a new property 'version' to
identify ip version more surely? With this, we could know which SoC
and its g2d ip version. The version property could have '0x14' or
others. And please add descriptions to dt document.
Err no. Are you suggesting using samsung,exynos5-g2d compatible
string
for Exynos4 specific IPs ? This would not be correct, and you still
can
I assumed the version 'v41' is the ip for Exynos5 SoC. So if this
version
means Exynos4 SoC then it should be samsung,exynos4-g2d.
Yes, v3.0 is implemented in the S5PC110 (Exynos3110) SoCs and
Exynos4210,
V4.1 can be found in Exynos4212 and Exynos4412, if I'm not mistaken.
So we could have:
compatible = samsung,exynos-g2d-3.0 /* for Exynos3110, Exynos4210 */
compatible = samsung,exynos-g2d-4.1 /* for Exynos4212, Exynos4412 */
In my opinion, this is better than later. Because as I said, when we can
use
IP version to identify, it is more clear and can be used
One more, how about following?
compatible = samsung,g2d-3.0
compatible = samsung,g2d-4.1
I think compatible string should be considered case by case.
For example,
If compatible = samsung,g2d-3.0 is added to exynos4210.dtsi, it'd be
reasonable. But what if that compatible string is added to exynos4.dtsi?.
This case isn't considered for exynos4412 SoC with v4.1.
You can always add the most common value for the compatible property
into exynos4.dtsi, and then override it in exyons4210.dtsi, or other files.
Still, the idea of including the SoC version in the compatible value is
a good idea.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] Add common binding documentation for video interfaces
On 12/15/2012 02:13 PM, Sylwester Nawrocki wrote: From: Guennadi Liakhovetski g.liakhovet...@gmx.de This patch adds a document describing common OF bindings for video capture, output and video processing devices. It is currently mainly focused on video capture devices, with data interfaces defined in standards like ITU-R BT.656 or MIPI CSI-2. It also documents a method of describing data links between devices. (quickly/briefly) Reviewed-by: Stephen Warren swar...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/6] video: add of helper for videomode
On 11/13/2012 04:08 AM, Thierry Reding wrote: On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote: This adds support for reading display timings from DT or/and convert one of those timings to a videomode. The of_display_timing implementation supports multiple children where each property can have up to 3 values. All children are read into an array, that can be queried. of_get_videomode converts exactly one of that timings to a struct videomode. diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt + - clock-frequency: displayclock in Hz display clock? I /think/ I had suggested naming this clock-frequency before so that the property name would be more standardized; other bindings use that same name. But I'm not too attached to the name I guess. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] of: add helper to parse display timings
On 10/31/2012 03:28 AM, Steffen Trumtrar wrote: Patch description? The patch defines the DT binding as well, which isn't mentioned in the patch subject. new file mode 100644 index 000..04c94a3 --- /dev/null +++ b/Documentation/devicetree/bindings/video/display-timings.txt +Usage in backend + Everything before this point in the binding docs looks reasonable to me. Everything after this point is Linux-specific/internal implementation detail, and hence shouldn't be in the binding document. I only read the DT binding. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/14] media: add a V4L2 OF parser
On 10/10/2012 04:51 PM, Laurent Pinchart wrote: On Wednesday 10 October 2012 10:50:20 Stephen Warren wrote: On 10/10/2012 07:18 AM, Laurent Pinchart wrote: On Monday 08 October 2012 17:15:53 Guennadi Liakhovetski wrote: ... But how do you get the subdev pointer? With the notifier I get it from i2c_get_clientdata(client) and what do you do without it? How do you get to the client? And can't it get that from DT as well? No, I don't think there is a way to get a device pointer from a DT node. I don't believe there's a generic API for this (although perhaps there could be), but it can be implemented quite easily. For example, on Tegra, the SMMU needs to flip a bit in the AHB register space in order to enable itself. The SMMU DT node contains a phandle that points at the AHB DT node. The SMMU driver parses the phandle and passes the DT node pointer to the AHB driver. The AHB driver looks up the struct device that was instantiated for that node. See drivers/amba/tegra-ahb.c:tegra_ahb_enable_smmu(). There are a few other cases of similar code in the kernel, although I can't remember the others! That's a very naive approach, but what about storing the struct device in struct device_node when the device is instantiated ? It's so simple that there's probably a good reason why that hasn't been implemented though. It sounds like that would work. The advantage of calling a function in the driver for the node, rather than just grabbing something from the node directly in code unrelated to the driver for the node, is that it any knowledge of what kind of device/driver is handling that node is embedded into a function in the driver for the node, not the driver using the node, which makes everything a bit more type-safe. Now, perhaps the implementation of that function could just pull a field out of struct of_node rather than calling driver_find_device(), but it'd then have to validate that the struct device that was found was truly managed by the expected driver, for safety. I assume that's pretty simple, but haven't checked. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/14] media: add a V4L2 OF parser
On 10/10/2012 07:18 AM, Laurent Pinchart wrote: On Monday 08 October 2012 17:15:53 Guennadi Liakhovetski wrote: ... But how do you get the subdev pointer? With the notifier I get it from i2c_get_clientdata(client) and what do you do without it? How do you get to the client? And can't it get that from DT as well? No, I don't think there is a way to get a device pointer from a DT node. I don't believe there's a generic API for this (although perhaps there could be), but it can be implemented quite easily. For example, on Tegra, the SMMU needs to flip a bit in the AHB register space in order to enable itself. The SMMU DT node contains a phandle that points at the AHB DT node. The SMMU driver parses the phandle and passes the DT node pointer to the AHB driver. The AHB driver looks up the struct device that was instantiated for that node. See drivers/amba/tegra-ahb.c:tegra_ahb_enable_smmu(). There are a few other cases of similar code in the kernel, although I can't remember the others! -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/08/2012 02:25 AM, Guennadi Liakhovetski wrote: On Fri, 5 Oct 2012, Stephen Warren wrote: On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote: Hi Steffen Sorry for chiming in so late in the game, but I've long been wanting to have a look at this and compare with what we do for V4L2, so, this seems a great opportunity to me:-) On Thu, 4 Oct 2012, Steffen Trumtrar wrote: diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt +timings-subnode +--- + +required properties: + - hactive, vactive: Display resolution + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters + in pixels + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in + lines + - clock: displayclock in Hz You're going to hate me for this, but eventually we want to actually reference clock objects in our DT bindings. For now, even if you don't want to actually add clock phandles and stuff here, I think, using the standard clock-frequency property would be much better! In a definition of a display timing, we will never need to use the clock binding; the clock binding would be used by the HW module that is generating a timing, not by the timing definition itself. You mean clock consumer bindings will be in the display device DT node? And the display-timings node will be its child? Yes ... + - interlaced (bool) Is interlaced a property of the hardware, i.e. of the board? Can the same display controller on one board require interlaced data and on another board - progressive? Interlace is a property of a display mode. It's quite possible for a particular display controller to switch between interlace and progressive output at run-time. For example, reconfiguring the output between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to a built-in LCD display, you're probably always going to be driving the single mode required by the panel, and that mode will likely always be progressive. However, since this binding attempts to describe any display timing, I think we still need this property per mode. But why do you need this in the DT then at all? Because the driver for the display controller has no idea what display or panel will be connected to it. If it's fixed, as required per display controller, then its driver will know it. If it's runtime configurable, then it's a purely software parameter and doesn't depend on the board? interlace-vs-progressive isn't fixed, as required per display controller, but is a property of the mode being sent by the display controller, and the requirements for that mode are driven by the panel/display connected to the display controller, not the display controller, in general. ... BTW, I'm not very familiar with display interfaces, but for interlaced you probably sometimes use a field signal, whose polarity you also want to specify here? We use a field-even-active integer property for it. I think that's a property of the display controller itself, rather than an individual mode, although I'm not 100% certain. My assertion is that the physical interface that the display controller is driving will determine whether embedded or separate sync is used, and in the separate sync case, how the field signal is defined, and that all interlace modes driven over that interface will use the same field signal definition. In general, I might be misunderstanding something, but don't we have to distinguish between 2 types of information about display timings: (1) is defined by the display controller requirements, is known to the display driver and doesn't need to be present in timings DT. We did have some of these parameters in board data previously, because we didn't have proper display controller drivers... Yes, there probably is data of that kind, but the display mode timings binding is only address standardized display timings information, not controller-specific information, and hence doesn't cover this case. (2) is board specific configuration, and is such it has to be present in DT. Certainly, yes. In that way, doesn't interlaced belong to type (1) and thus doesn't need to be present in DT? I don't believe so. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/08/2012 06:20 AM, Tomi Valkeinen wrote: On Mon, 2012-10-08 at 14:04 +0200, Laurent Pinchart wrote: On Monday 08 October 2012 12:01:18 Tomi Valkeinen wrote: On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski wrote: ... Of course, if this is about describing the hardware, the default-mode property doesn't really fit in... Maybe we should rename it to native-mode then ? Hmm, right, if it means native mode, then it is describing the hardware. But would it make sense to require that the native mode is the first mode in the list, then? This would make the separate default-mode/native-mode property not needed. I'm not sure if device-tree guarantees that the nodes enumerate in a specific order. If it does, then that may be a reasonable solution. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/14] media: add V4L2 DT binding documentation
On 10/02/2012 08:33 AM, Guennadi Liakhovetski wrote:
On Tue, 2 Oct 2012, Rob Herring wrote:
On 09/27/2012 09:07 AM, Guennadi Liakhovetski wrote:
This patch adds a document, describing common V4L2 device tree bindings.
diff --git a/Documentation/devicetree/bindings/media/v4l2.txt
b/Documentation/devicetree/bindings/media/v4l2.txt
One other comment below:
+
+General concept
+---
+
+Video pipelines consist of external devices, e.g. camera sensors,
controlled
+over an I2C, SPI or UART bus, and SoC internal IP blocks, including video
DMA
+engines and video data processors.
+
+SoC internal blocks are described by DT nodes, placed similarly to other
SoC
+blocks. External devices are represented as child nodes of their
respective bus
+controller nodes, e.g. I2C.
+
+Data interfaces on all video devices are described by port child DT
nodes.
+Configuration of a port depends on other devices participating in the data
+transfer and is described by link DT nodes, specified as children of the
+port nodes:
+
+/foo {
+ port@0 {
+ link@0 { ... };
+ link@1 { ... };
+ };
+ port@1 { ... };
+};
+
+If a port can be configured to work with more than one other device on the
same
+bus, a link child DT node must be provided for each of them. If more
than one
+port is present on a device or more than one link is connected to a port, a
+common scheme, using #address-cells, #size-cells and reg properties
is
+used.
+
+Optional link properties:
+- remote: phandle to the other endpoint link DT node.
This name is a little vague. Perhaps endpoint would be better.
endpoint can also refer to something local like in USB case. Maybe
rather the description of the remote property should be improved?
The documentation doesn't show up in all the .dts files that use it; it
might be useful to try and make the .dts file as obviously readable as
possible.
Perhaps remote-port or connected-port would be sufficiently descriptive.
(and yes, I know I'm probably bike-shedding now).
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/14] media: add V4L2 DT binding documentation
On 09/27/2012 08:07 AM, Guennadi Liakhovetski wrote: This patch adds a document, describing common V4L2 device tree bindings. Co-authored-by: Sylwester Nawrocki s.nawro...@samsung.com Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de I think this looks reasonable now, touch wood:-) I guess I won't ack the binding until the v4l2 - something-generic rename happens, but I don't see anything stopping me from doing so once the rename happens. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote: Hi Steffen Sorry for chiming in so late in the game, but I've long been wanting to have a look at this and compare with what we do for V4L2, so, this seems a great opportunity to me:-) On Thu, 4 Oct 2012, Steffen Trumtrar wrote: diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt +timings-subnode +--- + +required properties: + - hactive, vactive: Display resolution + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters + in pixels + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in + lines + - clock: displayclock in Hz You're going to hate me for this, but eventually we want to actually reference clock objects in our DT bindings. For now, even if you don't want to actually add clock phandles and stuff here, I think, using the standard clock-frequency property would be much better! In a definition of a display timing, we will never need to use the clock binding; the clock binding would be used by the HW module that is generating a timing, not by the timing definition itself. That said, your comment about renaming the property to avoid any kind of conceptual conflict is still quite valid. This is bike-shedding, but pixel-clock might be more in line with typical video mode terminology, although there's certainly preference in DT for using the generic term clock-frequency that you proposed. Either is fine by me. +optional properties: + - hsync-active-high (bool): Hsync pulse is active high + - vsync-active-high (bool): Vsync pulse is active high For the above two we also considered using bool properties but eventually settled down with integer ones: - hsync-active = 1 for active-high and 0 for active low. This has the added advantage of being able to omit this property in the .dts, which then doesn't mean, that the polarity is active low, but rather, that the hsync line is not used on this hardware. So, maybe it would be good to use the same binding here too? I agree. This also covers the case where analog display connectors often use polarity to differentiate similar modes, yet digital connectors often always use a fixed polarity since the receiving device can measure the signal in more complete ways. If the board HW inverts these lines, the same property names can exist in the display controller itself, and the two values XORd together to yield the final output polarity. + - de-active-high (bool): Data-Enable pulse is active high + - pixelclk-inverted (bool): pixelclock is inverted We don't (yet) have a de-active property in V4L, don't know whether we'll ever have to distingsuish between what some datasheets call HREF and HSYNC in DT, but maybe similarly to the above an integer would be preferred. As for pixclk, we call the property pclk-sample and it's also an integer. Thinking about this more: de-active-high is likely to be a board-specific property and hence something in the display controller, not in the mode definition? + - interlaced (bool) Is interlaced a property of the hardware, i.e. of the board? Can the same display controller on one board require interlaced data and on another board - progressive? Interlace is a property of a display mode. It's quite possible for a particular display controller to switch between interlace and progressive output at run-time. For example, reconfiguring the output between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to a built-in LCD display, you're probably always going to be driving the single mode required by the panel, and that mode will likely always be progressive. However, since this binding attempts to describe any display timing, I think we still need this property per mode. BTW, I'm not very familiar with display interfaces, but for interlaced you probably sometimes use a field signal, whose polarity you also want to specify here? We use a field-even-active integer property for it. I think that's a property of the display controller itself, rather than an individual mode, although I'm not 100% certain. My assertion is that the physical interface that the display controller is driving will determine whether embedded or separate sync is used, and in the separate sync case, how the field signal is defined, and that all interlace modes driven over that interface will use the same field signal definition. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
...
+ for_each_child_of_node(timings_np, entry) {
+ struct signal_timing *st;
+
+ st = of_get_display_timing(entry);
+
+ if (!st)
+ continue;
I wonder if that shouldn't be an error?
In the sense of a pr_err not a -EINVAL I presume?! It is a little bit quiet in
case of a faulty spec, that is right.
I did mean return an error; if we try to parse something and can't,
shouldn't we return an error?
I suppose it may be possible to limp on and use whatever subset of modes
could be parsed and drop the others, which is what this code does, but
the code after the loop would definitely return an error if zero timings
were parseable.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote: A patch description would be useful for something like this. diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt new file mode 100644 ... +Usage in backend + Everything before that point in the file looks fine to me. Everything after this point in the file seems to be Linux-specific implementation details. Does it really belong in the DT binding documentation, rather than some Linux-specific documentation file? +struct drm_display_mode +=== + + +--+-+--+---+ + | | | | | ↑ + | | | | | | + | | | | | | + +--###--+---+ | I suspect the entire horizontal box above (and the entire vertical box all the way down the left-hand side) should be on the bottom/right instead of top/left. The reason I think this is because all of vsync_start, vsync_end, vdisplay have to be referenced to some known point, which is usually zero or the start of the timing definition, /or/ there would be some value indicating the size of the top marging/porch in order to say where those other values are referenced to. + | # ↑ ↑ ↑# | | | + | # | | |# | | | + | # | | |# | | | + | # | | |# | | | + | # | | |# | | | + | # | | | hdisplay # | | | + | #--++---# | | | + | # | | |# | | | + | # |vsync_start |# | | | + | # | | |# | | | + | # | |vsync_end |# | | | + | # | | |vdisplay# | | | + | # | | |# | | | + | # | | |# | | | + | # | | |# | | | vtotal + | # | | |# | | | + | # | | | hsync_start# | | | + | #--+-+--+--| | | + | # | | |# | | | + | # | | | hsync_end # | | | + | #--+-+--+--| | + | # | | ↓# | | | + +--|#|--+---+ | + | | | | | | | | + | | | | | | | | + | | ↓ | | | | | + +--+-+---+--+---+ | + | | | | | | | + | | | | | | | + | | ↓ | | | ↓ + +--+-+--+---+ + htotal + - diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c +static int parse_property(struct device_node *np, char *name, + struct timing_entry *result) + if (cells == 1) + ret = of_property_read_u32_array(np, name, result-typ, cells); Should that branch not just set result-min/max to typ as well? Presumably it'd prevent any code that interprets struct timing_entry from having to check if those values were 0 or not? + else if (cells == 3) + ret = of_property_read_u32_array(np, name, result-min, cells); +struct display_timings
Re: [PATCH 2/2 v6] of: add generic videomode description
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
+++ b/drivers/of/of_videomode.c
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
+ st = display_timings_get(disp, index);
+
+ if (!st) {
It's a little odd to leave a blank line between those two lines.
Only half of the code in this file seems OF-related; the routines to
convert a timing to a videomode or drm display mode seem like they'd be
useful outside device tree, so I wonder if putting them into
of_videomode.c is the correct thing to do. Still, it's probably not a
big deal.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] of: add helper to parse display specs
On 09/24/2012 09:35 AM, Steffen Trumtrar wrote:
Parse a display-node with timings and hardware-specs from devictree.
diff --git a/Documentation/devicetree/bindings/video/display
b/Documentation/devicetree/bindings/video/display
new file mode 100644
index 000..722766a
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display
This should be display.txt.
@@ -0,0 +1,208 @@
+display bindings
+==
+
+display-node
+
I'm not personally convinced about the direction this is going. While I
think it's reasonable to define DT bindings for displays, and DT
bindings for display modes, I'm not sure that it's reasonable to couple
them together into a single binding.
I think creating a well-defined timing binding first will be much
simpler than doing so within the context of a display binding; the
scope/content of a general display binding seems much less well-defined
to me at least, for reasons I mentioned before.
+required properties:
+ - none
+
+optional properties:
+ - default-timing: the default timing value
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
At least those two properties should exist in the display timing instead
(or perhaps as well). There are certainly cases where different similar
display modes are differentiated by hsync/vsync polarity more than
anything else. This is probably more likely with analog display
connectors than digital, but I see no reason why a DT binding for
display timing shouldn't cover both.
+ - de-active-high (bool): Data-Enable pulse is active high
+ - pixelclk-inverted (bool): pixelclock is inverted
+ - pixel-per-clk
pixel-per-clk is probably something that should either be part of the
timing definition, or something computed internally to the display
driver based on rules for the signal type, rather than something
represented in DT.
The above comment assumes this property is intended to represent DVI's
requirement for pixel clock doubling for low-pixel-clock-rate modes. If
it's something to do with e.g. a single-data-rate vs. double-data-rate
property of the underlying physical connection, that's most likely
something that should be defined in a binding specific to e.g. LVDS,
rather than something generic.
+ - link-width: number of channels (e.g. LVDS)
+ - bpp: bits-per-pixel
+
+timings-subnode
+---
+
+required properties:
+subnodes that specify
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
+ in pixels
+ vfront-porch, vback-porch, vsync-len: Vertical display timing parameters
in
+ lines
+ - clock: displayclock in Hz
+
+There are different ways of describing a display and its capabilities. The
devicetree
+representation corresponds to the one commonly found in datasheets for
displays.
+The description of the display and its timing is split in two parts: first
the display
+properties like size in mm and (optionally) multiple subnodes with the
supported timings.
+If a display supports multiple signal timings, the default-timing can be
specified.
+
+Example:
+
+ display@0 {
+ width-mm = 800;
+ height-mm = 480;
+ default-timing = timing0;
+ timings {
+ timing0: timing@0 {
If you're going to use a unit address (@0) to ensure that node names
are unique (which is not mandatory), then each node also needs a reg
property with matching value, and #address-cells/#size-cells in the
parent. Instead, you could name the nodes something unique based on the
mode name to avoid this, e.g. 1080p24 { ... }.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] of: add helper to parse display specs
On 10/01/2012 01:16 PM, Mitch Bradley wrote:
On 10/1/2012 6:53 AM, Stephen Warren wrote:
On 09/24/2012 09:35 AM, Steffen Trumtrar wrote:
Parse a display-node with timings and hardware-specs from devictree.
diff --git a/Documentation/devicetree/bindings/video/display
b/Documentation/devicetree/bindings/video/display
new file mode 100644
index 000..722766a
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display
This should be display.txt.
@@ -0,0 +1,208 @@
+display bindings
+==
+
+display-node
+
I'm not personally convinced about the direction this is going. While I
think it's reasonable to define DT bindings for displays, and DT
bindings for display modes, I'm not sure that it's reasonable to couple
them together into a single binding.
I think creating a well-defined timing binding first will be much
simpler than doing so within the context of a display binding; the
scope/content of a general display binding seems much less well-defined
to me at least, for reasons I mentioned before.
+required properties:
+ - none
+
+optional properties:
+ - default-timing: the default timing value
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
At least those two properties should exist in the display timing instead
(or perhaps as well). There are certainly cases where different similar
display modes are differentiated by hsync/vsync polarity more than
anything else. This is probably more likely with analog display
connectors than digital, but I see no reason why a DT binding for
display timing shouldn't cover both.
+ - de-active-high (bool): Data-Enable pulse is active high
+ - pixelclk-inverted (bool): pixelclock is inverted
+ - pixel-per-clk
pixel-per-clk is probably something that should either be part of the
timing definition, or something computed internally to the display
driver based on rules for the signal type, rather than something
represented in DT.
The above comment assumes this property is intended to represent DVI's
requirement for pixel clock doubling for low-pixel-clock-rate modes. If
it's something to do with e.g. a single-data-rate vs. double-data-rate
property of the underlying physical connection, that's most likely
something that should be defined in a binding specific to e.g. LVDS,
rather than something generic.
+ - link-width: number of channels (e.g. LVDS)
+ - bpp: bits-per-pixel
+
+timings-subnode
+---
+
+required properties:
+subnodes that specify
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing
parameters
+ in pixels
+ vfront-porch, vback-porch, vsync-len: Vertical display timing
parameters in
+ lines
+ - clock: displayclock in Hz
+
+There are different ways of describing a display and its capabilities. The
devicetree
+representation corresponds to the one commonly found in datasheets for
displays.
+The description of the display and its timing is split in two parts: first
the display
+properties like size in mm and (optionally) multiple subnodes with the
supported timings.
+If a display supports multiple signal timings, the default-timing can be
specified.
+
+Example:
+
+ display@0 {
+ width-mm = 800;
+ height-mm = 480;
+ default-timing = timing0;
+ timings {
+ timing0: timing@0 {
If you're going to use a unit address (@0) to ensure that node names
are unique (which is not mandatory), then each node also needs a reg
property with matching value, and #address-cells/#size-cells in the
parent. Instead, you could name the nodes something unique based on the
mode name to avoid this, e.g. 1080p24 { ... }.
I'm concerned that numbered nodes are being misused as arrays.
It's easy to make real arrays by including multiple cells in the value
of each timing parameter, and easy to choose a cell by saying the array
index instead of using the phandle.
In this case though, arrays don't work out so well in my opinion:
We want to describe a set of unrelated display modes that the display
can handle. These are logically separate entities. I don't think
combining the values that represent say 5 different modes into a single
set of properties really makes sense here; a separate node or property
per display mode really does make sense because they're separate objects.
Related, each display timing parameter (e.g. hsync length, position,
...) has a range, so min/typical/max values. These are already
represented as a 3-cell property as I believe you're proposing.
Combining that with a cell that represents n different modes in a single
array seems like it'd end up with something rather hard to read, at
least for humans even if it would be admittedly trivial for a CPU.
--
To unsubscribe from this list
Re: [PATCH] media: add V4L2 DT binding documentation
On 09/11/2012 09:51 AM, Guennadi Liakhovetski wrote:
This patch adds a document, describing common V4L2 device tree bindings.
Co-authored-by: Sylwester Nawrocki s.nawro...@samsung.com
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
Overall, I think this looks pretty reasonable, so:
Acked-by: Stephen Warren swar...@wwwdotorg.org
Just a couple comments:
+++ b/Documentation/devicetree/bindings/media/v4l2.txt
+ ceu0: ceu@0xfe91 {
+ mclk: master_clock {
+ compatible = renesas,ceu-clock;
+ #clock-cells = 1;
Why 1? If there's only 1 clock output from this provider, I don't see a
need for any cells, unless there are some configuration flags?
+ clock-frequency = 5000; /* max clock frequency
*/
+ clock-output-names = mclk;
+ };
+
+ port {
...
+ ceu0_0: link@0 {
+ reg = 0;
+ remote = csi2_2;
+ immutable;
Did we decide immutable was actually needed? Presumably the driver for
the HW in question knows the HW isn't configurable, and would simply not
attempt to apply any configuration even if the .dts author erroneously
provided some?
+ };
+ };
+ };
+
+ i2c0: i2c@0xfff2 {
...
+ ov772x_1: camera@0x21 {
...
+ clocks = mclk 0;
So presumably that could just be clocks = mclk;?
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] media: add V4L2 DT binding documentation
On 09/12/2012 01:28 PM, Guennadi Liakhovetski wrote:
Hi Stephen
On Wed, 12 Sep 2012, Stephen Warren wrote:
On 09/11/2012 09:51 AM, Guennadi Liakhovetski wrote:
This patch adds a document, describing common V4L2 device tree bindings.
Co-authored-by: Sylwester Nawrocki s.nawro...@samsung.com
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
Overall, I think this looks pretty reasonable, so:
...
+ clock-frequency = 5000; /* max clock frequency
*/
+ clock-output-names = mclk;
+ };
+
+ port {
...
+ ceu0_0: link@0 {
+ reg = 0;
+ remote = csi2_2;
+ immutable;
Did we decide immutable was actually needed? Presumably the driver for
the HW in question knows the HW isn't configurable, and would simply not
attempt to apply any configuration even if the .dts author erroneously
provided some?
Well, I've been thinking about this today. I think, bridge drivers will
Sorry, I'm not sure what a bridge driver is; is it any v4l2 device?
call centrally provided helper functions to enumerate links inside ports.
Presumably a given driver would only parse the ports/links of its own DT
node, and hence would be able to provide a parameter to any helper
function that indicated the same information that immutable would?
While doing that they will want to differentiate between links to external
devices with explicit configuration, and links to internal devices, whose
configuration drivers might be able to figure out themselves. How should a
driver find out what device this link is pointing to? Should it follow the
remote phandle and then check the compatible property? The word
immutable is a hint, that this is a link to an internal device, but it
might either be unneeded or be transformed into something more
informative.
I would imagine that a given driver would only ever parse its own DT
node; the far end of any link is purely the domain of the other driver.
I thought that each link node would contain whatever hsync-active,
data-lanes, ... properties that were needed to configure the local device?
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] media: add V4L2 DT binding documentation
On 09/12/2012 03:17 PM, Guennadi Liakhovetski wrote:
On Wed, 12 Sep 2012, Stephen Warren wrote:
On 09/12/2012 01:28 PM, Guennadi Liakhovetski wrote:
Hi Stephen
On Wed, 12 Sep 2012, Stephen Warren wrote:
On 09/11/2012 09:51 AM, Guennadi Liakhovetski wrote:
This patch adds a document, describing common V4L2 device tree bindings.
Co-authored-by: Sylwester Nawrocki s.nawro...@samsung.com
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
Overall, I think this looks pretty reasonable, so:
...
+ clock-frequency = 5000; /* max clock frequency
*/
+ clock-output-names = mclk;
+ };
+
+ port {
...
+ ceu0_0: link@0 {
+ reg = 0;
+ remote = csi2_2;
+ immutable;
Did we decide immutable was actually needed? Presumably the driver for
the HW in question knows the HW isn't configurable, and would simply not
attempt to apply any configuration even if the .dts author erroneously
provided some?
Well, I've been thinking about this today. I think, bridge drivers will
Sorry, I'm not sure what a bridge driver is; is it any v4l2 device?
Right, sorry for not explaining. We call a bridge a device, that's sitting
on a bus like USB, PCI or - as in the SoC case - on an internal one and
interfacing to, say, a video sensor or a TV decoder or encoder. In the SoC
case most primitive bridges just take data from video sensors attached
over, say, an 8-bit parallel bus and DMA it into RAM buffers. Some bridges
can also perform some data processing.
call centrally provided helper functions to enumerate links inside ports.
Presumably a given driver would only parse the ports/links of its own DT
node, and hence would be able to provide a parameter to any helper
function that indicated the same information that immutable would?
Well, let's drop immutable for now, since we don't have a good idea
whether we need it or not. If we do need it, we'll add it later, removing
a part of a standart is more difficult.
While doing that they will want to differentiate between links to external
devices with explicit configuration, and links to internal devices, whose
configuration drivers might be able to figure out themselves. How should a
driver find out what device this link is pointing to? Should it follow the
remote phandle and then check the compatible property? The word
immutable is a hint, that this is a link to an internal device, but it
might either be unneeded or be transformed into something more
informative.
I would imagine that a given driver would only ever parse its own DT
node; the far end of any link is purely the domain of the other driver.
I thought that each link node would contain whatever hsync-active,
data-lanes, ... properties that were needed to configure the local device?
I think, information needed to configure a bridge device to connect to a
camera sensor, like sync polarities etc., might not be sufficient to
configure it to interface to an SoC internal unit, like a MIPI CSI-2
interface. I can see 2 possibilities to recognise, that this link is going
to an internal device: (1) follow the remote phandle, (2) use a
vendor-specific property to specify the remote device type. I guess, you'd
prefer the latter?
I guess I'm still not exactly clear on what/why the bridge device needs
to know; a concrete example might help.
Either way though, (2) sounds like it's probably best; all information
required to configure a given device is within that device's node.
Being pedantic, I'm not sure precisely what you mean by vendor-specific.
If you mean the DT property name would include a vendor prefix, then
yes, I imagine that's quite possible, although it'd be good to try and
create standardized properties that multiple vendors and devices can use
if it makes sense. If you mean something more than that, then I'd argue
that those properties aren't just vendor-specific, but rather specific
to the individual binding for the individual device (which does imply
the vendor prefix, but not the general applicability of the property to
all of a vendor's devices).
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v5] V4L DT bindings
On 09/11/2012 08:02 AM, Guennadi Liakhovetski wrote:
Hi Stephen
Thanks for the review.
On Wed, 5 Sep 2012, Stephen Warren wrote:
On 09/05/2012 04:57 AM, Guennadi Liakhovetski wrote:
Hi all
Version 5 of this RFC is a result of a discussion of its version 4, which
took place during the recent Linux Plumbers conference in San Diego.
Changes are:
(1) remove bus-width properties from device (bridge and client) top level.
This has actually already been decided upon during our discussions with
Sylwester, I just forgot to actually remove them, sorry.
(2) links are now grouped under ports. This should help better describe
device connection topology by making clearer, which interfaces links are
attached to. (help needed: in my notes I see port optional if only one
port is present, but I seem to remember, that the final decision was -
make ports compulsory for uniformity. Which one is true?)
I'd tend to make the port node compulsory.
A related question: What code is going to parse all the port/link
sub-nodes in a device?
I think we'll have to make a generic V4L DT parser. We certainly don't
want each driver reimplement this.
And, how does it know which sub-nodes are ports,
and which are something else entirely? Perhaps the algorithm is that all
port nodes must be named port?
Yes, that was the idea. Is anything speaking against it?
I think that's fine; it's certainly a nice and simple requirement. It's
just a rule that will have to be thought about when designing bindings
for all the devices that use this feature, to make sure they don't
define any other kind of port node that would confuse the parser.
I suppose if this ever becomes a problem, an individual binding could
choose to avoid conflicts by placing the port nodes in some specific
child node of its device node, and the driver would pass the name of
that node into the common parsing code, which would default to using the
device's main node when not otherwise specified. However, we should
avoid the conflicts if we can. In other words:
Normal:
/foo {
port@0 { ... };
port@1 { ... };
};
If there's ever a need to resolve some conflict with that standard layout:
/foo {
media-ports {
port@0 { ... };
port@1 { ... };
};
};
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v5] V4L DT bindings
On 09/05/2012 04:57 AM, Guennadi Liakhovetski wrote:
Hi all
Version 5 of this RFC is a result of a discussion of its version 4, which
took place during the recent Linux Plumbers conference in San Diego.
Changes are:
(1) remove bus-width properties from device (bridge and client) top level.
This has actually already been decided upon during our discussions with
Sylwester, I just forgot to actually remove them, sorry.
(2) links are now grouped under ports. This should help better describe
device connection topology by making clearer, which interfaces links are
attached to. (help needed: in my notes I see port optional if only one
port is present, but I seem to remember, that the final decision was -
make ports compulsory for uniformity. Which one is true?)
I'd tend to make the port node compulsory.
A related question: What code is going to parse all the port/link
sub-nodes in a device? And, how does it know which sub-nodes are ports,
and which are something else entirely? Perhaps the algorithm is that all
port nodes must be named port?
If there were (optionally) no port node, I think the answer to that
question would be a lot more complex, hence why I advocate for it always
being there.
(3) videolink is renamed to just link.
(4) client is renamed to remote and is now used on both sides of
links.
(5) clock-names in clock consumer nodes (e.g., camera sensors) should
reflect clock input pin names from respective datasheets
(6) also serial bus description should be placed under respective link
nodes.
(7) reference remote link DT nodes in remote properties, not to the
parent.
(8) use standard names for SoC-external (e.g., i2c) devices on their
respective busses. Sensor has been proposed, maybe camera is a better
match though.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
// Describe an imaginary configuration: a CEU serving either a parallel ov7725
// sensor, or a serial imx074 sensor, connected over a CSI-2 SoC interface
ceu0: ceu@0xfe91 {
compatible = renesas,sh-mobile-ceu;
reg = 0xfe91 0xa0;
interrupts = 0x880;
mclk: master_clock {
compatible = renesas,ceu-clock;
#clock-cells = 1;
clock-frequency = 5000; /* max clock frequency
*/
clock-output-names = mclk;
};
...
port@0 {
Since there's only 1 port node here, you can drop the @0 here.
#address-cells = 1;
#size-cells = 0;
ov772x_1_1: link@1 {
This isn't a comment on the binding definition, but on the example
itself. The label names (ov772x_1_1 here and csi2_0 below) feel
backwards to me; I'd personally use label names that describe the object
being labelled, rather than the object that's linked to the node being
labelled. In other words, ceu0_1 here, and ov772x_1 at the far end
of this link. But, these are just arbitrary names, so you can name/label
everything whatever you want and it'll still work.
reg = 1; /* local pad # */
remote = ceu0_1; /* remote phandle and
pad # */
bus-width = 8;/* used data lines */
data-shift = 0; /* lines 7:0 are used */
/* If [hv]sync-active are missing, embedded
bt.605 sync is used */
hsync-active = 1; /* active high */
vsync-active = 1; /* active high */
pclk-sample = 1; /* rising */
};
csi2_0: link@0 {
reg = 0;
remote = ceu0_2;
immutable;
};
};
};
i2c0: i2c@0xfff2 {
...
ov772x_1: camera@0x21 {
compatible = omnivision,ov772x;
reg = 0x21;
vddio-supply = regulator1;
vddcore-supply = regulator2;
clock-frequency = 2000;
clocks = mclk 0;
clock-names = xclk;
...
port {
/* With 1 link per port no need in addresses */
ceu0_1: link@0 {
You can drop @0 there too.
bus-width = 8;
remote = ov772x_1_1;
hsync-active = 1;
hsync-active = 0; /* who came up
Re: [RFC] media DT bindings
On 07/31/2012 11:59 PM, Laurent Pinchart wrote:
Hi Stephen,
On Tuesday 31 July 2012 17:29:24 Stephen Warren wrote:
On 07/31/2012 03:22 PM, Laurent Pinchart wrote:
On Tuesday 31 July 2012 14:39:07 Guennadi Liakhovetski wrote:
...
Ok, then, how about
#address-cells = 1;
#size-cells = 0;
...
ov772x-1 = {
reg = 1; /* local pad # */
client = ov772x@0x21-0 0;/* remote phandle and
pad */
The client property looks good, but isn't such a usage of the reg property
an abuse ? Maybe the local pad # should be a device-specific property.
Many hosts won't need it, and on others it would actually need to
reference a subdev, not just a pad.
That's a very odd syntax the the phandle; I assume that ov772x@0x21-0
is supposed to reference some other DT node. However, other nodes are
either referenced by:
foo where foo is a label, and the label name is unlikely to include
the text @0x21; the @ symbol probably isn't even legal in label names.
{/path/to/node} which might include the @0x21 syntax since it might
be part of the node's name, but your example didn't include {}.
I'm not sure what -0 is meant to be in that string - a math
expression, or ...? If it's intended to represent some separate field
relative to the node the phandle references, it needs to be just another
cell.
I'm actually not sure what -0 represents, and I don't think we need the
@0x21-0 at all. I believe ov772x@0x21-0 is supposed to just be a label. We
don't need an extra cell.
Ah, OK. The lexer in dtc has the following definition for label names:
LABEL [a-zA-Z_][a-zA-Z0-9_]*
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] media DT bindings
On 07/31/2012 03:22 PM, Laurent Pinchart wrote:
On Tuesday 31 July 2012 14:39:07 Guennadi Liakhovetski wrote:
...
Ok, then, how about
#address-cells = 1;
#size-cells = 0;
...
ov772x-1 = {
reg = 1; /* local pad # */
client = ov772x@0x21-0 0;/* remote phandle and
pad */
The client property looks good, but isn't such a usage of the reg property an
abuse ? Maybe the local pad # should be a device-specific property. Many
hosts
won't need it, and on others it would actually need to reference a subdev,
not
just a pad.
That's a very odd syntax the the phandle; I assume that ov772x@0x21-0
is supposed to reference some other DT node. However, other nodes are
either referenced by:
foo where foo is a label, and the label name is unlikely to include
the text @0x21; the @ symbol probably isn't even legal in label names.
{/path/to/node} which might include the @0x21 syntax since it might
be part of the node's name, but your example didn't include {}.
I'm not sure what -0 is meant to be in that string - a math
expression, or ...? If it's intended to represent some separate field
relative to the node the phandle references, it needs to be just another
cell.
So overall, perhaps:
/ {
...
pad: something { ... };
...
ov772x@1 = { /* @1 not -1 would be canonical syntax */
reg = 1;
client = pad 0 0;
...
I'm sorry I haven't followed the thread; I'm wondering why a client is a
pad, which to me means a pin/pad/ball on an IC package, so I'm still not
entirely sure if even this makes sense.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
