Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
> > It saves about 20k in compiled size, so the savings from reduced > > boilerplate are huge. (and I think it's more readable) > > > >> I would suggest to drop nct6775/nct6776 support to simplify the > >> code when you do that. Maybe as separate commit, though. > > > > Hehe - I'm testing on a nct6776 :) > > I'll look into it once the first conversion has been accepted. > > Wondering - why don't you use the nct6775 driver ? Probably because it got matched by sensors-detect before the nct6775 driver (?) or because I hadn't had it turned on in my kernel config. Both drivers work, but interestingly, they show different values for intrusion1 (and fan alarm). nct6776.c: nct6776-isa-0290 Adapter: ISA adapter Vcore: +1.16 V (min = +0.00 V, max = +1.74 V) in1:+1.85 V (min = +0.00 V, max = +0.00 V) ALARM AVCC: +3.36 V (min = +2.98 V, max = +3.63 V) +3.3V: +3.36 V (min = +2.98 V, max = +3.63 V) in4:+0.26 V (min = +0.00 V, max = +0.00 V) ALARM in5:+1.67 V (min = +0.00 V, max = +0.00 V) ALARM in6:+0.79 V (min = +0.00 V, max = +0.00 V) ALARM 3VSB: +3.44 V (min = +2.98 V, max = +3.63 V) Vbat: +3.26 V (min = +2.70 V, max = +3.63 V) fan1: 0 RPM (min =0 RPM) fan2: 2106 RPM (min =0 RPM) fan3: 0 RPM (min =0 RPM) fan4: 0 RPM (min =0 RPM) fan5: 0 RPM (min =0 RPM) SYSTIN: +35.0°C (high = +0.0°C, hyst = +0.0°C) ALARM sensor = thermistor CPUTIN: +44.0°C (high = +80.0°C, hyst = +75.0°C) sensor = thermistor AUXTIN: +43.5°C (high = +80.0°C, hyst = +75.0°C) sensor = thermistor PECI Agent 0: +59.0°C (high = +80.0°C, hyst = +75.0°C) (crit = +105.0°C) PCH_CHIP_TEMP: +0.0°C PCH_CPU_TEMP:+0.0°C PCH_MCH_TEMP:+0.0°C intrusion0:ALARM intrusion1:OK beep_enable: disabled w83627ehf.c: nct6776-isa-0290 Adapter: ISA adapter Vcore: +1.14 V (min = +0.00 V, max = +1.74 V) in1: +1.85 V (min = +0.00 V, max = +0.00 V) ALARM AVCC: +3.36 V (min = +2.98 V, max = +3.63 V) +3.3V: +3.36 V (min = +2.98 V, max = +3.63 V) in4: +0.24 V (min = +0.00 V, max = +0.00 V) ALARM in5: +1.68 V (min = +0.00 V, max = +0.00 V) ALARM 3VSB: +3.44 V (min = +2.98 V, max = +3.63 V) Vbat: +3.26 V (min = +2.70 V, max = +3.63 V) fan1:0 RPM (min =0 RPM) ALARM fan2: 2142 RPM (min =0 RPM) ALARM fan3:0 RPM (min =0 RPM) ALARM fan4:0 RPM (min =0 RPM) ALARM fan5:0 RPM (min =0 RPM) ALARM SYSTIN:+35.0°C (high = +0.0°C, hyst = +0.0°C) ALARM sensor = thermistor CPUTIN:+46.0°C (high = +80.0°C, hyst = +75.0°C) sensor = thermistor AUXTIN:+44.0°C (high = +80.0°C, hyst = +75.0°C) sensor = thermistor PECI Agent 0: +64.0°C cpu0_vid: +0.000 V intrusion0: ALARM intrusion1: ALARM Once the other patches have been accepted I'll work on that conversion/removal of the nct6775 code from the w83627ehf. Peter
Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
> > It saves about 20k in compiled size, so the savings from reduced > > boilerplate are huge. (and I think it's more readable) > > > >> I would suggest to drop nct6775/nct6776 support to simplify the > >> code when you do that. Maybe as separate commit, though. > > > > Hehe - I'm testing on a nct6776 :) > > I'll look into it once the first conversion has been accepted. > > Wondering - why don't you use the nct6775 driver ? Probably because it got matched by sensors-detect before the nct6775 driver (?) or because I hadn't had it turned on in my kernel config. Both drivers work, but interestingly, they show different values for intrusion1 (and fan alarm). nct6776.c: nct6776-isa-0290 Adapter: ISA adapter Vcore: +1.16 V (min = +0.00 V, max = +1.74 V) in1:+1.85 V (min = +0.00 V, max = +0.00 V) ALARM AVCC: +3.36 V (min = +2.98 V, max = +3.63 V) +3.3V: +3.36 V (min = +2.98 V, max = +3.63 V) in4:+0.26 V (min = +0.00 V, max = +0.00 V) ALARM in5:+1.67 V (min = +0.00 V, max = +0.00 V) ALARM in6:+0.79 V (min = +0.00 V, max = +0.00 V) ALARM 3VSB: +3.44 V (min = +2.98 V, max = +3.63 V) Vbat: +3.26 V (min = +2.70 V, max = +3.63 V) fan1: 0 RPM (min =0 RPM) fan2: 2106 RPM (min =0 RPM) fan3: 0 RPM (min =0 RPM) fan4: 0 RPM (min =0 RPM) fan5: 0 RPM (min =0 RPM) SYSTIN: +35.0°C (high = +0.0°C, hyst = +0.0°C) ALARM sensor = thermistor CPUTIN: +44.0°C (high = +80.0°C, hyst = +75.0°C) sensor = thermistor AUXTIN: +43.5°C (high = +80.0°C, hyst = +75.0°C) sensor = thermistor PECI Agent 0: +59.0°C (high = +80.0°C, hyst = +75.0°C) (crit = +105.0°C) PCH_CHIP_TEMP: +0.0°C PCH_CPU_TEMP:+0.0°C PCH_MCH_TEMP:+0.0°C intrusion0:ALARM intrusion1:OK beep_enable: disabled w83627ehf.c: nct6776-isa-0290 Adapter: ISA adapter Vcore: +1.14 V (min = +0.00 V, max = +1.74 V) in1: +1.85 V (min = +0.00 V, max = +0.00 V) ALARM AVCC: +3.36 V (min = +2.98 V, max = +3.63 V) +3.3V: +3.36 V (min = +2.98 V, max = +3.63 V) in4: +0.24 V (min = +0.00 V, max = +0.00 V) ALARM in5: +1.68 V (min = +0.00 V, max = +0.00 V) ALARM 3VSB: +3.44 V (min = +2.98 V, max = +3.63 V) Vbat: +3.26 V (min = +2.70 V, max = +3.63 V) fan1:0 RPM (min =0 RPM) ALARM fan2: 2142 RPM (min =0 RPM) ALARM fan3:0 RPM (min =0 RPM) ALARM fan4:0 RPM (min =0 RPM) ALARM fan5:0 RPM (min =0 RPM) ALARM SYSTIN:+35.0°C (high = +0.0°C, hyst = +0.0°C) ALARM sensor = thermistor CPUTIN:+46.0°C (high = +80.0°C, hyst = +75.0°C) sensor = thermistor AUXTIN:+44.0°C (high = +80.0°C, hyst = +75.0°C) sensor = thermistor PECI Agent 0: +64.0°C cpu0_vid: +0.000 V intrusion0: ALARM intrusion1: ALARM Once the other patches have been accepted I'll work on that conversion/removal of the nct6775 code from the w83627ehf. Peter
Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
On 03/21/2017 03:46 AM, Peter Hüwe wrote: Hi On Friday 03 March 2017 03:56:01 Guenter Roeck wrote: Hi Peter, On 03/02/2017 04:33 PM, Peter Hüwe wrote: Hi, is anybody else working on the conversion of the w83627ehf to the new hwmon_device_register_with_info interface? I don't think so. Otherwise I will probably update the driver to this interface within the next days - but since it's a lot of work I wanted to check for duplication first. Go ahead. So I'm close to have to conversion done, the current diff stat is about 647 insertions(+), 787 deletions(-) for the whole conversion. Should I try to break it down into smaller chunks for easier review? Although this would mean to convert stuff from A to B and then from B to C - otherwise the intermediate steps would be not fully functional. (since the sysfs nodes partially would exist under hwmon1/ and partially under hwmon1/device/ (as they currently are)). or just submit it? Just submit it. I don't really think a conversion like this can be split up cleanly. It saves about 20k in compiled size, so the savings from reduced boilerplate are huge. (and I think it's more readable) I would suggest to drop nct6775/nct6776 support to simplify the code when you do that. Maybe as separate commit, though. Hehe - I'm testing on a nct6776 :) I'll look into it once the first conversion has been accepted. Wondering - why don't you use the nct6775 driver ? Thanks, Guenter
Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
On 03/21/2017 03:46 AM, Peter Hüwe wrote: Hi On Friday 03 March 2017 03:56:01 Guenter Roeck wrote: Hi Peter, On 03/02/2017 04:33 PM, Peter Hüwe wrote: Hi, is anybody else working on the conversion of the w83627ehf to the new hwmon_device_register_with_info interface? I don't think so. Otherwise I will probably update the driver to this interface within the next days - but since it's a lot of work I wanted to check for duplication first. Go ahead. So I'm close to have to conversion done, the current diff stat is about 647 insertions(+), 787 deletions(-) for the whole conversion. Should I try to break it down into smaller chunks for easier review? Although this would mean to convert stuff from A to B and then from B to C - otherwise the intermediate steps would be not fully functional. (since the sysfs nodes partially would exist under hwmon1/ and partially under hwmon1/device/ (as they currently are)). or just submit it? Just submit it. I don't really think a conversion like this can be split up cleanly. It saves about 20k in compiled size, so the savings from reduced boilerplate are huge. (and I think it's more readable) I would suggest to drop nct6775/nct6776 support to simplify the code when you do that. Maybe as separate commit, though. Hehe - I'm testing on a nct6776 :) I'll look into it once the first conversion has been accepted. Wondering - why don't you use the nct6775 driver ? Thanks, Guenter
Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
Hi On Friday 03 March 2017 03:56:01 Guenter Roeck wrote: > Hi Peter, > > On 03/02/2017 04:33 PM, Peter Hüwe wrote: > > Hi, > > > > is anybody else working on the conversion of the w83627ehf to the new > > hwmon_device_register_with_info interface? > > I don't think so. > > > Otherwise I will probably update the driver to this interface within the > > next days - but since it's a lot of work I wanted to check for > > duplication first. > > Go ahead. So I'm close to have to conversion done, the current diff stat is about 647 insertions(+), 787 deletions(-) for the whole conversion. Should I try to break it down into smaller chunks for easier review? Although this would mean to convert stuff from A to B and then from B to C - otherwise the intermediate steps would be not fully functional. (since the sysfs nodes partially would exist under hwmon1/ and partially under hwmon1/device/ (as they currently are)). or just submit it? It saves about 20k in compiled size, so the savings from reduced boilerplate are huge. (and I think it's more readable) > I would suggest to drop nct6775/nct6776 support to simplify the > code when you do that. Maybe as separate commit, though. Hehe - I'm testing on a nct6776 :) I'll look into it once the first conversion has been accepted. Thanks, Peter
Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
Hi On Friday 03 March 2017 03:56:01 Guenter Roeck wrote: > Hi Peter, > > On 03/02/2017 04:33 PM, Peter Hüwe wrote: > > Hi, > > > > is anybody else working on the conversion of the w83627ehf to the new > > hwmon_device_register_with_info interface? > > I don't think so. > > > Otherwise I will probably update the driver to this interface within the > > next days - but since it's a lot of work I wanted to check for > > duplication first. > > Go ahead. So I'm close to have to conversion done, the current diff stat is about 647 insertions(+), 787 deletions(-) for the whole conversion. Should I try to break it down into smaller chunks for easier review? Although this would mean to convert stuff from A to B and then from B to C - otherwise the intermediate steps would be not fully functional. (since the sysfs nodes partially would exist under hwmon1/ and partially under hwmon1/device/ (as they currently are)). or just submit it? It saves about 20k in compiled size, so the savings from reduced boilerplate are huge. (and I think it's more readable) > I would suggest to drop nct6775/nct6776 support to simplify the > code when you do that. Maybe as separate commit, though. Hehe - I'm testing on a nct6776 :) I'll look into it once the first conversion has been accepted. Thanks, Peter
Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
Hi Peter, On Fri, 3 Mar 2017 01:33:01 +0100, Peter Hüwe wrote: > is anybody else working on the conversion of the w83627ehf to the new > hwmon_device_register_with_info interface? Not me. -- Jean Delvare SUSE L3 Support
Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
Hi Peter, On Fri, 3 Mar 2017 01:33:01 +0100, Peter Hüwe wrote: > is anybody else working on the conversion of the w83627ehf to the new > hwmon_device_register_with_info interface? Not me. -- Jean Delvare SUSE L3 Support
Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
Hi Peter, On 03/02/2017 04:33 PM, Peter Hüwe wrote: Hi, is anybody else working on the conversion of the w83627ehf to the new hwmon_device_register_with_info interface? I don't think so. Otherwise I will probably update the driver to this interface within the next days - but since it's a lot of work I wanted to check for duplication first. Go ahead. I would suggest to drop nct6775/nct6776 support to simplify the code when you do that. Maybe as separate commit, though. Do you think it makes sense to introduce a hwmon_sensor_types for "intrusion" as well? - there are currently 8 drivers who offer that interface. I don't really like the idea of introducing another type for just one attribute, but it might be the easiest and most consistent approach. Feel free to submit a patch to add it. Guenter
Re: Conversion of w83627ehf to hwmon_device_register_with_info ?
Hi Peter, On 03/02/2017 04:33 PM, Peter Hüwe wrote: Hi, is anybody else working on the conversion of the w83627ehf to the new hwmon_device_register_with_info interface? I don't think so. Otherwise I will probably update the driver to this interface within the next days - but since it's a lot of work I wanted to check for duplication first. Go ahead. I would suggest to drop nct6775/nct6776 support to simplify the code when you do that. Maybe as separate commit, though. Do you think it makes sense to introduce a hwmon_sensor_types for "intrusion" as well? - there are currently 8 drivers who offer that interface. I don't really like the idea of introducing another type for just one attribute, but it might be the easiest and most consistent approach. Feel free to submit a patch to add it. Guenter
