linux-kernel - Re: [PATCH v2] clk: si570: Add a driver for SI570 oscillators

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]

Message-ID: <86ee8eea-7a98-443b-a156-46f3db1f9678@CO1EHSMHS031.ehs.local>
Date:	Thu, 19 Sep 2013 13:59:38 -0700
From:	Sören Brinkmann <soren.brinkmann@...inx.com>
To:	Guenter Roeck <linux@...ck-us.net>
CC:	Rob Herring <rob.herring@...xeda.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Rob Landley <rob@...dley.net>,
	Mike Turquette <mturquette@...aro.org>,
	Grant Likely <grant.likely@...aro.org>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	<devicetree@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	Hyun Kwon <hyunk@...inx.com>
Subject: Re: [PATCH v2] clk: si570: Add a driver for SI570 oscillators

On Thu, Sep 19, 2013 at 09:44:38AM -0700, Guenter Roeck wrote:
> On Thu, Sep 19, 2013 at 09:01:01AM -0700, Sören Brinkmann wrote:
> > On Thu, Sep 19, 2013 at 06:17:12AM -0700, Guenter Roeck wrote:
> > > On Wed, Sep 18, 2013 at 03:43:38PM -0700, Soren Brinkmann wrote:
[...]
> > > > +	if (of_property_read_bool(client->dev.of_node,
> > > > +				"temperature-stability-7ppm"))
> > > > +		data->div_offset = SI570_DIV_OFFSET_7PPM;
> > > > +
> > > Just noticed that you dropped platform data support. Doesn't matter much for me
> > > right now, but in my previous company we used the chip on an x86 system which
> > > does not support devicetree. Would be nice to keep it and derive platform data
> > > from devicetree data if provided, like other drivers do it.
> > I'll look into this. The issue I have with that is, I can hardly test it
> > since we only use this on Zynq which uses DT. So, I'd rather prefer to
> > not include it unless somebody volunteers to test it.
> > 
> Fair enough. I can not test it myself anymore, and my previous employer
> now has a strict non-contributions-to-linux policy, so I guess they won't
> test it either or at least not publish any test results. Leave it out.
> 
> > > The 7ppm option is only relevant for si570/si751 and not supported on
> > > si598/si599. You should mention that in the bindings document and check for it.
> > Right, I'll add a note in the doc. And ignore it for devices this does
> > not apply.
> > 
> I would bail out, but that is your call.
Correct me if I'm wrong, but in general drivers do not test for
unsupported properties. So, in case we have one of the supported 59x
device, we should not test whether a (unsupported) property is present,
just to fail in that case, IMHO.

[...]
> > > > +	if (!of_property_read_u32(client->dev.of_node, "clock-frequency",
> > > > +				&initial_fout)) {
> > > > +		err = clk_set_rate(clk, initial_fout);
> > > > +		if (err)
> > > > +			dev_warn(&client->dev,
> > > > +					"unable to set initial output frequency %u: %d\n",
> > > > +					initial_fout, err);
> > > 
> > > No bailout ?
> > > 
> > > Also it seems that this generates two error messages, once in the code which
> > > experiences the error and once here.
> > I remove the message here.
> > 
> > > 
> > > Maybe it would be better to just bail out and return the error.
> > > After all, something is seriously wrong and the system won't operate
> > > as specified.
> > I do more think of a spurious error (typo in DT prop giving an f out of
> > range,...)  and a driver actually controlling this clock generator later.
> > In that case later calls to clk_set_rate() might succeed and everything's
> > fine or the driver can handle the error. In case of using this device as
> > a fixed clock, bailing out here might make more sense though. I'd prefer
> > leaving it like this.
> > 
> A typo in dt data isn't really a spurious error, and it would be easy to fix.
> I would suggest to bail out; this ensures that the devicetree data is correct.
Okay, bailing out it is.

	Sören


--
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/