Re: [PATCH v2 08/13] ASoC: pxa: remove the dmaengine compat need

[Daniel Mack]

On Thursday, May 24, 2018 09:06 AM, Robert Jarzmik wrote:

As the pxa architecture switched towards the dmaengine slave map, the
old compatibility mechanism to acquire the dma requestor line number and
priority are not needed anymore.

This patch simplifies the dma resource acquisition, using the more
generic function dma_request_slave_channel().

Signed-off-by: Robert Jarzmik <robert.jarz...@free.fr>


Reviewed-by: Daniel Mack <dan...@zonque.org>


---
  sound/arm/pxa2xx-ac97.c | 14 ++
  sound/arm/pxa2xx-pcm-lib.c  |  6 +++---
  sound/soc/pxa/pxa2xx-ac97.c | 32 +---
  sound/soc/pxa/pxa2xx-i2s.c  |  6 ++
  4 files changed, 12 insertions(+), 46 deletions(-)

diff --git a/sound/arm/pxa2xx-ac97.c b/sound/arm/pxa2xx-ac97.c
index 4bc244c40f80..236a63cdaf9f 100644
--- a/sound/arm/pxa2xx-ac97.c
+++ b/sound/arm/pxa2xx-ac97.c
@@ -63,28 +63,18 @@ static struct snd_ac97_bus_ops pxa2xx_ac97_ops = {
.reset  = pxa2xx_ac97_legacy_reset,
  };
  
-static struct pxad_param pxa2xx_ac97_pcm_out_req = {

-   .prio = PXAD_PRIO_LOWEST,
-   .drcmr = 12,
-};
-
  static struct snd_dmaengine_dai_dma_data pxa2xx_ac97_pcm_out = {
.addr   = __PREG(PCDR),
.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+   .chan_name  = "pcm_pcm_stereo_out",
.maxburst   = 32,
-   .filter_data= _ac97_pcm_out_req,
-};
-
-static struct pxad_param pxa2xx_ac97_pcm_in_req = {
-   .prio = PXAD_PRIO_LOWEST,
-   .drcmr = 11,
  };
  
  static struct snd_dmaengine_dai_dma_data pxa2xx_ac97_pcm_in = {

.addr   = __PREG(PCDR),
.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+   .chan_name  = "pcm_pcm_stereo_in",
.maxburst   = 32,
-   .filter_data= _ac97_pcm_in_req,
  };
  
  static struct snd_pcm *pxa2xx_ac97_pcm;

diff --git a/sound/arm/pxa2xx-pcm-lib.c b/sound/arm/pxa2xx-pcm-lib.c
index e8da3b8ee721..dcbe7ecc1835 100644
--- a/sound/arm/pxa2xx-pcm-lib.c
+++ b/sound/arm/pxa2xx-pcm-lib.c
@@ -125,9 +125,9 @@ int __pxa2xx_pcm_open(struct snd_pcm_substream *substream)
if (ret < 0)
return ret;
  
-	return snd_dmaengine_pcm_open_request_chan(substream,

-   pxad_filter_fn,
-   dma_params->filter_data);
+   return snd_dmaengine_pcm_open(
+   substream, dma_request_slave_channel(rtd->cpu_dai->dev,
+dma_params->chan_name));
  }
  EXPORT_SYMBOL(__pxa2xx_pcm_open);
  
diff --git a/sound/soc/pxa/pxa2xx-ac97.c b/sound/soc/pxa/pxa2xx-ac97.c

index 803818aabee9..1b41c0f2a8fb 100644
--- a/sound/soc/pxa/pxa2xx-ac97.c
+++ b/sound/soc/pxa/pxa2xx-ac97.c
@@ -68,61 +68,39 @@ static struct snd_ac97_bus_ops pxa2xx_ac97_ops = {
.reset  = pxa2xx_ac97_cold_reset,
  };
  
-static struct pxad_param pxa2xx_ac97_pcm_stereo_in_req = {

-   .prio = PXAD_PRIO_LOWEST,
-   .drcmr = 11,
-};
-
  static struct snd_dmaengine_dai_dma_data pxa2xx_ac97_pcm_stereo_in = {
.addr   = __PREG(PCDR),
.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+   .chan_name  = "pcm_pcm_stereo_in",
.maxburst   = 32,
-   .filter_data= _ac97_pcm_stereo_in_req,
-};
-
-static struct pxad_param pxa2xx_ac97_pcm_stereo_out_req = {
-   .prio = PXAD_PRIO_LOWEST,
-   .drcmr = 12,
  };
  
  static struct snd_dmaengine_dai_dma_data pxa2xx_ac97_pcm_stereo_out = {

.addr   = __PREG(PCDR),
.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+   .chan_name  = "pcm_pcm_stereo_out",
.maxburst   = 32,
-   .filter_data= _ac97_pcm_stereo_out_req,
  };
  
-static struct pxad_param pxa2xx_ac97_pcm_aux_mono_out_req = {

-   .prio = PXAD_PRIO_LOWEST,
-   .drcmr = 10,
-};
  static struct snd_dmaengine_dai_dma_data pxa2xx_ac97_pcm_aux_mono_out = {
.addr   = __PREG(MODR),
.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+   .chan_name  = "pcm_aux_mono_out",
.maxburst   = 16,
-   .filter_data= _ac97_pcm_aux_mono_out_req,
  };
  
-static struct pxad_param pxa2xx_ac97_pcm_aux_mono_in_req = {

-   .prio = PXAD_PRIO_LOWEST,
-   .drcmr = 9,
-};
  static struct snd_dmaengine_dai_dma_data pxa2xx_ac97_pcm_aux_mono_in = {
.addr   = __PREG(MODR),
.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+   .chan_name  = "pcm_aux_mono_in",
.maxburst   = 16,
-   .filter_data= _ac97_pcm_aux_mono_in_req,
  };
  
-static struct pxad_param pxa2xx_ac97_pcm_aux_mic_mono_req = {

-   .prio = PXAD_PRIO_LOWEST,
-   .drcmr = 8,
-};
  static struct snd_dmaengine_dai_dma_data pxa2xx_ac97_pcm_mic_mono_in = {
.addr   = __PREG(MCDR),
.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+   .

Re: [PATCH v2 13/13] ARM: pxa: change SSP DMA channels allocation

[Daniel Mack]

On Thursday, May 24, 2018 09:07 AM, Robert Jarzmik wrote:

Now the dma_slave_map is available for PXA architecture, switch the SSP
device to it.

This specifically means that :
- for platform data based machines, the DMA requestor channels are
   extracted from the slave map, where pxa-ssp-dai. is a 1-1 match to
   ssp., and the channels are either "rx" or "tx".

- for device tree platforms, the dma node should be hooked into the
   pxa2xx-ac97 or pxa-ssp-dai node.

Signed-off-by: Robert Jarzmik <robert.jarz...@free.fr>


Acked-by: Daniel Mack <dan...@zonque.org>


We should, however, merge what's left of this management glue code into 
the users of it, so the dma related properties can be put in the right 
devicetree node.


I'll prepare a patch for that for 4.18. This is a good preparation for 
this round though.



Thanks,
Daniel



---
Since v1: Removed channel names from platform_data
---
  arch/arm/plat-pxa/ssp.c| 47 --
  include/linux/pxa2xx_ssp.h |  2 --
  sound/soc/pxa/pxa-ssp.c|  5 ++---
  3 files changed, 2 insertions(+), 52 deletions(-)

diff --git a/arch/arm/plat-pxa/ssp.c b/arch/arm/plat-pxa/ssp.c
index ba13f793fbce..ed36dcab80f1 100644
--- a/arch/arm/plat-pxa/ssp.c
+++ b/arch/arm/plat-pxa/ssp.c
@@ -127,53 +127,6 @@ static int pxa_ssp_probe(struct platform_device *pdev)
if (IS_ERR(ssp->clk))
return PTR_ERR(ssp->clk);
  
-	if (dev->of_node) {

-   struct of_phandle_args dma_spec;
-   struct device_node *np = dev->of_node;
-   int ret;
-
-   /*
-* FIXME: we should allocate the DMA channel from this
-* context and pass the channel down to the ssp users.
-* For now, we lookup the rx and tx indices manually
-*/
-
-   /* rx */
-   ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells",
-0, _spec);
-
-   if (ret) {
-   dev_err(dev, "Can't parse dmas property\n");
-   return -ENODEV;
-   }
-   ssp->drcmr_rx = dma_spec.args[0];
-   of_node_put(dma_spec.np);
-
-   /* tx */
-   ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells",
-1, _spec);
-   if (ret) {
-   dev_err(dev, "Can't parse dmas property\n");
-   return -ENODEV;
-   }
-   ssp->drcmr_tx = dma_spec.args[0];
-   of_node_put(dma_spec.np);
-   } else {
-   res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
-   if (res == NULL) {
-   dev_err(dev, "no SSP RX DRCMR defined\n");
-   return -ENODEV;
-   }
-   ssp->drcmr_rx = res->start;
-
-   res = platform_get_resource(pdev, IORESOURCE_DMA, 1);
-   if (res == NULL) {
-   dev_err(dev, "no SSP TX DRCMR defined\n");
-   return -ENODEV;
-   }
-   ssp->drcmr_tx = res->start;
-   }
-
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (res == NULL) {
dev_err(dev, "no memory resource defined\n");
diff --git a/include/linux/pxa2xx_ssp.h b/include/linux/pxa2xx_ssp.h
index 8461b18e4608..03a7ca46735b 100644
--- a/include/linux/pxa2xx_ssp.h
+++ b/include/linux/pxa2xx_ssp.h
@@ -212,8 +212,6 @@ struct ssp_device {
int type;
int use_count;
int irq;
-   int drcmr_rx;
-   int drcmr_tx;
  
  	struct device_node	*of_node;

  };
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 0291c7cb64eb..e09368d89bbc 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -104,9 +104,8 @@ static int pxa_ssp_startup(struct snd_pcm_substream 
*substream,
dma = kzalloc(sizeof(struct snd_dmaengine_dai_dma_data), GFP_KERNEL);
if (!dma)
return -ENOMEM;
-
-   dma->filter_data = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
-   >drcmr_tx : >drcmr_rx;
+   dma->chan_name = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
+   "tx" : "rx";
  
  	snd_soc_dai_set_dma_data(cpu_dai, substream, dma);
  

Re: [PATCH 05/15] mtd: nand: pxa3xx: remove the dmaengine compat need

[Daniel Mack]

Hi Robert,

Please refer to the attached patch instead of the one I sent earlier. I 
missed to also remove the platform_get_resource(IORESOURCE_DMA) call.



Thanks,
Daniel


On Friday, May 18, 2018 11:31 PM, Daniel Mack wrote:

Hi Robert,

Thanks for this series.

On Monday, April 02, 2018 04:26 PM, Robert Jarzmik wrote:

From: Robert Jarzmik <robert.jarz...@renault.com>

As the pxa architecture switched towards the dmaengine slave map, the
old compatibility mechanism to acquire the dma requestor line number and
priority are not needed anymore.

This patch simplifies the dma resource acquisition, using the more
generic function dma_request_slave_channel().

Signed-off-by: Robert Jarzmik <robert.jarz...@free.fr>
---
   drivers/mtd/nand/pxa3xx_nand.c | 10 +-


This driver was replaced by drivers/mtd/nand/raw/marvell_nand.c
recently, so this patch can be dropped. I attached a version for the new
driver which you can pick instead.


Thanks,
Daniel



>From 72a306157dedb21f8c3289f0f7a288fc4542bd96 Mon Sep 17 00:00:00 2001
From: Daniel Mack <dan...@zonque.org>
Date: Sat, 12 May 2018 21:50:13 +0200
Subject: [PATCH] mtd: rawnand: marvell: remove dmaengine compat code

As the pxa architecture switched towards the dmaengine slave map, the
old compatibility mechanism to acquire the dma requestor line number and
priority are not needed anymore.

This patch simplifies the dma resource acquisition, using the more
generic function dma_request_slave_channel().

Signed-off-by: Daniel Mack <dan...@zonque.org>
---
 drivers/mtd/nand/raw/marvell_nand.c | 17 +
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index ebb1d141b900..319fea77daf1 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2612,8 +2612,6 @@ static int marvell_nfc_init_dma(struct marvell_nfc *nfc)
 		dev);
 	struct dma_slave_config config = {};
 	struct resource *r;
-	dma_cap_mask_t mask;
-	struct pxad_param param;
 	int ret;
 
 	if (!IS_ENABLED(CONFIG_PXA_DMA)) {
@@ -2626,20 +2624,7 @@ static int marvell_nfc_init_dma(struct marvell_nfc *nfc)
 	if (ret)
 		return ret;
 
-	r = platform_get_resource(pdev, IORESOURCE_DMA, 0);
-	if (!r) {
-		dev_err(nfc->dev, "No resource defined for data DMA\n");
-		return -ENXIO;
-	}
-
-	param.drcmr = r->start;
-	param.prio = PXAD_PRIO_LOWEST;
-	dma_cap_zero(mask);
-	dma_cap_set(DMA_SLAVE, mask);
-	nfc->dma_chan =
-		dma_request_slave_channel_compat(mask, pxad_filter_fn,
-		 , nfc->dev,
-		 "data");
+	nfc->dma_chan = dma_request_slave_channel(nfc->dev, "data");
 	if (!nfc->dma_chan) {
 		dev_err(nfc->dev,
 			"Unable to request data DMA channel\n");
-- 
2.14.3

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Daniel Mack]

Hi Maxime,

On Tuesday, May 22, 2018 09:54 PM, Maxime Ripard wrote:

On Mon, May 21, 2018 at 09:39:02AM +0200, Maxime Ripard wrote:

On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:



This set of patches is also not working for my MIPI platform (mine has
a 12 MHz external clock). I am pretty sure is isn't working because it
does not include the following, which my tests have found to be
necessary:

1) Setting pclk period reg in order to correct DPHY timing.
2) Disabling of MIPI lanes when streaming not enabled.
3) setting mipi_div to 1 when the scaler is disabled
4) Doubling ADC clock on faster resolutions.


