Messages in this thread | | | Date | Sun, 20 Sep 2015 05:15:44 +0100 | From | Lee Jones <> | Subject | Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver |
| |
On Thu, 17 Sep 2015, Barry Song wrote:
> From: Guo Zeng <Guo.Zeng@csr.com> > > 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@csr.com> > Signed-off-by: Barry Song <Baohua.Song@csr.com> > --- > .../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
| |