On 09/05/2014 16:31, Maxime Ripard wrote:
> Hi Boris,
>
> On Wed, May 07, 2014 at 07:58:35PM +0200, Boris BREZILLON wrote:
>
[...]
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + p2wi->regs = devm_ioremap_resource(dev, r);
> + if (IS_ERR(p2wi->regs)) {
> + dev_err(dev, "failed to retrieve iomem resource\n");
> + return PTR_ERR(p2wi->regs);
> You seem to be returning the error code in the next error messages. It
> would probably be a good idea to put it there too.
Yes, I'll print the err code in the error message.
>
>> + }
>> +
>> + snprintf(p2wi->adapter.name, sizeof(p2wi->adapter.name), pdev->name);
>> + ret = platform_get_irq(pdev, 0);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to retrieve irq: %d\n", ret);
>> + return ret;
>> + }
>> + p2wi->irq = ret;
>> +
>> + p2wi->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(p2wi->clk)) {
>> + ret = PTR_ERR(p2wi->clk);
>> + dev_err(dev, "failed to retrieve clk: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + ret = clk_prepare_enable(p2wi->clk);
>> + if (ret) {
>> + dev_err(dev, "failed to enable clk: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + parent_clk_freq = clk_get_rate(p2wi->clk);
>> +
>> + p2wi->rstc = devm_reset_control_get(dev, NULL);
>> + if (IS_ERR(p2wi->rstc)) {
>> + ret = PTR_ERR(p2wi->rstc);
>> + dev_err(dev, "failed to retrieve reset controller: %d\n",
>> + ret);
>> + goto err_clk_disable;
>> + }
>> +
>> + ret = reset_control_deassert(p2wi->rstc);
>> + if (ret) {
>> + dev_err(dev, "failed to deassert reset line: %d\n",
>> + ret);
>> + goto err_clk_disable;
>> + }
>> +
>> + init_completion(&p2wi->complete);
>> + p2wi->adapter.dev.parent = dev;
>> + p2wi->adapter.algo = &p2wi_algo;
>> + p2wi->adapter.owner = THIS_MODULE;
>> + p2wi->adapter.dev.of_node = pdev->dev.of_node;
>> + platform_set_drvdata(pdev, p2wi);
>> + i2c_set_adapdata(&p2wi->adapter, p2wi);
>> +
>> + ret = devm_request_irq(dev, p2wi->irq, p2wi_interrupt, 0, pdev->name,
>> + p2wi);
>> + if (ret) {
>> + dev_err(dev, "can't register interrupt handler irq%d: %d\n",
>> + p2wi->irq, ret);
>> + goto err_reset_assert;
>> + }
>> +
>> + writel(P2WI_CTRL_SOFT_RST, p2wi->regs + P2WI_CTRL);
>> +
>> + clk_div = parent_clk_freq / clk_freq;
>> + if (!clk_div) {
>> + dev_warn(dev,
>> + "clock-frequency is too high, setting it to %lu Hz\n",
>> + parent_clk_freq);
>> + clk_div = 1;
>> + } else if (clk_div > P2WI_CCR_MAX_CLK_DIV) {
>> + dev_warn(dev,
>> + "clock-frequency is too low, setting it to %lu Hz\n",
>> + parent_clk_freq / P2WI_CCR_MAX_CLK_DIV);
>> + clk_div = P2WI_CCR_MAX_CLK_DIV;
>> + }
>> +
>> + writel(P2WI_CCR_SDA_OUT_DELAY(1) | P2WI_CCR_CLK_DIV(clk_div),
>> + p2wi->regs + P2WI_CCR);
>> +
>> + if (p2wi_mode_reg >= 0 && p2wi_mode_val >= 0) {
>> + writel(P2WI_PMCR_PMU_INIT_SEND |
>> + P2WI_PMCR_PMU_MODE_REG(p2wi_mode_reg) |
>> + P2WI_PMCR_PMU_INIT_DATA(p2wi_mode_val) |
>> + P2WI_PMCR_PMU_DEV_ADDR(p2wi->slave_addr),
>> + p2wi->regs + P2WI_PMCR);
>> +
>> + while (readl(p2wi->regs + P2WI_PMCR) &
>> + P2WI_PMCR_PMU_INIT_SEND)
>> + cpu_relax();
>> + }
> Did we actually encounter such a case? From what I've seen so far,
> both community's u-boot and Allwinner's bootloader already do this.
>
> As you know, I'm really not fond of putting random values and
> registers offsets into the DT itself. Making the assumption that the
> PMIC is already switched to P2WI by the bootloader seems pretty safe
> to me.
Hans, any opinion ?
If everybody agrees on that point, I'll remove the initialization part
from the probe (and drop the allwinner,reg-xx properties retrieval).
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
