clk: si570: Add a driver for SI570 oscillators

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@roeck-us.net>
Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 .../devicetree/bindings/clock/silabs,si570.txt     |  31 ++
 drivers/clk/Kconfig                                |  10 +
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-si570.c                            | 546 +++++++++++++++++++++
 4 files changed, 588 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/silabs,si570.txt
 create mode 100644 drivers/clk/clk-si570.c

On Thu, Sep 12, 2013 at 05:55:37PM -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@roeck-us.net>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
>  .../devicetree/bindings/clock/silabs,si570.txt     |  31 ++
>  drivers/clk/Kconfig                                |  10 +
>  drivers/clk/Makefile                               |   1 +
>  drivers/clk/clk-si570.c                            | 546 +++++++++++++++++++++
>  4 files changed, 588 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/silabs,si570.txt
>  create mode 100644 drivers/clk/clk-si570.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt
> new file mode 100644
> index 0000000..099f0ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/silabs,si570.txt
> @@ -0,0 +1,31 @@
> +Binding for Silicon Labs 570, 571, 598 and 599  programmable
> +I2C clock generators.
> +
> +Reference
> +This binding uses the common clock binding[1]. Details about the devices can be
> +found in the data sheets[2][3].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +[2] Si57x Data Sheet
> +    http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf
> +[3] Si59x Data Sheet
> +    http://www.silabs.com/Support%20Documents/TechnicalDocs/si598-99.pdf
> +
> +Required properties:
> + - compatible: Shall be one of "silabs,si57x", "silabs,si59x".

This should probably explicitly list all supported devices, not "x".
After all, there might at some point be an incompatible device which
still matches the "x".

> + - reg: I2C device address.
> + - #clock-cells: From common clock bindings: Shall be 0.
> + - factory-fout: Factory set default frequency
> +
> +Optional properties:
> + - initial-fout: Initial output frequency to set during probe
> + - temperature-stability-7ppm: Indicate a device with a temperature stability
> +			       of 7ppm
> +
> +Example:
> +	si570: clock-generator@5d {
> +		#clock-cells = <0>;
> +		compatible = "silabs,si570";
> +		reg = <0x5d>;
> +		factory-fout = <156250000>;
> +	};
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 279407a..f5afabc 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -64,6 +64,16 @@ config COMMON_CLK_SI5351
>  	  This driver supports Silicon Labs 5351A/B/C programmable clock
>  	  generators.
>  
> +config COMMON_CLK_SI570
> +	tristate "Clock driver for SiLabs 57x/59x"

Better state something like "SiLabs Si570 and compatible chips"

> +	depends on I2C
> +	depends on OF
> +	select REGMAP_I2C
> +	help
> +	---help---
> +	  This driver supports Silicon Labs 570/571/598/599 programmable
> +	  clock generators.
> +
>  config COMMON_CLK_S2MPS11
>  	tristate "Clock driver for S2MPS11 MFD"
>  	depends on MFD_SEC_CORE
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 7b11106..c0e94b3 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o
>  obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
>  obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
>  obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o
> +obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o
>  obj-$(CONFIG_COMMON_CLK_S2MPS11) += clk-s2mps11.o
>  obj-$(CONFIG_CLK_TWL6040)	+= clk-twl6040.o
>  obj-$(CONFIG_CLK_PPC_CORENET)	+= clk-ppc-corenet.o
> diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c
> new file mode 100644
> index 0000000..960d689
> --- /dev/null
> +++ b/drivers/clk/clk-si570.c
> @@ -0,0 +1,546 @@
> +/*
> + * Driver for Silicon Labs Si570/Si571 Programmable XO/VCXO
> + *
> + * Copyright (C) 2010, 2011 Ericsson AB.
> + * Copyright (C) 2011 Guenter Roeck.
> + * Copyright (C) 2011 - 2013 Xilinx Inc.
> + *
> + * Author: Guenter Roeck <guenter.roeck@ericsson.com>
> + *	   Sören Brinkmann <soren.brinkmann@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* Si570 registers */
> +#define SI570_REG_HS_N1		7
> +#define SI570_REG_N1_RFREQ0	8
> +#define SI570_REG_RFREQ1	9
> +#define SI570_REG_RFREQ2	10
> +#define SI570_REG_RFREQ3	11
> +#define SI570_REG_RFREQ4	12
> +#define SI570_REG_CONTROL	135
> +#define SI570_REG_FREEZE_DCO	137
> +#define SI570_DIV_OFFSET_7PPM	6
> +
> +#define HS_DIV_SHIFT		5
> +#define HS_DIV_MASK		0xe0
> +#define HS_DIV_OFFSET		4
> +#define N1_6_2_MASK		0x1f
> +#define N1_1_0_MASK		0xc0
> +#define RFREQ_37_32_MASK	0x3f
> +
> +#define SI570_FOUT_FACTORY_DFLT	156250000LL
> +#define SI598_FOUT_FACTORY_DFLT	10000000LL
> +
> +#define SI570_MIN_FREQ		10000000L
> +#define SI570_MAX_FREQ		1417500000L
> +#define SI598_MAX_FREQ		525000000L
> +
> +#define FDCO_MIN		4850000000LL
> +#define FDCO_MAX		5670000000LL
> +
> +#define SI570_CNTRL_RECALL	(1 << 0)
> +#define SI570_CNTRL_FREEZE_M	(1 << 5)
> +#define SI570_CNTRL_NEWFREQ	(1 << 6)
> +
> +#define SI570_FREEZE_DCO	(1 << 4)
> +
> +/**
> + * struct clk_si570:
> + * @hw:	Clock hw struct
> + * @regmap:	Device's regmap
> + * @div_offset:	Rgister offset for dividers
> + * @max_freq:	Maximum frequency for this device
> + * @fxtal:	Factory xtal frequency
> + * @n1:		Clock divider N1
> + * @hs_div:	Clock divider HSDIV
> + * @rfreq:	Clock multiplier RFREQ
> + * @frequency:	Current output frequency
> + * @i2c_client:	I2C client pointer
> + */
> +struct clk_si570 {
> +	struct clk_hw hw;
> +	struct regmap *regmap;
> +	unsigned int div_offset;
> +	u64 max_freq;
> +	u64 fxtal;
> +	unsigned int n1;
> +	unsigned int hs_div;
> +	u64 rfreq;
> +	u64 frequency;
> +	struct i2c_client *i2c_client;
> +};
> +#define to_clk_si570(_hw)	container_of(_hw, struct clk_si570, hw)
> +
> +/**
> + * struct si570_device_data:
> + * @max_freq:	Maximum output frequency
> + * @default_fout:	Default factory output frequency value
> + */
> +struct si570_device_data {
> +	u64 max_freq;
> +	u32 default_fout;
> +};
> +
> +static const struct si570_device_data si57x_ddata = {
> +	.max_freq = SI570_MAX_FREQ,
> +	.default_fout = SI570_FOUT_FACTORY_DFLT
> +};
> +
> +static const struct si570_device_data si59x_ddata = {
> +	.max_freq = SI598_MAX_FREQ,
> +	.default_fout = SI598_FOUT_FACTORY_DFLT
> +};
> +
> +/**
> + * si570_get_divs() - Read clock dividers from HW
> + * @data:	Pointer to struct clk_si570
> + * @rfreq:	Fractional multiplier (output)
> + * @n1:		Divider N1 (output)
> + * @hs_div:	Divider HSDIV (output)
> + * Returns 0 on success, negative errno otherwise.
> + *
> + * Retrieve clock dividers and multipliers from the HW.
> + */
> +static int si570_get_divs(struct clk_si570 *data, u64 *rfreq,
> +		unsigned int *n1, unsigned int *hs_div)

