[PATCH 0/4] [media] Make lirc_bt829 a well-behaved PCI driver

[PATCH 0/4] [media] Make lirc_bt829 a well-behaved PCI driver

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

I noticed lirc_bt829 didn't have a module device ID table, so I set out
to fix that and ended up with this series.

It still appears to do everything else wrong (like reinventing
i2c-algo-bit) though...

This is compile-tested only.

Ben.

Ben Hutchings (4):
  [media] lirc_bt829: Make it a proper PCI driver
  [media] lirc_bt829: Fix physical address type
  [media] lirc_bt829: Fix iomap leak
  [media] lirc_bt829: Enable and disable memory BAR

 drivers/staging/media/lirc/lirc_bt829.c | 286 +++++++++++++++++---------------
 1 file changed, 154 insertions(+), 132 deletions(-)



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 1/4] [media] lirc_bt829: Make it a proper PCI driver

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 12693 bytes --]

Replace static variables with a device structure and pass pointers
to this into all the functions that need it.

Fold init_module(), do_pci_probe() and atir_init_start() into a
single probe function.  Use dev_err() to provide context for
logging.

This also fixes a device reference leak, as the driver wasn't
calling pci_dev_put().

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/staging/media/lirc/lirc_bt829.c | 276 +++++++++++++++++---------------
 1 file changed, 144 insertions(+), 132 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_bt829.c b/drivers/staging/media/lirc/lirc_bt829.c
index fa31ee7..c277bf3 100644
--- a/drivers/staging/media/lirc/lirc_bt829.c
+++ b/drivers/staging/media/lirc/lirc_bt829.c
@@ -30,25 +30,32 @@
 
 #include <media/lirc_dev.h>
 
-static int poll_main(void);
-static int atir_init_start(void);
+struct atir_device {
+	int minor;
+	unsigned char *pci_addr_lin;
+	struct lirc_driver driver;
+};
 
-static void write_index(unsigned char index, unsigned int value);
-static unsigned int read_index(unsigned char index);
+static int poll_main(struct atir_device *atir);
 
-static void do_i2c_start(void);
-static void do_i2c_stop(void);
+static void write_index(struct atir_device *atir, unsigned char index,
+			unsigned int value);
+static unsigned int read_index(struct atir_device *atir, unsigned char index);
 
-static void seems_wr_byte(unsigned char al);
-static unsigned char seems_rd_byte(void);
+static void do_i2c_start(struct atir_device *atir);
+static void do_i2c_stop(struct atir_device *atir);
 
-static unsigned int read_index(unsigned char al);
-static void write_index(unsigned char ah, unsigned int edx);
+static void seems_wr_byte(struct atir_device *atir, unsigned char al);
+static unsigned char seems_rd_byte(struct atir_device *atir);
+
+static unsigned int read_index(struct atir_device *atir, unsigned char al);
+static void write_index(struct atir_device *atir, unsigned char ah,
+			unsigned int edx);
 
 static void cycle_delay(int cycle);
 
-static void do_set_bits(unsigned char bl);
-static unsigned char do_get_bits(void);
+static void do_set_bits(struct atir_device *atir, unsigned char bl);
+static unsigned char do_get_bits(struct atir_device *atir);
 
 #define DATA_PCI_OFF 0x7FFC00
 #define WAIT_CYCLE   20
@@ -62,41 +69,12 @@ static bool debug;
 			printk(KERN_DEBUG DRIVER_NAME ": "fmt, ## args); \
 	} while (0)
 
-static int atir_minor;
-static unsigned long pci_addr_phys;
-static unsigned char *pci_addr_lin;
-
-static struct lirc_driver atir_driver;
-
-static struct pci_dev *do_pci_probe(void)
-{
-	struct pci_dev *my_dev;
-	my_dev = pci_get_device(PCI_VENDOR_ID_ATI,
-				PCI_DEVICE_ID_ATI_264VT, NULL);
-	if (my_dev) {
-		pr_err("Using device: %s\n", pci_name(my_dev));
-		pci_addr_phys = 0;
-		if (my_dev->resource[0].flags & IORESOURCE_MEM) {
-			pci_addr_phys = my_dev->resource[0].start;
-			pr_info("memory at 0x%08X\n",
-			       (unsigned int)pci_addr_phys);
-		}
-		if (pci_addr_phys == 0) {
-			pr_err("no memory resource ?\n");
-			return NULL;
-		}
-	} else {
-		pr_err("pci_probe failed\n");
-		return NULL;
-	}
-	return my_dev;
-}
-
 static int atir_add_to_buf(void *data, struct lirc_buffer *buf)
 {
+	struct atir_device *atir = data;
 	unsigned char key;
 	int status;
-	status = poll_main();
+	status = poll_main(atir);
 	key = (status >> 8) & 0xFF;
 	if (status & 0xFF) {
 		dprintk("reading key %02X\n", key);
@@ -117,172 +95,191 @@ static void atir_set_use_dec(void *data)
 	dprintk("driver is closed\n");
 }
 
-int init_module(void)
+static int atir_pci_probe(struct pci_dev *pdev,
+			  const struct pci_device_id *entry)
 {
-	struct pci_dev *pdev;
-
-	pdev = do_pci_probe();
-	if (pdev == NULL)
-		return -ENODEV;
-
-	if (!atir_init_start())
-		return -ENODEV;
-
-	strcpy(atir_driver.name, "ATIR");
-	atir_driver.minor       = -1;
-	atir_driver.code_length = 8;
-	atir_driver.sample_rate = 10;
-	atir_driver.data        = 0;
-	atir_driver.add_to_buf  = atir_add_to_buf;
-	atir_driver.set_use_inc = atir_set_use_inc;
-	atir_driver.set_use_dec = atir_set_use_dec;
-	atir_driver.dev         = &pdev->dev;
-	atir_driver.owner       = THIS_MODULE;
-
-	atir_minor = lirc_register_driver(&atir_driver);
-	if (atir_minor < 0) {
-		pr_err("failed to register driver!\n");
-		return atir_minor;
+	struct atir_device *atir;
+	unsigned long pci_addr_phys;
+	int rc;
+
+	atir = kzalloc(sizeof(*atir), GFP_KERNEL);
+	if (!atir)
+		return -ENOMEM;
+
+	pci_set_drvdata(pdev, atir);
+
+	if (!(pdev->resource[0].flags & IORESOURCE_MEM)) {
+		dev_err(&pdev->dev, "no memory resource ?\n");
+		rc = -ENODEV;
+		goto err_free;
 	}
-	dprintk("driver is registered on minor %d\n", atir_minor);
 
-	return 0;
-}
+	pci_addr_phys = pdev->resource[0].start;
+	dev_info(&pdev->dev, "memory at 0x%08X\n",
+		 (unsigned int)pci_addr_phys);
 
+	atir->pci_addr_lin = ioremap(pci_addr_phys + DATA_PCI_OFF, 0x400);
+	if (atir->pci_addr_lin == 0) {
+		dev_err(&pdev->dev, "pci mem must be mapped\n");
+		rc = -ENODEV;
+		goto err_free;
+	}
 
-void cleanup_module(void)
-{
-	lirc_unregister_driver(atir_minor);
+	strcpy(atir->driver.name, "ATIR");
+	atir->driver.minor       = -1;
+	atir->driver.code_length = 8;
+	atir->driver.sample_rate = 10;
+	atir->driver.data        = atir;
+	atir->driver.add_to_buf  = atir_add_to_buf;
+	atir->driver.set_use_inc = atir_set_use_inc;
+	atir->driver.set_use_dec = atir_set_use_dec;
+	atir->driver.dev         = &pdev->dev;
+	atir->driver.owner       = THIS_MODULE;
+
+	atir->minor = lirc_register_driver(&atir->driver);
+	if (atir->minor < 0) {
+		dev_err(&pdev->dev, "failed to register driver!\n");
+		rc = atir->minor;
+		goto err_free;
+	}
+	dprintk("driver is registered on minor %d\n", atir->minor);
+
+	return 0;
+
+err_free:
+	pci_set_drvdata(pdev, NULL);
+	kfree(atir);
+	return rc;
 }
 

-static int atir_init_start(void)
+static void atir_pci_remove(struct pci_dev *pdev)
 {
-	pci_addr_lin = ioremap(pci_addr_phys + DATA_PCI_OFF, 0x400);
-	if (pci_addr_lin == 0) {
-		pr_info("pci mem must be mapped\n");
-		return 0;
-	}
-	return 1;
+	struct atir_device *atir = pci_get_drvdata(pdev);
+
+	lirc_unregister_driver(atir->minor);
+	pci_set_drvdata(pdev, NULL);
+	kfree(atir);
 }
 
+
 static void cycle_delay(int cycle)
 {
 	udelay(WAIT_CYCLE*cycle);
 }
 

-static int poll_main(void)
+static int poll_main(struct atir_device *atir)
 {
 	unsigned char status_high, status_low;
 
-	do_i2c_start();
+	do_i2c_start(atir);
 
-	seems_wr_byte(0xAA);
-	seems_wr_byte(0x01);
+	seems_wr_byte(atir, 0xAA);
+	seems_wr_byte(atir, 0x01);
 
-	do_i2c_start();
+	do_i2c_start(atir);
 
-	seems_wr_byte(0xAB);
+	seems_wr_byte(atir, 0xAB);
 
-	status_low = seems_rd_byte();
-	status_high = seems_rd_byte();
+	status_low = seems_rd_byte(atir);
+	status_high = seems_rd_byte(atir);
 
-	do_i2c_stop();
+	do_i2c_stop(atir);
 
 	return (status_high << 8) | status_low;
 }
 
-static void do_i2c_start(void)
+static void do_i2c_start(struct atir_device *atir)
 {
-	do_set_bits(3);
+	do_set_bits(atir, 3);
 	cycle_delay(4);
 
-	do_set_bits(1);
+	do_set_bits(atir, 1);
 	cycle_delay(7);
 
-	do_set_bits(0);
+	do_set_bits(atir, 0);
 	cycle_delay(2);
 }
 
-static void do_i2c_stop(void)
+static void do_i2c_stop(struct atir_device *atir)
 {
 	unsigned char bits;
-	bits =  do_get_bits() & 0xFD;
-	do_set_bits(bits);
+	bits =  do_get_bits(atir) & 0xFD;
+	do_set_bits(atir, bits);
 	cycle_delay(1);
 
 	bits |= 1;
-	do_set_bits(bits);
+	do_set_bits(atir, bits);
 	cycle_delay(2);
 
 	bits |= 2;
-	do_set_bits(bits);
+	do_set_bits(atir, bits);
 	bits = 3;
-	do_set_bits(bits);
+	do_set_bits(atir, bits);
 	cycle_delay(2);
 }
 
-static void seems_wr_byte(unsigned char value)
+static void seems_wr_byte(struct atir_device *atir, unsigned char value)
 {
 	int i;
 	unsigned char reg;
 
-	reg = do_get_bits();
+	reg = do_get_bits(atir);
 	for (i = 0; i < 8; i++) {
 		if (value & 0x80)
 			reg |= 0x02;
 		else
 			reg &= 0xFD;
 
-		do_set_bits(reg);
+		do_set_bits(atir, reg);
 		cycle_delay(1);
 
 		reg |= 1;
-		do_set_bits(reg);
+		do_set_bits(atir, reg);
 		cycle_delay(1);
 
 		reg &= 0xFE;
-		do_set_bits(reg);
+		do_set_bits(atir, reg);
 		cycle_delay(1);
 		value <<= 1;
 	}
 	cycle_delay(2);
 
 	reg |= 2;
-	do_set_bits(reg);
+	do_set_bits(atir, reg);
 
 	reg |= 1;
-	do_set_bits(reg);
+	do_set_bits(atir, reg);
 
 	cycle_delay(1);
-	do_get_bits();
+	do_get_bits(atir);
 
 	reg &= 0xFE;
-	do_set_bits(reg);
+	do_set_bits(atir, reg);
 	cycle_delay(3);
 }
 
-static unsigned char seems_rd_byte(void)
+static unsigned char seems_rd_byte(struct atir_device *atir)
 {
 	int i;
 	int rd_byte;
 	unsigned char bits_2, bits_1;
 
-	bits_1 = do_get_bits() | 2;
-	do_set_bits(bits_1);
+	bits_1 = do_get_bits(atir) | 2;
+	do_set_bits(atir, bits_1);
 
 	rd_byte = 0;
 	for (i = 0; i < 8; i++) {
 		bits_1 &= 0xFE;
-		do_set_bits(bits_1);
+		do_set_bits(atir, bits_1);
 		cycle_delay(2);
 
 		bits_1 |= 1;
-		do_set_bits(bits_1);
+		do_set_bits(atir, bits_1);
 		cycle_delay(1);
 
-		bits_2 = do_get_bits();
+		bits_2 = do_get_bits(atir);
 		if (bits_2 & 2)
 			rd_byte |= 1;
 
@@ -293,15 +290,15 @@ static unsigned char seems_rd_byte(void)
 	if (bits_2 == 0)
 		bits_1 |= 2;
 
-	do_set_bits(bits_1);
+	do_set_bits(atir, bits_1);
 	cycle_delay(2);
 
 	bits_1 |= 1;
-	do_set_bits(bits_1);
+	do_set_bits(atir, bits_1);
 	cycle_delay(3);
 
 	bits_1 &= 0xFE;
-	do_set_bits(bits_1);
+	do_set_bits(atir, bits_1);
 	cycle_delay(2);
 
 	rd_byte >>= 1;
@@ -309,10 +306,10 @@ static unsigned char seems_rd_byte(void)
 	return rd_byte;
 }
 
-static void do_set_bits(unsigned char new_bits)
+static void do_set_bits(struct atir_device *atir, unsigned char new_bits)
 {
 	int reg_val;
-	reg_val = read_index(0x34);
+	reg_val = read_index(atir, 0x34);
 	if (new_bits & 2) {
 		reg_val &= 0xFFFFFFDF;
 		reg_val |= 1;
@@ -321,36 +318,36 @@ static void do_set_bits(unsigned char new_bits)
 		reg_val |= 0x20;
 	}
 	reg_val |= 0x10;
-	write_index(0x34, reg_val);
+	write_index(atir, 0x34, reg_val);
 
-	reg_val = read_index(0x31);
+	reg_val = read_index(atir, 0x31);
 	if (new_bits & 1)
 		reg_val |= 0x1000000;
 	else
 		reg_val &= 0xFEFFFFFF;
 
 	reg_val |= 0x8000000;
-	write_index(0x31, reg_val);
+	write_index(atir, 0x31, reg_val);
 }
 
-static unsigned char do_get_bits(void)
+static unsigned char do_get_bits(struct atir_device *atir)
 {
 	unsigned char bits;
 	int reg_val;
 
-	reg_val = read_index(0x34);
+	reg_val = read_index(atir, 0x34);
 	reg_val |= 0x10;
 	reg_val &= 0xFFFFFFDF;
-	write_index(0x34, reg_val);
+	write_index(atir, 0x34, reg_val);
 
-	reg_val = read_index(0x34);
+	reg_val = read_index(atir, 0x34);
 	bits = 0;
 	if (reg_val & 8)
 		bits |= 2;
 	else
 		bits &= 0xFD;
 
-	reg_val = read_index(0x31);
+	reg_val = read_index(atir, 0x31);
 	if (reg_val & 0x1000000)
 		bits |= 1;
 	else
@@ -359,26 +356,41 @@ static unsigned char do_get_bits(void)
 	return bits;
 }
 
-static unsigned int read_index(unsigned char index)
+static unsigned int read_index(struct atir_device *atir, unsigned char index)
 {
 	unsigned char *addr;
 	unsigned int value;
 	/*  addr = pci_addr_lin + DATA_PCI_OFF + ((index & 0xFF) << 2); */
-	addr = pci_addr_lin + ((index & 0xFF) << 2);
+	addr = atir->pci_addr_lin + ((index & 0xFF) << 2);
 	value = readl(addr);
 	return value;
 }
 
-static void write_index(unsigned char index, unsigned int reg_val)
+static void write_index(struct atir_device *atir, unsigned char index,
+			unsigned int reg_val)
 {
 	unsigned char *addr;
-	addr = pci_addr_lin + ((index & 0xFF) << 2);
+	addr = atir->pci_addr_lin + ((index & 0xFF) << 2);
 	writel(reg_val, addr);
 }
 
+static DEFINE_PCI_DEVICE_TABLE(atir_pci_table) = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_264VT) },
+	{ 0 }
+};
+
+static struct pci_driver atir_pci_driver = {
+	.name		= KBUILD_MODNAME,
+	.id_table	= atir_pci_table,
+	.probe		= atir_pci_probe,
+	.remove		= atir_pci_remove,
+};
+module_pci_driver(atir_pci_driver);
+
 MODULE_AUTHOR("Froenchenko Leonid");
 MODULE_DESCRIPTION("IR remote driver for bt829 based TV cards");
 MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, atir_pci_table);
 
 module_param(debug, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(debug, "Debug enabled or not");



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 2/4] [media] lirc_bt829: Fix physical address type

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/staging/media/lirc/lirc_bt829.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_bt829.c b/drivers/staging/media/lirc/lirc_bt829.c
index c277bf3..8c5ba2a 100644
--- a/drivers/staging/media/lirc/lirc_bt829.c
+++ b/drivers/staging/media/lirc/lirc_bt829.c
@@ -99,7 +99,7 @@ static int atir_pci_probe(struct pci_dev *pdev,
 			  const struct pci_device_id *entry)
 {
 	struct atir_device *atir;
-	unsigned long pci_addr_phys;
+	phys_addr_t pci_addr_phys;
 	int rc;
 
 	atir = kzalloc(sizeof(*atir), GFP_KERNEL);
@@ -115,8 +115,8 @@ static int atir_pci_probe(struct pci_dev *pdev,
 	}
 
 	pci_addr_phys = pdev->resource[0].start;
-	dev_info(&pdev->dev, "memory at 0x%08X\n",
-		 (unsigned int)pci_addr_phys);
+	dev_info(&pdev->dev, "memory at 0x%08llX\n",
+		 (unsigned long long)pci_addr_phys);
 
 	atir->pci_addr_lin = ioremap(pci_addr_phys + DATA_PCI_OFF, 0x400);
 	if (atir->pci_addr_lin == 0) {



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 3/4] [media] lirc_bt829: Fix iomap leak

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

We must call iounmap() when removed from a device.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/staging/media/lirc/lirc_bt829.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/lirc/lirc_bt829.c b/drivers/staging/media/lirc/lirc_bt829.c
index 8c5ba2a..76e6cfb 100644
--- a/drivers/staging/media/lirc/lirc_bt829.c
+++ b/drivers/staging/media/lirc/lirc_bt829.c
@@ -140,12 +140,14 @@ static int atir_pci_probe(struct pci_dev *pdev,
 	if (atir->minor < 0) {
 		dev_err(&pdev->dev, "failed to register driver!\n");
 		rc = atir->minor;
-		goto err_free;
+		goto err_unmap;
 	}
 	dprintk("driver is registered on minor %d\n", atir->minor);
 
 	return 0;
 
+err_unmap:
+	iounmap(atir->pci_addr_lin);
 err_free:
 	pci_set_drvdata(pdev, NULL);
 	kfree(atir);
@@ -158,6 +160,7 @@ static void atir_pci_remove(struct pci_dev *pdev)
 	struct atir_device *atir = pci_get_drvdata(pdev);
 
 	lirc_unregister_driver(atir->minor);
+	iounmap(atir->pci_addr_lin);
 	pci_set_drvdata(pdev, NULL);
 	kfree(atir);
 }



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 4/4] [media] lirc_bt829: Enable and disable memory BAR

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

