2009/12/30 Jukka Ruohonen <[email protected]>: > On Wed, Dec 30, 2009 at 03:49:35AM -0500, Constantine Aleksandrovich Murenin > wrote: >> >> Attached patch provides support for the hardware monitoring capabilities >> that are present in many modern desktop motherboards from ASUS featuring >> the ATK0110 ACPI device. > > Hi. Few small comments, all of them more or less trivial. > >> +struct aibs_sensor { >> + envsys_data_t s; >> + int64_t i; >> + int64_t l; >> + int64_t h; >> +}; > > The correct type for these would probably be uint64_t, given the relation to > the type ACPI_INTEGER, which AFAIR is an unsigned 64-bit integer (from ACPI > 2.0 onwards). Or maybe even plain ACPI_INTEGER?
Yes, this sounds good -- I'll try changing all of these to ACPI_INTEGER. >> + case ENVSYS_SVOLTS_DC: >> + name[0] = 'V'; >> + break; >> + default: >> + return; >> + } >> + >> + b.Length = ACPI_ALLOCATE_BUFFER; > > This could be ACPI_ALLOCATE_LOCAL_BUFFER. ACPI_ALLOCATE_LOCAL_BUFFER is not documented in the latest http://www.acpica.org/download/acpica-reference.pdf (Revision 1.26, from October 2009). Is there any principal difference between the two? >> + s = AcpiEvaluateObjectTyped(sc->sc_node->ad_handle, name, NULL, &b, >> + ACPI_TYPE_PACKAGE); >> + if (ACPI_FAILURE(s)) { >> + aprint_error_dev(self, "%s not found\n", name); >> + return; >> + } > > This is just a personal preference, but I would use acpi_eval_struct() and > verify the type manually, given that AcpiEvaluateObjectTyped() is just a > wrapper around AcpiEvaluateObject(). Maybe we should have something like > acpi_eval_struct_typed() though. > >> + bp = b.Pointer; >> + o = bp->Package.Elements; >> + if (o[0].Type != ACPI_TYPE_INTEGER) { >> + aprint_error_dev(self, "%s[0]: invalid type\n", name); >> + AcpiOsFree(b.Pointer); > > Use the ACPI_FREE macro instead of calling AcpiOsFree() directly. ACPICA Reference, Section 3.2.2.2, "ACPI Allocates Return Buffers", explicitly suggests the use of AcpiOsFree(). Is it wrong? >> + as = malloc(sizeof(*as) * n, M_DEVBUF, M_NOWAIT | M_ZERO); > > I reckon that there is no reason to prefer malloc(9) instead of kmem(9). I didn't use kmem(9) since that would require knowing the size of the structure on kmem_free(9). >> + for (i = 0, o++; i < n; i++, o++) { > > Would it be possible to use the utility function acpi_foreach_package_object() > here? It could clarify the loop, which I think is more readable in the > original aiboost(4). Actually, the original aiboost(4) also uses pointer arithmetics in a for loop similar to mine. :-) Thanks for pointing out this function, I'll take a further look if it'll be more useful. I'd prefer the driver to be as cross-portable as possible, though, and acpica functions seem like a natural choice for such a direction. >> + as[i].i = oi[0].Integer.Value; >> + strlcpy(as[i].s.desc, oi[1].String.Pointer, >> + sizeof(as[i].s.desc)); > > Is there a guarantee that "oi[1].String.Pointer" is not NULL? I'd certainly expect so, since NULL is not a string, and oi[1].Type is ACPI_TYPE_STRING. >> + aprint_verbose_dev(self, "%c%i: " >> + "0x%08llx %20s %5lli / %5lli 0x%llx\n", >> + name[0], i, >> + as[i].i, as[i].s.desc, as[i].l, as[i].h, >> + oi[4].Integer.Value); > > These should probably be %"PRId64" (or %"PRIx64" with uint64_t). Sure, looks good... Actually, what's the purpose of this? Is sizeof(unsigned long long) greater than sizeof(uint64_t) on amd64? If not, then why do you have to change llx to lx? :/ >> + sysmon_envsys_sensor_attach(sc->sc_sme, &as[i].s); >> + } > > The return value from sysmon_envsys_attach() could be validated, as there is > no proof what is in "oi[1].String.Pointer" (i.e. could it be empty string or > duplicate or something similar due buggy firmware?). Yes, sure, overlooked that during the porting... >> +static int >> +aibs_detach(device_t self, int flags) >> +{ >> + struct aibs_softc *sc = device_private(self); >> + >> + sysmon_envsys_unregister(sc->sc_sme); >> + if (sc->sc_asens_volt != NULL) >> + free(sc->sc_asens_volt, M_DEVBUF); >> + if (sc->sc_asens_temp != NULL) >> + free(sc->sc_asens_temp, M_DEVBUF); >> + if (sc->sc_asens_fan != NULL) >> + free(sc->sc_asens_fan, M_DEVBUF); >> + return 0; >> +} > > I wonder if the detach routine is needed at all? As David pointed out, for drvctl -d. :-) C.
