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

>>
>> 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:" ?

>> +
>> +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).

--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm
Innovation Center, Inc. is a member of the Code Aurora Forum.


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