Skip to contentSkip to conversation

A few SOLO6x10 patches.

PATCH In review Krzysztof Hałasa 2 participants 10 messages Jun 8, 2015 – Jun 14, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

Hi,

I'm attaching a few patches to SOLO6x10 video frame grabber driver. Nothing out of ordinary, an audio bug fix (nobody using SOLO with audio?), an rmmod-related panic and the register access "optimization". Feel free to pick up what you want.

The remaining issue is incorrect SDRAM size reported in certain cases. I still have to investigate it a bit.

Signature · 4 lines
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
#1 lore raw

PATCH 1/1 SOLO6x10: unmap registers only after free_irq().

Fixes a panic on ARM. Diagnosis by Russell King.

drivers/media/pci/solo6x10/solo6x10-core.c +1 -1
@@ -164,9 +164,9 @@ static void free_solo_dev(struct solo_dev *solo_dev)
 
 		/* Now cleanup the PCI device */
 		solo_irq_off(solo_dev, ~0);
-		pci_iounmap(pdev, solo_dev->reg_base);
 		if (pdev->irq)
 			free_irq(pdev->irq, solo_dev);
+		pci_iounmap(pdev, solo_dev->reg_base);
 	}
 
 	pci_release_regions(pdev);
maintainer
#2 lore raw

Re: [PATCH] SOLO6x10: unmap registers only after free_irq().

Quoted 18 lines · On Mon, 08 Jun 2015 15:37:15 +0200 [email protected] (Krzysztof Hałasa) wrote:

Fixes a panic on ARM. Diagnosis by Russell King.

drivers/media/pci/solo6x10/solo6x10-core.c +1 -2
@@ -164,9 +164,9 @@ static void free_solo_dev(struct solo_dev
 *solo_dev) 
 		/* Now cleanup the PCI device */
 		solo_irq_off(solo_dev, ~0);
-		pci_iounmap(pdev, solo_dev->reg_base);
 		if (pdev->irq)
 			free_irq(pdev->irq, solo_dev);
+		pci_iounmap(pdev, solo_dev->reg_base);
 	}
 
 	pci_release_regions(pdev);
--
Quoted 3 lines

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

#3 lore raw

PATCH 1/1 SOLO6x10: Fix G.723 minimum audio period count.

In reply to Krzysztof Hałasa

The period count is fixed, don't confuse ALSA.

drivers/media/pci/solo6x10/solo6x10-g723.c +6 -7
@@ -48,10 +48,8 @@
 /* The solo writes to 1k byte pages, 32 pages, in the dma. Each 1k page
  * is broken down to 20 * 48 byte regions (one for each channel possible)
  * with the rest of the page being dummy data. */
-#define G723_MAX_BUFFER		(G723_PERIOD_BYTES * PERIODS_MAX)
+#define PERIODS			G723_FDMA_PAGES
 #define G723_INTR_ORDER		4 /* 0 - 4 */
-#define PERIODS_MIN		(1 << G723_INTR_ORDER)
-#define PERIODS_MAX		G723_FDMA_PAGES
 
 struct solo_snd_pcm {
 	int				on;
@@ -130,11 +128,11 @@ static const struct snd_pcm_hardware snd_solo_pcm_hw = {
 	.rate_max		= SAMPLERATE,
 	.channels_min		= 1,
 	.channels_max		= 1,
-	.buffer_bytes_max	= G723_MAX_BUFFER,
+	.buffer_bytes_max	= G723_PERIOD_BYTES * PERIODS,
 	.period_bytes_min	= G723_PERIOD_BYTES,
 	.period_bytes_max	= G723_PERIOD_BYTES,
-	.periods_min		= PERIODS_MIN,
-	.periods_max		= PERIODS_MAX,
+	.periods_min		= PERIODS,
+	.periods_max		= PERIODS,
 };
 
 static int snd_solo_pcm_open(struct snd_pcm_substream *ss)
@@ -340,7 +338,8 @@ static int solo_snd_pcm_init(struct solo_dev *solo_dev)
 	ret = snd_pcm_lib_preallocate_pages_for_all(pcm,
 					SNDRV_DMA_TYPE_CONTINUOUS,
 					snd_dma_continuous_data(GFP_KERNEL),
-					G723_MAX_BUFFER, G723_MAX_BUFFER);
+					G723_PERIOD_BYTES * PERIODS,
+					G723_PERIOD_BYTES * PERIODS);
 	if (ret < 0)
 		return ret;
 
maintainer
#4 lore raw

Re: [PATCH] SOLO6x10: Fix G.723 minimum audio period count.

Quoted 47 lines · On Mon, 08 Jun 2015 15:35:05 +0200 [email protected] (Krzysztof Hałasa) wrote:

The period count is fixed, don't confuse ALSA.

drivers/media/pci/solo6x10/solo6x10-g723.c +0 -0
@@ -48,10 +48,8 @@
 /* The solo writes to 1k byte pages, 32 pages, in the dma. Each 1k

page
* is broken down to 20 * 48 byte regions (one for each channel
possible)
* with the rest of the page being dummy data. */
-#define G723_MAX_BUFFER (G723_PERIOD_BYTES *
PERIODS_MAX) +#define PERIODS G723_FDMA_PAGES
#define G723_INTR_ORDER 4 /* 0 - 4 */

