Message-ID: <6401b71c-8c18-4735-8b40-9692cd788336@VA3EHSMHS037.ehs.local>
Date:	Thu, 19 Sep 2013 09:01:01 -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 06:17:12AM -0700, Guenter Roeck wrote:
> On Wed, Sep 18, 2013 at 03:43:38PM -0700, Soren Brinkmann wrote:
> > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators.
> > The devices generate low-jitter clock signals and are reprogrammable via
> > an I2C interface.
> > 
> > Cc: Guenter Roeck <linux@...ck-us.net>
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@...inx.com>
> > ---
> > v2:
> >  - document clock-output-names in bindings documentation
> >  - don't use wildcards in compatibility string
> >  - change Kconfig entry to "... 570 and compatible devices"
> >  - change some indentation flaws
> >  - use 10000 as MIN and MAX value in usleep_range
> >  - fail probe() if 'factory-fout' is not provided in DT
> >    - default factory fout #defines removed
> >  - use i2c driver_data instead of OF data
> >    - remove some related structs and data
> >  - rename DT property 'initial-fout' => 'clock-frequency' (like si5351
> >    driver)
> >  - add some more details regarding the 'factory-fout' DT property
> > 
> >  .../devicetree/bindings/clock/silabs,si570.txt     |  38 ++
> >  drivers/clk/Kconfig                                |  10 +
> >  drivers/clk/Makefile                               |   1 +
> >  drivers/clk/clk-si570.c                            | 517 +++++++++++++++++++++
> >  4 files changed, 566 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/silabs,si570.txt
> >  create mode 100644 drivers/clk/clk-si570.c
> > 
[...]
> > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c
> > new file mode 100644
> > index 0000000..c20dfce
> > --- /dev/null
> > +++ b/drivers/clk/clk-si570.c
[...]
> > +static unsigned long si570_recalc_rate(struct clk_hw *hw,
> > +		unsigned long parent_rate)
> > +{
> > +	int err;
> > +	u64 rfreq, rate;
> > +	unsigned int n1, hs_div;
> > +	struct clk_si570 *data = to_clk_si570(hw);
> > +
> > +	err = si570_get_divs(data, &rfreq, &n1, &hs_div);
> > +	if (err) {
> > +		dev_err(&data->i2c_client->dev, "unable to recalc rate\n");
> 
> [ Not really helpful that you can not return an error here ]
IIUC, the CCF does not really support error reporting for this function. I
think a return value of recalc_rate is always interpreted as frequency.
Hence, I decided to take the last point of certainty and return the last
known frequency.

> 
> > +		return data->frequency;
> > +	}
> > +
> > +	rfreq = div_u64(rfreq, hs_div * n1);
> > +	rate = (data->fxtal * rfreq) >> 28;
> > +
> > +	return rate;
> > +}
[...]
> > +static int si570_set_rate(struct clk_hw *hw, unsigned long rate,
> > +		unsigned long parent_rate)
> > +{
> > +	struct clk_si570 *data = to_clk_si570(hw);
> > +	struct i2c_client *client = data->i2c_client;
> > +	int err;
> > +
> > +	if (rate < SI570_MIN_FREQ || rate > data->max_freq) {
> > +		dev_warn(&client->dev,
> > +			"requested frequency %lu Hz is out of range\n", rate);
> 
> Shouldn't this be dev_err ?
You're probably right. I'll change this.

> 
> > +		return -EINVAL;
> > +	}
[...]
> > +static int si570_probe(struct i2c_client *client,
> > +		const struct i2c_device_id *id)
> > +{
> > +	struct clk_si570 *data;
> > +	struct clk_init_data init;
> > +	struct clk *clk;
> > +	u32 initial_fout, factory_fout;
> > +	int err;
> > +
> > +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	init.ops = &si570_clk_ops;
> > +	init.flags = CLK_IS_ROOT;
> > +	init.num_parents = 0;
> > +	data->hw.init = &init;
> > +	data->max_freq = id->driver_data;
> > +	data->i2c_client = client;
> > +
> > +	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.

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

> Even better would be if it can be auto-detected; in that case you would not need
> the property at all. Unfortunately I don't have access to a chip, so I have
> no idea if that is possible and can not check it myself either.
I didn't see any way to detect this during runtime. And we don't have
such a device either.

> 
> > +	if (of_property_read_string(client->dev.of_node, "clock-output-names",
> > +			&init.name))
> > +		init.name = client->dev.of_node->name;
> > +
> > +	err = of_property_read_u32(client->dev.of_node, "factory-fout",
> > +			&factory_fout);
> > +	if (err) {
> > +		dev_err(&client->dev, "'factory-fout' property missing\n");
> > +		return err;
> > +	}
> > +
> > +	data->regmap = devm_regmap_init_i2c(client, &si570_regmap_config);
> > +	if (IS_ERR(data->regmap)) {
> > +		dev_err(&client->dev, "failed to allocate register map\n");
> > +		return PTR_ERR(data->regmap);
> > +	}
> > +
> > +	i2c_set_clientdata(client, data);
> > +	err = si570_get_defaults(data, factory_fout);
> > +	if (err)
> > +		return err;
> > +
> > +	clk = devm_clk_register(&client->dev, &data->hw);
> > +	if (IS_ERR(clk)) {
> > +		dev_err(&client->dev, "clock registration failed\n");
> > +		return PTR_ERR(clk);
> > +	}
> > +	err = of_clk_add_provider(client->dev.of_node, of_clk_src_simple_get,
> > +			clk);
> > +	if (err) {
> > +		dev_err(&client->dev, "unable to add clk provider\n");
> > +		return err;
> > +	}
> > +
> > +	/*
> > +	 * Read the requested initial fout from either platform data or the
> > +	 * device tree
> > +	 */
> 
> But you don't do anything with platform data.
copy&paste error. I'll remove it.

> 
> > +	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.


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

Powered by blists - more mailing lists