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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 10 Sep 2014 15:18:53 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Stefan Wahren <info@...egoodbye.de>
Cc:	"lgirdwood@...il.com" <lgirdwood@...il.com>,
	"broonie@...nel.org" <broonie@...nel.org>,
	"shawn.guo@...aro.org" <shawn.guo@...aro.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	Pawel Moll <Pawel.Moll@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>,
	"stefan.wahren@...e.com" <stefan.wahren@...e.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"festevam@...il.com" <festevam@...il.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator
 driver

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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/