[PATCH] ARM: mach-shmobile: Mackerel USB platform data update

[PATCH] ARM: mach-shmobile: Mackerel USB platform data update

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

This patch updates the board specific USB support
code for the sh7372 Mackerel board.

With this patch applied port CN22 is driven by the
recently added renesas_usbhs driver using the first
USB controller included in sh7372 aka USBHS0.

Hotplugging of USBHS0 unfortunately has to be
handled by software polling. The sh7372 SoC itself
obviously supports hotplug notification by IRQ but
on the Mackerel board this IRQ happens to be used
for the touch screen.

Also fix the pinmux configuration to avoid setting
up unused pins and fix minor spelling errors.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/arm/mach-shmobile/board-mackerel.c |  223 ++++++++++++++++++++++---------
 1 file changed, 159 insertions(+), 64 deletions(-)

--- 0001/arch/arm/mach-shmobile/board-mackerel.c
+++ work/arch/arm/mach-shmobile/board-mackerel.c	2011-06-08 13:32:27.000000000 +0900
@@ -126,7 +126,7 @@
  * ------+--------------------+--------------------+-------
  * IRQ0  | ICR1A.IRQ0SA\010  | SDHI2 card detect  | Low
  * IRQ6  | ICR1A.IRQ6SA\011  | Ether(LAN9220)     | High
- * IRQ7  | ICR1A.IRQ7SA\010  | LCD Tuch Panel     | Low
+ * IRQ7  | ICR1A.IRQ7SA\010  | LCD Touch Panel    | Low
  * IRQ8  | ICR2A.IRQ8SA\010  | MMC/SD card detect | Low
  * IRQ9  | ICR2A.IRQ9SA\010  | KEY(TCA6408)       | Low
  * IRQ21 | ICR4A.IRQ21SA\011 | Sensor(ADXL345)    | High
@@ -165,10 +165,10 @@
  * USB1 can become Host by r8a66597, and become Function by renesas_usbhs.
  * But don't select both drivers in same time.
  * These uses same IRQ number for request_irq(), and aren't supporting
- * IRQF_SHARD / IORESOURCE_IRQ_SHAREABLE.
+ * IRQF_SHARED / IORESOURCE_IRQ_SHAREABLE.
  *
  * Actually these are old/new version of USB driver.