Yeah, I left them out because I didn't think this was relevant to this
patchset but should come as future improvements. However, given that
it works with the parallel bus, maybe the two first are needed when
adjusting the rate.


I've checked for the pclk period, and it's hardcoded to the same value
all the time, so I guess this is not the reason it doesn't work on
MIPI CSI anymore.

Daniel, could you test:
http://code.bulix.org/ki6kgz-339327?raw


[Note that there's a missing parenthesis in this snippet]

Hmm, no, that doesn't change anything. Streaming doesn't work here, even 
if I move ov5640_load_regs() before any other initialization.


One of my test setup is the following gst pipeline:

  gst-launch-1.0\
v4l2src device=/dev/video0 ! \
videoconvert ! \
video/x-raw,format=UYVY,width=1920,height=1080 ! \
glimagesink

With the pixel clock hard-coded to 16660 in qcom camss, the setup 
works on 4.14, but as I said, it broke already before this series with 
5999f381e023 ("media: ov5640: Add horizontal and vertical

totals").

Frankly, my understanding of these chips is currently limited, so I 
don't really know where to start digging. It seems clear though that the 
timing registers setup is necessary for other register writes to succeed.


Can I help in any other way?


Thanks for all your efforts,
Daniel

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Daniel Mack]

Hi,

On Saturday, May 19, 2018 04:42 AM, Sam Bobrowicz wrote:

This set of patches is also not working for my MIPI platform (mine has
a 12 MHz external clock). I am pretty sure is isn't working because it
does not include the following, which my tests have found to be
necessary:

1) Setting pclk period reg in order to correct DPHY timing.
2) Disabling of MIPI lanes when streaming not enabled.
3) setting mipi_div to 1 when the scaler is disabled
4) Doubling ADC clock on faster resolutions.

I will run some more tests to see if anything else is broken and come
back with some suggestions.

I should mention that the upstream driver has never worked with my
platform. I suspect that the driver only ever worked previously with
MIPI platforms that have loose DPHY timing requirements and a specific
xclk (24MHz maybe?). Out of the interest of collecting more data, can
you provide the following info on your platform?

a) External clock frequency


Mine has a 24MHz oscillator.


b) List of resolutions (including framerates) that are working with
these patches (and your fix) applied


I have a somewhat limited support in userspace which is currently 
hard-coded to 1920x1080@30fps. I haven't tested any other resolution, 
but this one is not working with this patch set.



c) List of resolutions that were working prior to the regression you
experienced with the set_timings function


The one mentioned above did work before, except for one things: I need a 
patch on top that adds a V4L2_CID_PIXEL_RATE control. The qcom camss 
platform needs that in order to calculate its own clock rates. When I 
tested this patch set, I hard-coded the setting the camss driver.


I can send a patch that adds this control once this patch set has landed.


Thanks,
Daniel

Re: [PATCH 05/15] mtd: nand: pxa3xx: remove the dmaengine compat need

[Daniel Mack]

Hi Robert,

Thanks for this series.

On Monday, April 02, 2018 04:26 PM, Robert Jarzmik wrote:

From: Robert Jarzmik <robert.jarz...@renault.com>

As the pxa architecture switched towards the dmaengine slave map, the
old compatibility mechanism to acquire the dma requestor line number and
priority are not needed anymore.

This patch simplifies the dma resource acquisition, using the more
generic function dma_request_slave_channel().

Signed-off-by: Robert Jarzmik <robert.jarz...@free.fr>
---
  drivers/mtd/nand/pxa3xx_nand.c | 10 +-


This driver was replaced by drivers/mtd/nand/raw/marvell_nand.c 
recently, so this patch can be dropped. I attached a version for the new 
driver which you can pick instead.



Thanks,
Daniel
>From c63bc40bdfe2d596e42919235840109a2f1b2776 Mon Sep 17 00:00:00 2001
From: Daniel Mack <dan...@zonque.org>
Date: Sat, 12 May 2018 21:50:13 +0200
Subject: [PATCH] mtd: rawnand: marvell: remove dmaengine compat code

As the pxa architecture switched towards the dmaengine slave map, the
old compatibility mechanism to acquire the dma requestor line number and
priority are not needed anymore.

This patch simplifies the dma resource acquisition, using the more
generic function dma_request_slave_channel().

Signed-off-by: Daniel Mack <dan...@zonque.org>
---
 drivers/mtd/nand/raw/marvell_nand.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index ebb1d141b900..30017cd7d91c 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2612,8 +2612,6 @@ static int marvell_nfc_init_dma(struct marvell_nfc *nfc)
 		dev);
 	struct dma_slave_config config = {};
 	struct resource *r;
-	dma_cap_mask_t mask;
-	struct pxad_param param;
 	int ret;
 
 	if (!IS_ENABLED(CONFIG_PXA_DMA)) {
@@ -2632,14 +2630,7 @@ static int marvell_nfc_init_dma(struct marvell_nfc *nfc)
 		return -ENXIO;
 	}
 
-	param.drcmr = r->start;
-	param.prio = PXAD_PRIO_LOWEST;
-	dma_cap_zero(mask);
-	dma_cap_set(DMA_SLAVE, mask);
-	nfc->dma_chan =
-		dma_request_slave_channel_compat(mask, pxad_filter_fn,
-		 , nfc->dev,
-		 "data");
+	nfc->dma_chan = dma_request_slave_channel(nfc->dev, "data");
 	if (!nfc->dma_chan) {
 		dev_err(nfc->dev,
 			"Unable to request data DMA channel\n");
-- 
2.14.3

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Daniel Mack]

On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:

Part of the hardcoded initialization sequence is to set up the proper clock
dividers. However, this is now done dynamically through proper code and as
such, the static one is now redundant.

Let's remove it.

Signed-off-by: Maxime Ripard 
---


[...]


