| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGsJ_4wA417aQa8GY2C92MQOgPWdeh34rg2J_J7OXhEVqHqPsw@mail.gmail.com>
Date: Mon, 21 Sep 2015 10:38:09 +0800
From: Barry Song <21cnbao@...il.com>
To: Lee Jones <lee.jones@...aro.org>
Cc: Mark Brown <broonie@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
sameo <sameo@...ux.intel.com>,
LKML <linux-kernel@...r.kernel.org>,
DL-SHA-WorkGroupLinux <workgroup.linux@....com>,
Guo Zeng <Guo.Zeng@....com>, Barry Song <Baohua.Song@....com>
Subject: Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management
module driver
hi Lee,
thanks very much!
2015-09-20 12:15 GMT+08:00 Lee Jones <lee.jones@...aro.org>:
> On Thu, 17 Sep 2015, Barry Song wrote:
>
>> From: Guo Zeng <Guo.Zeng@....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@....com>
>> Signed-off-by: Barry Song <Baohua.Song@....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@...0 {
>> + 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@...0 {
>> + 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?
the registers are behind a rtc io bridge. HW needs to access the
bridge to access registers.
another example similar with pwrc MFD is sysrtc as below:
2019 rtc-iobg@...40000 {
2020 compatible = "sirf,prima2-rtciobg",
2021 "sirf-prima2-rtciobg-bus",
2022 "simple-bus";
2023 #address-cells = <1>;
2024 #size-cells = <1>;
2025 reg = <0x18840000 0x1000>;
2026
2027 sysrtc@...0 {
2028 compatible = "sirf,prima2-sysrtc";
2029 reg = <0x2000 0x100>;
2030 interrupts = <0 52 0>;
2031 };
2032 pwrc@...0 {
2033 compatible = "sirf,atlas7-pwrc";
2034 reg = <0x3000 0x100>;
2035 interrupts = <0 32 0>;
2036 rtcmclk: clock-controller {
its driver is at
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/rtc/rtc-sirfsoc.c
>
>> +#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.
i agree using MARCO is the general rules. but here moving to struct is
for avoiding things like:
if (prima2...)
...
elif(atlas7)
...
it is making registers offset private data which can be a member of the object.
>
> 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.
this was explained as they are not in memory space, they are behind a
bus bridge.
>
>> + 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.
there is only one device. why two regmap_irq_chip structures?
the driver is compatible with prima2 and atlas7, but any time there is
only one of them,
and the register needs to be adjust from dts and offset table.
>
>> + /* 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.
so do you think it is still a platform, what is the best way to probe them?
>
>> 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
-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists