Re: [PATCH v10 0/12] Palmas updates

Re: [PATCH v10 0/12] Palmas updates

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

Hi Ian,

On Fri, 22 Mar 2013 14:55:10 +0000 Ian Lartey <ian@slimlogic.co.uk> wrote:
>
> This patchset adds to the support for the Palmas series of PMIC chips.
> 
> Some of the patches have previously been submitted individually.
> The DT bindings doc has been added first due to comments that it was missing.
> 
> Patches based on linux-next-20130319

I can't really comment on the patch set except to say that you should not
base on linux-next, you should base on the tree to which you expect the
patch set to be applied.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v10 04/12] watchdog: add Palmas Watchdog support

[Guenter Roeck]

On Fri, Mar 22, 2013 at 02:55:14PM +0000, Ian Lartey wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
> 
> Add support for the Palmas watchdog timer which has a timeout configurable
> from 1s to 128s.
> 
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Ian Lartey <ian@slimlogic.co.uk>
> ---
>  drivers/watchdog/palmas_wdt.c |  169 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 169 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/watchdog/palmas_wdt.c
> 
> diff --git a/drivers/watchdog/palmas_wdt.c b/drivers/watchdog/palmas_wdt.c
> new file mode 100644
> index 0000000..da7e379
> --- /dev/null
> +++ b/drivers/watchdog/palmas_wdt.c
> @@ -0,0 +1,169 @@
> +/*
> + * Driver for Watchdog part of Palmas PMIC Chips
> + *
> + * Copyright 2011-2013 Texas Instruments Inc.
> + *
> + * Author: Graeme Gregory <gg@slimlogic.co.uk>
> + * Author: Ian Lartey <ian@slimlogic.co.uk>
> + *
> + * Based on twl4030_wdt.c
> + *
> + * Author: Timo Kokkonen <timo.t.kokkonen at nokia.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under  the terms of the GNU General  Public License as published by the
> + *  Free Software Foundation;  either version 2 of the License, or (at your
> + *  option) any later version.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/fs.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/mfd/palmas.h>
> +
> +struct palmas_wdt {
> +	struct palmas *palmas;
> +	struct watchdog_device wdt;
> +	struct device *dev;
> +
> +	int timer_margin;
> +};
> +
> +
One empty line would be sufficient.

> +static int nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, int, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> +	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static int palmas_wdt_write(struct palmas *palmas, unsigned int data)
> +{
> +	unsigned int addr;
> +
> +	addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_WATCHDOG);
> +
> +	return palmas_write(palmas, PALMAS_PMU_CONTROL_BASE, addr, addr);

2 x addr ? Should the second one (last parameter) be data ?

> +}
> +
> +static int palmas_wdt_enable(struct watchdog_device *wdt)
> +{
> +	struct palmas_wdt *driver_data = watchdog_get_drvdata(wdt);
> +	struct palmas *palmas = driver_data->palmas;
> +
> +	return palmas_wdt_write(palmas, driver_data->timer_margin |
> +				PALMAS_WATCHDOG_ENABLE);
> +}
> +
> +static int palmas_wdt_disable(struct watchdog_device *wdt)
> +{
> +	struct palmas_wdt *driver_data = watchdog_get_drvdata(wdt);
> +	struct palmas *palmas = driver_data->palmas;
> +
> +	return palmas_wdt_write(palmas, driver_data->timer_margin);

Just wondering - why not just write 0 ?

> +}
> +
> +static int palmas_wdt_set_timeout(struct watchdog_device *wdt, unsigned timeout)
> +{
> +	struct palmas_wdt *driver_data = watchdog_get_drvdata(wdt);
> +
> +	if (timeout < 1 || timeout > 128) {
> +		dev_warn(driver_data->dev,
> +			"Timeout can only be in the range [1-128] seconds");
> +		return -EINVAL;
> +	}
The watchdog core supports limit checking. Might as well use it.

On a side note, it might be an interesting experience if you try setting
a value of 0 or larger than 128. Unless I am missing something,
driver_data->dev is never initialized.

> +	driver_data->timer_margin = fls(timeout) - 1;
> +	return 0;
> +}
> +
> +static const struct watchdog_info palmas_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +	.identity = "Palmas Watchdog",
> +	.firmware_version = 0,
> +};
> +
> +static const struct watchdog_ops palmas_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = palmas_wdt_enable,
> +	.stop = palmas_wdt_disable,
> +	.ping = palmas_wdt_enable,
> +	.set_timeout = palmas_wdt_set_timeout,
> +};
> +
> +static int palmas_wdt_probe(struct platform_device *pdev)
> +{
> +	struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
> +	struct palmas_wdt *driver_data;
> +	struct watchdog_device *palmas_wdt;
> +	int ret = 0;
> +
> +	driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data),
> +				   GFP_KERNEL);
> +	if (!driver_data) {
> +		dev_err(&pdev->dev, "Unable to alloacate watchdog device\n");

AFAIK devm_ functions already issue a message on errors, so it is unnecessary
to issue another one here.

> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	driver_data->palmas = palmas;
> +
> +	palmas_wdt = &driver_data->wdt;
> +
> +	palmas_wdt->info = &palmas_wdt_info;
> +	palmas_wdt->ops = &palmas_wdt_ops;
> +	watchdog_set_nowayout(palmas_wdt, nowayout);
> +	watchdog_set_drvdata(palmas_wdt, driver_data);
> +

There is no call to watchdog_init_timeout, and the timeout value is not initialized
in palmas_wdt. So the driver depends on the limit being set from user space
with WDIOC_SETTIMEOUT. But that can not happen until after it was opened
and started, meaning it is initially 0 when the watchdog is started.
Is that on purpose ? A more common default would be 60 seconds.

Also, if user space does not explicitly set the timeout but requests it
with WDIOC_GETTIMEOUT, it will get a value of 0.

> +	ret = watchdog_register_device(&driver_data->wdt);
> +	if (ret) {
> +		platform_set_drvdata(pdev, NULL);

It is unnecessary to set the platform data to NULL. Besides, it isn't
even set yet (you only set it with dev_set_drvdata below).

> +		goto err;
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, driver_data);

Might use platform_set_drvdata.

> +
> +	return 0;
> +err:
> +	return ret;

Personal style, but I think this is unnecessary. Coding style examples suggest
to return directly in such cases (ie if there is no cleanup to do).

> +}
> +
> +static int palmas_wdt_remove(struct platform_device *pdev)
> +{
> +	struct palmas_wdt *driver_data = dev_get_drvdata(&pdev->dev);
> +
platform_get_drvdata ?

> +	watchdog_unregister_device(&driver_data->wdt);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id of_palmas_match_tbl[] = {
> +	{ .compatible = "ti,palmas-wdt", },
> +	{ .compatible = "ti,twl6035-wdt", },
> +	{ .compatible = "ti,twl6036-wdt", },
> +	{ .compatible = "ti,twl6037-wdt", },
> +	{ .compatible = "ti,tps65913-wdt", },
> +	{ .compatible = "ti,tps65914-wdt", },
> +	{ .compatible = "ti,tps80036-wdt", },
> +	{ /* end */ }
> +};
> +
> +static struct platform_driver palmas_wdt_driver = {
> +	.probe = palmas_wdt_probe,
> +	.remove = palmas_wdt_remove,
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_palmas_match_tbl,
> +		.name = "palmas-wdt",
> +	},
> +};
> +
> +module_platform_driver(palmas_wdt_driver);
> +
> +MODULE_AUTHOR("Graeme Gregory <gg@slimlogic.co.uk>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:palmas-wdt");
> +MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
> -- 
> 1.7.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
> 