- * This mean its register will be broken if it supports SHARD IRQ,
+ * This mean its register will be broken if it supports shared IRQ,
  */
 
 /*
@@ -562,7 +562,136 @@ out:
 		clk_put(hdmi_ick);
 }
 
-/* USB1 (Host) */
+/* USBHS0 is connected to CN22 which takes a USB Mini-B plug
+ *
+ * The sh7372 SoC has IRQ7 set aside for USBHS0 hotplug,
+ * but on this particular board IRQ7 is already used by
+ * the touch screen. This leaves us with software polling.
+ */
+#define USBHS0_POLL_INTERVAL (HZ * 5)
+
+struct usbhs_private {
+	unsigned int usbphyaddr;
+	unsigned int usbcrcaddr;
+	struct renesas_usbhs_platform_info info;
+	struct delayed_work work;
+	struct platform_device *pdev;
+};
+
+#define usbhs_get_priv(pdev)				\
+	container_of(renesas_usbhs_get_info(pdev),	\
+		     struct usbhs_private, info)
+
+#define usbhs_is_connected(priv)			\
+	(!((1 << 7) & __raw_readw(priv->usbcrcaddr)))
+
+static int usbhs_get_vbus(struct platform_device *pdev)
+{
+	return usbhs_is_connected(usbhs_get_priv(pdev));
+}
+
+static void usbhs_phy_reset(struct platform_device *pdev)
+{
+	struct usbhs_private *priv = usbhs_get_priv(pdev);
+
+	/* init phy */
+	__raw_writew(0x8a0a, priv->usbcrcaddr);
+}
+
+static int usbhs0_get_id(struct platform_device *pdev)
+{
+	return USBHS_GADGET;
+}
+
+static void usbhs0_work_function(struct work_struct *work)
+{
+	struct usbhs_private *priv = container_of(work, struct usbhs_private,
+						  work.work);
+
+	renesas_usbhs_call_notify_hotplug(priv->pdev);
+	schedule_delayed_work(&priv->work, USBHS0_POLL_INTERVAL);
+}
+
+static int usbhs0_hardware_init(struct platform_device *pdev)
+{
+	struct usbhs_private *priv = usbhs_get_priv(pdev);
+
+	priv->pdev = pdev;
+	INIT_DELAYED_WORK(&priv->work, usbhs0_work_function);
+	schedule_delayed_work(&priv->work, USBHS0_POLL_INTERVAL);
+	return 0;
+}
+
+static void usbhs0_hardware_exit(struct platform_device *pdev)
+{
+	struct usbhs_private *priv = usbhs_get_priv(pdev);
+
+	cancel_delayed_work_sync(&priv->work);
+}
+
+static u32 usbhs0_pipe_cfg[] = {
+	USB_ENDPOINT_XFER_CONTROL,
+	USB_ENDPOINT_XFER_ISOC,
+	USB_ENDPOINT_XFER_ISOC,
+	USB_ENDPOINT_XFER_BULK,
+	USB_ENDPOINT_XFER_BULK,
+	USB_ENDPOINT_XFER_BULK,
+	USB_ENDPOINT_XFER_INT,
+	USB_ENDPOINT_XFER_INT,
+	USB_ENDPOINT_XFER_INT,
+	USB_ENDPOINT_XFER_BULK,
+};
+
+static struct usbhs_private usbhs0_private = {
+	.usbcrcaddr	= 0xe605810c,		/* USBCR2 */
+	.info = {
+		.platform_callback = {
+			.hardware_init	= usbhs0_hardware_init,
+			.hardware_exit	= usbhs0_hardware_exit,
+			.phy_reset	= usbhs_phy_reset,
+			.get_id		= usbhs0_get_id,
+			.get_vbus	= usbhs_get_vbus,
+		},
+		.driver_param = {
+			.buswait_bwait	= 4,
+			.pipe_type	= usbhs0_pipe_cfg,
+			.pipe_size	= ARRAY_SIZE(usbhs0_pipe_cfg),
+		},
+	},
+};
+
+static struct resource usbhs0_resources[] = {
+	[0] = {
+		.name	= "USBHS0",
+		.start	= 0xe6890000,
+		.end	= 0xe68900e6 - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= evt2irq(0x1ca0) /* USB0_USB0I0 */,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device usbhs0_device = {
+	.name	= "renesas_usbhs",
+	.id	= 0,
+	.dev = {
+		.platform_data		= &usbhs0_private.info,
+	},
+	.num_resources	= ARRAY_SIZE(usbhs0_resources),
+	.resource	= usbhs0_resources,
+};
+
+/* USBHS1 is connected to CN31 which takes a USB Mini-AB plug
+ *
+ * Use J30 to select between Host and Function. This setting
+ * can however not be detected by software. Hotplug of USBHS1
+ * is provided via IRQ8.
+ */
+#define IRQ8 evt2irq(0x0300)
+
+/* USBHS1 USB Host support via r8a66597_hcd */
 static void usb1_host_port_power(int port, int power)
 {
 	if (!power) /* only power-on is supported for now */
@@ -579,9 +708,9 @@ static struct r8a66597_platdata usb1_hos
 
 static struct resource usb1_host_resources[] = {
 	[0] = {
-		.name	= "USBHS",
-		.start	= 0xE68B0000,
-		.end	= 0xE68B00E6 - 1,
+		.name	= "USBHS1",
+		.start	= 0xe68b0000,
+		.end	= 0xe68b00e6 - 1,
 		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
@@ -602,37 +731,14 @@ static struct platform_device usb1_host_
 	.resource	= usb1_host_resources,
 };
 
-/* USB1 (Function) */
+/* USBHS1 USB Function support via renesas_usbhs */
+
 #define USB_PHY_MODE		(1 << 4)
 #define USB_PHY_INT_EN		((1 << 3) | (1 << 2))
 #define USB_PHY_ON		(1 << 1)
 #define USB_PHY_OFF		(1 << 0)
 #define USB_PHY_INT_CLR		(USB_PHY_ON | USB_PHY_OFF)
 
-struct usbhs_private {
-	unsigned int irq;
-	unsigned int usbphyaddr;
-	unsigned int usbcrcaddr;
-	struct renesas_usbhs_platform_info info;
-};
-
-#define usbhs_get_priv(pdev)				\
-	container_of(renesas_usbhs_get_info(pdev),	\
-		     struct usbhs_private, info)
-
-#define usbhs_is_connected(priv)			\
-	(!((1 << 7) & __raw_readw(priv->usbcrcaddr)))
-
-static int usbhs1_get_id(struct platform_device *pdev)
-{
-	return USBHS_GADGET;
-}
-
-static int usbhs1_get_vbus(struct platform_device *pdev)
-{
-	return usbhs_is_connected(usbhs_get_priv(pdev));
-}
-
 static irqreturn_t usbhs1_interrupt(int irq, void *data)
 {
 	struct platform_device *pdev = data;
@@ -654,12 +760,10 @@ static int usbhs1_hardware_init(struct p
 	struct usbhs_private *priv = usbhs_get_priv(pdev);
 	int ret;
 
-	irq_set_irq_type(priv->irq, IRQ_TYPE_LEVEL_HIGH);
-
 	/* clear interrupt status */
 	__raw_writew(USB_PHY_MODE | USB_PHY_INT_CLR, priv->usbphyaddr);
 
-	ret = request_irq(priv->irq, usbhs1_interrupt, 0,
+	ret = request_irq(IRQ8, usbhs1_interrupt, IRQF_TRIGGER_HIGH,
 			  dev_name(&pdev->dev), pdev);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq err\n");
@@ -679,15 +783,7 @@ static void usbhs1_hardware_exit(struct
 	/* clear interrupt status */
 	__raw_writew(USB_PHY_MODE | USB_PHY_INT_CLR, priv->usbphyaddr);
 
-	free_irq(priv->irq, pdev);
-}
-
-static void usbhs1_phy_reset(struct platform_device *pdev)
-{
-	struct usbhs_private *priv = usbhs_get_priv(pdev);
-
-	/* init phy */
-	__raw_writew(0x8a0a, priv->usbcrcaddr);
+	free_irq(IRQ8, pdev);
 }
 
 static u32 usbhs1_pipe_cfg[] = {
@@ -710,16 +806,14 @@ static u32 usbhs1_pipe_cfg[] = {
 };
 
 static struct usbhs_private usbhs1_private = {
-	.irq		= evt2irq(0x0300),	/* IRQ8 */
-	.usbphyaddr	= 0xE60581E2,		/* USBPHY1INTAP */
-	.usbcrcaddr	= 0xE6058130,		/* USBCR4 */
+	.usbphyaddr	= 0xe60581e2,		/* USBPHY1INTAP */
+	.usbcrcaddr	= 0xe6058130,		/* USBCR4 */
 	.info = {
 		.platform_callback = {
 			.hardware_init	= usbhs1_hardware_init,
 			.hardware_exit	= usbhs1_hardware_exit,
-			.phy_reset	= usbhs1_phy_reset,
-			.get_id		= usbhs1_get_id,
-			.get_vbus	= usbhs1_get_vbus,
+			.phy_reset	= usbhs_phy_reset,
+			.get_vbus	= usbhs_get_vbus,
 		},
 		.driver_param = {
 			.buswait_bwait	= 4,
@@ -731,9 +825,9 @@ static struct usbhs_private usbhs1_priva
 
 static struct resource usbhs1_resources[] = {
 	[0] = {
-		.name	= "USBHS",
-		.start	= 0xE68B0000,
-		.end	= 0xE68B00E6 - 1,
+		.name	= "USBHS1",
+		.start	= 0xe68b0000,
+		.end	= 0xe68b00e6 - 1,
 		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
@@ -752,7 +846,6 @@ static struct platform_device usbhs1_dev
 	.resource	= usbhs1_resources,
 };
 
-
 /* LED */
 static struct gpio_led mackerel_leds[] = {
 	{
@@ -1203,6 +1296,7 @@ static struct platform_device *mackerel_
 	&nor_flash_device,
 	&smc911x_device,
 	&lcdc_device,
+	&usbhs0_device,
 	&usb1_host_device,
 	&usbhs1_device,
 	&leds_device,
@@ -1301,6 +1395,7 @@ static void __init mackerel_map_io(void)
 
 #define GPIO_PORT9CR	0xE6051009
 #define GPIO_PORT10CR	0xE605100A
+#define GPIO_PORT167CR	0xE60520A7
 #define GPIO_PORT168CR	0xE60520A8
 #define SRCR4		0xe61580bc
 #define USCCR1		0xE6058144
@@ -1354,17 +1449,17 @@ static void __init mackerel_init(void)
 	gpio_request(GPIO_PORT151, NULL); /* LCDDON */
 	gpio_direction_output(GPIO_PORT151, 1);
 
-	/* USB enable */
-	gpio_request(GPIO_FN_VBUS0_1,    NULL);
-	gpio_request(GPIO_FN_IDIN_1_18,  NULL);
-	gpio_request(GPIO_FN_PWEN_1_115, NULL);
-	gpio_request(GPIO_FN_OVCN_1_114, NULL);
-	gpio_request(GPIO_FN_EXTLP_1,    NULL);
-	gpio_request(GPIO_FN_OVCN2_1,    NULL);
-	gpio_pull_down(GPIO_PORT168CR);
+	/* USBHS0 */
+	gpio_request(GPIO_FN_VBUS0_0, NULL);
+	gpio_pull_down(GPIO_PORT168CR); /* VBUS0_0 pull down */
+
+	/* USBHS1 */
+	gpio_request(GPIO_FN_VBUS0_1, NULL);
+	gpio_pull_down(GPIO_PORT167CR); /* VBUS0_1 pull down */
+	gpio_request(GPIO_FN_IDIN_1_113, NULL);
 
-	/* setup USB phy */
-	__raw_writew(0x8a0a, 0xE6058130);	/* USBCR4 */
+	/* USB phy tweak to make the r8a66597_hcd host driver work */
+	__raw_writew(0x8a0a, 0xe6058130);       /* USBCR4 */
 
 	/* enable FSI2 port A (ak4643) */
 	gpio_request(GPIO_FN_FSIAIBT,	NULL);

Re: [PATCH] ARM: mach-shmobile: Mackerel USB platform data update

[Kuninori Morimoto]

Hi Magnus

Thank you for your patch.
small comment from me

> +static u32 usbhs0_pipe_cfg[] = {
> +	USB_ENDPOINT_XFER_CONTROL,
> +	USB_ENDPOINT_XFER_ISOC,
> +	USB_ENDPOINT_XFER_ISOC,
> +	USB_ENDPOINT_XFER_BULK,
> +	USB_ENDPOINT_XFER_BULK,
> +	USB_ENDPOINT_XFER_BULK,
> +	USB_ENDPOINT_XFER_INT,
> +	USB_ENDPOINT_XFER_INT,
> +	USB_ENDPOINT_XFER_INT,
> +	USB_ENDPOINT_XFER_BULK,
> +};

I think AP4 USB0 pipe9 is interrupt.
If so, this pipe config is not needed.
it is same as renesas_usbhs default pipe configs.
see
  renesas_usbhs/common.c :: usbhs_default_pipe_type

>  static struct resource usb1_host_resources[] = {
>  	[0] = {
> -		.name	= "USBHS",
> -		.start	= 0xE68B0000,
> -		.end	= 0xE68B00E6 - 1,
> +		.name	= "USBHS1",
> +		.start	= 0xe68b0000,
> +		.end	= 0xe68b00e6 - 1,
>  		.flags	= IORESOURCE_MEM,
>  	},

Is this "big/small char change" (start/end) needed ?

>  static struct usbhs_private usbhs1_private = {
> -	.irq		= evt2irq(0x0300),	/* IRQ8 */
> -	.usbphyaddr	= 0xE60581E2,		/* USBPHY1INTAP */
> -	.usbcrcaddr	= 0xE6058130,		/* USBCR4 */
> +	.usbphyaddr	= 0xe60581e2,		/* USBPHY1INTAP */
> +	.usbcrcaddr	= 0xe6058130,		/* USBCR4 */

here too.

>  static struct resource usbhs1_resources[] = {
>  	[0] = {
> -		.name	= "USBHS",
> -		.start	= 0xE68B0000,
> -		.end	= 0xE68B00E6 - 1,
> +		.name	= "USBHS1",
> +		.start	= 0xe68b0000,
> +		.end	= 0xe68b00e6 - 1,
>  		.flags	= IORESOURCE_MEM,
>  	},

here too

>  	[1] = {
> @@ -752,7 +846,6 @@ static struct platform_device usbhs1_dev
>  	.resource	= usbhs1_resources,
>  };
>  
> -
>  /* LED */
>  static struct gpio_led mackerel_leds[] = {

this "just line erase" is not needed =)

Thanks

Best regards
--
Kuninori Morimoto

Re: [PATCH] ARM: mach-shmobile: Mackerel USB platform data update

[Magnus Damm]

Hi Morimoto-san,

On Thu, Jun 9, 2011 at 4:09 PM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
> Hi Magnus
>
> Thank you for your patch.
> small comment from me
>
>> +static u32 usbhs0_pipe_cfg[] = {
>> +     USB_ENDPOINT_XFER_CONTROL,
>> +     USB_ENDPOINT_XFER_ISOC,
>> +     USB_ENDPOINT_XFER_ISOC,
>> +     USB_ENDPOINT_XFER_BULK,
>> +     USB_ENDPOINT_XFER_BULK,
>> +     USB_ENDPOINT_XFER_BULK,
>> +     USB_ENDPOINT_XFER_INT,
>> +     USB_ENDPOINT_XFER_INT,
>> +     USB_ENDPOINT_XFER_INT,
>> +     USB_ENDPOINT_XFER_BULK,
>> +};
>
> I think AP4 USB0 pipe9 is interrupt.
> If so, this pipe config is not needed.
> it is same as renesas_usbhs default pipe configs.
> see
>  renesas_usbhs/common.c :: usbhs_default_pipe_type

Ok, thanks, can you please send an incremental patch to fix this?

>>  static struct resource usb1_host_resources[] = {
>>       [0] = {
>> -             .name   = "USBHS",
>> -             .start  = 0xE68B0000,
>> -             .end    = 0xE68B00E6 - 1,
>> +             .name   = "USBHS1",
>> +             .start  = 0xe68b0000,
>> +             .end    = 0xe68b00e6 - 1,
>>               .flags  = IORESOURCE_MEM,
>>       },
>
> Is this "big/small char change" (start/end) needed ?
>
>>  static struct usbhs_private usbhs1_private = {
>> -     .irq            = evt2irq(0x0300),      /* IRQ8 */
>> -     .usbphyaddr     = 0xE60581E2,           /* USBPHY1INTAP */
>> -     .usbcrcaddr     = 0xE6058130,           /* USBCR4 */
>> +     .usbphyaddr     = 0xe60581e2,           /* USBPHY1INTAP */
>> +     .usbcrcaddr     = 0xe6058130,           /* USBCR4 */
>
> here too.
>
>>  static struct resource usbhs1_resources[] = {
>>       [0] = {
>> -             .name   = "USBHS",
>> -             .start  = 0xE68B0000,
>> -             .end    = 0xE68B00E6 - 1,
>> +             .name   = "USBHS1",
>> +             .start  = 0xe68b0000,
>> +             .end    = 0xe68b00e6 - 1,
>>               .flags  = IORESOURCE_MEM,
>>       },
>
> here too
>
>>       [1] = {
>> @@ -752,7 +846,6 @@ static struct platform_device usbhs1_dev
>>       .resource       = usbhs1_resources,
>>  };
>>
>> -
>>  /* LED */
>>  static struct gpio_led mackerel_leds[] = {
>
> this "just line erase" is not needed =)

Well, perhaps it's only me, but trying to make the code look less like
shit may be a good idea. =)

/ magnus

Re: [PATCH] ARM: mach-shmobile: Mackerel USB platform data update

[Paul Mundt]

On Thu, Jun 09, 2011 at 05:44:21PM +0900, Magnus Damm wrote:
> Hi Morimoto-san,
> 
> On Thu, Jun 9, 2011 at 4:09 PM, Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
> >
> > Hi Magnus
> >
> > Thank you for your patch.
> > small comment from me
> >
> >> +static u32 usbhs0_pipe_cfg[] = {
> >> + ? ? USB_ENDPOINT_XFER_CONTROL,
> >> + ? ? USB_ENDPOINT_XFER_ISOC,
> >> + ? ? USB_ENDPOINT_XFER_ISOC,
> >> + ? ? USB_ENDPOINT_XFER_BULK,
> >> + ? ? USB_ENDPOINT_XFER_BULK,
> >> + ? ? USB_ENDPOINT_XFER_BULK,
> >> + ? ? USB_ENDPOINT_XFER_INT,
> >> + ? ? USB_ENDPOINT_XFER_INT,
> >> + ? ? USB_ENDPOINT_XFER_INT,
> >> + ? ? USB_ENDPOINT_XFER_BULK,
> >> +};
> >
> > I think AP4 USB0 pipe9 is interrupt.
> > If so, this pipe config is not needed.
> > it is same as renesas_usbhs default pipe configs.
> > see
> > ?renesas_usbhs/common.c :: usbhs_default_pipe_type
> 
> Ok, thanks, can you please send an incremental patch to fix this?
> 
I've grudgingly applied this patch as it is, but I really don't like this
half-assed approach to patch submission. The onus is not on Morimoto-san
to fix up your submission when he's pointed out something that you
obviously failed to take under consideration on an initial patch. You
should have simply discarded this and submitted an updated version that
fixed this up, and in a timely fashion.

Since none of these things have happened and it's unlikely that any of
them will before some -next time leading up to the next -rc I've taken it
verbatim, but am certainly not happy with having had to do so, nor do I
see much reason to continue to do so in the future.

> >> ? ? ? [1] = {
> >> @@ -752,7 +846,6 @@ static struct platform_device usbhs1_dev
> >> ? ? ? .resource ? ? ? = usbhs1_resources,
> >> ?};
> >>
> >> -
> >> ?/* LED */
> >> ?static struct gpio_led mackerel_leds[] = {
> >
> > this "just line erase" is not needed =)
> 
> Well, perhaps it's only me, but trying to make the code look less like
> shit may be a good idea. =)
> 
Pot calling the kettle black much? Perhaps adding new lines following
comment blocks for no reason as your patch does should go, too? Or you
could make your multi-line comments comply with Documentation/CodingStyle
too, as everyone else seems to be able to. Or are we just randomly
choosing which parts of the kernel coding style to comply with and
criticise others over today?

Re: [PATCH] ARM: mach-shmobile: Mackerel USB platform data update

[Magnus Damm]

Hi Paul,

On Tue, Jun 14, 2011 at 3:31 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Thu, Jun 09, 2011 at 05:44:21PM +0900, Magnus Damm wrote:
>> Hi Morimoto-san,
>>
>> On Thu, Jun 9, 2011 at 4:09 PM, Kuninori Morimoto
>> <kuninori.morimoto.gx@renesas.com> wrote:
>> >
>> > Hi Magnus
>> >
>> > Thank you for your patch.
>> > small comment from me
>> >
>> >> +static u32 usbhs0_pipe_cfg[] = {
>> >> + ? ? USB_ENDPOINT_XFER_CONTROL,
>> >> + ? ? USB_ENDPOINT_XFER_ISOC,
>> >> + ? ? USB_ENDPOINT_XFER_ISOC,
>> >> + ? ? USB_ENDPOINT_XFER_BULK,
>> >> + ? ? USB_ENDPOINT_XFER_BULK,
>> >> + ? ? USB_ENDPOINT_XFER_BULK,
>> >> + ? ? USB_ENDPOINT_XFER_INT,
>> >> + ? ? USB_ENDPOINT_XFER_INT,
>> >> + ? ? USB_ENDPOINT_XFER_INT,
>> >> + ? ? USB_ENDPOINT_XFER_BULK,
>> >> +};
>> >
>> > I think AP4 USB0 pipe9 is interrupt.
>> > If so, this pipe config is not needed.
>> > it is same as renesas_usbhs default pipe configs.
>> > see
>> > ?renesas_usbhs/common.c :: usbhs_default_pipe_type
>>
>> Ok, thanks, can you please send an incremental patch to fix this?
>>
> I've grudgingly applied this patch as it is, but I really don't like this
> half-assed approach to patch submission. The onus is not on Morimoto-san
> to fix up your submission when he's pointed out something that you
> obviously failed to take under consideration on an initial patch. You
> should have simply discarded this and submitted an updated version that
> fixed this up, and in a timely fashion.

Thanks for picking up the patch. Under normal circumstances I would
agree that me sending a new version would be the best way forward, but
this case is somewhat different. I didn't send a V2 because:
a) I expected Morimoto-san to help me
b) I wasn't sure if his comment was correct

The background of all this is that I asked Morimoto-san for help with
USB platform data. When then got submitted to the mailing list only
_one_ of the USB ports were supported, and when I tried to encourage
people to add support for both ports i received the reply that this
was "impossible". So I decided to fix it myself. Believe it or not, I
expect both USB ports on my board to work.

> Since none of these things have happened and it's unlikely that any of
> them will before some -next time leading up to the next -rc I've taken it
> verbatim, but am certainly not happy with having had to do so, nor do I
> see much reason to continue to do so in the future.

I don't expect you to do so, but if you are unsure about any schedule
feel free to ask over mail or in person.

>> >> ? ? ? [1] = {
>> >> @@ -752,7 +846,6 @@ static struct platform_device usbhs1_dev
>> >> ? ? ? .resource ? ? ? = usbhs1_resources,
>> >> ?};
>> >>
>> >> -
>> >> ?/* LED */
>> >> ?static struct gpio_led mackerel_leds[] = {
>> >
>> > this "just line erase" is not needed =)
>>
>> Well, perhaps it's only me, but trying to make the code look less like
>> shit may be a good idea. =)
>>
> Pot calling the kettle black much? Perhaps adding new lines following
> comment blocks for no reason as your patch does should go, too? Or you
> could make your multi-line comments comply with Documentation/CodingStyle
> too, as everyone else seems to be able to. Or are we just randomly
> choosing which parts of the kernel coding style to comply with and
> criticise others over today?

This is only me being bitter. After spending half a day wading through
schematics with "special" board irq routing, incorrect software pinmux
settings and random spelling errors I did manage to prove that the
"impossible" indeed happened to be "possible".

Then after doing someone else's job what do I get? Coding style comments!

/ magnus

Re: [PATCH] ARM: mach-shmobile: Mackerel USB platform data update

[Paul Mundt]

On Tue, Jun 14, 2011 at 04:54:04PM +0900, Magnus Damm wrote:
> On Tue, Jun 14, 2011 at 3:31 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Thu, Jun 09, 2011 at 05:44:21PM +0900, Magnus Damm wrote:
> >> On Thu, Jun 9, 2011 at 4:09 PM, Kuninori Morimoto
> >> <kuninori.morimoto.gx@renesas.com> wrote:
> >> > I think AP4 USB0 pipe9 is interrupt.
> >> > If so, this pipe config is not needed.
> >> > it is same as renesas_usbhs default pipe configs.
> >> > see
> >> > ?renesas_usbhs/common.c :: usbhs_default_pipe_type
> >>
> >> Ok, thanks, can you please send an incremental patch to fix this?
> >>
> > I've grudgingly applied this patch as it is, but I really don't like this
> > half-assed approach to patch submission. The onus is not on Morimoto-san
> > to fix up your submission when he's pointed out something that you
> > obviously failed to take under consideration on an initial patch. You
> > should have simply discarded this and submitted an updated version that
> > fixed this up, and in a timely fashion.
> 
> Thanks for picking up the patch. Under normal circumstances I would
> agree that me sending a new version would be the best way forward, but
> this case is somewhat different. I didn't send a V2 because:
> a) I expected Morimoto-san to help me
> b) I wasn't sure if his comment was correct
> 
> The background of all this is that I asked Morimoto-san for help with
> USB platform data. When then got submitted to the mailing list only
> _one_ of the USB ports were supported, and when I tried to encourage
> people to add support for both ports i received the reply that this
> was "impossible". So I decided to fix it myself. Believe it or not, I
> expect both USB ports on my board to work.
> 
Ok, it would have been nice to know that in advance, then I would have
been much less grumpy. Since there seems to be some uncertainty with
regards to the pipe9 bulk vs interrupt setting I suppose all we can do
now is go with your version until it's demonstrably shown to be
incorrect (which works out well, since that's the version I merged).

> > Since none of these things have happened and it's unlikely that any of
> > them will before some -next time leading up to the next -rc I've taken it
> > verbatim, but am certainly not happy with having had to do so, nor do I
> > see much reason to continue to do so in the future.
> 
> I don't expect you to do so, but if you are unsure about any schedule
> feel free to ask over mail or in person.
> 
Usually we have a -rc every week, and I like to have things sitting in
-next for at least a couple of days before I send them out to Linus. This
sort of stuff is unlikely to introduce any regression in other areas
outside of the configuration that you're already testing, but we've
certainly been bitten by enough seemingly obvious and "tested" stuff in
the past that we really do need the padding. With something this big and
insular it's possible to be a bit more flexible, but generally things of
this size I expect around -rc2 at the latest.

The first I even found out that there was a driver was after the USB
merge during the merge window. This is normally ok, but we could easily
have coordinated this better, had the driver in a topic branch pulled in
from the USB tree (or pulled by the USB tree) with the platform data on
top so it all has enough time to be tested together and improved upon,
and ultimately all ready to go immediately once the merge dependencies
are taken care of.

Generally I don't mind adding platform data bits later in the -rc
timeframe, but in this particular case it's obvious that everyone would
have benefitted from having this out there much earlier, and with
increased visibility (I'm not singling you out for this, it's just a
general observation).

Re: [PATCH] ARM: mach-shmobile: Mackerel USB platform data update

[Magnus Damm]

On Tue, Jun 14, 2011 at 5:36 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Tue, Jun 14, 2011 at 04:54:04PM +0900, Magnus Damm wrote:
>> On Tue, Jun 14, 2011 at 3:31 PM, Paul Mundt <lethal@linux-sh.org> wrote:
>> > On Thu, Jun 09, 2011 at 05:44:21PM +0900, Magnus Damm wrote:
>> >> On Thu, Jun 9, 2011 at 4:09 PM, Kuninori Morimoto
>> >> <kuninori.morimoto.gx@renesas.com> wrote:
>> >> > I think AP4 USB0 pipe9 is interrupt.
>> >> > If so, this pipe config is not needed.
>> >> > it is same as renesas_usbhs default pipe configs.
>> >> > see
>> >> > ?renesas_usbhs/common.c :: usbhs_default_pipe_type
>> >>
>> >> Ok, thanks, can you please send an incremental patch to fix this?
>> >>
>> > I've grudgingly applied this patch as it is, but I really don't like this
>> > half-assed approach to patch submission. The onus is not on Morimoto-san
>> > to fix up your submission when he's pointed out something that you
>> > obviously failed to take under consideration on an initial patch. You
>> > should have simply discarded this and submitted an updated version that
>> > fixed this up, and in a timely fashion.
>>
>> Thanks for picking up the patch. Under normal circumstances I would
>> agree that me sending a new version would be the best way forward, but
>> this case is somewhat different. I didn't send a V2 because:
>> a) I expected Morimoto-san to help me
>> b) I wasn't sure if his comment was correct
>>
>> The background of all this is that I asked Morimoto-san for help with
>> USB platform data. When then got submitted to the mailing list only
>> _one_ of the USB ports were supported, and when I tried to encourage
>> people to add support for both ports i received the reply that this
>> was "impossible". So I decided to fix it myself. Believe it or not, I
>> expect both USB ports on my board to work.
>>
> Ok, it would have been nice to know that in advance, then I would have
> been much less grumpy. Since there seems to be some uncertainty with
> regards to the pipe9 bulk vs interrupt setting I suppose all we can do
> now is go with your version until it's demonstrably shown to be
> incorrect (which works out well, since that's the version I merged).

I've done some checking about USBHS0 pipe configuration and
Morimoto-san seems correct. Will submit an incremental patch.

>> > Since none of these things have happened and it's unlikely that any of
>> > them will before some -next time leading up to the next -rc I've taken it
>> > verbatim, but am certainly not happy with having had to do so, nor do I
>> > see much reason to continue to do so in the future.
>>
>> I don't expect you to do so, but if you are unsure about any schedule
>> feel free to ask over mail or in person.
>>
> Usually we have a -rc every week, and I like to have things sitting in
> -next for at least a couple of days before I send them out to Linus. This
> sort of stuff is unlikely to introduce any regression in other areas
> outside of the configuration that you're already testing, but we've
> certainly been bitten by enough seemingly obvious and "tested" stuff in
> the past that we really do need the padding. With something this big and
> insular it's possible to be a bit more flexible, but generally things of
> this size I expect around -rc2 at the latest.

Sounds good, I think your expectations are very reasonable.

> The first I even found out that there was a driver was after the USB
> merge during the merge window. This is normally ok, but we could easily
> have coordinated this better, had the driver in a topic branch pulled in
> from the USB tree (or pulled by the USB tree) with the platform data on
> top so it all has enough time to be tested together and improved upon,
> and ultimately all ready to go immediately once the merge dependencies
> are taken care of.

I believe I've asked to get the platform data submitted way before the
merge window, but I may have been a bit unclear.

> Generally I don't mind adding platform data bits later in the -rc
> timeframe, but in this particular case it's obvious that everyone would
> have benefitted from having this out there much earlier, and with
> increased visibility (I'm not singling you out for this, it's just a
> general observation).

Sure, but the earlier the better, I totally agree.

Thanks,

/ magnus