Messages in this thread | | | Date | Tue, 29 Sep 2015 08:16:36 +0100 | From | Lee Jones <> | Subject | Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver |
| |
On Tue, 29 Sep 2015, Barry Song wrote: > >> >> +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. > > it seems it is not a "ranges" thing, things behind rtciobrg is much > like things behind USB or sdio. we need to use a rtciobrg protocol to > do read/write. > they can not be randomly accessed by load/store, and can't be XIP. > they don't have any ranges in CPU memory space.
So what's the point of 'base' then? I assumed this was the base of the IP registers which where memory mapped?
> > [...] > > > >> >> + 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. > > that is ok, if this driver picks up one regmap_irq_chip from two > according to of compatible strings.
Okay, great.
> > [...] > > > >> >> +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, i don't have idea on this. > > as a module, if it is built-in, it is initilized during > device_initcall, if it is not built-in, it is initilized during > insmod. > when you say it is not a module, > do you mean it must be built-in, or do you mean it is not a device_initcall?
Use builtin_platform_driver_probe().
-- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
| |