Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Diego Elio Pettenò]

Il giorno sab, 17/03/2012 alle 12.31 +0000, Grant Likely ha scritto:
> Really?  The config line above claims IT8761E support.  How similar are
> these devices?  Either this line needs to be more specifc, or the drivers
> need to be merged.

I'm not sure about IT8761E, and it seems it's only partially the case
from the code I read; it probably would be possible â Denis would you
mind taking a look?

It could possibly work to merge them. For what it's worth I've more or
less followed on the steps of the two watchdog timer drivers for IT87
series.

> use #define pr_fmt and pr_*() macros.  Better yet, use dev_{info,err,debug}()
> macros where possible.

Will do.

> Why "superio_" prefix here?  Are these functions that are cut-and-paste
> from another driver, or are they it87xx specific?  If they are it87xx specific
> then they should have the it87_ prefix.

They seem to be shared across a number of different Super I/O chips; in
particular though they come out of wdt_it87 (which interfaces in part
through the GPIO). As I said, and as Guenter already confirmed, it might
be the case to have an it87_sio driver for the Super I/O chip that
provides the basic interface for gpio, hwmon and wdt at the same time.

I'm just not sure if I can get through it alone, especially since
reading through the mfd drivers there seem to be those that simply
"earmark" the ports for other drivers, which can then be loaded, and
those that simply create everything in their own code, and I wouldn't
know which one of the two to use ...

Another reason to have a MFD driver would be to allow tweaking the few
more knobs that (at least IT8728F) expose, such as the ability to switch
a few pre-defined inputs and outputs to GPIO instead, and so on...

> No spinlock around the critical regions?  This is racy.

Okay I'll reintroduce that. I wonder though if it should be a ngpio/8
array of spinlocks or just one, but I guess it's fast enough it
shouldn't matter.

> If you're going to do a lookup table like this, then be explicit for
> the offsets:

Will do that.

-- 
Diego Elio Pettenà â Flameeyes
http://blog.flameeyes.eu/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/