We must not assume that the PCI device is already enabled.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/staging/media/lirc/lirc_bt829.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/lirc/lirc_bt829.c b/drivers/staging/media/lirc/lirc_bt829.c
index 76e6cfb..b386628 100644
--- a/drivers/staging/media/lirc/lirc_bt829.c
+++ b/drivers/staging/media/lirc/lirc_bt829.c
@@ -118,11 +118,15 @@ static int atir_pci_probe(struct pci_dev *pdev,
 	dev_info(&pdev->dev, "memory at 0x%08llX\n",
 		 (unsigned long long)pci_addr_phys);
 
+	rc = pci_enable_device_mem(pdev);
+	if (rc)
+		goto err_free;
+
 	atir->pci_addr_lin = ioremap(pci_addr_phys + DATA_PCI_OFF, 0x400);
 	if (atir->pci_addr_lin == 0) {
 		dev_err(&pdev->dev, "pci mem must be mapped\n");
 		rc = -ENODEV;
-		goto err_free;
+		goto err_disable;
 	}
 
 	strcpy(atir->driver.name, "ATIR");
@@ -148,6 +152,8 @@ static int atir_pci_probe(struct pci_dev *pdev,
 
 err_unmap:
 	iounmap(atir->pci_addr_lin);
+err_disable:
+	pci_disable_device(pdev);
 err_free:
 	pci_set_drvdata(pdev, NULL);
 	kfree(atir);
@@ -161,6 +167,7 @@ static void atir_pci_remove(struct pci_dev *pdev)
 
 	lirc_unregister_driver(atir->minor);
 	iounmap(atir->pci_addr_lin);
+	pci_disable_device(pdev);
 	pci_set_drvdata(pdev, NULL);
 	kfree(atir);
 }


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/4] [media] lirc_bt829: Fix physical address type

