Patchwork [U-Boot,1/2] tegra: i2c: Add function to know about current bus

login
register
mail settings
Submitter Simon Glass
Date Oct. 30, 2012, 5:28 p.m.
Message ID <1351618133-14909-1-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/195569/
State Superseded
Delegated to: Heiko Schocher
Headers show

Comments

Simon Glass - Oct. 30, 2012, 5:28 p.m.
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(-)
Heiko Schocher - Oct. 31, 2012, 5:26 a.m.
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
Simon Glass - Nov. 5, 2012, 8:43 p.m.
Hi Heiko,

On Tue, Oct 30, 2012 at 10:26 PM, Heiko Schocher <hs@denx.de> wrote:
> 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?

Yes, it works fine. Tried on a seaboard.

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

Sounds good. Are you going to send a new patch?

>
>> +{
>> +     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?

Of course.

>
>>   unsigned int i2c_get_bus_speed(void)
>
>
> This function is not needed for the new framework!
> [...]

Yes, I thought I removed it in 2/2, perhaps not.

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

Will await your further patch, and I am ready to test. Thanks.

Regards,
Simon

>
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Heiko Schocher - Nov. 8, 2012, 7:05 a.m.
Hello Simon,

On 05.11.2012 21:43, Simon Glass wrote:
> Hi Heiko,
>
> On Tue, Oct 30, 2012 at 10:26 PM, Heiko Schocher<hs@denx.de>  wrote:
>> 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?
>
> Yes, it works fine. Tried on a seaboard.

Great, thanks for testing!

>>> 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
[...]
>> 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)
>
> Sounds good. Are you going to send a new patch?

Yes, want to do some more test ... and waiting for the result of the
vote, which direction we should go, see previous EMail ...

>>> +{
>>> +     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?
>
> Of course.

Ok, do this ...

>>>    unsigned int i2c_get_bus_speed(void)
>>
>>
>> This function is not needed for the new framework!
>> [...]
>
> Yes, I thought I removed it in 2/2, perhaps not.
>
>>
>>>    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!
>
> Will await your further patch, and I am ready to test. Thanks.

Thanks!

bye,
Heiko

Patch

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
@@ -262,7 +262,8 @@  exit:
 	return error;
 }
 
-static int tegra_i2c_write_data(u32 addr, u8 *data, u32 len)
+static int tegra_i2c_write_data(struct i2c_bus *bus, u32 addr, u8 *data,
+				u32 len)
 {
 	int error;
 	struct i2c_trans_info trans_info;
@@ -273,14 +274,15 @@  static int tegra_i2c_write_data(u32 addr, u8 *data, u32 len)
 	trans_info.num_bytes = len;
 	trans_info.is_10bit_address = 0;
 
-	error = send_recv_packets(&i2c_controllers[i2c_bus_num], &trans_info);
+	error = send_recv_packets(bus, &trans_info);
 	if (error)
 		debug("tegra_i2c_write_data: Error (%d) !!!\n", error);
 
 	return error;
 }
 
-static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len)
+static int tegra_i2c_read_data(struct i2c_bus *bus, u32 addr, u8 *data,
+			       u32 len)
 {
 	int error;
 	struct i2c_trans_info trans_info;
@@ -291,7 +293,7 @@  static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len)
 	trans_info.num_bytes = len;
 	trans_info.is_10bit_address = 0;
 
-	error = send_recv_packets(&i2c_controllers[i2c_bus_num], &trans_info);
+	error = send_recv_packets(bus, &trans_info);
 	if (error)
 		debug("tegra_i2c_read_data: Error (%d) !!!\n", error);
 
@@ -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;
+}
+
 unsigned int i2c_get_bus_speed(void)
 {
-	return i2c_controllers[i2c_bus_num].speed;
+	struct i2c_bus *bus;
+
+	bus = tegra_i2c_get_bus(i2c_bus_num);
+	if (!bus)
+		return 0;
+	return bus->speed;
 }
 
 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)
 }
 
 /* i2c write version without the register address */
