Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Stefan Wahren]

Hi,

Am 09.09.2014 20:22, schrieb Mark Rutland:
> [...]
>
> > +       regs = (__raw_readl(sreg->base_addr) & ~BM_POWER_LEVEL_TRG);
>
> I suspect you should be using the *_relaxed accessors rather than the
> __raw_* accessors.
>
> [...]
>
> > +static int mxs_regulator_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *np = dev->of_node;
> > +       struct device_node *parent;
> > +       struct regulator_desc *rdesc;
> > +       struct regulator_dev *rdev;
> > +       struct mxs_regulator *sreg;
> > +       struct regulator_init_data *initdata;
> > +       struct regulation_constraints *con;
> > +       struct regulator_config config = { };
> > +       void __iomem *base_addr = NULL;
> > +       void __iomem *power_addr = NULL;
> > +       u64 regaddr64 = 0;
> > +       const u32 *regaddr_p;
> > +       u32 val = 0;
> > +       int ret;
> > +
> > +       if (!np) {
> > +               dev_err(dev, "missing device tree\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* get device base address */
> > +       base_addr = of_iomap(np, 0);
> > +       if (!base_addr)
> > +               return -ENXIO;
> > +
> > +       parent = of_get_parent(np);
> > +       if (!parent)
> > +               return -ENXIO;
>
> Leak of the (successfully mapped) base_addr.
>
> > +
> > +       power_addr = of_iomap(parent, 0);
> > +       if (!power_addr)
> > +               return -ENXIO;
>
> Leak of base_addr and dangling refcount on parent. These apply to all
> subsequent returns.
>
> > +
> > +       regaddr_p = of_get_address(np, 0, NULL, NULL);
>
> of_get_address returns a __be32*, not a u32*, so sparse will be very
> unhappy here...
>
> > +       if (regaddr_p)
> > +               regaddr64 = of_translate_address(np, regaddr_p);
>
> ...and as of_translate_address returns a u64 you'll need a separate
> variable for the input and output.
>
> > +
> > +       if (!regaddr64) {
> > +               dev_err(dev, "no or invalid reg property set\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       initdata = of_get_regulator_init_data(dev, np);
> > +       if (!initdata)
> > +               return -EINVAL;
> > +
> > +       ret = of_property_read_u32(np, "mxs-max-reg-val",
> > +                                  &val);
> > +       if (!val) {
> > +               dev_err(dev, "no or invalid mxs-max-reg-val property set\n");
> > +               return ret;
> > +       }
> > +
> > +       dev_info(dev, "regulator found\n");
> > +
> > +       sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
> > +       if (!sreg)
> > +               return -ENOMEM;
> > +       sreg->initdata = initdata;
> > +       sreg->name = of_get_property(np, "regulator-name", NULL);
>
> I'm not keen on using of_get_property here. We have no idea if
> regulator-name is even a string (it should be, but we have no
> guarantee).

Better using of_property_read_string?

> > +       sreg->cur_uA = 0;
> > +       sreg->cur_uV = 0;
> > +       sreg->base_addr = base_addr;
> > +       sreg->power_addr = power_addr;
> > +       init_waitqueue_head(&sreg->wait_q);
> > +       spin_lock_init(&sreg->lock);
> > +       sreg->max_reg_val = val;
> > +
> > +       rdesc = &sreg->rdesc;
> > +       rdesc->name = sreg->name;
> > +       rdesc->owner = THIS_MODULE;
> > +       rdesc->ops = &mxs_rops;
> > +
> > +       if (strcmp(rdesc->name, "overall_current") == 0)
> > +               rdesc->type = REGULATOR_CURRENT;
> > +       else
> > +               rdesc->type = REGULATOR_VOLTAGE;
>
> Wouldn't it make more sense to explicitly match the names you expect?

Okay, i make "regulator-name" a required property and use a white list 
of all possible regulators.

> > +       con = &initdata->constraints;
> > +       rdesc->n_voltages = sreg->max_reg_val;
> > +       rdesc->min_uV = con->min_uV;
> > +       rdesc->uV_step = (con->max_uV - con->min_uV) / sreg->max_reg_val;
> > +       rdesc->linear_min_sel = 0;
> > +       rdesc->vsel_reg = regaddr64;
> > +       rdesc->vsel_mask = BM_POWER_LEVEL_TRG;
> > +
> > +       config.dev = &pdev->dev;
> > +       config.init_data = initdata;
> > +       config.driver_data = sreg;
> > +       config.of_node = np;
> > +
> > +       pr_debug("probing regulator %s %s %d\n",
> > +                       sreg->name,
> > +                       rdesc->name,
> > +                       pdev->id);
>
> Aren't those two names always the same per the code above?

Sure, i will fix that.

> > +
> > +       /* register regulator */
> > +       rdev = devm_regulator_register(dev, rdesc, &config);
> > +
> > +       if (IS_ERR(rdev)) {
> > +               dev_err(&pdev->dev, "failed to register %s\n",
> > +                       rdesc->name);
> > +               return PTR_ERR(rdev);
> > +       }
> > +
> > +       if (sreg->max_uA) {
> > +               struct regulator *regu;
> > +
> > +               regu = regulator_get(NULL, sreg->name);
> > +               sreg->nb.notifier_call = reg_callback;
> > +               regulator_register_notifier(regu, &sreg->nb);
> > +       }
> > +
> > +       platform_set_drvdata(pdev, rdev);
> > +
> > +       of_property_read_u32(np, "mxs-default-microvolt",
> > +                                  &val);
> > +
> > +       if (val)
> > +               mxs_set_voltage(rdev, val, val, NULL);
>
> As I mentioned in my comments on the binding, I'd like to know why this
> is necessary and if it is why it shouldn't be a standardised property.

From my understanding the standardised properties only defines a range, 
but no default state of the regulators. If the initialization from the 
bootloader or a hardcoded initialization in the driver is okay then the 
property is not necessary.

> Mark.

Thanks for your feedback.

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