Patchwork Simplify device enabling and initialization

login
register
about
Submitter Myles Watson
Date 2010-06-17 16:40:12
Message ID <AANLkTimDkFfjpRr1ZZCG0yDjGuVtwZweyYzrhzoxiOy0@mail.gmail.com>
Download mbox | patch
Permalink /patch/1526/
State Superseded
Headers show

Comments

Myles Watson - 2010-06-17 16:40:12
On Wed, Jun 16, 2010 at 6:22 PM, Ward Vandewege <ward@gnu.org> wrote:
> On Wed, Jun 16, 2010 at 02:50:42PM -0600, Myles Watson wrote:
>> This patch breaks the s2881, which was doing some odd acrobatics in
>> order to get a device initialized after its parent.  It should be an
>> easy fix to do it correctly now, but I don't have an s2881 to test on.
>>  Ward?
>
> Yep, I've got (the guts) of an s2881 lying on my desk here, and can test any
> patches you throw at me :)

Great.  Here are two patches.

The first one enables the driver exactly like it was before, only as a driver.

The second one tries to make it less board specific.  Putting the fan
settings in the device tree would complete that effort, I think.

Suggestions welcome.  Thanks for testing!

Signed-off-by: Myles Watson <mylesgw@gmail.com>

Thanks,
Myles
Ward Vandewege - 2010-06-21 02:11:42
Hi Myles,

Everything seems fine with either patch - but there are some differences in
the boot output.

I also ran the 'sensors' command.

Output here:

  http://ward.vandewege.net/coreboot/s2881/20100617-myles/

I ran 4 tests: stock r5635 (head), stock r5632 (revision prior to this
changeset), r5635 + patch 1 and r5635 + patch 2.

Let me know if you need anything else... 

Thanks,
Ward.
Myles Watson - 2010-06-21 11:59:52
On Sun, Jun 20, 2010 at 8:11 PM, Ward Vandewege <ward@gnu.org> wrote:
> Hi Myles,
>
> Everything seems fine with either patch - but there are some differences in
> the boot output.
>
> I also ran the 'sensors' command.
>
> Output here:
>
>  http://ward.vandewege.net/coreboot/s2881/20100617-myles/
>
> I ran 4 tests: stock r5635 (head), stock r5632 (revision prior to this
> changeset), r5635 + patch 1 and r5635 + patch 2.
Thanks for testing.

It looks like only 5632 has the "ADT7463 properly initialized"
message.  One problem is that patch 2 was meant to be applied after
patch 1, so the device didn't end up in the tree for that run.  I'll
have to think about why the only message from the new device with
patch 1 is "I2C: 00:d0 missing read_resources"  For some reason it
doesn't look like it got the correct ops.

I wonder why the temperature values look right in all cases.  Does it
need to be cold booted in order for the initialization to be needed?

Thanks,
Myles

Thanks,
Myles
Ward Vandewege - 2010-06-21 14:59:47
On Mon, Jun 21, 2010 at 05:59:52AM -0600, Myles Watson wrote:
> It looks like only 5632 has the "ADT7463 properly initialized"
> message.  One problem is that patch 2 was meant to be applied after
> patch 1, 

Ah, whoops, I was wondering if I was doing the right thing there...

> so the device didn't end up in the tree for that run.  I'll
> have to think about why the only message from the new device with
> patch 1 is "I2C: 00:d0 missing read_resources"  For some reason it
> doesn't look like it got the correct ops.
> 
> I wonder why the temperature values look right in all cases.  Does it
> need to be cold booted in order for the initialization to be needed?

All boots were cold.

Thanks,
Ward.
Myles Watson - 2010-06-21 15:17:54
On Mon, Jun 21, 2010 at 8:59 AM, Ward Vandewege <ward@gnu.org> wrote:
> On Mon, Jun 21, 2010 at 05:59:52AM -0600, Myles Watson wrote:
>> It looks like only 5632 has the "ADT7463 properly initialized"
>> message.  One problem is that patch 2 was meant to be applied after
>> patch 1,
>
> Ah, whoops, I was wondering if I was doing the right thing there...
I should have been clearer.

>> so the device didn't end up in the tree for that run.  I'll
>> have to think about why the only message from the new device with
>> patch 1 is "I2C: 00:d0 missing read_resources"  For some reason it
>> doesn't look like it got the correct ops.
>>
>> I wonder why the temperature values look right in all cases.  Does it
>> need to be cold booted in order for the initialization to be needed?
>
> All boots were cold.
OK.  Thanks for doing that.

I guess I don't know what I'm looking at in the sensors output to see
if the device initialization worked.

Thanks,
Myles

Patch