I would suggest to align continuation lines with the opening (,
though that is really up to you (and the maintainer).

> +{
> +	int err;
> +	u8 reg[6];
> +	u64 tmp;
> +
> +	err = regmap_bulk_read(data->regmap, SI570_REG_HS_N1 + data->div_offset,
> +			reg, ARRAY_SIZE(reg));
> +	if (err)
> +		return err;
> +
> +	*hs_div = ((reg[0] & HS_DIV_MASK) >> HS_DIV_SHIFT) + HS_DIV_OFFSET;
> +	*n1 = ((reg[0] & N1_6_2_MASK) << 2) + ((reg[1] & N1_1_0_MASK) >> 6) + 1;
> +	/* Handle invalid cases */
> +	if (*n1 > 1)
> +		*n1 &= ~1;
> +
> +	tmp = reg[1] & RFREQ_37_32_MASK;
> +	tmp = (tmp << 8) + reg[2];
> +	tmp = (tmp << 8) + reg[3];
> +	tmp = (tmp << 8) + reg[4];
> +	tmp = (tmp << 8) + reg[5];
> +	*rfreq = tmp;
> +
> +	return 0;
> +}
> +
> +/**
> + * si570_get_defaults() - Get default values
> + * @data:	Driver data structure
> + * @fout:	Factory frequency output
> + * Returns 0 on success, negative errno otherwise.
> + */
> +static int si570_get_defaults(struct clk_si570 *data, u64 fout)
> +{
> +	int err;
> +	u64 fdco;
> +
> +	regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_RECALL);
> +
> +	err = si570_get_divs(data, &data->rfreq, &data->n1, &data->hs_div);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Accept optional precision loss to avoid arithmetic overflows.
> +	 * Acceptable per Silicon Labs Application Note AN334.
> +	 */
> +	fdco = fout * data->n1 * data->hs_div;
> +	if (fdco >= (1LL << 36))
> +		data->fxtal = div64_u64(fdco << 24, data->rfreq >> 4);
> +	else
> +		data->fxtal = div64_u64(fdco << 28, data->rfreq);
> +
> +	data->frequency = fout;
> +
> +	return 0;
> +}
> +
> +/**
> + * si570_update_rfreq() - Update clock multiplier
> + * @data:	Driver data structure
> + * Passes on regmap_bulk_write() return value.
> + */
> +static int si570_update_rfreq(struct clk_si570 *data)
> +{
> +	u8 reg[5];
> +
> +	reg[0] = ((data->n1 - 1) << 6) |
> +		((data->rfreq >> 32) & RFREQ_37_32_MASK);
> +	reg[1] = (data->rfreq >> 24) & 0xff;
> +	reg[2] = (data->rfreq >> 16) & 0xff;
> +	reg[3] = (data->rfreq >> 8) & 0xff;
> +	reg[4] = data->rfreq & 0xff;
> +
> +	return regmap_bulk_write(data->regmap, SI570_REG_N1_RFREQ0 +
> +			data->div_offset, reg, ARRAY_SIZE(reg));
> +}
> +
> +/**
> + * si570_calc_divs() - Caluclate clock dividers
> + * @frequency:	Target frequency
> + * @data:	Driver data structure
> + * @out_rfreq:	RFREG fractional multiplier (output)
> + * @out_n1:	Clock divider N1 (output)
> + * @out_hs_div:	Clock divider HSDIV (output)
> + * Returns 0 on success, negative errno otherwise.
> + *
> + * Calculate the clock dividers (@out_hs_div, @out_n1) and clock multiplier
> + * (@out_rfreq) for a given target @frequency.
> + */
> +static int si570_calc_divs(unsigned long frequency, struct clk_si570 *data,
> +		u64 *out_rfreq, unsigned int *out_n1, unsigned int *out_hs_div)
> +{
> +	int i;
> +	unsigned int n1, hs_div;
> +	u64 fdco, best_fdco = ULLONG_MAX;
> +	static const uint8_t si570_hs_div_values[] = { 11, 9, 7, 6, 5, 4 };
> +
> +	for (i = 0; i < ARRAY_SIZE(si570_hs_div_values); i++) {
> +		hs_div = si570_hs_div_values[i];
> +		/* Calculate lowest possible value for n1 */
> +		n1 = div_u64(div_u64(FDCO_MIN, hs_div), frequency);
> +		if (!n1 || (n1 & 1))
> +			n1++;
> +		while (n1 <= 128) {
> +			fdco = (u64)frequency * (u64)hs_div * (u64)n1;
> +			if (fdco > FDCO_MAX)
> +				break;
> +			if (fdco >= FDCO_MIN && fdco < best_fdco) {
> +				*out_n1 = n1;
> +				*out_hs_div = hs_div;
> +				*out_rfreq = div64_u64(fdco << 28, data->fxtal);
> +				best_fdco = fdco;
> +			}
> +			n1 += (n1 == 1 ? 1 : 2);
> +		}
> +	}
> +
> +	if (best_fdco == ULLONG_MAX)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +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");
> +		return data->frequency;
> +	}
> +
> +	rfreq = div_u64(rfreq, hs_div * n1);
> +	rate = (data->fxtal * rfreq) >> 28;
> +
> +	return rate;
> +}
> +
> +static long si570_round_rate(struct clk_hw *hw, unsigned long rate,
> +		unsigned long *parent_rate)
> +{
> +	int err;
> +	u64 rfreq;
> +	unsigned int n1, hs_div;
> +	struct clk_si570 *data = to_clk_si570(hw);
> +
> +	if (!rate)
> +		return 0;
> +
> +	if (div64_u64(abs(rate - data->frequency) * 10000LL,
> +				data->frequency) < 35) {
> +		rfreq = div64_u64((data->rfreq * rate) +
> +				div64_u64(data->frequency, 2), data->frequency);
> +		n1 = data->n1;
> +		hs_div = data->hs_div;
> +
> +	} else {
> +		err = si570_calc_divs(rate, data, &rfreq, &n1, &hs_div);
> +		if (err) {
> +			dev_err(&data->i2c_client->dev,
> +					"unable to round rate\n");
> +			return 0;
> +		}
> +	}
> +
> +	return rate;
> +}
> +
> +/**
> + * si570_set_frequency() - Adjust output frequency
> + * @data:	Driver data structure
> + * @frequency:	Target frequency
> + * Returns 0 on success.
> + *
> + * Update output frequency for big frequency changes (> 3,500 ppm).
> + */
> +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency)
> +{
> +	int err;
> +
> +	err = si570_calc_divs(frequency, data, &data->rfreq, &data->n1,
> +			&data->hs_div);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * The DCO reg should be accessed with a read-modify-write operation
> +	 * per AN334
> +	 */
> +	regmap_write(data->regmap, SI570_REG_FREEZE_DCO, SI570_FREEZE_DCO);
> +	regmap_write(data->regmap, SI570_REG_HS_N1 + data->div_offset,
> +				  ((data->hs_div - HS_DIV_OFFSET) <<
> +				   HS_DIV_SHIFT)
> +				  | (((data->n1 - 1) >> 2) & N1_6_2_MASK));

This is an example where the alignment to ( would really help.

	regmap_write(data->regmap, SI570_REG_HS_N1 + data->div_offset,
		     ((data->hs_div - HS_DIV_OFFSET) << HS_DIV_SHIFT)
		     | (((data->n1 - 1) >> 2) & N1_6_2_MASK));

> +	si570_update_rfreq(data);
> +	regmap_write(data->regmap, SI570_REG_FREEZE_DCO, 0);
> +	regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_NEWFREQ);
> +
> +	/* Applying a new frequency can take up to 10ms */
> +	usleep_range(10000, 10001);

One microsecond range doesn't really buy anything besides forcing the compiler
to generate extra code. Why not just use 10000 ?

> +
> +	return 0;
> +}
> +
> +/**
> + * si570_set_frequency_small() - Adjust output frequency
> + * @data:	Driver data structure
> + * @frequency:	Target frequency
> + * Returns 0 on success.
> + *
> + * Update output frequency for small frequency changes (< 3,500 ppm).
> + */
> +static int si570_set_frequency_small(struct clk_si570 *data,
> +				     unsigned long frequency)
> +{
> +	/*
> +	 * This is a re-implementation of DIV_ROUND_CLOSEST
> +	 * using the div64_u64 function lieu of letting the compiler
> +	 * insert EABI calls
> +	 */
> +	data->rfreq = div64_u64((data->rfreq * frequency) +
> +			div_u64(data->frequency, 2), data->frequency);
> +	regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_FREEZE_M);
> +	si570_update_rfreq(data);
> +	regmap_write(data->regmap, SI570_REG_CONTROL, 0);
> +
> +	return 0;
> +}
> +
> +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);
> +		return -EINVAL;
> +	}
> +
> +	if (div64_u64(abs(rate - data->frequency) * 10000LL,
> +				data->frequency) < 35)
> +		err = si570_set_frequency_small(data, rate);
> +	else
> +		err = si570_set_frequency(data, rate);
> +
> +	if (err)
> +		return err;
> +
> +	data->frequency = rate;
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops si570_clk_ops = {
> +	.recalc_rate = si570_recalc_rate,
> +	.round_rate = si570_round_rate,
> +	.set_rate = si570_set_rate,
> +};
> +
> +static const struct of_device_id clk_si570_of_match[] = {
> +	{ .compatible = "silabs,si57x", .data = &si57x_ddata },
> +	{ .compatible = "silabs,si59x", .data = &si59x_ddata },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, clk_si570_of_match);
> +
> +static bool si570_regmap_is_volatile(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case 135:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool si570_regmap_is_writeable(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case 7 ... 18:
> +	case 135:
> +	case 137:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static struct regmap_config si570_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_RBTREE,
> +	.max_register = 137,
> +	.writeable_reg = si570_regmap_is_writeable,
> +	.volatile_reg = si570_regmap_is_volatile,
> +};
> +
> +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;
> +	const struct of_device_id *match;
> +	const struct si570_device_data *ddata;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	match = of_match_node(clk_si570_of_match, client->dev.of_node);
> +	if (!match)
> +		return -EINVAL;

Seems unusual. Is this really needed ? It precludes the driver from being used
in a non-devicetree environment, for example. I would guess that there is a match
if client->dev.of_node is set. Otherwise, this code would be needed in every
driver supporting devicetree, and I don't recall seeing that.

> +	ddata = match->data;
> +
You should be able to get this information (ie the pointer to si570_device_data)
from id->driver_data. That would be more consistent with other i2c devices.

> +	init.ops = &si570_clk_ops;
> +	init.flags = CLK_IS_ROOT;
> +	init.num_parents = 0;
> +	data->hw.init = &init;
> +	data->max_freq = ddata->max_freq;
> +	data->i2c_client = client;
> +
> +	if (of_property_read_bool(client->dev.of_node,
> +				"temperature-stability-7ppm"))
> +		data->div_offset = SI570_DIV_OFFSET_7PPM;
> +
> +	if (of_property_read_string(client->dev.of_node, "clock-output-names",
> +			&init.name))
> +		init.name = client->dev.of_node->name;
> +
> +	if (of_property_read_u32(client->dev.of_node, "factory-fout",
> +				&factory_fout)) {
> +		dev_warn(&client->dev,
> +			"DTS does not contain factory-fout, using default\n");

Is that really worth a warning ?

> +		factory_fout = ddata->default_fout;
> +	}
> +
> +	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
> +	 */
> +	if (!of_property_read_u32(client->dev.of_node, "initial-fout",
> +				&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);
> +	}
> +
> +	/* Display a message indicating that we've successfully registered */
> +	dev_info(&client->dev, "registered, current frequency %llu Hz\n",
> +			data->frequency);
> +
> +	return 0;
> +}
> +
> +static int si570_remove(struct i2c_client *client)
> +{
> +	struct clk_si570 *data = i2c_get_clientdata(client);
> +
> +	of_clk_del_provider(client->dev.of_node);
> +	clk_unregister(data->hw.clk);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id si570_id[] = {
> +	{ "si570", 0 },
> +	{ "si571", 0 },
> +	{ "si598", 1 },
> +	{ "si599", 1 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, si570_id);
> +
> +static struct i2c_driver si570_driver = {
> +	.driver = {
> +		.name	= "si570",
> +		.of_match_table = of_match_ptr(clk_si570_of_match),
> +	},
> +	.probe		= si570_probe,
> +	.remove		= si570_remove,
> +	.id_table	= si570_id,
> +};
> +module_i2c_driver(si570_driver);
> +
> +MODULE_AUTHOR("Guenter Roeck <guenter.roeck@ericsson.com>");
> +MODULE_AUTHOR("Soeren Brinkmann <soren.brinkmann@xilinx.com");
> +MODULE_DESCRIPTION("Si570 driver");
> +MODULE_LICENSE("GPL");
> -- 
> 1.8.4
> 
>

On Fri, Sep 13, 2013 at 10:00:05AM -0700, Guenter Roeck wrote:
> On Thu, Sep 12, 2013 at 05:55:37PM -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@roeck-us.net>
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > ---
> >  .../devicetree/bindings/clock/silabs,si570.txt     |  31 ++
> >  drivers/clk/Kconfig                                |  10 +
> >  drivers/clk/Makefile                               |   1 +
> >  drivers/clk/clk-si570.c                            | 546 +++++++++++++++++++++
> >  4 files changed, 588 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/silabs,si570.txt
> >  create mode 100644 drivers/clk/clk-si570.c
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt
> > new file mode 100644
> > index 0000000..099f0ee
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/silabs,si570.txt
> > @@ -0,0 +1,31 @@
> > +Binding for Silicon Labs 570, 571, 598 and 599  programmable
> > +I2C clock generators.
> > +
> > +Reference
> > +This binding uses the common clock binding[1]. Details about the devices can be
> > +found in the data sheets[2][3].
> > +
> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +[2] Si57x Data Sheet
> > +    http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf
> > +[3] Si59x Data Sheet
> > +    http://www.silabs.com/Support%20Documents/TechnicalDocs/si598-99.pdf
> > +
> > +Required properties:
> > + - compatible: Shall be one of "silabs,si57x", "silabs,si59x".
> 
> This should probably explicitly list all supported devices, not "x".
> After all, there might at some point be an incompatible device which
> still matches the "x".
Okay, I'll make it explicitly list 570, 571, 598 and 599.

> 
> > + - reg: I2C device address.
> > + - #clock-cells: From common clock bindings: Shall be 0.
> > + - factory-fout: Factory set default frequency
> > +
> > +Optional properties:
> > + - initial-fout: Initial output frequency to set during probe
> > + - temperature-stability-7ppm: Indicate a device with a temperature stability
> > +			       of 7ppm
> > +
> > +Example:
> > +	si570: clock-generator@5d {
> > +		#clock-cells = <0>;
> > +		compatible = "silabs,si570";
> > +		reg = <0x5d>;
> > +		factory-fout = <156250000>;
> > +	};
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 279407a..f5afabc 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -64,6 +64,16 @@ config COMMON_CLK_SI5351
> >  	  This driver supports Silicon Labs 5351A/B/C programmable clock
> >  	  generators.
> >  
> > +config COMMON_CLK_SI570
> > +	tristate "Clock driver for SiLabs 57x/59x"
> 
> Better state something like "SiLabs Si570 and compatible chips"
Sounds good to me. I'll change this.

> 
> > +	depends on I2C
> > +	depends on OF
> > +	select REGMAP_I2C
> > +	help
> > +	---help---
> > +	  This driver supports Silicon Labs 570/571/598/599 programmable
> > +	  clock generators.
> > +
> >  config COMMON_CLK_S2MPS11
> >  	tristate "Clock driver for S2MPS11 MFD"
> >  	depends on MFD_SEC_CORE
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 7b11106..c0e94b3 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -40,6 +40,7 @@ obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o
> >  obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
> >  obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
> >  obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o
> > +obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o
> >  obj-$(CONFIG_COMMON_CLK_S2MPS11) += clk-s2mps11.o
> >  obj-$(CONFIG_CLK_TWL6040)	+= clk-twl6040.o
> >  obj-$(CONFIG_CLK_PPC_CORENET)	+= clk-ppc-corenet.o
> > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c
> > new file mode 100644
> > index 0000000..960d689
> > --- /dev/null
> > +++ b/drivers/clk/clk-si570.c
> > @@ -0,0 +1,546 @@
> > +/*
> > + * Driver for Silicon Labs Si570/Si571 Programmable XO/VCXO
> > + *
> > + * Copyright (C) 2010, 2011 Ericsson AB.
> > + * Copyright (C) 2011 Guenter Roeck.
> > + * Copyright (C) 2011 - 2013 Xilinx Inc.
> > + *
> > + * Author: Guenter Roeck <guenter.roeck@ericsson.com>
> > + *	   Sören Brinkmann <soren.brinkmann@xilinx.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +/* Si570 registers */
> > +#define SI570_REG_HS_N1		7
> > +#define SI570_REG_N1_RFREQ0	8
> > +#define SI570_REG_RFREQ1	9
> > +#define SI570_REG_RFREQ2	10
> > +#define SI570_REG_RFREQ3	11
> > +#define SI570_REG_RFREQ4	12
> > +#define SI570_REG_CONTROL	135
> > +#define SI570_REG_FREEZE_DCO	137
> > +#define SI570_DIV_OFFSET_7PPM	6
> > +
> > +#define HS_DIV_SHIFT		5
> > +#define HS_DIV_MASK		0xe0
> > +#define HS_DIV_OFFSET		4
> > +#define N1_6_2_MASK		0x1f
> > +#define N1_1_0_MASK		0xc0
> > +#define RFREQ_37_32_MASK	0x3f
> > +
> > +#define SI570_FOUT_FACTORY_DFLT	156250000LL
> > +#define SI598_FOUT_FACTORY_DFLT	10000000LL
> > +
> > +#define SI570_MIN_FREQ		10000000L
> > +#define SI570_MAX_FREQ		1417500000L
> > +#define SI598_MAX_FREQ		525000000L
> > +
> > +#define FDCO_MIN		4850000000LL
> > +#define FDCO_MAX		5670000000LL
> > +
> > +#define SI570_CNTRL_RECALL	(1 << 0)
> > +#define SI570_CNTRL_FREEZE_M	(1 << 5)
> > +#define SI570_CNTRL_NEWFREQ	(1 << 6)
> > +
> > +#define SI570_FREEZE_DCO	(1 << 4)
> > +
> > +/**
> > + * struct clk_si570:
> > + * @hw:	Clock hw struct
> > + * @regmap:	Device's regmap
> > + * @div_offset:	Rgister offset for dividers
> > + * @max_freq:	Maximum frequency for this device
> > + * @fxtal:	Factory xtal frequency
> > + * @n1:		Clock divider N1
> > + * @hs_div:	Clock divider HSDIV
> > + * @rfreq:	Clock multiplier RFREQ
> > + * @frequency:	Current output frequency
> > + * @i2c_client:	I2C client pointer
> > + */
> > +struct clk_si570 {
> > +	struct clk_hw hw;
> > +	struct regmap *regmap;
> > +	unsigned int div_offset;
> > +	u64 max_freq;
> > +	u64 fxtal;
> > +	unsigned int n1;
> > +	unsigned int hs_div;
> > +	u64 rfreq;
> > +	u64 frequency;
> > +	struct i2c_client *i2c_client;
> > +};
> > +#define to_clk_si570(_hw)	container_of(_hw, struct clk_si570, hw)
> > +
> > +/**
> > + * struct si570_device_data:
> > + * @max_freq:	Maximum output frequency
> > + * @default_fout:	Default factory output frequency value
> > + */
> > +struct si570_device_data {
> > +	u64 max_freq;
> > +	u32 default_fout;
> > +};
> > +
> > +static const struct si570_device_data si57x_ddata = {
> > +	.max_freq = SI570_MAX_FREQ,
> > +	.default_fout = SI570_FOUT_FACTORY_DFLT
> > +};
> > +
> > +static const struct si570_device_data si59x_ddata = {
> > +	.max_freq = SI598_MAX_FREQ,
> > +	.default_fout = SI598_FOUT_FACTORY_DFLT
> > +};
> > +
> > +/**
> > + * si570_get_divs() - Read clock dividers from HW
> > + * @data:	Pointer to struct clk_si570
> > + * @rfreq:	Fractional multiplier (output)
> > + * @n1:		Divider N1 (output)
> > + * @hs_div:	Divider HSDIV (output)
> > + * Returns 0 on success, negative errno otherwise.
> > + *
> > + * Retrieve clock dividers and multipliers from the HW.
> > + */
> > +static int si570_get_divs(struct clk_si570 *data, u64 *rfreq,
> > +		unsigned int *n1, unsigned int *hs_div)
> 
> I would suggest to align continuation lines with the opening (,
> though that is really up to you (and the maintainer).
Hmm, I don't really like that since it is high maintenance and is easily
messed up by search and replace operations and similar. Therefore I rely
on vim's smart indent.

> 
> > +{
> > +	int err;
> > +	u8 reg[6];
> > +	u64 tmp;
> > +
> > +	err = regmap_bulk_read(data->regmap, SI570_REG_HS_N1 + data->div_offset,
> > +			reg, ARRAY_SIZE(reg));
> > +	if (err)
> > +		return err;
> > +
> > +	*hs_div = ((reg[0] & HS_DIV_MASK) >> HS_DIV_SHIFT) + HS_DIV_OFFSET;
> > +	*n1 = ((reg[0] & N1_6_2_MASK) << 2) + ((reg[1] & N1_1_0_MASK) >> 6) + 1;
> > +	/* Handle invalid cases */
> > +	if (*n1 > 1)
> > +		*n1 &= ~1;
> > +
> > +	tmp = reg[1] & RFREQ_37_32_MASK;
> > +	tmp = (tmp << 8) + reg[2];
> > +	tmp = (tmp << 8) + reg[3];
> > +	tmp = (tmp << 8) + reg[4];
> > +	tmp = (tmp << 8) + reg[5];
> > +	*rfreq = tmp;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * si570_get_defaults() - Get default values
> > + * @data:	Driver data structure
> > + * @fout:	Factory frequency output
> > + * Returns 0 on success, negative errno otherwise.
> > + */
> > +static int si570_get_defaults(struct clk_si570 *data, u64 fout)
> > +{
> > +	int err;
> > +	u64 fdco;
> > +
> > +	regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_RECALL);
> > +
> > +	err = si570_get_divs(data, &data->rfreq, &data->n1, &data->hs_div);
> > +	if (err)
> > +		return err;
> > +
> > +	/*
> > +	 * Accept optional precision loss to avoid arithmetic overflows.
> > +	 * Acceptable per Silicon Labs Application Note AN334.
> > +	 */
> > +	fdco = fout * data->n1 * data->hs_div;
> > +	if (fdco >= (1LL << 36))
> > +		data->fxtal = div64_u64(fdco << 24, data->rfreq >> 4);
> > +	else
> > +		data->fxtal = div64_u64(fdco << 28, data->rfreq);
> > +
> > +	data->frequency = fout;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * si570_update_rfreq() - Update clock multiplier
> > + * @data:	Driver data structure
> > + * Passes on regmap_bulk_write() return value.
> > + */
> > +static int si570_update_rfreq(struct clk_si570 *data)
> > +{
> > +	u8 reg[5];
> > +
> > +	reg[0] = ((data->n1 - 1) << 6) |
> > +		((data->rfreq >> 32) & RFREQ_37_32_MASK);
> > +	reg[1] = (data->rfreq >> 24) & 0xff;
> > +	reg[2] = (data->rfreq >> 16) & 0xff;
> > +	reg[3] = (data->rfreq >> 8) & 0xff;
> > +	reg[4] = data->rfreq & 0xff;
> > +
> > +	return regmap_bulk_write(data->regmap, SI570_REG_N1_RFREQ0 +
> > +			data->div_offset, reg, ARRAY_SIZE(reg));
> > +}
> > +
> > +/**
> > + * si570_calc_divs() - Caluclate clock dividers
> > + * @frequency:	Target frequency
> > + * @data:	Driver data structure
> > + * @out_rfreq:	RFREG fractional multiplier (output)
> > + * @out_n1:	Clock divider N1 (output)
> > + * @out_hs_div:	Clock divider HSDIV (output)
> > + * Returns 0 on success, negative errno otherwise.
> > + *
> > + * Calculate the clock dividers (@out_hs_div, @out_n1) and clock multiplier
> > + * (@out_rfreq) for a given target @frequency.
> > + */
> > +static int si570_calc_divs(unsigned long frequency, struct clk_si570 *data,
> > +		u64 *out_rfreq, unsigned int *out_n1, unsigned int *out_hs_div)
> > +{
> > +	int i;
> > +	unsigned int n1, hs_div;
> > +	u64 fdco, best_fdco = ULLONG_MAX;
> > +	static const uint8_t si570_hs_div_values[] = { 11, 9, 7, 6, 5, 4 };
> > +
> > +	for (i = 0; i < ARRAY_SIZE(si570_hs_div_values); i++) {
> > +		hs_div = si570_hs_div_values[i];
> > +		/* Calculate lowest possible value for n1 */
> > +		n1 = div_u64(div_u64(FDCO_MIN, hs_div), frequency);
> > +		if (!n1 || (n1 & 1))
> > +			n1++;
> > +		while (n1 <= 128) {
> > +			fdco = (u64)frequency * (u64)hs_div * (u64)n1;
> > +			if (fdco > FDCO_MAX)
> > +				break;
> > +			if (fdco >= FDCO_MIN && fdco < best_fdco) {
> > +				*out_n1 = n1;
> > +				*out_hs_div = hs_div;
> > +				*out_rfreq = div64_u64(fdco << 28, data->fxtal);
> > +				best_fdco = fdco;
> > +			}
> > +			n1 += (n1 == 1 ? 1 : 2);
> > +		}
> > +	}
> > +
> > +	if (best_fdco == ULLONG_MAX)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +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");
> > +		return data->frequency;
> > +	}
> > +
> > +	rfreq = div_u64(rfreq, hs_div * n1);
> > +	rate = (data->fxtal * rfreq) >> 28;
> > +
> > +	return rate;
> > +}
> > +
> > +static long si570_round_rate(struct clk_hw *hw, unsigned long rate,
> > +		unsigned long *parent_rate)
> > +{
> > +	int err;
> > +	u64 rfreq;
> > +	unsigned int n1, hs_div;
> > +	struct clk_si570 *data = to_clk_si570(hw);
> > +
> > +	if (!rate)
> > +		return 0;
> > +
> > +	if (div64_u64(abs(rate - data->frequency) * 10000LL,
> > +				data->frequency) < 35) {
> > +		rfreq = div64_u64((data->rfreq * rate) +
> > +				div64_u64(data->frequency, 2), data->frequency);
> > +		n1 = data->n1;
> > +		hs_div = data->hs_div;
> > +
> > +	} else {
> > +		err = si570_calc_divs(rate, data, &rfreq, &n1, &hs_div);
> > +		if (err) {
> > +			dev_err(&data->i2c_client->dev,
> > +					"unable to round rate\n");
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return rate;
> > +}
> > +
> > +/**
> > + * si570_set_frequency() - Adjust output frequency
> > + * @data:	Driver data structure
> > + * @frequency:	Target frequency
> > + * Returns 0 on success.
> > + *
> > + * Update output frequency for big frequency changes (> 3,500 ppm).
> > + */
> > +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency)
> > +{
> > +	int err;
> > +
> > +	err = si570_calc_divs(frequency, data, &data->rfreq, &data->n1,
> > +			&data->hs_div);
> > +	if (err)
> > +		return err;
> > +
> > +	/*
> > +	 * The DCO reg should be accessed with a read-modify-write operation
> > +	 * per AN334
> > +	 */
> > +	regmap_write(data->regmap, SI570_REG_FREEZE_DCO, SI570_FREEZE_DCO);
> > +	regmap_write(data->regmap, SI570_REG_HS_N1 + data->div_offset,
> > +				  ((data->hs_div - HS_DIV_OFFSET) <<
> > +				   HS_DIV_SHIFT)
> > +				  | (((data->n1 - 1) >> 2) & N1_6_2_MASK));
> 
> This is an example where the alignment to ( would really help.
> 
> 	regmap_write(data->regmap, SI570_REG_HS_N1 + data->div_offset,
> 		     ((data->hs_div - HS_DIV_OFFSET) << HS_DIV_SHIFT)
> 		     | (((data->n1 - 1) >> 2) & N1_6_2_MASK));
I agree on this one. I'll look into making this look a little prettier :)

> 
> > +	si570_update_rfreq(data);
> > +	regmap_write(data->regmap, SI570_REG_FREEZE_DCO, 0);
> > +	regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_NEWFREQ);
> > +
> > +	/* Applying a new frequency can take up to 10ms */
> > +	usleep_range(10000, 10001);
> 
> One microsecond range doesn't really buy anything besides forcing the compiler
> to generate extra code. Why not just use 10000 ?
That's due to checkpatch. I originally had 'msleep(10)' and checkpatch
suggested to use 'usleep_range()'. Then I put
'usleep_range(10000, 10000)' and checkpatch suggested to not use the
same values for MIN and MAX, so I added the 1 to the MAX value.
If it is considered to be safe and okay to put in MIN=MAX=10000, I'll
change it accordingly.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * si570_set_frequency_small() - Adjust output frequency
> > + * @data:	Driver data structure
> > + * @frequency:	Target frequency
> > + * Returns 0 on success.
> > + *
> > + * Update output frequency for small frequency changes (< 3,500 ppm).
> > + */
> > +static int si570_set_frequency_small(struct clk_si570 *data,
> > +				     unsigned long frequency)
> > +{
> > +	/*
> > +	 * This is a re-implementation of DIV_ROUND_CLOSEST
> > +	 * using the div64_u64 function lieu of letting the compiler
> > +	 * insert EABI calls
> > +	 */
> > +	data->rfreq = div64_u64((data->rfreq * frequency) +
> > +			div_u64(data->frequency, 2), data->frequency);
> > +	regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_FREEZE_M);
> > +	si570_update_rfreq(data);
> > +	regmap_write(data->regmap, SI570_REG_CONTROL, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +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);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (div64_u64(abs(rate - data->frequency) * 10000LL,
> > +				data->frequency) < 35)
> > +		err = si570_set_frequency_small(data, rate);
> > +	else
> > +		err = si570_set_frequency(data, rate);
> > +
> > +	if (err)
> > +		return err;
> > +
> > +	data->frequency = rate;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct clk_ops si570_clk_ops = {
> > +	.recalc_rate = si570_recalc_rate,
> > +	.round_rate = si570_round_rate,
> > +	.set_rate = si570_set_rate,
> > +};
> > +
> > +static const struct of_device_id clk_si570_of_match[] = {
> > +	{ .compatible = "silabs,si57x", .data = &si57x_ddata },
> > +	{ .compatible = "silabs,si59x", .data = &si59x_ddata },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, clk_si570_of_match);
> > +
> > +static bool si570_regmap_is_volatile(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case 135:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static bool si570_regmap_is_writeable(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case 7 ... 18:
> > +	case 135:
> > +	case 137:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static struct regmap_config si570_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.cache_type = REGCACHE_RBTREE,
> > +	.max_register = 137,
> > +	.writeable_reg = si570_regmap_is_writeable,
> > +	.volatile_reg = si570_regmap_is_volatile,
> > +};
> > +
> > +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;
> > +	const struct of_device_id *match;
> > +	const struct si570_device_data *ddata;
> > +
> > +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	match = of_match_node(clk_si570_of_match, client->dev.of_node);
> > +	if (!match)
> > +		return -EINVAL;
> 
> Seems unusual. Is this really needed ? It precludes the driver from being used
> in a non-devicetree environment, for example. I would guess that there is a match
> if client->dev.of_node is set. Otherwise, this code would be needed in every
> driver supporting devicetree, and I don't recall seeing that.
> 
> > +	ddata = match->data;
> > +
> You should be able to get this information (ie the pointer to si570_device_data)
> from id->driver_data. That would be more consistent with other i2c devices.
I think I copied this approach from the other clk-si... driver. I'll
do some research on your suggestion and change it. Could you point me to
an example for your proposal?

> 
> > +	init.ops = &si570_clk_ops;
> > +	init.flags = CLK_IS_ROOT;
> > +	init.num_parents = 0;
> > +	data->hw.init = &init;
> > +	data->max_freq = ddata->max_freq;
> > +	data->i2c_client = client;
> > +
> > +	if (of_property_read_bool(client->dev.of_node,
> > +				"temperature-stability-7ppm"))
> > +		data->div_offset = SI570_DIV_OFFSET_7PPM;
> > +
> > +	if (of_property_read_string(client->dev.of_node, "clock-output-names",
> > +			&init.name))
> > +		init.name = client->dev.of_node->name;
> > +
> > +	if (of_property_read_u32(client->dev.of_node, "factory-fout",
> > +				&factory_fout)) {
> > +		dev_warn(&client->dev,
> > +			"DTS does not contain factory-fout, using default\n");
> 
> Is that really worth a warning ?
My understanding is, that the default output frequency is part-specific
and required to calculate the frequency of the internal crystal. Hence,
if you do not provide the correct default output frequency for your part, all
frequencies generated by this driver will be off, unless the here used
default matches your part. Please correct me if I'm wrong, otherwise, I
think it's worth a warning.

	Sören

On Fri, Sep 13, 2013 at 10:26:04AM -0700, Sören Brinkmann wrote:
> On Fri, Sep 13, 2013 at 10:00:05AM -0700, Guenter Roeck wrote:
> > On Thu, Sep 12, 2013 at 05:55:37PM -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@roeck-us.net>
> > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > > ---
[ ... ]

> > > +	/* Applying a new frequency can take up to 10ms */
> > > +	usleep_range(10000, 10001);
> > 
> > One microsecond range doesn't really buy anything besides forcing the compiler
> > to generate extra code. Why not just use 10000 ?
> That's due to checkpatch. I originally had 'msleep(10)' and checkpatch
> suggested to use 'usleep_range()'. Then I put
> 'usleep_range(10000, 10000)' and checkpatch suggested to not use the
> same values for MIN and MAX, so I added the 1 to the MAX value.
> If it is considered to be safe and okay to put in MIN=MAX=10000, I'll
> change it accordingly.
> 
Guess what checkpatch means is to put in something resonable,
such as maybe (10000, 12000). (10000, 10001) just defeats checkpatch
which doesn't really provide any value. So either provide a reasonable
and acceptable range or use a single value.

[ ... ]

> > > +	match = of_match_node(clk_si570_of_match, client->dev.of_node);
> > > +	if (!match)
> > > +		return -EINVAL;
> > 
> > Seems unusual. Is this really needed ? It precludes the driver from being used
> > in a non-devicetree environment, for example. I would guess that there is a match
> > if client->dev.of_node is set. Otherwise, this code would be needed in every
> > driver supporting devicetree, and I don't recall seeing that.
> > 
> > > +	ddata = match->data;
> > > +
> > You should be able to get this information (ie the pointer to si570_device_data)
> > from id->driver_data. That would be more consistent with other i2c devices.
> I think I copied this approach from the other clk-si... driver. I'll
> do some research on your suggestion and change it. Could you point me to
> an example for your proposal?
> 
drivers/hwmon/lm90.c or drivers/hwmon/max6697.c.

> > > +
> > > +	if (of_property_read_u32(client->dev.of_node, "factory-fout",
> > > +				&factory_fout)) {
> > > +		dev_warn(&client->dev,
> > > +			"DTS does not contain factory-fout, using default\n");
> > 
> > Is that really worth a warning ?
> My understanding is, that the default output frequency is part-specific
> and required to calculate the frequency of the internal crystal. Hence,
> if you do not provide the correct default output frequency for your part, all
> frequencies generated by this driver will be off, unless the here used
> default matches your part. Please correct me if I'm wrong, otherwise, I
> think it's worth a warning.
> 
Maybe the property should be mandatory ?

Thanks,
Guenter

On Fri, Sep 13, 2013 at 12:48:22PM -0700, Guenter Roeck wrote:
> On Fri, Sep 13, 2013 at 10:26:04AM -0700, Sören Brinkmann wrote:
> > On Fri, Sep 13, 2013 at 10:00:05AM -0700, Guenter Roeck wrote:
> > > On Thu, Sep 12, 2013 at 05:55:37PM -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@roeck-us.net>
> > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > > > ---
> [ ... ]
> 
> > > > +	/* Applying a new frequency can take up to 10ms */
> > > > +	usleep_range(10000, 10001);
> > > 
> > > One microsecond range doesn't really buy anything besides forcing the compiler
> > > to generate extra code. Why not just use 10000 ?
> > That's due to checkpatch. I originally had 'msleep(10)' and checkpatch
> > suggested to use 'usleep_range()'. Then I put
> > 'usleep_range(10000, 10000)' and checkpatch suggested to not use the
> > same values for MIN and MAX, so I added the 1 to the MAX value.
> > If it is considered to be safe and okay to put in MIN=MAX=10000, I'll
> > change it accordingly.
> > 
> Guess what checkpatch means is to put in something resonable,
> such as maybe (10000, 12000). (10000, 10001) just defeats checkpatch
> which doesn't really provide any value. So either provide a reasonable
> and acceptable range or use a single value.
All right. I'll make it 10000 then, for MIN and MAX.

> 
> [ ... ]
> 
> > > > +	match = of_match_node(clk_si570_of_match, client->dev.of_node);
> > > > +	if (!match)
> > > > +		return -EINVAL;
> > > 
> > > Seems unusual. Is this really needed ? It precludes the driver from being used
> > > in a non-devicetree environment, for example. I would guess that there is a match
> > > if client->dev.of_node is set. Otherwise, this code would be needed in every
> > > driver supporting devicetree, and I don't recall seeing that.
> > > 
> > > > +	ddata = match->data;
> > > > +
> > > You should be able to get this information (ie the pointer to si570_device_data)
> > > from id->driver_data. That would be more consistent with other i2c devices.
> > I think I copied this approach from the other clk-si... driver. I'll
> > do some research on your suggestion and change it. Could you point me to
> > an example for your proposal?
> > 
> drivers/hwmon/lm90.c or drivers/hwmon/max6697.c.
Thanks.

> 
> > > > +
> > > > +	if (of_property_read_u32(client->dev.of_node, "factory-fout",
> > > > +				&factory_fout)) {
> > > > +		dev_warn(&client->dev,
> > > > +			"DTS does not contain factory-fout, using default\n");
> > > 
> > > Is that really worth a warning ?
> > My understanding is, that the default output frequency is part-specific
> > and required to calculate the frequency of the internal crystal. Hence,
> > if you do not provide the correct default output frequency for your part, all
> > frequencies generated by this driver will be off, unless the here used
> > default matches your part. Please correct me if I'm wrong, otherwise, I
> > think it's worth a warning.
> > 
> Maybe the property should be mandatory ?
It's actually listed as required property in the documentation. I'll
make it mandatory in the code as well and let probe() fail if it's not
provided.

	Sören

On Fri, Sep 13, 2013 at 02:05:49PM -0700, Sören Brinkmann wrote:
> On Fri, Sep 13, 2013 at 12:48:22PM -0700, Guenter Roeck wrote:
> > On Fri, Sep 13, 2013 at 10:26:04AM -0700, Sören Brinkmann wrote:
> > > On Fri, Sep 13, 2013 at 10:00:05AM -0700, Guenter Roeck wrote:
> > > > On Thu, Sep 12, 2013 at 05:55:37PM -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@roeck-us.net>
> > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > > > > ---
> > [ ... ]
> > 
> > > > > +	/* Applying a new frequency can take up to 10ms */
> > > > > +	usleep_range(10000, 10001);
> > > > 
> > > > One microsecond range doesn't really buy anything besides forcing the compiler
> > > > to generate extra code. Why not just use 10000 ?
> > > That's due to checkpatch. I originally had 'msleep(10)' and checkpatch
> > > suggested to use 'usleep_range()'. Then I put
> > > 'usleep_range(10000, 10000)' and checkpatch suggested to not use the
> > > same values for MIN and MAX, so I added the 1 to the MAX value.
> > > If it is considered to be safe and okay to put in MIN=MAX=10000, I'll
> > > change it accordingly.
> > > 
> > Guess what checkpatch means is to put in something resonable,
> > such as maybe (10000, 12000). (10000, 10001) just defeats checkpatch
> > which doesn't really provide any value. So either provide a reasonable
> > and acceptable range or use a single value.
> All right. I'll make it 10000 then, for MIN and MAX.
> 
> > 
> > [ ... ]
> > 
> > > > > +	match = of_match_node(clk_si570_of_match, client->dev.of_node);
> > > > > +	if (!match)
> > > > > +		return -EINVAL;
> > > > 
> > > > Seems unusual. Is this really needed ? It precludes the driver from being used
> > > > in a non-devicetree environment, for example. I would guess that there is a match
> > > > if client->dev.of_node is set. Otherwise, this code would be needed in every
> > > > driver supporting devicetree, and I don't recall seeing that.
> > > > 
> > > > > +	ddata = match->data;
> > > > > +
> > > > You should be able to get this information (ie the pointer to si570_device_data)
> > > > from id->driver_data. That would be more consistent with other i2c devices.
> > > I think I copied this approach from the other clk-si... driver. I'll
> > > do some research on your suggestion and change it. Could you point me to
> > > an example for your proposal?
> > > 
> > drivers/hwmon/lm90.c or drivers/hwmon/max6697.c.
> Thanks.
> 
> > 
> > > > > +
> > > > > +	if (of_property_read_u32(client->dev.of_node, "factory-fout",
> > > > > +				&factory_fout)) {
> > > > > +		dev_warn(&client->dev,
> > > > > +			"DTS does not contain factory-fout, using default\n");
> > > > 
> > > > Is that really worth a warning ?
> > > My understanding is, that the default output frequency is part-specific
> > > and required to calculate the frequency of the internal crystal. Hence,
> > > if you do not provide the correct default output frequency for your part, all
> > > frequencies generated by this driver will be off, unless the here used
> > > default matches your part. Please correct me if I'm wrong, otherwise, I
> > > think it's worth a warning.
> > > 
> > Maybe the property should be mandatory ?
> It's actually listed as required property in the documentation. I'll
> make it mandatory in the code as well and let probe() fail if it's not
> provided.
> 
Ok.

Thanks,
Guenter

On 09/13/2013 07:26 PM, Sören Brinkmann wrote:
> On Fri, Sep 13, 2013 at 10:00:05AM -0700, Guenter Roeck wrote:
>> On Thu, Sep 12, 2013 at 05:55:37PM -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@roeck-us.net>
>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>> ---
[...]
>>> diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c
>>> new file mode 100644
>>> index 0000000..960d689
>>> --- /dev/null
>>> +++ b/drivers/clk/clk-si570.c
>>> @@ -0,0 +1,546 @@
[...]
>>> +	match = of_match_node(clk_si570_of_match, client->dev.of_node);
>>> +	if (!match)
>>> +		return -EINVAL;
>>
>> Seems unusual. Is this really needed ? It precludes the driver from being used
>> in a non-devicetree environment, for example. I would guess that there is a match
>> if client->dev.of_node is set. Otherwise, this code would be needed in every
>> driver supporting devicetree, and I don't recall seeing that.
>>
>>> +	ddata = match->data;
>>> +
>> You should be able to get this information (ie the pointer to si570_device_data)
>> from id->driver_data. That would be more consistent with other i2c devices.
> I think I copied this approach from the other clk-si... driver. I'll
> do some research on your suggestion and change it. Could you point me to
> an example for your proposal?

Soeren,

I sent a patch for the match removal in clk-si5351 [1].
Mike must have missed it, I will resend soon.
You can take that as "an example for the proposal" above.

Sebastian

[1] https://lkml.org/lkml/2013/9/3/484

On 09/12/2013 06:55 PM, 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.

> diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt

> +Required properties:
> + - compatible: Shall be one of "silabs,si57x", "silabs,si59x".
> + - reg: I2C device address.
> + - #clock-cells: From common clock bindings: Shall be 0.
> + - factory-fout: Factory set default frequency

So, there's no way to query this from the device at all? Looking at the
data-sheet, all the frequency generation parameters are in registers,
and the device supports I2C read commands. As such, I'm not convinced
this property is necessary.

> +Optional properties:
> + - initial-fout: Initial output frequency to set during probe

"probe" is a Linux-specific concept. This property should be removed. If
the driver is asked to set a specific frequency, it should do so, but I
don't think it should program something pro-actively just because it
starts up.

If this property is acceptable, it'd be better to describe it more along
the lines of the following:

initial-fout: The frequency at which the system requires the clock to
operate.

> + - temperature-stability-7ppm: Indicate a device with a temperature stability
> +			       of 7ppm
> +
> +Example:
> +	si570: clock-generator@5d {
> +		#clock-cells = <0>;
> +		compatible = "silabs,si570";
> +		reg = <0x5d>;
> +		factory-fout = <156250000>;
> +	};

On Mon, Sep 16, 2013 at 10:34:28AM -0600, Stephen Warren wrote:
> On 09/12/2013 06:55 PM, 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.
> 
> > diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt
> 
> > +Required properties:
> > + - compatible: Shall be one of "silabs,si57x", "silabs,si59x".
> > + - reg: I2C device address.
> > + - #clock-cells: From common clock bindings: Shall be 0.
> > + - factory-fout: Factory set default frequency
> 
> So, there's no way to query this from the device at all? Looking at the
> data-sheet, all the frequency generation parameters are in registers,
> and the device supports I2C read commands. As such, I'm not convinced
> this property is necessary.
> 
Unfortunately, the chip does not report the factory setting for fout,
so the property is needed. The chip can not be programmed without it.

> > +Optional properties:
> > + - initial-fout: Initial output frequency to set during probe
> 
> "probe" is a Linux-specific concept. This property should be removed. If
> the driver is asked to set a specific frequency, it should do so, but I
> don't think it should program something pro-actively just because it
> starts up.
> 
> If this property is acceptable, it'd be better to describe it more along
> the lines of the following:
> 
> initial-fout: The frequency at which the system requires the clock to
> operate.
> 
It should probably be something like "clock-frequency". In many use cases
the programmed frequency is set to a constant frequency at system startup
and never changed, similar to other clocks.

Guenter

> > + - temperature-stability-7ppm: Indicate a device with a temperature stability
> > +			       of 7ppm
> > +
> > +Example:
> > +	si570: clock-generator@5d {
> > +		#clock-cells = <0>;
> > +		compatible = "silabs,si570";
> > +		reg = <0x5d>;
> > +		factory-fout = <156250000>;
> > +	};
> 
>

On 09/16/2013 10:49 AM, Guenter Roeck wrote:
> On Mon, Sep 16, 2013 at 10:34:28AM -0600, Stephen Warren wrote:
>> On 09/12/2013 06:55 PM, 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.
>>
>>> diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt
>>
>>> +Required properties:
>>> + - compatible: Shall be one of "silabs,si57x", "silabs,si59x".
>>> + - reg: I2C device address.
>>> + - #clock-cells: From common clock bindings: Shall be 0.
>>> + - factory-fout: Factory set default frequency
>>
>> So, there's no way to query this from the device at all? Looking at the
>> data-sheet, all the frequency generation parameters are in registers,
>> and the device supports I2C read commands. As such, I'm not convinced
>> this property is necessary.
>
> Unfortunately, the chip does not report the factory setting for fout,
> so the property is needed. The chip can not be programmed without it.

So fout is not the default overall output frequency, but rather
something internal that that feeds into the frequency generation
process, and the registers in the device only describe that frequency
generation process, not the frequency that feeds into it?

If so, it might be worth enhancing the binding documentation to briefly
describe that. Presumably the details are all in the HW documentation,
but it'd be nice if the binding doc was obviously correct without having
to fully understand the entire HW documentation.

>>> +Optional properties:
>>> + - initial-fout: Initial output frequency to set during probe
>>
>> "probe" is a Linux-specific concept. This property should be removed. If
>> the driver is asked to set a specific frequency, it should do so, but I
>> don't think it should program something pro-actively just because it
>> starts up.
>>
>> If this property is acceptable, it'd be better to describe it more along
>> the lines of the following:
>>
>> initial-fout: The frequency at which the system requires the clock to
>> operate.
>
> It should probably be something like "clock-frequency". In many use cases
> the programmed frequency is set to a constant frequency at system startup
> and never changed, similar to other clocks.

I was going to suggest that too, but re-considered since I think
clock-frequency is more appropriate for fixed-frequency clocks, rather
than to specify the value at which a programmable clock generator should
operate?

I don't think we have a good story yet for how to represent
how-we-want-the-clock-tree-configured, as opposed to representing the HW
itself (which is what DT should be more about).

On Mon, Sep 16, 2013 at 10:59:58AM -0600, Stephen Warren wrote:
> On 09/16/2013 10:49 AM, Guenter Roeck wrote:
> > On Mon, Sep 16, 2013 at 10:34:28AM -0600, Stephen Warren wrote:
> >> On 09/12/2013 06:55 PM, 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.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt
> >>
> >>> +Required properties:
> >>> + - compatible: Shall be one of "silabs,si57x", "silabs,si59x".
> >>> + - reg: I2C device address.
> >>> + - #clock-cells: From common clock bindings: Shall be 0.
> >>> + - factory-fout: Factory set default frequency
> >>
> >> So, there's no way to query this from the device at all? Looking at the
> >> data-sheet, all the frequency generation parameters are in registers,
> >> and the device supports I2C read commands. As such, I'm not convinced
> >> this property is necessary.
> >
> > Unfortunately, the chip does not report the factory setting for fout,
> > so the property is needed. The chip can not be programmed without it.
> 
> So fout is not the default overall output frequency, but rather
> something internal that that feeds into the frequency generation
> process, and the registers in the device only describe that frequency
> generation process, not the frequency that feeds into it?
> 
Here is what the datasheet has to say:

"The device's default output frequency is set at the
factory and can be reprogrammed through the two-wire
I2C serial port. Once the device is powered down, it will
return to its factory-set default output frequency."

So it is a default output frequency. However, the chip not report
what it is, and it can be programmed to other frequencies (and
commonly is). In reality it is a reference frequency which happens
to match the output frequency if all registers are at their
default (post-reset) setting.

Correct, the registers only describe the frequency generation process relative
to the reference frequency, so that frequency has to be known to calculate
the correct register values for other frequencies.

> If so, it might be worth enhancing the binding documentation to briefly
> describe that. Presumably the details are all in the HW documentation,
> but it'd be nice if the binding doc was obviously correct without having
> to fully understand the entire HW documentation.
> 
> >>> +Optional properties:
> >>> + - initial-fout: Initial output frequency to set during probe
> >>
> >> "probe" is a Linux-specific concept. This property should be removed. If
> >> the driver is asked to set a specific frequency, it should do so, but I
> >> don't think it should program something pro-actively just because it
> >> starts up.
> >>
> >> If this property is acceptable, it'd be better to describe it more along
> >> the lines of the following:
> >>
> >> initial-fout: The frequency at which the system requires the clock to
> >> operate.
> >
> > It should probably be something like "clock-frequency". In many use cases
> > the programmed frequency is set to a constant frequency at system startup
> > and never changed, similar to other clocks.
> 
> I was going to suggest that too, but re-considered since I think
> clock-frequency is more appropriate for fixed-frequency clocks, rather
> than to specify the value at which a programmable clock generator should
> operate?
> 
> I don't think we have a good story yet for how to represent
> how-we-want-the-clock-tree-configured, as opposed to representing the HW
> itself (which is what DT should be more about).
> 
In many cases the chip _is_ used to generate a fixed frequency, so we will
have to have a means to describe it. That it _can_ be used differently is a
different matter. After all, that is true for many clock generators.

Guenter

On 09/16/2013 11:35 AM, Guenter Roeck wrote:
> On Mon, Sep 16, 2013 at 10:59:58AM -0600, Stephen Warren wrote:
>> On 09/16/2013 10:49 AM, Guenter Roeck wrote:
>>> On Mon, Sep 16, 2013 at 10:34:28AM -0600, Stephen Warren wrote:
>>>> On 09/12/2013 06:55 PM, 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.
...
>>>>> +Optional properties:
>>>>> + - initial-fout: Initial output frequency to set during probe
>>>>
>>>> "probe" is a Linux-specific concept. This property should be removed. If
>>>> the driver is asked to set a specific frequency, it should do so, but I
>>>> don't think it should program something pro-actively just because it
>>>> starts up.
>>>>
>>>> If this property is acceptable, it'd be better to describe it more along
>>>> the lines of the following:
>>>>
>>>> initial-fout: The frequency at which the system requires the clock to
>>>> operate.
>>>
>>> It should probably be something like "clock-frequency". In many use cases
>>> the programmed frequency is set to a constant frequency at system startup
>>> and never changed, similar to other clocks.
>>
>> I was going to suggest that too, but re-considered since I think
>> clock-frequency is more appropriate for fixed-frequency clocks, rather
>> than to specify the value at which a programmable clock generator should
>> operate?
>>
>> I don't think we have a good story yet for how to represent
>> how-we-want-the-clock-tree-configured, as opposed to representing the HW
>> itself (which is what DT should be more about).
>
> In many cases the chip _is_ used to generate a fixed frequency, so we will
> have to have a means to describe it. That it _can_ be used differently is a
> different matter. After all, that is true for many clock generators.

Perhaps if clock-frequency is specified, the driver should refuse to
provide anything else. If clock-frequency isn't specified, the driver
shouldn't touch the HW when it initializes, but should honor any
requests that come in from other drivers? That would maintain what I
feel is clock-frequency's connection to being a fixed clock.

On Mon, Sep 16, 2013 at 12:37:43PM -0600, Stephen Warren wrote:
> On 09/16/2013 11:35 AM, Guenter Roeck wrote:
> > On Mon, Sep 16, 2013 at 10:59:58AM -0600, Stephen Warren wrote:
> >> On 09/16/2013 10:49 AM, Guenter Roeck wrote:
> >>> On Mon, Sep 16, 2013 at 10:34:28AM -0600, Stephen Warren wrote:
> >>>> On 09/12/2013 06:55 PM, 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.
> ...
> >>>>> +Optional properties:
> >>>>> + - initial-fout: Initial output frequency to set during probe
> >>>>
> >>>> "probe" is a Linux-specific concept. This property should be removed. If
> >>>> the driver is asked to set a specific frequency, it should do so, but I
> >>>> don't think it should program something pro-actively just because it
> >>>> starts up.
> >>>>
> >>>> If this property is acceptable, it'd be better to describe it more along
> >>>> the lines of the following:
> >>>>
> >>>> initial-fout: The frequency at which the system requires the clock to
> >>>> operate.
> >>>
> >>> It should probably be something like "clock-frequency". In many use cases
> >>> the programmed frequency is set to a constant frequency at system startup
> >>> and never changed, similar to other clocks.
> >>
> >> I was going to suggest that too, but re-considered since I think
> >> clock-frequency is more appropriate for fixed-frequency clocks, rather
> >> than to specify the value at which a programmable clock generator should
> >> operate?
> >>
> >> I don't think we have a good story yet for how to represent
> >> how-we-want-the-clock-tree-configured, as opposed to representing the HW
> >> itself (which is what DT should be more about).
> >
> > In many cases the chip _is_ used to generate a fixed frequency, so we will
> > have to have a means to describe it. That it _can_ be used differently is a
> > different matter. After all, that is true for many clock generators.
> 
> Perhaps if clock-frequency is specified, the driver should refuse to
> provide anything else. If clock-frequency isn't specified, the driver
> shouldn't touch the HW when it initializes, but should honor any
> requests that come in from other drivers? That would maintain what I
> feel is clock-frequency's connection to being a fixed clock.
> 
Ok with me, if that is in line with other clock drivers.

Guenter

On 09/16/2013 08:37 PM, Stephen Warren wrote:
> On 09/16/2013 11:35 AM, Guenter Roeck wrote:
>> On Mon, Sep 16, 2013 at 10:59:58AM -0600, Stephen Warren wrote:
>>> On 09/16/2013 10:49 AM, Guenter Roeck wrote:
>>>> On Mon, Sep 16, 2013 at 10:34:28AM -0600, Stephen Warren wrote:
>>>>> On 09/12/2013 06:55 PM, 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.
> ...
>>>>>> +Optional properties:
>>>>>> + - initial-fout: Initial output frequency to set during probe
>>>>>
>>>>> "probe" is a Linux-specific concept. This property should be removed. If
>>>>> the driver is asked to set a specific frequency, it should do so, but I
>>>>> don't think it should program something pro-actively just because it
>>>>> starts up.
>>>>>
>>>>> If this property is acceptable, it'd be better to describe it more along
>>>>> the lines of the following:
>>>>>
>>>>> initial-fout: The frequency at which the system requires the clock to
>>>>> operate.
>>>>
>>>> It should probably be something like "clock-frequency". In many use cases
>>>> the programmed frequency is set to a constant frequency at system startup
>>>> and never changed, similar to other clocks.
>>>
>>> I was going to suggest that too, but re-considered since I think
>>> clock-frequency is more appropriate for fixed-frequency clocks, rather
>>> than to specify the value at which a programmable clock generator should
>>> operate?
>>>
>>> I don't think we have a good story yet for how to represent
>>> how-we-want-the-clock-tree-configured, as opposed to representing the HW
>>> itself (which is what DT should be more about).
>>
>> In many cases the chip _is_ used to generate a fixed frequency, so we will
>> have to have a means to describe it. That it _can_ be used differently is a
>> different matter. After all, that is true for many clock generators.
>
> Perhaps if clock-frequency is specified, the driver should refuse to
> provide anything else. If clock-frequency isn't specified, the driver
> shouldn't touch the HW when it initializes, but should honor any
> requests that come in from other drivers? That would maintain what I
> feel is clock-frequency's connection to being a fixed clock.

For the clk-si5351 programmable clock driver in mainline, it already
uses "clock-frequency" for initial clock setup but allows to set it
later on. IMHO that is ok, because from a initial point-of-view, an
initial frequency is fixed. As soon as the driver takes over, the user
is free to do whatever he wants and should not be limited by DT.

But if we vote against that approach, we should probably also modifiy
clk-si5351 accordingly.

Sebastian

On 09/17/2013 12:59 AM, Sebastian Hesselbarth wrote:
> On 09/16/2013 08:37 PM, Stephen Warren wrote:
>> On 09/16/2013 11:35 AM, Guenter Roeck wrote:
>>> On Mon, Sep 16, 2013 at 10:59:58AM -0600, Stephen Warren wrote:
>>>> On 09/16/2013 10:49 AM, Guenter Roeck wrote:
>>>>> On Mon, Sep 16, 2013 at 10:34:28AM -0600, Stephen Warren wrote:
>>>>>> On 09/12/2013 06:55 PM, 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.
>> ...
>>>>>>> +Optional properties:
>>>>>>> + - initial-fout: Initial output frequency to set during probe
>>>>>>
>>>>>> "probe" is a Linux-specific concept. This property should be removed. If
>>>>>> the driver is asked to set a specific frequency, it should do so, but I
>>>>>> don't think it should program something pro-actively just because it
>>>>>> starts up.
>>>>>>
>>>>>> If this property is acceptable, it'd be better to describe it more along
>>>>>> the lines of the following:
>>>>>>
>>>>>> initial-fout: The frequency at which the system requires the clock to
>>>>>> operate.
>>>>>
>>>>> It should probably be something like "clock-frequency". In many use cases
>>>>> the programmed frequency is set to a constant frequency at system startup
>>>>> and never changed, similar to other clocks.
>>>>
>>>> I was going to suggest that too, but re-considered since I think
>>>> clock-frequency is more appropriate for fixed-frequency clocks, rather
>>>> than to specify the value at which a programmable clock generator should
>>>> operate?
>>>>
>>>> I don't think we have a good story yet for how to represent
>>>> how-we-want-the-clock-tree-configured, as opposed to representing the HW
>>>> itself (which is what DT should be more about).
>>>
>>> In many cases the chip _is_ used to generate a fixed frequency, so we will
>>> have to have a means to describe it. That it _can_ be used differently is a
>>> different matter. After all, that is true for many clock generators.
>>
>> Perhaps if clock-frequency is specified, the driver should refuse to
>> provide anything else. If clock-frequency isn't specified, the driver
>> shouldn't touch the HW when it initializes, but should honor any
>> requests that come in from other drivers? That would maintain what I
>> feel is clock-frequency's connection to being a fixed clock.
>
> For the clk-si5351 programmable clock driver in mainline, it already
> uses "clock-frequency" for initial clock setup but allows to set it
> later on. IMHO that is ok, because from a initial point-of-view, an
> initial frequency is fixed. As soon as the driver takes over, the user
> is free to do whatever he wants and should not be limited by DT.
>
> But if we vote against that approach, we should probably also modifiy
> clk-si5351 accordingly.
>

Not me; I am fine either way. Howeber, if there is a use case requiring both
it should be permitted, and if you ask me to vote I'll vote for being permissive.

Guenter

On 09/17/2013 01:59 AM, Sebastian Hesselbarth wrote:
> On 09/16/2013 08:37 PM, Stephen Warren wrote:
...
>> Perhaps if clock-frequency is specified, the driver should refuse to
>> provide anything else. If clock-frequency isn't specified, the driver
>> shouldn't touch the HW when it initializes, but should honor any
>> requests that come in from other drivers? That would maintain what I
>> feel is clock-frequency's connection to being a fixed clock.
> 
> For the clk-si5351 programmable clock driver in mainline, it already
> uses "clock-frequency" for initial clock setup but allows to set it
> later on. IMHO that is ok, because from a initial point-of-view, an
> initial frequency is fixed. As soon as the driver takes over, the user
> is free to do whatever he wants and should not be limited by DT.
> 
> But if we vote against that approach, we should probably also modifiy
> clk-si5351 accordingly.

I suppose that approach isn't unreasonable. So, if there's precedent for
it, this driver may as well follow it.

On Tue, Sep 17, 2013 at 10:14:01AM -0600, Stephen Warren wrote:
> On 09/17/2013 01:59 AM, Sebastian Hesselbarth wrote:
> > On 09/16/2013 08:37 PM, Stephen Warren wrote:
> ...
> >> Perhaps if clock-frequency is specified, the driver should refuse to
> >> provide anything else. If clock-frequency isn't specified, the driver
> >> shouldn't touch the HW when it initializes, but should honor any
> >> requests that come in from other drivers? That would maintain what I
> >> feel is clock-frequency's connection to being a fixed clock.
> > 
> > For the clk-si5351 programmable clock driver in mainline, it already
> > uses "clock-frequency" for initial clock setup but allows to set it
> > later on. IMHO that is ok, because from a initial point-of-view, an
> > initial frequency is fixed. As soon as the driver takes over, the user
> > is free to do whatever he wants and should not be limited by DT.
> > 
> > But if we vote against that approach, we should probably also modifiy
> > clk-si5351 accordingly.
> 
> I suppose that approach isn't unreasonable. So, if there's precedent for
> it, this driver may as well follow it.

All right, this sounds the most flexible to me and requires the least
changes as well :).
I think all I have to change for v2 in regards of this, is to rename the
property to 'clock-frequency'. I'll do that and send out v2 later today.

	Sören