Re: [Qualcomm PM8921 MFD v2 3/6] gpio: pm8xxx-gpio: Add pm8xxx gpio driver

[Abhijeet Dharmapurikar <adharmap@codeaurora.org>]

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