Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver
From: Lee Jones
Date: Sun Sep 20 2015 - 00:16:21 EST
On Thu, 17 Sep 2015, Barry Song wrote:
> From: Guo Zeng <Guo.Zeng@xxxxxxx>
>
> CSR SiRFSoC Power Control Module includes power on or power
> off for sysctl power and gnss, it also includes onkey, RTC
> domain clock controllers and interrupt controller for power
> events.
There are lots of Three (and four) Letter Abbreviations (TLAs) here,
which mean nothing to the average reader. Please break them out in
the commit log as I have done i.e. "Long Abbreviated Word (LAW)", so
us normies can see what they mean.
> Signed-off-by: Guo Zeng <Guo.Zeng@xxxxxxx>
> Signed-off-by: Barry Song <Baohua.Song@xxxxxxx>
> ---
> .../devicetree/bindings/mfd/sirf-pwrc.txt | 37 ++++
This should be in a separate patch.
> drivers/mfd/Kconfig | 12 ++
> drivers/mfd/Makefile | 2 +
> drivers/mfd/sirfsoc_pwrc.c | 238 +++++++++++++++++++++
> include/linux/mfd/sirfsoc_pwrc.h | 97 +++++++++
> 5 files changed, 386 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
> create mode 100644 drivers/mfd/sirfsoc_pwrc.c
> create mode 100644 include/linux/mfd/sirfsoc_pwrc.h
>
> diff --git a/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt b/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
> new file mode 100644
> index 0000000..b919cdd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
> @@ -0,0 +1,37 @@
> +SiRFSoC Power Controller (PWRC) module
> +
> +Power Controller is used to control the whole SoC power process.
> +The power controller controls the process of Power-On sequence,
s/power controller/Power Controller/
> +Power-Off sequence, different power modes switching and power
> +status configuration. the pwrc is access by io bridge, so the
s/the pwrc/The PWRC/
s/access/accessed/
s/io/IO/
> +node's parent should be io bridge.
s/io/IO/
Not quite sure what an IO bridge is though.
> +Required properties:
> +- compatible: "sirf,prima2-pwrc", or "sirf,atlas7-pwrc"
> +- reg: Address range of pwrc register set
Address start and size.
> +- sysctl:mfd cell device of pwrc
> +- rtcm-clk:mfd cell device of pwrc
> +- onkey:mfd cell device of pwrc
MFD is a Linuxisum. Please find another way to document them.
I always find documentation easier to read then it's tabbed out
nicely. Like:
Required properties:
- compatible : "sirf,prima2-pwrc", or "sirf,atlas7-pwrc"
- reg : Address range of pwrc register set
- sysctl : mfd cell device of pwrc
- rtcm-clk : mfd cell device of pwrc
- onkey : mfd cell device of pwrc
> +Example:
> +
> +pwrc@3000 {
> + compatible = "sirf,atlas7-pwrc";
> + reg = <0x3000 0x100>;
This doesn't look like a real address. It looks like an offset.
Please provide a full example, complete with the parent node (which I
assume has ranges set-up).
> + sysctl {
> + compatible = "sirf,sirf-sysctl";
> + };
> +
> + rtcm-clk {
> + compatible = "sirf,atlas7-rtcmclk";
> + };
> +
> + onkey {
> + compatible = "sirf,prima2-onkey";
> + };
Why do these require their own cells?
Do they have their own properties? If so, where are they documented?
> +};
> +
> +pwrc@3000 {
> + compatible = "sirf,prima2-pwrc";
> + reg = <0x3000 0x100>;
> +};
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 99d6367..5b67bee 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1515,5 +1515,17 @@ config MFD_VEXPRESS_SYSREG
> System Registers are the platform configuration block
> on the ARM Ltd. Versatile Express board.
>
> +config MFD_SIRFSOC_PWRC
> + bool "CSR SiRFSoC on-chip Power Control Module"
> + depends on ARCH_SIRF
> + default y
> + select MFD_CORE
> + select REGMAP_IRQ
> + help
> + CSR SiRFSoC Power Control Module includes power on or power
> + off for sysctl power and gnss, it also includes onkey, RTC
> + domain clock controllers and interrupt controller for power
> + events.
Break out the TLAs.
Your tabbing is all over the place here. Please match existing
entries.
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a59e3fc..255fb80 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -192,3 +192,5 @@ obj-$(CONFIG_MFD_SKY81452) += sky81452.o
> intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o
> obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
> obj-$(CONFIG_MFD_MT6397) += mt6397-core.o
> +
> +obj-$(CONFIG_MFD_SIRFSOC_PWRC) += sirfsoc_pwrc.o
> diff --git a/drivers/mfd/sirfsoc_pwrc.c b/drivers/mfd/sirfsoc_pwrc.c
> new file mode 100644
> index 0000000..b43f8b4
> --- /dev/null
> +++ b/drivers/mfd/sirfsoc_pwrc.c
> @@ -0,0 +1,238 @@
> +/*
> + * power management mfd for CSR SiRFSoC chips
"Power Management MFD for CSR SiRFSoC chips"
> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
This is out of date.
> + * Licensed under GPLv2 or later.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +#include <linux/regmap.h>
> +#include <linux/module.h>
This isn't a module.
> +#include <linux/rtc/sirfsoc_rtciobrg.h>
What's this for?
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/sirfsoc_pwrc.h>
Header files should be in alphabetical order.
> +struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc = {
> + .pwrc_pdn_ctrl_set = 0x0,
> + .pwrc_pdn_ctrl_clr = 0x4,
> + .pwrc_pon_status = 0x8,
> + .pwrc_trigger_en_set = 0xc,
> + .pwrc_trigger_en_clr = 0x10,
> + .pwrc_int_mask_set = 0x14,
> + .pwrc_int_mask_clr = 0x18,
> + .pwrc_int_status = 0x1c,
> + .pwrc_pin_status = 0x20,
> + .pwrc_rtc_pll_ctrl = 0x28,
> + .pwrc_gpio3_debug = 0x34,
> + .pwrc_rtc_noc_pwrctl_set = 0x38,
> + .pwrc_rtc_noc_pwrctl_clr = 0x3c,
> + .pwrc_rtc_can_ctrl = 0x48,
> + .pwrc_rtc_can_status = 0x4c,
> + .pwrc_fsm_m3_ctrl = 0x50,
> + .pwrc_fsm_state = 0x54,
> + .pwrc_rtcldo_reg = 0x58,
> + .pwrc_gnss_ctrl = 0x5c,
> + .pwrc_gnss_status = 0x60,
> + .pwrc_xtal_reg = 0x64,
> + .pwrc_xtal_ldo_mux_sel = 0x68,
> + .pwrc_rtc_sw_rstc_set = 0x6c,
> + .pwrc_rtc_sw_rstc_clr = 0x70,
> + .pwrc_power_sw_ctrl_set = 0x74,
> + .pwrc_power_sw_ctrl_clr = 0x78,
> + .pwrc_rtc_dcog = 0x7c,
> + .pwrc_m3_memories = 0x80,
> + .pwrc_can0_memory = 0x84,
> + .pwrc_rtc_gnss_memory = 0x88,
> + .pwrc_m3_clk_en = 0x8c,
> + .pwrc_can0_clk_en = 0x90,
> + .pwrc_spi0_clk_en = 0x94,
> + .pwrc_rtc_sec_clk_en = 0x98,
> + .pwrc_rtc_noc_clk_en = 0x9c,
> +};
> +
> +struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc = {
> + .pwrc_pdn_ctrl_set = 0x0,
> + .pwrc_pon_status = 0x4,
> + .pwrc_trigger_en_set = 0x8,
> + .pwrc_int_status = 0xc,
> + .pwrc_int_mask_set = 0x10,
> + .pwrc_pin_status = 0x14,
> + .pwrc_scratch_pad1 = 0x18,
> + .pwrc_scratch_pad2 = 0x1c,
> + .pwrc_scratch_pad3 = 0x20,
> + .pwrc_scratch_pad4 = 0x24,
> + .pwrc_scratch_pad5 = 0x28,
> + .pwrc_scratch_pad6 = 0x2c,
> + .pwrc_scratch_pad7 = 0x30,
> + .pwrc_scratch_pad8 = 0x34,
> + .pwrc_scratch_pad9 = 0x38,
> + .pwrc_scratch_pad10 = 0x3c,
> + .pwrc_scratch_pad11 = 0x40,
> + .pwrc_scratch_pad12 = 0x44,
> + .pwrc_gpio3_clk = 0x54,
> + .pwrc_gpio_ds = 0x78,
> +};
This is not the way we usually define register addresses.
Please #define them properly.
> +static const struct regmap_irq pwrc_irqs[] = {
> + /* INT0 */
> + [PWRC_IRQ_ONKEY] = {
> + .mask = BIT(PWRC_IRQ_ONKEY),
Better to do the BIT() shifting in the header file.
> + },
> + [PWRC_IRQ_EXT_ONKEY] = {
> + .mask = BIT(PWRC_IRQ_EXT_ONKEY),
> + },
> +};
> +
> +static struct regmap_irq_chip pwrc_irq_chip = {
> + .name = "pwrc_irq",
> + .irqs = pwrc_irqs,
> + .num_irqs = ARRAY_SIZE(pwrc_irqs),
> + .num_regs = 1,
> + .ack_invert = 1,
> + .init_ack_masked = true,
> +};
> +
> +static const struct of_device_id pwrc_ids[] = {
> + { .compatible = "sirf,prima2-pwrc", .data = &sirfsoc_prima2_pwrc},
> + { .compatible = "sirf,atlas7-pwrc", .data = &sirfsoc_a7da_pwrc},
> + {}
> +};
Please put these _just_ before you use them, so just above .probe() in
this case.
> +static const struct mfd_cell pwrc_devs[] = {
> + {
> + .name = "rtcmclk",
> + .of_compatible = "sirf,atlas7-rtcmclk",
> + }, {
> + .name = "sirf-sysctl",
> + .of_compatible = "sirf,sirf-sysctl",
> + }, {
> + .name = "gps-power",
> + .of_compatible = "sirf,gps-power",
> + }, {
> + .name = "onkey",
> + .of_compatible = "sirf,prima2-onkey",
> + },
> +};
> +
> +static const struct regmap_config pwrc_regmap_config = {
> + .fast_io = true,
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> +};
> +
> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + const struct of_device_id *match;
> + struct sirfsoc_pwrc_info *pwrcinfo;
> + struct regmap_irq_chip *regmap_irq_chip;
> + struct sirfsoc_pwrc_register *pwrc_reg;
> + struct regmap *map;
> + int ret;
> + u32 base;
> +
> + if (of_property_read_u32(np, "reg", &base))
> + panic("unable to find base address of pwrc node in dtb\n");
It looks like this driver should depend on OF.
Why are you obtaining the base address manually? Use:
res = platform_get_resource();
devm_ioremap_resource(res);
... instead.
> + pwrcinfo = devm_kzalloc(&pdev->dev,
> + sizeof(struct sirfsoc_pwrc_info), GFP_KERNEL);
sizeof(pwrcinfo)
> + if (!pwrcinfo)
> + return -ENOMEM;
> + pwrcinfo->base = base;
> +
> + /*
> + * pwrc behind rtciobrg offset is diff between prima2 and atlas7
> + * here match to each ids data for it.
> + */
Use uppercase characters for abbreviations.
Besides, I have no idea what you're trying to say.
> + match = of_match_node(pwrc_ids, np);
> + pwrcinfo->pwrc_reg = (struct sirfsoc_pwrc_register *)match->data;
It looks like you're trying to use the same variables two different
device's register sets. That's asking for trouble, please don't do
that.
> + if (of_device_is_compatible(np, "sirf,atlas7-pwrc"))
> + pwrcinfo->ver = PWRC_ATLAS7_VER;
> + else if (of_device_is_compatible(np, "sirf,prima2-pwrc"))
> + pwrcinfo->ver = PWRC_PRIMA2_VER;
> + else
> + return -EINVAL;
These aren't versions, are they? It's different hardware.
Can't you probe for this at runtime?
> + of_node_put(np);
What are you putting here?
> + map = (struct regmap *)devm_regmap_init_iobg(&pdev->dev,
> + &pwrc_regmap_config);
Why are you casting?
> + if (IS_ERR(map)) {
> + ret = PTR_ERR(map);
> + dev_err(&pdev->dev, "Failed to allocate register map: %d\n",
> + ret);
> + goto err;
Please remove the 'err' label and just return from here.
> + }
> +
> + pwrcinfo->regmap = map;
> + pwrcinfo->dev = &pdev->dev;
> + platform_set_drvdata(pdev, pwrcinfo);
> +
> + ret = mfd_add_devices(pwrcinfo->dev, 0, pwrc_devs,
> + ARRAY_SIZE(pwrc_devs), NULL, 0, NULL);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to add all pwrc subdev\n");
Capitalise all abbreviations.
Don't abbreviate things like "sub devices".
> + goto err;
Just return.
> + }
> +
> + ret = of_irq_get(pdev->dev.of_node, 0);
You already put this in to a succinct variable 'np'. Please use it.
> + if (ret <= 0) {
> + dev_info(&pdev->dev,
> + "Unable to find IRQ for pwrc. ret=%d\n", ret);
Just print ret. No need for the ugly "ret=".
> + goto err_irq;
> + }
> +
> + pwrcinfo->irq = ret;
Better change this to 'irq' instead of using 'ret'.
> + regmap_irq_chip = &pwrc_irq_chip;
> + pwrcinfo->regmap_irq_chip = regmap_irq_chip;
> +
> + pwrc_reg = pwrcinfo->pwrc_reg;
> + regmap_irq_chip->mask_base = pwrcinfo->base +
> + pwrc_reg->pwrc_int_mask_set;
> + regmap_irq_chip->unmask_base = pwrcinfo->base +
> + pwrc_reg->pwrc_int_mask_clr;
> + regmap_irq_chip->status_base = pwrcinfo->base +
> + pwrc_reg->pwrc_int_status;
> + regmap_irq_chip->ack_base = pwrcinfo->base +
> + pwrc_reg->pwrc_int_status;
This is ugly.
Better to create 2 regmap_irq_chip structures, one for each device.
> + /* enable onkey trigger interrupt controller */
Capital letters to start a sentence.
> + ret = regmap_update_bits(map,
> + pwrcinfo->base +
> + pwrc_reg->pwrc_trigger_en_set,
> + BIT(PWRC_IRQ_ONKEY) | BIT(PWRC_IRQ_EXT_ONKEY),
> + BIT(PWRC_IRQ_ONKEY) | BIT(PWRC_IRQ_EXT_ONKEY));
> + if (ret < 0)
> + goto err_irq;
> +
> + /* add irq controller for pwrc */
Capital letters to start a sentence and for abbreviations.
> + ret = regmap_add_irq_chip(map, pwrcinfo->irq, IRQF_ONESHOT,
> + -1, pwrcinfo->regmap_irq_chip,
> + &pwrcinfo->irq_data);
> +
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to add regmap irq controller for pwrc\n");
> + goto err;
> + }
> +
> + return 0;
> +err_irq:
> + mfd_remove_devices(pwrcinfo->dev);
> +err:
Remove this label, it's not helpful.
> + return ret;
> +}
> +
> +static struct platform_driver sirfsoc_pwrc_driver = {
> + .probe = sirfsoc_pwrc_probe,
.remove?
> + .driver = {
> + .name = "sirfsoc_pwrc",
> + .of_match_table = pwrc_ids,
of_match_ptr()
> + },
> +};
> +module_platform_driver(sirfsoc_pwrc_driver);
This isn't a module.
> diff --git a/include/linux/mfd/sirfsoc_pwrc.h b/include/linux/mfd/sirfsoc_pwrc.h
> new file mode 100644
> index 0000000..ee2b3d5
> --- /dev/null
> +++ b/include/linux/mfd/sirfsoc_pwrc.h
> @@ -0,0 +1,97 @@
> +/*
> + * CSR SiRFSoC power control module MFD interface
> + *
> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
Out of date.
> + * Licensed under GPLv2 or later.
> + */
'\n' here.
> +#ifndef _SIRFSOC_PWRC_H_
> +#define _SIRFSOC_PWRC_H_
'\n' here.
> +#include <linux/interrupt.h>
What's this for?
> +#include <linux/regmap.h>
> +
> +#define PWRC_PDN_CTRL_OFFSET 0
> +#define AUDIO_POWER_EN_BIT 14
> +
> +struct sirfsoc_pwrc_register {
> + /* hardware pwrc specific */
> + u32 pwrc_pdn_ctrl_set;
> + u32 pwrc_pdn_ctrl_clr;
> + u32 pwrc_pon_status;
> + u32 pwrc_trigger_en_set;
> + u32 pwrc_trigger_en_clr;
> + u32 pwrc_int_mask_set;
> + u32 pwrc_int_mask_clr;
> + u32 pwrc_int_status;
> + u32 pwrc_pin_status;
> + u32 pwrc_rtc_pll_ctrl;
> + u32 pwrc_gpio3_debug;
> + u32 pwrc_rtc_noc_pwrctl_set;
> + u32 pwrc_rtc_noc_pwrctl_clr;
> + u32 pwrc_rtc_can_ctrl;
> + u32 pwrc_rtc_can_status;
> + u32 pwrc_fsm_m3_ctrl;
> + u32 pwrc_fsm_state;
> + u32 pwrc_rtcldo_reg;
> + u32 pwrc_gnss_ctrl;
> + u32 pwrc_gnss_status;
> + u32 pwrc_xtal_reg;
> + u32 pwrc_xtal_ldo_mux_sel;
> + u32 pwrc_rtc_sw_rstc_set;
> + u32 pwrc_rtc_sw_rstc_clr;
> + u32 pwrc_power_sw_ctrl_set;
> + u32 pwrc_power_sw_ctrl_clr;
> + u32 pwrc_rtc_dcog;
> + u32 pwrc_m3_memories;
> + u32 pwrc_can0_memory;
> + u32 pwrc_rtc_gnss_memory;
> + u32 pwrc_m3_clk_en;
> + u32 pwrc_can0_clk_en;
> + u32 pwrc_spi0_clk_en;
> + u32 pwrc_rtc_sec_clk_en;
> + u32 pwrc_rtc_noc_clk_en;
> +
> + /*only for prima2*/
> + u32 pwrc_scratch_pad1;
> + u32 pwrc_scratch_pad2;
> + u32 pwrc_scratch_pad3;
> + u32 pwrc_scratch_pad4;
> + u32 pwrc_scratch_pad5;
> + u32 pwrc_scratch_pad6;
> + u32 pwrc_scratch_pad7;
> + u32 pwrc_scratch_pad8;
> + u32 pwrc_scratch_pad9;
> + u32 pwrc_scratch_pad10;
> + u32 pwrc_scratch_pad11;
> + u32 pwrc_scratch_pad12;
> + u32 pwrc_gpio3_clk;
> + u32 pwrc_gpio_ds;
> +
> +};
Please #define these instead.
> +enum pwrc_version {
> + PWRC_MARCO_VER,
> + PWRC_PRIMA2_VER,
> + PWRC_ATLAS7_VER,
> +};
Kerneldoc header?
> +struct sirfsoc_pwrc_info {
> + struct device *dev;
> + struct regmap *regmap;
> + struct sirfsoc_pwrc_register *pwrc_reg;
> + struct regmap_irq_chip *regmap_irq_chip;
> + struct regmap_irq_chip_data *irq_data;
Where are these reused?
> + u32 ver;
What is this used for?
> + u32 base;
Is this a u32 or a memory address?
> + int irq;
> +};
> +
> +enum {
> + PWRC_IRQ_ONKEY = 0,
> + PWRC_IRQ_EXT_ONKEY,
> + PWRC_MAX_IRQ,
> +};
> +
> +extern struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc;
> +extern struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc;
> +#endif
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/