David Hubbard wrote:
> Hi Jim,
hi David :-)
>
> This is a question about the w83627ehf driver, but it's nice and
> general, so I'm looking at the way you did the pc8736x_gpio driver.
> There's this line:
>
> static struct superio* gate;
>
> And the driver uses the global 'gate' to access the Super-I/O. Right
> now in the w83627ehf driver, there are several globals:
>
> /* The actual ISA address is read from Super-I/O configuration space */
> static unsigned short address;
>
> /*
> * Super-I/O constants and functions
> */
>
> static int REG; /* The register to read/write */
> static int VAL; /* The value to read/write */
>
those should go away I think, which is good. The uppercase gives me
heartburn.
>
> Later on in the file, there's another global:
>
> static struct i2c_driver w83627ehf_driver;
>
>
> Now, I'm not the most knowledgeable one about the I2C subsystem, but I
> believe that w83627ehf_driver is redeclared a little later with this:
>
> static struct i2c_driver w83627ehf_driver = {
> .driver = {
> .name = "w83627ehf",
> },
> .attach_adapter = w83627ehf_detect,
> .detach_client = w83627ehf_detach_client,
> };
>
>
I believe the 1st is a forward-decl, its there to make following refs to
it work.
$ grep -n w83627ehf_driver drivers/hwmon/w83627ehf.c
784:static struct i2c_driver w83627ehf_driver;
816: w83627ehf_driver.driver.name)) {
831: client->driver = &w83627ehf_driver;
904:static struct i2c_driver w83627ehf_driver = {
951: return i2c_isa_add_driver(&w83627ehf_driver);
956: i2c_isa_del_driver(&w83627ehf_driver);
> Jean mentioned that it might be a good idea to define .name at
> runtime, depending on whether a w83627ehf or a w83627dhg was detected.
> But the global stays, as part of the driver.
>
> In moving to a driver / device model, it's possible to put a struct
> superio inside struct w83627ehf_data, inside struct device, which is
> part of struct i2c_adapter. (If I understand the I2C structures...if
> not, I think this is on the right track.) While I'm at it, I can put
> unsigned short address in there too. Personally, I'd like to remove
> all the global variables. (But not the struct i2c_driver
> w83627ehf_driver, plus the sensor_device_attribute structs, because
> they are more like constants such as function callbacks.)
>
> So I'm sitting here thinking about it, and it seems it could be done
> that way. There are some parts I haven't worked out yet. Especially
> embedding struct superio inside struct device. Is that valid?
>
No. struct superio contains the lock itself, which must be shared by all
drivers using that superio-port. This precludes it being sucked into
private data.
WRT the placement of the static struct superio *gate,
pc8736x_gpio uses the gate-> during runtime, and therefore in a bunch of
functions.
In contrast, pc87360 needs the superio port only at initialization
(__init pc87360_find)
so it has its gate-> as an automatic/stack variable, which is released
Regarding the i2c & hwmon device remodeling, Im in need of an LDD3 re-read,
and a careful read of some of the newer (platform) drivers, vt1211 forex.
I was kinda hoping to watch someone else convert their driver, and learn
from their
mistakes rather than my own.
With Superio in the mix, my uncertainty goes up; maybe it too should
formally be
added to platform_driver (or parhaps isa_driver). Or maybe it should be
a sub-class
(is that even possible ?) Jean's been clear that he doesnt have the
time to review it,
which complicates things a bit since hwmon has many superio users, and
thus presents
a good context to evaluate superio-locks.
RERO (release early & often) suggests I push it to LKML, or (maybe
KMentors 1st),
but Im not a great juggler, and Jean wants separate alarms next,
and RERO doesnt say release prematurely :-O
OTOH, the superio-locks API might survive a platform conversion unchanged.
> Hmmm,
> David
>
hth, and thanks,
jimc
