lkml.org 
[lkml]   [2011]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [Qualcomm PM8921 MFD v2 3/6] gpio: pm8xxx-gpio: Add pm8xxx gpio driver
    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.



    \
     
     \ /
      Last update: 2011-03-16 20:57    [W:2.808 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site