[PATCH] gpio: palmas: add dt support

[PATCH] gpio: palmas: add dt support

[Laxman Dewangan]

Add of_device_id table for Palma GPIO to be enable the
driver from DT file.

The driver can be registered from DT file as:
	palmas: tps65913@58 {
		:::::::::::
		palmas_gpio: palmas_gpio {
			compatible = "ti,palmas-gpio";
			gpio-controller;
			#gpio-cells = <2>;
		};
	};

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpio/gpio-palmas.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpio-palmas.c b/drivers/gpio/gpio-palmas.c
index e3a4e56..fddaa0d 100644
--- a/drivers/gpio/gpio-palmas.c
+++ b/drivers/gpio/gpio-palmas.c
@@ -134,7 +134,7 @@ static int palmas_gpio_probe(struct platform_device *pdev)
 	palmas_gpio->gpio_chip.get	= palmas_gpio_get;
 	palmas_gpio->gpio_chip.dev = &pdev->dev;
 #ifdef CONFIG_OF_GPIO
-	palmas_gpio->gpio_chip.of_node = palmas->dev->of_node;
+	palmas_gpio->gpio_chip.of_node = pdev->dev.of_node;
 #endif
 	palmas_pdata = dev_get_platdata(palmas->dev);
 	if (palmas_pdata && palmas_pdata->gpio_base)
@@ -159,9 +159,18 @@ static int palmas_gpio_remove(struct platform_device *pdev)
 	return gpiochip_remove(&palmas_gpio->gpio_chip);
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id of_palmas_gpio_match[] = {
+	{ .compatible = "ti,palmas-gpio"},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_palmas_gpio_match);
+#endif
+
 static struct platform_driver palmas_gpio_driver = {
 	.driver.name	= "palmas-gpio",
 	.driver.owner	= THIS_MODULE,
+	.driver.of_match_table = of_match_ptr(of_palmas_gpio_match),
 	.probe		= palmas_gpio_probe,
 	.remove		= palmas_gpio_remove,
 };
-- 
1.7.1.1

Re: [PATCH] gpio: palmas: add dt support

[Stephen Warren]

On 03/21/2013 08:30 AM, Laxman Dewangan wrote:
> Add of_device_id table for Palma GPIO to be enable the
> driver from DT file.

On this, the RTC patches, and the regulator patches, I think you should
be Ccing the Slimlogic guys (CC'd now) and perhaps some people from TI
(CC'd now; I hope the right people) , to make sure so that you're all on
the same page w.r.t. how the DT bindings work, and so you don't step on
each-others toes.

Re: [PATCH] gpio: palmas: add dt support

[Linus Walleij]

On Thu, Mar 21, 2013 at 3:30 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

>  #ifdef CONFIG_OF_GPIO
> -       palmas_gpio->gpio_chip.of_node = palmas->dev->of_node;
> +       palmas_gpio->gpio_chip.of_node = pdev->dev.of_node;
>  #endif

OK I think that #ifdef is necessary...

> +#ifdef CONFIG_OF
> +static struct of_device_id of_palmas_gpio_match[] = {
> +       { .compatible = "ti,palmas-gpio"},
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, of_palmas_gpio_match);
> +#endif

But please drop the #ifdef here unless it causes compile errors
(I don't think it will.)

Yours,
Linus Walleij

Re: [PATCH] gpio: palmas: add dt support

[Stephen Warren]

On 03/27/2013 07:00 AM, Linus Walleij wrote:
> On Thu, Mar 21, 2013 at 3:30 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> 
>>  #ifdef CONFIG_OF_GPIO
>> -       palmas_gpio->gpio_chip.of_node = palmas->dev->of_node;
>> +       palmas_gpio->gpio_chip.of_node = pdev->dev.of_node;
>>  #endif
> 
> OK I think that #ifdef is necessary...

Laxman,

Don't we need to resolve and agree on the final DT bindings before we
can start making changes like this? It's not clear yet whether everyone
is on the same page re: how the MFD sub-devices are modelled in DT -
whether each sub-component really is a standalone device, or whether the
MFD itself instantiates all its children based on internal static tables
rather than DT.

Presumably, the answer to that question directly determines whether the
code change above is correct; the correct of_node might be either the
main Palmas node (if DT doesn't represent the MFD components) or it
might be the regulator sub-node (if DT is used to represent MFD components).

Given that, I'm not sure why the Slimlogic people aren't CC'd on this
patch:-(

Re: [PATCH] gpio: palmas: add dt support

[Laxman Dewangan]

On Wednesday 27 March 2013 09:27 PM, Stephen Warren wrote:
> On 03/27/2013 07:00 AM, Linus Walleij wrote:
>> On Thu, Mar 21, 2013 at 3:30 PM, Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>
>>>   #ifdef CONFIG_OF_GPIO
>>> -       palmas_gpio->gpio_chip.of_node = palmas->dev->of_node;
>>> +       palmas_gpio->gpio_chip.of_node = pdev->dev.of_node;
>>>   #endif
>> OK I think that #ifdef is necessary...
> Laxman,
>
> Don't we need to resolve and agree on the final DT bindings before we
> can start making changes like this? It's not clear yet whether everyone
> is on the same page re: how the MFD sub-devices are modelled in DT -
> whether each sub-component really is a standalone device, or whether the
> MFD itself instantiates all its children based on internal static tables
> rather than DT.
Yes, we need to agree on DT. I sent the patch based on the patches came 
from TI/Slimlogic and other discussion about DT patches.
Recently, you commented on  DT for palmas, it need to  be completely IP 
based or hw based. Mix will not work.
If it is IP based then almost all palmas-* file need to be rewrite, no 
one is written as IP based. All are using palma structures, palma 
header, palma macro etc
which should not be there.

>
> Given that, I'm not sure why the Slimlogic people aren't CC'd on this
> patch:-(

I have not added then on my original patch as 
./script/get_maintainers.pl did not give me name.
However, you added immediately them (TI and slimlogic) on this patch 
itself. I have not got any feedback from them yet.



I

Re: [PATCH] gpio: palmas: add dt support

[Laxman Dewangan]

On Wednesday 27 March 2013 06:30 PM, Linus Walleij wrote:
> On Thu, Mar 21, 2013 at 3:30 PM, Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
>> +#ifdef CONFIG_OF
>> +static struct of_device_id of_palmas_gpio_match[] = {
>> +       { .compatible = "ti,palmas-gpio"},
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, of_palmas_gpio_match);
>> +#endif
> But please drop the #ifdef here unless it causes compile errors
> (I don't think it will.)
>

I am using this table as

driver.of_match_table = of_match_ptr(of_palmas_gpio_match),
of_match_ptr is macro which is NULL in case of CONFIG_OF not defined.
So if I remove ifdefs then it may create build warning as unused variable.

Therefore, I think it is require here.

Re: [PATCH] gpio: palmas: add dt support

[Stephen Warren]

On 03/27/2013 11:59 PM, Laxman Dewangan wrote:
> On Wednesday 27 March 2013 06:30 PM, Linus Walleij wrote:
>> On Thu, Mar 21, 2013 at 3:30 PM, Laxman Dewangan
>> <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>
>>> +#ifdef CONFIG_OF
>>> +static struct of_device_id of_palmas_gpio_match[] = {
>>> +       { .compatible = "ti,palmas-gpio"},
>>> +       { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_palmas_gpio_match);
>>> +#endif
>>
>> But please drop the #ifdef here unless it causes compile errors
>> (I don't think it will.)
>>
> 
> I am using this table as
> 
> driver.of_match_table = of_match_ptr(of_palmas_gpio_match),
> of_match_ptr is macro which is NULL in case of CONFIG_OF not defined.
> So if I remove ifdefs then it may create build warning as unused variable.

I think Linus's point is that you can simply remove the use of
of_match_ptr(). The only disadvantage of doing so is that the table will
always be included in the object file, but it's so small that it's
probably not worth worrying about.

Re: [PATCH] gpio: palmas: add dt support

[Linus Walleij]

On Thu, Mar 28, 2013 at 3:55 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On 03/27/2013 11:59 PM, Laxman Dewangan wrote:
>> On Wednesday 27 March 2013 06:30 PM, Linus Walleij wrote:
>>> On Thu, Mar 21, 2013 at 3:30 PM, Laxman Dewangan
>>> <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>
>>>> +#ifdef CONFIG_OF
>>>> +static struct of_device_id of_palmas_gpio_match[] = {
>>>> +       { .compatible = "ti,palmas-gpio"},
>>>> +       { },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, of_palmas_gpio_match);
>>>> +#endif
>>>
>>> But please drop the #ifdef here unless it causes compile errors
>>> (I don't think it will.)
>>>
>>
>> I am using this table as
>>
>> driver.of_match_table = of_match_ptr(of_palmas_gpio_match),
>> of_match_ptr is macro which is NULL in case of CONFIG_OF not defined.
>> So if I remove ifdefs then it may create build warning as unused variable.
>
> I think Linus's point is that you can simply remove the use of
> of_match_ptr(). The only disadvantage of doing so is that the table will
> always be included in the object file, but it's so small that it's
> probably not worth worrying about.

Oh I wasn't that smart :-)

But what you're saying seems true.

The of_match_ptr() is something
I haven't quite seen before and don't quite understand the
semantics of, why would we use that?

Yours,
Linus Walleij

Re: [PATCH] gpio: palmas: add dt support

[Stephen Warren]

On 04/02/2013 03:01 AM, Linus Walleij wrote:
> On Thu, Mar 28, 2013 at 3:55 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 03/27/2013 11:59 PM, Laxman Dewangan wrote:
>>> On Wednesday 27 March 2013 06:30 PM, Linus Walleij wrote:
>>>> On Thu, Mar 21, 2013 at 3:30 PM, Laxman Dewangan
>>>> <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>>
>>>>> +#ifdef CONFIG_OF
>>>>> +static struct of_device_id of_palmas_gpio_match[] = {
>>>>> +       { .compatible = "ti,palmas-gpio"},
>>>>> +       { },
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, of_palmas_gpio_match);
>>>>> +#endif
>>>>
>>>> But please drop the #ifdef here unless it causes compile errors
>>>> (I don't think it will.)
>>>>
>>>
>>> I am using this table as
>>>
>>> driver.of_match_table = of_match_ptr(of_palmas_gpio_match),
>>> of_match_ptr is macro which is NULL in case of CONFIG_OF not defined.
>>> So if I remove ifdefs then it may create build warning as unused variable.
>>
>> I think Linus's point is that you can simply remove the use of
>> of_match_ptr(). The only disadvantage of doing so is that the table will
>> always be included in the object file, but it's so small that it's
>> probably not worth worrying about.
> 
> Oh I wasn't that smart :-)
> 
> But what you're saying seems true.
> 
> The of_match_ptr() is something
> I haven't quite seen before and don't quite understand the
> semantics of, why would we use that?

Very roughly,

of_match_ptr(x) == CONFIG_OF ? x : NULL

(although with ifdefs rather than the ternary operator)

So, if you have ifdef'd the match table with CONFIG_OF, and reference it
from code outside that ifdef, then you need to use of_match_ptr() to
reference the table, so it isn't referenced when CONFIG_OF is not
defined, and hence never causes link problems.