Hi Mark, Thanks for looking into this, I'm the author of the current f71882fg driver and I have adding pwm support on my todo list for quite a while now, but unfortunately sofar I haven't found the time for it. I have however already a clear idea how I would like to see the pwm implement (but I'm open for discussion). You've currently added both pwmX and fanX_target attributes, but those both show (and store!!) the same register in a different "notation", and at any given time only one of the 2 notations is valid, depending on the pwmX_enable attribute. Allowing writes to pwmX when the fintek is pwm set in rpm modes not pwm modes or the other way round is IMHO unacceptable, as this will lead to bogues settings. I have been thinking about this for some time now and I see 4 solutions for this: 1) Only support pwm modes as this is what all other hwmon IC's have, but this would require reprogramming the auto_points when in rpm auto mode so that we get sane settings when reprogramming the f71882fg in auto pwm mode. I don't like this as it causes the chip to be reprogrammed from its bios defaults without the user asking for it. 2) make pwmX / fanX_target come and go depending on pwmX_enable, so yes that would mean removing and adding sysfs atributes on the fly as pwmX_enable changes. Don't like it either, ugly, and racy what happens if an app keeps an attribute open? 3) mark the pwmX / fanX_target readonly, that is chmod the attribute on the fly as pwmX_enable changes, and check in show/store_pwm if we are in pwm mode, if not return EPERM for store (fixes attr kept open race) and return -1 as value when showing. Still quite complex 4) See rpm mode as just a different pwm mode, pwm is 0-100% dutycylce mapped to 0-255, see rpm mode as 0-100% of the fan full speed count register, note this is how the target speed per section is set in the registers when in rpm auto mode, and such for auto mode has very much an 1-1 mapping for all the auto regs, which can fully reuse our existing defined sysfs attr for auto pwm control (pwmX_auto_point_*****) The only place where this mapping isn't 1 on 1 is for manual rpm mode, where the fintek allows you to specify hard go X rpm, but mapping this to a % of full speed should be easy. I greatly prefer solution 4 for its simplicity and matching our currently defined sysfs attr standard for pwm and auto pwm I want to combine this with simply using either pwm or rpm mode (depending on what the BIOS gives us) and not allow changing this on the fly with pwm_enable, so reduce pwm_enable to simply choosing between auto and manual mode. I don't want to allow on the fly changing, because then the meaning of a lot of registers change and they would all need to be reinitialized to something sane in their new meaning. We could add a module option to force either mode at module load time (overriding bios settings for both the mode and the zone settings). Last is what todo with the full speed reg, I wonder have you tested if this gets set to anything sane by the BIOS or the chip itself? I know some hwmon IC's how have a register like this start by running the fan at full speed for 2 seconds and then measure that speed and store that in full speed, I wonder if that happens here. Anyways we should create an atrribute for it and it shoud _not_ be called fanX_max, max is already used in other cases and it means give an alarm if the reading goes above this value. Instead I would like to suggest to use what the datasheet uses and call it fanX_full_speed So what do you think of my proposal 4) for sysfs interface for the pwm part of the f71882fg ? And would you be willing to implement this? Thanks & Regards, Hans p.s. 1) If you want we can continue this discussion in private in Dutch, but I would greatly prefer to keep this on the list in English 2) Being bold and assusming you're Dutch, where in the Netherlands do you live? I'm always interested in meeting fellow Dutch Linux hackers :)
