Re: [PATCH v14 04/10] imx-drm: use defines for clock polarity settings
On Wed, Jun 25, 2014 at 09:43:27AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 25, 2014 at 06:48:45AM +0200, Sascha Hauer wrote:
> > On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote:
> > > +
> > > /*
> > > * Bitfield of Display Interface signal polarities.
> > > */
> > > @@ -37,7 +43,7 @@ struct ipu_di_signal_cfg {
> > > unsigned clksel_en:1;
> > > unsigned clkidle_en:1;
> > > unsigned data_pol:1;/* true = inverted */
> > > - unsigned clk_pol:1; /* true = rising edge */
> > > + unsigned clk_pol:1;
> > > unsigned enable_pol:1;
> > > unsigned Hsync_pol:1; /* true = active high */
> > > unsigned Vsync_pol:1;
> >
> > ...can we rename the flags to more meaningful names instead?
> >
> > unsigned clk_pol_rising_edge:1;
> > unsigned enable_pol_high:1;
> > unsigned hsync_active_high:1;
> > unsigned vsync_active_high:1;
>
> Now look at patch 7, where these become tri-state:
> - don't change
> - rising edge/active high
> - falling edge/active low
>
> So your suggestion is not a good idea.
Hm, you're right.
Still I think we should add a prefix to make the context of the flags
clear.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 v14 04/10] imx-drm: use defines for clock polarity settings
On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote:
> Signed-off-by: Denis Carikli
> ---
> ChangeLog v13->v14:
> - Rebased
> ChangeLog 12->v13:
> - No changes
> ChangeLog 11->v12:
> - Improved the define names to match the hardware:
> ENABLE_POL is not a clock signal but instead an enable signal.
>
> ChangeLog v9->v10:
> - New patch which was splitted out from:
> "staging: imx-drm: Use de-active and pixelclk-active display-timings.".
> - Fixes many issues in "staging: imx-drm: Use de-active and pixelclk-active
> display-timings.":
> - More clear meaning of the polarity settings.
> - The SET_CLK_POL and SET_DE_POL masks are not
> needed anymore.
> ---
> drivers/gpu/ipu-v3/ipu-di.c |4 ++--
> drivers/staging/imx-drm/ipuv3-crtc.c |4 ++--
> include/video/imx-ipu-v3.h |8 +++-
> 3 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
> index c490ba4..d00f357 100644
> --- a/drivers/gpu/ipu-v3/ipu-di.c
> +++ b/drivers/gpu/ipu-v3/ipu-di.c
> @@ -595,7 +595,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct
> ipu_di_signal_cfg *sig)
> }
> }
>
> - if (sig->clk_pol)
> + if (sig->clk_pol == CLK_POL_POSEDGE)
> di_gen |= DI_GEN_POLARITY_DISP_CLK;
>
> ipu_di_write(di, di_gen, DI_GENERAL);
> @@ -606,7 +606,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct
> ipu_di_signal_cfg *sig)
> reg = ipu_di_read(di, DI_POL);
> reg &= ~(DI_POL_DRDY_DATA_POLARITY | DI_POL_DRDY_POLARITY_15);
>
> - if (sig->enable_pol)
> + if (sig->enable_pol == ENABLE_POL_HIGH)
> reg |= DI_POL_DRDY_POLARITY_15;
> if (sig->data_pol)
> reg |= DI_POL_DRDY_DATA_POLARITY;
> diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c
> b/drivers/staging/imx-drm/ipuv3-crtc.c
> index 720868b..7fec438 100644
> --- a/drivers/staging/imx-drm/ipuv3-crtc.c
> +++ b/drivers/staging/imx-drm/ipuv3-crtc.c
> @@ -165,8 +165,8 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc,
> if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> sig_cfg.Vsync_pol = 1;
>
> - sig_cfg.enable_pol = 1;
> - sig_cfg.clk_pol = 0;
> + sig_cfg.enable_pol = ENABLE_POL_HIGH;
> + sig_cfg.clk_pol = CLK_POL_NEGEDGE;
> sig_cfg.width = mode->hdisplay;
> sig_cfg.height = mode->vdisplay;
> sig_cfg.pixel_fmt = out_pixel_fmt;
> diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
> index 3e43e22..305 100644
> --- a/include/video/imx-ipu-v3.h
> +++ b/include/video/imx-ipu-v3.h
> @@ -27,6 +27,12 @@ enum ipuv3_type {
>
> #define IPU_PIX_FMT_GBR24v4l2_fourcc('G', 'B', 'R', '3')
>
> +#define CLK_POL_NEGEDGE 0
> +#define CLK_POL_POSEDGE 1
> +
> +#define ENABLE_POL_LOW 0
> +#define ENABLE_POL_HIGH 1
Adding defines without a proper namespace (IPU_) outside a driver
private header file is not nice. Anyway, instead of adding the
defines ...
> +
> /*
> * Bitfield of Display Interface signal polarities.
> */
> @@ -37,7 +43,7 @@ struct ipu_di_signal_cfg {
> unsigned clksel_en:1;
> unsigned clkidle_en:1;
> unsigned data_pol:1;/* true = inverted */
> - unsigned clk_pol:1; /* true = rising edge */
> + unsigned clk_pol:1;
> unsigned enable_pol:1;
> unsigned Hsync_pol:1; /* true = active high */
> unsigned Vsync_pol:1;
...can we rename the flags to more meaningful names instead?
unsigned clk_pol_rising_edge:1;
unsigned enable_pol_high:1;
unsigned hsync_active_high:1;
unsigned vsync_active_high:1;
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 08/43] imx-drm: ipu-v3: Add units required for video capture
On Sat, Jun 07, 2014 at 02:56:10PM -0700, Steve Longerbeam wrote:
> Adds the following new IPU units:
>
> - Camera Sensor Interface (csi)
> - Sensor Multi FIFO Controller (smfc)
> - Image Converter (ic)
> - Image Rotator (irt)
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/staging/imx-drm/ipu-v3/Makefile |3 +-
> drivers/staging/imx-drm/ipu-v3/ipu-common.c | 67 ++-
> drivers/staging/imx-drm/ipu-v3/ipu-csi.c| 821 ++
> drivers/staging/imx-drm/ipu-v3/ipu-ic.c | 835
> +++
> drivers/staging/imx-drm/ipu-v3/ipu-irt.c| 103
> drivers/staging/imx-drm/ipu-v3/ipu-prv.h| 24 +
> drivers/staging/imx-drm/ipu-v3/ipu-smfc.c | 348 +++
> include/linux/platform_data/imx-ipu-v3.h| 151 +
> 8 files changed, 2350 insertions(+), 2 deletions(-)
> create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-csi.c
> create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-ic.c
> create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-irt.c
> create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-smfc.c
>
> diff --git a/drivers/staging/imx-drm/ipu-v3/Makefile
> b/drivers/staging/imx-drm/ipu-v3/Makefile
> index 28ed72e..79c0c88 100644
> --- a/drivers/staging/imx-drm/ipu-v3/Makefile
> +++ b/drivers/staging/imx-drm/ipu-v3/Makefile
> @@ -1,3 +1,4 @@
> obj-$(CONFIG_DRM_IMX_IPUV3_CORE) += imx-ipu-v3.o
>
> -imx-ipu-v3-objs := ipu-common.o ipu-dc.o ipu-di.o ipu-dp.o ipu-dmfc.o
> +imx-ipu-v3-objs := ipu-common.o ipu-csi.o ipu-dc.o ipu-di.o \
> + ipu-dp.o ipu-dmfc.o ipu-ic.o ipu-irt.o ipu-smfc.o
> diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> index 1c606b5..3d7e28d 100644
> --- a/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> +++ b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> @@ -834,6 +834,10 @@ struct ipu_devtype {
> unsigned long cpmem_ofs;
> unsigned long srm_ofs;
> unsigned long tpm_ofs;
> + unsigned long csi0_ofs;
> + unsigned long csi1_ofs;
> + unsigned long smfc_ofs;
> + unsigned long ic_ofs;
> unsigned long disp0_ofs;
> unsigned long disp1_ofs;
> unsigned long dc_tmpl_ofs;
> @@ -873,8 +877,12 @@ static struct ipu_devtype ipu_type_imx6q = {
> .cpmem_ofs = 0x0030,
> .srm_ofs = 0x0034,
> .tpm_ofs = 0x0036,
> + .csi0_ofs = 0x0023,
> + .csi1_ofs = 0x00238000,
> .disp0_ofs = 0x0024,
> .disp1_ofs = 0x00248000,
> + .smfc_ofs = 0x0025,
> + .ic_ofs = 0x0022,
> .dc_tmpl_ofs = 0x0038,
> .vdi_ofs = 0x00268000,
> .type = IPUV3H,
> @@ -915,8 +923,44 @@ static int ipu_submodules_init(struct ipu_soc *ipu,
> struct device *dev = &pdev->dev;
> const struct ipu_devtype *devtype = ipu->devtype;
>
> + ret = ipu_csi_init(ipu, dev, 0, ipu_base + devtype->csi0_ofs,
> +IPU_CONF_CSI0_EN, ipu_clk);
> + if (ret) {
> + unit = "csi0";
> + goto err_csi_0;
> + }
Please be nice to other SoCs. You set csi0_ofs for i.MX6, but not for
i.MX5. For i.MX5 you silently initialize the CSI with bogus register
offsets. Either test for _ofs == 0 before initializing it or add the
correct values for i.MX51/53 (preferred).
> +int ipu_csi_set_src(struct ipu_csi *csi, u32 vc, bool select_mipi_csi2)
> +{
> + struct ipu_soc *ipu = csi->ipu;
> + int ipu_id = ipu_get_num(ipu);
> + u32 val, bit;
> +
> + /*
> + * Set the muxes that choose between mipi-csi2 and parallel inputs
> + * to the CSI's.
> + */
> + if (csi->ipu->ipu_type == IPUV3HDL) {
> + /*
> + * On D/L, the mux is in GPR13. The TRM is unclear,
> + * but it appears the mux allows selecting the MIPI
> + * CSI-2 virtual channel number to route to the CSIs.
> + */
> + bit = GPR13_IPUV3HDL_CSI_MUX_CTL_SHIFT + csi->id * 3;
> + val = select_mipi_csi2 ? vc << bit : 4 << bit;
> + regmap_update_bits(ipu->gp_reg, IOMUXC_GPR13,
> +0x7 << bit, val);
> + } else if (csi->ipu->ipu_type == IPUV3H) {
> + /*
> + * For Q/D, the mux only exists on IPU0-CSI0 and IPU1-CSI1,
> + * and the routed virtual channel numbers are fixed at 0 and
> + * 3 respectively.
> + */
> + if ((ipu_id == 0 && csi->id == 0) ||
> + (ipu_id == 1 && csi->id == 1)) {
> + bit = GPR1_IPU_CSI_MUX_CTL_SHIFT + csi->ipu->id;
> + val = select_mipi_csi2 ? 0 : 1 << bit;
> + regmap_update_bits(ipu->gp_reg, IOMUXC_GPR1,
> +0x1 << bit, val);
> + }
> + } else {
> + dev_err(csi->ipu->dev,
> + "ERROR: %s: unsupported CPU version!\n",
> + __func__);
> + return
Re: [PATCH 30/43] ARM: dts: imx6: add pin groups for imx6q/dl for IPU1 CSI0
On Sat, Jun 07, 2014 at 02:56:32PM -0700, Steve Longerbeam wrote:
> Signed-off-by: Steve Longerbeam
> Signed-off-by: Jiada Wang
> ---
> arch/arm/boot/dts/imx6qdl.dtsi | 52
>
> 1 file changed, 52 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index 04c978c..d793cd6 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -664,6 +664,58 @@
> iomuxc: iomuxc@020e {
> compatible = "fsl,imx6dl-iomuxc",
> "fsl,imx6q-iomuxc";
> reg = <0x020e 0x4000>;
> +
> + ipu1 {
> + pinctrl_ipu1_csi0_d4_d7:
> ipu1-csi0-d4-d7 {
> + fsl,pins = <
> +
> MX6QDL_PAD_CSI0_DAT4__IPU1_CSI0_DATA04 0x8000
> +
> MX6QDL_PAD_CSI0_DAT5__IPU1_CSI0_DATA05 0x8000
> +
> MX6QDL_PAD_CSI0_DAT6__IPU1_CSI0_DATA06 0x8000
> +
> MX6QDL_PAD_CSI0_DAT7__IPU1_CSI0_DATA07 0x8000
> + >;
> + };
We no longer have the pinctrl groups in the SoC dts files. Please put
them into the boards instead.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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/43] imx-drm: ipu-v3: Add solo/dual-lite IPU device type
Hi Steve,
On Sat, Jun 07, 2014 at 02:56:06PM -0700, Steve Longerbeam wrote:
> Signed-off-by: Jiada Wang
> ---
> drivers/staging/imx-drm/ipu-v3/ipu-common.c | 18 ++
> include/linux/platform_data/imx-ipu-v3.h|1 +
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> index f8e8c56..2d95a7c 100644
> --- a/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> +++ b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> @@ -829,10 +829,28 @@ static struct ipu_devtype ipu_type_imx6q = {
> .type = IPUV3H,
> };
>
> +static struct ipu_devtype ipu_type_imx6dl = {
> + .name = "IPUv3HDL",
> + .cm_ofs = 0x0020,
> + .cpmem_ofs = 0x0030,
> + .srm_ofs = 0x0034,
> + .tpm_ofs = 0x0036,
> + .csi0_ofs = 0x0023,
> + .csi1_ofs = 0x00238000,
> + .disp0_ofs = 0x0024,
> + .disp1_ofs = 0x00248000,
> + .smfc_ofs = 0x0025,
> + .ic_ofs = 0x0022,
> + .vdi_ofs = 0x00268000,
> + .dc_tmpl_ofs = 0x0038,
> + .type = IPUV3HDL,
> +};
This breaks bisectibility. This patch must come after
[PATCH 08/43] imx-drm: ipu-v3: Add units required for video capture
Besides, is there any difference between IPUv3HDL and IPUv3H? Can't you
reuse it?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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: mx1_camera: Remove driver
On Sun, May 11, 2014 at 10:09:11AM +0400, Alexander Shiyan wrote: > That driver hasn't been really maintained for a long time. It doesn't > compile in any way, it includes non-existent headers, has no users, > and marked as "broken" more than year. Due to these factors, mx1_camera > is now removed from the tree. > > Signed-off-by: Alexander Shiyan I remember this driver was in the way of further cleanups. We should remove it. Acked-by: Sascha Hauer Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 PATCH] [media]: of: move graph helpers from drivers/media/v4l2-core to drivers/of
Hi Grant, On Mon, Feb 17, 2014 at 06:14:51PM +, Grant Likely wrote: > On Tue, 11 Feb 2014 07:56:33 -0600, Rob Herring wrote: > > On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel > > wrote: > > > From: Philipp Zabel > > > > > > This patch moves the parsing helpers used to parse connected graphs > > > in the device tree, like the video interface bindings documented in > > > Documentation/devicetree/bindings/media/video-interfaces.txt, from > > > drivers/media/v4l2-core to drivers/of. > > > > This is the opposite direction things have been moving... > > > > > This allows to reuse the same parser code from outside the V4L2 framework, > > > most importantly from display drivers. There have been patches that > > > duplicate > > > the code (and I am going to send one of my own), such as > > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html > > > and others that parse the same binding in a different way: > > > https://www.mail-archive.com/linux-omap@vger.kernel.org/msg100761.html > > > > > > I think that all common video interface parsing helpers should be moved > > > to a > > > single place, outside of the specific subsystems, so that it can be reused > > > by all drivers. > > > > Perhaps that should be done rather than moving to drivers/of now and > > then again to somewhere else. > > This is just parsing helpers though, isn't it? I have no problem pulling > helper functions into drivers/of if they are usable by multiple > subsystems. I don't really understand the model being used though. I > would appreciate a description of the usage model for these functions > for poor folks like me who can't keep track of what is going on in > subsystems. You can find it under Documentation/devicetree/bindings/media/video-interfaces.txt Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 Sat, Jul 20, 2013 at 07:59:10PM -0700, Greg KH wrote: > On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote: > > On Sat, 20 Jul 2013, Greg KH wrote: > > > > > > >>That should be passed using platform data. > > > > > > > > > >Ick, don't pass strings around, pass pointers. If you have platform > > > > >data you can get to, then put the pointer there, don't use a "name". > > > > > > > > I don't think I understood you here :-s We wont have phy pointer > > > > when we create the device for the controller no?(it'll be done in > > > > board file). Probably I'm missing something. > > > > > > Why will you not have that pointer? You can't rely on the "name" as the > > > device id will not match up, so you should be able to rely on the > > > pointer being in the structure that the board sets up, right? > > > > > > Don't use names, especially as ids can, and will, change, that is going > > > to cause big problems. Use pointers, this is C, we are supposed to be > > > doing that :) > > > > Kishon, I think what Greg means is this: The name you are using must > > be stored somewhere in a data structure constructed by the board file, > > right? Or at least, associated with some data structure somehow. > > Otherwise the platform code wouldn't know which PHY hardware > > corresponded to a particular name. > > > > Greg's suggestion is that you store the address of that data structure > > in the platform data instead of storing the name string. Have the > > consumer pass the data structure's address when it calls phy_create, > > instead of passing the name. Then you don't have to worry about two > > PHYs accidentally ending up with the same name or any other similar > > problems. > > Close, but the issue is that whatever returns from phy_create() should > then be used, no need to call any "find" functions, as you can just use > the pointer that phy_create() returns. Much like all other class api > functions in the kernel work. I think the problem here is to connect two from the bus structure completely independent devices. Several frameworks (ASoC, soc-camera) had this problem and this wasn't solved until the advent of devicetrees and their phandles. phy_create might be called from the probe function of some i2c device (the phy device) and the resulting pointer is then needed in some other platform devices (the user of the phy) probe function. The best solution we have right now is implemented in the clk framework which uses a string matching of the device names in clk_get() (at least in the non-dt case). Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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] Print parser position on error
Hi Laurent, On Wed, Jun 19, 2013 at 02:39:48AM +0200, Laurent Pinchart wrote: > Hi Sascha, > > On Monday 10 June 2013 15:56:01 Laurent Pinchart wrote: > > Hi Sascha, > > > > I'm sorry for the late reply. > > > > Great patch set, thank you. It's very helpful. > > Should I got ahead and apply the patch with the proposed modifications below ? That'd be nice. I'm a bit out of the loop at the moment and don't have a setup handy to test the changes. > > No need to cast to a char * here, end is already a char *. > > > > What would you think about adding > > > > + /* endp can be NULL. To avoid spreading NULL checks across the > > +* function, set endp to &end in that case. > > +*/ > > + if (endp == NULL) > > + endp = &end; > > > > at the beginning of the function and removing the endp NULL checks that are > > spread across the code below ? Good idea. Your other suggestions also look good. Regards Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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: mt9p031: add OF support
On Sun, May 26, 2013 at 06:38:54PM +0530, Prabhakar Lad wrote:
> From: Lad, Prabhakar
>
> add OF support for the mt9p031 sensor driver.
> Alongside this patch sorts the header inclusion alphabetically.
>
> Signed-off-by: Lad, Prabhakar
> Cc: Hans Verkuil
> Cc: Laurent Pinchart
> Cc: Mauro Carvalho Chehab
> Cc: Guennadi Liakhovetski
> Cc: Sylwester Nawrocki
> Cc: Sakari Ailus
> Cc: Grant Likely
> Cc: Sascha Hauer
> Cc: Rob Herring
> Cc: Rob Landley
> Cc: Arnd Bergmann
> Cc: devicetree-disc...@lists.ozlabs.org
> Cc: davinci-linux-open-sou...@linux.davincidsp.com
> Cc: linux-...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
Reviewed-by: Sascha Hauer
Sascha
> ---
> Changes for NON RFC v1:
> 1: added missing call for of_node_put().
>
> Changes for RFC v4 (https://patchwork.kernel.org/patch/2556251/):
> 1: Renamed "gpio-reset" property to "reset-gpios".
> 2: Dropped assigning the driver data from the of node.
>
> Changes for RFC v3(https://patchwork.kernel.org/patch/2515921/):
> 1: Dropped check if gpio-reset is valid.
> 2: Fixed some code nits.
> 3: Included a reference to the V4L2 DT bindings documentation.
>
> Changes for RFC v2 (https://patchwork.kernel.org/patch/2510201/):
> 1: Used '-' instead of '_' for device properties.
> 2: Specified gpio reset pin as phandle in device node.
> 3: Handle platform data properly even if kernel is compiled with
> devicetree support.
> 4: Used dev_* for messages in drivers instead of pr_*.
> 5: Changed compatible property to "aptina,mt9p031" and "aptina,mt9p031m".
> 6: Sorted the header inclusion alphabetically and fixed some minor code nits.
>
> RFC v1: https://patchwork.kernel.org/patch/2498791/
>
> .../devicetree/bindings/media/i2c/mt9p031.txt | 40 ++
> drivers/media/i2c/mt9p031.c| 43
> +++-
> 2 files changed, 81 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9p031.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> new file mode 100644
> index 000..59d613c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> @@ -0,0 +1,40 @@
> +* Aptina 1/2.5-Inch 5Mp CMOS Digital Image Sensor
> +
> +The Aptina MT9P031 is a 1/2.5-inch CMOS active pixel digital image sensor
> with
> +an active array size of 2592H x 1944V. It is programmable through a simple
> +two-wire serial interface.
> +
> +Required Properties :
> +- compatible : value should be either one among the following
> + (a) "aptina,mt9p031" for mt9p031 sensor
> + (b) "aptina,mt9p031m" for mt9p031m sensor
> +
> +- input-clock-frequency : Input clock frequency.
> +
> +- pixel-clock-frequency : Pixel clock frequency.
> +
> +Optional Properties :
> +- reset-gpios: Chip reset GPIO
> +
> +For further reading of port node refer
> Documentation/devicetree/bindings/media/
> +video-interfaces.txt.
> +
> +Example:
> +
> + i2c0@1c22000 {
> + ...
> + ...
> + mt9p031@5d {
> + compatible = "aptina,mt9p031";
> + reg = <0x5d>;
> + reset-gpios = <&gpio3 30 0>;
> +
> + port {
> + mt9p031_1: endpoint {
> + input-clock-frequency = <600>;
> + pixel-clock-frequency = <9600>;
> + };
> + };
> + };
> + ...
> + };
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index bf49899..bb1f993 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -16,9 +16,10 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -28,6 +29,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "aptina-pll.h"
> @@ -928,10 +930,37 @@ static const struct v4l2_subdev_internal_ops
> mt9p031_subdev_internal_ops = {
> * Driver initialization and probing
> */
>
> +static struct mt9p031_platform_data *
> +mt9p031_get_pdata(struct i2c_client *client)
> +{
> + struct mt9p031_platform_data *pdata = NULL;
> + struct device_node *np;
> +
> + if (!IS_ENABLED(
Re: [PATCH RFC v3] media: i2c: mt9p031: add OF support
On Tue, May 14, 2013 at 12:59:27AM +0200, Laurent Pinchart wrote:
> Hi Sascha,
>
> On Monday 13 May 2013 12:46:04 Sascha Hauer wrote:
> > On Wed, May 08, 2013 at 12:37:29PM +0200, Laurent Pinchart wrote:
> > > Hi Prabhakar,
> > >
> > > On Wednesday 08 May 2013 10:19:57 Prabhakar Lad wrote:
> > > > On Wed, May 8, 2013 at 7:32 AM, Laurent Pinchart wrote:
> > > > > On Tuesday 07 May 2013 15:10:36 Prabhakar Lad wrote:
> > > > >> On Mon, May 6, 2013 at 8:29 PM, Prabhakar Lad wrote:
> > > > >> > On Fri, May 3, 2013 at 8:04 PM, Arnd Bergmann wrote:
> > > > >> >> On Friday 03 May 2013, Prabhakar Lad wrote:
> > > > >> > [snip]
> > > > >> >
> > > > >> >>> +}
> > > > >> >>
> > > > >> >> Ok, good.
> > > > >> >>
> > > > >> >>> @@ -955,7 +998,17 @@ static int mt9p031_probe(struct i2c_client
> > > > >> >>> *client,
> > > > >> >>>
> > > > >> >>> mt9p031->pdata = pdata;
> > > > >> >>> mt9p031->output_control = MT9P031_OUTPUT_CONTROL_DEF;
> > > > >> >>> mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> > > > >> >>>
> > > > >> >>> - mt9p031->model = did->driver_data;
> > > > >> >>> +
> > > > >> >>> + if (!client->dev.of_node) {
> > > > >> >>> + mt9p031->model = (enum
> > > > >> >>> mt9p031_model)did->driver_data;
> > > > >> >>> + } else {
> > > > >> >>> + const struct of_device_id *of_id;
> > > > >> >>> +
> > > > >> >>> + of_id =
> > > > >> >>> of_match_device(of_match_ptr(mt9p031_of_match),
> > > > >> >>> + &client->dev);
> > > > >> >>> + if (of_id)
> > > > >> >>> + mt9p031->model = (enum
> > > > >> >>> mt9p031_model)of_id->data;
> > > > >> >>> + }
> > > > >> >>>
> > > > >> >>> mt9p031->reset = -1;
> > > > >> >>
> > > > >> >> Is this actually required? I thought the i2c core just compared
> > > > >> >> the
> > > > >> >> part of the "compatible" value after the first comma to the
> > > > >> >> string, so
> > > > >> >> "mt9p031->model = (enum mt9p031_model)did->driver_data" should
> > > > >> >> work
> > > > >> >> in both cases.
> >
> > At least on v3.8 I just checked that 'did' is indeed NULL for the
> > devicetree case. Also I see no indication that i2c starts comparing
> > after the first comma in the string.
> >
> > > > >> > I am OK with "mt9p031->model = (enum
> > > > >> > mt9p031_model)did->driver_data"
> > > > >> > but I see still few drivers doing this, I am not sure for what
> > > > >> > reason.
> > > > >> > If everyone is OK with it I can drop the above change.
> > > > >>
> > > > >> My bad, while booting with DT the i2c_device_id ie did in this case
> > > > >> will
> > > > >> be NULL, so the above changes are required :-)
> > > > >
> > > > > I've just tested your patch, and did isn't NULL when booting my
> > > > > Beagleboard with DT (on v3.9-rc5).
> > > >
> > > > I am pretty much sure you tested it compatible property as
> > > > "aptina,mt9p031"
> > > > if the compatible property is set to "aptina,mt9p031m" the did will be
> > > > NULL.>
> > > I've tested both :-)
> >
> > Sorry to nag, but did you use "aptina,mt9p031[m]" as a compatible string or
> > did you use "mt9p031[m]". With "aptina,..." 'did' should really be NULL.
>
> I've used "aptina,mt9p031[m]".
>
> Please see the of_modalias_node() call in of_i2c_register_devices()
> (drivers/of/of-i2c.c), that's where the I2C device type name should be
> initialized.
Ok, got it. I still had the older aptina,mt9p031m-sensor binding in my
patch.
Sorry for the noise.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 v3] media: i2c: mt9p031: add OF support
On Wed, May 08, 2013 at 12:37:29PM +0200, Laurent Pinchart wrote:
> Hi Prabhakar,
>
> On Wednesday 08 May 2013 10:19:57 Prabhakar Lad wrote:
> > On Wed, May 8, 2013 at 7:32 AM, Laurent Pinchart wrote:
> > > On Tuesday 07 May 2013 15:10:36 Prabhakar Lad wrote:
> > >> On Mon, May 6, 2013 at 8:29 PM, Prabhakar Lad wrote:
> > >> > On Fri, May 3, 2013 at 8:04 PM, Arnd Bergmann wrote:
> > >> >> On Friday 03 May 2013, Prabhakar Lad wrote:
> > >> > [snip]
> > >> >
> > >> >>> +}
> > >> >>
> > >> >> Ok, good.
> > >> >>
> > >> >>> @@ -955,7 +998,17 @@ static int mt9p031_probe(struct i2c_client
> > >> >>> *client,
> > >> >>>
> > >> >>> mt9p031->pdata = pdata;
> > >> >>> mt9p031->output_control = MT9P031_OUTPUT_CONTROL_DEF;
> > >> >>> mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> > >> >>>
> > >> >>> - mt9p031->model = did->driver_data;
> > >> >>> +
> > >> >>> + if (!client->dev.of_node) {
> > >> >>> + mt9p031->model = (enum
> > >> >>> mt9p031_model)did->driver_data;
> > >> >>> + } else {
> > >> >>> + const struct of_device_id *of_id;
> > >> >>> +
> > >> >>> + of_id =
> > >> >>> of_match_device(of_match_ptr(mt9p031_of_match),
> > >> >>> + &client->dev);
> > >> >>> + if (of_id)
> > >> >>> + mt9p031->model = (enum
> > >> >>> mt9p031_model)of_id->data;
> > >> >>> + }
> > >> >>>
> > >> >>> mt9p031->reset = -1;
> > >> >>
> > >> >> Is this actually required? I thought the i2c core just compared the
> > >> >> part of the "compatible" value after the first comma to the string, so
> > >> >> "mt9p031->model = (enum mt9p031_model)did->driver_data" should work
> > >> >> in both cases.
At least on v3.8 I just checked that 'did' is indeed NULL for the
devicetree case. Also I see no indication that i2c starts comparing
after the first comma in the string.
> > >> >
> > >> > I am OK with "mt9p031->model = (enum mt9p031_model)did->driver_data"
> > >> > but I see still few drivers doing this, I am not sure for what reason.
> > >> > If everyone is OK with it I can drop the above change.
> > >>
> > >> My bad, while booting with DT the i2c_device_id ie did in this case will
> > >> be NULL, so the above changes are required :-)
> > >
> > > I've just tested your patch, and did isn't NULL when booting my
> > > Beagleboard with DT (on v3.9-rc5).
> >
> > I am pretty much sure you tested it compatible property as "aptina,mt9p031"
> > if the compatible property is set to "aptina,mt9p031m" the did will be NULL.
>
> I've tested both :-)
Sorry to nag, but did you use "aptina,mt9p031[m]" as a compatible string or did
you use "mt9p031[m]". With "aptina,..." 'did' should really be NULL.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] Print more detailed parse error messages
On Wed, May 08, 2013 at 03:27:53PM +0200, Sascha Hauer wrote:
> The following errors usually resulted in the same 'Unable to parse link'
> message:
>
> - one of the given entities does not exist
> - one of the pads of a given entity does not exist
> - No link exists between given pads
> - syntax error in link description
>
> Add more detailed error messages to give the user a clue what is going wrong.
>
> Signed-off-by: Sascha Hauer
> ---
> src/mediactl.c | 35 ++-
> 1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/src/mediactl.c b/src/mediactl.c
> index 4783a58..c65de50 100644
> --- a/src/mediactl.c
> +++ b/src/mediactl.c
> @@ -537,31 +537,42 @@ struct media_pad *media_parse_pad(struct media_device
> *media,
>
> if (*p == '"') {
> for (end = (char *)p + 1; *end && *end != '"'; ++end);
> - if (*end != '"')
> + if (*end != '"') {
> + media_dbg(media, "missing matching '\"'\n");
> return NULL;
> + }
>
> entity = media_get_entity_by_name(media, p + 1, end - p - 1);
> - if (entity == NULL)
> + if (entity == NULL) {
> + media_dbg(media, "no such entity \"%.*s\"\n", end - p -
> 1, p + 1);
> return NULL;
> + }
>
> ++end;
> } else {
> entity_id = strtoul(p, &end, 10);
> entity = media_get_entity_by_id(media, entity_id);
> - if (entity == NULL)
> + if (entity == NULL) {
> + media_dbg(media, "no such entity %d\n", entity_id);
> return NULL;
> + }
> }
> for (; isspace(*end); ++end);
>
> - if (*end != ':')
> + if (*end != ':') {
> + media_dbg(media, "Expected ':'\n", *end);
> return NULL;
> + }
> +
> for (p = end + 1; isspace(*p); ++p);
>
> pad = strtoul(p, &end, 10);
> - for (p = end; isspace(*p); ++p);
Oops, this maybe should be a separate patch. This is not needed here...
>
> - if (pad >= entity->info.pads)
> + if (pad >= entity->info.pads) {
> + media_dbg(media, "No pad '%d' on entity \"%s\". Maximum pad
> number is %d\n",
> + pad, entity->info.name, entity->info.pads - 1);
> return NULL;
> + }
>
> for (p = end; isspace(*p); ++p);
... since eating whitespaces once is enough.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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
[PATCH 2/2] Print parser position on error
Most parser functions take a **endp argument to indicate the caller
where parsing has stopped. This is currently only used after parsing
something successfully. This patch sets **endp to the erroneous
position in the error case and prints its position after an error
has occured.
Signed-off-by: Sascha Hauer
---
src/mediactl.c | 48 +---
1 file changed, 45 insertions(+), 3 deletions(-)
diff --git a/src/mediactl.c b/src/mediactl.c
index c65de50..04ade15 100644
--- a/src/mediactl.c
+++ b/src/mediactl.c
@@ -40,6 +40,22 @@
#include "mediactl.h"
#include "tools.h"
+void media_print_streampos(struct media_device *media, const char *p, const
char *end)
+{
+ int pos;
+
+ pos = end - p + 1;
+
+ if (pos < 0)
+ pos = 0;
+ if (pos > strlen(p))
+ pos = strlen(p);
+
+ media_dbg(media, "\n");
+ media_dbg(media, " %s\n", p);
+ media_dbg(media, " %*s\n", pos, "^");
+}
+
struct media_pad *media_entity_remote_source(struct media_pad *pad)
{
unsigned int i;
@@ -538,12 +554,16 @@ struct media_pad *media_parse_pad(struct media_device
*media,
if (*p == '"') {
for (end = (char *)p + 1; *end && *end != '"'; ++end);
if (*end != '"') {
+ if (endp)
+ *endp = (char *)end;
media_dbg(media, "missing matching '\"'\n");
return NULL;
}
entity = media_get_entity_by_name(media, p + 1, end - p - 1);
if (entity == NULL) {
+ if (endp)
+ *endp = (char *)p + 1;
media_dbg(media, "no such entity \"%.*s\"\n", end - p -
1, p + 1);
return NULL;
}
@@ -553,6 +573,8 @@ struct media_pad *media_parse_pad(struct media_device
*media,
entity_id = strtoul(p, &end, 10);
entity = media_get_entity_by_id(media, entity_id);
if (entity == NULL) {
+ if (endp)
+ *endp = (char *)p;
media_dbg(media, "no such entity %d\n", entity_id);
return NULL;
}
@@ -560,6 +582,8 @@ struct media_pad *media_parse_pad(struct media_device
*media,
for (; isspace(*end); ++end);
if (*end != ':') {
+ if (endp)
+ *endp = end;
media_dbg(media, "Expected ':'\n", *end);
return NULL;
}
@@ -569,6 +593,8 @@ struct media_pad *media_parse_pad(struct media_device
*media,
pad = strtoul(p, &end, 10);
if (pad >= entity->info.pads) {
+ if (endp)
+ *endp = (char *)p;
media_dbg(media, "No pad '%d' on entity \"%s\". Maximum pad
number is %d\n",
pad, entity->info.name, entity->info.pads - 1);
return NULL;
@@ -591,10 +617,15 @@ struct media_link *media_parse_link(struct media_device
*media,
char *end;
source = media_parse_pad(media, p, &end);
- if (source == NULL)
+ if (source == NULL) {
+ if (endp)
+ *endp = end;
return NULL;
+ }
if (end[0] != '-' || end[1] != '>') {
+ if (endp)
+ *endp = end;
media_dbg(media, "Expected '->'\n");
return NULL;
}
@@ -602,8 +633,11 @@ struct media_link *media_parse_link(struct media_device
*media,
p = end + 2;
sink = media_parse_pad(media, p, &end);
- if (sink == NULL)
+ if (sink == NULL) {
+ if (endp)
+ *endp = end;
return NULL;
+ }
*endp = end;
@@ -629,6 +663,8 @@ int media_parse_setup_link(struct media_device *media,
link = media_parse_link(media, p, &end);
if (link == NULL) {
+ if (endp)
+ *endp = end;
media_dbg(media,
"%s: Unable to parse link\n", __func__);
return -EINVAL;
@@ -636,6 +672,8 @@ int media_parse_setup_link(struct media_device *media,
p = end;
if (*p++ != '[') {
+ if (endp)
+ *endp = (char *)p - 1;
media_dbg(media, "Unable to parse link flags: expected '['.\n");
return -EINVAL;
}
@@ -643,6 +681,8 @@ int media_parse_setup_link(st
[PATCH] media-ctl error messages
Hi All, As a first time media-ctl user it was quite frustrating to see that whatever I did media-ctl only responded with "Unable to parse link". The following is an attempt to add some more detailed error messages. With this applied media-ctl can answer with something like: No pad '1' on entity "mt9p031 0-0048". Maximum pad number is 0 media_parse_setup_link: Unable to parse link "mt9p031 0-0048":1->"/soc/cammultiplex@plx0":1[0] ^ Please consider applying. Sascha ---- Sascha Hauer (2): Print more detailed parse error messages Print parser position on error src/mediactl.c | 83 +- 1 file changed, 71 insertions(+), 12 deletions(-) -- 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
[PATCH 1/2] Print more detailed parse error messages
The following errors usually resulted in the same 'Unable to parse link'
message:
- one of the given entities does not exist
- one of the pads of a given entity does not exist
- No link exists between given pads
- syntax error in link description
Add more detailed error messages to give the user a clue what is going wrong.
Signed-off-by: Sascha Hauer
---
src/mediactl.c | 35 ++-
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/src/mediactl.c b/src/mediactl.c
index 4783a58..c65de50 100644
--- a/src/mediactl.c
+++ b/src/mediactl.c
@@ -537,31 +537,42 @@ struct media_pad *media_parse_pad(struct media_device
*media,
if (*p == '"') {
for (end = (char *)p + 1; *end && *end != '"'; ++end);
- if (*end != '"')
+ if (*end != '"') {
+ media_dbg(media, "missing matching '\"'\n");
return NULL;
+ }
entity = media_get_entity_by_name(media, p + 1, end - p - 1);
- if (entity == NULL)
+ if (entity == NULL) {
+ media_dbg(media, "no such entity \"%.*s\"\n", end - p -
1, p + 1);
return NULL;
+ }
++end;
} else {
entity_id = strtoul(p, &end, 10);
entity = media_get_entity_by_id(media, entity_id);
- if (entity == NULL)
+ if (entity == NULL) {
+ media_dbg(media, "no such entity %d\n", entity_id);
return NULL;
+ }
}
for (; isspace(*end); ++end);
- if (*end != ':')
+ if (*end != ':') {
+ media_dbg(media, "Expected ':'\n", *end);
return NULL;
+ }
+
for (p = end + 1; isspace(*p); ++p);
pad = strtoul(p, &end, 10);
- for (p = end; isspace(*p); ++p);
- if (pad >= entity->info.pads)
+ if (pad >= entity->info.pads) {
+ media_dbg(media, "No pad '%d' on entity \"%s\". Maximum pad
number is %d\n",
+ pad, entity->info.name, entity->info.pads - 1);
return NULL;
+ }
for (p = end; isspace(*p); ++p);
if (endp)
@@ -583,8 +594,11 @@ struct media_link *media_parse_link(struct media_device
*media,
if (source == NULL)
return NULL;
- if (end[0] != '-' || end[1] != '>')
+ if (end[0] != '-' || end[1] != '>') {
+ media_dbg(media, "Expected '->'\n");
return NULL;
+ }
+
p = end + 2;
sink = media_parse_pad(media, p, &end);
@@ -600,6 +614,9 @@ struct media_link *media_parse_link(struct media_device
*media,
return link;
}
+ media_dbg(media, "No link between \"%s\":%d and \"%s\":%d\n",
+ source->entity->info.name, source->index,
+ sink->entity->info.name, sink->index);
return NULL;
}
@@ -619,14 +636,14 @@ int media_parse_setup_link(struct media_device *media,
p = end;
if (*p++ != '[') {
- media_dbg(media, "Unable to parse link flags\n");
+ media_dbg(media, "Unable to parse link flags: expected '['.\n");
return -EINVAL;
}
flags = strtoul(p, &end, 10);
for (p = end; isspace(*p); p++);
if (*p++ != ']') {
- media_dbg(media, "Unable to parse link flags\n");
+ media_dbg(media, "Unable to parse link flags: expected ']'.\n");
return -EINVAL;
}
--
1.8.2.rc2
--
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 v2] media: i2c: mt9p031: add OF support
On Thu, May 02, 2013 at 11:52:34AM +0530, Prabhakar Lad wrote:
> From: Lad, Prabhakar
>
> add OF support for the mt9p031 sensor driver.
> Alongside this patch sorts the header inclusion alphabetically.
>
> Signed-off-by: Lad, Prabhakar
> Cc: Hans Verkuil
> Cc: Laurent Pinchart
> Cc: Mauro Carvalho Chehab
> Cc: Guennadi Liakhovetski
> Cc: Sylwester Nawrocki
> Cc: Sakari Ailus
> Cc: Grant Likely
> Cc: Sascha Hauer
> Cc: Rob Herring
> Cc: Rob Landley
> Cc: devicetree-disc...@lists.ozlabs.org
> Cc: davinci-linux-open-sou...@linux.davincidsp.com
> Cc: linux-...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> Changes for v2:
> 1: Used '-' instead of '_' for device properties.
> 2: Specified gpio reset pin as phandle in device node.
> 3: Handle platform data properly even if kernel is compiled with
> devicetree support.
> 4: Used dev_* for messages in drivers instead of pr_*.
> 5: Changed compatible property to "aptina,mt9p031" and "aptina,mt9p031m".
> 6: Sorted the header inclusion alphabetically and fixed some minor code nits.
>
> .../devicetree/bindings/media/i2c/mt9p031.txt | 37
> drivers/media/i2c/mt9p031.c| 62
> +++-
> 2 files changed, 97 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9p031.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> new file mode 100644
> index 000..cbe352b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> @@ -0,0 +1,37 @@
> +* Aptina 1/2.5-Inch 5Mp CMOS Digital Image Sensor
> +
> +The Aptina MT9P031 is a 1/2.5-inch CMOS active pixel digital image sensor
> with
> +an active array size of 2592H x 1944V. It is programmable through a simple
> +two-wire serial interface.
> +
> +Required Properties :
> +- compatible : value should be either one among the following
> + (a) "aptina,mt9p031" for mt9p031 sensor
> + (b) "aptina,mt9p031m" for mt9p031m sensor
> +
> +- input-clock-frequency : Input clock frequency.
> +
> +- pixel-clock-frequency : Pixel clock frequency.
> +
> +Optional Properties :
> +-gpio-reset: Chip reset GPIO
> +
> +Example:
> +
> +i2c0@1c22000 {
> + ...
> + ...
> + mt9p031@5d {
> + compatible = "aptina,mt9p031";
> + reg = <0x5d>;
> + gpio-reset = <&gpio3 30 0>;
> +
> + port {
> + mt9p031_1: endpoint {
> + input-clock-frequency = <600>;
> + pixel-clock-frequency = <9600>;
> + };
> + };
> + };
> + ...
> +};
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 28cf95b..8ce3561 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -16,9 +16,11 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> +#include
> +#include
> +#include
> #include
> #include
> #include
> @@ -28,6 +30,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "aptina-pll.h"
> @@ -928,10 +931,57 @@ static const struct v4l2_subdev_internal_ops
> mt9p031_subdev_internal_ops = {
> * Driver initialization and probing
> */
>
> +#if defined(CONFIG_OF)
> +static struct mt9p031_platform_data *
> + mt9p031_get_pdata(struct i2c_client *client)
> +
> +{
> + if (client->dev.of_node) {
By inverting the logic here and returning immediately you can safe an
indention level for the bulk of this function.
> + struct device_node *np;
> + struct mt9p031_platform_data *pdata;
> +
> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> + if (!np)
> + return NULL;
> +
> + pdata = devm_kzalloc(&client->dev,
> + sizeof(struct mt9p031_platform_data),
> + GFP_KERNEL);
> + if (!pdata) {
> + dev_err(&client->dev,
> + "mt9p031 failed allocate memeory\n");
> + return NULL;
s/memeory/memory/
Better drop this message completely. If you are really out of memory
you'll notice it quite fast anyway.
> + }
> + pdata->reset = of_get_named_gpio(c
Re: [PATCH v9 02/20] V4L2: support asynchronous subdevice registration
Hi Guennadi,
On Fri, Apr 12, 2013 at 05:40:22PM +0200, Guennadi Liakhovetski wrote:
> Currently bridge device drivers register devices for all subdevices
> synchronously, tupically, during their probing. E.g. if an I2C CMOS sensor
> is attached to a video bridge device, the bridge driver will create an I2C
> device and wait for the respective I2C driver to probe. This makes linking
> of devices straight forward, but this approach cannot be used with
> intrinsically asynchronous and unordered device registration systems like
> the Flattened Device Tree. To support such systems this patch adds an
> asynchronous subdevice registration framework to V4L2. To use it respective
> (e.g. I2C) subdevice drivers must register themselves with the framework.
> A bridge driver on the other hand must register notification callbacks,
> that will be called upon various related events.
>
> Signed-off-by: Guennadi Liakhovetski
> ---
> +
> +static struct v4l2_async_subdev *v4l2_async_belongs(struct
> v4l2_async_notifier *notifier,
> + struct
> v4l2_async_subdev_list *asdl)
> +{
> + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl);
> + struct v4l2_async_subdev *asd = NULL;
> + bool (*match)(struct device *,
> + struct v4l2_async_hw_info *);
> +
> + list_for_each_entry (asd, ¬ifier->waiting, list) {
> + struct v4l2_async_hw_info *hw = &asd->hw;
> +
> + /* bus_type has been verified valid before */
> + switch (hw->bus_type) {
> + case V4L2_ASYNC_BUS_CUSTOM:
> + match = hw->match.custom.match;
> + if (!match)
> + /* Match always */
> + return asd;
> + break;
> + case V4L2_ASYNC_BUS_PLATFORM:
> + match = match_platform;
> + break;
> + case V4L2_ASYNC_BUS_I2C:
> + match = match_i2c;
> + break;
> + default:
> + /* Cannot happen, unless someone breaks us */
> + WARN_ON(true);
> + return NULL;
> + }
> +
> + if (match && match(sd->dev, hw))
> + break;
> + }
> +
> + return asd;
'asd' can never be NULL here. You have to explicitly return NULL when
leaving the loop without match.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] media: i2c: mt9p031: add OF support
Hi,
One more point for your devicetree conversions,
On Mon, Apr 29, 2013 at 01:30:01PM +0530, Prabhakar Lad wrote:
> From: Lad, Prabhakar
>
> add OF support for the mt9p031 sensor driver.
>
> +static struct mt9p031_platform_data
> + *mt9p031_get_pdata(struct i2c_client *client)
> +
> +{
> + if (!client->dev.platform_data && client->dev.of_node) {
> + struct device_node *np;
> + struct mt9p031_platform_data *pdata;
> + int ret;
> +
> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> + if (!np)
> + return NULL;
> +
> + pdata = devm_kzalloc(&client->dev,
> + sizeof(struct mt9p031_platform_data),
> + GFP_KERNEL);
> + if (!pdata) {
> + pr_warn("mt9p031 failed allocate memeory\n");
> + return NULL;
> + }
> + ret = of_property_read_u32(np, "reset", &pdata->reset);
> + if (ret == -EINVAL)
> + pdata->reset = -1;
> + else if (ret == -ENODATA)
> + return NULL;
> +
> + if (of_property_read_u32(np, "ext_freq", &pdata->ext_freq))
> + return NULL;
> +
> + if (of_property_read_u32(np, "target_freq",
> + &pdata->target_freq))
> + return NULL;
> +
> + return pdata;
> + }
I don't know how the others see this, but IMO it would be cleaner to
first add a duplicate of the members of pdata in struct mt9p031 and then
initialize them either from pdata or from devicetree data. The
(artificial) creation of platform_data for the devicetree case adds a
new level of indirection. This may not be a problem here, but there are
cases where there is no 1:1 transcription between pdata and devicetree
possible.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 v9 02/20] V4L2: support asynchronous subdevice registration
On Fri, Apr 26, 2013 at 11:07:24PM +0200, Guennadi Liakhovetski wrote:
> Hi Sascha
>
> On Fri, 26 Apr 2013, Sascha Hauer wrote:
>
> > Hi Guennadi,
> >
> > On Fri, Apr 12, 2013 at 05:40:22PM +0200, Guennadi Liakhovetski wrote:
> > > +
> > > +static bool match_i2c(struct device *dev, struct v4l2_async_hw_info
> > > *hw_dev)
> > > +{
> > > + struct i2c_client *client = i2c_verify_client(dev);
> > > + return client &&
> > > + hw_dev->bus_type == V4L2_ASYNC_BUS_I2C &&
> > > + hw_dev->match.i2c.adapter_id == client->adapter->nr &&
> > > + hw_dev->match.i2c.address == client->addr;
> > > +}
> > > +
> > > +static bool match_platform(struct device *dev, struct v4l2_async_hw_info
> > > *hw_dev)
> > > +{
> > > + return hw_dev->bus_type == V4L2_ASYNC_BUS_PLATFORM &&
> > > + !strcmp(hw_dev->match.platform.name, dev_name(dev));
> > > +}
> >
> > I recently solved the same problem without being aware of your series.
> >
> > How about registering the asynchronous subdevices with a 'void *key'
> > instead of a bus specific matching function?
>
> Personally I don't think adding dummy data is a good idea. You can of
> course use pointers to real data, even just to the device object itself.
> But I really was trying to unbind host and subdevice devices, similar how
> clocks or pinmux entries or regulators are matched to their users. In the
> DT case we already use phandles to bind entities / pads / in whatever
> terms you prefer to think;-)
Do you have some preview patches for doing asynchronous subdevice
registration with devicetree? I mean this series and the v4l2 of parser
patches are not enough for the whole picture, right?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] media: i2c: mt9p031: add OF support
On Mon, Apr 29, 2013 at 01:30:01PM +0530, Prabhakar Lad wrote:
> From: Lad, Prabhakar
>
> add OF support for the mt9p031 sensor driver.
>
> Signed-off-by: Lad, Prabhakar
> Cc: Hans Verkuil
> Cc: Laurent Pinchart
> Cc: Mauro Carvalho Chehab
> Cc: Guennadi Liakhovetski
> Cc: Sylwester Nawrocki
> Cc: Sakari Ailus
> Cc: Grant Likely
> Cc: Rob Herring
> Cc: Rob Landley
> Cc: devicetree-disc...@lists.ozlabs.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> .../devicetree/bindings/media/i2c/mt9p031.txt | 43 ++
> drivers/media/i2c/mt9p031.c| 61
> +++-
> 2 files changed, 103 insertions(+), 1 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9p031.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> new file mode 100644
> index 000..b985e63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> @@ -0,0 +1,43 @@
> +* Aptina 1/2.5-Inch 5Mp CMOS Digital Image Sensor
> +
> +The Aptina MT9P031 is a 1/2.5-inch CMOS active pixel digital image sensor
> with
> +an active imaging pixel array of 2592H x 1944V. It incorporates sophisticated
> +camera functions on-chip such as windowing, column and row skip mode, and
> +snapshot mode. It is programmable through a simple two-wire serial interface.
> +
> +The MT9P031 is a progressive-scan sensor that generates a stream of pixel
> data
> +at a constant frame rate. It uses an on-chip, phase-locked loop (PLL) to
> +generate all internal clocks from a single master input clock running
> between 6
> +and 27 MHz. The maximum pixel rate is 96 Mp/s, corresponding to a clock rate
> of
> +96 MHz.
> +
> +Required Properties :
> +- compatible : value should be either one among the following
> + (a) "aptina,mt9p031-sensor" for mt9p031 sensor
> + (b) "aptina,mt9p031m-sensor" for mt9p031m sensor
> +
> +- ext_freq: Input clock frequency.
> +
> +- target_freq: Pixel clock frequency.
For devicetree properties '-' is preferred over '_'. Most devicetree
bindings we already have suggest that we shoud use 'frequency' and no
abbreviation. probably 'clock-frequency' should be used.
> +
> +Optional Properties :
> +-reset: Chip reset GPIO (If not specified defaults to -1)
gpios must be specified as phandles, see of_get_named_gpio().
> +
> +Example:
> +
> +i2c0@1c22000 {
> + ...
> + ...
> + mt9p031@5d {
> + compatible = "aptina,mt9p031-sensor";
> + reg = <0x5d>;
> +
> + port {
> + mt9p031_1: endpoint {
> + ext_freq = <600>;
> + target_freq = <9600>;
> + };
> + };
> + };
> + ...
> +};
> \ No newline at end of file
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 28cf95b..66078a6 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -23,11 +23,13 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> #include
> #include
> +#include
> #include
>
> #include "aptina-pll.h"
> @@ -928,10 +930,66 @@ static const struct v4l2_subdev_internal_ops
> mt9p031_subdev_internal_ops = {
> * Driver initialization and probing
> */
>
> +#if defined(CONFIG_OF)
> +static const struct of_device_id mt9p031_of_match[] = {
> + {.compatible = "aptina,mt9p031-sensor", },
> + {.compatible = "aptina,mt9p031m-sensor", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, mt9p031_of_match);
> +
> +static struct mt9p031_platform_data
> + *mt9p031_get_pdata(struct i2c_client *client)
> +
> +{
> + if (!client->dev.platform_data && client->dev.of_node) {
Just because the Kernel is compiled with devicetree support does not
necessarily mean you actually boot from devicetree. You must still
handle platform data properly.
> + struct device_node *np;
> + struct mt9p031_platform_data *pdata;
> + int ret;
> +
> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> + if (!np)
> + return NULL;
> +
> + pdata = devm_kzalloc(&client->dev,
> + sizeof(struct mt9p031_platform_data),
> + GFP_KERNEL);
> + if (!pdata) {
> + pr_warn("mt9p031 failed allocate memeory\n");
Use dev_* for messages inside drivers.
> + return NULL;
> + }
> + ret = of_property_read_u32(np, "reset", &pdata->reset);
> + if (ret == -EINVAL)
> + pdata->reset = -1;
> + else if (ret == -ENODATA)
> + return NULL;
> +
> + if (of_property_read_u32(np, "ext_freq", &pdata->ext_freq))
> + return NULL;
> +
Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data
On Fri, Apr 26, 2013 at 10:43:28AM +0200, Guennadi Liakhovetski wrote: > Hi Sascha > > On Fri, 26 Apr 2013, Sascha Hauer wrote: > > > > > > > > > That information should be conveyed by platform/DT data for the host, > > > > and be > > > > used to convert the 12-bit media bus code into a 8-bit media bus code > > > > in the > > > > host (a core helper function would probably be helpful). > > > > > > Yes, and we discussed this before too, I think. I proposed based then to > > > implement some compatibility table of "trivial" transformations, like a > > > 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer > > > etc. Such transformations would fit nicely in soc_mediabus.c ;-) This > > > just > > > needs to be implemented... > > > > These "trivial" transformations may turn out not to be so trivial. In > > the devicetree we would then need kind of 'shift-4-bit-left' properties. > > We already have a "data-shift" property exactly for this purpose. > > > How about instead describing the sensor node with: > > > > mbus-formats = <0x3010, 0x2013>; > > > > and the corresponding host interface with: > > > > mbus-formats = <0x3013, 0x2001>; > > How would this describe _how_ the transformation should be performed? nth index in the sensor array matches nth index in the csi array. The above describes: V4L2_MBUS_FMT_SGBRG12_1X12 on the sensor matches V4L2_MBUS_FMT_SGBRG8_1X8 on the host V4L2_MBUS_FMT_Y12_1X12 on the sensor matches V4L2_MBUS_FMT_Y8_1X8 on the host effectively implementing a shift by four bits. But also more complicated transformations could be described, like a colour space converter implemented in a DSP (not sure if anyone does this, but you get the idea) > And why does the host driver need mbus formats? Because mbus formats are effectively the input of a host driver. I assumed that we translate the mbus formats the sensor can output into the corresponding mbus formats that arrive at the host interface. Then afterwards the usual translation from mbus to fourcc a host interface can do is performed. I think what you aim at instead is a translation directly from the sensor to memory which I think is more complicated to build correctly. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 v9 02/20] V4L2: support asynchronous subdevice registration
Hi Guennadi,
On Fri, Apr 12, 2013 at 05:40:22PM +0200, Guennadi Liakhovetski wrote:
> +
> +static bool match_i2c(struct device *dev, struct v4l2_async_hw_info *hw_dev)
> +{
> + struct i2c_client *client = i2c_verify_client(dev);
> + return client &&
> + hw_dev->bus_type == V4L2_ASYNC_BUS_I2C &&
> + hw_dev->match.i2c.adapter_id == client->adapter->nr &&
> + hw_dev->match.i2c.address == client->addr;
> +}
> +
> +static bool match_platform(struct device *dev, struct v4l2_async_hw_info
> *hw_dev)
> +{
> + return hw_dev->bus_type == V4L2_ASYNC_BUS_PLATFORM &&
> + !strcmp(hw_dev->match.platform.name, dev_name(dev));
> +}
I recently solved the same problem without being aware of your series.
How about registering the asynchronous subdevices with a 'void *key'
instead of a bus specific matching function? With platform based devices
the key could simply be a pointer to some dummy value which is used by
both the subdevice and the device in its platform_data. for the shmobile
patch you have later in this series this would become:
static int csi2_r2025sd_key;
static struct r2025sd_platform_data r2025sd_pdata {
.key = &csi2_r2025sd_key,
};
static struct i2c_board_info i2c1_devices[] = {
{
I2C_BOARD_INFO("r2025sd", 0x32),
.platform_data = &r2025sd_pdata,
},
};
static struct sh_csi2_pdata csi2_info = {
.flags = SH_CSI2_ECC | SH_CSI2_CRC,
.key = &csi2_r2025sd_key,
};
For devicetree based devices the pointer to the subdevices devicenode
could be used as key.
I think this would make your matching code easier and also bus type
agnostic.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data
Hi Guennadi, On Mon, Apr 22, 2013 at 02:39:57PM +0200, Guennadi Liakhovetski wrote: > On Mon, 22 Apr 2013, Laurent Pinchart wrote: > > > Hi Guennadi, > > > > On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote: > > > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote: > > > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data > > > > allows > > > > the driver to use generic functions to manage sensor power supplies. > > > > > > > > Signed-off-by: Guennadi Liakhovetski > > > > > > A small addition to this one too: to be absolutely honest, I also had to > > > replace 12-bit formats with their 8-bit counterparts, because only 8 data > > > lanes are connected to my camera host. We'll need to somehow properly > > > solve this too. > > > > That information should be conveyed by platform/DT data for the host, and > > be > > used to convert the 12-bit media bus code into a 8-bit media bus code in > > the > > host (a core helper function would probably be helpful). > > Yes, and we discussed this before too, I think. I proposed based then to > implement some compatibility table of "trivial" transformations, like a > 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer > etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just > needs to be implemented... These "trivial" transformations may turn out not to be so trivial. In the devicetree we would then need kind of 'shift-4-bit-left' properties. How about instead describing the sensor node with: mbus-formats = <0x3010, 0x2013>; and the corresponding host interface with: mbus-formats = <0x3013, 0x2001>; This would allow to describe arbitrary transformations without having to limit to the 'trivial' ones. The result would be easier to understand also I think. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 10/10] drivers: misc: use module_platform_driver_probe()
On Thu, Mar 14, 2013 at 01:58:05PM +, Arnd Bergmann wrote: > On Thursday 14 March 2013, Fabio Porcedda wrote: > > This patch converts the drivers to use the > > module_platform_driver_probe() macro which makes the code smaller and > > a bit simpler. > > > > Signed-off-by: Fabio Porcedda > > Cc: Greg Kroah-Hartman > > Cc: Arnd Bergmann > > --- > > drivers/misc/atmel_pwm.c | 12 +--- > > drivers/misc/ep93xx_pwm.c | 13 + > > 2 files changed, 2 insertions(+), 23 deletions(-) > > The patch itself seems fine, but there are two issues around it: > > * The PWM drivers should really get moved to drivers/pwm and converted to the > new > PWM subsystem. I don't know if Hartley or Hans-Christian have plans to do > that already. > > * Regarding the use of module_platform_driver_probe, I'm a little worried > about > the interactions with deferred probing. I don't think there are any > regressions, > but we should probably make people aware that one cannot return > -EPROBE_DEFER > from a platform_driver_probe function. I'm worried about this aswell. I think platform_driver_probe shouldn't be used anymore. Even if a driver does not explicitly make use of -EPROBE_DEFER, it leaks in very quickly if a driver for example uses a regulator and just returns the error value from regulator_get. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 09/15] media: coda: don't build on multiplatform
On Mon, Jan 21, 2013 at 05:16:02PM +, Arnd Bergmann wrote: > The coda video codec driver depends on a mach-imx or mach-mxs specific > header file "mach/iram.h". This is not available when building for > multiplatform, so let us disable this driver for v3.8 when building > multiplatform, and hopefully find a proper fix for v3.9. > > drivers/media/platform/coda.c:27:23: fatal error: mach/iram.h: No such file > or directory I just sent a pull request for this with a proper fix. > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index 3dcfea6..049d2b2 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -142,7 +142,7 @@ if V4L_MEM2MEM_DRIVERS > > config VIDEO_CODA > tristate "Chips&Media Coda multi-standard codec IP" > - depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC > + depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_MXC && !ARCH_MULTIPLATFORM This breakage is not multiplatform related at all, it won't compile without multiplatform support either. So depends on BROKEN would be more appropriate if you want to go this way. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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
[PATCH] [media] coda: Fix build due to iram.h rename
commit c045e3f13 (ARM: imx: include iram.h rather than mach/iram.h) changed the
location of iram.h, which causes the following build error when building the
coda
driver:
drivers/media/platform/coda.c:27:23: error: mach/iram.h: No such file or
directory
drivers/media/platform/coda.c: In function 'coda_probe':
drivers/media/platform/coda.c:2000: error: implicit declaration of function
'iram_alloc'
drivers/media/platform/coda.c:2001: warning: assignment makes pointer from
integer without a cast
drivers/media/platform/coda.c: In function 'coda_remove':
drivers/media/platform/coda.c:2024: error: implicit declaration of function
'iram_free'
Since the content of iram.h is not imx specific, move it to
include/linux/platform_data/imx-iram.h instead. This is an intermediate solution
until the i.MX iram allocator is converted to the generic SRAM allocator.
Signed-off-by: Sascha Hauer
---
Based on an earlier version from Fabio Estevam, but this one moves iram.h
to include/linux/platform_data/imx-iram.h instead of include/linux/iram.h
which is a less generic name.
arch/arm/mach-imx/iram.h | 41
arch/arm/mach-imx/iram_alloc.c |3 +--
drivers/media/platform/coda.c |2 +-
include/linux/platform_data/imx-iram.h | 41
4 files changed, 43 insertions(+), 44 deletions(-)
delete mode 100644 arch/arm/mach-imx/iram.h
create mode 100644 include/linux/platform_data/imx-iram.h
diff --git a/arch/arm/mach-imx/iram.h b/arch/arm/mach-imx/iram.h
deleted file mode 100644
index 022690c..000
--- a/arch/arm/mach-imx/iram.h
+++ /dev/null
@@ -1,41 +0,0 @@
-/*
- * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
- * MA 02110-1301, USA.
- */
-#include
-
-#ifdef CONFIG_IRAM_ALLOC
-
-int __init iram_init(unsigned long base, unsigned long size);
-void __iomem *iram_alloc(unsigned int size, unsigned long *dma_addr);
-void iram_free(unsigned long dma_addr, unsigned int size);
-
-#else
-
-static inline int __init iram_init(unsigned long base, unsigned long size)
-{
- return -ENOMEM;
-}
-
-static inline void __iomem *iram_alloc(unsigned int size, unsigned long
*dma_addr)
-{
- return NULL;
-}
-
-static inline void iram_free(unsigned long base, unsigned long size) {}
-
-#endif
diff --git a/arch/arm/mach-imx/iram_alloc.c b/arch/arm/mach-imx/iram_alloc.c
index 6c80424..e05cf40 100644
--- a/arch/arm/mach-imx/iram_alloc.c
+++ b/arch/arm/mach-imx/iram_alloc.c
@@ -22,8 +22,7 @@
#include
#include
#include
-
-#include "iram.h"
+#include "linux/platform_data/imx-iram.h"
static unsigned long iram_phys_base;
static void __iomem *iram_virt_base;
diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
index 7b8b547..afadd3a 100644
--- a/drivers/media/platform/coda.c
+++ b/drivers/media/platform/coda.c
@@ -23,8 +23,8 @@
#include
#include
#include
+#include
-#include
#include
#include
#include
diff --git a/include/linux/platform_data/imx-iram.h
b/include/linux/platform_data/imx-iram.h
new file mode 100644
index 000..022690c
--- /dev/null
+++ b/include/linux/platform_data/imx-iram.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301, USA.
+ */
+#include
+
+#ifdef CONFIG_IRAM_ALLOC
+
+int __init iram_init(unsigned long base, unsigned long size);
+void __iomem *iram_alloc(unsigned int size, unsigned long *dma_addr);
+void iram_free(un
Re: [PATCH] [media] coda: Fix build due to iram.h rename
Hi Mauro, On Thu, Dec 27, 2012 at 08:15:12PM -0200, Mauro Carvalho Chehab wrote: > Em Mon, 17 Dec 2012 10:37:14 +0100 > Sascha Hauer escreveu: > > > On Wed, Nov 14, 2012 at 11:04:42AM -0200, Fabio Estevam wrote: > > > commit c045e3f13 (ARM: imx: include iram.h rather than mach/iram.h) > > > changed the > > > location of iram.h, which causes the following build error when building > > > the coda > > > driver: > > > > > > drivers/media/platform/coda.c:27:23: error: mach/iram.h: No such file or > > > directory > > > drivers/media/platform/coda.c: In function 'coda_probe': > > > drivers/media/platform/coda.c:2000: error: implicit declaration of > > > function 'iram_alloc' > > > drivers/media/platform/coda.c:2001: warning: assignment makes pointer > > > from integer without a cast > > > drivers/media/platform/coda.c: In function 'coda_remove': > > > drivers/media/platform/coda.c:2024: error: implicit declaration of > > > function 'iram_free > > > > > > Since the content of iram.h is not imx specific, move it to > > > include/linux/iram.h > > > instead. > > > > Generally we need a fix for this, but: > > > > > diff --git a/arch/arm/mach-imx/iram.h b/include/linux/iram.h > > > similarity index 100% > > > rename from arch/arm/mach-imx/iram.h > > > rename to include/linux/iram.h > > > > We shouldn't introduce a file include/linux/iram.h which is purely i.MX > > specific. The name is far too generic. I would rather suggest > > include/linux/platform_data/imx-iram.h (Although it's not exactly > > platform_data, so I'm open for better suggestions). > > > > As a side note this i.MX specific iram stuff (hopefully) is obsolete > > after the next merge window as Philip already has patches for a generic > > iram allocator which didn't make it into this merge window. > > Hi Sasha, > > This compilation breakage seems to still be happening. > > Just tested here with arm32 "allmodconfig", on a tree based on Linus one, > with -next and -media patches applied on it: > > drivers/media//platform/coda.c:27:23: fatal error: mach/iram.h: No such file > or directory > compilation terminated. > > I don't mind how this would be named, but this should be fixed somehow ;) I will prepare a patch for this next week when I'm back in the office. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 v2 0/5] Common Display Framework
On Thu, Dec 27, 2012 at 01:57:56PM -0600, Rob Clark wrote:
> On Thu, Dec 27, 2012 at 1:18 PM, Sascha Hauer wrote:
> > On Thu, Dec 27, 2012 at 09:54:55AM -0600, Rob Clark wrote:
> >> On Mon, Dec 24, 2012 at 7:37 AM, Laurent Pinchart
> >
> > This implies that the master driver knows all potential subdevices,
> > something which is not true for SoCs which have external i2c encoders
> > attached to unrelated i2c controllers.
>
> well, it can be brute-forced.. ie. drm driver calls common
> register_all_panels() fxn, which, well, registers all the
> panel/display subdev's based on their corresponding CONFIG_FOO_PANEL
> defines. If you anyways aren't building the panels as separate
> modules, that would work. Maybe not the most *elegant* approach, but
> simple and functional.
>
> I guess it partly depends on the structure in devicetree. If you are
> assuming that the i2c encoder belongs inside the i2c bus, like:
>
> &i2cN {
> foo-i2c-encoder {
>
> };
> };
>
> and you are letting devicetree create the devices, then it doesn't
> quite work. I'm not entirely convinced you should do it that way.
> Really any device like that is going to be hooked up to at least a
> couple busses.. i2c, some sort of bus carrying pixel data, maybe some
> gpio's, etc. So maybe makes more sense for a virtual drm/kms bus, and
> then use phandle stuff to link it to the various other busses it
> needs:
>
> mydrmdev {
> foo-i2c-encoder {
>i2c = <&i2cN>;
>gpio = <&gpioM 2 3>
>...
> };
> };
This seems to shift initialization order problem to another place.
Here we have to make sure the controller is initialized before the drm
driver. Same with suspend/resume.
It's not only i2c devices, also platform devices. On i.MX for example we
have a hdmi transmitter which is somewhere on the physical address
space.
I think grouping the different units together in a devicetree blob
because we think they might form a logical virtual device is not going
to work. It might make it easier from a drm perspective, but I think
doing this will make for a lot of special cases. What will happen for
example if you have two encoder devices in a row to configure? The
foo-i2c-encoder would then get another child node.
Right now the devicetree is strictly ordered by (control-, not data-)
bus topology. Linux has great helper code to support this model. Giving
up this help to brute force a different topology and then trying to fit
the result back into the Linux Bus hierarchy doesn't sound like a good
idea to me.
>
> ok, admittedly that is a bit different from other proposals about how
> this all fits in devicetree.. but otoh, I'm not a huge believer in
> letting something that is supposed to make life easier (DT), actually
> make things harder or more complicated. Plus this CDF stuff all needs
> to also work on platforms not using OF/DT.
Right, but every other platform I know of is also described by its bus
topology, be it platform device based or PCI or maybe even USB based.
CDF has to solve the same problem as ASoC and soc-camera: subdevices for
a virtual device can come from many different corners of the system. BTW
one example for a i2c encoder would be the SiI9022 which could not only
be part of a drm device, but also of an ASoC device.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 v2 0/5] Common Display Framework
On Thu, Dec 27, 2012 at 10:04:22AM -0600, Rob Clark wrote: > On Mon, Dec 24, 2012 at 11:27 AM, Laurent Pinchart > wrote: > > On Wednesday 19 December 2012 16:57:56 Jani Nikula wrote: > >> It just seems to me that, at least from a DRM/KMS perspective, adding > >> another layer (=CDF) for HDMI or DP (or legacy outputs) would be > >> overengineering it. They are pretty well standardized, and I don't see > >> there > >> would be a need to write multiple display drivers for them. Each display > >> controller has one, and can easily handle any chip specific requirements > >> right there. It's my gut feeling that an additional framework would just > >> get > >> in the way. Perhaps there could be more common HDMI/DP helper style code in > >> DRM to reduce overlap across KMS drivers, but that's another thing. > >> > >> So is the HDMI/DP drivers using CDF a more interesting idea from a non-DRM > >> perspective? Or, put another way, is it more of an alternative to using > >> DRM? > >> Please enlighten me if there's some real benefit here that I fail to see! > > > > As Rob pointed out, you can have external HDMI/DP encoders, and even > > internal > > HDMI/DP encoder IPs can be shared between SoCs and SoC vendors. CDF aims at > > sharing a single driver between SoCs and boards for a given HDMI/DP encoder. > > just fwiw, drm already has something a bit like this.. the i2c > encoder-slave. With support for a couple external i2c encoders which > could in theory be shared between devices. The problem with this code is that it only works when the i2c device is registered by a master driver. Once the i2c device comes from the devicetree there is no possibility to find it. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 v2 0/5] Common Display Framework
On Thu, Dec 27, 2012 at 09:54:55AM -0600, Rob Clark wrote: > On Mon, Dec 24, 2012 at 7:37 AM, Laurent Pinchart > wrote: > > Hi Rob, > > > > On Tuesday 18 December 2012 00:21:32 Rob Clark wrote: > >> On Mon, Dec 17, 2012 at 11:04 PM, Dave Airlie wrote: > >> >> Many developers showed interest in the first RFC, and I've had the > >> >> opportunity to discuss it with most of them. I would like to thank (in > >> >> no particular order) Tomi Valkeinen for all the time he spend helping me > >> >> to draft v2, Marcus Lorentzon for his useful input during Linaro Connect > >> >> Q4 2012, and Linaro for inviting me to Connect and providing a venue to > >> >> discuss this topic. > >> > > >> > So this might be a bit off topic but this whole CDF triggered me > >> > looking at stuff I generally avoid: > >> > > >> > The biggest problem I'm having currently with the whole ARM graphics > >> > and output world is the proliferation of platform drivers for every > >> > little thing. The whole ordering of operations with respect to things > >> > like suspend/resume or dynamic power management is going to be a real > >> > nightmare if there are dependencies between the drivers. How do you > >> > enforce ordering of s/r operations between all the various components? > >> > >> I tend to think that sub-devices are useful just to have a way to probe hw > >> which may or may not be there, since on ARM we often don't have any > >> alternative.. but beyond that, suspend/resume, and other life-cycle > >> aspects, > >> they should really be treated as all one device. Especially to avoid > >> undefined suspend/resume ordering. > > > > I tend to agree, except that I try to reuse the existing PM infrastructure > > when possible to avoid reinventing the wheel. So far handling suspend/resume > > ordering related to data busses in early suspend/late resume operations and > > allowing the Linux PM core to handle control busses using the Linux device > > tree worked pretty well. > > > >> CDF or some sort of mechanism to share panel drivers between drivers is > >> useful. Keeping it within drm, is probably a good idea, if nothing else to > >> simplify re-use of helper fxns (like avi-infoframe stuff, for example) and > >> avoid dealing with merging changes across multiple trees. Treating them > >> more > >> like shared libraries and less like sub-devices which can be dynamically > >> loaded/unloaded (ie. they should be not built as separate modules or > >> suspend/resumed or probed/removed independently of the master driver) is a > >> really good idea to avoid uncovering nasty synchronization issues later > >> (remove vs modeset or pageflip) or surprising userspace in bad ways. > > > > We've tried that in V4L2 years ago and realized that the approach led to a > > dead-end, especially when OF/DT got involved. With DT-based device probing, > > I2C camera sensors started getting probed asynchronously to the main camera > > device, as they are children of the I2C bus master. We will have similar > > issues with I2C HDMI transmitters or panels, so we should be prepared for > > it. > > What I've done to avoid that so far is that the master device > registers the drivers for it's output sub-devices before registering > it's own device. At least this way I can control that they are probed > first. Not the prettiest thing, but avoids even uglier problems. This implies that the master driver knows all potential subdevices, something which is not true for SoCs which have external i2c encoders attached to unrelated i2c controllers. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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] coda: Fix build due to iram.h rename
On Wed, Nov 14, 2012 at 11:04:42AM -0200, Fabio Estevam wrote: > commit c045e3f13 (ARM: imx: include iram.h rather than mach/iram.h) changed > the > location of iram.h, which causes the following build error when building the > coda > driver: > > drivers/media/platform/coda.c:27:23: error: mach/iram.h: No such file or > directory > drivers/media/platform/coda.c: In function 'coda_probe': > drivers/media/platform/coda.c:2000: error: implicit declaration of function > 'iram_alloc' > drivers/media/platform/coda.c:2001: warning: assignment makes pointer from > integer without a cast > drivers/media/platform/coda.c: In function 'coda_remove': > drivers/media/platform/coda.c:2024: error: implicit declaration of function > 'iram_free > > Since the content of iram.h is not imx specific, move it to > include/linux/iram.h > instead. Generally we need a fix for this, but: > diff --git a/arch/arm/mach-imx/iram.h b/include/linux/iram.h > similarity index 100% > rename from arch/arm/mach-imx/iram.h > rename to include/linux/iram.h We shouldn't introduce a file include/linux/iram.h which is purely i.MX specific. The name is far too generic. I would rather suggest include/linux/platform_data/imx-iram.h (Although it's not exactly platform_data, so I'm open for better suggestions). As a side note this i.MX specific iram stuff (hopefully) is obsolete after the next merge window as Philip already has patches for a generic iram allocator which didn't make it into this merge window. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 v2 0/5] Common Display Framework
Hi Laurent, On Thu, Nov 22, 2012 at 10:45:31PM +0100, Laurent Pinchart wrote: > From: Laurent Pinchart > > The CDF models this using a Russian doll's model. From the display controller > point of view only the first external entity (LVDS to DSI converter) is > visible. The display controller thus calls the control operations implemented > by the LVDS to DSI transmitter driver (left-most green arrow). The driver is > aware of the next entity in the chain, I can't find this in the code. I can see the video operations propagating upstream using the source field of struct display_entity, but how do the control operations propagate downstream? Am I missing something? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 v12 3/6] fbmon: add videomode helpers
Hi Laurent, On Wed, Nov 21, 2012 at 11:02:44PM +0100, Laurent Pinchart wrote: > Hi Steffen, > > > + > > + htotal = vm->hactive + vm->hfront_porch + vm->hback_porch + > > +vm->hsync_len; > > + vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch + > > +vm->vsync_len; > > + fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal); > > This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest solution > is probably to use 64-bit computation. You have displays with a pixelclock > 4GHz? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 v12 3/6] fbmon: add videomode helpers
On Thu, Nov 22, 2012 at 09:50:10AM +0100, Laurent Pinchart wrote: > Hi Sascha, > > On Thursday 22 November 2012 07:20:00 Sascha Hauer wrote: > > On Wed, Nov 21, 2012 at 11:02:44PM +0100, Laurent Pinchart wrote: > > > Hi Steffen, > > > > > > > + > > > > + htotal = vm->hactive + vm->hfront_porch + vm->hback_porch + > > > > +vm->hsync_len; > > > > + vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch + > > > > +vm->vsync_len; > > > > + fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal); > > > > > > This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest > > > solution is probably to use 64-bit computation. > > > > You have displays with a pixelclock > 4GHz? > > vm->pixelclock is expressed in Hz. vm->pixelclock * 1000 will thus overflow > if > the clock frequency is >= ~4.3 MHz. I have displays with a clock frequency > higher than that :-) If vm->pixelclock is in Hz, then the * 1000 above is wrong. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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: i.MX27: Add platform support for IRAM.
On Mon, Nov 05, 2012 at 04:59:44PM +0100, Javier Martin wrote:
> Add support for IRAM to i.MX27 non-DT platforms using
> iram_init() function.
>
> Signed-off-by: Javier Martin
> ---
> arch/arm/mach-imx/mm-imx27.c |3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/mach-imx/mm-imx27.c b/arch/arm/mach-imx/mm-imx27.c
> index e7e24af..fd2416d 100644
> --- a/arch/arm/mach-imx/mm-imx27.c
> +++ b/arch/arm/mach-imx/mm-imx27.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> /* MX27 memory map definition */
> static struct map_desc imx27_io_desc[] __initdata = {
> @@ -94,4 +95,6 @@ void __init imx27_soc_init(void)
> /* imx27 has the imx21 type audmux */
> platform_device_register_simple("imx21-audmux", 0, imx27_audmux_res,
> ARRAY_SIZE(imx27_audmux_res));
> + /* imx27 has an iram of 46080 bytes size */
> + iram_init(MX27_IRAM_BASE_ADDR, 46080);
For this rather Philipps sram allocater patches should be used. This
would also solve the problem that mach/iram.h cannot be accessed anymore
in current -next. Fabio already sent a patch addressing this, but I
think we should go for a proper fix rather than just moving iram.h
to include/linux/
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 2/2] mx2_camera: Fix regression caused by clock conversion
On Wed, Nov 14, 2012 at 04:22:40PM -0200, Fabio Estevam wrote: > Hi Sascha, > > On Wed, Oct 31, 2012 at 9:57 AM, Mauro Carvalho Chehab > wrote: > > > As it seems that those patches depend on some patches at the arm tree, > > the better is to merge them via -arm tree. > > > > So, > > > > Acked-by: Mauro Carvalho Chehab > > Could you please apply this series via your tree? Sure, I already have this in my queue. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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
Hi Steffen,
You lose memory in several places:
On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
> +static struct display_timing *of_get_display_timing(struct device_node *np)
> +{
> + struct display_timing *dt;
> + int ret = 0;
> +
> + dt = kzalloc(sizeof(*dt), GFP_KERNEL);
> + if (!dt) {
> + pr_err("%s: could not allocate display_timing struct\n",
> __func__);
> + return NULL;
> + }
> +
> + ret |= parse_property(np, "hback-porch", &dt->hback_porch);
> + ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
> + ret |= parse_property(np, "hactive", &dt->hactive);
> + ret |= parse_property(np, "hsync-len", &dt->hsync_len);
> + ret |= parse_property(np, "vback-porch", &dt->vback_porch);
> + ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
> + ret |= parse_property(np, "vactive", &dt->vactive);
> + ret |= parse_property(np, "vsync-len", &dt->vsync_len);
> + ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
> +
> + of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
> + of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
> + of_property_read_u32(np, "de-active", &dt->de_pol_active);
> + of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
> + dt->interlaced = of_property_read_bool(np, "interlaced");
> + dt->doublescan = of_property_read_bool(np, "doublescan");
> +
> + if (ret) {
> + pr_err("%s: error reading timing properties\n", __func__);
> + return NULL;
Here
> + }
> +
> + return dt;
> +}
> +
> +/**
> + * of_get_display_timings - parse all display_timing entries from a
> device_node
> + * @np: device_node with the subnodes
> + **/
> +struct display_timings *of_get_display_timings(struct device_node *np)
> +{
> + struct device_node *timings_np;
> + struct device_node *entry;
> + struct device_node *native_mode;
> + struct display_timings *disp;
> +
> + if (!np) {
> + pr_err("%s: no devicenode given\n", __func__);
> + return NULL;
> + }
> +
> + timings_np = of_find_node_by_name(np, "display-timings");
> + if (!timings_np) {
> + pr_err("%s: could not find display-timings node\n", __func__);
> + return NULL;
> + }
> +
> + disp = kzalloc(sizeof(*disp), GFP_KERNEL);
> + if (!disp)
> + return -ENOMEM;
> +
> + entry = of_parse_phandle(timings_np, "native-mode", 0);
> + /* assume first child as native mode if none provided */
> + if (!entry)
> + entry = of_get_next_child(np, NULL);
> + if (!entry) {
> + pr_err("%s: no timing specifications given\n", __func__);
> + return NULL;
Here
> + }
> +
> + pr_info("%s: using %s as default timing\n", __func__, entry->name);
> +
> + native_mode = entry;
> +
> + disp->num_timings = of_get_child_count(timings_np);
> + disp->timings = kzalloc(sizeof(struct display_timing
> *)*disp->num_timings,
> + GFP_KERNEL);
> + if (!disp->timings)
> + return -ENOMEM;
Here
> +
> + disp->num_timings = 0;
> + disp->native_mode = 0;
> +
> + for_each_child_of_node(timings_np, entry) {
> + struct display_timing *dt;
> +
> + dt = of_get_display_timing(entry);
> + if (!dt) {
> + /* to not encourage wrong devicetrees, fail in case of
> an error */
> + pr_err("%s: error in timing %d\n", __func__,
> disp->num_timings+1);
> + return NULL;
Here
> + }
> +
> + if (native_mode == entry)
> + disp->native_mode = disp->num_timings;
> +
> + disp->timings[disp->num_timings] = dt;
> + disp->num_timings++;
> + }
> + of_node_put(timings_np);
> +
> + if (disp->num_timings > 0)
> + pr_info("%s: got %d timings. Using timing #%d as default\n",
> __func__,
> + disp->num_timings , disp->native_mode + 1);
> + else {
> + pr_err("%s: no valid timings specified\n", __func__);
> + return NULL;
and here
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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: i.MX27: Add platform support for IRAM.
On Tue, Nov 06, 2012 at 12:37:35PM +0100, Guennadi Liakhovetski wrote: > Hi Javier > > On Mon, 5 Nov 2012, Javier Martin wrote: > > > Add support for IRAM to i.MX27 non-DT platforms using > > iram_init() function. > > I'm not sure this belongs in a camera driver. Can IRAM not be used for > anything else? I'll check the i.MX27 datasheet when I'm back home after > the conference, so far this seems a bit odd. This patch just adds the sram pool to the system in i.MX27 code, the patch is not camera specific. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 1/2] ARM: clk-imx27: Add missing clock for mx2-camera
On Wed, Oct 31, 2012 at 04:53:03PM -0200, Mauro Carvalho Chehab wrote: > Em Wed, 31 Oct 2012 14:53:47 +0100 (CET) > Guennadi Liakhovetski escreveu: > > > On Wed, 31 Oct 2012, Fabio Estevam wrote: > > > > > Hi Sascha, > > > > > > On Wed, Oct 31, 2012 at 11:16 AM, Sascha Hauer > > > wrote: > > > > > > > Quoting yourself: > > > > > > > >> Forgot to comment: as patch 2 relies on this change, the better, IMHO, > > > >> is > > > >> to send both via the same tree. If you decide to do so, please get arm > > > >> maintainer's ack, instead, and we can merge both via my tree. > > > > > > > > That's why Fabio resent these patches with my Ack. You are free to take > > > > these. > > > > > > I have just realized that this patch (1/2) will not apply against > > > media tree because it does not have commit 27b76486a3 (media: > > > mx2_camera: remove cpu_is_xxx by using platform_device_id), which > > > changes from mx2_camera.0 to imx27-camera.0. > > > > This is exactly the reason why I wasn't able to merge it. The problem was, > > that this "media: mx2_camera: remove cpu_is_xxx by using > > platform_device_id" patch non-trivially touched both arch/arm/ and > > drivers/media/ directories. And being patch 27/34 I didn't feel like > > asking the author to redo it again:-) This confirms, that it's better to > > avoid such overlapping patches whenever possible. > > > > > So it seems to be better to merge this via arm tree to avoid such > > > conflict. > > I agree with Fabio and Guennadi. There are so many changes happening at arm > that merging those two patches there will likely be easier for everybody. Ok, then I'll take them. I wasn't aware in arm-soc are sitting patches for this driver already. > > Otherwise, I'll need to pull from some arm tree that never rebase, with > the needed patches, and coordinate with you during the merge window, > to be sure that patches will arrive there at the right order, from the > right tree. Hopefully these kind of cross dependencies become fewer over time. SoC code is getting smaller and gets better abstracted from the drivers, so chances are good. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 1/2] ARM: clk-imx27: Add missing clock for mx2-camera
Hi Mauro, On Wed, Oct 31, 2012 at 09:56:32AM -0200, Mauro Carvalho Chehab wrote: > Em Tue, 30 Oct 2012 10:03:25 -0200 > Fabio Estevam escreveu: > > > During the clock conversion for mx27 the "per4_gate" clock was missed to get > > registered as a dependency of mx2-camera driver. > > > > In the old mx27 clock driver we used to have: > > > > DEFINE_CLOCK1(csi_clk, 0, NULL, 0, parent, &csi_clk1, &per4_clk); > > > > ,so does the same in the new clock driver > > > > Signed-off-by: Fabio Estevam > > Acked-by: Sascha Hauer > > As it seems that those patches depend on some patches at the arm tree, > the better is to merge them via -arm tree. Quoting yourself: > Forgot to comment: as patch 2 relies on this change, the better, IMHO, is > to send both via the same tree. If you decide to do so, please get arm > maintainer's ack, instead, and we can merge both via my tree. That's why Fabio resent these patches with my Ack. You are free to take these. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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: clk-imx27: Add missing clock for mx2-camera
On Fri, Oct 05, 2012 at 06:53:01PM -0300, Fabio Estevam wrote: > During the clock conversion for mx27 the "per4_gate" clock was missed to get > registered as a dependency of mx2-camera driver. > > In the old mx27 clock driver we used to have: > > DEFINE_CLOCK1(csi_clk, 0, NULL, 0, parent, &csi_clk1, &per4_clk); > > ,so does the same in the new clock driver. > > Signed-off-by: Fabio Estevam I'm fine with merging this through the media tree. Acked-by: Sascha Hauer Sascha > --- > arch/arm/mach-imx/clk-imx27.c |1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c > index 3b6b640..5ef0f08 100644 > --- a/arch/arm/mach-imx/clk-imx27.c > +++ b/arch/arm/mach-imx/clk-imx27.c > @@ -224,6 +224,7 @@ int __init mx27_clocks_init(unsigned long fref) > clk_register_clkdev(clk[lcdc_ipg_gate], "ipg", "imx-fb.0"); > clk_register_clkdev(clk[lcdc_ahb_gate], "ahb", "imx-fb.0"); > clk_register_clkdev(clk[csi_ahb_gate], "ahb", "mx2-camera.0"); > + clk_register_clkdev(clk[per4_gate], "per", "mx2-camera.0"); > clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc"); > clk_register_clkdev(clk[usb_ipg_gate], "ipg", "fsl-usb2-udc"); > clk_register_clkdev(clk[usb_ahb_gate], "ahb", "fsl-usb2-udc"); > -- > 1.7.9.5 > > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 Wed, Oct 10, 2012 at 05:51:27PM +0900, Mark Brown wrote: > On Wed, Oct 10, 2012 at 10:40:06AM +0200, Sascha Hauer wrote: > > > Mark, when do we get the same for aSoC? ;) > > Well, I'm unlikely to work on it as I don't have any systems which can > boot with device tree. If it's only that I'm sure we could spare a i.MX53 LOCO ;) > > The big, big problem you've got doing this is lots of dynamic changes at > runtime and in general complicated systems. It's trivial to describe > the physical links but they don't provide anything like enough > information to use things. Quite frankly I'm not sure device tree is a > useful investment of time for advanced audio systems anyway, it's really > not solving any problems people actually have. Right now the i.MX audmux binding is only enough to say which ports should be connected, but not which format should be used. Just today we had the problem where a codec has two DAIs and wanted to add the information on which port our SSI unit is connected to the devicetree. So I think it's worthwile to do, but that would be a big big task... Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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
Hi Guennadi, On Mon, Oct 08, 2012 at 09:58:58AM +0200, Guennadi Liakhovetski wrote: > On Fri, 5 Oct 2012, Sascha Hauer wrote: > > > On Fri, Oct 05, 2012 at 05:41:00PM +0200, Guennadi Liakhovetski wrote: > > > Hi Sascha > > > > > > > > > > > Maybe the example would be clearer if you split it up in two, one simple > > > > case with the csi2_1 <-> imx074_1 and a more advanced with the link in > > > > between. > > > > > > With no link between two ports no connection is possible, so, only > > > examples with links make sense. > > > > I should have said with the renesas,sh-mobile-ceu in between. > > > > So simple example: csi2_1 <-l-> imx074_1 > > advanced: csi2_2 <-l-> ceu <-l-> ov772x > > No, CEU is the DMA engine with some image processing, so, it's always > present. The CSI-2 is just a MIPI CSI-2 interface, that in the above case > is used with the CEU too. So, it's like > >,-l- ov772x > / > ceu0 < > \ >`-l- csi2 -l- imx074 > > > > > It took me some time until I figured out that these are two > > > > separate camera/sensor pairs. Somehow I was looking for a multiplexer > > > > between them. > > > > > > Maybe I can add more comments to the file, perhaps, add an ASCII-art > > > chart. > > > > That would be good. > > Is the above good enough? :) Yes, thanks. We thought and disucssed about this devicetree binding quite much the last days. Finally I understood it and I must say that I like it. I think more prosa to explain the general concept would be good in the binding doc. Mark, when do we get the same for aSoC? ;) Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 Fri, Oct 05, 2012 at 05:41:00PM +0200, Guennadi Liakhovetski wrote:
> Hi Sascha
>
> > > +
> > > + 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 {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + ceu0_1: link@1 {
> > > + reg = <1>; /* local link # */
> > > + remote = <&ov772x_1_1>; /* remote phandle */
> > > + 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 */
> > > + data-active = <1>; /* active high */
> > > + pclk-sample = <1>; /* rising */
> > > + };
> > > +
> > > + ceu0_0: link@0 {
> > > + reg = <0>;
> > > + remote = <&csi2_2>;
> > > + immutable;
> > > + };
> > > + };
> > > + };
> > > +
> > > + i2c0: i2c@0xfff2 {
> > > + ...
> > > + ov772x_1: camera@0x21 {
> > > + compatible = "omnivision,ov772x";
> > > + reg = <0x21>;
> > > + vddio-supply = <®ulator1>;
> > > + vddcore-supply = <®ulator2>;
> > > +
> > > + clock-frequency = <2000>;
> > > + clocks = <&mclk 0>;
> > > + clock-names = "xclk";
> > > +
> > > + port {
> > > + /* With 1 link per port no need in addresses */
> > > + ov772x_1_1: link {
> > > + bus-width = <8>;
> > > + remote = <&ceu0_1>;
> > > + hsync-active = <1>;
> > > + vsync-active = <0>; /* who came up
> > > with an inverter here?... */
> > > + data-active = <1>;
> > > + pclk-sample = <1>;
> > > + };
> >
> > I currently do not understand why these properties are both in the sensor
> > and in the link. What happens if they conflict? Are inverters assumed
> > like suggested above? I think the bus can only have a single bus-width,
> > why allow multiple bus widths here?
>
> Yes, these nodes represent port configuration of each party on a certain
> link. And they can differ in certain properties, like - as you correctly
> notice - in the case, when there's an inverter on a line. As for other
> properties, some of them must be identical, like bus-width, still, they
> have to be provided on both ends, because generally drivers have to be
> able to perform all the required configuration based only on the
> information from their own nodes, without looking at "remote" partner node
> properties.
So the port associated to the ov772x_1 only describes how to configure
the ov772x and it's up to me to make sure that this configuration
matches the partner device. If I don't then it won't work but soc-camera
will happily continue.
Ok, that's good, I thought there would be some kind of matching
mechanism take place here. It may be worth to make this more explicit in
the docs.
>
> > > + reg = <0xffc9 0x1000>;
> > > + interrupts = <0x17a0>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@1 {
> > > + compatible = "renesas,csi2c"; /* one of CSI2I and
> > > CSI2C */
> > > + reg = <1>; /* CSI-2 PHY #1 of 2:
> > > PHY_S, PHY_M has port address 0, is unused */
> > > +
> > > + csi2_1: link {
> > > + clock-lanes = <0>;
> > > + data-lanes = <2>, <1>;
> > > + remote = <&imx074_1>;
> > > + };
> > > + };
> > > + port@2 {
> > > + reg = <2>; /* port 2: link to the
> > > CEU */
> > > +
> > > + csi2_2: link {
> > > + immutable;
> > > + remote = <&ceu0_0>;
> > > + };
> > > + };
> >
> > Maybe the example would be clearer i
Re: [PATCH 04/14] media: add V4L2 DT binding documentation
Hi Guennadi,
Some comments inline.
On Thu, Sep 27, 2012 at 04:07:23PM +0200, Guennadi Liakhovetski wrote:
> This patch adds a document, describing common V4L2 device tree bindings.
>
> Co-authored-by: Sylwester Nawrocki
> Signed-off-by: Guennadi Liakhovetski
> ---
> Documentation/devicetree/bindings/media/v4l2.txt | 162
> ++
> 1 files changed, 162 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/media/v4l2.txt
>
> diff --git a/Documentation/devicetree/bindings/media/v4l2.txt
> b/Documentation/devicetree/bindings/media/v4l2.txt
> new file mode 100644
> index 000..b8b3f41
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/v4l2.txt
> @@ -0,0 +1,162 @@
> +Video4Linux Version 2 (V4L2)
> +
> +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.
> +- slave-mode: a boolean property, run the link in slave mode. Default is
> master
> + mode.
> +- data-shift: on parallel data busses, if data-width is used to specify the
> + number of data lines, data-shift can be used to specify which data lines
> are
> + used, e.g. "data-width=<10>; data-shift=<2>;" means, that lines 9:2 are
> used.
> +- hsync-active: 1 or 0 for active-high or -low HSYNC signal polarity
> + respectively.
> +- vsync-active: ditto for VSYNC. Note, that if HSYNC and VSYNC polarities are
> + not specified, embedded synchronisation may be required, where supported.
> +- data-active: similar to HSYNC and VSYNC specifies data line polarity.
> +- field-even-active: field signal level during the even field data
> transmission.
> +- pclk-sample: rising (1) or falling (0) edge to sample the pixel clock pin.
> +- data-lanes: array of serial, e.g. MIPI CSI-2, data hardware lane numbers in
> + the ascending order, beginning with logical lane 0.
> +- clock-lanes: hardware lane number, used for the clock lane.
> +- clock-noncontinuous: a boolean property to allow MIPI CSI-2 non-continuous
> + clock mode.
> +
> +Example:
> +
> + 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 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ceu0_1: link@1 {
> + reg = <1>; /* local link # */
> + remote = <&ov772x_1_1>; /* remote phandle */
> + 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 */
> + data-active = <1>; /* active high */
> + pclk-sample = <1>; /* rising */
> + };
> +
> + ceu0_0: link@0 {
> + reg = <0>;
> + remote = <&csi2_2>;
> + immutable;
> + };
> + };
> + };
> +
> + i2c0: i2c@0xfff2 {
> + ...
> + ov772x_1: camera@0x21 {
> + compatible = "omnivision,ov772x";
> + reg = <0x21>;
Re: [PATCH 00/34] i.MX multi-platform support
Hi Shawn, On Mon, Sep 17, 2012 at 01:34:29PM +0800, Shawn Guo wrote: > The series enables multi-platform support for imx. Since the required > frameworks (clk, pwm) and spare_irq have already been adopted on imx, > the series is all about cleaning up mach/* headers. Along with the > changes, arch/arm/plat-mxc gets merged into arch/arm/mach-imx. > > It's based on a bunch of branches (works from others), Rob's initial > multi-platform series, Arnd's platform-data and smp_ops (Marc's) and > imx 3.7 material (Sascha and myself). > > It's available on branch below. > > git://git.linaro.org/people/shawnguo/linux-2.6.git imx/multi-platform > > It's been tested on imx5 and imx6, and only compile-tested on imx2 and > imx3, so testing on imx2/3 are appreciated. > > Subsystem maintainers, > > I plan to send the whole series via arm-soc tree at the end of 3.7 > merge window when all dependant bits hit mainline. Please have a > look at the patches you get copied and provide ACKs if the changes > are good. Thanks. I just had a look at the remaining initcalls in arch-imx. Most of them are protected with a cpu_is_*, but this one should be fixed before i.MX is enabled for multi platform: arch/arm/mach-imx/devices/devices.c:48:core_initcall(mxc_device_init); I think this won't harm others directly, but it will register i.MX related devices on foreign platforms. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 00/34] i.MX multi-platform support
Hi Shawn, On Mon, Sep 17, 2012 at 01:34:29PM +0800, Shawn Guo wrote: > The series enables multi-platform support for imx. Since the required > frameworks (clk, pwm) and spare_irq have already been adopted on imx, > the series is all about cleaning up mach/* headers. Along with the > changes, arch/arm/plat-mxc gets merged into arch/arm/mach-imx. > > It's based on a bunch of branches (works from others), Rob's initial > multi-platform series, Arnd's platform-data and smp_ops (Marc's) and > imx 3.7 material (Sascha and myself). > > It's available on branch below. > > git://git.linaro.org/people/shawnguo/linux-2.6.git imx/multi-platform > > It's been tested on imx5 and imx6, and only compile-tested on imx2 and > imx3, so testing on imx2/3 are appreciated. Great work! This really pushes the i.MX architecture one step closer to a clean code base. I gave it a test on i.MX1, i.MX27, i.MX31 and i.MX35. All run fine, but the last patch breaks the imx_v4_v5_defconfig: Somehow it now defaults to ARMv7 based machines. I haven't looked into it, just reenabled ARMv4/ARMv5 and the boards again -> works. The config should be updated with the last patch. I'm fine with the changes to mx2-camera, but Javier should give his ok to it, he has worked on it quite a lot recently. One other issue related to imx-dma, see comment to that patch. Otherwise: Acked-by: Sascha Hauer Tested-by: Sascha Hauer Thanks Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 1/5] video: Add generic display panel core
On Thu, Sep 13, 2012 at 03:40:40AM +0200, Laurent Pinchart wrote:
> Hi Sascha,
>
> > > +int panel_get_modes(struct panel *panel, const struct fb_videomode
> > > **modes)
> > > +{
> > > + if (!panel->ops || !panel->ops->get_modes)
> > > + return 0;
> > > +
> > > + return panel->ops->get_modes(panel, modes);
> > > +}
> > > +EXPORT_SYMBOL_GPL(panel_get_modes);
> >
> > You have seen my of videomode helper proposal. One result there was that
> > we want to have ranges for the margin/synclen fields. Does it make sense
> > to base this new panel framework on a more sophisticated internal
> > reprentation of the panel parameters?
>
> I think it does, yes. We need a common video mode structure, and the panel
> framework should use it. I'll try to rebase my patches on top of your
> proposal. Have you posted the latest version ?
V2 is the newest version. I'd like to implement ranges for the display
timings which then makes for a new common video mode structure, which
then could be used by drm and fbdev, probably with helper functions to
convert from common videomode to drm/fbdev specific variants.
>
> > This could then be converted to struct fb_videomode / struct
> > drm_display_mode as needed. This would also make it more suitable for drm
> > drivers which are not interested in struct fb_videomode.
> >
> > Related to this I suggest to change the API to be able to iterate over
> > the different modes, like:
> >
> > struct videomode *panel_get_mode(struct panel *panel, int num);
> >
> > This was we do not have to have an array of modes in memory, which may
> > be a burden for some panel drivers.
>
> I currently have mixed feelings about this. Both approaches have pros and
> cons. Iterating over the modes would be more complex for drivers that use
> panels, and would be race-prone if the modes can change at runtime (OK, this
> isn't supported by the current panel API proposal).
I just remember that the array approach was painful when I worked on an
fbdev driver some time ago. There some possible modes came from platform_data,
other modes were default modes in the driver and all had to be merged
into a single array. I don't remember the situation exactly, but it
would have been simpler if it had been a list instead of an array.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 1/5] video: Add generic display panel core
Hi Laurent,
On Fri, Aug 17, 2012 at 02:49:39AM +0200, Laurent Pinchart wrote:
> +/**
> + * panel_get_modes - Get video modes supported by the panel
> + * @panel: The panel
> + * @modes: Pointer to an array of modes
> + *
> + * Fill the modes argument with a pointer to an array of video modes. The
> array
> + * is owned by the panel.
> + *
> + * Return the number of supported modes on success (including 0 if no mode is
> + * supported) or a negative error code otherwise.
> + */
> +int panel_get_modes(struct panel *panel, const struct fb_videomode **modes)
> +{
> + if (!panel->ops || !panel->ops->get_modes)
> + return 0;
> +
> + return panel->ops->get_modes(panel, modes);
> +}
> +EXPORT_SYMBOL_GPL(panel_get_modes);
You have seen my of videomode helper proposal. One result there was that
we want to have ranges for the margin/synclen fields. Does it make sense
to base this new panel framework on a more sophisticated internal
reprentation of the panel parameters? This could then be converted to
struct fb_videomode / struct drm_display_mode as needed. This would also
make it more suitable for drm drivers which are not interested in struct
fb_videomode.
Related to this I suggest to change the API to be able to iterate over
the different modes, like:
struct videomode *panel_get_mode(struct panel *panel, int num);
This was we do not have to have an array of modes in memory, which may
be a burden for some panel drivers.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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
[PATCH 1/2] media v4l2-mem2mem: Use list_first_entry
Use list_first_entry instead of list_entry which makes the intention of the code more clear. Signed-off-by: Sascha Hauer --- drivers/media/video/v4l2-mem2mem.c |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/media/video/v4l2-mem2mem.c b/drivers/media/video/v4l2-mem2mem.c index 975d0fa..aaa67d3 100644 --- a/drivers/media/video/v4l2-mem2mem.c +++ b/drivers/media/video/v4l2-mem2mem.c @@ -102,7 +102,7 @@ void *v4l2_m2m_next_buf(struct v4l2_m2m_queue_ctx *q_ctx) return NULL; } - b = list_entry(q_ctx->rdy_queue.next, struct v4l2_m2m_buffer, list); + b = list_first_entry(&q_ctx->rdy_queue, struct v4l2_m2m_buffer, list); spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags); return &b->vb; } @@ -122,7 +122,7 @@ void *v4l2_m2m_buf_remove(struct v4l2_m2m_queue_ctx *q_ctx) spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags); return NULL; } - b = list_entry(q_ctx->rdy_queue.next, struct v4l2_m2m_buffer, list); + b = list_first_entry(&q_ctx->rdy_queue, struct v4l2_m2m_buffer, list); list_del(&b->list); q_ctx->num_rdy--; spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags); @@ -175,7 +175,7 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev) return; } - m2m_dev->curr_ctx = list_entry(m2m_dev->job_queue.next, + m2m_dev->curr_ctx = list_first_entry(&m2m_dev->job_queue, struct v4l2_m2m_ctx, queue); m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING; spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); -- 1.7.10.4 -- 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
[PATCH 2/2] media v4l2-mem2mem: fix src/out and dst/cap num_rdy
src bufs belong to out queue, dst bufs belong to in queue. Currently
this is not a real problem since all users currently need exactly one
input and one output buffer.
Signed-off-by: Sascha Hauer
---
include/media/v4l2-mem2mem.h |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 16ac473..131cc4a 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -140,7 +140,7 @@ void v4l2_m2m_buf_queue(struct v4l2_m2m_ctx *m2m_ctx,
struct vb2_buffer *vb);
static inline
unsigned int v4l2_m2m_num_src_bufs_ready(struct v4l2_m2m_ctx *m2m_ctx)
{
- return m2m_ctx->cap_q_ctx.num_rdy;
+ return m2m_ctx->out_q_ctx.num_rdy;
}
/**
@@ -150,7 +150,7 @@ unsigned int v4l2_m2m_num_src_bufs_ready(struct
v4l2_m2m_ctx *m2m_ctx)
static inline
unsigned int v4l2_m2m_num_dst_bufs_ready(struct v4l2_m2m_ctx *m2m_ctx)
{
- return m2m_ctx->out_q_ctx.num_rdy;
+ return m2m_ctx->cap_q_ctx.num_rdy;
}
void *v4l2_m2m_next_buf(struct v4l2_m2m_queue_ctx *q_ctx);
--
1.7.10.4
--
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
[PATCH] v4l2 mem2mem
Two small patches, one fix and one more or less cosmetic patch for the v4l2 mem2mem framework. Comments welcome. Thanks, Sascha Sascha Hauer (2): media v4l2-mem2mem: Use list_first_entry media v4l2-mem2mem: fix src/out and dst/cap num_rdy drivers/media/video/v4l2-mem2mem.c |6 +++--- include/media/v4l2-mem2mem.h |4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) -- 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: mx2_camera: Remove MX2_CAMERA_SWAP16 and MX2_CAMERA_PACK_DIR_MSB flags.
On Mon, Aug 20, 2012 at 10:08:39AM +0200, javier Martin wrote:
> Hi,
>
> On 30 July 2012 17:33, Guennadi Liakhovetski wrote:
> > Hi Javier
> >
> > On Mon, 30 Jul 2012, javier Martin wrote:
> >
> >> Hi,
> >> thank you for yor ACKs.
> >>
> >> On 20 July 2012 13:31, Guennadi Liakhovetski wrote:
> >> > On Thu, 12 Jul 2012, Javier Martin wrote:
> >> >
> >> >> These flags are not used any longer and can be safely removed
> >> >> since the following patch:
> >> >> http://www.spinics.net/lists/linux-media/msg50165.html
> >> >>
> >> >> Signed-off-by: Javier Martin
> >> >
> >> > For the ARM tree:
> >> >
> >> > Acked-by: Guennadi Liakhovetski
> >>
> >> forgive my ignorance on the matter. Could you please point me to the
> >> git repository this patch should be merged?
> >
> > Sorry, my "for the ARM tree" comment was probably not clear enough. This
> > patch should certainly go via the ARM (SoC) tree, since it only touches
> > arch/arm. So, the maintainer (Sascha - added to CC), that will be
> > forwarding this patch to Linus can thereby add my "acked-by" to this
> > patch, if he feels like it.
> >
>
> Sascha, do you have any comments on this one? I can't find it in
> arm-soc, did you already merge it?
Applied, thanks. I have rewritten the commit message as follows:
Author: Javier Martin
Date: Thu Jul 12 11:03:29 2012 +0200
ARM i.MX mx2_camera: Remove MX2_CAMERA_SWAP16 and MX2_CAMERA_PACK_DIR_MSB
flags.
These flags are not used any longer and can be safely removed
since:
| commit 8a76e5383fb5f58868fdd3a2fe1f4b95988f10a8
| Author: Javier Martin
| Date: Wed Jul 11 17:34:54 2012 +0200
|
|media: mx2_camera: Fix mbus format handling
Signed-off-by: Javier Martin
Acked-by: Laurent Pinchart
Acked-by: Guennadi Liakhovetski
Signed-off-by: Sascha Hauer
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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: Vacations.
On Fri, Aug 03, 2012 at 05:09:15PM +0200, Guennadi Liakhovetski wrote: > Hi Javier > > On Fri, 3 Aug 2012, javier Martin wrote: > > > Hi, > > I will be out of the office until August the 19th. > > > > I just wanted to send a reminder of some patches that I have pending. > > > > For Mauro 3.7: > > > > [PULL] video_visstrim for 3.6 > > [PATCH] media: i.MX27: Fix mx2_emmaprp mem2mem driver clocks. > > > > For Guennadi: > > > > [PATCH 1/4] i.MX27: Fix emma-prp and csi clocks. > > As I mentioned several times, the above patch is not for me. Have a nice > vacation. Indeed it's for me. Queued this up. Thanks Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 v2] i.MX27: Visstrim_M10: Add support for deinterlacing driver.
On Thu, Jul 12, 2012 at 01:35:29PM +0200, Javier Martin wrote:
> Visstrim_M10 have a tvp5150 whose video output must be deinterlaced.
> The new mem2mem deinterlacing driver is very useful for that purpose.
>
> Signed-off-by: Javier Martin
> ---
> Changes since v1:
> - Removed commented out code.
>
> ---
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c | 27
> ++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
> b/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
> index 214e4ff..dbef59d 100644
> --- a/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
> +++ b/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
> @@ -232,7 +232,7 @@ static void __init visstrim_camera_init(void)
> static void __init visstrim_reserve(void)
> {
> /* reserve 4 MiB for mx2-camera */
> - mx2_camera_base = arm_memblock_steal(2 * MX2_CAMERA_BUF_SIZE,
> + mx2_camera_base = arm_memblock_steal(3 * MX2_CAMERA_BUF_SIZE,
> MX2_CAMERA_BUF_SIZE);
> }
>
> @@ -419,6 +419,30 @@ static void __init visstrim_coda_init(void)
> return;
> }
>
> +/* DMA deinterlace */
> +static struct platform_device visstrim_deinterlace = {
> + .name = "m2m-deinterlace",
> + .id = 0,
> +};
> +
> +static void __init visstrim_deinterlace_init(void)
> +{
> + int ret = -ENOMEM;
> + struct platform_device *pdev = &visstrim_deinterlace;
> + int dma;
> +
> + ret = platform_device_register(pdev);
ret is unused.
Better use platform_device_register_simple().
> +
> + dma = dma_declare_coherent_memory(&pdev->dev,
> + mx2_camera_base + 2 *
> MX2_CAMERA_BUF_SIZE,
> + mx2_camera_base + 2 *
> MX2_CAMERA_BUF_SIZE,
> + MX2_CAMERA_BUF_SIZE,
> + DMA_MEMORY_MAP |
> DMA_MEMORY_EXCLUSIVE);
Shouldn't this be done before registering the device?
> + if (!(dma & DMA_MEMORY_MAP))
> + return;
> +}
if (!flag) return; else return ?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 4/4] media: mx2_camera: Fix clock handling for i.MX27.
Hi Guennadi,
On Tue, Jul 31, 2012 at 05:14:25PM +0200, Guennadi Liakhovetski wrote:
> Hi Javier
>
> > @@ -436,7 +436,8 @@ static void mx2_camera_deactivate(struct mx2_camera_dev
> > *pcdev)
> > {
> > unsigned long flags;
> >
> > - clk_disable(pcdev->clk_csi);
> > + if (cpu_is_mx27())
> > + clk_disable_unprepare(pcdev->clk_csi);
>
> This tells me, there are already 2 things going on here:
>
> 1. add clock-(un)prepare operations to enable / disable. Is this a problem
> atm? is the driver non-functional without this change or is it just an API
> correctness change? I thought you replied to this already, but
> unfortunately I couldn't find that your reply again, sorry.
Since the common clock framework clk_prepare is mandatory.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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: "[PULL] video_visstrim for 3.6"
On Tue, Jul 31, 2012 at 08:13:59AM +0200, javier Martin wrote: > On 31 July 2012 00:42, Mauro Carvalho Chehab wrote: > > Em 26-07-2012 06:36, Javier Martin escreveu: > >> Hi Mauro, > >> this pull request is composed of two series that provide support for two > >> mem2mem devices: > >> - 'm2m-deinterlace' video deinterlacer > >> - 'coda video codec' > >> I've included platform support for them too. > >> > >> > >> The following changes since commit > >> 6887a4131da3adaab011613776d865f4bcfb5678: > >> > >>Linux 3.5-rc5 (2012-06-30 16:08:57 -0700) > >> > >> are available in the git repository at: > >> > >>https://github.com/jmartinc/video_visstrim.git for_3.6 > >> > >> for you to fetch changes up to 9bb10266da63ae7f8f198573e099580e9f98f4e8: > >> > >>i.MX27: Visstrim_M10: Add support for deinterlacing driver. (2012-07-26 > >> 10:57:30 +0200) > >> > >> > >> Javier Martin (5): > >>i.MX: coda: Add platform support for coda in i.MX27. > >>media: coda: Add driver for Coda video codec. > >>Visstrim M10: Add support for Coda. > >>media: Add mem2mem deinterlacing driver. > >>i.MX27: Visstrim_M10: Add support for deinterlacing driver. > >> > >> arch/arm/mach-imx/clk-imx27.c |4 +- > >> arch/arm/mach-imx/devices-imx27.h |4 + > >> arch/arm/mach-imx/mach-imx27_visstrim_m10.c | 49 +- > >> arch/arm/plat-mxc/devices/Kconfig |6 +- > >> arch/arm/plat-mxc/devices/Makefile |1 + > >> arch/arm/plat-mxc/devices/platform-imx27-coda.c | 37 + > >> arch/arm/plat-mxc/include/mach/devices-common.h |8 + > > > > I need ARM maintainer's ack for the patches that touch the above files. Generally: Acked-by: Sascha Hauer I think that these are quite late for this merge window though. The pull request should have been out before the 3.5 Release. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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] [v3] i.MX27: Fix emma-prp clocks in mx2_camera.c
On Mon, Jul 09, 2012 at 09:46:03AM +0200, javier Martin wrote:
> On 9 July 2012 09:43, Sascha Hauer wrote:
> > On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
> >> On 9 July 2012 09:28, Sascha Hauer wrote:
> >> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
> >> >> This driver wasn't converted to the new clock changes
> >> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
> >> >> of emma-prp related clocks for the i.MX27 was not correct.
> >> >> ---
> >> >> Enable clocks only for i.MX27.
> >> >>
> >> >
> >> > Indeed,
> >> >
> >> >>
> >> >> - pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> >> >> - if (IS_ERR(pcdev->clk_csi)) {
> >> >> - dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> - err = PTR_ERR(pcdev->clk_csi);
> >> >> - goto exit_kfree;
> >> >> + if (cpu_is_mx27()) {
> >> >> + pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> >> >> + if (IS_ERR(pcdev->clk_csi)) {
> >> >> + dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> + err = PTR_ERR(pcdev->clk_csi);
> >> >> + goto exit_kfree;
> >> >> + }
> >> >
> >> > but why? Now the i.MX25 won't get a clock anymore.
> >>
> >> What are the clocks needed by i.MX25? csi only?
> >>
> >> By the way, is anybody using this driver with i.MX25?
> >
> > It seems Baruch (added to Cc) has used it on an i.MX25.
>
> Baruch,
> could you tell us what are the clocks needed by i.MX25?
I just had a look and the i.MX25 it needs three clocks: ipg, ahb and
peripheral clock. So this is broken anyway and should probably be fixed
seperately, that is:
- provide dummy clocks for the csi clocks on i.MX27
- clk_get ipg, ahb and peripheral clocks on all SoCs
- clk_get emma clocks on i.MX27 only
As said, this is a separate topic, so your original patch should be fine
for now.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] [v3] i.MX27: Fix emma-prp clocks in mx2_camera.c
On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
> On 9 July 2012 09:28, Sascha Hauer wrote:
> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
> >> This driver wasn't converted to the new clock changes
> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
> >> of emma-prp related clocks for the i.MX27 was not correct.
> >> ---
> >> Enable clocks only for i.MX27.
> >>
> >
> > Indeed,
> >
> >>
> >> - pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> >> - if (IS_ERR(pcdev->clk_csi)) {
> >> - dev_err(&pdev->dev, "Could not get csi clock\n");
> >> - err = PTR_ERR(pcdev->clk_csi);
> >> - goto exit_kfree;
> >> + if (cpu_is_mx27()) {
> >> + pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> >> + if (IS_ERR(pcdev->clk_csi)) {
> >> + dev_err(&pdev->dev, "Could not get csi clock\n");
> >> + err = PTR_ERR(pcdev->clk_csi);
> >> + goto exit_kfree;
> >> + }
> >
> > but why? Now the i.MX25 won't get a clock anymore.
>
> What are the clocks needed by i.MX25? csi only?
>
> By the way, is anybody using this driver with i.MX25?
It seems Baruch (added to Cc) has used it on an i.MX25.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] [v3] i.MX27: Fix emma-prp clocks in mx2_camera.c
On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
> This driver wasn't converted to the new clock changes
> (clk_prepare_enable/clk_disable_unprepare). Also naming
> of emma-prp related clocks for the i.MX27 was not correct.
> ---
> Enable clocks only for i.MX27.
>
Indeed,
>
> - pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> - if (IS_ERR(pcdev->clk_csi)) {
> - dev_err(&pdev->dev, "Could not get csi clock\n");
> - err = PTR_ERR(pcdev->clk_csi);
> - goto exit_kfree;
> + if (cpu_is_mx27()) {
> + pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> + if (IS_ERR(pcdev->clk_csi)) {
> + dev_err(&pdev->dev, "Could not get csi clock\n");
> + err = PTR_ERR(pcdev->clk_csi);
> + goto exit_kfree;
> + }
but why? Now the i.MX25 won't get a clock anymore.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] [v2] i.MX27: Fix emma-prp clocks in mx2_camera.c
On Fri, Jul 06, 2012 at 09:46:46AM +0200, javier Martin wrote:
> On 6 July 2012 09:34, Sascha Hauer wrote:
> > On Fri, Jul 06, 2012 at 09:13:11AM +0200, Javier Martin wrote:
> >> if (cpu_is_mx27()) {
> >> free_irq(pcdev->irq_emma, pcdev);
> >> - clk_disable(pcdev->clk_emma);
> >> - clk_put(pcdev->clk_emma);
> >> + clk_disable_unprepare(pcdev->clk_emma_ipg);
> >> + clk_disable_unprepare(pcdev->clk_emma_ahb);
> >
> > The clk_disable_unprepare is inside a cpu_is_mx27() which seems correct.
> > Shouldn't the corresponding clk_get be in cpu_is_mx27() aswell?
>
> Yes indeed. Should I fix it in a new version of this patch or should I
> send another one instead?
Another version of this patch should be fine.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] [v2] i.MX27: Fix emma-prp clocks in mx2_camera.c
On Fri, Jul 06, 2012 at 09:13:11AM +0200, Javier Martin wrote:
> This driver wasn't converted to the new clock changes
> (clk_prepare_enable/clk_disable_unprepare). Also naming
> of emma-prp related clocks for the i.MX27 was not correct.
>
> Signed-off-by: Javier Martin
> ---
> arch/arm/mach-imx/clk-imx27.c|8 ---
> drivers/media/video/mx2_camera.c | 47
> +-
> 2 files changed, 31 insertions(+), 24 deletions(-)
>
> @@ -1616,23 +1616,12 @@ static int __devinit mx27_camera_emma_init(struct
> mx2_camera_dev *pcdev)
> goto exit_iounmap;
> }
>
> - pcdev->clk_emma = clk_get(NULL, "emma");
> - if (IS_ERR(pcdev->clk_emma)) {
> - err = PTR_ERR(pcdev->clk_emma);
> - goto exit_free_irq;
> - }
> -
> - clk_enable(pcdev->clk_emma);
> -
> err = mx27_camera_emma_prp_reset(pcdev);
> if (err)
> - goto exit_clk_emma_put;
> + goto exit_free_irq;
>
> return err;
>
> -exit_clk_emma_put:
> - clk_disable(pcdev->clk_emma);
> - clk_put(pcdev->clk_emma);
> exit_free_irq:
> free_irq(pcdev->irq_emma, pcdev);
> exit_iounmap:
> @@ -1655,6 +1644,7 @@ static int __devinit mx2_camera_probe(struct
> platform_device *pdev)
>
> res_csi = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> irq_csi = platform_get_irq(pdev, 0);
> +
> if (res_csi == NULL || irq_csi < 0) {
> dev_err(&pdev->dev, "Missing platform resources data\n");
> err = -ENODEV;
> @@ -1668,12 +1658,26 @@ static int __devinit mx2_camera_probe(struct
> platform_device *pdev)
> goto exit;
> }
>
> - pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> + pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> if (IS_ERR(pcdev->clk_csi)) {
> dev_err(&pdev->dev, "Could not get csi clock\n");
> err = PTR_ERR(pcdev->clk_csi);
> goto exit_kfree;
> }
> + pcdev->clk_emma_ipg = devm_clk_get(&pdev->dev, "emma-ipg");
> + if (IS_ERR(pcdev->clk_emma_ipg)) {
> + err = PTR_ERR(pcdev->clk_emma_ipg);
> + goto exit_kfree;
> + }
> + pcdev->clk_emma_ahb = devm_clk_get(&pdev->dev, "emma-ahb");
> + if (IS_ERR(pcdev->clk_emma_ahb)) {
> + err = PTR_ERR(pcdev->clk_emma_ahb);
> + goto exit_kfree;
> + }
> +
> + clk_prepare_enable(pcdev->clk_csi);
> + clk_prepare_enable(pcdev->clk_emma_ipg);
> + clk_prepare_enable(pcdev->clk_emma_ahb);
>
> pcdev->res_csi = res_csi;
> pcdev->pdata = pdev->dev.platform_data;
> @@ -1768,8 +1772,8 @@ exit_free_emma:
> eallocctx:
> if (cpu_is_mx27()) {
> free_irq(pcdev->irq_emma, pcdev);
> - clk_disable(pcdev->clk_emma);
> - clk_put(pcdev->clk_emma);
> + clk_disable_unprepare(pcdev->clk_emma_ipg);
> + clk_disable_unprepare(pcdev->clk_emma_ahb);
The clk_disable_unprepare is inside a cpu_is_mx27() which seems correct.
Shouldn't the corresponding clk_get be in cpu_is_mx27() aswell?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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: media: i.MX27: Fix emma-prp clocks in mx2_camera.c
Hi Javier,
On Fri, Jul 06, 2012 at 08:31:49AM +0200, Javier Martin wrote:
> This driver wasn't converted to the new clock changes
> (clk_prepare_enable/clk_disable_unprepare). Also naming
> of emma-prp related clocks for the i.MX27 was not correct.
Thanks for fixing this. Sorry for breaking this in the first place.
> @@ -1668,12 +1658,26 @@ static int __devinit mx2_camera_probe(struct
> platform_device *pdev)
> goto exit;
> }
>
> - pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> + pcdev->clk_csi = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(pcdev->clk_csi)) {
> dev_err(&pdev->dev, "Could not get csi clock\n");
> err = PTR_ERR(pcdev->clk_csi);
> goto exit_kfree;
> }
> + pcdev->clk_emma_ipg = devm_clk_get(&pdev->dev, "ipg");
> + if (IS_ERR(pcdev->clk_emma_ipg)) {
> + err = PTR_ERR(pcdev->clk_emma_ipg);
> + goto exit_kfree;
> + }
> + pcdev->clk_emma_ahb = devm_clk_get(&pdev->dev, "ahb");
> + if (IS_ERR(pcdev->clk_emma_ahb)) {
> + err = PTR_ERR(pcdev->clk_emma_ahb);
> + goto exit_kfree;
> + }
So we have three clocks involved here, a csi ahb clock and two emma
clocks. Can we rename the clocks to:
clk_register_clkdev(clk[csi_ahb_gate], "ahb", "mx2-camera.0");
clk_register_clkdev(clk[emma_ahb_gate], "emma-ahb", "mx2-camera.0");
clk_register_clkdev(clk[emma_ipg_gate], "emma-ipg", "mx2-camera.0");
The rationale is that the csi_ahb_gate really is a ahb clock related to
the csi whereas the emma clocks are normally for the emma device, but
the csi driver happens to use parts of the emma.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] Support for 'Coda' video codec IP.
On Mon, Jul 02, 2012 at 12:36:46PM +0200, javier Martin wrote:
> Hi Sascha,
> I almost have a final version ready which includes multi-instance
> support (not tested though) [1]. As I stated, we assumed the extra
> effort of looking at your code in [2] in order to provide a mechanism
> that preserves compatibility between VPUs in i.MX21, i.MX51 and
> i.MX53. This is the only thing left in order to send the driver for
> mainline submission.
>
> While I was reading your code I found out that you keep the following
> formats for v1 (codadx6-i.MX27) codec:
>
> static int vpu_v1_codecs[VPU_CODEC_MAX] = {
> [VPU_CODEC_AVC_DEC] = 2,
> [VPU_CODEC_VC1_DEC] = -1,
> [VPU_CODEC_MP2_DEC] = -1,
> [VPU_CODEC_DV3_DEC] = -1,
> [VPU_CODEC_RV_DEC] = -1,
> [VPU_CODEC_MJPG_DEC] = 0x82,
> [VPU_CODEC_AVC_ENC] = 3,
> [VPU_CODEC_MP4_ENC] = 1,
> [VPU_CODEC_MJPG_ENC] = 0x83,
> };
>
> As I understand, this means the following operations are supported:
>
> 1- H264 decoding.
> 2- H264 encoding
> 3- MP4 encoding.
> 4- MJPG decoding.
> 5- MJPG encoding.
>
> I totally agree with MP4 and H264 formats but, are you sure about
> MJPG? I have a i.MX27 v1 codec (codadx6) but I didn't know that this
> codec supported MJPG. Have you tested this code with an i.MX27 and
> MJPG? Where did you find out that it supports this format?
We haven't tested MJPG on the i.MX27. The table above is from the
original Freescale code, so I assume it's correct and I assume that
the coda dx6 can do MJPEG.
> Are you
> using firmware version 2.2.4 for v1 codecs?
No, 2.2.5
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] Support for 'Coda' video codec IP.
On Wed, Jun 20, 2012 at 10:25:50PM +0800, Shawn Guo wrote: > On Wed, Jun 20, 2012 at 04:10:27PM +0200, javier Martin wrote: > > If I drop this platform data it is OK with you if I don't add device > > tree support by now? > > > I'm fine. Sascha? Fine with me. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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] Support for 'Coda' video codec IP.
On Wed, Jun 20, 2012 at 09:51:32AM +0200, javier Martin wrote: > Hi Sascha, > thank you for your review. > > > > > Since we all move to devicetree shouldn't we stop adding new > > platform devices? > > Our platfrom, 'visstrim_m10' doesn't currently support devicetree yet, > so it would be highly difficult for us to test it at the moment. > Couldn't you add devicetree support in a later patch? I try to motivate people moving to devicetree. At some point I'd like to get rid of the platform based boards in the tree. Adding new platform seems like delaying this instead of working towards a platform board free Kernel. Any other opinions on this? > > > > The Coda device supports four instances. In this patch you only use > > instance 0, but you do not protect this function from being opened > > multiple times. Does this work with multiple opens? > > No, it doesn't work with multiple opens. It would need either > multi-instance handling support or a restriction so that only can be > opened once, as you said. > > > Can we do this driver multiple instance from the start? This could be > > done more easily if we do not create seperate device nodes for > > encoding/decoding, but when we create a single device node which can be > > opened exactly 4 times. The decision whether we do encoder or decoder > > can then be done in set_fmt. > > I don't think adding multi-instance support is that difficult, let me > take a look at your code and if this is the case I'll do it. The only difficult thing in multi instance support is that the core has memory for four different contexts, but only a single processing engine. So you have to queue up the next frames for all instances in a single list and let the driver pick the next frame from the list. In our code we use 'write' to get the datastream from userspace. This means that it may happen that there is not enough data available for the next decoding frame. For encoding we use 'read' to pass the datastream to userspace. This means that there may be not enough space in the fifo. Handling this is a bit complicated. Since you are using mem2mem and work on buffers instead of streams this should be much simpler than in our driver. I'm just telling you so that you don't get confused when you look at our code. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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] Support for H.264/MPEG4 encoder (VPU) in i.MX27.
On Fri, Jun 08, 2012 at 01:32:27PM +0200, javier Martin wrote: > On 8 June 2012 11:23, Sascha Hauer wrote: > > On Fri, Jun 08, 2012 at 11:02:31AM +0200, javier Martin wrote: > > Hi Sascha, > > From our point of view the current situation is the following: > We have a very reliable driver for the VPU which is not mainline but > it's been used for two years in our products. This driver only > supports encoding in the i.MX27 chip. > In parallel, you have a a multichip driver in progress which is not > mainline either, not fully V4L2 compatible and not tested for i.MX27. > [1] > At the same time, we have a driver in progress for i.MX27 encoding > only which follows V4L2 mem2mem framework. [2]. > > The first thing to decide would be which of both drivers we take as a > base for final mainline developing. > In our view, cleaning up driver from Pengutronix [1] would imply a lot > of effort of maintaining code that we cannot really test (i.MX5, > i.MX6) whereas if we continue using [2] we would have something valid > for i.MX27 much faster. Support for decoding and other chips could be > added later. > > The second thing would be whether we keep on developing or whether we > should wait for you to have something in mainline. We have already > allocated resources to the development of the driver and long-term > testing to achieve product level reliability. Pengutronix does not > seem to be committed to developing the features relevant to our > product (lack of YUV420 support for i.MX27 camera driver[6]) nor > committed to any deadline (lack of answers or development on dmaengine > for i.MX27[4][5]). Moreover, development effort is only 50% of the > cost and we would still have to spend a lot of time checking the video > stream manually in different real-rife conditions (only extensive > testing allowed us to catch the "P frame marked as IDR" bug [7]). > > As a conclusion we propose that we keep on developing our driver for > encoding in the i.MX27 VPU under the following conditions: > - We will provide a more generic name for the driver than "codadx6", > maybe something as "imx_vpu". Since we already know that this is a codadx6 we should name it codadx instead of anything vpu specific. I also suggest codadx instead of anything more specific like codadx6 since we should rather motivate people to merge other coda cores into this driver. > - We will do an extra effort and will study your code in [1] in order > to provide a modular approach that makes adding new functionality (new > chips or decoding) as easy as possible while making obvious that > further patches do not break support for encoding in the i.MX27. This sounds like a plan. I am happy that you are willing to keep an eye on our driver. It would be a pity to have unnecessary barriers for merging codadx9 stuff later. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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] Support for H.264/MPEG4 encoder (VPU) in i.MX27.
On Fri, Jun 08, 2012 at 11:02:31AM +0200, javier Martin wrote: > Hi, > I've checked this matter with a colleague and we have several reasons > to doubt that the i.MX27 and the i.MX53 can share the same driver for > their Video Processing Units (VPU): > > 1. The VPU in the i.MX27 is a "codadx6" with support for H.264, H.263 > and MPEG4-Part2 [1]. Provided Freescale is using the same IP provider > for i.MX53 and looking at the features that the VPU in this SoC > supports (1080p resolution, VP8) we are probably dealing with a "coda > 9 series" [2]. > > 2. An important part of the functionality for controlling the > "codadx6" is implemented using software messages between the main CPU > and the VPU, this means that a different firmware loaded in the VPU > can substantially change the way it is handled. As previously stated, > i.MX27 and i.MX53 have different IP blocks and because of this, those > messages will be very different. > > For these reasons we suggest that we carry on developing different > drivers for the i.MX27 and the i.MX53. Though it's true that both > drivers could share some overhead given by the use of mem2mem > framework, I don't think this is a good enough reason the merge them. > > By the way, driver for the VPU in the i.MX27 will be called > "codadx6"[3], I suggest you call your driver "coda9" to avoid > confusion. Well, our driver works on i.MX27 and i.MX5. Yes, it needs some abstraction for different register layouts and different features, but the cores behave sufficiently similar that it makes sense to share the code in a single driver. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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] Support for H.264/MPEG4 encoder (VPU) in i.MX27.
On Fri, Jun 08, 2012 at 09:39:15AM +0200, javier Martin wrote: > Hi Robert, > > On 8 June 2012 09:26, Robert Schwebel wrote: > > Hi Javier, > > > > On Fri, Jun 08, 2012 at 09:21:13AM +0200, javier Martin wrote: > >> If you refer to driver in [1] I have some concerns: i.MX27 VPU should > >> be implemented as a V4L2 mem2mem device since it gets raw pictures > >> from memory and outputs encoded frames to memory (some discussion > >> about the subject can be fond here [2]), as Exynos driver from Samsung > >> does. However, this driver you've mentioned doesn't do that: it just > >> creates several mapping regions so that the actual functionality is > >> implemented in user space by a library provided by Freescale, which > >> regarding i.MX27 it is also GPL. > >> > >> What we are trying to do is implementing all the functionality in > >> kernel space using mem2mem V4L2 framework so that it can be accepted > >> in mainline. > > > > We will work on the VPU driver and it's migration towards a proper > > mem2mem device very soon, mainly on MX53, but of course MX27 should be > > taken care of by the same driver. > > > > So I'd suggest that we coordinate that work somehow. > > Do you plan to provide both encoding and decoding support or just one of them? We have both encoding and decoding. It works on i.MX51/53, but was originally written for i.MX27 aswell. I haven't tested i.MX27 for longer now, so it might or might not work. Find the source here: git://git.pengutronix.de/git/imx/gst-plugins-fsl-vpu.git The main difference from the FSL code is that the whole VPU functionality is in a kernel module which talks (mostly) v4l2. Our next taks is to convert this into a real mem2mem device, right now it only works with the included gstreamer plugin. You'll need a small kernel patch to register the device and to add the clocks. The gstreamer plugin is in a horrible state, but with the conversion to mem2mem we hope to get rid of this entirely. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 08/15] video: mx2_emmaprp: Use clk_prepare_enable/clk_disable_unprepare
On Fri, May 25, 2012 at 08:14:49PM -0300, Fabio Estevam wrote: > From: Fabio Estevam > > Prepare the clock before enabling it. > > Cc: Guennadi Liakhovetski > Cc: > Signed-off-by: Fabio Estevam Acked-by: Sascha Hauer > --- > drivers/media/video/mx2_emmaprp.c |4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/video/mx2_emmaprp.c > b/drivers/media/video/mx2_emmaprp.c > index 0bd5815..b364557 100644 > --- a/drivers/media/video/mx2_emmaprp.c > +++ b/drivers/media/video/mx2_emmaprp.c > @@ -800,7 +800,7 @@ static int emmaprp_open(struct file *file) > return ret; > } > > - clk_enable(pcdev->clk_emma); > + clk_prepare_enable(pcdev->clk_emma); > ctx->q_data[V4L2_M2M_SRC].fmt = &formats[1]; > ctx->q_data[V4L2_M2M_DST].fmt = &formats[0]; > > @@ -816,7 +816,7 @@ static int emmaprp_release(struct file *file) > > dprintk(pcdev, "Releasing instance %p\n", ctx); > > - clk_disable(pcdev->clk_emma); > + clk_disable_unprepare(pcdev->clk_emma); > v4l2_m2m_ctx_release(ctx->m2m_ctx); > kfree(ctx); > > -- > 1.7.1 > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 07/15] video: mx2_camera: Use clk_prepare_enable/clk_disable_unprepare
On Fri, May 25, 2012 at 08:14:48PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam
>
> Prepare the clock before enabling it.
>
> Cc: Guennadi Liakhovetski
> Cc:
> Signed-off-by: Fabio Estevam
Acked-by: Sascha Hauer
> ---
> drivers/media/video/mx2_camera.c | 12 ++--
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/video/mx2_camera.c
> b/drivers/media/video/mx2_camera.c
> index ded26b7..71b67a3 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -402,7 +402,7 @@ static void mx2_camera_deactivate(struct mx2_camera_dev
> *pcdev)
> {
> unsigned long flags;
>
> - clk_disable(pcdev->clk_csi);
> + clk_disable_unprepare(pcdev->clk_csi);
> writel(0, pcdev->base_csi + CSICR1);
> if (cpu_is_mx27()) {
> writel(0, pcdev->base_emma + PRP_CNTL);
> @@ -430,7 +430,7 @@ static int mx2_camera_add_device(struct soc_camera_device
> *icd)
> if (pcdev->icd)
> return -EBUSY;
>
> - ret = clk_enable(pcdev->clk_csi);
> + ret = clk_prepare_enable(pcdev->clk_csi);
> if (ret < 0)
> return ret;
>
> @@ -1664,7 +1664,7 @@ static int __devinit mx27_camera_emma_init(struct
> mx2_camera_dev *pcdev)
> goto exit_free_irq;
> }
>
> - clk_enable(pcdev->clk_emma);
> + clk_prepare_enable(pcdev->clk_emma);
>
> err = mx27_camera_emma_prp_reset(pcdev);
> if (err)
> @@ -1673,7 +1673,7 @@ static int __devinit mx27_camera_emma_init(struct
> mx2_camera_dev *pcdev)
> return err;
>
> exit_clk_emma_put:
> - clk_disable(pcdev->clk_emma);
> + clk_disable_unprepare(pcdev->clk_emma);
> clk_put(pcdev->clk_emma);
> exit_free_irq:
> free_irq(pcdev->irq_emma, pcdev);
> @@ -1810,7 +1810,7 @@ exit_free_emma:
> eallocctx:
> if (cpu_is_mx27()) {
> free_irq(pcdev->irq_emma, pcdev);
> - clk_disable(pcdev->clk_emma);
> + clk_disable_unprepare(pcdev->clk_emma);
> clk_put(pcdev->clk_emma);
> iounmap(pcdev->base_emma);
> release_mem_region(pcdev->res_emma->start,
> resource_size(pcdev->res_emma));
> @@ -1850,7 +1850,7 @@ static int __devexit mx2_camera_remove(struct
> platform_device *pdev)
> iounmap(pcdev->base_csi);
>
> if (cpu_is_mx27()) {
> - clk_disable(pcdev->clk_emma);
> + clk_disable_unprepare(pcdev->clk_emma);
> clk_put(pcdev->clk_emma);
> iounmap(pcdev->base_emma);
> res = pcdev->res_emma;
> --
> 1.7.1
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 06/15] video: mx1_camera: Use clk_prepare_enable/clk_disable_unprepare
On Fri, May 25, 2012 at 08:14:47PM -0300, Fabio Estevam wrote: > From: Fabio Estevam > > Prepare the clock before enabling it. > > Cc: Guennadi Liakhovetski > Cc: > Signed-off-by: Fabio Estevam Acked-by: Sascha Hauer > --- > drivers/media/video/mx1_camera.c |4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/video/mx1_camera.c > b/drivers/media/video/mx1_camera.c > index 4296a83..dc58084 100644 > --- a/drivers/media/video/mx1_camera.c > +++ b/drivers/media/video/mx1_camera.c > @@ -402,7 +402,7 @@ static void mx1_camera_activate(struct mx1_camera_dev > *pcdev) > > dev_dbg(pcdev->icd->parent, "Activate device\n"); > > - clk_enable(pcdev->clk); > + clk_prepare_enable(pcdev->clk); > > /* enable CSI before doing anything else */ > __raw_writel(csicr1, pcdev->base + CSICR1); > @@ -421,7 +421,7 @@ static void mx1_camera_deactivate(struct mx1_camera_dev > *pcdev) > /* Disable all CSI interface */ > __raw_writel(0x00, pcdev->base + CSICR1); > > - clk_disable(pcdev->clk); > + clk_disable_unprepare(pcdev->clk); > } > > /* > -- > 1.7.1 > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 v2 2/3] i.MX27: visstrim_m10: Remove use of MX2_CAMERA_SWAP16.
On Mon, Mar 26, 2012 at 03:17:47PM +0200, Javier Martin wrote:
>
> Signed-off-by: Javier Martin
Acked-by: Sascha Hauer
Should go via the media tree.
Sascha
> ---
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
> b/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
> index 3128cfe..4db00c6 100644
> --- a/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
> +++ b/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
> @@ -164,7 +164,7 @@ static struct platform_device visstrim_tvp5150 = {
>
>
> static struct mx2_camera_platform_data visstrim_camera = {
> - .flags = MX2_CAMERA_CCIR | MX2_CAMERA_CCIR_INTERLACE |
> MX2_CAMERA_SWAP16 | MX2_CAMERA_PCLK_SAMPLE_RISING,
> + .flags = MX2_CAMERA_CCIR | MX2_CAMERA_CCIR_INTERLACE |
> MX2_CAMERA_PCLK_SAMPLE_RISING,
> .clk = 10,
> };
>
> --
> 1.7.0.4
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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] ARM: i.MX35: Add set_rate and round_rate calls to csi_clk
On Tue, Mar 20, 2012 at 12:29:52PM +0200, Alex Gershgorin wrote:
> This patch add set_rate and round_rate calls to csi_clk. This is needed
> to give mx3-camera control over master clock rate to camera.
The file you are patching is scheduled for removal in favour for the
common clock framework, so you are flogging a dead horse here. I suggest
that you wait some time until I finished the i.MX35 patches for this.
> +static int csi_set_rate(struct clk *clk, unsigned long rate)
> +{
> + unsigned long div;
> + unsigned long parent_rate;
> + unsigned long pdr2 = __raw_readl(CCM_BASE + CCM_PDR2);
> +
> + if (pdr2 & (1 << 7))
> + parent_rate = get_rate_arm();
> + else
> + parent_rate = get_rate_ppll();
> +
> + div = parent_rate / rate;
> +
> + /* Set clock divider */
> + pdr2 |= ((div - 1) & 0x3f) << 16;
btw you forget to clear the divider bits in pdr2 before
setting the new values.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 v1] i.MX35-PDK: Add Camera support
On Mon, Mar 19, 2012 at 07:43:32PM -0300, Fabio Estevam wrote: > Hi Sascha, > > On Mon, Mar 19, 2012 at 7:37 PM, Sascha Hauer wrote: > > > It's scheduled there. I should have responded with an applied message. > > Please apply this one too: http://patchwork.ozlabs.org/patch/144942/ > Will do. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 v1] i.MX35-PDK: Add Camera support
On Mon, Mar 19, 2012 at 07:17:27PM -0300, Mauro Carvalho Chehab wrote: > Em 19-03-2012 19:03, Mauro Carvalho Chehab escreveu: > > Em 13-03-2012 12:05, Alex Gershgorin escreveu: > >> In i.MX35-PDK, OV2640 camera is populated on the > >> personality board. This camera is registered as a subdevice via soc-camera > >> interface. > >> > >> Signed-off-by: Alex Gershgorin > > > > Patch doesn't apply over v3.3: > > Sorry, the previous version of this patch didn't apply. This compiles OK. > > Sorry for the mess. > > Anyway, it should be applied via arm subtree. It's scheduled there. I should have responded with an applied message. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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] i.MX35-PDK: Add Camera support
On Mon, Mar 12, 2012 at 06:28:51PM +0200, Alex wrote: > In i.MX35-PDK, OV2640 camera is populated on the > personality board. This camera is registered as a subdevice via soc-camera > interface. > > Signed-off-by: Alex Gershgorin > --- > arch/arm/mach-imx/mach-mx35_3ds.c | 87 > + > 1 files changed, 87 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c > b/arch/arm/mach-imx/mach-mx35_3ds.c This one does not apply as it is obviously based on (an earlier version of) the framebuffer patches of this board. Please always base your stuff on some -rc kernel. We can resolve conflicts later upstream, but we cannot easily apply a patch when we do not have the correct base. Please add the linux arm kernel mailing list next time. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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] V4L: mx2_camera: remove unsupported i.MX27 DMA mode, make EMMA mandatory
On Mon, Feb 20, 2012 at 04:17:39PM +0100, Guennadi Liakhovetski wrote:
> From: Sascha Hauer
>
> The i.MX27 DMA support was introduced with the initial commit of this
> driver and originally created by me. However, I never got this stable
> due to the racy DMA engine and used the EMMA engine instead. As the DMA
> support is most probably unused and broken in its current state, remove
> it. EMMA becomes the only supported mode on i.MX27.
>
> This also helps us get rid of another user of the legacy i.MX DMA
> support and remove the dependency on ARCH_MX* macros as these are
> scheduled for removal.
>
> Signed-off-by: Sascha Hauer
> [g.liakhovet...@gmx.de: remove unused goto]
> Signed-off-by: Guennadi Liakhovetski
> ---
>
> Sascha, I'd prefer to merge your two mx2_camera patches and additionally
> slightly improve it by removing a dangling goto. Please confirm, that
> you're ok with this version.
>
Looks good.
Acked-by: Sascha Hauer
> Thanks
> Guennadi
>
> drivers/media/video/mx2_camera.c | 245 -
> 1 files changed, 27 insertions(+), 218 deletions(-)
>
> diff --git a/drivers/media/video/mx2_camera.c
> b/drivers/media/video/mx2_camera.c
> index 04aab0c..f771f53 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -38,9 +38,6 @@
> #include
>
> #include
> -#ifdef CONFIG_MACH_MX27
> -#include
> -#endif
> #include
>
> #include
> @@ -206,8 +203,6 @@
> #define PRP_INTR_LBOVF (1 << 7)
> #define PRP_INTR_CH2OVF (1 << 8)
>
> -#define mx27_camera_emma(pcdev) (cpu_is_mx27() && pcdev->use_emma)
> -
> #define MAX_VIDEO_MEM16
>
> struct mx2_prp_cfg {
> @@ -250,8 +245,6 @@ struct mx2_camera_dev {
> struct mx2_buffer *fb1_active;
> struct mx2_buffer *fb2_active;
>
> - int use_emma;
> -
> u32 csicr1;
>
> void*discard_buffer;
> @@ -330,7 +323,7 @@ static void mx2_camera_deactivate(struct mx2_camera_dev
> *pcdev)
>
> clk_disable(pcdev->clk_csi);
> writel(0, pcdev->base_csi + CSICR1);
> - if (mx27_camera_emma(pcdev)) {
> + if (cpu_is_mx27()) {
> writel(0, pcdev->base_emma + PRP_CNTL);
> } else if (cpu_is_mx25()) {
> spin_lock_irqsave(&pcdev->lock, flags);
> @@ -362,7 +355,7 @@ static int mx2_camera_add_device(struct soc_camera_device
> *icd)
>
> csicr1 = CSICR1_MCLKEN;
>
> - if (mx27_camera_emma(pcdev)) {
> + if (cpu_is_mx27()) {
> csicr1 |= CSICR1_PRP_IF_EN | CSICR1_FCC |
> CSICR1_RXFF_LEVEL(0);
> } else if (cpu_is_mx27())
> @@ -402,42 +395,6 @@ static void mx2_camera_remove_device(struct
> soc_camera_device *icd)
> pcdev->icd = NULL;
> }
>
> -#ifdef CONFIG_MACH_MX27
> -static void mx27_camera_dma_enable(struct mx2_camera_dev *pcdev)
> -{
> - u32 tmp;
> -
> - imx_dma_enable(pcdev->dma);
> -
> - tmp = readl(pcdev->base_csi + CSICR1);
> - tmp |= CSICR1_RF_OR_INTEN;
> - writel(tmp, pcdev->base_csi + CSICR1);
> -}
> -
> -static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
> -{
> - struct mx2_camera_dev *pcdev = data;
> - u32 status = readl(pcdev->base_csi + CSISR);
> -
> - if (status & CSISR_SOF_INT && pcdev->active) {
> - u32 tmp;
> -
> - tmp = readl(pcdev->base_csi + CSICR1);
> - writel(tmp | CSICR1_CLR_RXFIFO, pcdev->base_csi + CSICR1);
> - mx27_camera_dma_enable(pcdev);
> - }
> -
> - writel(CSISR_SOF_INT | CSISR_RFF_OR_INT, pcdev->base_csi + CSISR);
> -
> - return IRQ_HANDLED;
> -}
> -#else
> -static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
> -{
> - return IRQ_NONE;
> -}
> -#endif /* CONFIG_MACH_MX27 */
> -
> static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
> int state)
> {
> @@ -617,28 +574,7 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
> vb->state = VIDEOBUF_QUEUED;
> list_add_tail(&vb->queue, &pcdev->capture);
>
> - if (mx27_camera_emma(pcdev)) {
> - goto out;
> -#ifdef CONFIG_MACH_MX27
> - } else if (cpu_is_mx27()) {
> - int ret;
> -
> - if (pcdev->active == NULL) {
> - ret = imx_dma_setup_single(pcdev->dma,
> - videobuf_to_dma_contig(vb), vb-&g
Re: [PATCH 1/2] media/video mx2_camera: make using emma mandatory for i.MX27
On Fri, Feb 17, 2012 at 10:24:13AM +0100, Guennadi Liakhovetski wrote:
> Hi Sascha
>
> Thanks for the patch. Just one question:
>
> On Fri, 17 Feb 2012, Sascha Hauer wrote:
>
> > The i.MX27 dma support was introduced with the initial commit of
> > this driver and originally created by me. However, I never got
> > this stable due to the racy dma engine and used the EMMA engine
> > instead. As the DMA support is most probably unused and broken in
> > its current state, remove it. This also helps us to get rid of
> > another user of the legacy i.MX DMA support,
> > Also, remove the dependency on ARCH_MX* macros as these are scheduled
> > for removal.
> >
> > This patch only removes the use_emma variable and assumes it's
> > hardcoded '1'. The resulting dead code is removed in the next patch.
> >
> > Signed-off-by: Sascha Hauer
> > ---
> > drivers/media/video/mx2_camera.c | 21 -
> > 1 files changed, 8 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/video/mx2_camera.c
> > b/drivers/media/video/mx2_camera.c
> > index 04aab0c..65709e4 100644
> > --- a/drivers/media/video/mx2_camera.c
> > +++ b/drivers/media/video/mx2_camera.c
>
> [snip]
>
> > @@ -1620,7 +1616,6 @@ static int __devinit mx2_camera_probe(struct
> > platform_device *pdev)
> >
> > if (res_emma && irq_emma >= 0) {
> > dev_info(&pdev->dev, "Using EMMA\n");
> > - pcdev->use_emma = 1;
> > pcdev->res_emma = res_emma;
> > pcdev->irq_emma = irq_emma;
> > if (mx27_camera_emma_init(pcdev))
>
> If emma is becoming the only way to use this driver on i.MX27, shouldn't
> the EMMA memory and IRQ resources become compulsory? I.e., if any of them
> is missing we should error out?
Yes, done in 2/2.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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: video: mx2_camera: Remove ifdef's
Hi Guennadi, On Thu, Feb 16, 2012 at 08:06:16PM +0100, Guennadi Liakhovetski wrote: > Hi > > On Thu, 16 Feb 2012, Baruch Siach wrote: > > > Hi Fabio, > > > > On Thu, Feb 16, 2012 at 04:25:39PM -0200, Fabio Estevam wrote: > > > As we are able to build a same kernel that supports both mx27 and mx25, > > > we should remove > > > the ifdef's for CONFIG_MACH_MX27 in the mx2_camera driver. > > > > > > Signed-off-by: Fabio Estevam > > > > Acked-by: Baruch Siach > > I'm still hoping to merge this > > http://patchwork.linuxtv.org/patch/298/ > > patch, after it is suitably updated... Sascha, any progress? Just sent an updated series. Let me know if I have to rebase it onto another branch. I tried to do this change mechanically which means that there might be further cleanups possible after my series, but I don't want to break a driver which I currently can't test. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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
[PATCH 2/2] media/video mx2_camera: remove now unused code
As the i.MX27 dma code was disabled in the last patch we can
now remove the resulting dead code. I tried to do this as
mechanically as possible as I currently have no setup to test
this.
Signed-off-by: Sascha Hauer
---
drivers/media/video/mx2_camera.c | 225 --
1 files changed, 21 insertions(+), 204 deletions(-)
diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 65709e4..3972dc2 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -38,9 +38,6 @@
#include
#include
-#ifdef CONFIG_MACH_MX27
-#include
-#endif
#include
#include
@@ -398,42 +395,6 @@ static void mx2_camera_remove_device(struct
soc_camera_device *icd)
pcdev->icd = NULL;
}
-#ifdef CONFIG_MACH_MX27
-static void mx27_camera_dma_enable(struct mx2_camera_dev *pcdev)
-{
- u32 tmp;
-
- imx_dma_enable(pcdev->dma);
-
- tmp = readl(pcdev->base_csi + CSICR1);
- tmp |= CSICR1_RF_OR_INTEN;
- writel(tmp, pcdev->base_csi + CSICR1);
-}
-
-static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
-{
- struct mx2_camera_dev *pcdev = data;
- u32 status = readl(pcdev->base_csi + CSISR);
-
- if (status & CSISR_SOF_INT && pcdev->active) {
- u32 tmp;
-
- tmp = readl(pcdev->base_csi + CSICR1);
- writel(tmp | CSICR1_CLR_RXFIFO, pcdev->base_csi + CSICR1);
- mx27_camera_dma_enable(pcdev);
- }
-
- writel(CSISR_SOF_INT | CSISR_RFF_OR_INT, pcdev->base_csi + CSISR);
-
- return IRQ_HANDLED;
-}
-#else
-static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
-{
- return IRQ_NONE;
-}
-#endif /* CONFIG_MACH_MX27 */
-
static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
int state)
{
@@ -615,26 +576,7 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
if (cpu_is_mx27()) {
goto out;
-#ifdef CONFIG_MACH_MX27
- } else if (cpu_is_mx27()) {
- int ret;
-
- if (pcdev->active == NULL) {
- ret = imx_dma_setup_single(pcdev->dma,
- videobuf_to_dma_contig(vb), vb->size,
- (u32)pcdev->base_dma + 0x10,
- DMA_MODE_READ);
- if (ret) {
- vb->state = VIDEOBUF_ERROR;
- wake_up(&vb->done);
- goto out;
- }
-
- vb->state = VIDEOBUF_ACTIVE;
- pcdev->active = buf;
- }
-#endif
- } else { /* cpu_is_mx25() */
+ } else if (cpu_is_mx25()) {
u32 csicr3, dma_inten = 0;
if (pcdev->fb1_active == NULL) {
@@ -1197,117 +1139,6 @@ static int mx2_camera_reqbufs(struct soc_camera_device
*icd,
return 0;
}
-#ifdef CONFIG_MACH_MX27
-static void mx27_camera_frame_done(struct mx2_camera_dev *pcdev, int state)
-{
- struct videobuf_buffer *vb;
- struct mx2_buffer *buf;
- unsigned long flags;
- int ret;
-
- spin_lock_irqsave(&pcdev->lock, flags);
-
- if (!pcdev->active) {
- dev_err(pcdev->dev, "%s called with no active buffer!\n",
- __func__);
- goto out;
- }
-
- vb = &pcdev->active->vb;
- buf = container_of(vb, struct mx2_buffer, vb);
- WARN_ON(list_empty(&vb->queue));
- dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
- vb, vb->baddr, vb->bsize);
-
- /* _init is used to debug races, see comment in pxa_camera_reqbufs() */
- list_del_init(&vb->queue);
- vb->state = state;
- do_gettimeofday(&vb->ts);
- vb->field_count++;
-
- wake_up(&vb->done);
-
- if (list_empty(&pcdev->capture)) {
- pcdev->active = NULL;
- goto out;
- }
-
- pcdev->active = list_entry(pcdev->capture.next,
- struct mx2_buffer, vb.queue);
-
- vb = &pcdev->active->vb;
- vb->state = VIDEOBUF_ACTIVE;
-
- ret = imx_dma_setup_single(pcdev->dma, videobuf_to_dma_contig(vb),
- vb->size, (u32)pcdev->base_dma + 0x10, DMA_MODE_READ);
-
- if (ret) {
- vb->state = VIDEOBUF_ERROR;
- pcdev->active = NULL;
- wake_up(&vb->done);
- }
-
-out:
- spin_unlock_irqrestore(&pcdev->lock, flags);
-}
-
-static void mx27_camera_dma_err_callback(int channel, void *data, int err)
-{
- struct mx2_camera_dev *pcdev = data;
-
- mx27_camera_frame_done(pcdev, VIDEOBUF_ERROR);
-}
-
-s
[PATCH 1/2] media/video mx2_camera: make using emma mandatory for i.MX27
The i.MX27 dma support was introduced with the initial commit of
this driver and originally created by me. However, I never got
this stable due to the racy dma engine and used the EMMA engine
instead. As the DMA support is most probably unused and broken in
its current state, remove it. This also helps us to get rid of
another user of the legacy i.MX DMA support,
Also, remove the dependency on ARCH_MX* macros as these are scheduled
for removal.
This patch only removes the use_emma variable and assumes it's
hardcoded '1'. The resulting dead code is removed in the next patch.
Signed-off-by: Sascha Hauer
---
drivers/media/video/mx2_camera.c | 21 -
1 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 04aab0c..65709e4 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -206,8 +206,6 @@
#define PRP_INTR_LBOVF (1 << 7)
#define PRP_INTR_CH2OVF(1 << 8)
-#define mx27_camera_emma(pcdev)(cpu_is_mx27() && pcdev->use_emma)
-
#define MAX_VIDEO_MEM 16
struct mx2_prp_cfg {
@@ -250,8 +248,6 @@ struct mx2_camera_dev {
struct mx2_buffer *fb1_active;
struct mx2_buffer *fb2_active;
- int use_emma;
-
u32 csicr1;
void*discard_buffer;
@@ -330,7 +326,7 @@ static void mx2_camera_deactivate(struct mx2_camera_dev
*pcdev)
clk_disable(pcdev->clk_csi);
writel(0, pcdev->base_csi + CSICR1);
- if (mx27_camera_emma(pcdev)) {
+ if (cpu_is_mx27()) {
writel(0, pcdev->base_emma + PRP_CNTL);
} else if (cpu_is_mx25()) {
spin_lock_irqsave(&pcdev->lock, flags);
@@ -362,7 +358,7 @@ static int mx2_camera_add_device(struct soc_camera_device
*icd)
csicr1 = CSICR1_MCLKEN;
- if (mx27_camera_emma(pcdev)) {
+ if (cpu_is_mx27()) {
csicr1 |= CSICR1_PRP_IF_EN | CSICR1_FCC |
CSICR1_RXFF_LEVEL(0);
} else if (cpu_is_mx27())
@@ -617,7 +613,7 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
vb->state = VIDEOBUF_QUEUED;
list_add_tail(&vb->queue, &pcdev->capture);
- if (mx27_camera_emma(pcdev)) {
+ if (cpu_is_mx27()) {
goto out;
#ifdef CONFIG_MACH_MX27
} else if (cpu_is_mx27()) {
@@ -939,7 +935,7 @@ static int mx2_camera_set_bus_param(struct
soc_camera_device *icd)
if (bytesperline < 0)
return bytesperline;
- if (mx27_camera_emma(pcdev)) {
+ if (cpu_is_mx27()) {
ret = mx27_camera_emma_prp_reset(pcdev);
if (ret)
return ret;
@@ -1089,7 +1085,7 @@ static int mx2_camera_set_fmt(struct soc_camera_device
*icd,
pix->colorspace = mf.colorspace;
icd->current_fmt= xlate;
- if (mx27_camera_emma(pcdev))
+ if (cpu_is_mx27())
pcdev->emma_prp = mx27_emma_prp_get_format(xlate->code,
xlate->host_fmt->fourcc);
@@ -1620,7 +1616,6 @@ static int __devinit mx2_camera_probe(struct
platform_device *pdev)
if (res_emma && irq_emma >= 0) {
dev_info(&pdev->dev, "Using EMMA\n");
- pcdev->use_emma = 1;
pcdev->res_emma = res_emma;
pcdev->irq_emma = irq_emma;
if (mx27_camera_emma_init(pcdev))
@@ -1643,7 +1638,7 @@ static int __devinit mx2_camera_probe(struct
platform_device *pdev)
return 0;
exit_free_emma:
- if (mx27_camera_emma(pcdev)) {
+ if (cpu_is_mx27()) {
free_irq(pcdev->irq_emma, pcdev);
clk_disable(pcdev->clk_emma);
clk_put(pcdev->clk_emma);
@@ -1682,14 +1677,14 @@ static int __devexit mx2_camera_remove(struct
platform_device *pdev)
imx_dma_free(pcdev->dma);
#endif /* CONFIG_MACH_MX27 */
free_irq(pcdev->irq_csi, pcdev);
- if (mx27_camera_emma(pcdev))
+ if (cpu_is_mx27())
free_irq(pcdev->irq_emma, pcdev);
soc_camera_host_unregister(&pcdev->soc_host);
iounmap(pcdev->base_csi);
- if (mx27_camera_emma(pcdev)) {
+ if (cpu_is_mx27()) {
clk_disable(pcdev->clk_emma);
clk_put(pcdev->clk_emma);
iounmap(pcdev->base_emma);
--
1.7.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
i.MX27 camera: remove i.MX27 DMA support
i.MX27 DMA support was initially introduced by me and I never got this to work properly. As this is also uses a legacy DMA API and is the source of ifdeffery in the driver, remove it. Sascha Hauer (2): media/video mx2_camera: make using emma mandatory for i.MX27 media/video mx2_camera: remove now unused code drivers/media/video/mx2_camera.c | 244 +- 1 files changed, 28 insertions(+), 216 deletions(-) -- 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: video: mx2_camera: Remove ifdef's
On Thu, Feb 16, 2012 at 04:25:39PM -0200, Fabio Estevam wrote:
> As we are able to build a same kernel that supports both mx27 and mx25, we
> should remove
> the ifdef's for CONFIG_MACH_MX27 in the mx2_camera driver.
It's not that simple. Yes, we are able to build a kernel for both
i.MX25 and i.MX27 and this patch will work when both architectures
are compiled in, but it will break if we try to build it for only
i.MX25.
Sascha
>
> Signed-off-by: Fabio Estevam
> ---
> drivers/media/video/mx2_camera.c | 22 +++---
> 1 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/video/mx2_camera.c
> b/drivers/media/video/mx2_camera.c
> index 04aab0c..afb4619 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -38,9 +38,7 @@
> #include
>
> #include
> -#ifdef CONFIG_MACH_MX27
> #include
> -#endif
> #include
>
> #include
> @@ -402,7 +400,6 @@ static void mx2_camera_remove_device(struct
> soc_camera_device *icd)
> pcdev->icd = NULL;
> }
>
> -#ifdef CONFIG_MACH_MX27
> static void mx27_camera_dma_enable(struct mx2_camera_dev *pcdev)
> {
> u32 tmp;
> @@ -419,6 +416,9 @@ static irqreturn_t mx27_camera_irq(int irq_csi, void
> *data)
> struct mx2_camera_dev *pcdev = data;
> u32 status = readl(pcdev->base_csi + CSISR);
>
> + if(!cpu_is_mx27())
> + return IRQ_NONE;
> +
> if (status & CSISR_SOF_INT && pcdev->active) {
> u32 tmp;
>
> @@ -431,12 +431,6 @@ static irqreturn_t mx27_camera_irq(int irq_csi, void
> *data)
>
> return IRQ_HANDLED;
> }
> -#else
> -static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
> -{
> - return IRQ_NONE;
> -}
> -#endif /* CONFIG_MACH_MX27 */
>
> static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
> int state)
> @@ -619,7 +613,6 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
>
> if (mx27_camera_emma(pcdev)) {
> goto out;
> -#ifdef CONFIG_MACH_MX27
> } else if (cpu_is_mx27()) {
> int ret;
>
> @@ -637,7 +630,6 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
> vb->state = VIDEOBUF_ACTIVE;
> pcdev->active = buf;
> }
> -#endif
> } else { /* cpu_is_mx25() */
> u32 csicr3, dma_inten = 0;
>
> @@ -1201,7 +1193,6 @@ static int mx2_camera_reqbufs(struct soc_camera_device
> *icd,
> return 0;
> }
>
> -#ifdef CONFIG_MACH_MX27
> static void mx27_camera_frame_done(struct mx2_camera_dev *pcdev, int state)
> {
> struct videobuf_buffer *vb;
> @@ -1310,7 +1301,6 @@ err_out:
>
> return err;
> }
> -#endif /* CONFIG_MACH_MX27 */
>
> static unsigned int mx2_camera_poll(struct file *file, poll_table *pt)
> {
> @@ -1558,13 +1548,11 @@ static int __devinit mx2_camera_probe(struct
> platform_device *pdev)
> clk_get_rate(pcdev->clk_csi));
>
> /* Initialize DMA */
> -#ifdef CONFIG_MACH_MX27
> if (cpu_is_mx27()) {
> err = mx27_camera_dma_init(pdev, pcdev);
> if (err)
> goto exit_clk_put;
> }
> -#endif /* CONFIG_MACH_MX27 */
>
> pcdev->res_csi = res_csi;
> pcdev->pdata = pdev->dev.platform_data;
> @@ -1657,12 +1645,10 @@ exit_iounmap:
> exit_release:
> release_mem_region(res_csi->start, resource_size(res_csi));
> exit_dma_free:
> -#ifdef CONFIG_MACH_MX27
> if (cpu_is_mx27())
> imx_dma_free(pcdev->dma);
> exit_clk_put:
> clk_put(pcdev->clk_csi);
> -#endif /* CONFIG_MACH_MX27 */
> exit_kfree:
> kfree(pcdev);
> exit:
> @@ -1677,10 +1663,8 @@ static int __devexit mx2_camera_remove(struct
> platform_device *pdev)
> struct resource *res;
>
> clk_put(pcdev->clk_csi);
> -#ifdef CONFIG_MACH_MX27
> if (cpu_is_mx27())
> imx_dma_free(pcdev->dma);
> -#endif /* CONFIG_MACH_MX27 */
> free_irq(pcdev->irq_csi, pcdev);
> if (mx27_camera_emma(pcdev))
> free_irq(pcdev->irq_emma, pcdev);
> --
> 1.7.1
>
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 v3 1/2] MX2: Add platform definitions for eMMa-PrP device.
On Thu, Dec 08, 2011 at 10:58:32AM -0200, Mauro Carvalho Chehab wrote:
> On 23-11-2011 13:13, Javier Martin wrote:
> >eMMa-PrP device included in Freescale i.MX2 chips can also
> >be used separately to process memory buffers.
>
> This patch is just the arch glue to the driver, so it should be applied via
> the
> media tree, and likely as patch 2, in order to avoid breaking git bisect.
>
> Yet, I'd like to have the mach-imx maintainer's ack on this.
Acked-by: Sascha Hauer
Sascha
>
> Regards,
> Mauro.
>
> >
> >Changes since v2:
> >- Define imx_add_mx2_emmaprp function which also registers device,
> >not only alloc.
> >- Change definition of emma_clk.
> >- Minor fixes.
> >
> >Signed-off-by: Javier Martin
> >---
> > arch/arm/mach-imx/clock-imx27.c |2 +-
> > arch/arm/mach-imx/devices-imx27.h |2 ++
> > arch/arm/plat-mxc/devices/platform-mx2-camera.c | 18 ++
> > arch/arm/plat-mxc/include/mach/devices-common.h |2 ++
> > 4 files changed, 23 insertions(+), 1 deletions(-)
> >
> >diff --git a/arch/arm/mach-imx/clock-imx27.c
> >b/arch/arm/mach-imx/clock-imx27.c
> >index 88fe00a..dc2d7a5 100644
> >--- a/arch/arm/mach-imx/clock-imx27.c
> >+++ b/arch/arm/mach-imx/clock-imx27.c
> >@@ -661,7 +661,7 @@ static struct clk_lookup lookups[] = {
> > _REGISTER_CLOCK(NULL, "dma", dma_clk)
> > _REGISTER_CLOCK(NULL, "rtic", rtic_clk)
> > _REGISTER_CLOCK(NULL, "brom", brom_clk)
> >-_REGISTER_CLOCK(NULL, "emma", emma_clk)
> >+_REGISTER_CLOCK("m2m-emmaprp.0", NULL, emma_clk)
> > _REGISTER_CLOCK(NULL, "slcdc", slcdc_clk)
> > _REGISTER_CLOCK("imx27-fec.0", NULL, fec_clk)
> > _REGISTER_CLOCK(NULL, "emi", emi_clk)
> >diff --git a/arch/arm/mach-imx/devices-imx27.h
> >b/arch/arm/mach-imx/devices-imx27.h
> >index 2f727d7..28537a5 100644
> >--- a/arch/arm/mach-imx/devices-imx27.h
> >+++ b/arch/arm/mach-imx/devices-imx27.h
> >@@ -50,6 +50,8 @@ extern const struct imx_imx_uart_1irq_data
> >imx27_imx_uart_data[];
> > extern const struct imx_mx2_camera_data imx27_mx2_camera_data;
> > #define imx27_add_mx2_camera(pdata)\
> > imx_add_mx2_camera(&imx27_mx2_camera_data, pdata)
> >+#define imx27_add_mx2_emmaprp(pdata)\
> >+imx_add_mx2_emmaprp(&imx27_mx2_camera_data)
> >
> > extern const struct imx_mxc_ehci_data imx27_mxc_ehci_otg_data;
> > #define imx27_add_mxc_ehci_otg(pdata) \
> >diff --git a/arch/arm/plat-mxc/devices/platform-mx2-camera.c
> >b/arch/arm/plat-mxc/devices/platform-mx2-camera.c
> >index b3f4828..11eace9 100644
> >--- a/arch/arm/plat-mxc/devices/platform-mx2-camera.c
> >+++ b/arch/arm/plat-mxc/devices/platform-mx2-camera.c
> >@@ -62,3 +62,21 @@ struct platform_device *__init imx_add_mx2_camera(
> > res, data->iobaseemmaprp ? 4 : 2,
> > pdata, sizeof(*pdata), DMA_BIT_MASK(32));
> > }
> >+
> >+struct platform_device *__init imx_add_mx2_emmaprp(
> >+const struct imx_mx2_camera_data *data)
> >+{
> >+struct resource res[] = {
> >+{
> >+.start = data->iobaseemmaprp,
> >+.end = data->iobaseemmaprp + data->iosizeemmaprp - 1,
> >+.flags = IORESOURCE_MEM,
> >+}, {
> >+.start = data->irqemmaprp,
> >+.end = data->irqemmaprp,
> >+.flags = IORESOURCE_IRQ,
> >+},
> >+};
> >+return imx_add_platform_device_dmamask("m2m-emmaprp", 0,
> >+res, 2, NULL, 0, DMA_BIT_MASK(32));
> >+}
> >diff --git a/arch/arm/plat-mxc/include/mach/devices-common.h
> >b/arch/arm/plat-mxc/include/mach/devices-common.h
> >index def9ba5..1b2258d 100644
> >--- a/arch/arm/plat-mxc/include/mach/devices-common.h
> >+++ b/arch/arm/plat-mxc/include/mach/devices-common.h
> >@@ -223,6 +223,8 @@ struct imx_mx2_camera_data {
> > struct platform_device *__init imx_add_mx2_camera(
> > const struct imx_mx2_camera_data *data,
> > const struct mx2_camera_platform_data *pdata);
> >+struct platform_device *__init imx_add_mx2_emmaprp(
> >+const struct imx_mx2_camera_data *data);
> >
> > #include
> > struct imx_mxc_ehci_data {
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 v2 2/2] MEM2MEM: Add support for eMMa-PrP mem2mem operations.
On Wed, Nov 23, 2011 at 01:32:29PM +0100, javier Martin wrote:
> Hi Sascha,
> I was just trying to fix the issues you pointed previously and I have
> a question for you.
>
> On 22 November 2011 21:55, Sascha Hauer wrote:
> > Hi Javier,
> >> +
> >> +static int emmaprp_probe(struct platform_device *pdev)
> >> +{
> >> + struct emmaprp_dev *pcdev;
> >> + struct video_device *vfd;
> >> + struct resource *res_emma;
> >> + int irq_emma;
> >> + int ret;
> >> +
> >> + pcdev = kzalloc(sizeof *pcdev, GFP_KERNEL);
> >> + if (!pcdev)
> >> + return -ENOMEM;
> >> +
> >> + spin_lock_init(&pcdev->irqlock);
> >> +
> >> + pcdev->clk_emma = clk_get(NULL, "emma");
> >
> > You should change the entry for the emma in
> > arch/arm/mach-imx/clock-imx27.c to the following:
> >
> > _REGISTER_CLOCK("m2m-emmaprp", NULL, emma_clk)
> >
> > and use clk_get(&pdev->dev, NULL) here.
> >
>
> Is this what you are asking for?
>
> --- a/arch/arm/mach-imx/clock-imx27.c
> +++ b/arch/arm/mach-imx/clock-imx27.c
> @@ -661,7 +661,7 @@ static struct clk_lookup lookups[] = {
> _REGISTER_CLOCK(NULL, "dma", dma_clk)
> _REGISTER_CLOCK(NULL, "rtic", rtic_clk)
> _REGISTER_CLOCK(NULL, "brom", brom_clk)
> - _REGISTER_CLOCK(NULL, "emma", emma_clk)
> + _REGISTER_CLOCK("m2m-emmaprp", NULL, emma_clk)
> _REGISTER_CLOCK(NULL, "slcdc", slcdc_clk)
> _REGISTER_CLOCK("imx27-fec.0", NULL, fec_clk)
> _REGISTER_CLOCK(NULL, "emi", emi_clk)
>
> If I do that, mx2_camera.c will stop working.
This depends on the platform device id. If you use with -1
you have to use "m2m-emmaprp", if you use 0 (as you did I think)
you have to use "m2m-emmaprp.0". Basically the string has to
match the device name as found in /sys/devices/platform/
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 v2 1/2] MX2: Add platform definitions for eMMa-PrP device.
On Tue, Nov 22, 2011 at 01:01:55PM +0100, Javier Martin wrote:
> eMMa-PrP device included in Freescale i.MX2 chips can also
> be used separately to process memory buffers.
>
> Signed-off-by: Javier Martin
> ---
> arch/arm/mach-imx/devices-imx27.h |2 +
> arch/arm/plat-mxc/devices/platform-mx2-camera.c | 33
> +++
> arch/arm/plat-mxc/include/mach/devices-common.h |2 +
> 3 files changed, 37 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-imx/devices-imx27.h
> b/arch/arm/mach-imx/devices-imx27.h
> index 2f727d7..519aa36 100644
> --- a/arch/arm/mach-imx/devices-imx27.h
> +++ b/arch/arm/mach-imx/devices-imx27.h
> @@ -50,6 +50,8 @@ extern const struct imx_imx_uart_1irq_data
> imx27_imx_uart_data[];
> extern const struct imx_mx2_camera_data imx27_mx2_camera_data;
> #define imx27_add_mx2_camera(pdata) \
> imx_add_mx2_camera(&imx27_mx2_camera_data, pdata)
> +#define imx27_alloc_mx2_emmaprp(pdata) \
> + imx_alloc_mx2_emmaprp(&imx27_mx2_camera_data)
>
> extern const struct imx_mxc_ehci_data imx27_mxc_ehci_otg_data;
> #define imx27_add_mxc_ehci_otg(pdata)\
> diff --git a/arch/arm/plat-mxc/devices/platform-mx2-camera.c
> b/arch/arm/plat-mxc/devices/platform-mx2-camera.c
> index b3f4828..4a8bd73 100644
> --- a/arch/arm/plat-mxc/devices/platform-mx2-camera.c
> +++ b/arch/arm/plat-mxc/devices/platform-mx2-camera.c
> @@ -6,6 +6,7 @@
> * the terms of the GNU General Public License version 2 as published by the
> * Free Software Foundation.
> */
> +#include
> #include
> #include
>
> @@ -62,3 +63,35 @@ struct platform_device *__init imx_add_mx2_camera(
> res, data->iobaseemmaprp ? 4 : 2,
> pdata, sizeof(*pdata), DMA_BIT_MASK(32));
> }
> +
> +struct platform_device *__init imx_alloc_mx2_emmaprp(
> + const struct imx_mx2_camera_data *data)
Why only alloc and not register?
> +{
> + struct resource res[] = {
> + {
> + .start = data->iobaseemmaprp,
> + .end = data->iobaseemmaprp + data->iosizeemmaprp - 1,
> + .flags = IORESOURCE_MEM,
> + }, {
> + .start = data->irqemmaprp,
> + .end = data->irqemmaprp,
> + .flags = IORESOURCE_IRQ,
> + },
> + };
> + struct platform_device *pdev;
> + int ret = -ENOMEM;
> +
> + pdev = platform_device_alloc("m2m-emmaprp", 0);
> + if (!pdev)
> + goto err;
> +
> + ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
> + if (ret)
> + goto err;
> +
> + return pdev;
> +err:
> + platform_device_put(pdev);
> + return ERR_PTR(-ENODEV);
> +
> +}
> diff --git a/arch/arm/plat-mxc/include/mach/devices-common.h
> b/arch/arm/plat-mxc/include/mach/devices-common.h
> index def9ba5..ce64bd5 100644
> --- a/arch/arm/plat-mxc/include/mach/devices-common.h
> +++ b/arch/arm/plat-mxc/include/mach/devices-common.h
> @@ -223,6 +223,8 @@ struct imx_mx2_camera_data {
> struct platform_device *__init imx_add_mx2_camera(
> const struct imx_mx2_camera_data *data,
> const struct mx2_camera_platform_data *pdata);
> +struct platform_device *__init imx_alloc_mx2_emmaprp(
> + const struct imx_mx2_camera_data *data);
>
> #include
> struct imx_mxc_ehci_data {
> --
> 1.7.0.4
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 v2 2/2] MEM2MEM: Add support for eMMa-PrP mem2mem operations.
Hi Javier,
On Tue, Nov 22, 2011 at 01:01:56PM +0100, Javier Martin wrote:
> Changes since v1:
> - Embed queue data in ctx structure to allow multi instance.
> - Remove redundant job_ready callback.
> - Adjust format against device capabilities.
> - Register/unregister video device at the right time.
> - Other minor coding fixes.
>
> Signed-off-by: Javier Martin
> ---
> drivers/media/video/Kconfig | 10 +
> drivers/media/video/Makefile |2 +
> drivers/media/video/mx2_emmaprp.c | 1035
> +
> 3 files changed, 1047 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/mx2_emmaprp.c
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index b303a3f..77d7921 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -1107,4 +1107,14 @@ config VIDEO_SAMSUNG_S5P_MFC
> help
> MFC 5.1 driver for V4L2.
>
> +config VIDEO_MX2_EMMAPRP
> + tristate "MX2 eMMa-PrP support"
> + depends on VIDEO_DEV && VIDEO_V4L2 && MACH_MX27
Please do not add new references to MACH_MX27. Use SOC_IMX27 instead.
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_MEM2MEM_DEV
> + help
> + MX2X chips have a PrP that can be used to process buffers from
> + memory to memory. Operations include resizing and format
> + conversion.
> +
[...]
> +
> +static int emmaprp_probe(struct platform_device *pdev)
> +{
> + struct emmaprp_dev *pcdev;
> + struct video_device *vfd;
> + struct resource *res_emma;
> + int irq_emma;
> + int ret;
> +
> + pcdev = kzalloc(sizeof *pcdev, GFP_KERNEL);
> + if (!pcdev)
> + return -ENOMEM;
> +
> + spin_lock_init(&pcdev->irqlock);
> +
> + pcdev->clk_emma = clk_get(NULL, "emma");
You should change the entry for the emma in
arch/arm/mach-imx/clock-imx27.c to the following:
_REGISTER_CLOCK("m2m-emmaprp", NULL, emma_clk)
and use clk_get(&pdev->dev, NULL) here.
> + if (IS_ERR(pcdev->clk_emma)) {
> + ret = PTR_ERR(pcdev->clk_emma);
> + goto free_dev;
> + }
> +
> + irq_emma = platform_get_irq(pdev, 0);
> + res_emma = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (irq_emma < 0 || res_emma == NULL) {
> + dev_err(&pdev->dev, "Missing platform resources data\n");
> + ret = -ENODEV;
> + goto free_clk;
> + }
> +
> + ret = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev);
> + if (ret)
> + goto free_clk;
> +
> + mutex_init(&pcdev->dev_mutex);
> +
> + vfd = video_device_alloc();
> + if (!vfd) {
> + v4l2_err(&pcdev->v4l2_dev, "Failed to allocate video device\n");
> + ret = -ENOMEM;
> + goto unreg_dev;
> + }
> +
> + *vfd = emmaprp_videodev;
> + vfd->lock = &pcdev->dev_mutex;
> +
> + video_set_drvdata(vfd, pcdev);
> + snprintf(vfd->name, sizeof(vfd->name), "%s", emmaprp_videodev.name);
> + pcdev->vfd = vfd;
> + v4l2_info(&pcdev->v4l2_dev, EMMAPRP_MODULE_NAME
> + " Device registered as /dev/video%d\n", vfd->num);
> +
> + platform_set_drvdata(pdev, pcdev);
> +
> + if (!request_mem_region(res_emma->start, resource_size(res_emma),
> + MEM2MEM_NAME)) {
> + ret = -EBUSY;
> + goto rel_vdev;
> + }
> +
> + pcdev->base_emma = ioremap(res_emma->start, resource_size(res_emma));
> + if (!pcdev->base_emma) {
> + ret = -ENOMEM;
> + goto rel_mem;
> + }
> + pcdev->irq_emma = irq_emma;
> + pcdev->res_emma = res_emma;
> +
> + ret = request_irq(pcdev->irq_emma, emmaprp_irq, 0,
> + MEM2MEM_NAME, pcdev);
> + if (ret)
> + goto rel_map;
> +
consider using devm_request_mem_region, devm_ioremap and
devm_request_irq here. It simplifies your error handling considerably.
> +
> + pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
> + if (IS_ERR(pcdev->alloc_ctx)) {
> + v4l2_err(&pcdev->v4l2_dev, "Failed to alloc vb2 context\n");
> + ret = PTR_ERR(pcdev->alloc_ctx);
> + goto rel_irq;
> + }
> +
> + pcdev->m2m_dev = v4l2_m2m_init(&m2m_ops);
> + if (IS_ERR(pcdev->m2m_dev)) {
> + v4l2_err(&pcdev->v4l2_dev, "Failed to init mem2mem device\n");
> + ret = PTR_ERR(pcdev->m2m_dev);
> + goto rel_ctx;
> + }
> +
[...]
> +
> +static struct platform_driver emmaprp_pdrv = {
> + .probe = emmaprp_probe,
> + .remove = emmaprp_remove,
> + .driver = {
> + .name = MEM2MEM_NAME,
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static void __exit emmaprp_exit(void)
> +{
> + platform_driver_unregister(&emmaprp_pdrv);
> +}
> +
> +static int __init emmaprp_init(void)
> +{
> + return platform_driver_register(&emmaprp_pdrv);
> +}
> +
> +mo
Re: [PATCH] media i.MX27 camera: remove legacy dma support
Hi Guennadi,
On Wed, Aug 24, 2011 at 09:19:24AM +0200, Guennadi Liakhovetski wrote:
> Sure, if it's broken, let's remove it. But there are a couple of points,
> that we have to fix in this patch. Sorry, a stupid question: has this been
> tested on i.MX27?
Nope, I currently do not have mainline board support for this driver.
Could be a good opportunity to add some...
Your other points are totally valid and I will fix them in the next
round. Let's first see if someone proves me wrong and says this dma
support is indeed working.
> > - return IRQ_HANDLED;
> > -}
> > -#else
> > static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
> > {
> > return IRQ_NONE;
> > }
>
> If this is really all, what's needed for i.MX27 ISR, let's remove it
> completely. But maybe you could explain to me, how it is now supposed to
> work on i.MX27. In probe() we have
>
> irq_handler_t mx2_cam_irq_handler = cpu_is_mx25() ? mx25_camera_irq
> : mx27_camera_irq;
>
> ...
>
> err = request_irq(pcdev->irq_csi, mx2_cam_irq_handler, 0,
> MX2_CAM_DRV_NAME, pcdev);
>
> So, after this patch i.MX27 will always have a dummy camera ISR and just
> use EMMA, right?
Yes, only the EMMA irq is used, we can remove this one.
> Then maybe we have to make EMMA resource availability
> compulsory on those SoCs, and not optional, as now? You'll have to make
> emma the only possibility on i.MX27, then pcdev->use_emma will disappear,
> locations like
ok.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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
[PATCH] media i.MX27 camera: remove legacy dma support
The i.MX27 dma support was introduced with the initial commit of
this driver and originally created by me. However, I never got
this stable due to the racy dma engine and used the EMMA engine
instead. As the DMA support is most probably unused and broken in
its current state, remove it. This also helps us to get rid of
another user of the legacy i.MX DMA support,
Also, remove the dependency on ARCH_MX* macros as these are scheduled
for removal.
Signed-off-by: Sascha Hauer
Cc: Baruch Siach
Cc: linux-media@vger.kernel.org
Cc: Guennadi Liakhovetski
---
drivers/media/video/Kconfig |2 +-
drivers/media/video/mx2_camera.c | 183 --
2 files changed, 1 insertions(+), 184 deletions(-)
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index f574dc0..27b41b8 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -941,7 +941,7 @@ config VIDEO_MX2_HOSTSUPPORT
config VIDEO_MX2
tristate "i.MX27/i.MX25 Camera Sensor Interface driver"
- depends on VIDEO_DEV && SOC_CAMERA && (MACH_MX27 || ARCH_MX25)
+ depends on VIDEO_DEV && SOC_CAMERA && ARCH_MXC
select VIDEOBUF_DMA_CONTIG
select VIDEO_MX2_HOSTSUPPORT
---help---
diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index ec2410c..3b5c8eb 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -38,9 +38,6 @@
#include
#include
-#ifdef CONFIG_MACH_MX27
-#include
-#endif
#include
#include
@@ -330,41 +327,10 @@ static void mx2_camera_remove_device(struct
soc_camera_device *icd)
pcdev->icd = NULL;
}
-#ifdef CONFIG_MACH_MX27
-static void mx27_camera_dma_enable(struct mx2_camera_dev *pcdev)
-{
- u32 tmp;
-
- imx_dma_enable(pcdev->dma);
-
- tmp = readl(pcdev->base_csi + CSICR1);
- tmp |= CSICR1_RF_OR_INTEN;
- writel(tmp, pcdev->base_csi + CSICR1);
-}
-
-static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
-{
- struct mx2_camera_dev *pcdev = data;
- u32 status = readl(pcdev->base_csi + CSISR);
-
- if (status & CSISR_SOF_INT && pcdev->active) {
- u32 tmp;
-
- tmp = readl(pcdev->base_csi + CSICR1);
- writel(tmp | CSICR1_CLR_RXFIFO, pcdev->base_csi + CSICR1);
- mx27_camera_dma_enable(pcdev);
- }
-
- writel(CSISR_SOF_INT | CSISR_RFF_OR_INT, pcdev->base_csi + CSISR);
-
- return IRQ_HANDLED;
-}
-#else
static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
{
return IRQ_NONE;
}
-#endif /* CONFIG_MACH_MX27 */
static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
int state)
@@ -547,25 +513,6 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
if (mx27_camera_emma(pcdev)) {
goto out;
-#ifdef CONFIG_MACH_MX27
- } else if (cpu_is_mx27()) {
- int ret;
-
- if (pcdev->active == NULL) {
- ret = imx_dma_setup_single(pcdev->dma,
- videobuf_to_dma_contig(vb), vb->size,
- (u32)pcdev->base_dma + 0x10,
- DMA_MODE_READ);
- if (ret) {
- vb->state = VIDEOBUF_ERROR;
- wake_up(&vb->done);
- goto out;
- }
-
- vb->state = VIDEOBUF_ACTIVE;
- pcdev->active = buf;
- }
-#endif
} else { /* cpu_is_mx25() */
u32 csicr3, dma_inten = 0;
@@ -1037,117 +984,6 @@ static int mx2_camera_reqbufs(struct soc_camera_device
*icd,
return 0;
}
-#ifdef CONFIG_MACH_MX27
-static void mx27_camera_frame_done(struct mx2_camera_dev *pcdev, int state)
-{
- struct videobuf_buffer *vb;
- struct mx2_buffer *buf;
- unsigned long flags;
- int ret;
-
- spin_lock_irqsave(&pcdev->lock, flags);
-
- if (!pcdev->active) {
- dev_err(pcdev->dev, "%s called with no active buffer!\n",
- __func__);
- goto out;
- }
-
- vb = &pcdev->active->vb;
- buf = container_of(vb, struct mx2_buffer, vb);
- WARN_ON(list_empty(&vb->queue));
- dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
- vb, vb->baddr, vb->bsize);
-
- /* _init is used to debug races, see comment in pxa_camera_reqbufs() */
- list_del_init(&vb->queue);
- vb->state = state;
- do_gettimeofday(&vb->ts);
- vb->field_count++;
-
- wake_up(&vb->done);
-
- if (list_empty(&pcdev->capture)) {
-
[PATCH 1/6] media/video i.MX: use IMX_HAVE_PLATFORM_* macros
The i.MX architecture provides IMX_HAVE_PLATFORM_* macros to signal that a selected SoC supports a certain hardware. Use them instead of depending on ARCH_* directly. Signed-off-by: Sascha Hauer Acked-by: Uwe Kleine-König Cc: linux-media@vger.kernel.org Cc: Mauro Carvalho Chehab Cc: Hans Verkuil --- drivers/media/video/Kconfig |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index aa02160..6f869ed 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -814,7 +814,7 @@ config MX1_VIDEO config VIDEO_MX1 tristate "i.MX1/i.MXL CMOS Sensor Interface driver" - depends on VIDEO_DEV && ARCH_MX1 && SOC_CAMERA + depends on VIDEO_DEV && SOC_CAMERA && IMX_HAVE_PLATFORM_MX1_CAMERA select FIQ select VIDEOBUF_DMA_CONTIG select MX1_VIDEO @@ -872,7 +872,7 @@ config VIDEO_MX2_HOSTSUPPORT config VIDEO_MX2 tristate "i.MX27/i.MX25 Camera Sensor Interface driver" - depends on VIDEO_DEV && SOC_CAMERA && (MACH_MX27 || ARCH_MX25) + depends on VIDEO_DEV && SOC_CAMERA && IMX_HAVE_PLATFORM_MX2_CAMERA select VIDEOBUF_DMA_CONTIG select VIDEO_MX2_HOSTSUPPORT ---help--- -- 1.7.2.3 -- 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
[PATCH] soc-camera: Compile fixes for mx2-camera
mx2-camera got broken during the last merge window. This patch
fixes this and removes some unused variables.
Signed-off-by: Sascha Hauer
---
drivers/media/video/mx2_camera.c | 13 +
1 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 4a27862..072bd2d 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -31,6 +31,7 @@
#include
#include
+#include
#include
#include
#include
@@ -903,8 +904,6 @@ static int mx2_camera_set_crop(struct soc_camera_device
*icd,
static int mx2_camera_set_fmt(struct soc_camera_device *icd,
struct v4l2_format *f)
{
- struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
- struct mx2_camera_dev *pcdev = ici->priv;
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
const struct soc_camera_format_xlate *xlate;
struct v4l2_pix_format *pix = &f->fmt.pix;
@@ -943,8 +942,6 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,
static int mx2_camera_try_fmt(struct soc_camera_device *icd,
struct v4l2_format *f)
{
- struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
- struct mx2_camera_dev *pcdev = ici->priv;
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
const struct soc_camera_format_xlate *xlate;
struct v4l2_pix_format *pix = &f->fmt.pix;
@@ -1024,13 +1021,13 @@ static int mx2_camera_querycap(struct soc_camera_host
*ici,
return 0;
}
-static int mx2_camera_reqbufs(struct soc_camera_file *icf,
+static int mx2_camera_reqbufs(struct soc_camera_device *icd,
struct v4l2_requestbuffers *p)
{
int i;
for (i = 0; i < p->count; i++) {
- struct mx2_buffer *buf = container_of(icf->vb_vidq.bufs[i],
+ struct mx2_buffer *buf = container_of(icd->vb_vidq.bufs[i],
struct mx2_buffer, vb);
INIT_LIST_HEAD(&buf->vb.queue);
}
@@ -1151,9 +1148,9 @@ err_out:
static unsigned int mx2_camera_poll(struct file *file, poll_table *pt)
{
- struct soc_camera_file *icf = file->private_data;
+ struct soc_camera_device *icd = file->private_data;
- return videobuf_poll_stream(file, &icf->vb_vidq, pt);
+ return videobuf_poll_stream(file, &icd->vb_vidq, pt);
}
static struct soc_camera_host_ops mx2_soc_camera_host_ops = {
--
1.7.2.3
--
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/5] mx2_camera: change to register and probe
On Wed, Aug 04, 2010 at 01:01:34AM +0200, Guennadi Liakhovetski wrote: > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > On Tue, Aug 03, 2010 at 08:22:13PM +0200, Guennadi Liakhovetski wrote: > > > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > > > > > change this driver back to register and probe, since some platforms > > > > first have to initialize an already registered power regulator to switch > > > > on the camera. > > > > > > Sorry, don't see a difference. Can you give an example of two call > > > sequences, where this change changes the behaviour? > > > > > > > Yes, when you look at the today posted patch [1] you find the function > > pcm970_baseboard_init_late as an late_initcall. It uses an already > > registred regulator device to turn on the power of the camera before the > > cameras device registration. > > > > [1] [PATCH 1/2] ARM: i.MX27 pcm970: Add camera support > > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022317.html > > Sorry again, still don't understand. What I mean is the following: take > two cases - before and after your patch. What is the difference? As far as > I know, the difference between platform_driver_probe() and > platform_driver_register() is just that the probe method gets discarded in > an __init section, which is suitable for non hotpluggable devices. I don't > know what the difference this should make for call order. So, that's what > I am asking about. Can you explain, how this patch changes the call order > in your case? Can you tell, that in the unpatches case the probe is called > at that moment, and in the patched case it is called at a different point > of time and that fixes the problem. The following is above platform_driver_probe: * Use this instead of platform_driver_register() when you know the device * is not hotpluggable and has already been registered, and you want to * remove its run-once probe() infrastructure from memory after the * driver has bound to the device. So platform_driver_probe will only call the probe function when the device is already there when this function runs. This is not the case on our board. We have to register the camera in late_initcall (to make sure the needed regulators are already there). During late_initcall time the platform_driver_probe has already run. I don't really like the trend to platform_driver_probe, because this makes cases like camera needs regulator which in turn needs SPI even more complicated. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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/4] mx2_camera: return IRQ_NONE when doing nothing
On Tue, Jul 27, 2010 at 03:06:08PM +0300, Baruch Siach wrote: > Signed-off-by: Baruch Siach > --- > drivers/media/video/mx2_camera.c |8 +--- > 1 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/video/mx2_camera.c > b/drivers/media/video/mx2_camera.c > index 1536bd4..b42ad8d 100644 > --- a/drivers/media/video/mx2_camera.c > +++ b/drivers/media/video/mx2_camera.c > @@ -420,15 +420,17 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void > *data) > struct mx2_camera_dev *pcdev = data; > u32 status = readl(pcdev->base_csi + CSISR); > > - if (status & CSISR_DMA_TSF_FB1_INT) > + writel(status, pcdev->base_csi + CSISR); > + > + if (!(status & (CSISR_DMA_TSF_FB1_INT | CSISR_DMA_TSF_FB2_INT))) > + return IRQ_NONE; I'm not sure this is correct. When we get here, the interrupt definitely is from the camera, it's not a shared interrupt. So this only provokes a 'nobody cared' message from the kernel (if it's still present, I don't know). Sascha > + else if (status & CSISR_DMA_TSF_FB1_INT) > mx25_camera_frame_done(pcdev, 1, VIDEOBUF_DONE); > else if (status & CSISR_DMA_TSF_FB2_INT) > mx25_camera_frame_done(pcdev, 2, VIDEOBUF_DONE); > > /* FIXME: handle CSISR_RFF_OR_INT */ > > - writel(status, pcdev->base_csi + CSISR); > - > return IRQ_HANDLED; > } > > -- > 1.7.1 > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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: Pull request mxc
[Added Guennadi, Baruch and linux-media to cc] On Mon, Jul 26, 2010 at 11:32:19AM +0100, Russell King - ARM Linux wrote: > On Mon, Jul 26, 2010 at 10:28:55AM +0100, Russell King - ARM Linux wrote: > > On Mon, Jul 26, 2010 at 11:00:14AM +0200, Sascha Hauer wrote: > > > 160 files changed, 6547 insertions(+), 3276 deletions(-) > > > > I get: > > > > 160 files changed, 6530 insertions(+), 3265 deletions > > > > which seems to be down to this difference between your diffstat and mine: > > > > > arch/arm/mach-mx3/mach-mx31lilly.c | 48 +- > > > > arch/arm/mach-mx3/mach-mx31lilly.c | 20 +- > > > > This seems to be down to 4d5d859 (ARM: mx3: mx31lilly: fix build error for > > !CONFIG_USB_ULPI). > > Err... > > commit 10ee61384e444133e4d2cf2a1f21fdd50c2be297 > Author: Baruch Siach > Date: Sun Jul 4 07:55:10 2010 +0300 > > mx2_camera: Add soc_camera support for i.MX25/i.MX27 > > This is the soc_camera support developed by Sascha Hauer for the i.MX27. > Alan > Carvalho de Assis modified the original driver to get it working on more > recent > kernels. I modified it further to add support for i.MX25. This driver has > been > tested on i.MX25 and i.MX27 based platforms. > > Signed-off-by: Baruch Siach > Acked-by: Guennadi Liakhovetski > Signed-off-by: Sascha Hauer > > Why has no one from the V4L community reviewed this patch? It was posted to linux-media and reviewed there. I put it in my tree to prevent problems due to missing platform_data declarations when board support for the camera gets added. > > Why are you using void __iomem * with the virtual address returned from > dma_alloc_coherent()? It doesn't return IOMEM addresses! Baruch, can you fix this? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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: [PATCHv6] mx2_camera: Add soc_camera support for i.MX25/i.MX27
On Sun, Jul 11, 2010 at 01:18:27PM +0200, Guennadi Liakhovetski wrote: > On Sun, 4 Jul 2010, Baruch Siach wrote: > > > This is the soc_camera support developed by Sascha Hauer for the i.MX27. > > Alan > > Carvalho de Assis modified the original driver to get it working on more > > recent > > kernels. I modified it further to add support for i.MX25. This driver has > > been > > tested on i.MX25 and i.MX27 based platforms. > > > > Signed-off-by: Baruch Siach > > Acked-by: Guennadi Liakhovetski > > So, who shall be taking this patch? I'd prefer it go via ARM. Then you can > easier satisfy any dependencies and enforce a certain patch order. It has > my ack, just verified v6 - my ack still holds (I hope, Baruch did > compile-test this new version on various i.MX2x versions). However, if you > prefer, I can also take it via soc-camera / v4l. Sascha, what's your take? There won't be conflicts when this goes over v4l, but otoh if Baruch plans to add board support till the next window the platform_data will be missing and my tree won't compile. So it's probably best to push it through the i.MX tree. Will apply this afternoon if noone objects. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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: [PATCHv4 0/3] Driver for the i.MX2x CMOS Sensor Interface
Hi Baruch, > > Baruch Siach (3): > mx2_camera: Add soc_camera support for i.MX25/i.MX27 > mx27: add support for the CSI device > mx25: add support for the CSI device applied 2/3 and 3/3 for 2.6.36. Sascha > > arch/arm/mach-mx2/clock_imx27.c |2 +- > arch/arm/mach-mx2/devices.c | 31 + > arch/arm/mach-mx2/devices.h |1 + > arch/arm/mach-mx25/clock.c | 14 +- > arch/arm/mach-mx25/devices.c | 22 + > arch/arm/mach-mx25/devices.h |1 + > arch/arm/plat-mxc/include/mach/memory.h |4 +- > arch/arm/plat-mxc/include/mach/mx25.h|2 + > arch/arm/plat-mxc/include/mach/mx2_cam.h | 46 + > drivers/media/video/Kconfig | 13 + > drivers/media/video/Makefile |1 + > drivers/media/video/mx2_camera.c | 1493 > ++ > 12 files changed, 1625 insertions(+), 5 deletions(-) > create mode 100644 arch/arm/plat-mxc/include/mach/mx2_cam.h > create mode 100644 drivers/media/video/mx2_camera.c > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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: mt9m111 swap_rgb_red_blue
On Mon, May 31, 2010 at 08:46:00AM +0200, Guennadi Liakhovetski wrote: > On Mon, 31 May 2010, Robert Jarzmik wrote: > > > Sascha Hauer writes: > > > > > Hi Robert, > > > > > > I have digged around in the Datasheet and if I understand it correctly > > > the PXA swaps red/blue in RGB mode. So if we do not use rgb mode but yuv > > > (which should be a pass through) we should be able to support rgb on PXA > > > aswell. Robert, can you confirm that with the following patch applied > > > you still get an image but with red/blue swapped? > > I can confirm the color swap. > > If you want to follow that path, I would suggest instead : > >cicr1 |= CICR1_COLOR_SP_VAL(0); > > > > There is no difference from a processing point of view, it's just that > > CICR1_COLOR_SP_VAL(0) is "raw colorspace", with means "pass through", and > > that > > seems to be your goal here. > > That would be the default case in that switch, but raw only supports 8, 9, > or 10 bpp, so, you'd have to use 8bpp but then fake the pixels-per-line > field. That's why I suggested yuv. I could leave a big comment why this is done, but I would implement it using raw mode aswell if that's prefered. > But that would be the cleanest way, yes. Would that work like that? > > > Note that the patch would have to be completed with the BGR565 into RGB565 > > conversion, if the sensor was to provide only BGR565. But that could very > > well > > be for another patch. Will do, I just wanted to see if this works at all. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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: mt9m111 swap_rgb_red_blue
Hi Robert, On Wed, May 26, 2010 at 10:19:21PM +0200, Robert Jarzmik wrote: > Sascha Hauer writes: > > > Hi, > > > > The mt9m111 soc-camera driver has a swap_rgb_red_blue variable which is > > hardcoded to 1. This results in, well the name says it, red and blue being > > swapped in my picture. > > Is this value needed on some boards or is it just a leftover from > > development? > > Hi Sascha, > > It's not a development leftover, it's something that the sensor and the host > have to agree upon (ie. agree upon the output the sensor has to deliver to the > host). > > By now, only the Marvell PXA27x CPU was used as the host of this sensor, and > the > PXA expects the inverted Red/Blue order (ie. have BGR format). I have digged around in the Datasheet and if I understand it correctly the PXA swaps red/blue in RGB mode. So if we do not use rgb mode but yuv (which should be a pass through) we should be able to support rgb on PXA aswell. Robert, can you confirm that with the following patch applied you still get an image but with red/blue swapped? Sascha >From c7b7d94eca2ed3c17121c558b4cbd31eaadb9dc0 Mon Sep 17 00:00:00 2001 From: Sascha Hauer Date: Fri, 28 May 2010 08:23:20 +0200 Subject: [PATCH] pxa_camera: Allow real rgb565 format Signed-off-by: Sascha Hauer --- drivers/media/video/pxa_camera.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c index 7fe70e7..f635ad2 100644 --- a/drivers/media/video/pxa_camera.c +++ b/drivers/media/video/pxa_camera.c @@ -1129,7 +1129,7 @@ static void pxa_camera_setup_cicr(struct soc_camera_device *icd, CICR1_TBIT | CICR1_COLOR_SP_VAL(1); break; case V4L2_PIX_FMT_RGB565: - cicr1 |= CICR1_COLOR_SP_VAL(1) | CICR1_RGB_BPP_VAL(2); + cicr1 |= CICR1_COLOR_SP_VAL(2); break; } -- 1.7.1 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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