Re: [PATCH v10 05/12] watchdog: Kconfig for Palmas watchdog

[Guenter Roeck]

On Fri, Mar 22, 2013 at 02:55:15PM +0000, Ian Lartey wrote:
> Add the Kconfig and Makefile for the Palmas watchdog driver
> 
> Signed-off-by: Ian Lartey <ian@slimlogic.co.uk>
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> ---

I think this patch should be merged with the previous one.

Guenter

Re: [PATCH v10 0/12] Palmas updates

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

On Sat, Mar 23, 2013 at 06:04:55AM +1100, Stephen Rothwell wrote:
> On Fri, 22 Mar 2013 14:55:10 +0000 Ian Lartey <ian@slimlogic.co.uk> wrote:

> > Patches based on linux-next-20130319

> I can't really comment on the patch set except to say that you should not
> base on linux-next, you should base on the tree to which you expect the
> patch set to be applied.

It's fairly common to suggest to people working over lots of subsystems
like this that they use -next since for most subsystems it contains the
trees that should be submitted against and it's much more efficient.
This works OK most of the time when people are posting patches as
opposed to pull requests, though obviously cross tree issues do happen
from time to time.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v10 12/12] regulator: palmas remove palmas-charger option from DT bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 128 bytes --]

On Fri, Mar 22, 2013 at 02:55:22PM +0000, Ian Lartey wrote:
> Signed-off-by: Ian Lartey <ian@slimlogic.co.uk>

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v10 01/12] mfd: DT bindings for the palmas family MFD

[Stephen Warren]

On 03/22/2013 08:55 AM, Ian Lartey wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
> 
> Add the various binding files for the palmas family of chips. There is a
> top level MFD binding then a seperate binding for IP blocks on chips.

> diff --git a/Documentation/devicetree/bindings/clock/palmas-clk.txt b/Documentation/devicetree/bindings/clock/palmas-clk.txt

Where is the reg property; if you're instantiating the clk device as a
standalone DT node and driver, it should be possible to retrieve the
address of this IP block instance from DT, not using driver-internal APIs.

This same comment likely applies everywhere, so I won't repeat it.

