Re: [PATCH 1/2] arm:da8xx:usb: move common OHCI platform setup code to mach-davinci/usb.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello.

On 03/12/2012 05:05 PM, Manjunathappa, Prakash wrote:

Patch abstracts and moves common USB OHCI platform setup code of da8xx
based boards to mach-davinci/usb.c file, thereby it avoids repetition of
code.
Patch is based on consolidation concern raised by Sekhar Nori.
https://lkml.org/lkml/2011/12/21/48

Signed-off-by: Manjunathappa, Prakash<prakash.pm@xxxxxx>
---
  arch/arm/mach-davinci/board-da830-evm.c     |   86 +++-----------------
  arch/arm/mach-davinci/board-omapl138-hawk.c |   93 +++-------------------
  arch/arm/mach-davinci/include/mach/da8xx.h  |    2 +
  arch/arm/mach-davinci/include/mach/usb.h    |   32 +++++++-
  arch/arm/mach-davinci/usb.c                 |  116 +++++++++++++++++++++++++++
  drivers/usb/host/ohci-da8xx.c               |   15 ++--
  6 files changed, 178 insertions(+), 166 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index dc1afe5..edc8277 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -46,59 +46,23 @@ static const short da830_evm_usb11_pins[] = {
[...]
-	/* TPS2065 switch @ 5V */
-	.potpgt		= (3 + 1) / 2,	/* 3 ms max */

Why are you removing this initializer? You don't seem to pass it anywhere else...

+	.type			= GPIO_BASED,
+	.method	= {
+		.gpio_method = {
+			.power_control_pin	= ON_BD_USB_DRV,
+			.over_current_indicator = ON_BD_USB_OVC,
+		},
+	},
+	.board_ocic_handler	= da830_evm_usb_ocic_irq,
  };

diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index 45e8157..9214923 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -184,77 +184,34 @@ mmc_setup_wp_fail:
[...]
  static struct da8xx_ohci_root_hub omapl138_hawk_usb11_pdata = {
-	.set_power      = hawk_usb_set_power,
-	.get_power      = hawk_usb_get_power,
-	.get_oci        = hawk_usb_get_oci,
-	.ocic_notify    = hawk_usb_ocic_notify,
-	/* TPS2087 switch @ 5V */
-	.potpgt         = (3 + 1) / 2,  /* 3 ms max */

   Same question.

diff --git a/arch/arm/mach-davinci/include/mach/usb.h b/arch/arm/mach-davinci/include/mach/usb.h
index e0bc4ab..1a6f211 100644
--- a/arch/arm/mach-davinci/include/mach/usb.h
+++ b/arch/arm/mach-davinci/include/mach/usb.h
[...]
@@ -33,25 +35,47 @@
  #define CFGCHIP2_REFFREQ_12MHZ	(1<<  0)
  #define CFGCHIP2_REFFREQ_24MHZ	(2<<  0)
  #define CFGCHIP2_REFFREQ_48MHZ	(3<<  0)

   Empty line needed here.

+enum usb_power_n_ovc_method {
+	GPIO_BASED = 1,
+};

  struct	da8xx_ohci_root_hub;

  typedef void (*da8xx_ocic_handler_t)(struct da8xx_ohci_root_hub *hub,
  				     unsigned port);

+struct gpio_based {
+	u32 power_control_pin;
+	u32 over_current_indicator;

   Aren't GPIOs just 'unsigned' in gpiolib, without size?

+};
+
  /* Passed as the platform data to the OHCI driver */
  struct	da8xx_ohci_root_hub {
  	/* Switch the port power on/off */
-	int	(*set_power)(unsigned port, int on);
+	int	(*set_power)(unsigned port, struct da8xx_ohci_root_hub *hub,
+			int on);
  	/* Read the port power status */
-	int	(*get_power)(unsigned port);
+	int	(*get_power)(unsigned port, struct da8xx_ohci_root_hub *hub);
  	/* Read the port over-current indicator */
-	int	(*get_oci)(unsigned port);
+	int	(*get_oci)(unsigned port, struct da8xx_ohci_root_hub *hub);
  	/* Over-current indicator change notification (pass NULL to disable) */
-	int	(*ocic_notify)(da8xx_ocic_handler_t handler);
+	int	(*ocic_notify)(da8xx_ocic_handler_t handler,
+			struct da8xx_ohci_root_hub *hub);

  	/* Time from power on to power good (in 2 ms units) */
  	u8	potpgt;
+
+	/* Power control and over current control method */
+	unsigned int type;
+	union power_n_overcurrent_pins {
+		struct gpio_based gpio_method;
+		/* Add data pertaining other methods here. For example if its
+		 * I2C based.
+		 */

I thought that you need to first convert the existing GPIO-based method, and think about the extensions when the need arises, in the next patches. You're looking too far ahead in this patch...

+	} method;
+
+	/* board specific handler */
+	irq_handler_t board_ocic_handler;
  };

  void davinci_setup_usb(unsigned mA, unsigned potpgt_ms);
diff --git a/arch/arm/mach-davinci/usb.c b/arch/arm/mach-davinci/usb.c
index 23d2b6d..25c3520 100644
--- a/arch/arm/mach-davinci/usb.c
+++ b/arch/arm/mach-davinci/usb.c
@@ -11,12 +12,70 @@
  #include<mach/irqs.h>
  #include<mach/cputype.h>
  #include<mach/usb.h>
+#include<mach/mux.h>

  #define DAVINCI_USB_OTG_BASE	0x01c64000

  #define DA8XX_USB0_BASE 	0x01e00000
  #define DA8XX_USB1_BASE 	0x01e25000

+static int da8xx_usb_set_power(unsigned port, struct da8xx_ohci_root_hub *hub,
+		int on)
+{
+	struct gpio_based *gpio = (hub->type == GPIO_BASED) ?
+		&hub->method.gpio_method : NULL;
+
+	if (hub->type == GPIO_BASED)
+		gpio_set_value(gpio->power_control_pin, on);

   The code doesn't look very flexible. Why not just use:

	switch (hub->type) {
	case GPIO_BASED:
		{
			struct gpio_based *gpio = &hub->method.gpio_method;

			gpio_set_value(gpio->power_control_pin, on);
		}
	}

+	return 0;
+}
+
+static int da8xx_usb_get_power(unsigned port, struct da8xx_ohci_root_hub *hub)
+{
+	struct gpio_based *gpio = (hub->type == GPIO_BASED) ?
+		&hub->method.gpio_method : NULL;
+
+	if (hub->type == GPIO_BASED)
+		return gpio_get_value(gpio->power_control_pin);
+	return 0;
+}

+static int da8xx_usb_get_oci(unsigned port, struct da8xx_ohci_root_hub *hub)
+{
+	struct gpio_based *gpio = (hub->type == GPIO_BASED) ?
+		&hub->method.gpio_method : NULL;
+
+	if (hub->type == GPIO_BASED)
+		return !gpio_get_value(gpio->over_current_indicator);
+	return 0;
+}

I see another type of inflexibility in the code: you always assume that over-current indicator is an inverted signal. Likewise the power control is always uninverted, in your assumption (more realistic, I admit).

@@ -177,4 +236,61 @@ int __init da8xx_register_usb11(struct da8xx_ohci_root_hub *pdata)
  	da8xx_usb11_device.dev.platform_data = pdata;
  	return platform_device_register(&da8xx_usb11_device);
  }
+
+void __init da8xx_board_usb_init(const short pins[],
+		struct da8xx_ohci_root_hub *usb11_pdata)
+{
[...]
+	/* TPS2087 switch @ 5V */
+	usb11_pdata->potpgt = (3 + 1) / 2;

   This is board specific data, not generic in any way. Leave it were it was.

I can't say I like this generaization result. Certain NAK on implementation details.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux