Re: [PATCH 06/13] mfd: twl-core: Add initial DT support for twl4030/twl6030

Rajendra Nayak
2011-09-27T05:42:19Z

On Monday 26 September 2011 10:20 PM, Benoit Cousson wrote:
> Add initial device-tree support for twl familly chips.

s/familly/family

> The current version is missing the regulator entries due
> to the lack of DT regulator bindings for the moment.
> Only the simple sub-modules that do not depend on
> platform_data information can be initialized properly.
>
> Add documentation for the Texas Instruments TWL Integrated Chip.
>
> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
> Cc: Balaji T K<balajitk@ti.com>
> Cc: Graeme Gregory<gg@slimlogic.co.uk>
> Cc: Samuel Ortiz<sameo@linux.intel.com>
> ---
> .../devicetree/bindings/mfd/twl-familly.txt | 47 +++++++++++++++++
> drivers/mfd/twl-core.c | 53 ++++++++++++++++++--
> 2 files changed, 96 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/mfd/twl-familly.txt

s/familly.txt/family.txt

>
> diff --git a/Documentation/devicetree/bindings/mfd/twl-familly.txt b/Documentation/devicetree/bindings/mfd/twl-familly.txt
> new file mode 100644
> index 0000000..ff4cacd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/twl-familly.txt
> @@ -0,0 +1,47 @@
> +Texas Instruments TWL family
> +
> +The TWLs are Integrated Power Management Chips.
> +Some version might contain much more analog function like
> +USB transceiver or Audio amplifier.
> +These chips are connected to an i2c bus.
> +
> +
> +Required properties:
> +- compatible : Must be "ti,twl4030";
> + For Integrated power-management/audio CODEC device used in OMAP3
> + based boards
> +- compatible : Must be "ti,twl6030";
> + For Integrated power-management used in OMAP4 based boards
> +- interrupts : This i2c device has an IRQ line connected to the main SoC
> +- interrupt-controller : Since the twl support several interrupts internally,
> + it is considered as an interrupt controller cascaded to the SoC one.
> +- #interrupt-cells =<1>;
> +- interrupt-parent : The parent interrupt controller.
> +
> +Optional node:
> +- Child nodes contain in the twl. The twl family is made of severals variants
> + that support a different number of features.
> + The children nodes will thus depend of the capabilty of the variant.
> +
> +
> +Example:
> +/*
> + * Integrated Power Management Chip
> + * http://www.ti.com/lit/ds/symlink/twl6030.pdf
> + */
> +twl@48 {
> + compatible = "ti,twl6030";
> + reg =<0x48>;

What does the 'reg' property signify here for twl?

> + interrupts =<39>; /* IRQ_SYS_1N cascaded to gic */
> + interrupt-controller;
> + #interrupt-cells =<1>;
> + interrupt-parent =<&gic>;
> + #address-cells =<1>;
> + #size-cells =<0>;
> +
> + twl_rtc {
> + compatible = "ti,twl_rtc";
> + interrupts =<11>;
> + reg =<0>;

Does the 'reg' property need to be faked for
every twl child node, even if it does not have
any?

> + };
> +};
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index 01ecfee..3ef0b43 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -33,6 +33,10 @@
> #include<linux/platform_device.h>
> #include<linux/clk.h>
> #include<linux/err.h>
> +#include<linux/slab.h>
> +#include<linux/of_irq.h>
> +#include<linux/of_platform.h>
> +#include<linux/irqdomain.h>
>
> #include<linux/regulator/machine.h>
>
> @@ -1182,22 +1186,53 @@ twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
> int status;
> unsigned i;
> struct twl4030_platform_data *pdata = client->dev.platform_data;
> + struct device_node *node = client->dev.of_node;
> u8 temp;
> int ret = 0;
>
> + if (node&& !pdata) {
> + /*
> + * XXX: Temporary fake pdata until the information
> + * is correctly retrieved by every TWL modules from DT.
> + */
> + pdata = kzalloc(sizeof(struct twl4030_platform_data),
> + GFP_KERNEL);

devm_kzalloc instead?

> + if (!pdata) {
> + status = -ENOMEM;
> + goto exit;
> + }
> +
> + /*
> + * XXX: For the moment the IRQs for TWL seems to be encoded in
> + * the global OMAP space. That should be cleaned to allow
> + * dynamically adding a new IRQ controller.
> + */
> + if ((id->driver_data)& TWL6030_CLASS) {
> + pdata->irq_base = TWL6030_IRQ_BASE;
> + pdata->irq_end = pdata->irq_base + TWL6030_BASE_NR_IRQS;
> + } else {
> + pdata->irq_base = TWL4030_IRQ_BASE;
> + pdata->irq_end = pdata->irq_base + TWL4030_BASE_NR_IRQS;
> + }
> + irq_domain_add_simple(node, pdata->irq_base);
> + }
> +
> if (!pdata) {
> dev_dbg(&client->dev, "no platform data?\n");
> - return -EINVAL;
> + status = -EINVAL;
> + goto fail_free;
> }
>
> if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C) == 0) {
> dev_dbg(&client->dev, "can't talk I2C?\n");
> - return -EIO;
> + status = -EIO;
> + goto fail_free;
> }
>
> if (inuse) {
> dev_dbg(&client->dev, "driver is already in use\n");
> - return -EBUSY;
> + status = -EBUSY;
> + goto fail_free;
> }
>
> for (i = 0; i< TWL_NUM_SLAVES; i++) {
> @@ -1269,10 +1304,20 @@ twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
> twl_i2c_write_u8(TWL4030_MODULE_INTBR, temp, REG_GPPUPDCTR1);
> }
>
> - status = add_children(pdata, id->driver_data);
> +#ifdef CONFIG_OF_DEVICE

is the #ifdef really needed?

> + if (node)
> + status = of_platform_populate(node, NULL, NULL,&client->dev);
> + else
> +#endif
> + status = add_children(pdata, id->driver_data);
> +
> fail:
> if (status< 0)
> twl_remove(client);
> +fail_free:
> + if (node)
> + kfree(pdata);
> +exit:
> return status;
> }
>