[U-Boot] [PATCH] dm: gpio: pca953x: introduce driver model support for pca953x

[Peng Fan]

Hi Michal,
On Tue, Apr 12, 2016 at 07:17:55AM +0200, Michal Simek wrote:
>On 12.4.2016 03:25, Peng Fan wrote:
>> Hi Michal,
>> 
>> On Mon, Apr 11, 2016 at 02:40:06PM +0200, Michal Simek wrote:
>>> On 11.4.2016 14:09, Michal Simek wrote:
>>>> On 11.4.2016 07:47, Peng Fan wrote:
>>>>> On Sat, Apr 09, 2016 at 12:33:34PM -0600, Simon Glass wrote:
>>>>>> On 18 March 2016 at 03:54, Peng Fan <van.freenix at gmail.com> wrote:
>>>>>>> Introduce a new driver that supports driver model for pca953x.
>>>>>>> The pca953x chips are used as I2C I/O expanders.
>>>>>>> This driver is designed to support the following chips:
>>>>>>> "
>>>>>>> 4 bits: pca9536, pca9537
>>>>>>> 8 bits: max7310, max7315, pca6107, pca9534, pca9538, pca9554,
>>>>>>>         pca9556, pca9557, pca9574, tca6408, xra1202
>>>>>>> 16 bits: max7312, max7313, pca9535, pca9539, pca9555, pca9575,
>>>>>>>          tca6416
>>>>>>> 24 bits: tca6424
>>>>>>> 40 bits: pca9505, pca9698
>>>>>>> "
>>>>>>> But for now this driver only supports max 24 bits and pca953x compatible
>>>>>>> chips. pca957x compatible chips are not supported now.
>>>>>>> These can be addressed when we need to add such support for the different
>>>>>>> chips.
>>>>>>> This driver has been tested on i.MX6 SoloX Sabreauto board with max7310
>>>>>>> i2c expander using gpio command as following:
>>>>>>>
>>>>>>> =>gpio status -a
>>>>>>> Bank gpio at 48:
>>>>>>> gpio at 480: input: 1 [ ]
>>>>>>> => gpio clear gpio at 480
>>>>>>> gpio: pin gpio at 480 (gpio 224) value is 0
>>>>>>> => gpio status -a
>>>>>>> Bank gpio at 48:
>>>>>>> gpio at 480: output: 0 [ ]
>>>>>>
>>>>>> Don't you think 480 is confusing? Perhaps you should have gpio at 48_ as
>>>>>> the bank name? Also I think you should support a gpio-bank-name
>>>>>> property in the node, to allow a sensible name to be provided.
>>>>>
>>>>> 480 is added by gpio uclass driver I think.
>>>>> The dts is copied from Linux side. I'd not change the dts, will try to
>>>>> see how to introudce a sensible name here.
>>>>
>>>> What's the binding you are using?
>>>>
>>>> The part of this patch should be DT binding.
>>>>
>>>> This is my node.
>>>> 	tca6416_u61: gpio at 21 {
>>>> 		compatible = "ti,tca6416";
>>>> 		reg = <0x21>;
>>>> 		gpio-controller;
>>>> 		#gpio-cells = <2>;
>>>> 	};
>>>>
>>>
>>> Ok. I found where the problem is.
>>>
>>> i2c bus has
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>>
>>> And then OF_CHECK_COUNTS thinks that it is incorrect setting for bus
>>> but it should be valid for i2c where only address without size is used.
>>> When I apply patch below driver is probed
>> 
>> I did not met this issue.
>
>That's interesting. Can you please add
>
>Can you please apply this and look at na and ns values?
>If you are on imx6q then you should reach the same problem as I if this
>code is called that's why I would like to confirm this.
>
>diff --git a/common/fdt_support.c b/common/fdt_support.c
>index ced119e70d9f..9c18b312d647 100644
>--- a/common/fdt_support.c
>+++ b/common/fdt_support.c
>@@ -1109,6 +1109,7 @@ static u64 __of_translate_address(void *blob, int
>node_offset, const fdt32_t *in
>
>        /* Cound address cells & copy address locally */
>        bus->count_cells(blob, parent, &na, &ns);
>+       printf("addr cells %d, size cells %d\n", na, ns);
>        if (!OF_CHECK_COUNTS(na, ns)) {
>                printf("%s: Bad cell count for %s\n", __FUNCTION__,
>                       fdt_get_name(blob, node_offset, NULL));
>
>
>> 
>>>
>>>
>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>> index ced119e70d9f..5f5b49c6210b 100644
>>> --- a/common/fdt_support.c
>>> +++ b/common/fdt_support.c
>>> @@ -941,7 +941,7 @@ void fdt_del_node_and_alias(void *blob, const char
>>> *alias)
>>> #define OF_MAX_ADDR_CELLS      4
>>> #define OF_BAD_ADDR    FDT_ADDR_T_NONE
>>> #define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <=
>>> OF_MAX_ADDR_CELLS && \
>>> -                       (ns) > 0)
>>> +                       (ns) >= 0)

You are correct.

I have no idea why I first test my patch, I did not met the issue as you.
After applying your patch as above, gpio status -a can correctly detect
max7310.

The dts I used is for mx6sxsabreauto board, not in U-Boot now.

Thanks,
Peng.

>>>
>>> /* Debug utility */
>>> #ifdef DEBUG
>>>
>>> Regarding numbers. It is just hex to int conversion + gpio number.
>>> It means gpio at 20 is 0x20 which is 32. Using hex everywhere make sense
>>> and this is the fix. Maybe better to also use underscore.
>>>
>>> diff --git a/drivers/gpio/pca953x_gpio.c b/drivers/gpio/pca953x_gpio.c
>>> index 6b67b079a17b..1eeb4c8f199a 100644
>>> --- a/drivers/gpio/pca953x_gpio.c
>>> +++ b/drivers/gpio/pca953x_gpio.c
>>> @@ -292,7 +292,7 @@ static int pca953x_probe(struct udevice *dev)
>>>                return ret;
>>>        }
>>>
>>> -       snprintf(name, sizeof(name), "gpio@%u", info->addr);
>>> +       snprintf(name, sizeof(name), "gpio@%x_", info->addr);
>>>        str = strdup(name);
>>>        if (!str)
>>>                return -ENOMEM;
>> 
>> Thanks for this.
>> Also Would you mind I add your Tested-By in V2?
>
>I will when this is fixed and we know more about that issue above.
>
>Thanks,
>Michal
>
>