Hi Jean, Rudolf,
There's something wrong with the way the driver is registered. This
patch is UNUSABLE. But here's the progress I have so far. I might be
able to finish it, if time permits.
On 3/12/07, Jean Delvare <khali at linux-fr.org> wrote:
> I would have prefered a separate patch for that, now it makes you patch
> even larger (39 kB). Same goes for the help text updates in Kconfig.
After looking at it, the globals can only be removed when we move them
into the w83627ehf_data (which is referenced from dev->platform_data).
So we need to do it all at once (though, I agree, it would be nice to
do in two patches when that's a possibility).
> > struct w83627ehf_data {
> > - struct i2c_client client;
> > + int addr;
> > + enum kinds kind;
>
> You don't actually use "kind" anywhere.
>
> > +enum kinds { w83627ehf, w83627ehg, w83627dhg };
>
> Why define a separate kind for w83627ehg given that you treat it
> everywhere as a w83627ehf? I think it's convenient that the enum matches
> the prefixes used in every driver.
I removed static const char * sio_names and placed it in
w83627ehf_find(), where I marked it __initdata. Since I use it to
initialize sio_name, I'm not sure if I need to keep it the way it is,
as a separate array. If I embedded the static strings in the switch(),
would they still be __initdata?
At this point, it looks like I can merge w83627ehf_sio_data and
w83627ehf_data. This eliminates the duplicate sio_data->name ==
data->name, and sio_data->hm_base == data->addr, and saves a little
memory by only allocating one block instead of two.
Storing w83627ehf_data as pdev->dev.platform_data only makes sense as
long as the driver only has one instance. This is a FIXME that I'll
address once the deadlock below is fixed.
> Also, only two drivers pass the Super-I/O base address as a parameter
> to the superio functions right now (pc87360 and pc87427), and both pass
> it as the first parameter, while you pass it as the last. I'd like all
> drivers to use the same convention to avoid unnecessary confusion.
>
> (Ultimately, I think we want to share all this code between all our
> Super-I/O drivers to kill the redundancy and enforce the conventions.)
This is something I'm excited to see (someday). I reversed the order
of the superio_* functions, but I'd appreciate it if someone would
double-check me, since the arguments are all the same type... it would
be very easy to get the argument order wrong or miss a superio_inb()
somewhere.
> > 4) chip w83627ehg report its name in "name" attribute as w83627ehf
> > to preserve compatibility of libsensors
> > 4) changed the resource handling, platform device allocates the full 8byte
> > region, and report itself there with sio name. So in my case:
> >
> > 0290-0297 : W83627EHG
> > 0295-0296 : w83627ehf
> >
> > The datasheet mentions that the chip decodes just +5 and +6 but it also mentions
> > that last three bits belong to the chip. I must check if it will hoose acpi pnp
> > or not.
>
> This is a risky change. I know for a fact that some motherboards with a
> W83627HF reserve 0x295-0x296 as a PNP resource, so we must not ask for
> the whole 0x290-0x297 range at the device declaration level, otherwise
> the driver core will reject our request. The W83627EHF might be
> different but I would be surprised. As a matter of fact, see:
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=ada0c2f8fa087dc1dbc34e096c318739b1d6381a
>
> What you are doing here is pretty much the opposite of the fix Petr
> committed over a year ago. So I think you should back up this change.
>
> I suppose that the clean fix to this problem would be to use the pnp
> device when available instead of declaring our own platform device. But
> I'm not familiar with pnp so I don't really know, and I also prefer
> that we concentrate on killing i2c-isa for now. Further cleanups can
> happen later.
>
> As for using the Super-I/O chip name as the resource name, I considered
> it, but then realized it means we need to keep these name strings in
> memory forever. So I decided to use the driver name everywhere. But it
> doesn't really matter, do as you wish.
This is the code I changed to address this, please review:
+ memset(&res, 0, sizeof(res));
+ data->name = w83627ehf_device_names[data->kind];
+ res.name = data->name;
+ res.start = data->addr + REGION_OFFSET;
+ res.end = data->addr + REGION_OFFSET + REGION_LENGTH - 1;
+ res.flags = IORESOURCE_IO;
+ err = platform_device_add_resources(pdev, &res, 1);
> Ideally you should not access the Super-I/O config area past the
> initial detection in w83627ehf_find, so you shouldn't need to store
> this value in sio_data. But I agree this can be left for a future
> cleanup, it doesn't really belong to this patch. The f71805f driver
> does it right already, the w83627hf driver not yet.
I think it's an easy thing to do -- maybe this will be the next patch.
> The error path doesn't look quite right to me. You can't release the
> region before removing the sysfs files, it's not safe.
I reversed the order of these.
> I suggest adding warning messages when the base I/O isn't set and when
> we need to forcibly enable the device, as I did in the w83627hf driver.
> This will make support easier.
I'm not clear on which part of the w83627hf driver does this. I can't
find the patch in the mail archives that converts w83627hf to a
platform driver, but I think this is the section of code
(pre-platform-driver) that you're talking about: w83627hf.c lines
1399-1403:
/* Minimize conflicts with other winbond i2c-only clients... */
/* disable i2c subclients... how to disable main i2c client?? */
/* force i2c address to relatively uncommon address */
w83627hf_write_value(client, W83781D_REG_I2C_SUBADDR, 0x89);
w83627hf_write_value(client, W83781D_REG_I2C_ADDR, force_i2c);
Let's make the "forcibly enable the device" a separate patch.
Again, the attached patch deadlocks when the module is being removed.
The call trace is:
__sched_text_start
wait_for_completion
default_wake_function
klist_remove
__device_release_driver
device_release_driver
bus_remove_device
device_del
platform_device_del
platform_device_unregister
sensors_w83627ehf_ea_unreg_fn
sensors_w83627ehf_exit
What this means is that line 321 of drivers/base/dd.c:
klist_remove(&dev->knode_driver)
is deadlocking. I don't know what the code is doing incorrectly when
registering the driver.
Here is the patch, if you'd like to take a look.
David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: david-UNUSABLE-convert-to-platform.patch
Type: application/octet-stream
Size: 38832 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070329/8c63fb99/attachment.obj
