On Wed, 2012-04-18 at 02:01 -0700, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Apr 17, 2012 at 06:00:28PM -0700, Rhyland Klein wrote:
> > Add device tree based initialization support for TI's tps65910 pmic.
>
> Actually, now I look at the larger patch this probably wants to be split
> up by driver and possibly split further within that.
>
> > + board_data = tps65910->board_data;
> > + if (board_data->use_dt_for_init_data && tps65910->dev->of_node)
> > + ret = tps65910_gpio_parse_dt(tps65910->dev, board_data);
> > +
>
> This is a really odd idiom - normally the pattern for device tree
> support is to just go and try to use the device tree data if it's there
> and there's no need for any flag to say if it should be used.
>
I agree its odd. My concern was that the idiom is that is pdata assigned
from board files should override dt data. At this point, we don't know
where the tps65910->board_data is coming from, dt or board files.
Arbitrarily using dt breaks that idiom. We could do a check like this if
you prefer:
if (!(dev_get_platdata(tps65910->dev) && tps65910->dev->of_node)
i.e. if doesn't have pdata supplied from board files, but does have dt
node.
> > + if (pdata->irq_base <= 0)
> > + pdata->irq_base = irq_alloc_descs(-1, 0, tps65910->irq_num, -1);
> > +
> > + if (pdata->irq_base <= 0) {
> > + dev_err(tps65910->dev, "Failed to allocate irq descs: %d\n",
> > + pdata->irq_base);
> > + return pdata->irq_base;
> > + }
> > +
> > + tps65910->irq_mask = 0xFFFFFF;
> > +
> > + mutex_init(&tps65910->irq_lock);
> > + tps65910->chip_irq = irq;
> > + tps65910->irq_base = pdata->irq_base;
>
> While this is needed for DT support it can be done separately and would
> probably be better split out into a separate patch.
>
ok.
> > + /* Pass of data to child devices */
> > + for (idx = 0; idx < ARRAY_SIZE(tps65910s); idx++) {
> > + tps65910s[idx].platform_data = pmic_plat_data;
> > + tps65910s[idx].pdata_size = sizeof(*pmic_plat_data);
> > + }
>
> Why is this needed - can't the DT parsing just be moved where it's used?
>
> > + for_each_child_of_node(regulators, child) {
> > + struct regulator_init_data *init_data;
> > +
> > + init_data = of_get_regulator_init_data(&pdev->dev, child);
> > + if (!init_data) {
> > + dev_err(&pdev->dev,
> > + "failed to parse DT for regulator %s\n",
> > + child->name);
> > + return -EINVAL;
> > + }
> > +
> > + for (idx = 0; idx < pmic->num_regulators; idx++) {
>
> Hrm, this iteration over a group of regulators to find the relevant
> node by name is going to be a fairly common pattern (there's already
> at least one driver doing this IIRC) - we should really factor it out
> into common code. Please consider doing this when you resubmit.
Ok.
>
> > + if (!strcasecmp(info[idx].name, child->name)) {
> > + if (all_data[idx]) {
> > + dev_err(&pdev->dev,
> > + "Duplicate Regulator Node %s\n",
>
> Please fix the capitalisation in the error message.
>
> > + /* Check to see if we iterated without finding its name */
> > + if (idx == pmic->num_regulators) {
> > + dev_err(&pdev->dev,
> > + "Unknown regulator node found [%s]\n",
> > + child->name);
> > + return -EINVAL;
> > + }
>
> It'd seem more robust to only print the warning and not return the
> error, that way we don't completely fail the device initialisation if
> there's data we don't understand.
>
> I'm also not seeing a change here that passes the DT node to
> regulator_register() - you should be doing that, it's needed so
> consumers can bind to the regulator.
> * Unknown Key
> * 0x6E30FDDD
Thanks,
rhyland
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
