[U-Boot] [PATCH v2 4/7] tegra: Add I2C driver

[Heiko Schocher]

Hello Simon,

Simon Glass wrote:
> From: Yen Lin <yelin at nvidia.com>
> 
> Add basic i2c driver for Tegra2 with 8- and 16-bit address support.
> The driver requires CONFIG_OF_CONTROL to obtain its configuration
> from the device tree.
>
> (Simon Glass: sjg at chromium.org modified for upstream)
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> Changes in v2:
> - Use DIV_ROUND_UP() instead of a home-grown macro
> - Tidy comment style
> - Change i2c array to static
> - Adjust definitions to fit new peripheral clock bindings
> - Remove i2c configuring using CONFIG (use fdt instead)

Why? Ah found it ... Hmm.. why we don't need the non OF
case?

> - Make i2c/dvc decision come from fdt
> - Use new fdtdec alias decode function
> - Simplify code in i2c_addr_ok()
> - Return an error if an unavailable i2c bus is selected
> 
>  arch/arm/include/asm/arch-tegra2/tegra2_i2c.h |  160 +++++++
>  drivers/i2c/Makefile                          |    1 +
>  drivers/i2c/tegra2_i2c.c                      |  551 +++++++++++++++++++++++++
>  include/fdtdec.h                              |    2 +
>  lib/fdtdec.c                                  |    2 +
>  5 files changed, 716 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/include/asm/arch-tegra2/tegra2_i2c.h
>  create mode 100644 drivers/i2c/tegra2_i2c.c
[...]
> diff --git a/drivers/i2c/tegra2_i2c.c b/drivers/i2c/tegra2_i2c.c
> new file mode 100644
> index 0000000..a7db714
> --- /dev/null
> +++ b/drivers/i2c/tegra2_i2c.c
> @@ -0,0 +1,551 @@
[...]
> +static void i2c_init_controller(struct i2c_bus *i2c_bus)
> +{
> +	/* TODO: Fix bug which makes us need to do this */
> +	clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_OSC,
> +			       i2c_bus->speed * (8 * 2 - 1));
                                                 Can you use here some defines?
What is (8 * 2 - 1) ?

> +#ifndef CONFIG_OF_CONTROL
> +#error "Please enable device tree support to use this driver"
> +#endif

Hmm.. see above question. Ok, if somebody need want to use this
driver without CONFIG_OF_CONTROL it must be added ...

> +/*
> + * Process a list of nodes, adding them to our list of I2C ports.
> + *
> + * @param blob		fdt blob
> + * @param node_list	list of nodes to process (any <=0 are ignored)
> + * @param count		number of nodes to process
> + * @param is_dvc	1 if these are DVC ports, 0 if standard I2C
> + * @return 0 if ok, -1 on error
> + */
> +static int process_nodes(const void *blob, int node_list[], int count,
> +			 int is_dvc)
> +{
> +	struct i2c_bus *i2c_bus;
> +	int i;
> +
> +	/* build the i2c_controllers[] for each controller */
> +	for (i = 0; i < count; i++) {
> +		int node = node_list[i];
> +
> +		if (node <= 0)
> +			continue;
> +
> +		i2c_bus = &i2c_controllers[i];
> +		i2c_bus->id = i;
> +
> +		if (i2c_get_config(blob, node, i2c_bus)) {
> +			printf("i2c_init_board: failed to decode bus %d\n", i);
> +			return -1;
> +		}
> +
> +		i2c_bus->is_dvc = is_dvc;
> +		if (is_dvc) {
> +			i2c_bus->control =
> +				&((struct dvc_ctlr *)i2c_bus->regs)->control;
> +		} else {
> +			i2c_bus->control = &i2c_bus->regs->control;
> +		}
> +		debug("%s: controller bus %d at %p, periph_id %d, speed %d: ",
> +		      is_dvc ? "dvc" : "i2c", i, i2c_bus->regs,
> +		      i2c_bus->periph_id, i2c_bus->speed);
> +		i2c_init_controller(i2c_bus);
> +		debug("ok\n");
> +		i2c_bus->inited = 1;
> +
> +		/* Mark position as used */
> +		node_list[i] = -1;
> +	}
> +
> +	return 0;
> +}
> +
> +int i2c_init_board(void)
> +{
> +	int node_list[CONFIG_SYS_MAX_I2C_BUS];
> +	const void *blob = gd->fdt_blob;
> +	int count;
> +
> +	/* First get the normal i2c ports */
> +	count = fdtdec_find_aliases_for_id(blob, "i2c",
> +			COMPAT_NVIDIA_TEGRA20_I2C, node_list,
> +			CONFIG_SYS_MAX_I2C_BUS);
> +	if (process_nodes(blob, node_list, count, 0))
> +		return -1;
> +
> +	/* Now look for dvc ports */
> +	count = fdtdec_add_aliases_for_id(blob, "i2c",
> +			COMPAT_NVIDIA_TEGRA20_DVC, node_list,
> +			CONFIG_SYS_MAX_I2C_BUS);
> +	if (process_nodes(blob, node_list, count, 1))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +void i2c_init(int speed, int slaveaddr)
> +{
> +	debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
> +}

Hmm... i2c_init is called to init the i2c subsystem ... you do nothing
here ... and use i2c_init_board for init the i2c bus, right?

But i2c_init_board is not called from the driver ... ah, you do this
in board code ... Ok ...

I think, you do this, because i2c_init is called very early, and
so processing fdt is slow?

[...]

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany