lkml.org 
[lkml]   [2015]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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


\
 
 \ /
  Last update: 2015-09-29 08:41    [W:0.075 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site