drivers/media/pci/solo6x10/solo6x10-g723.c +5 -7
-#define PERIODS_MIN		(1 << G723_INTR_ORDER)
-#define PERIODS_MAX		G723_FDMA_PAGES
 
 struct solo_snd_pcm {
 	int				on;
@@ -130,11 +128,11 @@ static const struct snd_pcm_hardware
 snd_solo_pcm_hw = { .rate_max		= SAMPLERATE,
 	.channels_min		= 1,
 	.channels_max		= 1,
-	.buffer_bytes_max	= G723_MAX_BUFFER,
+	.buffer_bytes_max	= G723_PERIOD_BYTES * PERIODS,
 	.period_bytes_min	= G723_PERIOD_BYTES,
 	.period_bytes_max	= G723_PERIOD_BYTES,
-	.periods_min		= PERIODS_MIN,
-	.periods_max		= PERIODS_MAX,
+	.periods_min		= PERIODS,
+	.periods_max		= PERIODS,
 };
 
 static int snd_solo_pcm_open(struct snd_pcm_substream *ss)
@@ -340,7 +338,8 @@ static int solo_snd_pcm_init(struct solo_dev
 *solo_dev) ret = snd_pcm_lib_preallocate_pages_for_all(pcm,
 					SNDRV_DMA_TYPE_CONTINUOUS,
 					snd_dma_continuous_data(GFP_KERNEL),
-					G723_MAX_BUFFER,
 G723_MAX_BUFFER);
+					G723_PERIOD_BYTES * PERIODS,
+					G723_PERIOD_BYTES * PERIODS);
 	if (ret < 0)
 		return ret;
 
--
Quoted 3 lines

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

#5 lore raw

PATCH 1/1 SOLO6x10: remove unneeded register locking and barriers.

In reply to Krzysztof Hałasa

readl() and writel() are atomic, we don't need the spin lock. Also, flushing posted write buffer isn't required. Especially on read :-)

drivers/media/pci/solo6x10/solo6x10-core.c +0 -1
@@ -483,7 +483,6 @@ static int solo_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	solo_dev->type = id->driver_data;
 	solo_dev->pdev = pdev;
-	spin_lock_init(&solo_dev->reg_io_lock);
 	ret = v4l2_device_register(&pdev->dev, &solo_dev->v4l2_dev);
 	if (ret)
 		goto fail_probe;
drivers/media/pci/solo6x10/solo6x10.h +1 -25
@@ -201,7 +201,6 @@ struct solo_dev {
 	int			nr_ext;
 	u32			irq_mask;
 	u32			motion_mask;
-	spinlock_t		reg_io_lock;
 	struct v4l2_device	v4l2_dev;
 
 	/* tw28xx accounting */
@@ -283,36 +282,13 @@ struct solo_dev {
 
 static inline u32 solo_reg_read(struct solo_dev *solo_dev, int reg)
 {
-	unsigned long flags;
-	u32 ret;
-	u16 val;
-
-	spin_lock_irqsave(&solo_dev->reg_io_lock, flags);
-
-	ret = readl(solo_dev->reg_base + reg);
-	rmb();
-	pci_read_config_word(solo_dev->pdev, PCI_STATUS, &val);
-	rmb();
-
-	spin_unlock_irqrestore(&solo_dev->reg_io_lock, flags);
-
-	return ret;
+	return readl(solo_dev->reg_base + reg);
 }
 
 static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
 				  u32 data)
 {
-	unsigned long flags;
-	u16 val;
-
-	spin_lock_irqsave(&solo_dev->reg_io_lock, flags);
-
 	writel(data, solo_dev->reg_base + reg);
-	wmb();
-	pci_read_config_word(solo_dev->pdev, PCI_STATUS, &val);
-	rmb();
-
-	spin_unlock_irqrestore(&solo_dev->reg_io_lock, flags);
 }
 
 static inline void solo_irq_on(struct solo_dev *dev, u32 mask)
maintainer
#6 lore raw

Re: [PATCH] SOLO6x10: remove unneeded register locking and barriers.

Quoted 54 lines · On Mon, 08 Jun 2015 15:42:24 +0200 [email protected] (Krzysztof Hałasa) wrote:

readl() and writel() are atomic, we don't need the spin lock. Also, flushing posted write buffer isn't required. Especially on read :-)

drivers/media/pci/solo6x10/solo6x10-core.c +0 -1
@@ -483,7 +483,6 @@ static int solo_pci_probe(struct pci_dev *pdev,
 const struct pci_device_id *id) 
 	solo_dev->type = id->driver_data;
 	solo_dev->pdev = pdev;
-	spin_lock_init(&solo_dev->reg_io_lock);
 	ret = v4l2_device_register(&pdev->dev, &solo_dev->v4l2_dev);
 	if (ret)
 		goto fail_probe;