@@ -625,8 +623,8 @@ static const struct reg_value 
ov5640_setting_30fps_1080P_1920_1080[] = {
{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-   {0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0}, {0x3035, 0x11, 0, 0},
-   {0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
+   {0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
+   {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},


This is the mode that I'm testing with. Previously, the hard-coded 
registers here were:


 OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
 OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
 OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07

Your new code that calculates the clock rates dynamically ends up with 
different values however:


 OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
 OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
 OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03

Interestingly, leaving the hard-coded values in the array *and* letting 
ov5640_set_mipi_pclk() do its thing later still works. So again it seems 
that writes to registers after 0x3035/0x3036/0x3037 seem to depend on 
the values of these timing registers. You might need to leave these 
values as dummies in the array. Confusing.


Any idea?


Thanks,
Daniel

Re: [PATCH v3 01/12] media: ov5640: Fix timings setup code

[Daniel Mack]

Hi,

On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:

From: Samuel Bobrowicz 

The current code, when changing the mode and changing the scaling or
sampling parameters, will look at the horizontal and vertical total size,
which, since 5999f381e023 ("media: ov5640: Add horizontal and vertical
totals") has been moved from the static register initialization to after
the mode change.

That means that the values are no longer set up before the code retrieves
them, which is obviously a bug.


I tried 'for-4.18-5' before your patch set now and noticed it didn't 
work. I then bisected the regression down to the same commit that you 
mentioned above.


The old code (before 5999f381e023) used to have the timing registers 
embedded in a large register blob. Omitting them during the bulk upload 
and writing them later doesn't work here, even if the values are the 
same. It seems that the order in which registers are written matters.


Hence your patch in this email doesn't fix it for me either. I have to 
move ov5640_set_timings() before ov5640_load_regs() to revive my platform.


One of the subsequent patches in this series introduces a new regression 
for me, unfortunately, possibly for the same reason. I'll dig a bit more.


What cameras are you testing this with? MIPI or parallel?


Thanks,
Daniel




Fixes: 5999f381e023 ("media: ov5640: Add horizontal and vertical totals")
Signed-off-by: Samuel Bobrowicz 
Signed-off-by: Maxime Ripard 
---
  drivers/media/i2c/ov5640.c | 12 
  1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e480e53b369b..4bd968b478db 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1462,6 +1462,10 @@ static int ov5640_set_mode_exposure_calc(struct 
ov5640_dev *sensor,
if (ret < 0)
return ret;
  
+	ret = ov5640_set_timings(sensor, mode);

+   if (ret < 0)
+   return ret;
+
/* read capture VTS */
ret = ov5640_get_vts(sensor);
if (ret < 0)
@@ -1589,6 +1593,10 @@ static int ov5640_set_mode_direct(struct ov5640_dev 
*sensor,
if (ret < 0)
return ret;
  
+	ret = ov5640_set_timings(sensor, mode);

+   if (ret < 0)
+   return ret;
+
/* turn auto gain/exposure back on for direct mode */
ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
if (ret)
@@ -1633,10 +1641,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
ret = ov5640_set_mode_direct(sensor, mode, exposure);
}
  
-	if (ret < 0)

-   return ret;
-
-   ret = ov5640_set_timings(sensor, mode);
if (ret < 0)
return ret;
  

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Daniel Mack]

Hi,

On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:

Here is a "small" series that mostly cleans up the ov5640 driver code,
slowly getting rid of the big data array for more understandable code
(hopefully).

The biggest addition would be the clock rate computation at runtime,
instead of relying on those arrays to setup the clock tree
properly. As a side effect, it fixes the framerate that was off by
around 10% on the smaller resolutions, and we now support 60fps.

This also introduces a bunch of new features.


I'd like to give this a try. What tree should this patch set be applied 
on? I had no luck with media_tree/for-4.18-6.


Thanks,
Daniel




Let me know what you think,
Maxime

Changes from v2:
   - Rebased on latest Sakari PR
   - Fixed the issues reported by Hugues: improper FPS returned for
 formats, improper rounding of the FPS, some with his suggestions,
 some by simplifying the logic.
   - Expanded the clock tree comments based on the feedback from Samuel
 Bobrowicz and Loic Poulain
   - Merged some of the changes made by Samuel Bobrowicz to fix the
 MIPI rate computation, fix the call sites of the
 ov5640_set_timings function, the auto-exposure calculation call,
 etc.
   - Split the patches into smaller ones in order to make it more
 readable (hopefully)

Changes from v1:
   - Integrated Hugues' suggestions to fix v4l2-compliance
   - Fixed the bus width with JPEG
   - Dropped the clock rate calculation loops for something simpler as
 suggested by Sakari
   - Cache the exposure value instead of using the control value
   - Rebased on top of 4.17

Maxime Ripard (11):
   media: ov5640: Adjust the clock based on the expected rate
   media: ov5640: Remove the clocks registers initialization
   media: ov5640: Remove redundant defines
   media: ov5640: Remove redundant register setup
   media: ov5640: Compute the clock rate at runtime
   media: ov5640: Remove pixel clock rates
   media: ov5640: Enhance FPS handling
   media: ov5640: Make the return rate type more explicit
   media: ov5640: Make the FPS clamping / rounding more extendable
   media: ov5640: Add 60 fps support
   media: ov5640: Remove duplicate auto-exposure setup

Samuel Bobrowicz (1):
   media: ov5640: Fix timings setup code

  drivers/media/i2c/ov5640.c | 701 +
  1 file changed, 392 insertions(+), 309 deletions(-)

Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

[Daniel Mack]

Hi,

On Tuesday, April 24, 2018 12:22 PM, Sakari Ailus wrote:
> On Fri, Apr 20, 2018 at 11:44:18AM +0200, Daniel Mack wrote:
>> Add v4l2 controls to report the pixel and MIPI link rates of each mode.
>> The camss camera subsystem needs them to set up the correct hardware
>> clocks.
>>
>> Tested on msm8016 based hardware.
>>
>> Signed-off-by: Daniel Mack <dan...@zonque.org>
> 
> Maxime has written a number of patches against the driver that seem very
> much related; could you rebase these on his set (v2)?
> 
> <URL:https://patchwork.linuxtv.org/project/linux-media/list/?submitter=Maxime+Ripard=*=ov5640>

I didn't know about the ongoing work in this area, so I think both this
and 3/3 are not needed. If you want, you can still pick the 1st patch in
this series, but that's just a cosmetic cleanup.


Thanks,
Daniel

Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

[Daniel Mack]

Hi,

On Friday, April 20, 2018 04:15 PM, Maxime Ripard wrote:
> On Fri, Apr 20, 2018 at 11:44:18AM +0200, Daniel Mack wrote:

>>  struct ov5640_ctrls {
>>  struct v4l2_ctrl_handler handler;
>> +struct {
>> +struct v4l2_ctrl *link_freq;
>> +struct v4l2_ctrl *pixel_rate;
>> +};
>>  struct {
>>  struct v4l2_ctrl *auto_exp;
>>  struct v4l2_ctrl *exposure;
>> @@ -732,6 +752,8 @@ static const struct ov5640_mode_info 
>> ov5640_mode_init_data = {
>>  .dn_mode= SUBSAMPLING,
>>  .width  = 640,
>>  .height = 480,
>> +.pixel_rate = 2776,
>> +.link_freq_idx  = OV5640_LINK_FREQ_111,
> 
> I'm not sure where this is coming from, but on a parallel sensor I
> have a quite different pixel rate.

Ah, interesting. What exactly do you mean by 'parallel'? What kind of
module is that, and what are your pixel rates?

> I have a serie ongoing that tries to deal with this, hopefully in
> order to get rid of all the clock setup done in the initialiasation
> array.
> 
> See https://patchwork.linuxtv.org/patch/48710/ for the patch and
> https://www.spinics.net/lists/linux-media/msg132201.html for a
> discussion on what the clock tree might look like on a MIPI-CSI bus.

Okay, nice. Even better if this patch isn't needed in the end.


Thanks!
Daniel

[PATCH 1/3] media: ov5640: initialize mode data structs by name

[Daniel Mack]

This patch initializes the members of struct ov5640_mode_info by name for
better readability. This makes later additions to this struct easier.

No functional change intended.

Signed-off-by: Daniel Mack <dan...@zonque.org>
---
 drivers/media/i2c/ov5640.c | 207 +
 1 file changed, 152 insertions(+), 55 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 852026baa2e7..96f1564abdf5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -728,67 +728,164 @@ static const struct reg_value 
ov5640_setting_15fps_QSXGA_2592_1944[] = {
 
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
-   0, SUBSAMPLING, 640, 480, ov5640_init_setting_30fps_VGA,
-   ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
+   .id = 0,
+   .dn_mode= SUBSAMPLING,
+   .width  = 640,
+   .height = 480,
+   .reg_data   = ov5640_init_setting_30fps_VGA,
+   .reg_data_size  = ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
 
 static const struct ov5640_mode_info
 ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
+{
+   {
+   .id = OV5640_MODE_QCIF_176_144,
+   .dn_mode= SUBSAMPLING,
+   .width  = 176,
+   .height = 144,
+   .reg_data   = ov5640_setting_15fps_QCIF_176_144,
+   .reg_data_size  = ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144),
+   },
+   {
+   .id = OV5640_MODE_QVGA_320_240,
+   .dn_mode= SUBSAMPLING,
+   .width  = 320,
+   .height = 240,
+   .reg_data   = ov5640_setting_15fps_QVGA_320_240,
+   .reg_data_size  = ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240),
+   },
{
-   {OV5640_MODE_QCIF_176_144, SUBSAMPLING, 176, 144,
-ov5640_setting_15fps_QCIF_176_144,
-ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
-   {OV5640_MODE_QVGA_320_240, SUBSAMPLING, 320,  240,
-ov5640_setting_15fps_QVGA_320_240,
-ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
-   {OV5640_MODE_VGA_640_480, SUBSAMPLING, 640,  480,
-ov5640_setting_15fps_VGA_640_480,
-ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
-   {OV5640_MODE_NTSC_720_480, SUBSAMPLING, 720, 480,
-ov5640_setting_15fps_NTSC_720_480,
-ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
-   {OV5640_MODE_PAL_720_576, SUBSAMPLING, 720, 576,
-ov5640_setting_15fps_PAL_720_576,
-ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
-   {OV5640_MODE_XGA_1024_768, SUBSAMPLING, 1024, 768,
-ov5640_setting_15fps_XGA_1024_768,
-ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
-   {OV5640_MODE_720P_1280_720, SUBSAMPLING, 1280, 720,
-ov5640_setting_15fps_720P_1280_720,
-ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
-   {OV5640_MODE_1080P_1920_1080, SCALING, 1920, 1080,
-ov5640_setting_15fps_1080P_1920_1080,
-ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
-   {OV5640_MODE_QSXGA_2592_1944, SCALING, 2592, 1944,
-ov5640_setting_15fps_QSXGA_2592_1944,
-ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
-   }, {
-   {OV5640_MODE_QCIF_176_144, SUBSAMPLING, 176, 144,
-ov5640_setting_30fps_QCIF_176_144,
-ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
-   {OV5640_MODE_QVGA_320_240, SUBSAMPLING, 320,  240,
-ov5640_setting_30fps_QVGA_320_240,
-ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
-   {OV5640_MODE_VGA_640_480, SUBSAMPLING, 640,  480,
-ov5640_setting_30fps_VGA_640_480,
-ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
-   {OV5640_MODE_NTSC_720_480, SUBSAMPLING, 720, 480,
-ov5640_setting_30fps_NTSC_720_480,
-ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
-   {OV5640_MODE_PAL_720_576, SUBSAMPLING, 720, 576,
-ov5640_setting_30fps_PAL_720_576,
-ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
-   {OV5640_MODE_XGA_1024_768, SUBSAMPLING, 1024, 768,
-ov5640_setting_30fps_XGA_1024_768,
-ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
-   {OV5640_MODE_720P_1280_720, SUBSAMPLING, 1280, 720,
-ov5640_setting_30fps_720P_1280_720,
-ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
-   {OV5640_MODE_1080P_1920_1080, SCALING, 1920

[PATCH 3/3] media: ov5640: add support for xclk frequency control

[Daniel Mack]

Allow setting the xclk rate via an optional 'clock-frequency' property in
the device tree node.

Signed-off-by: Daniel Mack <dan...@zonque.org>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.txt |  2 ++
 drivers/media/i2c/ov5640.c | 10 ++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt 
b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
index 8e36da0d8406..584bbc944978 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
@@ -13,6 +13,8 @@ Optional Properties:
   This is an active low signal to the OV5640.
 - powerdown-gpios: reference to the GPIO connected to the powerdown pin,
   if any. This is an active high signal to the OV5640.
+- clock-frequency: frequency to set on the xclk input clock. The clock
+  is left untouched if this property is missing.
 
 The device node must contain one 'port' child node for its digital output
 video port, in accordance with the video interface bindings defined in
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 78669ed386cd..2d94d6dbda5d 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2685,6 +2685,7 @@ static int ov5640_probe(struct i2c_client *client,
struct fwnode_handle *endpoint;
struct ov5640_dev *sensor;
struct v4l2_mbus_framefmt *fmt;
+   u32 freq;
int ret;
 
sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
@@ -2731,6 +2732,15 @@ static int ov5640_probe(struct i2c_client *client,
return PTR_ERR(sensor->xclk);
}
 
+   ret = of_property_read_u32(dev->of_node, "clock-frequency", );
+   if (ret == 0) {
+   ret = clk_set_rate(sensor->xclk, freq);
+   if (ret) {
+   dev_err(dev, "could not set xclk frequency\n");
+   return ret;
+   }
+   }
+
sensor->xclk_freq = clk_get_rate(sensor->xclk);
if (sensor->xclk_freq < OV5640_XCLK_MIN ||
sensor->xclk_freq > OV5640_XCLK_MAX) {
-- 
2.14.3

[PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

[Daniel Mack]

Add v4l2 controls to report the pixel and MIPI link rates of each mode.
The camss camera subsystem needs them to set up the correct hardware
clocks.

Tested on msm8016 based hardware.

Signed-off-by: Daniel Mack <dan...@zonque.org>
---
 drivers/media/i2c/ov5640.c | 77 ++
 1 file changed, 77 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 96f1564abdf5..78669ed386cd 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -91,6 +91,20 @@
 #define OV5640_REG_SDE_CTRL5   0x5585
 #define OV5640_REG_AVG_READOUT 0x56a1
 
+#define OV5640_LINK_FREQ_111   0
+#define OV5640_LINK_FREQ_166   1
+#define OV5640_LINK_FREQ_222   2
+#define OV5640_LINK_FREQ_333   3
+#define OV5640_LINK_FREQ_666   4
+
+static const s64 link_freq_menu_items[] = {
+   11106,
+   16660,
+   22213,
+   33220,
+   66640,
+};
+
 enum ov5640_mode_id {
OV5640_MODE_QCIF_176_144 = 0,
OV5640_MODE_QVGA_320_240,
@@ -167,12 +181,18 @@ struct ov5640_mode_info {
enum ov5640_downsize_mode dn_mode;
u32 width;
u32 height;
+   u32 pixel_rate;
+   u32 link_freq_idx;
const struct reg_value *reg_data;
u32 reg_data_size;
 };
 
 struct ov5640_ctrls {
struct v4l2_ctrl_handler handler;
+   struct {
+   struct v4l2_ctrl *link_freq;
+   struct v4l2_ctrl *pixel_rate;
+   };
struct {
struct v4l2_ctrl *auto_exp;
struct v4l2_ctrl *exposure;
@@ -732,6 +752,8 @@ static const struct ov5640_mode_info ov5640_mode_init_data 
= {
.dn_mode= SUBSAMPLING,
.width  = 640,
.height = 480,
+   .pixel_rate = 2776,
+   .link_freq_idx  = OV5640_LINK_FREQ_111,
.reg_data   = ov5640_init_setting_30fps_VGA,
.reg_data_size  = ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -744,6 +766,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = 
{
.dn_mode= SUBSAMPLING,
.width  = 176,
.height = 144,
+   .pixel_rate = 2776,
+   .link_freq_idx  = OV5640_LINK_FREQ_111,
.reg_data   = ov5640_setting_15fps_QCIF_176_144,
.reg_data_size  = ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144),
},
@@ -752,6 +776,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = 
{
.dn_mode= SUBSAMPLING,
.width  = 320,
.height = 240,
+   .pixel_rate = 2776,
+   .link_freq_idx  = OV5640_LINK_FREQ_111,
.reg_data   = ov5640_setting_15fps_QVGA_320_240,
.reg_data_size  = ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240),
},
@@ -760,6 +786,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = 
{
.dn_mode= SUBSAMPLING,
.width  = 640,
.height = 480,
+   .pixel_rate = 2776,
+   .link_freq_idx  = OV5640_LINK_FREQ_111,
.reg_data   = ov5640_setting_15fps_VGA_640_480,
.reg_data_size  = ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)
},
@@ -768,6 +796,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = 
{
.dn_mode= SUBSAMPLING,
.width  = 720,
.height = 480,
+   .pixel_rate = 2776,
+   .link_freq_idx  = OV5640_LINK_FREQ_111,
.reg_data   = ov5640_setting_15fps_NTSC_720_480,
.reg_data_size  = ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480),
},
@@ -776,6 +806,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = 
{
.dn_mode= SUBSAMPLING,
.width  = 720,
.height = 576,
+   .pixel_rate = 2776,
+   .link_freq_idx  = OV5640_LINK_FREQ_111,
.reg_data   = ov5640_setting_15fps_PAL_720_576,
.reg_data_size  = ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576),
},
@@ -784,6 +816,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = 
{
.dn_mode= SUBSAMPLING,
.width  = 1024,
.height = 768,
+   .pixel_rate = 2776,
+   .link_freq_idx  = OV5640_LINK_FREQ_111,
.reg_data   = ov5640_setting_15fps_XGA_1024_768,
.reg_data_size  = ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768),
},
@@ -792,6 +826,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = 
{
.dn_mode= SUBSAMPLING,
.width  

Re: camss: camera controls missing on vfe interfaces

[Daniel Mack]

Hi,

FWIW, we are using the attached trivial patch in our tree now. Not sure
if this is interesting for upstream, but it does solve our problem. Let
me know what you think.


Thanks,
Daniel


On Monday, November 27, 2017 12:50 AM, Daniel Mack wrote:
> Hi Todor, everyone,
> 
> On Monday, November 20, 2017 11:59 AM, Daniel Mack wrote:
>> On Monday, November 20, 2017 09:32 AM, Todor Tomov wrote:
>>> It is not a missing feature, it is more of a missing userspace 
>>> implementation.
>>> When working with a media oriented device driver, the userspace has to
>>> config the media pipeline too and if controls are exposed by the subdev 
>>> nodes,
>>> the userspace has to configure them on the subdev nodes.
>>>
>>> As there weren't a lot of media oriented drivers there is no generic
>>> implementation/support for this in the userspace (at least I'm not aware of
>>> any). There have been discussions about adding such functionality in libv4l
>>> so that applications which do not support media configuration can still
>>> use these drivers. I'm not sure if decision for this was taken or not or
>>> is it just that there was noone to actually do the work. Probably Laurent,
>>> Mauro or Hans know more about what were the plans for this.
>>
>> Hmm, that's not good.
>>
>> Considering the use-case in our application, the pipeline is set up once
>> and considered more or less static, and then applications such as the
>> Chrome browsers make use of the high-level VFE interface. If there are
>> no controls exposed on that interface, they are not available to the
>> application. Patching all userspace applications is an uphill battle
>> that can't be won I'm afraid.
>>
>> Is there any good reason not to expose the sensor controls on the VFE? I
>> guess it would be easy to do, right?
> 
> Do you see an alternative to implementing the above in order to support
> existing v4l-enabled applications?
> 
> 
> Thanks,
> Daniel
> 

>From 5142b5cdf613f6b61e9506eee81e908045b22404 Mon Sep 17 00:00:00 2001
From: Daniel Mack <dan...@zonque.org>
Date: Tue, 5 Dec 2017 11:39:42 +0100
Subject: [PATCH] camss: expose sensor controls on video node

---
 .../media/platform/qcom/camss-8x16/camss-video.c   | 32 ++
 1 file changed, 32 insertions(+)

diff --git a/drivers/media/platform/qcom/camss-8x16/camss-video.c b/drivers/media/platform/qcom/camss-8x16/camss-video.c
index 2998ad677bee..fd9403b988ec 100644
--- a/drivers/media/platform/qcom/camss-8x16/camss-video.c
+++ b/drivers/media/platform/qcom/camss-8x16/camss-video.c
@@ -640,10 +640,34 @@ static const struct v4l2_ioctl_ops msm_vid_ioctl_ops = {
  * V4L2 file operations
  */
 
+static struct media_entity *video_find_sensor(struct camss_video *video)
+{
+	struct media_pad *pad = >pad;
+
+	while (1) {
+		struct media_entity *entity;
+
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			return NULL;
+
+		pad = media_entity_remote_pad(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			return NULL;
+
+		entity = pad->entity;
+
+		if (entity->function == MEDIA_ENT_F_CAM_SENSOR)
+			return entity;
+
+		pad = >pads[0];
+	}
+}
+
 static int video_open(struct file *file)
 {
 	struct video_device *vdev = video_devdata(file);
 	struct camss_video *video = video_drvdata(file);
+	struct media_entity *sensor_entity;
 	struct v4l2_fh *vfh;
 	int ret;
 
@@ -660,6 +684,14 @@ static int video_open(struct file *file)
 
 	file->private_data = vfh;
 
+	sensor_entity = video_find_sensor(video);
+	if (sensor_entity) {
+		struct v4l2_subdev *sd =
+			media_entity_to_v4l2_subdev(sensor_entity);
+
+		vdev->ctrl_handler = sd->ctrl_handler;
+	}
+
 	ret = v4l2_pipeline_pm_use(>entity, 1);
 	if (ret < 0) {
 		dev_err(video->camss->dev, "Failed to power up pipeline: %d\n",
-- 
2.14.3

Re: camss: camera controls missing on vfe interfaces

[Daniel Mack]

Hi Todor, everyone,

On Monday, November 20, 2017 11:59 AM, Daniel Mack wrote:
> On Monday, November 20, 2017 09:32 AM, Todor Tomov wrote:
>> It is not a missing feature, it is more of a missing userspace 
>> implementation.
>> When working with a media oriented device driver, the userspace has to
>> config the media pipeline too and if controls are exposed by the subdev 
>> nodes,
>> the userspace has to configure them on the subdev nodes.
>>
>> As there weren't a lot of media oriented drivers there is no generic
>> implementation/support for this in the userspace (at least I'm not aware of
>> any). There have been discussions about adding such functionality in libv4l
>> so that applications which do not support media configuration can still
>> use these drivers. I'm not sure if decision for this was taken or not or
>> is it just that there was noone to actually do the work. Probably Laurent,
>> Mauro or Hans know more about what were the plans for this.
> 
> Hmm, that's not good.
> 
> Considering the use-case in our application, the pipeline is set up once
> and considered more or less static, and then applications such as the
> Chrome browsers make use of the high-level VFE interface. If there are
> no controls exposed on that interface, they are not available to the
> application. Patching all userspace applications is an uphill battle
> that can't be won I'm afraid.
> 
> Is there any good reason not to expose the sensor controls on the VFE? I
> guess it would be easy to do, right?

Do you see an alternative to implementing the above in order to support
existing v4l-enabled applications?


Thanks,
Daniel

Re: camss: camera controls missing on vfe interfaces

[Daniel Mack]

Hi Todor,

Thanks for following up!

On Monday, November 20, 2017 09:32 AM, Todor Tomov wrote:
> On 15.11.2017 21:31, Daniel Mack wrote:
>> Todor et all,
>>
>> Any hint on how to tackle this?
>>
>> I can contribute patches, but I'd like to understand what the idea is.
>>
>>
>> Thanks,
>> Daniel
>>
>>
>> On Thursday, October 26, 2017 06:11 PM, Daniel Mack wrote:
>>> Hi Todor,
>>>
>>> When using the camss driver trough one of its /dev/videoX device nodes,
>>> applications are currently unable to see the video controls the camera
>>> sensor exposes.
>>>
>>> Same goes for other ioctls such as VIDIOC_ENUM_FMT, so the only valid
>>> resolution setting for applications to use is the one that was
>>> previously set through the media controller layer. Applications usually
>>> query the available formats and then pick one using the standard V4L2
>>> APIs, and many can't easily be forced to use a specific one.
>>>
>>> If I'm getting this right, could you explain what's the rationale here?
>>> Is that simply a missing feature or was that approach chosen on purpose?
>>>
> 
> It is not a missing feature, it is more of a missing userspace implementation.
> When working with a media oriented device driver, the userspace has to
> config the media pipeline too and if controls are exposed by the subdev nodes,
> the userspace has to configure them on the subdev nodes.
> 
> As there weren't a lot of media oriented drivers there is no generic
> implementation/support for this in the userspace (at least I'm not aware of
> any). There have been discussions about adding such functionality in libv4l
> so that applications which do not support media configuration can still
> use these drivers. I'm not sure if decision for this was taken or not or
> is it just that there was noone to actually do the work. Probably Laurent,
> Mauro or Hans know more about what were the plans for this.

Hmm, that's not good.

Considering the use-case in our application, the pipeline is set up once
and considered more or less static, and then applications such as the
Chrome browsers make use of the high-level VFE interface. If there are
no controls exposed on that interface, they are not available to the
application. Patching all userspace applications is an uphill battle
that can't be won I'm afraid.

Is there any good reason not to expose the sensor controls on the VFE? I
guess it would be easy to do, right?


Thanks,
Daniel

Re: camss: camera controls missing on vfe interfaces

[Daniel Mack]

Todor et all,

Any hint on how to tackle this?

I can contribute patches, but I'd like to understand what the idea is.


Thanks,
Daniel


On Thursday, October 26, 2017 06:11 PM, Daniel Mack wrote:
> Hi Todor,
> 
> When using the camss driver trough one of its /dev/videoX device nodes,
> applications are currently unable to see the video controls the camera
> sensor exposes.
> 
> Same goes for other ioctls such as VIDIOC_ENUM_FMT, so the only valid
> resolution setting for applications to use is the one that was
> previously set through the media controller layer. Applications usually
> query the available formats and then pick one using the standard V4L2
> APIs, and many can't easily be forced to use a specific one.
> 
> If I'm getting this right, could you explain what's the rationale here?
> Is that simply a missing feature or was that approach chosen on purpose?
> 
> 
> Thanks,
> Daniel
> 

camss: camera controls missing on vfe interfaces

[Daniel Mack]

Hi Todor,

When using the camss driver trough one of its /dev/videoX device nodes,
applications are currently unable to see the video controls the camera
sensor exposes.

Same goes for other ioctls such as VIDIOC_ENUM_FMT, so the only valid
resolution setting for applications to use is the one that was
previously set through the media controller layer. Applications usually
query the available formats and then pick one using the standard V4L2
APIs, and many can't easily be forced to use a specific one.

If I'm getting this right, could you explain what's the rationale here?
Is that simply a missing feature or was that approach chosen on purpose?


Thanks,
Daniel

Re: [PATCH v4 04/21] doc: media/v4l-drivers: Add Qualcomm Camera Subsystem driver document

[Daniel Mack]

Hi Todor,

On Wednesday, October 25, 2017 02:07 PM, Todor Tomov wrote:
> On 16.10.2017 18:01, Daniel Mack wrote:
>> I'd be grateful for any pointer about what I could investigate on.
>>
> 
> Everything that you have described seems correct.
> 
> As you say that frames do not contain any data, do
> VFE_0_IRQ_STATUS_0_IMAGE_MASTER_n_PING_PONG
> fire at all or not?
> 
> Do you see any interrupts on the ISPIF? Which?
> 
> Could you please share what hardware setup you have - mezzanine and camera 
> module.

Thanks for your reply!

I now got it working, at least as far as camss is concerned. I can
confirm the camss driver is working for 4 lanes, and that the DTS
settings described above are correct.

Some of the problems I had were related to the sensor driver reporting
wrong clock values, which in turn caused camss to not configure its
hardware clocks correctly.

Linux userspace does not seem to be prepared for Bayer 10bit packed
formats at all however, but that's out of scope for this list I guess.



Thanks again!
Daniel

Re: [PATCH v4 04/21] doc: media/v4l-drivers: Add Qualcomm Camera Subsystem driver document

[Daniel Mack]

Hi,

On 28.08.2017 09:10, Todor Tomov wrote:
> On 25.08.2017 17:10, Daniel Mack wrote:
>> Could you explain how ISPIF, CSID and CSIPHY are related?
>>
>> I have a userspace test setup that works fine for USB webcams, but when
>> operating on any of the video devices exposed by this driver, the
>> lowlevel functions such as .s_power of the ISPIF, CSID, CSIPHY and the
>> sensor driver layers aren't called into.
> 
> Have you activated the media controller links? The s_power is called
> when the subdev is part of a pipeline in which the video device node
> is opened. You can see example configurations for the Qualcomm CAMSS
> driver on:
> https://github.com/96boards/documentation/blob/master/ConsumerEdition/DragonBoard-410c/Guides/CameraModule.md
> This will probably answer most of your questions.

It did in fact, yes. Thanks again for the pointer.

I am however struggling getting a 4-lane OV13855 camera to work with
this camss driver, and I'd be happy to hear about similar setups that work.

In short, here's what my setup looks like:

1. I wrote a driver for the OV13855 sensor, based on the one for OV13858
but with updated register values. It announces
MEDIA_BUS_FMT_SBGGR10_1X10 as bus format which is what the sensor should
be sending, if I understand the specs correctly.


2. The DTS snippet for the endpoint connection look like this:

_i2c6 {
cam0: ov13855@16 {
/* ... */
port {
cam0_ep: endpoint {
clock-lanes = <1>;
data-lanes = <0 2 3 4>;
remote-endpoint = <_ep>;
};
};
};
};

 {
ports {
port@0 {
reg = <0>;
csiphy0_ep: endpoint {
clock-lanes = <1>;
data-lanes = <0 2 3 4>;
remote-endpoint = <_ep>;
};
};
};
};

There are also no lane swaps or any intermediate components in hardware.
We've checked the electrical bits many times, and that end seems alright.


3. The pads and links are set up like this:

# media-ctl -d /dev/media0 -l
'"msm_csiphy0":1->"msm_csid0":0[1],"msm_csid0":1->"msm_ispif0":0[1],"msm_ispif0":1->"msm_vfe0_rdi0":0[1]'

# media-ctl -d /dev/media0 -V '"ov13855
1-0010":0[fmt:SBGGR10_1X10/4224x3136
field:none],"msm_csiphy0":0[fmt:SBGGR10_1X10/4224x3136
field:none],"msm_csid0":0[fmt:SBGGR10_1X10/4224x3136
field:none],"msm_ispif0":0[fmt:SBGGR10_1X10/4224x3136
field:none],"msm_vfe0_rdi0":0[fmt:SBGGR10_1X10/4224x3136 field:none]'

Both commands succeed.


4. When streaming is started, the power consumption of the device goes
up, all necessary external clocks and voltages are provided and are
stable, and I can see a continuous stream of data on all 4 MIPI lanes
using an oscilloscope.


5. Capturing frames with the following yavta command doesn't work
though. The task is mostly stuck in the buffer dequeing ioctl:

# yavta -B capture-mplane -c10 -I -n 5 -f SBGGR10P -s 4224x3136 /dev/video0

vfe_isr() does fire sometimes with VFE_0_IRQ_STATUS_1_RDIn_SOF(0) set,
but very occasionally only, and the frames do not contain data.

FWIW, an ov6540 is connected to port 1 of the camss, and this sensor
works fine.

I'd be grateful for any pointer about what I could investigate on.


Thanks,
Daniel

Re: [PATCH v4 04/21] doc: media/v4l-drivers: Add Qualcomm Camera Subsystem driver document

[Daniel Mack]

Hi Todor,

On 08/28/2017 09:10 AM, Todor Tomov wrote:
> On 25.08.2017 17:10, Daniel Mack wrote:
>> I have a userspace test setup that works fine for USB webcams, but when
>> operating on any of the video devices exposed by this driver, the
>> lowlevel functions such as .s_power of the ISPIF, CSID, CSIPHY and the
>> sensor driver layers aren't called into.
> 
> Have you activated the media controller links? The s_power is called
> when the subdev is part of a pipeline in which the video device node
> is opened. You can see example configurations for the Qualcomm CAMSS
> driver on:
> https://github.com/96boards/documentation/blob/master/ConsumerEdition/DragonBoard-410c/Guides/CameraModule.md
> This will probably answer most of your questions.

Yes, it does, thank you! I didn't expect the necessity for any manual
setup on this level due to this sentence in the documentation:

> Runtime configuration of the hardware (updating settings while
> streaming) is not required to implement the currently supported
> functionality.

I hence assumed there is a fixed mapping that is partly derived from DTS
information etc. Anyway, this seems to work now, so thanks a bunch for
the pointer!

Another thing that confused me for a while is the CCI driver, which also
exposes media pads. I have the cameras connected to a regular I2C bus
however, and it seems to work fine. That just leaves the question why
this CCI driver exists at all.

I also have some more questions, but they are even more platform
specific, so I'll rather post them in the 96boards forum.


Thanks again!
Daniel

Re: [PATCH v4 04/21] doc: media/v4l-drivers: Add Qualcomm Camera Subsystem driver document

[Daniel Mack]

Hi Todor,

Thanks a lot for working on the upstream support for this!

On 08/08/2017 03:30 PM, Todor Tomov wrote:
> +The Camera Subsystem hardware found on 8x16 processors and supported by the
> +driver consists of:
> +
> +- 2 CSIPHY modules. They handle the Physical layer of the CSI2 receivers.
> +  A separate camera sensor can be connected to each of the CSIPHY module;
> +- 2 CSID (CSI Decoder) modules. They handle the Protocol and Application 
> layer
> +  of the CSI2 receivers. A CSID can decode data stream from any of the 
> CSIPHY.
> +  Each CSID also contains a TG (Test Generator) block which can generate
> +  artificial input data for test purposes;
> +- ISPIF (ISP Interface) module. Handles the routing of the data streams from
> +  the CSIDs to the inputs of the VFE;
> +- VFE (Video Front End) module. Contains a pipeline of image processing 
> hardware
> +  blocks. The VFE has different input interfaces. The PIX input interface 
> feeds
> +  the input data to the image processing pipeline. Three RDI input interfaces
> +  bypass the image processing pipeline. The VFE also contains the AXI bus
> +  interface which writes the output data to memory.

[I'm based on the 4.9 Linaro downstream version of this code right now,
but at a glance the driver version there looks very much identical to
this one.]

Could you explain how ISPIF, CSID and CSIPHY are related?

I have a userspace test setup that works fine for USB webcams, but when
operating on any of the video devices exposed by this driver, the
lowlevel functions such as .s_power of the ISPIF, CSID, CSIPHY and the
sensor driver layers aren't called into.

The general setup seems to work fine though. The sensor is probed,
camss_subdev_notifier_complete() is called, and the v4l2 subdevices
exist. But the stream start is not propagated to the other layers, and
I'm trying to understand why.

My DTS looks something like this right now, and the hardware is an
APQ8016 board (Variscite DART SD410).

 {
cam0: ov5640@3c {
compatible = "ovti,ov5640";
reg = <0x3c>;

// clocks, regulators, gpios etc are omitted

port {
cam0_ep: endpoint {
clock-lanes = <1>;
data-lanes = <0 2>;
remote-endpoint = <_ep>;
};
};
};
};

 {
ports {
port@0 {
reg = <0>;
csiphy0_ep: endpoint {
clock-lanes = <1>;
data-lanes = <0 1 2 3>;
qcom,settle-cnt = <0xe>;
remote-endpoint = <_ep>;
};
};
};
};

Also, which video device should be opened when accessing the cameras on
each of the hardware ports? And what are the other two devices doing?

I'm sure I'm missing something trivial, but at least I can't find this
information in the documentation.


Thanks,
Daniel

Re: [RFC v2 3/3] dt: bindings: Add a binding for referencing EEPROM from camera sensors

[Daniel Mack]

On 05/29/2017 02:20 PM, Sakari Ailus wrote:
> On Mon, May 08, 2017 at 12:24:18PM -0500, Rob Herring wrote:
>> On Fri, May 05, 2017 at 11:48:30AM +0300, Sakari Ailus wrote:
>>> Many camera sensor devices contain EEPROM chips that describe the
>>> properties of a given unit --- the data is specific to a given unit can
>>> thus is not stored e.g. in user space or the driver.
>>>
>>> Some sensors embed the EEPROM chip and it can be accessed through the
>>> sensor's I2C interface. This property is to be used for devices where the
>>> EEPROM chip is accessed through a different I2C address than the sensor.
>>
>> Different I2C address or bus? We already have i2c bindings for sub 
>> devices downstream of another I2C device. Either the upstream device 
>> passes thru the I2C transactions or itself is an I2C controller with a 
>> separate downstream bus. For those cases the EEPROM should be a child 
>> node. A phandle only makes sense if you have the sensor and eeprom 
>> connected to 2 entirely separate host buses.
> 
> Right. It's a different address but located in the same package with the
> module, just like the lens. I should have actually said "module", not the
> "sensor".
> 
> The EEPROM integration to the module is done by the module vendor, but it's
> entirely possible to have another module vendor to use the sensor but not
> add an EEPROM or use a different EEPROM. It is also possible that the EEPROM
> is on the same silicon with the sensor, then it's always guaranteed to be
> there.
> 
> The bottom line is still that a sensor is simply one of the component in a
> camera module; the rest of the module (lens, eeprom etc.) is not defined by
> the sensor, the parts are rather picked by the module vendor. Yet the sensor
> is a central piece in a module: it's always guaranteed to be there or it's
> not a camera module.

I agree, yes. I think the only way to solve this is to have a generic
EEPROM API that allows the camera sensor to read data from it. If
another vendor uses a different type of EEPROM, the sensor driver would
remain the same, as it only reads data from the storage behind the
phandle, not caring about the details.

Same goes for the lens driver, and after thinking about it for awhile,
I'd say it makes most sense to allow referencing a v4l2_subdev device
through a phandle from another v4l2_subdev, and then offload certain
commands such as V4L2_CID_FOCUS_ABSOLUTE to the device that does the
actual work. Opinions?


Thanks,
Daniel

Composite MIPI camera devices

[Daniel Mack]

Hi everyone,

On an embedded board I'm working on, I need to interface a camera that
exposes three distinct I2C addresses: one for the imaging sensor
(OmniVision), one focus motor driver and one EEPROM. Usually such
cameras hide their complexity behind one common interface, but this one
just leaves that to the user.

I wonder what is the best way to support such a device under Linux. I'm
currently at the point where I have drivers for the imaging sensor and
for the focus motor, but I somehow need to link them, both in devicetree
and in the v4l userspace API. Is it possible to make a v4l2_subdev a
child of another v4l2_subdev or something? I guess an alternative would
be to offload certain functionality (V4L2_CID_FOCUS_ABSOLUTE) from the
image sensor driver to the motor driver, but I don't enough about the
v4l2 implementation details. Have similar devices been supported in the
past?

About the userspace side, is there any support for handling the focus
(setting focus to a certain point etc) in a convenient way?

I'd be grateful for any pointer.


Thank you,
Daniel

Re: [PATCH 44/50] sound: usb: caiaq: spin_lock in complete() cleanup

[Daniel Mack]

On 11.07.2013 11:06, Ming Lei wrote:
 Complete() will be run with interrupt enabled, so change to
 spin_lock_irqsave().
 
 Cc: Daniel Mack zon...@gmail.com
 Cc: Jaroslav Kysela pe...@perex.cz
 Cc: Takashi Iwai ti...@suse.de
 Cc: alsa-de...@alsa-project.org
 Signed-off-by: Ming Lei ming@canonical.com

Sound right to me, thanks.

Acked-by: Daniel Mack zon...@gmail.com



 ---
  sound/usb/caiaq/audio.c |5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
 
 diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
 index 7103b09..e5675ab 100644
 --- a/sound/usb/caiaq/audio.c
 +++ b/sound/usb/caiaq/audio.c
 @@ -672,10 +672,11 @@ static void read_completed(struct urb *urb)
   offset += len;
  
   if (len  0) {
 - spin_lock(cdev-spinlock);
 + unsigned long flags;
 + spin_lock_irqsave(cdev-spinlock, flags);
   fill_out_urb(cdev, out, out-iso_frame_desc[outframe]);
   read_in_urb(cdev, urb, urb-iso_frame_desc[frame]);
 - spin_unlock(cdev-spinlock);
 + spin_unlock_irqrestore(cdev-spinlock, flags);
   check_for_elapsed_periods(cdev, cdev-sub_playback);
   check_for_elapsed_periods(cdev, cdev-sub_capture);
   send_it = 1;
 

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

On 03.11.2012 15:10, Christof Meerwald wrote:
 On Sat, 20 Oct 2012 23:15:17 + (GMT), Artem S. Tashkinov wrote:
 It's almost definitely either a USB driver bug or video4linux driver bug:

 I'm CC'ing linux-media and linux-usb mailing lists, the problem is described 
 here:
 https://lkml.org/lkml/2012/10/20/35
 https://lkml.org/lkml/2012/10/20/148
 
 Not sure if it's related, but I am seeing a kernel freeze with a
 usb-audio headset (connected via an external USB hub) on Linux 3.5.0
 (Ubuntu 12.10) - see

Does Ubuntu 12.10 really ship with 3.5.0? Not any more recent

 http://comments.gmane.org/gmane.comp.voip.twinkle/3052 and
 http://pastebin.com/aHGe1S1X for a self-contained C test.

Some questions:

 - Are you seeing the same issue with 3.6.x?
 - If you can reproduce this issue, could you paste the messages in
dmesg when this happens? Do they resemble to the list corruption that
was reported?
 - Do you see the same problem with 3.4?
 - Are you able to apply the patch Alan Stern posted in this thread earlier?

We should really sort this out, but I unfortunately lack a system or
setup that shows the bug.


Thanks,
Daniel

--
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: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

On 22.10.2012 17:17, Alan Stern wrote:
 On Sun, 21 Oct 2012, Artem S. Tashkinov wrote:
 
 dmesg messages up to a crash can be seen here: 
 https://bugzilla.kernel.org/attachment.cgi?id=84221
 
 The first problem in the log is endpoint list corruption.  Here's a 
 debugging patch which should provide a little more information.

Maybe add a BUG() after each of these dev_err() so we stop at the first
occurance and also see where we're coming from?




  drivers/usb/core/hcd.c |   36 
  1 file changed, 36 insertions(+)
 
 Index: usb-3.6/drivers/usb/core/hcd.c
 ===
 --- usb-3.6.orig/drivers/usb/core/hcd.c
 +++ usb-3.6/drivers/usb/core/hcd.c
 @@ -1083,6 +1083,8 @@ EXPORT_SYMBOL_GPL(usb_calc_bus_time);
  
  /*-*/
  
 +static bool list_error;
 +
  /**
   * usb_hcd_link_urb_to_ep - add an URB to its endpoint queue
   * @hcd: host controller to which @urb was submitted
 @@ -1126,6 +1128,20 @@ int usb_hcd_link_urb_to_ep(struct usb_hc
*/
   if (HCD_RH_RUNNING(hcd)) {
   urb-unlinked = 0;
 +
 + {
 + struct list_head *cur = urb-ep-urb_list;
 + struct list_head *prev = cur-prev;
 +
 + if (prev-next != cur  !list_error) {
 + list_error = true;
 + dev_err(urb-dev-dev,
 + ep %x list add corruption: %p %p %p\n,
 + urb-ep-desc.bEndpointAddress,
 + cur, prev, prev-next);
 + }
 + }
 +
   list_add_tail(urb-urb_list, urb-ep-urb_list);
   } else {
   rc = -ESHUTDOWN;
 @@ -1193,6 +1209,26 @@ void usb_hcd_unlink_urb_from_ep(struct u
  {
   /* clear all state linking urb to this dev (and hcd) */
   spin_lock(hcd_urb_list_lock);
 + {
 + struct list_head *cur = urb-urb_list;
 + struct list_head *prev = cur-prev;
 + struct list_head *next = cur-next;
 +
 + if (prev-next != cur  !list_error) {
 + list_error = true;
 + dev_err(urb-dev-dev,
 + ep %x list del corruption prev: %p %p %p\n,
 + urb-ep-desc.bEndpointAddress,
 + cur, prev, prev-next);
 + }
 + if (next-prev != cur  !list_error) {
 + list_error = true;
 + dev_err(urb-dev-dev,
 + ep %x list del corruption next: %p %p %p\n,
 + urb-ep-desc.bEndpointAddress,
 + cur, next, next-prev);
 + }
 + }
   list_del_init(urb-urb_list);
   spin_unlock(hcd_urb_list_lock);
  }
 

--
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: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

On 21.10.2012 01:15, Artem S. Tashkinov wrote:
 You don't get me - I have *no* VirtualBox (or any proprietary) modules running
 - but I can reproduce this problem using *the same system running under* 
 VirtualBox
 in Windows 7 64.
 
 It's almost definitely either a USB driver bug or video4linux driver bug:
 
 I'm CC'ing linux-media and linux-usb mailing lists, the problem is described 
 here:
 https://lkml.org/lkml/2012/10/20/35
 https://lkml.org/lkml/2012/10/20/148
 
 Here are  the last lines from my dmesg (with usbmon loaded):
 
 [  292.164833] hub 1-0:1.0: state 7 ports 8 chg  evt 0002
 [  292.168091] ehci_hcd :00:1f.5: GetStatus port:1 status 00100a 0  ACK 
 POWER sig=se0 PEC CSC
 [  292.172063] hub 1-0:1.0: port 1, status 0100, change 0003, 12 Mb/s
 [  292.174883] usb 1-1: USB disconnect, device number 2
 [  292.178045] usb 1-1: unregistering device
 [  292.183539] usb 1-1: unregistering interface 1-1:1.0
 [  292.197034] usb 1-1: unregistering interface 1-1:1.1
 [  292.204317] usb 1-1: unregistering interface 1-1:1.2
 [  292.234519] usb 1-1: unregistering interface 1-1:1.3
 [  292.236175] usb 1-1: usb_disable_device nuking all URBs
 [  292.364429] hub 1-0:1.0: debounce: port 1: total 100ms stable 100ms status 
 0x100
 [  294.364279] hub 1-0:1.0: hub_suspend
 [  294.366045] usb usb1: bus auto-suspend, wakeup 1
 [  294.367375] ehci_hcd :00:1f.5: suspend root hub
 [  296.501084] usb usb1: usb wakeup-resume
 [  296.508311] usb usb1: usb auto-resume
 [  296.509833] ehci_hcd :00:1f.5: resume root hub
 [  296.560149] hub 1-0:1.0: hub_resume
 [  296.562240] ehci_hcd :00:1f.5: GetStatus port:1 status 001003 0  ACK 
 POWER sig=se0 CSC CONNECT
 [  296.566141] hub 1-0:1.0: port 1: status 0501 change 0001
 [  296.670413] hub 1-0:1.0: state 7 ports 8 chg 0002 evt 
 [  296.673222] hub 1-0:1.0: port 1, status 0501, change , 480 Mb/s
 [  297.311720] usb 1-1: new high-speed USB device number 3 using ehci_hcd
 [  300.547237] usb 1-1: skipped 1 descriptor after configuration
 [  300.549443] usb 1-1: skipped 4 descriptors after interface
 [  300.552273] usb 1-1: skipped 2 descriptors after interface
 [  300.556499] usb 1-1: skipped 1 descriptor after endpoint
 [  300.559392] usb 1-1: skipped 2 descriptors after interface
 [  300.560960] usb 1-1: skipped 1 descriptor after endpoint
 [  300.562169] usb 1-1: skipped 2 descriptors after interface
 [  300.563440] usb 1-1: skipped 1 descriptor after endpoint
 [  300.564639] usb 1-1: skipped 2 descriptors after interface
 [  300.565828] usb 1-1: skipped 2 descriptors after endpoint
 [  300.567084] usb 1-1: skipped 9 descriptors after interface
 [  300.569205] usb 1-1: skipped 1 descriptor after endpoint
 [  300.570484] usb 1-1: skipped 53 descriptors after interface
 [  300.595843] usb 1-1: default language 0x0409
 [  300.602503] usb 1-1: USB interface quirks for this device: 2
 [  300.605700] usb 1-1: udev 3, busnum 1, minor = 2
 [  300.606959] usb 1-1: New USB device found, idVendor=046d, idProduct=081d
 [  300.610298] usb 1-1: New USB device strings: Mfr=0, Product=0, 
 SerialNumber=1
 [  300.613742] usb 1-1: SerialNumber: 48C5D2B0
 [  300.617703] usb 1-1: usb_probe_device
 [  300.620594] usb 1-1: configuration #1 chosen from 1 choice
 [  300.639218] usb 1-1: adding 1-1:1.0 (config #1, interface 0)
 [  300.640736] snd-usb-audio 1-1:1.0: usb_probe_interface
 [  300.642307] snd-usb-audio 1-1:1.0: usb_probe_interface - got id
 [  301.050296] usb 1-1: adding 1-1:1.1 (config #1, interface 1)
 [  301.054897] usb 1-1: adding 1-1:1.2 (config #1, interface 2)
 [  301.056934] uvcvideo 1-1:1.2: usb_probe_interface
 [  301.058072] uvcvideo 1-1:1.2: usb_probe_interface - got id
 [  301.059395] uvcvideo: Found UVC 1.00 device unnamed (046d:081d)
 [  301.090173] input: UVC Camera (046d:081d) as 
 /devices/pci:00/:00:1f.5/usb1/1-1/1-1:1.2/input/input7

That seems to be a Logitech model.

 [  301.111289] usb 1-1: adding 1-1:1.3 (config #1, interface 3)
 [  301.131207] usb 1-1: link qh16-0001/f48d64c0 start 2 [1/0 us]
 [  301.137066] usb 1-1: unlink qh16-0001/f48d64c0 start 2 [1/0 us]
 [  301.156451] ehci_hcd :00:1f.5: reused qh f48d64c0 schedule
 [  301.158310] usb 1-1: link qh16-0001/f48d64c0 start 2 [1/0 us]
 [  301.160238] usb 1-1: unlink qh16-0001/f48d64c0 start 2 [1/0 us]
 [  301.196606] set resolution quirk: cval-res = 384
 [  371.309569] e1000: eth1 NIC Link is Up 1000 Mbps Full Duplex, Flow 
 Control: RX
 [  390.729568] ehci_hcd :00:1f.5: reused qh f48d64c0 schedule
 f5ade900 2296555[  390.730023] usb 1-1: link qh16-0001/f48d64c0 start 2 [1/0 
 us]
 437 S Ii:1:003:7[  390.736394] usb 1-1: unlink qh16-0001/f48d64c0 start 2 
 [1/0 us]
  -115:128 16 
 f5ade900 2296566256 C Ii:1:003:7 -2:128 0
 [  391.100896] ehci_hcd :00:1f.5: reused qh f48d64c0 schedule
 [  391.103188] usb 1-1: link qh16-0001/f48d64c0 start 2 [1/0 us]
 f5ade900 2296926929 S Ii:1:003:7[  391.104889] usb 1-1: unlink 
 qh16-0001/f48d64c0 start 2 [1/0 us]
  -115:128 16 
 

Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

On 21.10.2012 12:34, Daniel Mack wrote:
 On 21.10.2012 01:15, Artem S. Tashkinov wrote:
 You don't get me - I have *no* VirtualBox (or any proprietary) modules 
 running
 - but I can reproduce this problem using *the same system running under* 
 VirtualBox
 in Windows 7 64.

 It's almost definitely either a USB driver bug or video4linux driver bug:

 I'm CC'ing linux-media and linux-usb mailing lists, the problem is described 
 here:
 https://lkml.org/lkml/2012/10/20/35
 https://lkml.org/lkml/2012/10/20/148

 Here are  the last lines from my dmesg (with usbmon loaded):

 [  292.164833] hub 1-0:1.0: state 7 ports 8 chg  evt 0002
 [  292.168091] ehci_hcd :00:1f.5: GetStatus port:1 status 00100a 0  ACK 
 POWER sig=se0 PEC CSC
 [  292.172063] hub 1-0:1.0: port 1, status 0100, change 0003, 12 Mb/s
 [  292.174883] usb 1-1: USB disconnect, device number 2
 [  292.178045] usb 1-1: unregistering device
 [  292.183539] usb 1-1: unregistering interface 1-1:1.0
 [  292.197034] usb 1-1: unregistering interface 1-1:1.1
 [  292.204317] usb 1-1: unregistering interface 1-1:1.2
 [  292.234519] usb 1-1: unregistering interface 1-1:1.3
 [  292.236175] usb 1-1: usb_disable_device nuking all URBs
 [  292.364429] hub 1-0:1.0: debounce: port 1: total 100ms stable 100ms 
 status 0x100
 [  294.364279] hub 1-0:1.0: hub_suspend
 [  294.366045] usb usb1: bus auto-suspend, wakeup 1
 [  294.367375] ehci_hcd :00:1f.5: suspend root hub
 [  296.501084] usb usb1: usb wakeup-resume
 [  296.508311] usb usb1: usb auto-resume
 [  296.509833] ehci_hcd :00:1f.5: resume root hub
 [  296.560149] hub 1-0:1.0: hub_resume
 [  296.562240] ehci_hcd :00:1f.5: GetStatus port:1 status 001003 0  ACK 
 POWER sig=se0 CSC CONNECT
 [  296.566141] hub 1-0:1.0: port 1: status 0501 change 0001
 [  296.670413] hub 1-0:1.0: state 7 ports 8 chg 0002 evt 
 [  296.673222] hub 1-0:1.0: port 1, status 0501, change , 480 Mb/s
 [  297.311720] usb 1-1: new high-speed USB device number 3 using ehci_hcd
 [  300.547237] usb 1-1: skipped 1 descriptor after configuration
 [  300.549443] usb 1-1: skipped 4 descriptors after interface
 [  300.552273] usb 1-1: skipped 2 descriptors after interface
 [  300.556499] usb 1-1: skipped 1 descriptor after endpoint
 [  300.559392] usb 1-1: skipped 2 descriptors after interface
 [  300.560960] usb 1-1: skipped 1 descriptor after endpoint
 [  300.562169] usb 1-1: skipped 2 descriptors after interface
 [  300.563440] usb 1-1: skipped 1 descriptor after endpoint
 [  300.564639] usb 1-1: skipped 2 descriptors after interface
 [  300.565828] usb 1-1: skipped 2 descriptors after endpoint
 [  300.567084] usb 1-1: skipped 9 descriptors after interface
 [  300.569205] usb 1-1: skipped 1 descriptor after endpoint
 [  300.570484] usb 1-1: skipped 53 descriptors after interface
 [  300.595843] usb 1-1: default language 0x0409
 [  300.602503] usb 1-1: USB interface quirks for this device: 2
 [  300.605700] usb 1-1: udev 3, busnum 1, minor = 2
 [  300.606959] usb 1-1: New USB device found, idVendor=046d, idProduct=081d
 [  300.610298] usb 1-1: New USB device strings: Mfr=0, Product=0, 
 SerialNumber=1
 [  300.613742] usb 1-1: SerialNumber: 48C5D2B0
 [  300.617703] usb 1-1: usb_probe_device
 [  300.620594] usb 1-1: configuration #1 chosen from 1 choice
 [  300.639218] usb 1-1: adding 1-1:1.0 (config #1, interface 0)
 [  300.640736] snd-usb-audio 1-1:1.0: usb_probe_interface
 [  300.642307] snd-usb-audio 1-1:1.0: usb_probe_interface - got id
 [  301.050296] usb 1-1: adding 1-1:1.1 (config #1, interface 1)
 [  301.054897] usb 1-1: adding 1-1:1.2 (config #1, interface 2)
 [  301.056934] uvcvideo 1-1:1.2: usb_probe_interface
 [  301.058072] uvcvideo 1-1:1.2: usb_probe_interface - got id
 [  301.059395] uvcvideo: Found UVC 1.00 device unnamed (046d:081d)
 [  301.090173] input: UVC Camera (046d:081d) as 
 /devices/pci:00/:00:1f.5/usb1/1-1/1-1:1.2/input/input7
 
 That seems to be a Logitech model.
 
 [  301.111289] usb 1-1: adding 1-1:1.3 (config #1, interface 3)
 [  301.131207] usb 1-1: link qh16-0001/f48d64c0 start 2 [1/0 us]
 [  301.137066] usb 1-1: unlink qh16-0001/f48d64c0 start 2 [1/0 us]
 [  301.156451] ehci_hcd :00:1f.5: reused qh f48d64c0 schedule
 [  301.158310] usb 1-1: link qh16-0001/f48d64c0 start 2 [1/0 us]
 [  301.160238] usb 1-1: unlink qh16-0001/f48d64c0 start 2 [1/0 us]
 [  301.196606] set resolution quirk: cval-res = 384
 [  371.309569] e1000: eth1 NIC Link is Up 1000 Mbps Full Duplex, Flow 
 Control: RX
 [  390.729568] ehci_hcd :00:1f.5: reused qh f48d64c0 schedule
 f5ade900 2296555[  390.730023] usb 1-1: link qh16-0001/f48d64c0 start 2 [1/0 
 us]
 437 S Ii:1:003:7[  390.736394] usb 1-1: unlink qh16-0001/f48d64c0 start 2 
 [1/0 us]
  -115:128 16 
 f5ade900 2296566256 C Ii:1:003:7 -2:128 0
 [  391.100896] ehci_hcd :00:1f.5: reused qh f48d64c0 schedule
 [  391.103188] usb 1-1: link qh16-0001/f48d64c0 start 2 [1/0 us]
 f5ade900 2296926929 S Ii:1:003:7[  391.104889] usb 1-1: unlink 
 qh16-0001

Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

On 21.10.2012 13:59, Artem S. Tashkinov wrote:
 On Oct 21, 2012, Borislav Petkov wrote:

 On Sun, Oct 21, 2012 at 01:57:21AM +, Artem S. Tashkinov wrote:
 The freeze happens on my *host* Linux PC. For an experiment I decided
 to check if I could reproduce the freeze under a virtual machine - it
 turns out the Linux kernel running under it also freezes.

 I know that - but a freeze != oops - at least not necessarily. Which
 means it could very well be a different issue now that vbox is gone.

 Or, it could be the same issue with different incarnations: with vbox
 you get the corruptions and without it, you get the freezes. I'm
 assuming you do the same flash player thing in both cases?

 Here's a crazy idea: can you try to reproduce it in KVM?
 
 OK, dismiss VBox altogether - it has a very buggy USB implementation, thus
 it just hangs when trying to access my webcam.

Ok.

 What I've found out is that my system crashes *only* when I try to enable
 usb-audio (from the same webcam) - I still have no idea how to capture a
 panic message, but I ran
 
 while :; do dmesg -c; done in xterm, then I got like thousands of messages
 and I photographed my monitor:
 
 http://imageshack.us/a/img685/9452/panicz.jpg

A hint at least. How did you enable the audio record exactly? Can you
reproduce this with arecord?

What chipset are you on? Please provide both lspci -v and lsusb -v
dumps. As I said, I fail to reproduce that issue on any of my machines.


Daniel

--
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: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

On 21.10.2012 13:59, Artem S. Tashkinov wrote:
 On Oct 21, 2012, Borislav Petkov wrote:

 On Sun, Oct 21, 2012 at 01:57:21AM +, Artem S. Tashkinov wrote:
 The freeze happens on my *host* Linux PC. For an experiment I decided
 to check if I could reproduce the freeze under a virtual machine - it
 turns out the Linux kernel running under it also freezes.

 I know that - but a freeze != oops - at least not necessarily. Which
 means it could very well be a different issue now that vbox is gone.

 Or, it could be the same issue with different incarnations: with vbox
 you get the corruptions and without it, you get the freezes. I'm
 assuming you do the same flash player thing in both cases?

 Here's a crazy idea: can you try to reproduce it in KVM?
 
 OK, dismiss VBox altogether - it has a very buggy USB implementation, thus
 it just hangs when trying to access my webcam.
 
 What I've found out is that my system crashes *only* when I try to enable
 usb-audio (from the same webcam)

It would also be interesting to know whether you have problems with
*only* the video capture, with some tool like cheese. It might be
you're hitting a host controller issue here, and then isochronous input
packets on the video interface would most likely also trigger such am
effect. Actually, knowing whether that's the case would be crucial for
further debugging.


Daniel

--
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: was: Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

[Cc: alsa-devel]

On 21.10.2012 14:30, Artem S. Tashkinov wrote:
 On Oct 21, 2012, Daniel Mack wrote: 
 
 A hint at least. How did you enable the audio record exactly? Can you
 reproduce this with arecord?

 What chipset are you on? Please provide both lspci -v and lsusb -v
 dumps. As I said, I fail to reproduce that issue on any of my machines.
 
 All other applications can read from the USB audio without problems, it's
 just something in the way Adobe Flash polls my audio input which causes
 a crash.
 
 Just video capture (without audio) works just fine in Adobe Flash.

Ok, so that pretty much rules out the host controller. I just wonder why
I still don't see it here, and I haven't heard of any such problem from
anyone else.

Some more questions:

- Which version of Flash are you running?
- Does this also happen with Firefox?
- Does flash access the device directly or via PulseAudio?
- Could you please apply the attached patch and see what it spits out to
dmesg once Flash opens the device? It returns -EINVAL in the hw_params
callback to prevent the actual streaming. On my machine with Flash
11.4.31.110, I get values of 2/44800/1/32768/2048/0, which seems sane.
Or does your machine still crash before anything is written to the logs?

 Only and only when I choose to use 
 
 USB Device 0x46d:0x81d my system crashes in Adobe Flash.

 See the screenshot:
 
 https://bugzilla.kernel.org/attachment.cgi?id=84151

When exactly does the crash happen? Right after you selected that entry
from the list? There's a little recording level meter in that dialog.
Does that show any input from the microphone?

 My hardware information can be fetched from here:
 
 https://bugzilla.kernel.org/show_bug.cgi?id=49181
 
 On a second thought that can be even an ALSA crash or pretty much
 anything else.

We'll see. Thanks for your help to sort this out!


Daniel



diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index f782ce1..5664b45 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -453,6 +453,18 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
 	unsigned int channels, rate, format;
 	int ret, changed;
 
+
+	printk( %s()\n, __func__);
+
+	printk(format: %d\n, params_format(hw_params));
+	printk(rate: %d\n, params_rate(hw_params));
+	printk(channels: %d\n, params_channels(hw_params));
+	printk(buffer bytes: %d\n, params_buffer_bytes(hw_params));
+	printk(period bytes: %d\n, params_period_bytes(hw_params));
+	printk(access: %d\n, params_access(hw_params));
+
+	return -EINVAL;
+
 	ret = snd_pcm_lib_alloc_vmalloc_buffer(substream,
 	   params_buffer_bytes(hw_params));
 	if (ret  0)

Re: was: Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

On 21.10.2012 16:57, Artem S. Tashkinov wrote:
 On Oct 21, 2012, Daniel Mack  wrote: 

 [Cc: alsa-devel]

 On 21.10.2012 14:30, Artem S. Tashkinov wrote:
 On Oct 21, 2012, Daniel Mack wrote: 

 A hint at least. How did you enable the audio record exactly? Can you
 reproduce this with arecord?

 What chipset are you on? Please provide both lspci -v and lsusb -v
 dumps. As I said, I fail to reproduce that issue on any of my machines.

 All other applications can read from the USB audio without problems, it's
 just something in the way Adobe Flash polls my audio input which causes
 a crash.

 Just video capture (without audio) works just fine in Adobe Flash.

 Ok, so that pretty much rules out the host controller. I just wonder why
 I still don't see it here, and I haven't heard of any such problem from
 anyone else.

 Some more questions:

 - Which version of Flash are you running?
 
 Google Chrome has its own version of Adobe Flash:
 
 Name: Shockwave Flash
 Description:  Shockwave Flash 11.4 r31
 Version:  11.4.31.110

So that's the same that I'm using.

 - Does this also happen with Firefox?
 
 No, Adobe Flash in Firefox is an older version (Shockwave Flash 11.1 r102), 
 it shows
 just two input devices instead of three which the newer Flash players sees.
 
 * HDA Intel PCH
 * USB Device 0x46d:0x81d

And that works, I assume? Does the second choice in the newer Flash
version work maybe?

 - Does flash access the device directly or via PulseAudio?
 
 PA is not installed on my computer, so Flash accesses it directly via ALSA 
 calls.

Ok, Same here.

 - Could you please apply the attached patch and see what it spits out to
 dmesg once Flash opens the device? It returns -EINVAL in the hw_params
 callback to prevent the actual streaming. On my machine with Flash
 11.4.31.110, I get values of 2/44800/1/32768/2048/0, which seems sane.
 Or does your machine still crash before anything is written to the logs?
 
 I will try it a bit later.

Yes, we need to trace the call chain and see at which point the trouble
starts. What could help is tracing the google-chrome binary with strace
maybe. At least we would see the ioctl command sequence, if the log file
survives the crash.

As the usb list is still in Cc: - Artem's lcpci dump shows that his
machine features XHCI controllers. Can anyone think of a relation to
this problem?

And Artem, is there any way you boot your system on an older machine
that only has EHCI ports? Thinking about it, I wonder whether the freeze
in VBox and the crashes on native hardware have the same root cause. In
that case, would it be possible to share that VBox image?


Daniel

--
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: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

On 21.10.2012 21:49, Artem S. Tashkinov wrote:

 On Oct 21, 2012, Borislav Petkov b...@alien8.de wrote: 

 On Sun, Oct 21, 2012 at 11:59:36AM +, Artem S. Tashkinov wrote:
 http://imageshack.us/a/img685/9452/panicz.jpg

 list_del corruption. prev-next should be ... but was ...

 Btw, this is one of the debug options I told you to enable.

 I cannot show you more as I have no serial console to use :( and the kernel
 doesn't have enough time to push error messages to rsyslog and fsync
 /var/log/messages

 I already told you how to catch that oops: boot with pause_on_oops=600
 on the kernel command line and photograph the screen when the first oops
 happens. This'll show us where the problem begins.
 
 This option didn't have any effect, or maybe it's because it's such a serious 
 crash
 the kernel has no time to actually print an ooops/panic message.
 
 dmesg messages up to a crash can be seen here: 
 https://bugzilla.kernel.org/attachment.cgi?id=84221

Nice. Could you do that again with the patch applied I sent yo some
hours ago?


Thanks,
Daniel

--
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: A reliable kernel panic (3.6.2) and system crash when visiting a particular website

[Daniel Mack]

On 21.10.2012 22:43, Artem S. Tashkinov wrote:
 Nice. Could you do that again with the patch applied I sent yo some
 hours ago?
 
 That patch was of no help - the system has crashed and I couldn't spot 
 relevant
 messages.
 
 I've no idea what it means.

The sequence of driver callbacks issued on a stream start is

 .open()
 .hw_params()
 .prepare()
 .trigger()

If the ALSA part really causes this issue, the bad things happen either
in any of the driver callback functions or in the core underneath.

The patch I sent returns an error from the hw_params callback, and as
you still see the problem, that means that the crash happens before any
of the USB audio streaming really starts.

Could you try and return -EINVAL from snd_usb_capture_open() please?

If anyone has a better idea on how to debug this, please chime in.


Daniel

--
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] drivers/media/dvb/dvb-usb/dib0700: fix return values

[Daniel Mack]

Propagte correct error values instead of returning -1 which just means
-EPERM (Permission denied)

Signed-off-by: Daniel Mack dan...@caiaq.de
Cc: Wolfram Sang w.s...@pengutronix.de
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Jiri Slaby jsl...@suse.cz
Cc: Dmitry Torokhov d...@mail.ru
Cc: Devin Heitmueller dheitmuel...@kernellabs.com
Cc: linux-media@vger.kernel.org
---
 drivers/media/dvb/dvb-usb/dib0700_core.c |   16 +++-
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c 
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 4f961d2..d2dabac 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -640,10 +640,10 @@ int dib0700_rc_setup(struct dvb_usb_device *d)
return 0;
 
/* Set the IR mode */
-   i = dib0700_ctrl_wr(d, rc_setup, 3);
-   if (i0) {
+   i = dib0700_ctrl_wr(d, rc_setup, sizeof(rc_setup));
+   if (i  0) {
err(ir protocol setup failed);
-   return -1;
+   return i;
}
 
if (st-fw_version  0x10200)
@@ -653,14 +653,14 @@ int dib0700_rc_setup(struct dvb_usb_device *d)
purb = usb_alloc_urb(0, GFP_KERNEL);
if (purb == NULL) {
err(rc usb alloc urb failed\n);
-   return -1;
+   return -ENOMEM;
}
 
purb-transfer_buffer = kzalloc(RC_MSG_SIZE_V1_20, GFP_KERNEL);
if (purb-transfer_buffer == NULL) {
err(rc kzalloc failed\n);
usb_free_urb(purb);
-   return -1;
+   return -ENOMEM;
}
 
purb-status = -EINPROGRESS;
@@ -669,12 +669,10 @@ int dib0700_rc_setup(struct dvb_usb_device *d)
  dib0700_rc_urb_completion, d);
 
ret = usb_submit_urb(purb, GFP_ATOMIC);
-   if (ret != 0) {
+   if (ret)
err(rc submit urb failed\n);
-   return -1;
-   }
 
-   return 0;
+   return ret;
 }
 
 static int dib0700_probe(struct usb_interface *intf,
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] drivers/media/dvb/dvb-usb/dib0700: CodingStyle fixes

[Daniel Mack]

Signed-off-by: Daniel Mack dan...@caiaq.de
Cc: Wolfram Sang w.s...@pengutronix.de
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Jiri Slaby jsl...@suse.cz
Cc: Dmitry Torokhov d...@mail.ru
Cc: Devin Heitmueller dheitmuel...@kernellabs.com
Cc: linux-media@vger.kernel.org
---
 drivers/media/dvb/dvb-usb/dib0700_core.c |   66 --
 1 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c 
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index d2dabac..7deade7 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -53,7 +53,7 @@ static int dib0700_ctrl_wr(struct dvb_usb_device *d, u8 *tx, 
u8 txlen)
int status;
 
deb_data( );
-   debug_dump(tx,txlen,deb_data);
+   debug_dump(tx, txlen, deb_data);
 
status = usb_control_msg(d-udev, usb_sndctrlpipe(d-udev,0),
tx[0], USB_TYPE_VENDOR | USB_DIR_OUT, 0, 0, tx, txlen,
@@ -98,7 +98,7 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 
txlen, u8 *rx, u8 rxlen
deb_info(ep 0 read error (status = %d)\n,status);
 
deb_data( );
-   debug_dump(rx,rxlen,deb_data);
+   debug_dump(rx, rxlen, deb_data);
 
return status; /* length in case of success */
 }
@@ -106,28 +106,29 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 
txlen, u8 *rx, u8 rxlen
 int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8 
gpio_dir, u8 gpio_val)
 {
u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir  0x01)  7) | 
((gpio_val  0x01)  6) };
-   return dib0700_ctrl_wr(d,buf,3);
+   return dib0700_ctrl_wr(d, buf, sizeof(buf));
 }
 
 static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 
nb_ts_packets)
 {
-struct dib0700_state *st = d-priv;
-u8 b[3];
-int ret;
-
-if (st-fw_version = 0x10201) {
-   b[0] = REQUEST_SET_USB_XFER_LEN;
-   b[1] = (nb_ts_packets  8)0xff;
-   b[2] = nb_ts_packets  0xff;
-
-   deb_info(set the USB xfer len to %i Ts packet\n, nb_ts_packets);
-
-   ret = dib0700_ctrl_wr(d, b, 3);
-} else {
-   deb_info(this firmware does not allow to change the USB xfer len\n);
-   ret = -EIO;
-}
-return ret;
+   struct dib0700_state *st = d-priv;
+   u8 b[3];
+   int ret;
+
+   if (st-fw_version = 0x10201) {
+   b[0] = REQUEST_SET_USB_XFER_LEN;
+   b[1] = (nb_ts_packets  8)  0xff;
+   b[2] = nb_ts_packets  0xff;
+
+   deb_info(set the USB xfer len to %i Ts packet\n, 
nb_ts_packets);
+
+   ret = dib0700_ctrl_wr(d, b, sizeof(b));
+   } else {
+   deb_info(this firmware does not allow to change the USB xfer 
len\n);
+   ret = -EIO;
+   }
+
+   return ret;
 }
 
 /*
@@ -178,7 +179,8 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, 
struct i2c_msg *msg,
value = ((en_start  7) | (en_stop  6) |
 (msg[i].len  0x3F))  8 | i2c_dest;
/* I2C ctrl + FE bus; */
-   index = ((gen_mode6)0xC0) | ((bus_mode4)0x30);
+   index = ((gen_mode  6)  0xC0) |
+   ((bus_mode  4)  0x30);
 
result = usb_control_msg(d-udev,
 usb_rcvctrlpipe(d-udev, 0),
@@ -198,11 +200,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, 
struct i2c_msg *msg,
} else {
/* Write request */
buf[0] = REQUEST_NEW_I2C_WRITE;
-   buf[1] = (msg[i].addr  1);
+   buf[1] = msg[i].addr  1;
buf[2] = (en_start  7) | (en_stop  6) |
(msg[i].len  0x3F);
/* I2C ctrl + FE bus; */
-   buf[3] = ((gen_mode6)0xC0) | ((bus_mode4)0x30);
+   buf[3] = ((gen_mode  6)  0xC0) |
+((bus_mode  4)  0x30);
/* The Actual i2c payload */
memcpy(buf[4], msg[i].buf, msg[i].len);
 
@@ -240,7 +243,7 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 
for (i = 0; i  num; i++) {
/* fill in the address */
-   buf[1] = (msg[i].addr  1);
+   buf[1] = msg[i].addr  1;
/* fill the buffer */
memcpy(buf[2], msg[i].buf, msg[i].len);
 
@@ -368,7 +371,8 @@ int dib0700_download_firmware(struct usb_device *udev, 
const struct firmware *fw
u8 buf[260];
 
while ((ret = dvb_usb_get_hexline(fw, hx, pos))  0) {
-   deb_fwdata(writing to address 0x%08x (buffer: 0x%02x 
%02x)\n,hx.addr, hx.len, hx.chk);
+   deb_fwdata(writing to address 0x%08x (buffer: 0x%02x %02x)\n,
+   hx.addr

Re: [PATCH] drivers/media/dvb/dvb-usb/dib0700: CodingStyle fixes

[Daniel Mack]

On Mon, May 24, 2010 at 08:56:24AM -0700, Dmitry Torokhov wrote:
 On Mon, May 24, 2010 at 05:23:05PM +0200, Daniel Mack wrote:
  Signed-off-by: Daniel Mack dan...@caiaq.de
  Cc: Wolfram Sang w.s...@pengutronix.de
  Cc: Mauro Carvalho Chehab mche...@infradead.org
  Cc: Jiri Slaby jsl...@suse.cz
  Cc: Dmitry Torokhov d...@mail.ru
  Cc: Devin Heitmueller dheitmuel...@kernellabs.com
  Cc: linux-media@vger.kernel.org
 
 Not sure how I got on the list but chnages look good to me.

get_maintainer.pl :)

Daniel
--
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] drivers/media/dvb/dvb-usb/dib0700: fix return values

[Daniel Mack]

Propagte correct error values instead of returning -1 which just means
-EPERM (Permission denied)

While at it, also fix some coding style violations.

Signed-off-by: Daniel Mack dan...@caiaq.de
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Jiri Slaby jsl...@suse.cz
Cc: Dmitry Torokhov d...@mail.ru
Cc: Devin Heitmueller dheitmuel...@kernellabs.com
Cc: linux-media@vger.kernel.org
---
 drivers/media/dvb/dvb-usb/dib0700_core.c |   47 ++---
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c 
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index d5e2c23..c73da6b 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -111,23 +111,24 @@ int dib0700_set_gpio(struct dvb_usb_device *d, enum 
dib07x0_gpios gpio, u8 gpio_
 
 static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 
nb_ts_packets)
 {
-struct dib0700_state *st = d-priv;
-u8 b[3];
-int ret;
-
-if (st-fw_version = 0x10201) {
-   b[0] = REQUEST_SET_USB_XFER_LEN;
-   b[1] = (nb_ts_packets  8)0xff;
-   b[2] = nb_ts_packets  0xff;
-
-   deb_info(set the USB xfer len to %i Ts packet\n, nb_ts_packets);
-
-   ret = dib0700_ctrl_wr(d, b, 3);
-} else {
-   deb_info(this firmware does not allow to change the USB xfer len\n);
-   ret = -EIO;
-}
-return ret;
+   struct dib0700_state *st = d-priv;
+   u8 b[3];
+   int ret;
+
+   if (st-fw_version = 0x10201) {
+   b[0] = REQUEST_SET_USB_XFER_LEN;
+   b[1] = (nb_ts_packets  8)0xff;
+   b[2] = nb_ts_packets  0xff;
+
+   deb_info(set the USB xfer len to %i Ts packet\n, 
nb_ts_packets);
+
+   ret = dib0700_ctrl_wr(d, b, 3);
+   } else {
+   deb_info(this firmware does not allow to change the USB xfer 
len\n);
+   ret = -EIO;
+   }
+
+   return ret;
 }
 
 /*
@@ -642,7 +643,7 @@ int dib0700_rc_setup(struct dvb_usb_device *d)
i = dib0700_ctrl_wr(d, rc_setup, 3);
if (i0) {
err(ir protocol setup failed);
-   return -1;
+   return i;
}
 
if (st-fw_version  0x10200)
@@ -652,7 +653,7 @@ int dib0700_rc_setup(struct dvb_usb_device *d)
purb = usb_alloc_urb(0, GFP_KERNEL);
if (purb == NULL) {
err(rc usb alloc urb failed\n);
-   return -1;
+   return -ENOMEM;
}
 
purb-transfer_buffer = usb_buffer_alloc(d-udev, RC_MSG_SIZE_V1_20,
@@ -661,7 +662,7 @@ int dib0700_rc_setup(struct dvb_usb_device *d)
if (purb-transfer_buffer == NULL) {
err(rc usb_buffer_alloc() failed\n);
usb_free_urb(purb);
-   return -1;
+   return -ENOMEM;
}
 
purb-status = -EINPROGRESS;
@@ -670,12 +671,10 @@ int dib0700_rc_setup(struct dvb_usb_device *d)
  dib0700_rc_urb_completion, d);
 
ret = usb_submit_urb(purb, GFP_ATOMIC);
-   if (ret != 0) {
+   if (ret != 0)
err(rc submit urb failed\n);
-   return -1;
-   }
 
-   return 0;
+   return ret;
 }
 
 static int dib0700_probe(struct usb_interface *intf,
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] drivers/media/dvb/dvb-usb/dib0700: fix return values

[Daniel Mack]

Propagte correct error values instead of returning -1 which just means
-EPERM (Permission denied)

While at it, also fix some coding style violations.

Signed-off-by: Daniel Mack dan...@caiaq.de
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Jiri Slaby jsl...@suse.cz
Cc: Dmitry Torokhov d...@mail.ru
Cc: Devin Heitmueller dheitmuel...@kernellabs.com
Cc: linux-media@vger.kernel.org
---
 drivers/media/dvb/dvb-usb/dib0700_core.c |   82 +++--
 1 files changed, 42 insertions(+), 40 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c 
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 4f961d2..45aec3a 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -53,7 +53,7 @@ static int dib0700_ctrl_wr(struct dvb_usb_device *d, u8 *tx, 
u8 txlen)
int status;
 
deb_data( );
-   debug_dump(tx,txlen,deb_data);
+   debug_dump(tx, txlen, deb_data);
 
status = usb_control_msg(d-udev, usb_sndctrlpipe(d-udev,0),
tx[0], USB_TYPE_VENDOR | USB_DIR_OUT, 0, 0, tx, txlen,
@@ -98,7 +98,7 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 
txlen, u8 *rx, u8 rxlen
deb_info(ep 0 read error (status = %d)\n,status);
 
deb_data( );
-   debug_dump(rx,rxlen,deb_data);
+   debug_dump(rx, rxlen, deb_data);
 
return status; /* length in case of success */
 }
@@ -106,28 +106,29 @@ int dib0700_ctrl_rd(struct dvb_usb_device *d, u8 *tx, u8 
txlen, u8 *rx, u8 rxlen
 int dib0700_set_gpio(struct dvb_usb_device *d, enum dib07x0_gpios gpio, u8 
gpio_dir, u8 gpio_val)
 {
u8 buf[3] = { REQUEST_SET_GPIO, gpio, ((gpio_dir  0x01)  7) | 
((gpio_val  0x01)  6) };
-   return dib0700_ctrl_wr(d,buf,3);
+   return dib0700_ctrl_wr(d, buf, sizeof(buf));
 }
 
 static int dib0700_set_usb_xfer_len(struct dvb_usb_device *d, u16 
nb_ts_packets)
 {
-struct dib0700_state *st = d-priv;
-u8 b[3];
-int ret;
-
-if (st-fw_version = 0x10201) {
-   b[0] = REQUEST_SET_USB_XFER_LEN;
-   b[1] = (nb_ts_packets  8)0xff;
-   b[2] = nb_ts_packets  0xff;
-
-   deb_info(set the USB xfer len to %i Ts packet\n, nb_ts_packets);
-
-   ret = dib0700_ctrl_wr(d, b, 3);
-} else {
-   deb_info(this firmware does not allow to change the USB xfer len\n);
-   ret = -EIO;
-}
-return ret;
+   struct dib0700_state *st = d-priv;
+   u8 b[3];
+   int ret;
+
+   if (st-fw_version = 0x10201) {
+   b[0] = REQUEST_SET_USB_XFER_LEN;
+   b[1] = (nb_ts_packets  8)  0xff;
+   b[2] = nb_ts_packets  0xff;
+
+   deb_info(set the USB xfer len to %i Ts packet\n, 
nb_ts_packets);
+
+   ret = dib0700_ctrl_wr(d, b, 3);
+   } else {
+   deb_info(this firmware does not allow to change the USB xfer 
len\n);
+   ret = -EIO;
+   }
+
+   return ret;
 }
 
 /*
@@ -178,7 +179,8 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, 
struct i2c_msg *msg,
value = ((en_start  7) | (en_stop  6) |
 (msg[i].len  0x3F))  8 | i2c_dest;
/* I2C ctrl + FE bus; */
-   index = ((gen_mode6)0xC0) | ((bus_mode4)0x30);
+   index = ((gen_mode  6)  0xC0) |
+   ((bus_mode  4)  0x30);
 
result = usb_control_msg(d-udev,
 usb_rcvctrlpipe(d-udev, 0),
@@ -198,11 +200,12 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, 
struct i2c_msg *msg,
} else {
/* Write request */
buf[0] = REQUEST_NEW_I2C_WRITE;
-   buf[1] = (msg[i].addr  1);
+   buf[1] = msg[i].addr  1;
buf[2] = (en_start  7) | (en_stop  6) |
(msg[i].len  0x3F);
/* I2C ctrl + FE bus; */
-   buf[3] = ((gen_mode6)0xC0) | ((bus_mode4)0x30);
+   buf[3] = ((gen_mode  6)  0xC0) |
+((bus_mode  4)  0x30);
/* The Actual i2c payload */
memcpy(buf[4], msg[i].buf, msg[i].len);
 
@@ -240,7 +243,7 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap,
 
for (i = 0; i  num; i++) {
/* fill in the address */
-   buf[1] = (msg[i].addr  1);
+   buf[1] = msg[i].addr  1;
/* fill the buffer */
memcpy(buf[2], msg[i].buf, msg[i].len);
 
@@ -368,7 +371,8 @@ int dib0700_download_firmware(struct usb_device *udev, 
const struct firmware *fw
u8 buf[260];
 
while ((ret = dvb_usb_get_hexline(fw, hx, pos))  0) {
-   deb_fwdata(writing to address 0x%08x (buffer: 0x%02x 
%02x)\n,hx.addr, hx.len, hx.chk