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

[Stefan Wahren <info@lategoodbye.de>]

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