[Fabio Estevam]

On Sun, Sep 1, 2013 at 9:39 PM, Ben Hutchings <ben@decadent.org.uk> wrote:

>         pci_addr_phys = pdev->resource[0].start;
> -       dev_info(&pdev->dev, "memory at 0x%08X\n",
> -                (unsigned int)pci_addr_phys);
> +       dev_info(&pdev->dev, "memory at 0x%08llX\n",
> +                (unsigned long long)pci_addr_phys);

You could use %pa instead for printing phys_addr_t:

dev_info(&pdev->dev, "memory at %pa\n", &pci_addr_phys);

Re: [PATCH 2/4] [media] lirc_bt829: Fix physical address type

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Sun, 2013-09-01 at 22:37 -0300, Fabio Estevam wrote:
> On Sun, Sep 1, 2013 at 9:39 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> 
> >         pci_addr_phys = pdev->resource[0].start;
> > -       dev_info(&pdev->dev, "memory at 0x%08X\n",
> > -                (unsigned int)pci_addr_phys);
> > +       dev_info(&pdev->dev, "memory at 0x%08llX\n",
> > +                (unsigned long long)pci_addr_phys);
> 
> You could use %pa instead for printing phys_addr_t:
> 
> dev_info(&pdev->dev, "memory at %pa\n", &pci_addr_phys);

