Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

From: Lee Jones
Date: Thu Sep 24 2015 - 14:13:29 EST

On Mon, 21 Sep 2015, Barry Song wrote:
> 2015-09-20 12:15 GMT+08:00 Lee Jones <lee.jones@xxxxxxxxxx>:
> > 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

[...]

> >> +#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@18840000 {
> 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@2000 {
> 2028 compatible = "sirf,prima2-sysrtc";
> 2029 reg = <0x2000 0x100>;
> 2030 interrupts = <0 52 0>;
> 2031 };
> 2032 pwrc@3000 {
> 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

I meant what are you using in the header file, but I looked it up.

> >> +#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.

Hmm... it's pretty unusual, but I can't think of a better way of doing
it yet.

[...]

> >> +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.

Use 'ranges' in the DT, then you can pull out the proper address
without hand rolling your own method.

[...]

> >> + 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.

Why does the 'base' offset have to be drawn from DT? Does it change?

I think you should create two static regmap_irq_chip structures and do
only pass the relevant one to regmap.

See how everyone else does it.

[...]

> >> +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?

Yes, it's still a platform. It's just not a module.

[...]

--
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/