On Wed, Mar 16, 2011 at 11:55:19AM -0700, Abhijeet Dharmapurikar wrote:

quoted

quoted

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 664660e..c5e6f51 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig

@@ -411,4 +411,14 @@ config GPIO_JANZ_TTL

	  This driver provides support for driving the pins in output
	  mode only. Input mode is not supported.
+comment "SSBI GPIO expanders:"

SSBI?  Also, the comment seems rather out of place when there
currently appears to only be one of such devices.

SSBI is a bus architecture used to access this device's register
(actually this is a subdevice in the pmic and ssbi is used to access
all the registers in the pmic).The bus driver can be found here
https://patchwork.kernel.org/patch/601771/

It didnt occur to me that the comment is only meant for buses which
have more than one gpio devices. I saw
comment "MODULbus GPIO expanders:"
comment "AC97 GPIO expanders:"
both containing single devices and thought it is a norm to add a
comment  if a device running on a new bus is introduced.

Let me know if you still think I should remove
comment "SSBI GPIO expanders:" ?

Yes, remove the comment.  I'll probably also remove the comment for
MODULbus and AC97.

g.

quoted

quoted

+
+struct pm_gpio_chip {
+	struct list_head	link;
+	struct gpio_chip	gpio_chip;
+	struct mutex		pm_lock;
+	u8			*bank1;
+	int			irq_base;
+};
+
+static LIST_HEAD(pm_gpio_chips);

Looks like you need a mutex for protecting this list from mutual access.

Yes will fix this.

quoted

quoted

+

quoted

quoted

+#ifndef __PM8XXX_GPIO_H
+#define __PM8XXX_GPIO_H
+
+#include <linux/errno.h>
+
+#define PM8XXX_GPIO_DEV_NAME	"pm8xxx-gpio"
+
+struct pm8xxx_gpio_core_data {
+	u32	rev;
+	int	ngpios;
+};
+
+struct pm8xxx_gpio_platform_data {
+	struct pm8xxx_gpio_core_data	gpio_cdata;
+	int				gpio_base;
+};

There doesn't seem to be any value it splitting pm8xxx_gpio_core_data
into a separate structure from what I see in this patch.  How is this
going to be used?

gpio_base comes from the platform data because the board file knows
where in the global gpio numbers the pm8xxx gpio start. For example
if the MSM code supports 150 gpios(0 through 149), the board file
will set gpio_base to indicate pm8xxx gpios should start from 150.

The pm8xxx_gpio_core_data is meant to be filled in by the pm8921
core. We have different pmic chips with similar gpio
implementations. For example 8058 pmic, 8901 pmic and 8921 pmic, all
have the same gpio implementation but different number of gpio
lines. The pm8xxx-gpio.c is used for all these pmics. Hence we
separated core specific gpio information (number of gpio lines
supported) from board specific gpio information (where in the global
map the gpio number for this device starts).

I could see the argument if multiple pm8xxx instances pointed to a
single pm8xxx_gpio_core_data structure, but in this case it is simply
encapsulated.  Personally I'd just drop the extra structure.

g.