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

[Lee Jones]

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