> +Example:
> +
> +clk {
> +    compatible = "ti,twl6035-clk", "ti,palmas-clk";
> +    ti,clk32kg-mode-sleep = <0>;
> +    ti,clk32kgaudio-mode-sleep = <0>;

> +    #clock-cells = <1>;
> +    clock-frequency = <32000000>;
> +    clock-names = "clk32kg", "clk32kgaudio";

The binding description itself should describe what clocks this node
provides and consumes.

What is clock-frequency; which clock does it affect?

The presence of #clock-cells implies this is a clock provider. This
binding should define the format of the clock cells that this property
implies. I assume it's just the ID of the output clocks, but what values
correspond to which output clocks? That mapping table needs to be listed
here.

The presence of clock-names implies this is a clock consumer. However,
there is no clocks property in the example. Is clks missing from the
example, or should this property be clock-output-names instead? If this
node is a clock consumer, the list of which clocks it requires should be
documented.

> diff --git a/Documentation/devicetree/bindings/gpio/gpio-palmas.txt b/Documentation/devicetree/bindings/gpio/gpio-palmas.txt

> +- gpio-controller: mark the device as a GPIO controller
> +- gpio-cells = <1>:  GPIO lines are provided.

That's #gpio-cells not gpio-cells.

I assume that cell simply contains the GPIO ID/number. That should be
documented for clarity.

Surely gpio-cells should be 2 not 1, so there is space for flags. At
least an "inverted" or "active-low" flag should be included; GPIO
consumers would typically implement that flag in SW, so there' no
requirement that the flag only be supported if the HW supports the feature.

> +- interrupt-controller : palmas has its own internal IRQs
> +- #interrupt-cells : should be set to 2 for IRQ number and flags
> +  The first cell is the IRQ number.
> +  The second cell is the flags, encoded as the trigger masks from
> +  Documentation/devicetree/bindings/interrupts.txt

You need "/interrupt-controller" before the filename there.

> +- interrupt-parent : The parent interrupt controller.

If this IP block has interrupt outputs, it should require an
"interrupts" property too. This is the same concept that drives the need
for a reg property. This same comment likely applies everywhere, so I
won't repeat it.

> diff --git a/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt b/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt

> +- interrupts: the interrupt outputs of the controller.

How many entries are there, what do they mean, and in what order must
they appear? (Note that the binding of a node must define the order of
the interrupts property, since historically it's been accessed by index,
not by name, and all bindings must allow that access method to be used).

> +- interrupt-names : Should be the name of irq resource. Each interrupt
> +  binds its interrupt-name.

The binding needs to define standard names for the entries in this
property, or define that interrupts are only retrieved by index, in
which case interrupt-names shouldn't be present. This same comment
likely applies everywhere, so I won't repeat it.

> diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt

> +Required properties:
...

I believe the Palmas devices have many separate I2C addresses, even
buses, which are I think are at least partially independently SW
configurable. In that case, the reg property for this node should
probably enumerate all the I2C addresses that this chip responds to, so
that they can each be passed down to the child nodes.

> diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt

> +Optional nodes:
> +- regulators : should contain the constrains and init information for the
> +	       regulators. It should contain a subnode per regulator from the
> +	       list.
> +	       For ti,palmas-pmic - smps12, smps123, smps3 depending on OTP,
> +	       smps45, smps457, smps7 depending on varient, smps6, smps[8-10],
> +	       ldo[1-9], ldoln, ldousb
> +	       For ti,palmas-charger-pmic - smps12, smps123, smps3 depending on OTP,
> +	       smps[6-9], boost, ldo[1-14], ldoln, ldousb

The list of legal compatible values for this node above doesn't include
both ti,palmas-pmic and ti,palmas-charger-pmic. Should it? This node
should describe this PMIC block in a completely standalone fashion,
without the need to go look at the top-level node to see if it's a
"charger" variant or not.

> +	       optional chip specific regulator fields :-
> +	       ti,warm-reset - maintain voltage during warm reset
> +	       ti,roof-floor - control voltage selection by pin

I assume those are Boolean not integer. It's worth mentioning the type
of each property.

Re: [PATCH v10 01/12] mfd: DT bindings for the palmas family MFD

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]

On Mon, Mar 25, 2013 at 11:59:11AM -0600, Stephen Warren wrote:
> On 03/22/2013 08:55 AM, Ian Lartey wrote:
> > From: Graeme Gregory <gg@slimlogic.co.uk>

> > +Optional nodes:
> > +- regulators : should contain the constrains and init information for the
> > +	       regulators. It should contain a subnode per regulator from the
> > +	       list.
> > +	       For ti,palmas-pmic - smps12, smps123, smps3 depending on OTP,
> > +	       smps45, smps457, smps7 depending on varient, smps6, smps[8-10],
> > +	       ldo[1-9], ldoln, ldousb
> > +	       For ti,palmas-charger-pmic - smps12, smps123, smps3 depending on OTP,
> > +	       smps[6-9], boost, ldo[1-14], ldoln, ldousb

> The list of legal compatible values for this node above doesn't include
> both ti,palmas-pmic and ti,palmas-charger-pmic. Should it? This node
> should describe this PMIC block in a completely standalone fashion,
> without the need to go look at the top-level node to see if it's a
> "charger" variant or not.

The latter was removed from the code in this series, only palmas-pmic is
present now.

Just as a general thing there seems to be an awful lot of stuff here
about the boilerplate for the generic properties like the interrupt
and GPIO controller stuff - we probably need to spin round and look at
factoring this out to make life easier.  There seems to be a lot of
boiler plate in the bindings that is factored out well by the frameworks
in the code so people don't even need to think about it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v10 0/12] Palmas updates

[Samuel Ortiz]

Hi Ian,

On Fri, Mar 22, 2013 at 02:55:10PM +0000, Ian Lartey wrote:
> This patchset adds to the support for the Palmas series of PMIC chips.
> 
> Some of the patches have previously been submitted individually.
> The DT bindings doc has been added first due to comments that it was missing.
> 
> Patches based on linux-next-20130319
Are you expecting all those patches to go through the MFD tree ?
Also, you stil have a few comments from Milo and Stephen to adress. Are you
planning to send a v11 ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH v10 03/12] mfd: palmas add variant and OTP detection