drivers/media/pci/solo6x10/solo6x10.h +1 -19
@@ -201,7 +201,6 @@ struct solo_dev {
 	int			nr_ext;
 	u32			irq_mask;
 	u32			motion_mask;
-	spinlock_t		reg_io_lock;
 	struct v4l2_device	v4l2_dev;
 
 	/* tw28xx accounting */
@@ -283,36 +282,13 @@ struct solo_dev {
 
 static inline u32 solo_reg_read(struct solo_dev *solo_dev, int reg)
 {
-	unsigned long flags;
-	u32 ret;
-	u16 val;
-
-	spin_lock_irqsave(&solo_dev->reg_io_lock, flags);
-
-	ret = readl(solo_dev->reg_base + reg);
-	rmb();
-	pci_read_config_word(solo_dev->pdev, PCI_STATUS, &val);
-	rmb();
-
-	spin_unlock_irqrestore(&solo_dev->reg_io_lock, flags);
-
-	return ret;
+	return readl(solo_dev->reg_base + reg);
 }
 
 static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
 				  u32 data)
 {
-	unsigned long flags;
-	u16 val;
-
-	spin_lock_irqsave(&solo_dev->reg_io_lock, flags);

drivers/media/pci/solo6x10/solo6x10.h struct solo_dev { · line 294

Quoted 10 lines
drivers/media/pci/solo6x10/solo6x10.h +0 -6
-
 	writel(data, solo_dev->reg_base + reg);
-	wmb();
-	pci_read_config_word(solo_dev->pdev, PCI_STATUS, &val);
-	rmb();
-
-	spin_unlock_irqrestore(&solo_dev->reg_io_lock, flags);
 }
 
 static inline void solo_irq_on(struct solo_dev *dev, u32 mask)
#7 lore raw

PATCH 1/1 SOLO6x10: Remove dead code.

In reply to Krzysztof Hałasa

solo_dev and pdev cannot be NULL here. It doesn't matter if we initialized the PCI device or not.

drivers/media/pci/solo6x10/solo6x10-core.c +2 -15
@@ -134,23 +134,11 @@ static irqreturn_t solo_isr(int irq, void *data)
 
 static void free_solo_dev(struct solo_dev *solo_dev)
 {
-	struct pci_dev *pdev;
-
-	if (!solo_dev)
-		return;
+	struct pci_dev *pdev = solo_dev->pdev;
 
 	if (solo_dev->dev.parent)
 		device_unregister(&solo_dev->dev);
 
-	pdev = solo_dev->pdev;
-
-	/* If we never initialized the PCI device, then nothing else
-	 * below here needs cleanup */
-	if (!pdev) {
-		kfree(solo_dev);
-		return;
-	}
-
 	if (solo_dev->reg_base) {
 		/* Bring down the sub-devices first */
 		solo_g723_exit(solo_dev);
@@ -164,8 +152,7 @@ static void free_solo_dev(struct solo_dev *solo_dev)
 
 		/* Now cleanup the PCI device */
 		solo_irq_off(solo_dev, ~0);
-		if (pdev->irq)
-			free_irq(pdev->irq, solo_dev);
+		free_irq(pdev->irq, solo_dev);
 		pci_iounmap(pdev, solo_dev->reg_base);
 	}
 
maintainer
#8 lore raw

Re: [PATCH] SOLO6x10: Remove dead code.

Quoted 33 lines · On Mon, 08 Jun 2015 15:50:22 +0200 [email protected] (Krzysztof Hałasa) wrote:

solo_dev and pdev cannot be NULL here. It doesn't matter if we initialized the PCI device or not.

drivers/media/pci/solo6x10/solo6x10-core.c +1 -13
@@ -134,23 +134,11 @@ static irqreturn_t solo_isr(int irq, void *data)
 
 static void free_solo_dev(struct solo_dev *solo_dev)
 {
-	struct pci_dev *pdev;
-
-	if (!solo_dev)
-		return;
+	struct pci_dev *pdev = solo_dev->pdev;
 
 	if (solo_dev->dev.parent)
 		device_unregister(&solo_dev->dev);
 
-	pdev = solo_dev->pdev;
-
-	/* If we never initialized the PCI device, then nothing else
-	 * below here needs cleanup */
-	if (!pdev) {
-		kfree(solo_dev);
-		return;
-	}
-
 	if (solo_dev->reg_base) {
 		/* Bring down the sub-devices first */
 		solo_g723_exit(solo_dev);

drivers/media/pci/solo6x10/solo6x10-core.c static void free_solo_dev(struct solo_dev · line 158

Quoted 9 lines
drivers/media/pci/solo6x10/solo6x10-core.c +1 -2
@@ -164,8 +152,7 @@ static void free_solo_dev(struct solo_dev
 *solo_dev) 
 		/* Now cleanup the PCI device */
 		solo_irq_off(solo_dev, ~0);
-		if (pdev->irq)
-			free_irq(pdev->irq, solo_dev);
+		free_irq(pdev->irq, solo_dev);
 		pci_iounmap(pdev, solo_dev->reg_base);
 	}