From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiko Schocher <hs@denx.de>
Date: Wed, 31 Oct 2012 06:26:33 +0100
Subject: [U-Boot] [PATCH 1/2] tegra: i2c: Add function to know about
	current bus
In-Reply-To: <1351618133-14909-1-git-send-email-sjg@chromium.org>
References: <1351618133-14909-1-git-send-email-sjg@chromium.org>
Message-ID: <5090B689.3080006@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hello Simon,

On 30.10.2012 18:28, Simon Glass wrote:
> Rather than using a variable in various places, add a single function,
> tegra_i2c_get_bus(), which returns a pointer to information about a
> bus.
>
> This will make it easier to move to the new i2c framework.
>
> Signed-off-by: Simon Glass<sjg@chromium.org>
> ---
>   drivers/i2c/tegra_i2c.c |   78 ++++++++++++++++++++++++++++++++++++----------
>   1 files changed, 61 insertions(+), 17 deletions(-)

First question, did you tried the new i2c framework on some HW? Works it
for you?

> diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c
> index efc77fa..3e157f4 100644
> --- a/drivers/i2c/tegra_i2c.c
> +++ b/drivers/i2c/tegra_i2c.c
[...]
> @@ -302,18 +304,48 @@ static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len)
>   #error "Please enable device tree support to use this driver"
>   #endif
>
> +/**
> + * Check that a bus number is valid and return a pointer to it
> + *
> + * @param bus_num	Bus number to check / return
> + * @return pointer to bus, if valid, else NULL
> + */
> +static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num)
> +{
> +	struct i2c_bus *bus;
> +
> +	if (bus_num>= TEGRA_I2C_NUM_CONTROLLERS) {
> +		debug("%s: Invalid bus number %u\n", __func__, bus_num);
> +		return NULL;
> +	}
> +	bus =&i2c_controllers[bus_num];
> +	if (!bus->inited) {
> +		debug("%s: Bus %u not available\n", __func__, bus_num);
> +		return NULL;
> +	}
> +
> +	return bus;
> +}
> +

I added, as Stephen and you suggested, in the "struct i2c_adapter"
to the driver callbacks a new parameter "struct i2c_adapter *adap":

struct i2c_adapter {
         void            (*init)(struct i2c_adapter *adap, int speed,
                                 int slaveaddr);
         int             (*probe)(struct i2c_adapter *adap, uint8_t chip);
         int             (*read)(struct i2c_adapter *adap, uint8_t chip,
                                 uint addr, int alen, uint8_t *buffer,
                                 int len);
         int             (*write)(struct i2c_adapter *adap, uint8_t chip,
                                 uint addr, int alen, uint8_t *buffer,
                                 int len);
         uint            (*set_bus_speed)(struct i2c_adapter *adap,
                                 uint speed);
[...]

so the probe/read/write and set_bus_speed() functions gets now a
"struct i2c_adapter" pointer ... i2c driver internally, we need only
the "struct i2c_adapter", which the i2c core passes to the i2c driver.
So this function would look like now:

 > +static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap)
 > +{
 > +	struct i2c_bus *bus;
         ^^^^^^^^^^^^^^
         maybe a rename to "i2c_tegra_driver" would be good?
 > +	bus =&i2c_controllers[adap->hwadapnr];
 > +	if (!bus->inited) {
 > +		debug("%s: Bus %u not available\n", __func__, bus_num);
 > +		return NULL;
 > +	}
 > +
 > +	return bus;
 > +}

With this, we can get rid of i2c_bus_num in this driver.

Also the probe/read/write and set_bus_speed() functions needs to
be adapted. I can do this for you, and add this patchset to my
v2 post, if it is ok for you?

>   unsigned int i2c_get_bus_speed(void)

This function is not needed for the new framework!
[...]

>   int i2c_set_bus_speed(unsigned int speed)
>   {
> -	struct i2c_bus *i2c_bus;
> +	struct i2c_bus *bus;
>
> -	i2c_bus =&i2c_controllers[i2c_bus_num];
> -	i2c_bus->speed = speed;
> -	i2c_init_controller(i2c_bus);
> +	bus = tegra_i2c_get_bus(i2c_bus_num);
> +	if (!bus)
> +		return 0;
> +	bus->speed = speed;
> +	i2c_init_controller(bus);
>
>   	return 0;
>   }
> @@ -426,7 +458,7 @@ void i2c_init(int speed, int slaveaddr)
[...]

Rest looks good to me.

Thanks!

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