[Samuel Ortiz]

Hi Ian,

On Fri, Mar 22, 2013 at 02:55:13PM +0000, Ian Lartey wrote:
> @@ -278,20 +329,20 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
>  	int ret;
>  	u32 prop;
>  
> -	ret = of_property_read_u32(node, "ti,mux_pad1", &prop);
> +	ret = of_property_read_u32(node, "ti,mux-pad1", &prop);
>  	if (!ret) {
>  		pdata->mux_from_pdata = 1;
>  		pdata->pad1 = prop;
>  	}
>  
> -	ret = of_property_read_u32(node, "ti,mux_pad2", &prop);
> +	ret = of_property_read_u32(node, "ti,mux-pad2", &prop);
>  	if (!ret) {
>  		pdata->mux_from_pdata = 1;
>  		pdata->pad2 = prop;
>  	}
>  
>  	/* The default for this register is all masked */
> -	ret = of_property_read_u32(node, "ti,power_ctrl", &prop);
> +	ret = of_property_read_u32(node, "ti,power-ctrl", &prop);
>  	if (!ret)
>  		pdata->power_ctrl = prop;
>  	else
> @@ -309,7 +360,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
>  	struct palmas_platform_data *pdata;
>  	struct device_node *node = i2c->dev.of_node;
>  	int ret = 0, i;
> -	unsigned int reg, addr;
> +	unsigned int reg;
>  	int slave;
>  	struct mfd_cell *children;
The '-' to '_' fix has been sent by J Keerthy and is on my mfd-next tree
aready. Would you mind removing it from this patch ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH v10 02/12] mfd: palmas: is_palmas_charger needed by multiple drivers

[Samuel Ortiz]

Hi Ian,

On Fri, Mar 22, 2013 at 02:55:12PM +0000, Ian Lartey wrote:
> is_palmas_charger checks for the presence of charging
> functionality in the device
> 
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Ian Lartey <ian@slimlogic.co.uk>
> Acked-by: Laxman Dewangani <ldewangan@nvidia.com>
> ---
>  include/linux/mfd/palmas.h |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
I applied this one now, thanks.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH v10 0/12] Palmas updates

[Graeme Gregory]

On 05/04/13 17:30, Samuel Ortiz wrote:
> Hi Ian,
>
> On Fri, Mar 22, 2013 at 02:55:10PM +0000, Ian Lartey wrote:
>> This patchset adds to the support for the Palmas series of PMIC chips.
>>
>> Some of the patches have previously been submitted individually.
>> The DT bindings doc has been added first due to comments that it was missing.
>>
>> Patches based on linux-next-20130319
> Are you expecting all those patches to go through the MFD tree ?
> Also, you stil have a few comments from Milo and Stephen to adress. Are you
> planning to send a v11 ?
>
>
Hi Samuel,

The device is an MFD so I suspect the best path is through MFD tree
although I notice you have applied the patch that was required by all
other patches so maybe this is not so much the case now!

Real life has got in the way of the next updated series of patches but
they will be issued just as soon as we can.

Thanks

Graeme

Re: [PATCH v10 0/12] Palmas updates

[Samuel Ortiz]

Hi Graeme,

On Mon, Apr 08, 2013 at 04:55:58PM +0100, Graeme Gregory wrote:
> On 05/04/13 17:30, Samuel Ortiz wrote:
> > Hi Ian,
> >
> > On Fri, Mar 22, 2013 at 02:55:10PM +0000, Ian Lartey wrote:
> >> This patchset adds to the support for the Palmas series of PMIC chips.
> >>
> >> Some of the patches have previously been submitted individually.
> >> The DT bindings doc has been added first due to comments that it was missing.
> >>
> >> Patches based on linux-next-20130319
> > Are you expecting all those patches to go through the MFD tree ?
> > Also, you stil have a few comments from Milo and Stephen to adress. Are you
> > planning to send a v11 ?
> >
> >
> Hi Samuel,
> 
> The device is an MFD so I suspect the best path is through MFD tree
Yes, that's fine. I just need to make sure everyone is on sync.


> although I notice you have applied the patch that was required by all
> other patches so maybe this is not so much the case now!
> 
> Real life has got in the way of the next updated series of patches but
> they will be issued just as soon as we can.
I know about real life getting in the way, no worries ;)

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH v10 01/12] mfd: DT bindings for the palmas family MFD

[keerthy]

On 03/25/2013 11:29 PM, Stephen Warren wrote:

> On 03/22/2013 08:55 AM, Ian Lartey wrote:
>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>
>> Add the various binding files for the palmas family of chips. There is a
>> top level MFD binding then a seperate binding for IP blocks on chips.
> 
>> diff --git a/Documentation/devicetree/bindings/clock/palmas-clk.txt b/Documentation/devicetree/bindings/clock/palmas-clk.txt
> 
> Where is the reg property; if you're instantiating the clk device as a
> standalone DT node and driver, it should be possible to retrieve the
> address of this IP block instance from DT, not using driver-internal APIs.
> 
> This same comment likely applies everywhere, so I won't repeat it.
> 
>> +Example:
>> +
>> +clk {
>> +    compatible = "ti,twl6035-clk", "ti,palmas-clk";
>> +    ti,clk32kg-mode-sleep = <0>;
>> +    ti,clk32kgaudio-mode-sleep = <0>;
> 
>> +    #clock-cells = <1>;
>> +    clock-frequency = <32000000>;
>> +    clock-names = "clk32kg", "clk32kgaudio";
> 
> The binding description itself should describe what clocks this node
> provides and consumes.
> 
> What is clock-frequency; which clock does it affect?
> 
> The presence of #clock-cells implies this is a clock provider. This
> binding should define the format of the clock cells that this property
> implies. I assume it's just the ID of the output clocks, but what values
> correspond to which output clocks? That mapping table needs to be listed
> here.
> 
> The presence of clock-names implies this is a clock consumer. However,
> there is no clocks property in the example. Is clks missing from the
> example, or should this property be clock-output-names instead? If this
> node is a clock consumer, the list of which clocks it requires should be
> documented.
> 
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-palmas.txt b/Documentation/devicetree/bindings/gpio/gpio-palmas.txt
> 
>> +- gpio-controller: mark the device as a GPIO controller
>> +- gpio-cells = <1>:  GPIO lines are provided.
> 
> That's #gpio-cells not gpio-cells.
> 
> I assume that cell simply contains the GPIO ID/number. That should be
> documented for clarity.
> 
> Surely gpio-cells should be 2 not 1, so there is space for flags. At
> least an "inverted" or "active-low" flag should be included; GPIO
> consumers would typically implement that flag in SW, so there' no
> requirement that the flag only be supported if the HW supports the feature.
> 
>> +- interrupt-controller : palmas has its own internal IRQs
>> +- #interrupt-cells : should be set to 2 for IRQ number and flags
>> +  The first cell is the IRQ number.
>> +  The second cell is the flags, encoded as the trigger masks from
>> +  Documentation/devicetree/bindings/interrupts.txt
> 
> You need "/interrupt-controller" before the filename there.
> 
>> +- interrupt-parent : The parent interrupt controller.
> 
> If this IP block has interrupt outputs, it should require an
> "interrupts" property too. This is the same concept that drives the need
> for a reg property. This same comment likely applies everywhere, so I
> won't repeat it.
> 
>> diff --git a/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt b/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt
> 
>> +- interrupts: the interrupt outputs of the controller.
> 
> How many entries are there, what do they mean, and in what order must
> they appear? (Note that the binding of a node must define the order of
> the interrupts property, since historically it's been accessed by index,
> not by name, and all bindings must allow that access method to be used).
> 
>> +- interrupt-names : Should be the name of irq resource. Each interrupt
>> +  binds its interrupt-name.
> 
> The binding needs to define standard names for the entries in this
> property, or define that interrupts are only retrieved by index, in
> which case interrupt-names shouldn't be present. This same comment
> likely applies everywhere, so I won't repeat it.
> 
>> diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt
> 
>> +Required properties:
> ...
> 
> I believe the Palmas devices have many separate I2C addresses, even
> buses, which are I think are at least partially independently SW
> configurable. In that case, the reg property for this node should
> probably enumerate all the I2C addresses that this chip responds to, so
> that they can each be passed down to the child nodes.


Stephen,

The palmas devices do have multiple I2C slave IDs. From OMAP5 as the master
all the palmas slave devices are connected via I2C1 bus.

I did not understand the SW configurable part. It is more board
dependent. Correct me if i understood it wrongly.

Graeme,

Can i send a separate Documentation patch for palmas-core and palmas-regulators
outside this series since the drivers are already part of mainline kernel.
This will ensure at least the bare minimal DT support for palmas family
will be available in the mainline kernel?

> 

>> diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> 
>> +Optional nodes:
>> +- regulators : should contain the constrains and init information for the
>> +	       regulators. It should contain a subnode per regulator from the
>> +	       list.
>> +	       For ti,palmas-pmic - smps12, smps123, smps3 depending on OTP,
>> +	       smps45, smps457, smps7 depending on varient, smps6, smps[8-10],
>> +	       ldo[1-9], ldoln, ldousb
>> +	       For ti,palmas-charger-pmic - smps12, smps123, smps3 depending on OTP,
>> +	       smps[6-9], boost, ldo[1-14], ldoln, ldousb
> 
> The list of legal compatible values for this node above doesn't include
> both ti,palmas-pmic and ti,palmas-charger-pmic. Should it? This node
> should describe this PMIC block in a completely standalone fashion,
> without the need to go look at the top-level node to see if it's a
> "charger" variant or not.
> 
>> +	       optional chip specific regulator fields :-
>> +	       ti,warm-reset - maintain voltage during warm reset
>> +	       ti,roof-floor - control voltage selection by pin
> 
> I assume those are Boolean not integer. It's worth mentioning the type
> of each property.




Reagrds,
Keerthy

Re: [PATCH v10 01/12] mfd: DT bindings for the palmas family MFD

[Stephen Warren]

On 05/30/2013 05:33 AM, keerthy wrote:
> 
> On 03/25/2013 11:29 PM, Stephen Warren wrote:
> 
>> On 03/22/2013 08:55 AM, Ian Lartey wrote:
>>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>>
>>> Add the various binding files for the palmas family of chips. There is a
>>> top level MFD binding then a seperate binding for IP blocks on chips.
>>
>>> diff --git a/Documentation/devicetree/bindings/clock/palmas-clk.txt b/Documentation/devicetree/bindings/clock/palmas-clk.txt
>>
>> Where is the reg property; if you're instantiating the clk device as a
>> standalone DT node and driver, it should be possible to retrieve the
>> address of this IP block instance from DT, not using driver-internal APIs.
>>
>> This same comment likely applies everywhere, so I won't repeat it.
>>
>>> +Example:
>>> +
>>> +clk {
>>> +    compatible = "ti,twl6035-clk", "ti,palmas-clk";
>>> +    ti,clk32kg-mode-sleep = <0>;
>>> +    ti,clk32kgaudio-mode-sleep = <0>;
>>
>>> +    #clock-cells = <1>;
>>> +    clock-frequency = <32000000>;
>>> +    clock-names = "clk32kg", "clk32kgaudio";
>>
>> The binding description itself should describe what clocks this node
>> provides and consumes.
>>
>> What is clock-frequency; which clock does it affect?
>>
>> The presence of #clock-cells implies this is a clock provider. This
>> binding should define the format of the clock cells that this property
>> implies. I assume it's just the ID of the output clocks, but what values
>> correspond to which output clocks? That mapping table needs to be listed
>> here.
>>
>> The presence of clock-names implies this is a clock consumer. However,
>> there is no clocks property in the example. Is clks missing from the
>> example, or should this property be clock-output-names instead? If this
>> node is a clock consumer, the list of which clocks it requires should be
>> documented.
>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-palmas.txt b/Documentation/devicetree/bindings/gpio/gpio-palmas.txt
>>
>>> +- gpio-controller: mark the device as a GPIO controller
>>> +- gpio-cells = <1>:  GPIO lines are provided.
>>
>> That's #gpio-cells not gpio-cells.
>>
>> I assume that cell simply contains the GPIO ID/number. That should be
>> documented for clarity.
>>
>> Surely gpio-cells should be 2 not 1, so there is space for flags. At
>> least an "inverted" or "active-low" flag should be included; GPIO
>> consumers would typically implement that flag in SW, so there' no
>> requirement that the flag only be supported if the HW supports the feature.
>>
>>> +- interrupt-controller : palmas has its own internal IRQs
>>> +- #interrupt-cells : should be set to 2 for IRQ number and flags
>>> +  The first cell is the IRQ number.
>>> +  The second cell is the flags, encoded as the trigger masks from
>>> +  Documentation/devicetree/bindings/interrupts.txt
>>
>> You need "/interrupt-controller" before the filename there.
>>
>>> +- interrupt-parent : The parent interrupt controller.
>>
>> If this IP block has interrupt outputs, it should require an
>> "interrupts" property too. This is the same concept that drives the need
>> for a reg property. This same comment likely applies everywhere, so I
>> won't repeat it.
>>
>>> diff --git a/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt b/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt
>>
>>> +- interrupts: the interrupt outputs of the controller.
>>
>> How many entries are there, what do they mean, and in what order must
>> they appear? (Note that the binding of a node must define the order of
>> the interrupts property, since historically it's been accessed by index,
>> not by name, and all bindings must allow that access method to be used).
>>
>>> +- interrupt-names : Should be the name of irq resource. Each interrupt
>>> +  binds its interrupt-name.
>>
>> The binding needs to define standard names for the entries in this
>> property, or define that interrupts are only retrieved by index, in
>> which case interrupt-names shouldn't be present. This same comment
>> likely applies everywhere, so I won't repeat it.
>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt
>>
>>> +Required properties:
>> ...
>>
>> I believe the Palmas devices have many separate I2C addresses, even
>> buses, which are I think are at least partially independently SW
>> configurable. In that case, the reg property for this node should
>> probably enumerate all the I2C addresses that this chip responds to, so
>> that they can each be passed down to the child nodes.
> 
> 
> Stephen,
> 
> The palmas devices do have multiple I2C slave IDs. From OMAP5 as the master
> all the palmas slave devices are connected via I2C1 bus.
> 
> I did not understand the SW configurable part. It is more board
> dependent. Correct me if i understood it wrongly.

IIRC (and I may not sine it's been a while since I looked at this),
there are SW registers that can modify the I2C address that the chip
will respond to, so you could access the main I2C address, then program
which other I2C addresses get used.

Or, perhaps I was just thinking of the fact that there are strapping
pins on the chip that affect both the main I2C address, and some/all of
the other I2C addresses, so the driver needs to be told each and every
I2C address, not just one single overall I2C address.

RE: [PATCH v10 01/12] mfd: DT bindings for the palmas family MFD

[gg]

On 2013-06-03 07:55, J, KEERTHY wrote:
> Hi Stephen,
> 
>> -----Original Message-----
>> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
>> Sent: Friday, May 31, 2013 4:34 AM
>> To: J, KEERTHY
>> Cc: gg@slimlogic.co.uk; Ian Lartey; linux-kernel@vger.kernel.org;
>> linux-doc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; 
>> linux-
>> leds@vger.kernel.org; linux-watchdog@vger.kernel.org; devicetree-
>> discuss@lists.ozlabs.org; grant.likely@secretlab.ca;
>> broonie@opensource.wolfsonmicro.com; rob.herring@calxeda.com;
>> rob@landley.net; mturquette@linaro.org; linus.walleij@linaro.org;
>> cooloney@gmail.com; sfr@canb.auug.org.au; rpurdie@rpsys.net;
>> akpm@linux-foundation.org; sameo@linux.intel.com; wim@iguana.be;
>> lgirdwood@gmail.com; ldewangan@nvidia.com; Kristo, Tero
>> Subject: Re: [PATCH v10 01/12] mfd: DT bindings for the palmas family
>> MFD
>> 
>> On 05/30/2013 05:33 AM, keerthy wrote:
>> >
>> > On 03/25/2013 11:29 PM, Stephen Warren wrote:
>> >
>> >> On 03/22/2013 08:55 AM, Ian Lartey wrote:
>> >>> From: Graeme Gregory <gg@slimlogic.co.uk>
>> >>>
>> >>> Add the various binding files for the palmas family of chips. There
>> >>> is a top level MFD binding then a seperate binding for IP blocks on
>> chips.
>> >>
>> >>> diff --git a/Documentation/devicetree/bindings/clock/palmas-clk.txt
>> >>> b/Documentation/devicetree/bindings/clock/palmas-clk.txt
>> >>
>> >> Where is the reg property; if you're instantiating the clk device as
>> >> a standalone DT node and driver, it should be possible to retrieve
>> >> the address of this IP block instance from DT, not using driver-
>> internal APIs.
>> >>
>> >> This same comment likely applies everywhere, so I won't repeat it.
>> >>
>> >>> +Example:
>> >>> +
>> >>> +clk {
>> >>> +    compatible = "ti,twl6035-clk", "ti,palmas-clk";
>> >>> +    ti,clk32kg-mode-sleep = <0>;
>> >>> +    ti,clk32kgaudio-mode-sleep = <0>;
>> >>
>> >>> +    #clock-cells = <1>;
>> >>> +    clock-frequency = <32000000>;
>> >>> +    clock-names = "clk32kg", "clk32kgaudio";
>> >>
>> >> The binding description itself should describe what clocks this node
>> >> provides and consumes.
>> >>
>> >> What is clock-frequency; which clock does it affect?
>> >>
>> >> The presence of #clock-cells implies this is a clock provider. This
>> >> binding should define the format of the clock cells that this
>> >> property implies. I assume it's just the ID of the output clocks,
>> but
>> >> what values correspond to which output clocks? That mapping table
>> >> needs to be listed here.
>> >>
>> >> The presence of clock-names implies this is a clock consumer.
>> >> However, there is no clocks property in the example. Is clks missing
>> >> from the example, or should this property be clock-output-names
>> >> instead? If this node is a clock consumer, the list of which clocks
>> >> it requires should be documented.
>> >>
>> >>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-palmas.txt
>> >>> b/Documentation/devicetree/bindings/gpio/gpio-palmas.txt
>> >>
>> >>> +- gpio-controller: mark the device as a GPIO controller
>> >>> +- gpio-cells = <1>:  GPIO lines are provided.
>> >>
>> >> That's #gpio-cells not gpio-cells.
>> >>
>> >> I assume that cell simply contains the GPIO ID/number. That should
>> be
>> >> documented for clarity.
>> >>
>> >> Surely gpio-cells should be 2 not 1, so there is space for flags. At
>> >> least an "inverted" or "active-low" flag should be included; GPIO
>> >> consumers would typically implement that flag in SW, so there' no
>> >> requirement that the flag only be supported if the HW supports the
>> feature.
>> >>
>> >>> +- interrupt-controller : palmas has its own internal IRQs
>> >>> +- #interrupt-cells : should be set to 2 for IRQ number and flags
>> >>> +  The first cell is the IRQ number.
>> >>> +  The second cell is the flags, encoded as the trigger masks from
>> >>> +  Documentation/devicetree/bindings/interrupts.txt
>> >>
>> >> You need "/interrupt-controller" before the filename there.
>> >>
>> >>> +- interrupt-parent : The parent interrupt controller.
>> >>
>> >> If this IP block has interrupt outputs, it should require an
>> >> "interrupts" property too. This is the same concept that drives the
>> >> need for a reg property. This same comment likely applies
>> everywhere,
>> >> so I won't repeat it.
>> >>
>> >>> diff --git
>> >>> a/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt
>> >>> b/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt
>> >>
>> >>> +- interrupts: the interrupt outputs of the controller.
>> >>
>> >> How many entries are there, what do they mean, and in what order
>> must
>> >> they appear? (Note that the binding of a node must define the order
>> >> of the interrupts property, since historically it's been accessed by
>> >> index, not by name, and all bindings must allow that access method
>> to be used).
>> >>
>> >>> +- interrupt-names : Should be the name of irq resource. Each
>> >>> +interrupt
>> >>> +  binds its interrupt-name.
>> >>
>> >> The binding needs to define standard names for the entries in this
>> >> property, or define that interrupts are only retrieved by index, in
>> >> which case interrupt-names shouldn't be present. This same comment
>> >> likely applies everywhere, so I won't repeat it.
>> >>
>> >>> diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt
>> >>> b/Documentation/devicetree/bindings/mfd/palmas.txt
>> >>
>> >>> +Required properties:
>> >> ...
>> >>
>> >> I believe the Palmas devices have many separate I2C addresses, even
>> >> buses, which are I think are at least partially independently SW
>> >> configurable. In that case, the reg property for this node should
>> >> probably enumerate all the I2C addresses that this chip responds to,
>> >> so that they can each be passed down to the child nodes.
>> >
>> >
>> > Stephen,
>> >
>> > The palmas devices do have multiple I2C slave IDs. From OMAP5 as the
>> > master all the palmas slave devices are connected via I2C1 bus.
>> >
>> > I did not understand the SW configurable part. It is more board
>> > dependent. Correct me if i understood it wrongly.
>> 
>> IIRC (and I may not sine it's been a while since I looked at this),
>> there are SW registers that can modify the I2C address that the chip
>> will respond to, so you could access the main I2C address, then 
>> program
>> which other I2C addresses get used.
> 
> Ok. I guess you are referring to the I2C_SPI register of Palmas.
> This register is indeed SW configurable and I tried changing the
> Slave IDs on the fly and I could change them. AFAIK these are OTP
> And never changed through software on the fly.
> 
>> 
>> Or, perhaps I was just thinking of the fact that there are strapping
>> pins on the chip that affect both the main I2C address, and some/all 
>> of
>> the other I2C addresses, so the driver needs to be told each and every
>> I2C address, not just one single overall I2C address.
> 
> Looking at the register spec there seem to be 2 possible combinations:
> 0x48, 0x49, 0x4A or 0x58, 0x59, 0x5A. Again these are OTP. By default
> It is 0x48, 0x49, 0x4A. So passing 0x48 or 0x58 as the main I2C
> Address seems sufficient here.
> 
The I2C addresses are set in OTP, I do not think they should ever be 
changed
on the fly.

Graeme

Re: [PATCH v10 04/12] watchdog: add Palmas Watchdog support

[Wim Van Sebroeck]

Hi Ian,

> +static int palmas_wdt_set_timeout(struct watchdog_device *wdt, unsigned timeout)
> +{
> +	struct palmas_wdt *driver_data = watchdog_get_drvdata(wdt);
> +
> +	if (timeout < 1 || timeout > 128) {
> +		dev_warn(driver_data->dev,
> +			"Timeout can only be in the range [1-128] seconds");
> +		return -EINVAL;
> +	}
> +	driver_data->timer_margin = fls(timeout) - 1;
> +	return 0;
> +}

The core can test this also with the .min_timeout and .max_timeout fields of the watchdog_device.

> +
> +static const struct watchdog_info palmas_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +	.identity = "Palmas Watchdog",
> +	.firmware_version = 0,
> +};
> +
> +static const struct watchdog_ops palmas_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = palmas_wdt_enable,
> +	.stop = palmas_wdt_disable,
> +	.ping = palmas_wdt_enable,

Since .ping is the same as .start you don't need to add .ping .

> +	.set_timeout = palmas_wdt_set_timeout,
> +};
> +
> +static int palmas_wdt_probe(struct platform_device *pdev)
> +{
> +	struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
> +	struct palmas_wdt *driver_data;
> +	struct watchdog_device *palmas_wdt;
> +	int ret = 0;
> +
> +	driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data),
> +				   GFP_KERNEL);
> +	if (!driver_data) {
> +		dev_err(&pdev->dev, "Unable to alloacate watchdog device\n");
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	driver_data->palmas = palmas;
> +
> +	palmas_wdt = &driver_data->wdt;
> +
> +	palmas_wdt->info = &palmas_wdt_info;
> +	palmas_wdt->ops = &palmas_wdt_ops;
> +	watchdog_set_nowayout(palmas_wdt, nowayout);
> +	watchdog_set_drvdata(palmas_wdt, driver_data);


I think you want a watchdog_init_timeout(struct watchdog_device *wdd, unsigned int timeout_parm, struct device *dev); here.
And also make sure that the watchdog isn't running.

> +
> +	ret = watchdog_register_device(&driver_data->wdt);
> +	if (ret) {
> +		platform_set_drvdata(pdev, NULL);
> +		goto err;
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, driver_data);
> +
> +	return 0;
> +err:
> +	return ret;
> +}
> +
> +static int palmas_wdt_remove(struct platform_device *pdev)
> +{
> +	struct palmas_wdt *driver_data = dev_get_drvdata(&pdev->dev);
> +
> +	watchdog_unregister_device(&driver_data->wdt);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id of_palmas_match_tbl[] = {
> +	{ .compatible = "ti,palmas-wdt", },
> +	{ .compatible = "ti,twl6035-wdt", },
> +	{ .compatible = "ti,twl6036-wdt", },
> +	{ .compatible = "ti,twl6037-wdt", },
> +	{ .compatible = "ti,tps65913-wdt", },
> +	{ .compatible = "ti,tps65914-wdt", },
> +	{ .compatible = "ti,tps80036-wdt", },
> +	{ /* end */ }
> +};
> +
> +static struct platform_driver palmas_wdt_driver = {
> +	.probe = palmas_wdt_probe,
> +	.remove = palmas_wdt_remove,
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_palmas_match_tbl,
> +		.name = "palmas-wdt",
> +	},
> +};
> +
> +module_platform_driver(palmas_wdt_driver);
> +
> +MODULE_AUTHOR("Graeme Gregory <gg@slimlogic.co.uk>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:palmas-wdt");
> +MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
> -- 
> 1.7.0.4
> 

If you can add the devicetree bindings Documentation and the Kconfig patch together with the revised version of this patch into a single patch then we can complete the review.

Kind regards,
Wim.