On Wed, Mar 16, 2011 at 11:55:19AM -0700, Abhijeet Dharmapurikar wrote: > > >> > >>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. > > >>+ > >>+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. > > > > >>+ > > >>+#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. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
