On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote: > Hi, [...] > >> + 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? Yes. That will check the value is NUL-terminated, at least. > >> + 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. I'm not sure if regulator-name is the right way to go. I believe the thing to do is match on the node name of the subnodes (as you were doing here). The only thing I didn't like was assuming the nodes were voltage regulators if they weren't called "overall_current". I'd prefer explicit matching, or something more general. [...] > >> + 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. Sure. My questions was why it is necessary to preconfigure the regulators at all rather than why it is necessary to do so in this manner. Mark. -- 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