Index: svn/src/drivers/i2c/adt7463/adt7463.c
===================================================================
--- svn.orig/src/drivers/i2c/adt7463/adt7463.c
+++ svn/src/drivers/i2c/adt7463/adt7463.c
@@ -35,67 +35,56 @@ 
  */
 static void adt7463_init(device_t dev)
 {
-	device_t smbus_dev, adt7463;
-	struct device_path path;
+	device_t parent;
 	int result;
 
-	/* Find the SMBus controller (AMD-8111). */
-	smbus_dev = dev_find_device(0x1022, 0x746b, 0);
-	if (!smbus_dev)
-		die("SMBus controller not found\n");
-	printk(BIOS_DEBUG, "SMBus controller found\n");
-
-	/* Find the ADT7463 device. */
-	path.type = DEVICE_PATH_I2C;
-	path.i2c.device = 0x2d;
-	adt7463 = find_dev_path(smbus_dev->link_list, &path);
-	if (!adt7463)
-		die("ADT7463 not found\n");
-	printk(BIOS_DEBUG, "ADT7463 found\n");
+	/* Find the ADT7463's parent device. */
+	parent = dev->bus->dev;
+	printk(BIOS_DEBUG, "ADT7463 parent is %s\n", dev_path(parent));
 
 	/* Set all fans to 'Fastest Speed Calculated by All 3 Temperature
 	 * Channels Controls PWMx'.
 	 */
-	result = smbus_write_byte(adt7463, 0x5c, 0xc2);
-	result = smbus_write_byte(adt7463, 0x5d, 0xc2);
-	result = smbus_write_byte(adt7463, 0x5e, 0xc2);
+	result = smbus_write_byte(parent, 0x5c, 0xc2);
+	result = smbus_write_byte(parent, 0x5d, 0xc2);
+	result = smbus_write_byte(parent, 0x5e, 0xc2);
 
 	/* Make sure that our fans never stop when temp. falls below Tmin,
 	 * but rather keep going at minimum duty cycle (applies to automatic
 	 * fan control mode only).
 	 */
-	result = smbus_write_byte(adt7463, 0x62, 0xc0);
+	result = smbus_write_byte(parent, 0x62, 0xc0);
 
 	/* Set minimum PWM duty cycle to 25%, rather than the default 50%. */
-	result = smbus_write_byte(adt7463, 0x64, 0x40);
-	result = smbus_write_byte(adt7463, 0x65, 0x40);
-	result = smbus_write_byte(adt7463, 0x66, 0x40);
+	result = smbus_write_byte(parent, 0x64, 0x40);
+	result = smbus_write_byte(parent, 0x65, 0x40);
+	result = smbus_write_byte(parent, 0x66, 0x40);
 
 	/* Set Tmin to 55C, rather than the default 90C. Above this temperature
 	 * the fans will start blowing harder as temperature increases
 	 * (automatic mode only).
 	 */
-	result = smbus_write_byte(adt7463, 0x67, 0x37);
-	result = smbus_write_byte(adt7463, 0x68, 0x37);
-	result = smbus_write_byte(adt7463, 0x69, 0x37);
+	result = smbus_write_byte(parent, 0x67, 0x37);
+	result = smbus_write_byte(parent, 0x68, 0x37);
+	result = smbus_write_byte(parent, 0x69, 0x37);
 
 	/* Set THERM limit to 70C, rather than the default 100C.
 	 * The fans will kick in at 100% if the sensors reach this temperature,
 	 * (only in automatic mode, but supposedly even when hardware is
 	 * locked up). This is a failsafe measure.
 	 */
-	result = smbus_write_byte(adt7463, 0x6a, 0x46);
-	result = smbus_write_byte(adt7463, 0x6b, 0x46);
-	result = smbus_write_byte(adt7463, 0x6c, 0x46);
+	result = smbus_write_byte(parent, 0x6a, 0x46);
+	result = smbus_write_byte(parent, 0x6b, 0x46);
+	result = smbus_write_byte(parent, 0x6c, 0x46);
 
 	/* Remote temperature 1 offset (LSB == 0.25C). */
-	result = smbus_write_byte(adt7463, 0x70, 0x02);
+	result = smbus_write_byte(parent, 0x70, 0x02);
 
 	/* Remote temperature 2 offset (LSB == 0.25C). */
-	result = smbus_write_byte(adt7463, 0x72, 0x01);
+	result = smbus_write_byte(parent, 0x72, 0x01);
 
 	/* Set TACH measurements to normal (1/second). */
-	result = smbus_write_byte(adt7463, 0x78, 0xf0);
+	result = smbus_write_byte(parent, 0x78, 0xf0);
 
 	printk(BIOS_DEBUG, "ADT7463 properly initialized\n");
 }