Could do, but I'm not sure it's clearer.  And all these %p formats
defeat type-checking anyway.

Ben.

-- 
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/4] [media] lirc_bt829: Fix physical address type

[Fabio Estevam]

On Sun, Sep 1, 2013 at 10:41 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Sun, 2013-09-01 at 22:37 -0300, Fabio Estevam wrote:
>> On Sun, Sep 1, 2013 at 9:39 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>>
>> >         pci_addr_phys = pdev->resource[0].start;
>> > -       dev_info(&pdev->dev, "memory at 0x%08X\n",
>> > -                (unsigned int)pci_addr_phys);
>> > +       dev_info(&pdev->dev, "memory at 0x%08llX\n",
>> > +                (unsigned long long)pci_addr_phys);
>>
>> You could use %pa instead for printing phys_addr_t:
>>
>> dev_info(&pdev->dev, "memory at %pa\n", &pci_addr_phys);
>
> Could do, but I'm not sure it's clearer.  And all these %p formats
> defeat type-checking anyway.

IMHO using %pa looks clearer and it is also recommended in
Documentation/printk-formats.txt.

Re: [PATCH 0/4] [media] Make lirc_bt829 a well-behaved PCI driver

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

On Mon, 2013-09-02 at 01:36 +0100, Ben Hutchings wrote:
> I noticed lirc_bt829 didn't have a module device ID table, so I set out
> to fix that and ended up with this series.
> 
> It still appears to do everything else wrong (like reinventing
> i2c-algo-bit) though...
> 
> This is compile-tested only.

On reflection, I think it might be better to leave this driver 'badly
behaved'.  It wants to use registers on a Mach64 VT, which has a
separate kernel framebuffer driver (atyfb) and userland X driver
(mach64).  If this driver is compatible with them now, changing it is
liable to break that.

I'll repost the minor fixes.

Ben.

-- 
Ben Hutchings
Knowledge is power.  France is bacon.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]