Re: aibs(4): ASUSTeK AI Booster (ACPI ATK0110) hardware monitor with limit support

[Constantine Aleksandrovich Murenin <C++%Cns.SU@localhost>]

2009/12/30 Constantine Aleksandrovich Murenin <C++%cns.su@localhost>:
> 2009/12/30 Jukka Ruohonen <jruohonen%iki.fi@localhost>:
>> 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.

[...]

>>> +             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...

Attached is the patch that addresses the comments.

BTW, has anyone encountered a -12V, or any negative voltage, in ATK0110?

Best regards,
Constantine.

From cc70bdd4c255b4912f81ecdffc34c561dd553684 Mon Sep 17 00:00:00 2001
From: Constantine A. Murenin <cnst+netbsd%bugmail.mojo.ru@localhost>
Date: Thu, 31 Dec 2009 06:46:24 -0500
Subject: [PATCH 24/24] use ACPI_INTEGER and PRIx64, and check success of 
sensor_attach; suggested by Jukka Ruohonen

---
 sys/dev/acpi/atk0110.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/sys/dev/acpi/atk0110.c b/sys/dev/acpi/atk0110.c
index 54c040a..4598c94 100644
--- a/sys/dev/acpi/atk0110.c
+++ b/sys/dev/acpi/atk0110.c
@@ -47,9 +47,9 @@ __KERNEL_RCSID(0, "$NetBSD$");
 
 struct aibs_sensor {
        envsys_data_t   s;
-       int64_t         i;
-       int64_t         l;
-       int64_t         h;
+       ACPI_INTEGER    i;
+       ACPI_INTEGER    l;
+       ACPI_INTEGER    h;
 };
 
 struct aibs_softc {
@@ -252,11 +252,14 @@ aibs_attach_sif(device_t self, enum envsys_units st)
                as[i].s.monitor = true;
 #endif
                aprint_verbose_dev(self, "%c%i: "
-                   "0x%08llx %20s %5lli / %5lli  0x%llx\n",
+                   "0x%08"PRIx64" %20s %5"PRIi64" / %5"PRIi64"  "
+                   "0x%"PRIx64"\n",
                    name[0], i,
-                   as[i].i, as[i].s.desc, as[i].l, as[i].h,
+                   as[i].i, as[i].s.desc, (int64_t)as[i].l, (int64_t)as[i].h,
                    oi[4].Integer.Value);
-               sysmon_envsys_sensor_attach(sc->sc_sme, &as[i].s);
+               if (sysmon_envsys_sensor_attach(sc->sc_sme, &as[i].s))
+                       aprint_error_dev(self, "%c%i: unable to attach\n",
+                           name[0], i);
        }
 
        AcpiOsFree(b.Pointer);
@@ -292,8 +295,8 @@ aibs_refresh(struct sysmon_envsys *sme, envsys_data_t 
*edata)
        int                     i;
        const char              *name;
        struct aibs_sensor      *as;
-       int64_t                 v;
-       int64_t                 l, h;
+       ACPI_INTEGER            v;
+       ACPI_INTEGER            l, h;
 
        switch (st) {
        case ENVSYS_STEMP:
@@ -398,7 +401,7 @@ aibs_get_limits(struct sysmon_envsys *sme, envsys_data_t 
*edata,
        enum envsys_units       st = s->units;
        int                     i;
        struct aibs_sensor      *as;
-       int64_t                 l, h;
+       ACPI_INTEGER            l, h;
 
        switch (st) {
        case ENVSYS_STEMP:
-- 
1.6.4