Wolfgang Grandegger wrote: > Jean Delvare wrote: >> Hi Wolfgang, >> >> On Wed, 13 Aug 2008 12:14:29 +0200, Wolfgang Grandegger wrote: >>> Jean Delvare wrote: >>>> On Tue, 12 Aug 2008 11:28:54 +0200, Wolfgang Grandegger wrote: >>>>> Jean Delvare wrote: >>>>>> Hi Wolfgang, >>>>>> >>>>>> On Tue, 12 Aug 2008 10:55:58 +0200, Wolfgang Grandegger wrote: >>>>>>> We need support for the MON35W42 on an embedded MPC8544 board, which >>>>>>> seems to be compatible with the w83782d. There are more issues, e.g. the >>>>>>> early ISA bus probing of the existing driver hangs the system. Maybe i2c >>>>>>> probing should be done first!? >>>>>> If both interfaces are available, we want to use the ISA one, because >>>>>> it's much faster, so it must be registered first. >>>>>> >>>>>> What could be done though would be disabling the ISA interface >>>>>> altogether on architectures where it isn't supported (in particular >>>>>> PPC.) This shouldn't be too difficult. If you write a patch doing this, >>>>>> I'll be happy to review it. Affected drivers would be w83781d and lm78. >>>>> Some PPC have an ISA bus as well: >>>>> >>>>> http://lxr.linux.no/linux+v2.6.26.2/arch/powerpc/Kconfig#L494 >>>>> >>>>> I think selecting the ISA code with '#ifdef CONFIG_ISA' would be the >>>>> right solution? >>>> Agreed. >>> See the attached patch below. When I have some more free time, I will >>> convert the driver to new style as well. >> Weren't you supposed to work on top of my 2 patches? Please do, >> especially when my patches have an impact on the relation between the >> ISA device and potential I2C devices. > > Sorry, I forgot about them :-(. > >> Your patch breaks the build with CONFIG_ISA=y: >> >> CC [M] drivers/hwmon/w83781d.o >> drivers/hwmon/w83781d.c: In function `w83781d_read_value_i2c_isa': >> drivers/hwmon/w83781d.c:1668: error: `client' undeclared (first use in this function) >> drivers/hwmon/w83781d.c:1668: error: (Each undeclared identifier is reported only once >> drivers/hwmon/w83781d.c:1668: error: for each function it appears in.) >> drivers/hwmon/w83781d.c:1660: warning: unused variable `bank' >> drivers/hwmon/w83781d.c: In function `w83781d_write_value_i2c_isa': >> drivers/hwmon/w83781d.c:1694: warning: unused variable `bank' >> drivers/hwmon/w83781d.c:1695: warning: unused variable `cl' >> drivers/hwmon/w83781d.c: In function `w83781d_isa_add_driver': >> drivers/hwmon/w83781d.c:1967: error: `w83781d_read_value_isa' undeclared (first use in this function) >> drivers/hwmon/w83781d.c:1968: error: `w83781d_write_value_isa' undeclared (first use in this function) >> drivers/hwmon/w83781d.c: At top level: >> drivers/hwmon/w83781d.c:1659: warning: 'w83781d_read_value_i2c_isa' defined but not used >> drivers/hwmon/w83781d.c:1692: warning: 'w83781d_write_value_i2c_isa' defined but not used >> make[2]: *** [drivers/hwmon/w83781d.o] Error 1 >> make[1]: *** [drivers/hwmon] Error 2 >> make: *** [drivers] Error 2 > >> Apparently you didn't test that case - you really have to. > > I thought I did but... Anyway, I cannot really test the ISA interface > because I just have an embedded PowerPC board with a w83782. > >> I am surprised by the size of your patch. I expected something much >> smaller and less intrusive than that. As I understand it, you tried to >> optimize the case CONFIG_ISA=n. Did you check how much exactly this was >> saving? I guess not much. > > I mainly moved all ISA related code within one #ifdef ... #endif block > to avoild plenty of #ifdef's. Furthermore I splitted up the register > read and write function for i2c and isa and introduced function pointers > for w83781d_read_value and w83781d_write_value. > >> I don't think that going to this level of optimization is needed. Or, >> if you insist on doing it, that would be a separate patch. The first >> patch should really only move things around to group them inside the >> #ifdef/#endif, and skip the detection of the ISA device and >> registration of the platform driver. You shouldn't have to modify the >> actual driver code. > > I did not do any optimization saving code. > >>> (...) >>> +static int __devinit >>> +w83781d_isa_add_driver(void) >>> +{ >>> + int res; >>> >>> if (w83781d_isa_found(isa_address)) { >>> res = platform_driver_register(&w83781d_isa_driver); >>> if (res) >>> - goto exit_unreg_i2c_driver; >>> + goto exit; >>> >>> /* Sets global pdev as a side effect */ >>> res = w83781d_isa_device_add(isa_address); >>> if (res) >>> goto exit_unreg_isa_driver; >>> - } >>> >>> + /* Use ISA bus to access registers */ >>> + w83781d_read_value = w83781d_read_value_isa; >>> + w83781d_write_value = w83781d_write_value_isa; >> You can't do that. It is possible, and perfectly valid, to have two >> supported chips on the same system, one on the ISA bus and one on an >> I2C bus. I do have a system like that, with a W83781D on the >> motherboard's ISA bus and a W83783S on the graphics adapter. The bus >> type (and thus the register access method) is really a chip-specific >> and not driver-wide. > > I do not see a functional difference to the existing driver. If ISA > probing is successful, the device will be accessd with ISA bus, > otherwise via I2C bus. Did I miss something? Obviously I missed something :-(. The function pointers must be per device, of course. I'm going to prepare a patch with just #idef's added where necessary. A more comprehensive restructuring could be done when converting to new-style. Wolfgang.