-int i2c_write_data(uchar chip, uchar *buffer, int len)
+int i2c_write_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len)
 {
 	int rc;
 
@@ -438,7 +470,7 @@  int i2c_write_data(uchar chip, uchar *buffer, int len)
 	debug("\n");
 
 	/* Shift 7-bit address over for lower-level i2c functions */
-	rc = tegra_i2c_write_data(chip << 1, buffer, len);
+	rc = tegra_i2c_write_data(bus, chip << 1, buffer, len);
 	if (rc)
 		debug("i2c_write_data(): rc=%d\n", rc);
 
@@ -446,13 +478,13 @@  int i2c_write_data(uchar chip, uchar *buffer, int len)
 }
 
 /* i2c read version without the register address */
-int i2c_read_data(uchar chip, uchar *buffer, int len)
+int i2c_read_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len)
 {
 	int rc;
 
 	debug("inside i2c_read_data():\n");
 	/* Shift 7-bit address over for lower-level i2c functions */
-	rc = tegra_i2c_read_data(chip << 1, buffer, len);
+	rc = tegra_i2c_read_data(bus, chip << 1, buffer, len);
 	if (rc) {
 		debug("i2c_read_data(): rc=%d\n", rc);
 		return rc;
@@ -470,12 +502,16 @@  int i2c_read_data(uchar chip, uchar *buffer, int len)
 /* Probe to see if a chip is present. */
 int i2c_probe(uchar chip)
 {
+	struct i2c_bus *bus;
 	int rc;
 	uchar reg;
 
 	debug("i2c_probe: addr=0x%x\n", chip);
+	bus = tegra_i2c_get_bus(i2c_get_bus_num());
+	if (!bus)
+		return 1;
 	reg = 0;
-	rc = i2c_write_data(chip, &reg, 1);
+	rc = i2c_write_data(bus, chip, &reg, 1);
 	if (rc) {
 		debug("Error probing 0x%x.\n", chip);
 		return 1;
@@ -492,11 +528,15 @@  static int i2c_addr_ok(const uint addr, const int alen)
 /* Read bytes */
 int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 {
+	struct i2c_bus *bus;
 	uint offset;
 	int i;
 
 	debug("i2c_read: chip=0x%x, addr=0x%x, len=0x%x\n",
 				chip, addr, len);
+	bus = tegra_i2c_get_bus(i2c_bus_num);
+	if (!bus)
+		return 1;
 	if (!i2c_addr_ok(addr, alen)) {
 		debug("i2c_read: Bad address %x.%d.\n", addr, alen);
 		return 1;
@@ -508,13 +548,13 @@  int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 				data[alen - i - 1] =
 					(addr + offset) >> (8 * i);
 			}
-			if (i2c_write_data(chip, data, alen)) {
+			if (i2c_write_data(bus, chip, data, alen)) {
 				debug("i2c_read: error sending (0x%x)\n",
 					addr);
 				return 1;
 			}
 		}
-		if (i2c_read_data(chip, buffer + offset, 1)) {
+		if (i2c_read_data(bus, chip, buffer + offset, 1)) {
 			debug("i2c_read: error reading (0x%x)\n", addr);
 			return 1;
 		}
@@ -526,11 +566,15 @@  int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 /* Write bytes */
 int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 {
+	struct i2c_bus *bus;
 	uint offset;
 	int i;
 
 	debug("i2c_write: chip=0x%x, addr=0x%x, len=0x%x\n",
 				chip, addr, len);
+	bus = tegra_i2c_get_bus(i2c_bus_num);
+	if (!bus)
+		return 1;
 	if (!i2c_addr_ok(addr, alen)) {
 		debug("i2c_write: Bad address %x.%d.\n", addr, alen);
 		return 1;
@@ -540,7 +584,7 @@  int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 		for (i = 0; i < alen; i++)
 			data[alen - i - 1] = (addr + offset) >> (8 * i);
 		data[alen] = buffer[offset];
-		if (i2c_write_data(chip, data, alen + 1)) {
+		if (i2c_write_data(bus, chip, data, alen + 1)) {
 			debug("i2c_write: error sending (0x%x)\n", addr);
 			return 1;
 		}