Hi Charles,
I'm very sorry for the late answer, I had forgotten about this post.
> To handle the 2.4/2.6 different for the SiS adapter, there are extra
> sis elements (@pci_adapters_sis5595 @pci_adapters_sis645
> @pci_adapters_sis96x ~ line 844) and a new routine
> (adapter_pci_detection_sis_96x() ~ line 2134) which then pushes one or
> more of the new elements onto the @pci_adapters list as needed (called
> ~ line 2183). Line numbers are based on sensors-detect perl script
> from the lm-sensors package version 2.10.0.
>
> Since the w83781d is part of the @chip_ids list and not part of the
> @pci_adapters list (or checks), it seemed like trying to overuse the
> sis subroutine may not make sense. Thus, there seemed to be three
> choices for handling the w83791d:
>
> 1) Follow the general sis pattern and create two new entries for the
> w83791d - one for the 2.4 kernel and one for the 2.6 kernel, then
> create a routine to pushd the correct entry onto the @chip_ids list
> (similar to what the SiS is doing for the adapter, but for chips).
>
> 2) Create a routine that walks the @chip_ids list and modifies
> specific chip_id entries for earlier kernels as needed.
>
> 3) Special case the driver field to allow for a "driver-choice"
> (similar to "to-be-written" and "not-a-sensor" that's available now).
> When the "driver-choice" string is seen, then have a driver_choice
> field (a new field in a chip element) point to a subroutine to give
> you the actual driver name.
>
> 4) Other???
>
> I admit, Perl is not my native language so it's possible (4) is the
> right answer. So, before submitting an "official" patch, I prototyped
> choices (1) and (2) above and included them below for comment. If it's
> easier, I can turn one (or both) into a real patch for people to
> try...
>
> Preferences? Thoughts? Comments?
My preference goes to option 1 because that's the way we handle the SiS
SMBus case, and using the same mechanism everywhere will make it easier
to the reader. Your proposed implementation looks OK, with the minor
comments below:
> --- sensors-detect.000 Tue Apr 25 11:20:26 2006
> +++ sensors-detect Wed Apr 26 14:37:30 2006
> @@ -915,6 +915,24 @@
> w83791sd_detect vt1211_i2c_detect vt1211_alias_detect
> smsc47m192_detect);
>
> +# Special case chip information goes here and would be included in
> +# the chip_special_cases routine below
> +use vars qw($chip_ids_kern24_w83791d $chip_ids_kern26_w83791d);
"ids" doesn't make much sense here.
> +$chip_ids_kern24_w83791d = {
> + name => "Winbond W83791D",
> + driver => "w83781d",
> + i2c_addrs => [0x2c..0x2f],
> + i2c_detect => sub { w83781d_detect 7, @_},
Add a space before the closing curly brace.
> +};
> +
> +$chip_ids_kern26_w83791d = {
> + name => "Winbond W83791D",
> + driver => "w83791d",
> + i2c_addrs => [0x2c..0x2f],
> + i2c_detect => sub { w83781d_detect 7, @_},
> +};
Ditto.
> +
> +
> # This is a list of all recognized chips.
> # Each entry must have the following fields:
> # name: The full chip name
> @@ -1054,12 +1072,6 @@
> i2c_detect => sub { w83781d_detect 2, @_},
> } ,
> {
> - name => "Winbond W83791D",
> - driver => "w83781d",
> - i2c_addrs => [0x2c..0x2f],
> - i2c_detect => sub { w83781d_detect 7, @_},
> - },
> - {
> name => "Winbond W83792D",
> driver => "w83792d",
> i2c_addrs => [0x2c..0x2f],
> @@ -2844,6 +2856,20 @@
> # this program will surely confuse them. But we guarantee never to write to
> # any of these devices.
>
> +# This routine allows you to select which chips are optionally added to the
> +# chip detection list. The most common use is to allow for different chip
> +# detection/drivers based on different linux kernels
> +# This routine follows the pattern of the Sis adapter special cases
> +sub chip_special_cases
> +{
> + # Based on the kernel, add the appropriate chip structure to the
> + # chip_ids detection list
> + if (kernel_version_at_least(2,6,0)) {
> + push @chip_ids, $chip_ids_kern26_w83791d;
> + } elsif (kernel_version_at_least(2,4,0)) {
This second test is not needed, a simple "else" would do (we do no more
support kernels older than 2.4.10 anyway.)
> + push @chip_ids, $chip_ids_kern24_w83791d;
> + }
> +}
>
> # $_[0]: A reference to the file descriptor to access this chip.
> # We may assume an i2c_set_slave_addr was already done.
> @@ -5077,6 +5103,10 @@
> }
> }
>
> + # Before looking for chips, make sure any special case chips are
> + # added to the chip_ids list
> + chip_special_cases();
> +
> print "\n We are now going to do the adapter probings. Some adapters may ",
> "hang halfway\n",
> " through; we can't really help that. Also, some chips will be double ",
I would also suggest that you place the w83791d entries after the array
with all the other entries, rather than before (again to make it
similar to what we did for the SiS case.)
Care to submit a real patch for us to apply?
Also, I can't remember if I asked you already... Would you mind
becoming the maintainer for the driver you wrote/ported? It's
convenient for me when individual drivers have a separate maintainer so
as to lower my workload. If you accept, please send a patch adding an
entry into MAINTAINERS (into alphabetical order.) You may take a look
at the F71805F entry for an example.
Thanks,
--
Jean Delvare
