[RFC] drivers/gpio/gpio-pca953x.c broken for PCA9505 ?

[RFC] drivers/gpio/gpio-pca953x.c broken for PCA9505 ?

[Oza Oza]

Hi,

Our SOC has PCA9505 IO expander,
Basically I was implementing PCI hotplug, e.g. PCI present lines got
to IO expander.

I had to fix the driver at some places.
but I am not sure, if driver was written considering 9505 chip ?
in fact it declares 9505 as PCA953X_TYPE.

some of the fixes which I have made are

1) #define PCAL953X_INT_MASK       36  (it was 37 before)
pca953x_write_regs_24  has very odd way of calculating register offset.

int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
i2c write to
reg << bank_shift

which was coming 37 << 3 as 0x128 and which is u8 data type.
will address 0x28 which is invalid locaiton.

36 << 3 as 0x120, which will address 0x20 correctly.
although again thsi is odd way of deriving offset.
I am following datasheet:
http://www.nxp.com/docs/en/data-sheet/PCA9505_9506.pdf

2) #define PCAL953X_IN_LATCH       34
above LATCH is not present.

3) #define PCAL953X_INT_STAT       38
above also points to indirect locaiton.

please suggest if this diver was written with 9505 in mind ?

Regards,
Oza.

Re: [RFC] drivers/gpio/gpio-pca953x.c broken for PCA9505 ?

[Phil Reid]

G'day Oza,

On 20/07/2017 18:07, Oza Oza wrote:
> Hi,
> 
> Our SOC has PCA9505 IO expander,
> Basically I was implementing PCI hotplug, e.g. PCI present lines got
> to IO expander.
> 
> I had to fix the driver at some places.
> but I am not sure, if driver was written considering 9505 chip ?
> in fact it declares 9505 as PCA953X_TYPE.
> 
> some of the fixes which I have made are
> 
> 1) #define PCAL953X_INT_MASK       36  (it was 37 before)
This looks to be for PCA_PCAL flaged devices only.
The pca9505 is not one of these.

The following address calcs are for the wrong chip type.

> pca953x_write_regs_24  has very odd way of calculating register offset.
> 
> int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> i2c write to
> reg << bank_shift
> 
> which was coming 37 << 3 as 0x128 and which is u8 data type.
> will address 0x28 which is invalid locaiton.
> 
> 36 << 3 as 0x120, which will address 0x20 correctly.
> although again thsi is odd way of deriving offset.
> I am following datasheet:
> http://www.nxp.com/docs/en/data-sheet/PCA9505_9506.pdf
> 
> 2) #define PCAL953X_IN_LATCH       34
> above LATCH is not present.
> 
> 3) #define PCAL953X_INT_STAT       38
> above also points to indirect locaiton.
> 
> please suggest if this diver was written with 9505 in mind ?

The driver looks fine for pca9505 without interrupt support.
I think it needs modification to enable irq's for the 9505.
Which would new define like
#define PCA953X_MASK	4

and some logic to set it pca953x_irq_bus_sync_unlock() I think.



-- 
Regards
Phil Reid