Messages in this thread | | | From | Barry Song <> | Date | Tue, 29 Sep 2015 14:18:58 +0800 | Subject | Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver |
| |
>> >> +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.
> > [...] > >> >> + 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.
> > [...] > >> >> +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?
-barry
| |