Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 19 Oct 2009 12:17:22 +0100
From:      Rui Paulo <rpaulo@freebsd.org>
To:        Andriy Gapon <avg@icyb.net.ua>
Cc:        freebsd-hackers@freebsd.org, freebsd-acpi@freebsd.org
Subject:   Re: SB7xx watchdog: new driver for review and testing
Message-ID:  <5E0DD277-CAA3-4F01-8561-35CF6C511718@freebsd.org>
In-Reply-To: <4ADB764C.2010900@icyb.net.ua>
References:  <4ADB764C.2010900@icyb.net.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
On 18 Oct 2009, at 21:10, Andriy Gapon wrote:

> Please review and/or test a new driver for watchdog driver included  
> into AMD SB7xx:
> http://people.freebsd.org/~avg/amdsbwd.tgz
> I have tested this driver only with SB700 on Gigabyte GA-MA780G-UD3H  
> motherboard.
>
> ichwd driver was used as a starting point for this driver. This can  
> be seen from
> some function names, general code organization and some small code  
> snippets.
> Many thanks to ichwd authors and maintainers!
>
> Right now I have infrastructure only for building this driver as a  
> module.
>
> Things for which that I need the most feedback/ideas:
> 1. If the driver actually works on your hardware and the hardware  
> description.
> The driver can be tested by loading the driver and doing 'watchdog - 
> t <small
> number>'. Having debug.bootverbose=1 may provide additional useful  
> info.
> And better to test this from single-user mode with filesystems  
> mounted r/o.
>
> 2. Better name for the driver. amdsbwd stands for "AMD S(outh)B(ridge)
> WatchDog", but this abbreviation could be cryptic to decipher.
>
> 3. Proper location for this driver.
> At least on my system this driver needs resources (I/O ports and MEM  
> range) that
> are claimed by ACPI, thus I've made it a child of acpi bus. But this  
> driver
> doesn't have anything else ACPI-ish in it, so I decided that it  
> doesn't belong
> under acpica/ or acpi_support/. Am I correct about this?
>
> Anything else you would like to report or comment or advise to me.
> Thank you very much for your help.

The driver looks good in general. A few questions:
- Can you make the magic numbers a define ? Where did they come from ?
- Are you missing a device_set_desc() call ?
- If this is what you want to commit, C++ comments are not allowed per- 
style

Regards,
--
Rui Paulo




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5E0DD277-CAA3-4F01-8561-35CF6C511718>