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

[Peng Fan <van.freenix@gmail.com>]

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

>
>
>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)
>
> /* 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?

Thanks,
Peng.

>
>Output below.
>
>Thanks,
>Michal
>
>ZynqMP> dm tree
> Class       Probed   Name
>----------------------------------------
> root        [ + ]    root_driver
> simple_bus  [   ]    |-- amba_apu
> simple_bus  [ + ]    `-- amba
> eth         [ + ]        |-- ethernet at ff0e0000
> i2c         [ + ]        |-- i2c at ff020000
> gpio        [ + ]        |   |-- gpio at 20
> gpio        [ + ]        |   |-- gpio at 21
> i2c_mux     [   ]        |   `-- i2cswitch at 75
> i2c         [   ]        |       |-- i2c at 0
> i2c         [   ]        |       |-- i2c at 1
> i2c         [   ]        |       `-- i2c at 2
> i2c         [   ]        |-- i2c at ff030000
> i2c_mux     [   ]        |   |-- i2cswitch at 74
> i2c         [   ]        |   |   |-- i2c at 0
> i2c         [   ]        |   |   |-- i2c at 1
> i2c         [   ]        |   |   |-- i2c at 2
> i2c         [   ]        |   |   |-- i2c at 3
> i2c         [   ]        |   |   `-- i2c at 4
> i2c_mux     [   ]        |   `-- i2cswitch at 75
> i2c         [   ]        |       |-- i2c at 0
> i2c         [   ]        |       |-- i2c at 1
> i2c         [   ]        |       |-- i2c at 2
> i2c         [   ]        |       |-- i2c at 3
> i2c         [   ]        |       |-- i2c at 4
> i2c         [   ]        |       |-- i2c at 5
> i2c         [   ]        |       |-- i2c at 6
> i2c         [   ]        |       `-- i2c at 7
> mmc         [ + ]        |-- sdhci at ff170000
> serial      [ + ]        |-- serial at ff000000
> serial      [   ]        `-- serial at ff010000
>ZynqMP> i2c bus
>Bus 0:	i2c at ff020000  (active 0)
>   20: gpio at 20, offset len 1, flags 0
>   21: gpio at 21, offset len 1, flags 0
>   75: i2cswitch at 75, offset len 1, flags 0
>Bus 750:	i2c at 0
>Bus 751:	i2c at 1
>Bus 752:	i2c at 2
>Bus 1:	i2c at ff030000
>   74: i2cswitch at 74, offset len 1, flags 0
>   75: i2cswitch at 75, offset len 1, flags 0
>Bus 750:	i2c at 0
>Bus 751:	i2c at 1
>Bus 752:	i2c at 2
>Bus 1743:	i2c at 3
>Bus 1744:	i2c at 4
>Bus 750:	i2c at 0
>Bus 751:	i2c at 1
>Bus 752:	i2c at 2
>Bus 1743:	i2c at 3
>Bus 1744:	i2c at 4
>Bus 1755:	i2c at 5
>Bus 1756:	i2c at 6
>Bus 1757:	i2c at 7
>ZynqMP> gpio status -a
>Bank gpio at 20_:
>gpio at 20_0: output: 1 [ ]
>gpio at 20_1: output: 1 [ ]
>gpio at 20_2: output: 1 [ ]
>gpio at 20_3: output: 1 [ ]
>gpio at 20_4: output: 0 [ ]
>gpio at 20_5: output: 1 [ ]
>gpio at 20_6: output: 1 [ ]
>gpio at 20_7: output: 1 [ ]
>gpio at 20_8: output: 0 [ ]
>gpio at 20_9: output: 0 [ ]
>gpio at 20_10: output: 0 [ ]
>gpio at 20_11: output: 0 [ ]
>gpio at 20_12: output: 0 [ ]
>gpio at 20_13: output: 0 [ ]
>gpio at 20_14: output: 0 [ ]
>gpio at 20_15: output: 0 [ ]
>
>Bank gpio at 21_:
>gpio at 21_0: input: 1 [ ]
>gpio at 21_1: input: 1 [ ]
>gpio at 21_2: input: 1 [ ]
>gpio at 21_3: input: 1 [ ]
>gpio at 21_4: input: 1 [ ]
>gpio at 21_5: input: 0 [ ]
>gpio at 21_6: input: 0 [ ]
>gpio at 21_7: input: 0 [ ]
>gpio at 21_8: input: 0 [ ]
>gpio at 21_9: input: 0 [ ]
>gpio at 21_10: input: 0 [ ]
>gpio at 21_11: input: 0 [ ]
>gpio at 21_12: input: 0 [ ]
>gpio at 21_13: input: 0 [ ]
>gpio at 21_14: input: 0 [ ]
>gpio at 21_15: input: 0 [ ]
>ZynqMP>
>