[PATCH 01/20] Staging: ipack/bridges/tpci200: Put the TPCI200 control registers into a struct.

[PATCH 01/20] Staging: ipack/bridges/tpci200: Put the TPCI200 control registers into a struct.

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

This saves us from a little pointer arithmetic and cleans up the code a bit.

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |   28 +++++++---------------------
 drivers/staging/ipack/bridges/tpci200.h |   21 +++++++++++++--------
 2 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index b81a8c9..4383953 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -16,14 +16,6 @@
 
 static struct ipack_bus_ops tpci200_bus_ops;
 
-/* TPCI200 controls registers */
-static int control_reg[] = {
-	TPCI200_CONTROL_A_REG,
-	TPCI200_CONTROL_B_REG,
-	TPCI200_CONTROL_C_REG,
-	TPCI200_CONTROL_D_REG
-};
-
 static int tpci200_slot_unregister(struct ipack_device *dev);
 
 static struct tpci200_board *check_slot(struct ipack_device *dev)
@@ -87,8 +79,7 @@ static irqreturn_t tpci200_interrupt(int irq, void *dev_id)
 	irqreturn_t ret = IRQ_NONE;
 
 	/* Read status register */
-	status_reg = readw(tpci200->info->interface_regs +
-			   TPCI200_STATUS_REG);
+	status_reg = readw(&tpci200->info->interface_regs->status);
 
 	if (status_reg & TPCI200_SLOT_INT_MASK) {
 		unhandled_ints = status_reg & TPCI200_SLOT_INT_MASK;
@@ -109,7 +100,7 @@ static irqreturn_t tpci200_interrupt(int irq, void *dev_id)
 			}
 		}
 	}
-	/* Interrupt not handled are disabled */
+	/* Interrupts not handled are disabled */
 	if (unhandled_ints) {
 		for (i = 0; i < TPCI200_NB_SLOT; i++) {
 			if (unhandled_ints & ((TPCI200_INT0_EN | TPCI200_INT1_EN) << (2*i))) {
@@ -117,13 +108,11 @@ static irqreturn_t tpci200_interrupt(int irq, void *dev_id)
 					 "No registered ISR for slot [%d:%d]!. IRQ will be disabled.\n",
 					 tpci200->number, i);
 				reg_value = readw(
-					tpci200->info->interface_regs +
-					control_reg[i]);
+					&tpci200->info->interface_regs->control[i]);
 				reg_value &=
 					~(TPCI200_INT0_EN | TPCI200_INT1_EN);
 				writew(reg_value,
-				       (tpci200->info->interface_regs +
-					control_reg[i]));
+				       &tpci200->info->interface_regs->control[i]);
 			}
 		}
 	}
@@ -221,8 +210,7 @@ static int tpci200_register(struct tpci200_board *tpci200)
 			(void __iomem *)mem_base + TPCI200_MEM8_GAP*i;
 		tpci200->slots[i].mem_phys.size = TPCI200_MEM8_SIZE;
 
-		writew(slot_ctrl, (tpci200->info->interface_regs +
-				   control_reg[i]));
+		writew(slot_ctrl, &tpci200->info->interface_regs->control[i]);
 	}
 
 	res = request_irq(tpci200->info->pdev->irq,
@@ -261,8 +249,7 @@ static int __tpci200_request_irq(struct tpci200_board *tpci200,
 	 * clock rate 8 MHz
 	 */
 	slot_ctrl = TPCI200_INT0_EN | TPCI200_INT1_EN;
-	writew(slot_ctrl, (tpci200->info->interface_regs +
-			   control_reg[dev->slot]));
+	writew(slot_ctrl, &tpci200->info->interface_regs->control[dev->slot]);
 
 	return 0;
 }
@@ -281,8 +268,7 @@ static void __tpci200_free_irq(struct tpci200_board *tpci200,
 	 * clock rate 8 MHz
 	 */
 	slot_ctrl = 0;
-	writew(slot_ctrl, (tpci200->info->interface_regs +
-			   control_reg[dev->slot]));
+	writew(slot_ctrl, &tpci200->info->interface_regs->control[dev->slot]);
 }
 
 static int tpci200_free_irq(struct ipack_device *dev)
diff --git a/drivers/staging/ipack/bridges/tpci200.h b/drivers/staging/ipack/bridges/tpci200.h
index 38acba1..af48295 100644
--- a/drivers/staging/ipack/bridges/tpci200.h
+++ b/drivers/staging/ipack/bridges/tpci200.h
@@ -37,13 +37,15 @@
 #define TPCI200_MEM16_SPACE_BAR       4
 #define TPCI200_MEM8_SPACE_BAR        5
 
-#define TPCI200_REVISION_REG          0x00
-#define TPCI200_CONTROL_A_REG         0x02
-#define TPCI200_CONTROL_B_REG         0x04
-#define TPCI200_CONTROL_C_REG         0x06
-#define TPCI200_CONTROL_D_REG         0x08
-#define TPCI200_RESET_REG             0x0A
-#define TPCI200_STATUS_REG            0x0C
+struct tpci200_regs {
+	__le16	revision;
+	/* writes to control should occur with the mutex held to protect
+	 * read-modify-write operations */
+	__le16  control[4];
+	__le16	reset;
+	__le16	status;
+	u8	reserved[242];
+} __packed;
 
 #define TPCI200_IFACE_SIZE            0x100
 
@@ -63,6 +65,7 @@
 #define TPCI200_MEM16_GAP             0x00800000
 #define TPCI200_MEM16_SIZE            0x00800000
 
+/* control field in tpci200_regs */
 #define TPCI200_INT0_EN               0x0040
 #define TPCI200_INT1_EN               0x0080
 #define TPCI200_INT0_EDGE             0x0010
@@ -72,11 +75,13 @@
 #define TPCI200_RECOVER_EN            0x0002
 #define TPCI200_CLK32                 0x0001
 
+/* reset field in tpci200_regs */
 #define TPCI200_A_RESET               0x0001
 #define TPCI200_B_RESET               0x0002
 #define TPCI200_C_RESET               0x0004
 #define TPCI200_D_RESET               0x0008
 
+/* status field in tpci200_regs */
 #define TPCI200_A_TIMEOUT             0x1000
 #define TPCI200_B_TIMEOUT             0x2000
 #define TPCI200_C_TIMEOUT             0x4000
@@ -149,7 +154,7 @@ struct tpci200_slot {
 struct tpci200_infos {
 	struct pci_dev			*pdev;
 	struct pci_device_id		*id_table;
-	void __iomem			*interface_regs;
+	struct tpci200_regs __iomem	*interface_regs;
 	void __iomem			*ioidint_space;
 	void __iomem			*mem8_space;
 	void __iomem			*cfg_regs;
-- 
1.7.10.4

[PATCH 02/20] Staging: ipack: Provide several carrier callbacks.

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

We provide callbacks to:
 - set/get the clockrate a module is accessed at,
 - get the error state of a slot,
 - get/reset the timeout state of a slot.

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/ipack.h |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/staging/ipack/ipack.h b/drivers/staging/ipack/ipack.h
index 0f482fd..0526613 100644
--- a/drivers/staging/ipack/ipack.h
+++ b/drivers/staging/ipack/ipack.h
@@ -113,6 +113,15 @@ struct ipack_driver {
  *	@request_irq: request IRQ
  *	@free_irq: free IRQ
  *	@remove_device: tell the bridge module that the device has been removed
+ *	@get_clockrate: Returns the clockrate the carrier is currently
+ *		communicating with the device at.
+ *	@set_clockrate: Sets the clock-rate for carrier / module communication.
+ *		Should return -EINVAL if the requested speed is not supported.
+ *	@get_error: Returns the error state for the slot the device is attached
+ *		to.
+ *	@get_timeout: Returns 1 if the communication with the device has
+ *		previously timed out.
+ *	@reset_timeout: Resets the state returned by get_timeout.
  */
 struct ipack_bus_ops {
 	int (*map_space) (struct ipack_device *dev, unsigned int memory_size, int space);
@@ -120,6 +129,12 @@ struct ipack_bus_ops {
 	int (*request_irq) (struct ipack_device *dev, int vector, int (*handler)(void *), void *arg);
 	int (*free_irq) (struct ipack_device *dev);
 	int (*remove_device) (struct ipack_device *dev);
+
+	int (*get_clockrate) (struct ipack_device *dev);
+	int (*set_clockrate) (struct ipack_device *dev, int mherz);
+	int (*get_error) (struct ipack_device *dev);
+	int (*get_timeout) (struct ipack_device *dev);
+	int (*reset_timeout) (struct ipack_device *dev);
 };
 
 /**
-- 
1.7.10.4

[PATCH 03/20] Staging: ipack/bridges/tpci200: provide new callbacks to tpci200

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

Provide get_clockrate, set_clockrate, get_error, get_timeout and reset_timeout
callbacks.

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |  107 +++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 4383953..861b00d 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -14,6 +14,20 @@
 #include <linux/module.h>
 #include "tpci200.h"
 
+static u16 tpci200_status_timeout[] = {
+	TPCI200_A_TIMEOUT,
+	TPCI200_B_TIMEOUT,
+	TPCI200_C_TIMEOUT,
+	TPCI200_D_TIMEOUT,
+};
+
+static u16 tpci200_status_error[] = {
+	TPCI200_A_ERROR,
+	TPCI200_B_ERROR,
+	TPCI200_C_ERROR,
+	TPCI200_D_ERROR,
+};
+
 static struct ipack_bus_ops tpci200_bus_ops;
 
 static int tpci200_slot_unregister(struct ipack_device *dev);
@@ -507,6 +521,94 @@ out_unlock:
 	return res;
 }
 
+static int tpci200_get_clockrate(struct ipack_device *dev)
+{
+	struct tpci200_board *tpci200 = check_slot(dev);
+	__le16 __iomem *addr;
+
+	if (!tpci200)
+		return -ENODEV;
+
+	addr = &tpci200->info->interface_regs->control[dev->slot];
+	return (ioread16(addr) & TPCI200_CLK32) ? 32 : 8;
+}
+
+static int tpci200_set_clockrate(struct ipack_device *dev, int mherz)
+{
+	struct tpci200_board *tpci200 = check_slot(dev);
+	__le16 __iomem *addr;
+	u16 reg;
+
+	if (!tpci200)
+		return -ENODEV;
+
+	addr = &tpci200->info->interface_regs->control[dev->slot];
+
+	/* Ensure the control register is not changed by another task after we
+	 * have read it. */
+	mutex_lock(&tpci200->mutex);
+	reg = ioread16(addr);
+	switch (mherz) {
+	case 8:
+		reg &= ~(TPCI200_CLK32); break;
+	case 32:
+		reg |= TPCI200_CLK32; break;
+	default:
+		mutex_unlock(&tpci200->mutex);
+		return -EINVAL;
+	}
+	iowrite16(reg, addr);
+	mutex_unlock(&tpci200->mutex);
+	return 0;
+}
+
+static int tpci200_get_error(struct ipack_device *dev)
+{
+	struct tpci200_board *tpci200 = check_slot(dev);
+	__le16 __iomem *addr;
+	u16 mask;
+
+	if (!tpci200)
+		return -ENODEV;
+
+	addr = &tpci200->info->interface_regs->status;
+	mask = tpci200_status_error[dev->slot];
+	return (ioread16(addr) & mask) ? 1 : 0;
+}
+
+static int tpci200_get_timeout(struct ipack_device *dev)
+{
+	struct tpci200_board *tpci200 = check_slot(dev);
+	__le16 __iomem *addr;
+	u16 mask;
+
+	if (!tpci200)
+		return -ENODEV;
+
+	addr = &tpci200->info->interface_regs->status;
+	mask = tpci200_status_timeout[dev->slot];
+
+	return (ioread16(addr) & mask) ? 1 : 0;
+}
+
+static int tpci200_reset_timeout(struct ipack_device *dev)
+{
+	struct tpci200_board *tpci200 = check_slot(dev);
+	__le16 __iomem *addr;
+	u16 mask;
+
+	if (!tpci200)
+		return -ENODEV;
+
+	addr = &tpci200->info->interface_regs->status;
+	mask = tpci200_status_timeout[dev->slot];
+
+	iowrite16(mask, addr);
+	return 0;
+}
+
+
+
 static void tpci200_uninstall(struct tpci200_board *tpci200)
 {
 	int i;
@@ -524,6 +626,11 @@ static struct ipack_bus_ops tpci200_bus_ops = {
 	.request_irq = tpci200_request_irq,
 	.free_irq = tpci200_free_irq,
 	.remove_device = tpci200_slot_unregister,
+	.get_clockrate = tpci200_get_clockrate,
+	.set_clockrate = tpci200_set_clockrate,
+	.get_error     = tpci200_get_error,
+	.get_timeout   = tpci200_get_timeout,
+	.reset_timeout = tpci200_reset_timeout,
 };
 
 static int tpci200_install(struct tpci200_board *tpci200)
-- 
1.7.10.4

Re: [PATCH 03/20] Staging: ipack/bridges/tpci200: provide new callbacks to tpci200

[Dan Carpenter]

I had a few style comments on this patchset.  Nothing that couldn't
be fixed later.

On Mon, Sep 10, 2012 at 10:51:41AM +0200, Samuel Iglesias Gonsálvez wrote:
> From: Jens Taprogge <jens.taprogge@taprogge.org>
> 
> Provide get_clockrate, set_clockrate, get_error, get_timeout and reset_timeout
> callbacks.
> 
> Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> ---
>  drivers/staging/ipack/bridges/tpci200.c |  107 +++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
> index 4383953..861b00d 100644
> --- a/drivers/staging/ipack/bridges/tpci200.c
> +++ b/drivers/staging/ipack/bridges/tpci200.c
> @@ -14,6 +14,20 @@
>  #include <linux/module.h>
>  #include "tpci200.h"
>  
> +static u16 tpci200_status_timeout[] = {
> +	TPCI200_A_TIMEOUT,
> +	TPCI200_B_TIMEOUT,
> +	TPCI200_C_TIMEOUT,
> +	TPCI200_D_TIMEOUT,
> +};
> +
> +static u16 tpci200_status_error[] = {
> +	TPCI200_A_ERROR,
> +	TPCI200_B_ERROR,
> +	TPCI200_C_ERROR,
> +	TPCI200_D_ERROR,
> +};
> +
>  static struct ipack_bus_ops tpci200_bus_ops;
>  
>  static int tpci200_slot_unregister(struct ipack_device *dev);
> @@ -507,6 +521,94 @@ out_unlock:
>  	return res;
>  }
>  
> +static int tpci200_get_clockrate(struct ipack_device *dev)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;

The point of the underscores in the __le16 is that you don't want to
pollute user space headers in glibc with a bunch of kernel typedefs.
It is not needed here.  (Or if it is, then we would need to replace
the u16 uses as well).

> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->control[dev->slot];
> +	return (ioread16(addr) & TPCI200_CLK32) ? 32 : 8;
> +}
> +
> +static int tpci200_set_clockrate(struct ipack_device *dev, int mherz)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;
> +	u16 reg;
> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->control[dev->slot];
> +
> +	/* Ensure the control register is not changed by another task after we
> +	 * have read it. */
> +	mutex_lock(&tpci200->mutex);
> +	reg = ioread16(addr);
> +	switch (mherz) {
> +	case 8:
> +		reg &= ~(TPCI200_CLK32); break;
> +	case 32:
> +		reg |= TPCI200_CLK32; break;

Put the breaks on the next line so that we can see them.  At first I
thought it fell through.

> +	default:
> +		mutex_unlock(&tpci200->mutex);
> +		return -EINVAL;
> +	}
> +	iowrite16(reg, addr);
> +	mutex_unlock(&tpci200->mutex);
> +	return 0;
> +}
> +
> +static int tpci200_get_error(struct ipack_device *dev)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;
> +	u16 mask;
> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->status;
> +	mask = tpci200_status_error[dev->slot];
> +	return (ioread16(addr) & mask) ? 1 : 0;
> +}
> +
> +static int tpci200_get_timeout(struct ipack_device *dev)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;
> +	u16 mask;
> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->status;
> +	mask = tpci200_status_timeout[dev->slot];
> +
> +	return (ioread16(addr) & mask) ? 1 : 0;
> +}
> +
> +static int tpci200_reset_timeout(struct ipack_device *dev)
> +{
> +	struct tpci200_board *tpci200 = check_slot(dev);
> +	__le16 __iomem *addr;
> +	u16 mask;
> +
> +	if (!tpci200)
> +		return -ENODEV;
> +
> +	addr = &tpci200->info->interface_regs->status;
> +	mask = tpci200_status_timeout[dev->slot];
> +
> +	iowrite16(mask, addr);
> +	return 0;
> +}
> +
> +
> +

