| 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
| ||
|
Message-ID: <5428FE7B.8060700@i2se.com>
Date: Mon, 29 Sep 2014 08:38:51 +0200
From: Stefan Wahren <stefan.wahren@...e.com>
To: Mark Brown <broonie@...nel.org>
CC: lgirdwood@...il.com, shawn.guo@...aro.org, robh+dt@...nel.org,
pawel.moll@....com, mark.rutland@....com,
ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
festevam@...il.com, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH 2/2] regulator: add mxs regulator driver
Hi Mark,
thanks for your comments. Now it looks to me, that i try to reinvent the
wheel.
I'm searching for a good regulator implementation example.
Does it apply to ti-abb-regulator.c and twl-regulator.c?
Am 28.09.2014 um 12:16 schrieb Mark Brown:
> On Sat, Sep 27, 2014 at 12:59:48AM +0000, Stefan Wahren wrote:
>
>> + pr_debug("%s: min_uV %d, max_uV %d, min %d, max %d\n", __func__,
>> + min_uV, max_uV, con->min_uV, con->max_uV);
>> +
>> + if (max_uV < con->min_uV || max_uV > con->max_uV)
>> + return -EINVAL;
> This is replicating checks done by the core.
>
>> + val = (max_uV - con->min_uV) * sreg->rdesc.n_voltages /
>> + (con->max_uV - con->min_uV);
> Drivers should never look at their constraints, they should let the core
> do that and just do what they're asked. In this case it is probably
> best to implement a get_voltage_sel() operation and have the conversion
> to voltage done by regulator_map_voltage_linear(), this will both make
> the code look better and mean the driver gets the benefit of all the
> error checking done by the core.
Okay, i will do that. For the list_voltage operation
regulator_list_voltage_linear()
should be the perfect candidate.
>> + writel(val | regs, sreg->base_addr);
>> + for (i = 20; i; i--) {
>> + /* delay for fast mode */
>> + if (readl(power_sts) & BM_POWER_STS_DC_OK)
>> + return 0;
>> +
>> + udelay(1);
>> + }
>> +
>> + writel(val | regs, sreg->base_addr);
>> + start = jiffies;
>> + while (1) {
>> + /* delay for normal mode */
>> + if (readl(power_sts) & BM_POWER_STS_DC_OK)
>> + return 0;
> This really needs a comment to explain what on earth is going on here -
> the whole thing with writing the same thing twice with two delays is
> more than a little odd. It looks like the driver is trying to busy wait
> in cases where the change happens quickly but the comments about "fast"
> and "normal" mode make this unclear.
The regulator driver polls for the DC_OK bit in the power status register.
Quote for reference manual (p. 935): "High when switching DC-DC
converter control loop has stabilized after a voltage target change."
The two loops comes from the different regulator modes
(REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL).
In REGULATOR_MODE_FAST the voltage steping is disabled and changing
voltage should be fast. In REGULATOR_MODE_NORMAL voltage steping is
enabled and it's take a while for reaching the target voltage.
Do you see more a problem with the two different loops or the redundant
register write?
Is it acceptable that the function blocks here?
>> + pr_debug("%s: %s register val %d\n", __func__, sreg->name, val);
>> +
>> + switch (sreg->rdesc.id) {
>> + case MXS_VDDA:
>> + val >>= 16;
>> + break;
>> + case MXS_VDDD:
>> + val >>= 20;
>> + break;
>> + }
>> +
>> + return val ? 1 : 0;
>> +}
> This seems buggy - it'll always return true for MXS_VDDD if MXS_VDDA
> enabled won't it?
Shame on me, i forgot to remove this function. mxs_is_enabled is never used.
>> +static unsigned int mxs_get_mode(struct regulator_dev *reg)
>> +{
>> + struct mxs_regulator *sreg = rdev_get_drvdata(reg);
>> + u32 val = readl(sreg->base_addr) & sreg->mode_mask;
>> +
>> + return val ? REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
>> +}
> Please try to avoid the ternery operator, it does nothing for
> legibility.
if (readl(sreg->base_addr) & sreg->mode_mask)
return REGULATOR_MODE_FAST;
return REGULATOR_MODE_NORMAL;
Better?
>> + if (of_property_read_string(np, "regulator-name", &name)) {
>> + dev_err(dev, "missing property regulator-name\n");
>> + return -EINVAL;
>> + }
> Use different compatibles to identify the regulators, regulator-name
> should never be mandatory.
I will remove this and use the compatibles.
>> + switch (sreg->rdesc.id) {
>> + case MXS_VDDIO:
>> + sreg->mode_mask = BIT(17);
>> + break;
>> + case MXS_VDDA:
>> + sreg->mode_mask = BIT(18);
>> + break;
>> + case MXS_VDDD:
>> + sreg->mode_mask = BIT(22);
>> + break;
>> + }
> Why is this not looked up from the data structure like the rest of the
> data?
I was a little bit confused why there wasn't a mode_mask in the struct
regulator_desc. I will do it like the ti-abb-regulator in the matching
table.
>
>> + dev_info(dev, "%s found\n", name);
> Don't add noise like this to the boot log, it provides no useful
> information.
Okay, i will remove this.
>> + regulator_unregister(rdev);
>> + iounmap(power_addr);
>> + iounmap(base_addr);
> Use devm_ versions of functions.
As a result the remove function for the drivers becomes unnecessary. Am
i right?
>
>> +static int __init mxs_regulator_init(void)
>> +{
>> + return platform_driver_register(&mxs_regulator_driver);
>> +}
>> +postcore_initcall(mxs_regulator_init);
> module_platform_driver().
I wasn't sure because of the postcore stuff. I will fix it.
Best regards
Stefan
--
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/
Powered by blists - more mailing lists