Re: [PATCH 1/2] regulator: max1586 add device-tree support

[Robert Jarzmik <robert.jarzmik@free.fr>]

Mark Brown <broonie@kernel.org> writes:

> On Tue, Jun 17, 2014 at 09:16:52PM +0200, Robert Jarzmik wrote:
>> Mark Brown <broonie@kernel.org> writes:
>> > On Sat, Jun 14, 2014 at 04:54:24PM +0200, Robert Jarzmik wrote:
>
>> >> +	matched = of_regulator_match(dev, np, rmatch, ARRAY_SIZE(rmatch));
>> >> +	of_node_put(np);
>> >> +	if (matched <= 0)
>> >> +		return matched;
>
>> > Why is this treating zero as an error?  We should be able to at least
>> > report the current state of regulators even if none are configured in
>> > the device tree.
>
>> Euh how so an error ?
>
>> If 0 is returned, this means no regulators are found in device-tree. It's not an
>> error, it's a lack of regulators (ie. no Output_V3 and no Output_V6), and no
>> more handling is necessary in this function, while returning "ok", ie 0 ...
>
> OK, so there's just nothing to do in that case.  That's fine, but it's
> just not at all clear from the code.  A comment would help.
OK, no problem.

>
>> As for the "state report", this max1586 doesn't report anything, it cannot even
>> be queried about the current voltage, sic ...
>
> It can't?  That's unfortunate, though I was able to turn up a datasheet
> which appears to support that.
Oh really ? Well, tell me where you read it.

My personal reading from the Max1586 specs is (page 21, chapter "Serial Interface") :
    The LSB of the address word is the read/write (R/W) bit.
    R/W indicates whether the master is writing or reading
    (RD/W 0 = write, RD/W 1 = read). The MAX1586/
    MAX1587 only support the SEND BYTE format; there-
    fore, RD/W is required to be 0.

I'm wondering if you have this sentence in your datasheet too.

>> If you want me to modify this bit I need a bit more of an explanation to
>> understand.
>
> Where the driver is doing unusual things if they are actually sensible
> then the change needs to be clearer about why.
So would a comment like this address your comment ?

	/* Either matched < 0 and return the error. Or matched is 0 which means
	 * no init data was found, ie. no regulator is configured, and return 0
	 * to caller, stating neither error nor any matched regulator.
	 */

	if (matched <= 0)
		return matched;

Cheers.

-- 
Robert