Hi Lu,
On Fri, 9 Oct 2009 14:07:41 +0000, Lu Zhihe wrote:
> Hi Jean,
>
> Thank you very much for your reply!
>
> 2009/10/8 Jean Delvare <khali@xxxxxxxxxxxx>:
> > Hi Lu,
> > Can you please elaborate about it? Don't you ever plan to get your
> > driver included in the kernel tree?
>
> I am glad to know you think it maybe worth to merge to the kernel
> tree. :-) Thank you again!
> Sure, if it can even be merged, I will try to improve it to meet the
> need. Tobias and I had work on this aim too.
OK, great. Then please send the latest version of you driver as a patch
on top of Linus' latest kernel, and I'll review it.
> > If your driver won't ever be merged, I admit I am not interested in
> > reviewing it. If the plan is to merge it, then I will review it.
> >
> > I can't test your code, I do not have any supporter piece of hardware,
> > sorry.
> >
> > I've looked at the intel-agp driver, which binds to the same PCI
> > devices. It's way too big and complex to resort to the same hack we
> > have in sis5595 and i2c-sis5595 for example. The intel-agp driver must
> > remain a "real" pci driver which binds to its devices, in particular so
> > that power management works OK.
> >
> > This leaves us with 3 alternatives.
> >
> > 1* Add hwmon support to the intel-agp driver directly. As this driver
> > is already somewhat big (77 kB) it would probably go unnoticed. The
> > hwmon part in your driver is probably about 350 lines of code.
> >
> > 2* Implement hwmon as a separate module, simply use pci_get_dev()
> > during initialization of that module, and if a device is found,
> > register a platform driver and instantiate a platform device for the
> > hwmon part. This is essentially what your implementation does,
> > except that your module registers the platform driver and device
> > regardless of the presence of a supported chip, which is bad.
> >
> > 3* Let intel-agp instantiate the platform device for hwmon, and only
> > register the driver in the separate module. This avoid some code
> > duplication, and we can have the hwmon module auto-load. Maybe the
> > best compromise code-wise, but possibly somewhat more difficult to
> > maintain.
> >
> > Note that driver auto-loading should also work with option #2, although
> > your implementation doesn't do it. It shouldn't be too hard to add.
>
> Excuse me, I don''t quite catch about "driver auto-loading". Do you
> meet gm965temp build-in, while not as a module?
> Can you give me more hint? And I will try to improve gm965temp. Thanks a lot!
No. I am referring to the fact that Linux kernel modules can be loaded
automatically when the supported hardware is present, thanks to udev.
Kernel drivers can come with a list of hardware is supports. When a
device is then instantiated in the kernel, and shows up in sysfs
(somewhere under /sys/devices), udev sees it. If the device declared a
modalias name, udev will test against the supported list provided by
all kernel drivers, and if any matches, the driver in question is
loaded. The idea is that the user no longer has to know or care about
what hardware he/she has: required drivers load automatically.
If you look at the intel-agp driver, the statement which makes
auto-loading possible is:
MODULE_DEVICE_TABLE(pci, agp_intel_pci_table);
This exports to user-space (udev) the list of devices in
agp_intel_pci_table. You can see it by running:
/sbin/modinfo intel-agp
These are all the "alias:" lines.
So, what I want is that your driver loads automatically too, just like
intel-agp. For this you need a MODULE_DEVICE_TABLE call or equivalent.
If you use options 1 or 2 above then you want a pci-based
MODULE_DEVICE_TABLE, exactly as in the intel-agp driver.
If you use option 3, then we probably want a platform-based module
alias.
Anyway, this can easily be added afterwards, when your driver is ready.
So if you don't understand that part, it doesn't matter much.
> > I don't have a strong opinion between these 3 choices, but I want the
> > driver to auto-load as needed.
>
> The updated sensor-detect patch based on svn code is as following:
> ---
>
> From: Lu Zhihe <tombowfly@xxxxxxxxx>
>
> Subject: [PATCH] sensors-detect add gm965temp detected for lm-sensors
>
> Signed-off-by: Lu Zhihe <tombowfly@xxxxxxxxx>
>
> ---
>
> --- prog/detect/sensors-detect.orig 2009-10-09 21:43:12.000000000 +0000
> +++ prog/detect/sensors-detect 2009-10-09 21:50:13.000000000 +0000
> @@ -147,6 +147,16 @@ $revision =~ s/ \([^()]*\)//;
> procid => "Intel SCH",
> driver => "i2c-isch",
> }, {
> + vendid => 0x8086,
> + devid => 0x2a00,
> + procid => "Intel GM965",
> + driver => "gm965temp",
> + }, {
> + vendid => 0x8086,
> + devid => 0x2a10,
> + procid => "Intel GME965",
> + driver => "gm965temp",
> + }, {
This part is wrong: these chips are not I2C/SMBus masters!
> vendid => 0x1106,
> devid => 0x3040,
> procid => "VIA Technologies VT82C586B Apollo ACPI",
> @@ -1860,6 +1870,10 @@ use vars qw(@cpu_ids);
> driver => "i5k_amb",
> detect => \&intel_amb_detect,
> }, {
> + name => "Intel GM/GME965 thermal sensor",
> + driver => "gm965temp",
> + detect => \&intel_gm965_detect,
> + }, {
> name => "VIA C7 thermal and voltage sensors",
> driver => "c7temp",
> detect => \&c7temp_detect,
> @@ -5301,6 +5315,15 @@ sub intel_amb_detect
> return;
> }
>
> +sub intel_gm965_detect
> +{
> + if ((exists $pci_list{'8086:2a00'}) || # Intel GM965
> + (exists $pci_list{'8086:2a10'})) { # Intel GME965
> + return 9;
> + }
> + return;
> +}
> +
This part is correct, however we want to add all the other chips
supported by your driver, right? Please send an updated patch with all
the chips. It's OK to have a single entry in @cpu_ids if you want, but
all PCI IDs must be listed in intel_gm965_detect().
Thanks,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
