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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGsJ_4ygLOf9LOhBFe4yJS+ZkWK40r6Hf_WEouKYS=TOppPjNw@mail.gmail.com>
Date:	Tue, 29 Sep 2015 14:18:58 +0800
From:	Barry Song <21cnbao@...il.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	Mark Brown <broonie@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	sameo <sameo@...ux.intel.com>,
	LKML <linux-kernel@...r.kernel.org>,
	DL-SHA-WorkGroupLinux <workgroup.linux@....com>,
	Guo Zeng <Guo.Zeng@....com>, Barry Song <Baohua.Song@....com>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/