Only one blank line is here.

>  static void tpci200_uninstall(struct tpci200_board *tpci200)
>  {
>  	int i;
> @@ -524,6 +626,11 @@ static struct ipack_bus_ops tpci200_bus_ops = {
>  	.request_irq = tpci200_request_irq,
>  	.free_irq = tpci200_free_irq,
>  	.remove_device = tpci200_slot_unregister,
> +	.get_clockrate = tpci200_get_clockrate,
> +	.set_clockrate = tpci200_set_clockrate,
> +	.get_error     = tpci200_get_error,
> +	.get_timeout   = tpci200_get_timeout,
> +	.reset_timeout = tpci200_reset_timeout,
>  };
>  
>  static int tpci200_install(struct tpci200_board *tpci200)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [PATCH 03/20] Staging: ipack/bridges/tpci200: provide new callbacks to tpci200

[Jens Taprogge]

On Tue, Sep 11, 2012 at 11:47:02AM +0300, Dan Carpenter wrote:
> 
> I had a few style comments on this patchset.  Nothing that couldn't
> be fixed later.
> 
> On Mon, Sep 10, 2012 at 10:51:41AM +0200, Samuel Iglesias Gonsálvez wrote:
> > From: Jens Taprogge <jens.taprogge@taprogge.org>
> > 
> > Provide get_clockrate, set_clockrate, get_error, get_timeout and reset_timeout
> > callbacks.
> > 
> > Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> > ---
> >  drivers/staging/ipack/bridges/tpci200.c |  107 +++++++++++++++++++++++++++++++
> >  1 file changed, 107 insertions(+)
> > 
> > diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
> > index 4383953..861b00d 100644
> > --- a/drivers/staging/ipack/bridges/tpci200.c
> > +++ b/drivers/staging/ipack/bridges/tpci200.c
> > @@ -14,6 +14,20 @@
> >  #include <linux/module.h>
> >  #include "tpci200.h"
> >  
> > +static u16 tpci200_status_timeout[] = {
> > +	TPCI200_A_TIMEOUT,
> > +	TPCI200_B_TIMEOUT,
> > +	TPCI200_C_TIMEOUT,
> > +	TPCI200_D_TIMEOUT,
> > +};
> > +
> > +static u16 tpci200_status_error[] = {
> > +	TPCI200_A_ERROR,
> > +	TPCI200_B_ERROR,
> > +	TPCI200_C_ERROR,
> > +	TPCI200_D_ERROR,
> > +};
> > +
> >  static struct ipack_bus_ops tpci200_bus_ops;
> >  
> >  static int tpci200_slot_unregister(struct ipack_device *dev);
> > @@ -507,6 +521,94 @@ out_unlock:
> >  	return res;
> >  }
> >  
> > +static int tpci200_get_clockrate(struct ipack_device *dev)
> > +{
> > +	struct tpci200_board *tpci200 = check_slot(dev);
> > +	__le16 __iomem *addr;
> 
> The point of the underscores in the __le16 is that you don't want to
> pollute user space headers in glibc with a bunch of kernel typedefs.
> It is not needed here.  (Or if it is, then we would need to replace
> the u16 uses as well).

I was under the impression that "__le16" is used to indicate the
byteorder of the pointed to memory.  As far as I can see that
information is lost when we use u16.  Am I missing something?

Which u16 uses are you referring to?

> 
> > +
> > +	if (!tpci200)
> > +		return -ENODEV;
> > +
> > +	addr = &tpci200->info->interface_regs->control[dev->slot];
> > +	return (ioread16(addr) & TPCI200_CLK32) ? 32 : 8;
> > +}
> > +
> > +static int tpci200_set_clockrate(struct ipack_device *dev, int mherz)
> > +{
> > +	struct tpci200_board *tpci200 = check_slot(dev);
> > +	__le16 __iomem *addr;
> > +	u16 reg;
> > +
> > +	if (!tpci200)
> > +		return -ENODEV;
> > +
> > +	addr = &tpci200->info->interface_regs->control[dev->slot];
> > +
> > +	/* Ensure the control register is not changed by another task after we
> > +	 * have read it. */
> > +	mutex_lock(&tpci200->mutex);
> > +	reg = ioread16(addr);
> > +	switch (mherz) {
> > +	case 8:
> > +		reg &= ~(TPCI200_CLK32); break;
> > +	case 32:
> > +		reg |= TPCI200_CLK32; break;
> 
> Put the breaks on the next line so that we can see them.  At first I
> thought it fell through.
> 
> > +	default:
> > +		mutex_unlock(&tpci200->mutex);
> > +		return -EINVAL;
> > +	}
> > +	iowrite16(reg, addr);
> > +	mutex_unlock(&tpci200->mutex);
> > +	return 0;
> > +}
> > +
> > +static int tpci200_get_error(struct ipack_device *dev)
> > +{
> > +	struct tpci200_board *tpci200 = check_slot(dev);
> > +	__le16 __iomem *addr;
> > +	u16 mask;
> > +
> > +	if (!tpci200)
> > +		return -ENODEV;
> > +
> > +	addr = &tpci200->info->interface_regs->status;
> > +	mask = tpci200_status_error[dev->slot];
> > +	return (ioread16(addr) & mask) ? 1 : 0;
> > +}
> > +
> > +static int tpci200_get_timeout(struct ipack_device *dev)
> > +{
> > +	struct tpci200_board *tpci200 = check_slot(dev);
> > +	__le16 __iomem *addr;
> > +	u16 mask;
> > +
> > +	if (!tpci200)
> > +		return -ENODEV;
> > +
> > +	addr = &tpci200->info->interface_regs->status;
> > +	mask = tpci200_status_timeout[dev->slot];
> > +
> > +	return (ioread16(addr) & mask) ? 1 : 0;
> > +}
> > +
> > +static int tpci200_reset_timeout(struct ipack_device *dev)
> > +{
> > +	struct tpci200_board *tpci200 = check_slot(dev);
> > +	__le16 __iomem *addr;
> > +	u16 mask;
> > +
> > +	if (!tpci200)
> > +		return -ENODEV;
> > +
> > +	addr = &tpci200->info->interface_regs->status;
> > +	mask = tpci200_status_timeout[dev->slot];
> > +
> > +	iowrite16(mask, addr);
> > +	return 0;
> > +}
> > +
> > +
> > +
> 
> Only one blank line is here.
> 
> >  static void tpci200_uninstall(struct tpci200_board *tpci200)
> >  {
> >  	int i;
> > @@ -524,6 +626,11 @@ static struct ipack_bus_ops tpci200_bus_ops = {
> >  	.request_irq = tpci200_request_irq,
> >  	.free_irq = tpci200_free_irq,
> >  	.remove_device = tpci200_slot_unregister,
> > +	.get_clockrate = tpci200_get_clockrate,
> > +	.set_clockrate = tpci200_set_clockrate,
> > +	.get_error     = tpci200_get_error,
> > +	.get_timeout   = tpci200_get_timeout,
> > +	.reset_timeout = tpci200_reset_timeout,
> >  };
> >  
> >  static int tpci200_install(struct tpci200_board *tpci200)
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > devel mailing list
> > devel@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [PATCH 03/20] Staging: ipack/bridges/tpci200: provide new callbacks to tpci200

[Dan Carpenter]

On Wed, Sep 12, 2012 at 11:28:33AM +0200, Jens Taprogge wrote:
> On Tue, Sep 11, 2012 at 11:47:02AM +0300, Dan Carpenter wrote:
> > > +static int tpci200_get_clockrate(struct ipack_device *dev)
> > > +{
> > > +	struct tpci200_board *tpci200 = check_slot(dev);
> > > +	__le16 __iomem *addr;
> > 
> > The point of the underscores in the __le16 is that you don't want to
> > pollute user space headers in glibc with a bunch of kernel typedefs.
> > It is not needed here.  (Or if it is, then we would need to replace
> > the u16 uses as well).
> 
> I was under the impression that "__le16" is used to indicate the
> byteorder of the pointed to memory.  As far as I can see that
> information is lost when we use u16.  Am I missing something?
> 

Use the no-underscore version unless it's inside a header which is
exported to userspace.

	le16 __iomem *addr;

regards,
dan carpenter

Re: [PATCH 03/20] Staging: ipack/bridges/tpci200: provide new callbacks to tpci200

[Samuel Iglesias Gonsálvez]

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

On Wed, 2012-09-12 at 14:13 +0300, Dan Carpenter wrote:
> On Wed, Sep 12, 2012 at 11:28:33AM +0200, Jens Taprogge wrote:
> > On Tue, Sep 11, 2012 at 11:47:02AM +0300, Dan Carpenter wrote:
> > > > +static int tpci200_get_clockrate(struct ipack_device *dev)
> > > > +{
> > > > +	struct tpci200_board *tpci200 = check_slot(dev);
> > > > +	__le16 __iomem *addr;
> > > 
> > > The point of the underscores in the __le16 is that you don't want to
> > > pollute user space headers in glibc with a bunch of kernel typedefs.
> > > It is not needed here.  (Or if it is, then we would need to replace
> > > the u16 uses as well).
> > 
> > I was under the impression that "__le16" is used to indicate the
> > byteorder of the pointed to memory.  As far as I can see that
> > information is lost when we use u16.  Am I missing something?
> > 
> 
> Use the no-underscore version unless it's inside a header which is
> exported to userspace.
> 
> 	le16 __iomem *addr;
> 

But it is not declared in linux/types, it is?

I have found only this typedef in a quick search:

http://lxr.linux.no/#linux+v3.5.3/fs/ntfs/types.h#L28

Should we define them in ipack.h header file or they are defined in
other place?

Best regards,

Sam

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

Re: [PATCH 03/20] Staging: ipack/bridges/tpci200: provide new callbacks to tpci200

[Dan Carpenter]

On Wed, Sep 12, 2012 at 01:21:17PM +0200, Samuel Iglesias Gonsálvez wrote:
> On Wed, 2012-09-12 at 14:13 +0300, Dan Carpenter wrote:
> > On Wed, Sep 12, 2012 at 11:28:33AM +0200, Jens Taprogge wrote:
> > > On Tue, Sep 11, 2012 at 11:47:02AM +0300, Dan Carpenter wrote:
> > > > > +static int tpci200_get_clockrate(struct ipack_device *dev)
> > > > > +{
> > > > > +	struct tpci200_board *tpci200 = check_slot(dev);
> > > > > +	__le16 __iomem *addr;
> > > > 
> > > > The point of the underscores in the __le16 is that you don't want to
> > > > pollute user space headers in glibc with a bunch of kernel typedefs.
> > > > It is not needed here.  (Or if it is, then we would need to replace
> > > > the u16 uses as well).
> > > 
> > > I was under the impression that "__le16" is used to indicate the
> > > byteorder of the pointed to memory.  As far as I can see that
> > > information is lost when we use u16.  Am I missing something?
> > > 
> > 
> > Use the no-underscore version unless it's inside a header which is
> > exported to userspace.
> > 
> > 	le16 __iomem *addr;
> > 
> 
> But it is not declared in linux/types, it is?
> 
> I have found only this typedef in a quick search:
> 
> http://lxr.linux.no/#linux+v3.5.3/fs/ntfs/types.h#L28
> 
> Should we define them in ipack.h header file or they are defined in
> other place?
> 

Oh crap!  You're right.  That's embarrassing.  I'm just totally
wrong here.  Sorry for that.

regards,
dan carpenter

Re: [PATCH 03/20] Staging: ipack/bridges/tpci200: provide new callbacks to tpci200

[Jens Taprogge]

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

On Wed, Sep 12, 2012 at 02:59:26PM +0300, Dan Carpenter wrote:
> On Wed, Sep 12, 2012 at 01:21:17PM +0200, Samuel Iglesias Gonsálvez wrote:
> > On Wed, 2012-09-12 at 14:13 +0300, Dan Carpenter wrote:
> > > On Wed, Sep 12, 2012 at 11:28:33AM +0200, Jens Taprogge wrote:
> > > > On Tue, Sep 11, 2012 at 11:47:02AM +0300, Dan Carpenter wrote:
> > > > > > +static int tpci200_get_clockrate(struct ipack_device *dev)
> > > > > > +{
> > > > > > +	struct tpci200_board *tpci200 = check_slot(dev);
> > > > > > +	__le16 __iomem *addr;
> > > > > 
> > > > > The point of the underscores in the __le16 is that you don't want to
> > > > > pollute user space headers in glibc with a bunch of kernel typedefs.
> > > > > It is not needed here.  (Or if it is, then we would need to replace
> > > > > the u16 uses as well).
> > > > 
> > > > I was under the impression that "__le16" is used to indicate the
> > > > byteorder of the pointed to memory.  As far as I can see that
> > > > information is lost when we use u16.  Am I missing something?
> > > > 
> > > 
> > > Use the no-underscore version unless it's inside a header which is
> > > exported to userspace.
> > > 
> > > 	le16 __iomem *addr;
> > > 
> > 
> > But it is not declared in linux/types, it is?
> > 
> > I have found only this typedef in a quick search:
> > 
> > http://lxr.linux.no/#linux+v3.5.3/fs/ntfs/types.h#L28
> > 
> > Should we define them in ipack.h header file or they are defined in
> > other place?
> > 
> 
> Oh crap!  You're right.  That's embarrassing.  I'm just totally
> wrong here.  Sorry for that.
> 
> regards,
> dan carpenter

Sam,

can you please add the endianess-aware versions back?  Patch attached.

One of the earlier uses was not correct.

Best Regards,
Jens

[-- Attachment #2: 0001-staging-ipack-bridges-tpci200-Use-endianess-aware-ty.patch --]
[-- Type: text/x-diff, Size: 2613 bytes --]

>From 3ed4dd686dec8b8b6fed1a35752daf4525002d0c Mon Sep 17 00:00:00 2001
From: Jens Taprogge <jens.taprogge@taprogge.org>
Date: Wed, 12 Sep 2012 14:25:25 +0200
Subject: [PATCH] staging: ipack/bridges/tpci200: Use endianess-aware types
 where applicable.

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
---
 drivers/staging/ipack/bridges/tpci200.c |   10 +++++-----
 drivers/staging/ipack/bridges/tpci200.h |    8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 5341aa8..af1cac8 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -357,7 +357,7 @@ out_disable_pci:
 static int tpci200_get_clockrate(struct ipack_device *dev)
 {
 	struct tpci200_board *tpci200 = check_slot(dev);
-	u16 __iomem *addr;
+	__le16 __iomem *addr;
 
 	if (!tpci200)
 		return -ENODEV;
@@ -369,7 +369,7 @@ static int tpci200_get_clockrate(struct ipack_device *dev)
 static int tpci200_set_clockrate(struct ipack_device *dev, int mherz)
 {
 	struct tpci200_board *tpci200 = check_slot(dev);
-	u16 __iomem *addr;
+	__le16 __iomem *addr;
 
 	if (!tpci200)
 		return -ENODEV;
@@ -392,7 +392,7 @@ static int tpci200_set_clockrate(struct ipack_device *dev, int mherz)
 static int tpci200_get_error(struct ipack_device *dev)
 {
 	struct tpci200_board *tpci200 = check_slot(dev);
-	u16 __iomem *addr;
+	__le16 __iomem *addr;
 	u16 mask;
 
 	if (!tpci200)
@@ -406,7 +406,7 @@ static int tpci200_get_error(struct ipack_device *dev)
 static int tpci200_get_timeout(struct ipack_device *dev)
 {
 	struct tpci200_board *tpci200 = check_slot(dev);
-	u16 __iomem *addr;
+	__le16 __iomem *addr;
 	u16 mask;
 
 	if (!tpci200)
@@ -421,7 +421,7 @@ static int tpci200_get_timeout(struct ipack_device *dev)
 static int tpci200_reset_timeout(struct ipack_device *dev)
 {
 	struct tpci200_board *tpci200 = check_slot(dev);
-	u16 __iomem *addr;
+	__le16 __iomem *addr;
 	u16 mask;
 
 	if (!tpci200)
diff --git a/drivers/staging/ipack/bridges/tpci200.h b/drivers/staging/ipack/bridges/tpci200.h
index 3327025..982f319 100644
--- a/drivers/staging/ipack/bridges/tpci200.h
+++ b/drivers/staging/ipack/bridges/tpci200.h
@@ -37,12 +37,12 @@
 #define TPCI200_MEM8_SPACE_BAR        5
 
 struct tpci200_regs {
-	u16	revision;
+	__le16	revision;
 	/* writes to control should occur with the mutex held to protect
 	 * read-modify-write operations */
-	u16  control[4];
-	u16	reset;
-	u16	status;
+	__le16  control[4];
+	__le16	reset;
+	__le16	status;
 	u8	reserved[242];
 } __packed;
 
-- 
1.7.9.5

Re: [PATCH 03/20] Staging: ipack/bridges/tpci200: provide new callbacks to tpci200

[Samuel Iglesias Gonsálvez]

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

On Wed, 2012-09-12 at 14:28 +0200, Jens Taprogge wrote:
[...]
> Sam,
> 
> can you please add the endianess-aware versions back?  Patch attached.
> 
> One of the earlier uses was not correct.

Done! 

The patch will be submitted with the next batch.

Thanks, 

Sam

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

Re: [PATCH 03/20] Staging: ipack/bridges/tpci200: provide new callbacks to tpci200

[Jens Taprogge]

On Wed, Sep 12, 2012 at 02:13:30PM +0300, Dan Carpenter wrote:
> On Wed, Sep 12, 2012 at 11:28:33AM +0200, Jens Taprogge wrote:
> > On Tue, Sep 11, 2012 at 11:47:02AM +0300, Dan Carpenter wrote:
> > > > +static int tpci200_get_clockrate(struct ipack_device *dev)
> > > > +{
> > > > +	struct tpci200_board *tpci200 = check_slot(dev);
> > > > +	__le16 __iomem *addr;
> > > 
> > > The point of the underscores in the __le16 is that you don't want to
> > > pollute user space headers in glibc with a bunch of kernel typedefs.
> > > It is not needed here.  (Or if it is, then we would need to replace
> > > the u16 uses as well).
> > 
> > I was under the impression that "__le16" is used to indicate the
> > byteorder of the pointed to memory.  As far as I can see that
> > information is lost when we use u16.  Am I missing something?
> > 
> 
> Use the no-underscore version unless it's inside a header which is
> exported to userspace.
> 
> 	le16 __iomem *addr;
> 
> regards,
> dan carpenter

Doing a quick
  grep " le16 "
in drivers I have not found a single occurance that is not within 
a comment.  There are plenty of __le16 occurances though.

Best Regards,
Jens

[PATCH 04/20] Staging: ipack: Obtain supported speeds from ID ROM.

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/ipack.c |    6 ++++++
 drivers/staging/ipack/ipack.h |    2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
index b3736c0..521ff55 100644
--- a/drivers/staging/ipack/ipack.c
+++ b/drivers/staging/ipack/ipack.c
@@ -261,15 +261,21 @@ static void ipack_parse_id1(struct ipack_device *dev)
 
 	dev->id_vendor = id[4];
 	dev->id_device = id[5];
+	dev->speed_8mhz = 1;
+	dev->speed_32mhz = (id[7] == 'H');
 }
 
 static void ipack_parse_id2(struct ipack_device *dev)
 {
 	__be16 *id = (__be16 *) dev->id;
+	u16 flags;
 
 	dev->id_vendor = ((be16_to_cpu(id[3]) & 0xff) << 16)
 			 + be16_to_cpu(id[4]);
 	dev->id_device = be16_to_cpu(id[5]);
+	flags = be16_to_cpu(id[10]);
+	dev->speed_8mhz = ((flags & 2) != 0);
+	dev->speed_32mhz = ((flags & 4) != 0);
 }
 
 static int ipack_device_read_id(struct ipack_device *dev)
diff --git a/drivers/staging/ipack/ipack.h b/drivers/staging/ipack/ipack.h
index 0526613..cb77494 100644
--- a/drivers/staging/ipack/ipack.h
+++ b/drivers/staging/ipack/ipack.h
@@ -79,6 +79,8 @@ struct ipack_device {
 	u32			 id_vendor;
 	u32			 id_device;
 	u8			 id_format;
+	unsigned int		 speed_8mhz:1;
+	unsigned int		 speed_32mhz:1;
 };
 
 /**
-- 
1.7.10.4

Re: [PATCH 04/20] Staging: ipack: Obtain supported speeds from ID ROM.

[Dan Carpenter]

On Mon, Sep 10, 2012 at 10:51:42AM +0200, Samuel Iglesias Gonsálvez wrote:
> From: Jens Taprogge <jens.taprogge@taprogge.org>
> 
> Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> ---
>  drivers/staging/ipack/ipack.c |    6 ++++++
>  drivers/staging/ipack/ipack.h |    2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
> index b3736c0..521ff55 100644
> --- a/drivers/staging/ipack/ipack.c
> +++ b/drivers/staging/ipack/ipack.c
> @@ -261,15 +261,21 @@ static void ipack_parse_id1(struct ipack_device *dev)
>  
>  	dev->id_vendor = id[4];
>  	dev->id_device = id[5];
> +	dev->speed_8mhz = 1;
> +	dev->speed_32mhz = (id[7] == 'H');
>  }
>  
>  static void ipack_parse_id2(struct ipack_device *dev)
>  {
>  	__be16 *id = (__be16 *) dev->id;
> +	u16 flags;
>  
>  	dev->id_vendor = ((be16_to_cpu(id[3]) & 0xff) << 16)
>  			 + be16_to_cpu(id[4]);
>  	dev->id_device = be16_to_cpu(id[5]);
> +	flags = be16_to_cpu(id[10]);
> +	dev->speed_8mhz = ((flags & 2) != 0);
> +	dev->speed_32mhz = ((flags & 4) != 0);

I really dislike "!= 0" double negatives.  I would prefer this:

	dev->speed_8mhz = !!(flags & 2);
	dev->speed_32mhz = !!(flags & 4);

For me the !! is idiomatic and means turn this into a bool.  I don't
know if maybe I'm the only person who feels this way.

regards,
dan carpenter

[PATCH 05/20] Staging: ipack: Choose the optimum bus speed by default.

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/ipack.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
index 521ff55..6e0b441 100644
--- a/drivers/staging/ipack/ipack.c
+++ b/drivers/staging/ipack/ipack.c
@@ -386,6 +386,11 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
 		return NULL;
 	}
 
+	/* if the device supports 32 MHz operation, use it. */
+	ret = bus->ops->set_clockrate(dev, dev->speed_32mhz ? 32 : 8);
+	if (ret < 0)
+		dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
+
 	ret = device_register(&dev->dev);
 	if (ret < 0) {
 		kfree(dev->id);
-- 
1.7.10.4

Re: [PATCH 05/20] Staging: ipack: Choose the optimum bus speed by default.

[Dan Carpenter]

On Mon, Sep 10, 2012 at 10:51:43AM +0200, Samuel Iglesias Gonsálvez wrote:
> From: Jens Taprogge <jens.taprogge@taprogge.org>
> 
> Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> ---
>  drivers/staging/ipack/ipack.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
> index 521ff55..6e0b441 100644
> --- a/drivers/staging/ipack/ipack.c
> +++ b/drivers/staging/ipack/ipack.c
> @@ -386,6 +386,11 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
>  		return NULL;
>  	}
>  
> +	/* if the device supports 32 MHz operation, use it. */
> +	ret = bus->ops->set_clockrate(dev, dev->speed_32mhz ? 32 : 8);
> +	if (ret < 0)
> +		dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");

It's strange that we don't test the ->speed_8mhz flag.  Do we even
need that flag?  It might be better to write it like this:

	if (dev->speed_32mhz) {
		ret = bus->ops->set_clockrate(dev, 32);
		 if (ret < 0) {
		 	dev_err(&dev->dev,
			        "failed to switch to 32 MHz operation.\n");
		}
	}

The error message is not accurate in the original code because it
says 32 even if you tried to set it to 8.

regards,
dan carpenter

[PATCH 06/20] Staging: ipack: remove field driver from struct ipack_device.

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

After a successful match is found the driver field in struct device is
set by the core device code. We can use this field.

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/ipack.c |   17 +++++++----------
 drivers/staging/ipack/ipack.h |    2 --
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
index 6e0b441..4f3c945 100644
--- a/drivers/staging/ipack/ipack.c
+++ b/drivers/staging/ipack/ipack.c
@@ -58,32 +58,29 @@ static int ipack_bus_match(struct device *dev, struct device_driver *drv)
 	const struct ipack_device_id *found_id;
 
 	found_id = ipack_match_id(idrv->id_table, idev);
-	if (found_id) {
-		idev->driver = idrv;
-		return 1;
-	}
-
-	return 0;
+	return found_id ? 1 : 0;
 }
 
 static int ipack_bus_probe(struct device *device)
 {
 	struct ipack_device *dev = to_ipack_dev(device);
+	struct ipack_driver *drv = to_ipack_driver(device->driver);
 
-	if (!dev->driver->ops->probe)
+	if (!drv->ops->probe)
 		return -EINVAL;
 
-	return dev->driver->ops->probe(dev);
+	return drv->ops->probe(dev);
 }
 
 static int ipack_bus_remove(struct device *device)
 {
 	struct ipack_device *dev = to_ipack_dev(device);
+	struct ipack_driver *drv = to_ipack_driver(device->driver);
 
-	if (!dev->driver->ops->remove)
+	if (!drv->ops->remove)
 		return -EINVAL;
 
-	dev->driver->ops->remove(dev);
+	drv->ops->remove(dev);
 	return 0;
 }
 
diff --git a/drivers/staging/ipack/ipack.h b/drivers/staging/ipack/ipack.h
index cb77494..d3be8ba67 100644
--- a/drivers/staging/ipack/ipack.h
+++ b/drivers/staging/ipack/ipack.h
@@ -53,7 +53,6 @@ struct ipack_addr_space {
  *	@bus_nr: IP bus number where the device is plugged
  *	@slot: Slot where the device is plugged in the carrier board
  *	@irq: IRQ vector
- *	@driver: Pointer to the ipack_driver that manages the device
  *	@bus: ipack_bus_device where the device is plugged to.
  *	@id_space: Virtual address to ID space.
  *	@io_space: Virtual address to IO space.
@@ -68,7 +67,6 @@ struct ipack_device {
 	unsigned int bus_nr;
 	unsigned int slot;
 	unsigned int irq;
-	struct ipack_driver *driver;
 	struct ipack_bus_device *bus;
 	struct ipack_addr_space id_space;
 	struct ipack_addr_space io_space;
-- 
1.7.10.4

[PATCH 07/20] Staging: ipack/bridges/tpci200: remove struct list_head

[Samuel Iglesias Gonsálvez]

As the linked list was removed before, delete the useless struct list_head

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.h |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.h b/drivers/staging/ipack/bridges/tpci200.h
index af48295..0fc1900 100644
--- a/drivers/staging/ipack/bridges/tpci200.h
+++ b/drivers/staging/ipack/bridges/tpci200.h
@@ -161,7 +161,6 @@ struct tpci200_infos {
 	struct ipack_bus_device		*ipack_bus;
 };
 struct tpci200_board {
-	struct list_head	list;
 	unsigned int		number;
 	struct mutex		mutex;
 	struct tpci200_slot	*slots;
-- 
1.7.10.4

[PATCH 08/20] Staging: ipack: Switch to 8MHz operation before reading ID.

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

Reading the ID space at 8 MHz is always supported.  Most carriers will
boot up in 8MHz mode.  Still, play it safe and ensure we are operating at
8Mhz.

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/ipack.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
index 4f3c945..95f56b8 100644
--- a/drivers/staging/ipack/ipack.c
+++ b/drivers/staging/ipack/ipack.c
@@ -376,6 +376,9 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
 	dev_set_name(&dev->dev,
 		     "ipack-dev.%u.%u", dev->bus_nr, dev->slot);
 
+	if (bus->ops->set_clockrate(dev, 8))
+		dev_warn(&dev->dev, "failed to switch to 8 MHz operation for reading of device ID.\n");
+
 	ret = ipack_device_read_id(dev);
 	if (ret < 0) {
 		dev_err(&dev->dev, "error reading device id section.\n");
@@ -384,9 +387,11 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
 	}
 
 	/* if the device supports 32 MHz operation, use it. */
-	ret = bus->ops->set_clockrate(dev, dev->speed_32mhz ? 32 : 8);
-	if (ret < 0)
-		dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
+	if (dev->speed_32mhz) {
+		ret = bus->ops->set_clockrate(dev, 32);
+		if (ret < 0)
+			dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
+	}
 
 	ret = device_register(&dev->dev);
 	if (ret < 0) {
-- 
1.7.10.4

Re: [PATCH 08/20] Staging: ipack: Switch to 8MHz operation before reading ID.

[Dan Carpenter]

On Mon, Sep 10, 2012 at 10:51:46AM +0200, Samuel Iglesias Gonsálvez wrote:
> From: Jens Taprogge <jens.taprogge@taprogge.org>
> 
> Reading the ID space at 8 MHz is always supported.  Most carriers will
> boot up in 8MHz mode.  Still, play it safe and ensure we are operating at
> 8Mhz.
> 
> Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> ---
>  drivers/staging/ipack/ipack.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
> index 4f3c945..95f56b8 100644
> --- a/drivers/staging/ipack/ipack.c
> +++ b/drivers/staging/ipack/ipack.c
> @@ -376,6 +376,9 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
>  	dev_set_name(&dev->dev,
>  		     "ipack-dev.%u.%u", dev->bus_nr, dev->slot);
>  
> +	if (bus->ops->set_clockrate(dev, 8))
> +		dev_warn(&dev->dev, "failed to switch to 8 MHz operation for reading of device ID.\n");
> +
>  	ret = ipack_device_read_id(dev);
>  	if (ret < 0) {
>  		dev_err(&dev->dev, "error reading device id section.\n");
> @@ -384,9 +387,11 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
>  	}
>  
>  	/* if the device supports 32 MHz operation, use it. */
> -	ret = bus->ops->set_clockrate(dev, dev->speed_32mhz ? 32 : 8);
> -	if (ret < 0)
> -		dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> +	if (dev->speed_32mhz) {
> +		ret = bus->ops->set_clockrate(dev, 32);
> +		if (ret < 0)
> +			dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> +	}

Ah.  Well done.  That's what I suggested earlier.  I think you
should get rid of the ->speed_8mhz all together.

regards,
dan carpenter

Re: [PATCH 08/20] Staging: ipack: Switch to 8MHz operation before reading ID.

[Samuel Iglesias Gonsálvez]

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

On Tue, 2012-09-11 at 11:48 +0300, Dan Carpenter wrote:
> On Mon, Sep 10, 2012 at 10:51:46AM +0200, Samuel Iglesias Gonsálvez wrote:
> > From: Jens Taprogge <jens.taprogge@taprogge.org>
> > 
> > Reading the ID space at 8 MHz is always supported.  Most carriers will
> > boot up in 8MHz mode.  Still, play it safe and ensure we are operating at
> > 8Mhz.
> > 
> > Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> > ---
> >  drivers/staging/ipack/ipack.c |   11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
> > index 4f3c945..95f56b8 100644
> > --- a/drivers/staging/ipack/ipack.c
> > +++ b/drivers/staging/ipack/ipack.c
> > @@ -376,6 +376,9 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
> >  	dev_set_name(&dev->dev,
> >  		     "ipack-dev.%u.%u", dev->bus_nr, dev->slot);
> >  
> > +	if (bus->ops->set_clockrate(dev, 8))
> > +		dev_warn(&dev->dev, "failed to switch to 8 MHz operation for reading of device ID.\n");
> > +
> >  	ret = ipack_device_read_id(dev);
> >  	if (ret < 0) {
> >  		dev_err(&dev->dev, "error reading device id section.\n");
> > @@ -384,9 +387,11 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
> >  	}
> >  
> >  	/* if the device supports 32 MHz operation, use it. */
> > -	ret = bus->ops->set_clockrate(dev, dev->speed_32mhz ? 32 : 8);
> > -	if (ret < 0)
> > -		dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> > +	if (dev->speed_32mhz) {
> > +		ret = bus->ops->set_clockrate(dev, 32);
> > +		if (ret < 0)
> > +			dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> > +	}
> 
> Ah.  Well done.  That's what I suggested earlier.  I think you
> should get rid of the ->speed_8mhz all together.

I will do it in a follow-up patch. The rest of suggestions have been
integrated in the patches. I will submit the patches very soon.

Thanks,

Sam 


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

Re: [PATCH 08/20] Staging: ipack: Switch to 8MHz operation before reading ID.

[Jens Taprogge]

On Tue, Sep 11, 2012 at 01:31:06PM +0200, Samuel Iglesias Gonsálvez wrote:
> On Tue, 2012-09-11 at 11:48 +0300, Dan Carpenter wrote:
> > On Mon, Sep 10, 2012 at 10:51:46AM +0200, Samuel Iglesias Gonsálvez wrote:
> > > From: Jens Taprogge <jens.taprogge@taprogge.org>
> > > 
> > > Reading the ID space at 8 MHz is always supported.  Most carriers will
> > > boot up in 8MHz mode.  Still, play it safe and ensure we are operating at
> > > 8Mhz.
> > > 
> > > Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> > > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> > > ---
> > >  drivers/staging/ipack/ipack.c |   11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
> > > index 4f3c945..95f56b8 100644
> > > --- a/drivers/staging/ipack/ipack.c
> > > +++ b/drivers/staging/ipack/ipack.c
> > > @@ -376,6 +376,9 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
> > >  	dev_set_name(&dev->dev,
> > >  		     "ipack-dev.%u.%u", dev->bus_nr, dev->slot);
> > >  
> > > +	if (bus->ops->set_clockrate(dev, 8))
> > > +		dev_warn(&dev->dev, "failed to switch to 8 MHz operation for reading of device ID.\n");
> > > +
> > >  	ret = ipack_device_read_id(dev);
> > >  	if (ret < 0) {
> > >  		dev_err(&dev->dev, "error reading device id section.\n");
> > > @@ -384,9 +387,11 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
> > >  	}
> > >  
> > >  	/* if the device supports 32 MHz operation, use it. */
> > > -	ret = bus->ops->set_clockrate(dev, dev->speed_32mhz ? 32 : 8);
> > > -	if (ret < 0)
> > > -		dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> > > +	if (dev->speed_32mhz) {
> > > +		ret = bus->ops->set_clockrate(dev, 32);
> > > +		if (ret < 0)
> > > +			dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> > > +	}
> > 
> > Ah.  Well done.  That's what I suggested earlier.  I think you
> > should get rid of the ->speed_8mhz all together.
> 
> I will do it in a follow-up patch. The rest of suggestions have been
> integrated in the patches. I will submit the patches very soon.

I have not seen any IPack V2 device yet.  But since the ID space
provides the 8MHz flag, it might be useful.  On the other hand, we can
still readd it when we see it.

When you remove the flag, you can perhaps leave a comment in the code so
that it is easy to re-enable.

Best Regards,
Jens

Re: [PATCH 08/20] Staging: ipack: Switch to 8MHz operation before reading ID.

[Samuel Iglesias Gonsálvez]

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

On Tue, 2012-09-11 at 13:39 +0200, Jens Taprogge wrote:
> On Tue, Sep 11, 2012 at 01:31:06PM +0200, Samuel Iglesias Gonsálvez wrote:
> > On Tue, 2012-09-11 at 11:48 +0300, Dan Carpenter wrote:
> > > On Mon, Sep 10, 2012 at 10:51:46AM +0200, Samuel Iglesias Gonsálvez wrote:
> > > > From: Jens Taprogge <jens.taprogge@taprogge.org>
> > > > 
> > > > Reading the ID space at 8 MHz is always supported.  Most carriers will
> > > > boot up in 8MHz mode.  Still, play it safe and ensure we are operating at
> > > > 8Mhz.
> > > > 
> > > > Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> > > > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> > > > ---
> > > >  drivers/staging/ipack/ipack.c |   11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
> > > > index 4f3c945..95f56b8 100644
> > > > --- a/drivers/staging/ipack/ipack.c
> > > > +++ b/drivers/staging/ipack/ipack.c
> > > > @@ -376,6 +376,9 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
> > > >  	dev_set_name(&dev->dev,
> > > >  		     "ipack-dev.%u.%u", dev->bus_nr, dev->slot);
> > > >  
> > > > +	if (bus->ops->set_clockrate(dev, 8))
> > > > +		dev_warn(&dev->dev, "failed to switch to 8 MHz operation for reading of device ID.\n");
> > > > +
> > > >  	ret = ipack_device_read_id(dev);
> > > >  	if (ret < 0) {
> > > >  		dev_err(&dev->dev, "error reading device id section.\n");
> > > > @@ -384,9 +387,11 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
> > > >  	}
> > > >  
> > > >  	/* if the device supports 32 MHz operation, use it. */
> > > > -	ret = bus->ops->set_clockrate(dev, dev->speed_32mhz ? 32 : 8);
> > > > -	if (ret < 0)
> > > > -		dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> > > > +	if (dev->speed_32mhz) {
> > > > +		ret = bus->ops->set_clockrate(dev, 32);
> > > > +		if (ret < 0)
> > > > +			dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> > > > +	}
> > > 
> > > Ah.  Well done.  That's what I suggested earlier.  I think you
> > > should get rid of the ->speed_8mhz all together.
> > 
> > I will do it in a follow-up patch. The rest of suggestions have been
> > integrated in the patches. I will submit the patches very soon.
> 
> I have not seen any IPack V2 device yet.  But since the ID space
> provides the 8MHz flag, it might be useful.  On the other hand, we can
> still readd it when we see it.
> 

If we have the flag, we should use it in some point. Not only save it in
the variable speed_8mhz and don't use it at all.

> When you remove the flag, you can perhaps leave a comment in the code so
> that it is easy to re-enable.
> 

OK. I will send a patch in Industry Pack mailing list and we can discuss
there if this is a good solution or not before sending to staging ML.

Thanks,

Sam


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

Re: [PATCH 08/20] Staging: ipack: Switch to 8MHz operation before reading ID.

[Dan Carpenter]

On Tue, Sep 11, 2012 at 04:39:37PM +0200, Samuel Iglesias Gonsálvez wrote:
> On Tue, 2012-09-11 at 13:39 +0200, Jens Taprogge wrote:
> > On Tue, Sep 11, 2012 at 01:31:06PM +0200, Samuel Iglesias Gonsálvez wrote:
> > > On Tue, 2012-09-11 at 11:48 +0300, Dan Carpenter wrote:
> > > > On Mon, Sep 10, 2012 at 10:51:46AM +0200, Samuel Iglesias Gonsálvez wrote:
> > > > > From: Jens Taprogge <jens.taprogge@taprogge.org>
> > > > > 
> > > > > Reading the ID space at 8 MHz is always supported.  Most carriers will
> > > > > boot up in 8MHz mode.  Still, play it safe and ensure we are operating at
> > > > > 8Mhz.
> > > > > 
> > > > > Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> > > > > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> > > > > ---
> > > > >  drivers/staging/ipack/ipack.c |   11 ++++++++---
> > > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
> > > > > index 4f3c945..95f56b8 100644
> > > > > --- a/drivers/staging/ipack/ipack.c
> > > > > +++ b/drivers/staging/ipack/ipack.c
> > > > > @@ -376,6 +376,9 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
> > > > >  	dev_set_name(&dev->dev,
> > > > >  		     "ipack-dev.%u.%u", dev->bus_nr, dev->slot);
> > > > >  
> > > > > +	if (bus->ops->set_clockrate(dev, 8))
> > > > > +		dev_warn(&dev->dev, "failed to switch to 8 MHz operation for reading of device ID.\n");
> > > > > +
> > > > >  	ret = ipack_device_read_id(dev);
> > > > >  	if (ret < 0) {
> > > > >  		dev_err(&dev->dev, "error reading device id section.\n");
> > > > > @@ -384,9 +387,11 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
> > > > >  	}
> > > > >  
> > > > >  	/* if the device supports 32 MHz operation, use it. */
> > > > > -	ret = bus->ops->set_clockrate(dev, dev->speed_32mhz ? 32 : 8);
> > > > > -	if (ret < 0)
> > > > > -		dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> > > > > +	if (dev->speed_32mhz) {
> > > > > +		ret = bus->ops->set_clockrate(dev, 32);
> > > > > +		if (ret < 0)
> > > > > +			dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
> > > > > +	}
> > > > 
> > > > Ah.  Well done.  That's what I suggested earlier.  I think you
> > > > should get rid of the ->speed_8mhz all together.
> > > 
> > > I will do it in a follow-up patch. The rest of suggestions have been
> > > integrated in the patches. I will submit the patches very soon.
> > 
> > I have not seen any IPack V2 device yet.  But since the ID space
> > provides the 8MHz flag, it might be useful.  On the other hand, we can
> > still readd it when we see it.
> > 
> 
> If we have the flag, we should use it in some point. Not only save it in
> the variable speed_8mhz and don't use it at all.
> 
> > When you remove the flag, you can perhaps leave a comment in the code so
> > that it is easy to re-enable.
> > 
> 
> OK. I will send a patch in Industry Pack mailing list and we can discuss
> there if this is a good solution or not before sending to staging ML.

People worry too much about things they might need in the future.
This email thread is there on google.  That's the first place people
will look for the information if we need it.

The rest of my inbox today is full of people (Aaro and Hartley)
removing write only variables that where we probed the hardware
for all kinds interesting stuff and don't use it.

regards,
dan carpenter

[PATCH 09/20] Staging: ipack: reset previous timeouts during device registration.

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/ipack.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
index 95f56b8..eba2021 100644
--- a/drivers/staging/ipack/ipack.c
+++ b/drivers/staging/ipack/ipack.c
@@ -378,6 +378,8 @@ struct ipack_device *ipack_device_register(struct ipack_bus_device *bus,
 
 	if (bus->ops->set_clockrate(dev, 8))
 		dev_warn(&dev->dev, "failed to switch to 8 MHz operation for reading of device ID.\n");
+	if (bus->ops->reset_timeout(dev))
+		dev_warn(&dev->dev, "failed to reset potential timeout.");
 
 	ret = ipack_device_read_id(dev);
 	if (ret < 0) {
-- 
1.7.10.4

Re: [PATCH 09/20] Staging: ipack: reset previous timeouts during device registration.

[Dan Carpenter]

On Mon, Sep 10, 2012 at 10:51:47AM +0200, Samuel Iglesias Gonsálvez wrote:
> From: Jens Taprogge <jens.taprogge@taprogge.org>
> 
> Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>

Please explain the user visible effects of this bug fix in the
changelog.  *grumble*.

regards,
dan carpenter

[PATCH 10/20] Staging: ipack: check the device ID space CRC.

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

We check the CRC and store the result of the check in struct ipac_device.
A warning is emitted if the check fails.  However we leave it to the
IPack module device to refuse to initialize due to a bad CRC.  I have seen
otherwise good modules with bad CRCs.

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/ipack.c |   59 ++++++++++++++++++++++++++++++++++++++++-
 drivers/staging/ipack/ipack.h |    1 +
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
index eba2021..b368a26 100644
--- a/drivers/staging/ipack/ipack.c
+++ b/drivers/staging/ipack/ipack.c
@@ -252,20 +252,71 @@ void ipack_driver_unregister(struct ipack_driver *edrv)
 }
 EXPORT_SYMBOL_GPL(ipack_driver_unregister);
 
+static u16 ipack_crc_byte(u16 crc, u8 c)
+{
+	int i;
+
+	crc ^= c << 8;
+	for (i = 0; i < 8; i++)
+		crc = (crc << 1) ^ ((crc & 0x8000) ? 0x1021 : 0);
+	return crc;
+}
+
+/*
+ * The algorithm in lib/crc-ccitt.c does not seem to apply since it uses the
+ * opposite bit ordering.
+ */
+static u8 ipack_calc_crc1(struct ipack_device *dev)
+{
+	u8 c;
+	u16 crc;
+	unsigned int i;
+
+	crc = 0xffff;
+	for (i = 0; i < dev->id_avail; i++) {
+		c = (i != 11) ? dev->id[i] : 0;
+		crc = ipack_crc_byte(crc, c);
+	}
+	crc = ~crc;
+	return crc & 0xff;
+}
+
+static u16 ipack_calc_crc2(struct ipack_device *dev)
+{
+	u8 c;
+	u16 crc;
+	unsigned int i;
+
+	crc = 0xffff;
+	for (i = 0; i < dev->id_avail; i++) {
+		c = ((i != 0x18) && (i != 0x19)) ? dev->id[i] : 0;
+		crc = ipack_crc_byte(crc, c);
+	}
+	crc = ~crc;
+	return crc;
+}
+
 static void ipack_parse_id1(struct ipack_device *dev)
 {
 	u8 *id = dev->id;
+	u8 crc;
 
 	dev->id_vendor = id[4];
 	dev->id_device = id[5];
 	dev->speed_8mhz = 1;
 	dev->speed_32mhz = (id[7] == 'H');
+	crc = ipack_calc_crc1(dev);
+	dev->id_crc_correct = (crc == id[11]);
+	if (!dev->id_crc_correct) {
+		dev_warn(&dev->dev, "ID CRC invalid found 0x%x, expected 0x%x.\n",
+				id[11], crc);
+	}
 }
 
 static void ipack_parse_id2(struct ipack_device *dev)
 {
 	__be16 *id = (__be16 *) dev->id;
-	u16 flags;
+	u16 flags, crc;
 
 	dev->id_vendor = ((be16_to_cpu(id[3]) & 0xff) << 16)
 			 + be16_to_cpu(id[4]);
@@ -273,6 +324,12 @@ static void ipack_parse_id2(struct ipack_device *dev)
 	flags = be16_to_cpu(id[10]);
 	dev->speed_8mhz = ((flags & 2) != 0);
 	dev->speed_32mhz = ((flags & 4) != 0);
+	crc = ipack_calc_crc2(dev);
+	dev->id_crc_correct = (crc == be16_to_cpu(id[12]));
+	if (!dev->id_crc_correct) {
+		dev_warn(&dev->dev, "ID CRC invalid found 0x%x, expected 0x%x.\n",
+				id[11], crc);
+	}
 }
 
 static int ipack_device_read_id(struct ipack_device *dev)
diff --git a/drivers/staging/ipack/ipack.h b/drivers/staging/ipack/ipack.h
index d3be8ba67..e2cbf4a 100644
--- a/drivers/staging/ipack/ipack.h
+++ b/drivers/staging/ipack/ipack.h
@@ -77,6 +77,7 @@ struct ipack_device {
 	u32			 id_vendor;
 	u32			 id_device;
 	u8			 id_format;
+	unsigned int		 id_crc_correct:1;
 	unsigned int		 speed_8mhz:1;
 	unsigned int		 speed_32mhz:1;
 };
-- 
1.7.10.4

[PATCH 11/20] Staging: ipack/bridges/tpci200: reorder the iounmap and pci_release_region

[Samuel Iglesias Gonsálvez]

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 861b00d..5150794 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -66,10 +66,12 @@ static void tpci200_unregister(struct tpci200_board *tpci200)
 	pci_iounmap(tpci200->info->pdev, tpci200->info->interface_regs);
 	pci_iounmap(tpci200->info->pdev, tpci200->info->ioidint_space);
 	pci_iounmap(tpci200->info->pdev, tpci200->info->mem8_space);
+	pci_iounmap(tpci200->info->pdev, tpci200->info->cfg_regs);
 
 	pci_release_region(tpci200->info->pdev, TPCI200_IP_INTERFACE_BAR);
 	pci_release_region(tpci200->info->pdev, TPCI200_IO_ID_INT_SPACES_BAR);
 	pci_release_region(tpci200->info->pdev, TPCI200_MEM8_SPACE_BAR);
+	pci_release_region(tpci200->info->pdev, TPCI200_CFG_MEM_BAR);
 
 	pci_disable_device(tpci200->info->pdev);
 	pci_dev_put(tpci200->info->pdev);
@@ -752,9 +754,6 @@ static void __tpci200_pci_remove(struct tpci200_board *tpci200)
 	tpci200_uninstall(tpci200);
 	ipack_bus_unregister(tpci200->info->ipack_bus);
 
-	iounmap(tpci200->info->cfg_regs);
-	pci_release_region(tpci200->info->pdev, TPCI200_CFG_MEM_BAR);
-
 	kfree(tpci200->info);
 	kfree(tpci200);
 }
-- 
1.7.10.4

Re: [PATCH 11/20] Staging: ipack/bridges/tpci200: reorder the iounmap and pci_release_region

[Dan Carpenter]

On Mon, Sep 10, 2012 at 10:51:49AM +0200, Samuel Iglesias Gonsálvez wrote:
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>

The change log says what you are doing (although it's not 100%
accurate), but it doesn't say why.  I assume this is a bugfix as
well?  Please describe the user visible effects of this change in
the changelog.

regards,
dan carpenter

[PATCH 12/20] Staging: ipack/bridges/tpci200: increment the reference counter of the pci_dev

[Samuel Iglesias Gonsálvez]

As indicated in the documentation of pci_dev_get.

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 5150794..c1ce311 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -672,6 +672,8 @@ static int tpci200_pciprobe(struct pci_dev *pdev,
 		goto out_err_info;
 	}
 
+	pci_dev_get(pdev);
+
 	/* Obtain a mapping of the carrier's PCI configuration registers */
 	ret = pci_request_region(pdev, TPCI200_CFG_MEM_BAR,
 				 KBUILD_MODNAME " Configuration Memory");
@@ -743,6 +745,7 @@ out_err_install:
 out_err_ioremap:
 	pci_release_region(pdev, TPCI200_CFG_MEM_BAR);
 out_err_pci_request:
+	pci_dev_put(pdev);
 	kfree(tpci200->info);
 out_err_info:
 	kfree(tpci200);
-- 
1.7.10.4

[PATCH 13/20] Staging: ipack/bridges/tpci200: fix the uninstall the ipack device

[Samuel Iglesias Gonsálvez]

Using the call to the ipack_device_unregister() function to avoid the
strange way it was doing, as the device model will take care of calling
the bus's .remove function when a device is being unregistered.

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index c1ce311..293456b 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -30,8 +30,6 @@ static u16 tpci200_status_error[] = {
 
 static struct ipack_bus_ops tpci200_bus_ops;
 
-static int tpci200_slot_unregister(struct ipack_device *dev);
-
 static struct tpci200_board *check_slot(struct ipack_device *dev)
 {
 	struct tpci200_board *tpci200;
@@ -387,7 +385,6 @@ static int tpci200_slot_unregister(struct ipack_device *dev)
 		return -ERESTARTSYS;
 
 	tpci200->slots[dev->slot].dev = NULL;
-	ipack_device_unregister(dev);
 	mutex_unlock(&tpci200->mutex);
 
 	return 0;
@@ -616,7 +613,7 @@ static void tpci200_uninstall(struct tpci200_board *tpci200)
 	int i;
 
 	for (i = 0; i < TPCI200_NB_SLOT; i++)
-		tpci200_slot_unregister(tpci200->slots[i].dev);
+		ipack_device_unregister(tpci200->slots[i].dev);
 
 	tpci200_unregister(tpci200);
 	kfree(tpci200->slots);
-- 
1.7.10.4

[PATCH 14/20] Staging: ipack/devices/ipoctal: change exiting procedure

[Samuel Iglesias Gonsálvez]

The ipoctal devices can be uninstalled from the ipack_driver_unregister()
call as the device model calles the bus's .remove() function for each device
registered by the driver and it will execute the .remove() function of the
ipoctal driver.

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/devices/ipoctal.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/ipack/devices/ipoctal.c b/drivers/staging/ipack/devices/ipoctal.c
index c94a9df..272832f 100644
--- a/drivers/staging/ipack/devices/ipoctal.c
+++ b/drivers/staging/ipack/devices/ipoctal.c
@@ -853,11 +853,6 @@ static int __init ipoctal_init(void)
 
 static void __exit ipoctal_exit(void)
 {
-	struct ipoctal *p, *next;
-
-	list_for_each_entry_safe(p, next, &ipoctal_list, list)
-		p->dev->bus->ops->remove_device(p->dev);
-
 	ipack_driver_unregister(&driver);
 }
 
-- 
1.7.10.4

[PATCH 15/20] Staging: ipack/devices/ipoctal: free the IRQ.

[Samuel Iglesias Gonsálvez]

As the IRQ was requested by the driver, it should free it also.

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/devices/ipoctal.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/ipack/devices/ipoctal.c b/drivers/staging/ipack/devices/ipoctal.c
index 272832f..35513d9 100644
--- a/drivers/staging/ipack/devices/ipoctal.c
+++ b/drivers/staging/ipack/devices/ipoctal.c
@@ -803,6 +803,8 @@ static void __ipoctal_remove(struct ipoctal *ipoctal)
 {
 	int i;
 
+	ipoctal->dev->bus->ops->free_irq(ipoctal->dev);
+
 	for (i = 0; i < NR_CHANNELS; i++) {
 		tty_unregister_device(ipoctal->tty_drv, i);
 		tty_port_free_xmit_buf(&ipoctal->tty_port[i]);
-- 
1.7.10.4

[PATCH 16/20] Staging: ipack: unregister devices when uninstall the carrier device.

[Samuel Iglesias Gonsálvez]

Find the IP modules that are plugged to the carrier and unregister them.

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/ipack.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/ipack/ipack.c b/drivers/staging/ipack/ipack.c
index b368a26..bf6a36d 100644
--- a/drivers/staging/ipack/ipack.c
+++ b/drivers/staging/ipack/ipack.c
@@ -228,8 +228,20 @@ struct ipack_bus_device *ipack_bus_register(struct device *parent, int slots,
 }
 EXPORT_SYMBOL_GPL(ipack_bus_register);
 
+static int ipack_unregister_bus_member(struct device *dev, void *data)
+{
+	struct ipack_device *idev = to_ipack_dev(dev);
+	struct ipack_bus_device *bus = data;
+
+	if (idev->bus_nr == bus->bus_nr)
+		ipack_device_unregister(idev);
+
+	return 1;
+}
+
 int ipack_bus_unregister(struct ipack_bus_device *bus)
 {
+	bus_for_each_dev(&ipack_bus_type, NULL, bus, ipack_unregister_bus_member);
 	ida_simple_remove(&ipack_ida, bus->bus_nr);
 	kfree(bus);
 	return 0;
-- 
1.7.10.4

[PATCH 17/20] Staging: ipack/bridges/tpci200: delete ipack_device_unregister calls when exiting

[Samuel Iglesias Gonsálvez]

As the ipack_bus_unregister() takes care of unregistering the devices plugged
in the carrier, it is not needed to do it in the carrier driver.

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 293456b..12f2ca6 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -610,11 +610,6 @@ static int tpci200_reset_timeout(struct ipack_device *dev)
 
 static void tpci200_uninstall(struct tpci200_board *tpci200)
 {
-	int i;
-
-	for (i = 0; i < TPCI200_NB_SLOT; i++)
-		ipack_device_unregister(tpci200->slots[i].dev);
-
 	tpci200_unregister(tpci200);
 	kfree(tpci200->slots);
 }
@@ -751,8 +746,8 @@ out_err_info:
 
 static void __tpci200_pci_remove(struct tpci200_board *tpci200)
 {
-	tpci200_uninstall(tpci200);
 	ipack_bus_unregister(tpci200->info->ipack_bus);
+	tpci200_uninstall(tpci200);
 
 	kfree(tpci200->info);
 	kfree(tpci200);
-- 
1.7.10.4

[PATCH 18/20] Staging: ipack/bridges/tpci200: remove tpci200_slot_unregister

[Samuel Iglesias Gonsálvez]

It is not needed as the IP module should free its IRQ using
tpci200_free_irq callback.

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |   24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 12f2ca6..e58e312 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -368,28 +368,6 @@ out_unlock:
 	return 0;
 }
 
-static int tpci200_slot_unregister(struct ipack_device *dev)
-{
-	struct tpci200_board *tpci200;
-
-	if (dev == NULL)
-		return -ENODEV;
-
-	tpci200 = check_slot(dev);
-	if (tpci200 == NULL)
-		return -EINVAL;
-
-	tpci200_free_irq(dev);
-
-	if (mutex_lock_interruptible(&tpci200->mutex))
-		return -ERESTARTSYS;
-
-	tpci200->slots[dev->slot].dev = NULL;
-	mutex_unlock(&tpci200->mutex);
-
-	return 0;
-}
-
 static int tpci200_slot_map_space(struct ipack_device *dev,
 				  unsigned int memory_size, int space)
 {
@@ -619,7 +597,7 @@ static struct ipack_bus_ops tpci200_bus_ops = {
 	.unmap_space = tpci200_slot_unmap_space,
 	.request_irq = tpci200_request_irq,
 	.free_irq = tpci200_free_irq,
-	.remove_device = tpci200_slot_unregister,
+	.remove_device = NULL,
 	.get_clockrate = tpci200_get_clockrate,
 	.set_clockrate = tpci200_set_clockrate,
 	.get_error     = tpci200_get_error,
-- 
1.7.10.4

[PATCH 19/20] Staging: ipack: delete .remove_device() callback

[Samuel Iglesias Gonsálvez]

As the IP module driver takes care of freeing its resources.

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |    1 -
 drivers/staging/ipack/ipack.h           |    3 ---
 2 files changed, 4 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index e58e312..531dfd5 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -597,7 +597,6 @@ static struct ipack_bus_ops tpci200_bus_ops = {
 	.unmap_space = tpci200_slot_unmap_space,
 	.request_irq = tpci200_request_irq,
 	.free_irq = tpci200_free_irq,
-	.remove_device = NULL,
 	.get_clockrate = tpci200_get_clockrate,
 	.set_clockrate = tpci200_set_clockrate,
 	.get_error     = tpci200_get_error,
diff --git a/drivers/staging/ipack/ipack.h b/drivers/staging/ipack/ipack.h
index e2cbf4a..33311c7 100644
--- a/drivers/staging/ipack/ipack.h
+++ b/drivers/staging/ipack/ipack.h
@@ -113,7 +113,6 @@ struct ipack_driver {
  *	@unmap_space: unmap IP address space
  *	@request_irq: request IRQ
  *	@free_irq: free IRQ
- *	@remove_device: tell the bridge module that the device has been removed
  *	@get_clockrate: Returns the clockrate the carrier is currently
  *		communicating with the device at.
  *	@set_clockrate: Sets the clock-rate for carrier / module communication.
@@ -129,8 +128,6 @@ struct ipack_bus_ops {
 	int (*unmap_space) (struct ipack_device *dev, int space);
 	int (*request_irq) (struct ipack_device *dev, int vector, int (*handler)(void *), void *arg);
 	int (*free_irq) (struct ipack_device *dev);
-	int (*remove_device) (struct ipack_device *dev);
-
 	int (*get_clockrate) (struct ipack_device *dev);
 	int (*set_clockrate) (struct ipack_device *dev, int mherz);
 	int (*get_error) (struct ipack_device *dev);
-- 
1.7.10.4

[PATCH 20/20] Staging: ipack/bridges/tpci200: Store the irq holder in slot_irq.

[Samuel Iglesias Gonsálvez]

From: Jens Taprogge <jens.taprogge@taprogge.org>

This way we do no longer need to keep a dangling pointer to struct
ipack_device in tpci200_slot after the device has been removed.

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 drivers/staging/ipack/bridges/tpci200.c |   19 ++++++++++++-------
 drivers/staging/ipack/bridges/tpci200.h |    2 +-
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/ipack/bridges/tpci200.c b/drivers/staging/ipack/bridges/tpci200.c
index 531dfd5..6873b4d 100644
--- a/drivers/staging/ipack/bridges/tpci200.c
+++ b/drivers/staging/ipack/bridges/tpci200.c
@@ -91,6 +91,7 @@ static irqreturn_t tpci200_interrupt(int irq, void *dev_id)
 	unsigned short status_reg, reg_value;
 	unsigned short unhandled_ints = 0;
 	irqreturn_t ret = IRQ_NONE;
+	struct slot_irq *slot_irq;
 
 	/* Read status register */
 	status_reg = readw(&tpci200->info->interface_regs->status);
@@ -99,15 +100,17 @@ static irqreturn_t tpci200_interrupt(int irq, void *dev_id)
 		unhandled_ints = status_reg & TPCI200_SLOT_INT_MASK;
 		/* callback to the IRQ handler for the corresponding slot */
 		for (i = 0; i < TPCI200_NB_SLOT; i++) {
-			if ((tpci200->slots[i].irq != NULL) &&
+			slot_irq = tpci200->slots[i].irq;
+
+			if ((slot_irq != NULL) &&
 			    (status_reg & ((TPCI200_A_INT0 | TPCI200_A_INT1) << (2*i)))) {
 
-				ret = tpci200->slots[i].irq->handler(tpci200->slots[i].irq->arg);
+				ret = slot_irq->handler(slot_irq->arg);
 
 				/* Dummy reads */
-				readw(tpci200->slots[i].dev->io_space.address +
+				readw(slot_irq->holder->io_space.address +
 				      0xC0);
-				readw(tpci200->slots[i].dev->io_space.address +
+				readw(slot_irq->holder->io_space.address +
 				      0xC2);
 
 				unhandled_ints &= ~(((TPCI200_A_INT0 | TPCI200_A_INT1) << (2*i)));
@@ -117,8 +120,10 @@ static irqreturn_t tpci200_interrupt(int irq, void *dev_id)
 	/* Interrupts not handled are disabled */
 	if (unhandled_ints) {
 		for (i = 0; i < TPCI200_NB_SLOT; i++) {
+			slot_irq = tpci200->slots[i].irq;
+
 			if (unhandled_ints & ((TPCI200_INT0_EN | TPCI200_INT1_EN) << (2*i))) {
-				dev_info(&tpci200->slots[i].dev->dev,
+				dev_info(&slot_irq->holder->dev,
 					 "No registered ISR for slot [%d:%d]!. IRQ will be disabled.\n",
 					 tpci200->number, i);
 				reg_value = readw(
@@ -489,6 +494,7 @@ static int tpci200_request_irq(struct ipack_device *dev, int vector,
 	slot_irq->vector = vector;
 	slot_irq->handler = handler;
 	slot_irq->arg = arg;
+	slot_irq->holder = dev;
 
 	tpci200->slots[dev->slot].irq = slot_irq;
 	res = __tpci200_request_irq(tpci200, dev);
@@ -703,8 +709,7 @@ static int tpci200_pciprobe(struct pci_dev *pdev,
 	 * The TPCI200 has assigned his own two IRQ by PCI bus driver
 	 */
 	for (i = 0; i < TPCI200_NB_SLOT; i++)
-		tpci200->slots[i].dev =
-			ipack_device_register(tpci200->info->ipack_bus, i, i);
+		ipack_device_register(tpci200->info->ipack_bus, i, i);
 	return 0;
 
 out_err_bus_register:
diff --git a/drivers/staging/ipack/bridges/tpci200.h b/drivers/staging/ipack/bridges/tpci200.h
index 0fc1900..b9de6be 100644
--- a/drivers/staging/ipack/bridges/tpci200.h
+++ b/drivers/staging/ipack/bridges/tpci200.h
@@ -121,6 +121,7 @@ struct tpci200_regs {
  *
  */
 struct slot_irq {
+	struct ipack_device *holder;
 	int		vector;
 	int		(*handler)(void *);
 	void		*arg;
@@ -136,7 +137,6 @@ struct slot_irq {
  *
  */
 struct tpci200_slot {
-	struct ipack_device	*dev;
 	struct slot_irq		*irq;
 	struct ipack_addr_space io_phys;
 	struct ipack_addr_space id_phys;
-- 
1.7.10.4

Re: [PATCH 20/20] Staging: ipack/bridges/tpci200: Store the irq holder in slot_irq.

[Dan Carpenter]

The whole patchset looks good.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH 20/20] Staging: ipack/bridges/tpci200: Store the irq holder in slot_irq.

[Samuel Iglesias Gonsálvez]

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

On Tue, 2012-09-11 at 11:51 +0300, Dan Carpenter wrote:
> The whole patchset looks good.
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> regards,
> dan carpenter

Taking into account that I need to resend the patches, I can add these
suggestions.

Thanks for your revision,

Sam

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

Re: [PATCH 20/20] Staging: ipack/bridges/tpci200: Store the irq holder in slot_irq.

[Dan Carpenter]

On Tue, Sep 11, 2012 at 11:05:10AM +0200, Samuel Iglesias Gonsálvez wrote:
> On Tue, 2012-09-11 at 11:51 +0300, Dan Carpenter wrote:
> > The whole patchset looks good.
> > 
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > regards,
> > dan carpenter
> 
> Taking into account that I need to resend the patches, I can add these
> suggestions.
> 

Do whatever is easiest.  Staging/ has different review standards
from mm/ and we're trying to churn through the code and clean it up
as quickly as feasible.  The review comments I had were minor style
issues, which I hope you will consider for later patches, but not
something which is worth delaying over.

regards,
dan carpenter

Re: [PATCH 01/20] Staging: ipack/bridges/tpci200: Put the TPCI200 control registers into a struct.

[Greg Kroah-Hartman]

On Mon, Sep 10, 2012 at 10:51:39AM +0200, Samuel Iglesias Gonsálvez wrote:
> From: Jens Taprogge <jens.taprogge@taprogge.org>
> 
> This saves us from a little pointer arithmetic and cleans up the code a bit.
> 
> Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> ---
>  drivers/staging/ipack/bridges/tpci200.c |   28 +++++++---------------------
>  drivers/staging/ipack/bridges/tpci200.h |   21 +++++++++++++--------
>  2 files changed, 20 insertions(+), 29 deletions(-)

This patch doesn't apply to my tree for some reason.

Also, all of your patches are in base64 mode, which makes it impossible
for me to fix them up by hand for issues like this.

Care to redo your series, and fix your email client, and resend them?

thanks,

greg k-h

Re: [PATCH 01/20] Staging: ipack/bridges/tpci200: Put the TPCI200 control registers into a struct.

[Samuel Iglesias Gonsálvez]

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

On Mon, 2012-09-10 at 11:29 -0700, Greg Kroah-Hartman wrote:
> On Mon, Sep 10, 2012 at 10:51:39AM +0200, Samuel Iglesias Gonsálvez wrote:
> > From: Jens Taprogge <jens.taprogge@taprogge.org>
> > 
> > This saves us from a little pointer arithmetic and cleans up the code a bit.
> > 
> > Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
> > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> > ---
> >  drivers/staging/ipack/bridges/tpci200.c |   28 +++++++---------------------
> >  drivers/staging/ipack/bridges/tpci200.h |   21 +++++++++++++--------
> >  2 files changed, 20 insertions(+), 29 deletions(-)
> 
> This patch doesn't apply to my tree for some reason.
> 

OK, I will rebase my repo and send them again.

> Also, all of your patches are in base64 mode, which makes it impossible
> for me to fix them up by hand for issues like this.
> 
> Care to redo your series, and fix your email client, and resend them?

It's quite strange... Looking at the headers of my local copy of the
patches I see the following:

--------------------------------
[...]
Subject: [PATCH 01/20] Staging: ipack/bridges/tpci200: Put the TPCI200
control registers into a struct.
Date: Mon, 10 Sep 2012 10:51:39 +0200
Message-id: <1347267118-9580-1-git-send-email-siglesias@igalia.com>
X-mailer: git-send-email 1.7.10.4
Mime-version: 1.0
Content-type: text/plain; charset=UTF-8
Content-transfer-encoding: 8bit
--------------------------------

I don't see the base64. However, I will try to figure out how can I fix
this issue in git-send-email.

Thanks,

Sam

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

Re: [PATCH 01/20] Staging: ipack/bridges/tpci200: Put the TPCI200 control registers into a struct.

[Miguel Gómez]

>[...]
>
> I don't see the base64. However, I will try to figure out how can I fix
> this issue in git-send-email.

I've received them in base64 as well.

But it also happened to me before that round of patches was encoded in 
base64 using format-patch and send-email, instead of using utf8, with no 
apparent reason, and just recreating the patches and resending them 
fixed the encoding to 8 bits.

Regards!

-- 
Miguel Gómez
Igalia - http://www.igalia.com