| Message ID | 20210708120111.519444-1-nuno.sa@analog.com |
|---|---|
| Headers | show |
| Series | AXI FAN new features and improvements | expand |
Hi Guenter, > From: Nuno Sá <nuno.sa@analog.com> > Sent: Thursday, July 8, 2021 2:01 PM > To: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org > Cc: Guenter Roeck <linux@roeck-us.net>; Rob Herring > <robh+dt@kernel.org>; Jean Delvare <jdelvare@suse.com> > Subject: [RFC PATCH 0/6] AXI FAN new features and improvements > > This series adds some new features to the axi-fan-control driver. On > top > of that, the HW had some changes (basically it now starts automatically > out of reset) so that the driver needed some minor refactoring. The > reason I'm sending this as RFC, is mainly because of the last patch > ("hwmon: axi-fan-control: support temperature vs pwm points"). The > core > has some predefined values which define a temperature vs pwm > curve [1]. > It also exposes registers so that users can change it according to their > needs. As I could not find standard attributes in the subsystem, I'm > proposing some "raw" sysfs files. Looking at [2], the pwm_auto_point > stuff looked to be what I want. Obviously I might be wrong :). If this > is accepted, I will add a proper sysfs DOC file describing the new files > (being lazy in the RFC). > > For patch 5 ("hwmon: axi-fan-control: clear the fan fault irq at > startup"), > it's also arguable if we really need it. The main reason I have it is > because of some userland apps that might take some drastic measures > by > just reading 1 fan_fault alarm. Obviously, we can argue that the > problem > is in the app and not in the driver. Though it's such a minimal change > that I decided to include it (I'm more than fine in dropping the patch). > > [1]: https://wiki.analog.com/resources/fpga/docs/axi_fan_control > [2]: > https://urldefense.com/v3/__https://www.kernel.org/doc/Documen > tation/hwmon/sysfs- > interface__;!!A3Ni8CS0y2Y!uwjpaOT8QEBVfKTCWELJNbjJJ69iR7S3tKS > WV4B0K742CtcARkTtAqMxknnpPw$ > > Nuno Sá (6): > hwmon: axi-fan-control: make sure the clock is enabled > hwmon: axi-fan-control: add tacho devicetree properties > dt-bindings: axi-fan-control: add tacho properties > hwmon: axi-fan-control: handle irqs in natural order > hwmon: axi-fan-control: clear the fan fault irq at startup > hwmon: axi-fan-control: support temperature vs pwm points > The HW guy is willing to change how the core works. This means, that all that unstable pwm - rpm points will go away and we will have a register where we can set the minimum fan speed for evaluating the FAN. He also said that the default value for the this setting will be pretty low so that we should only have _real_ faults at startup which means patch 5 should not be needed anymore... Anyways, I will send a new pull with patches 1,3 and 5 and as soon as I have some HW ready to test, I will send the other patches. With the new mechanism, we can also simplify the IRQ handling [1]... For the new devicetree property, I think now it really is a fan property which makes me wonder if the property will be accepted in the controller bindings or if I need to send a fan.yaml... [1]: https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/hwmon/axi-fan-control.c#L280 - Nuno Sá
Hi, On 7/27/21 1:42 AM, Sa, Nuno wrote: > > Hi Guenter, > >> From: Nuno Sá <nuno.sa@analog.com> >> Sent: Thursday, July 8, 2021 2:01 PM >> To: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org >> Cc: Guenter Roeck <linux@roeck-us.net>; Rob Herring >> <robh+dt@kernel.org>; Jean Delvare <jdelvare@suse.com> >> Subject: [RFC PATCH 0/6] AXI FAN new features and improvements >> >> This series adds some new features to the axi-fan-control driver. On >> top >> of that, the HW had some changes (basically it now starts automatically >> out of reset) so that the driver needed some minor refactoring. The >> reason I'm sending this as RFC, is mainly because of the last patch >> ("hwmon: axi-fan-control: support temperature vs pwm points"). The >> core >> has some predefined values which define a temperature vs pwm >> curve [1]. >> It also exposes registers so that users can change it according to their >> needs. As I could not find standard attributes in the subsystem, I'm >> proposing some "raw" sysfs files. Looking at [2], the pwm_auto_point >> stuff looked to be what I want. Obviously I might be wrong :). If this >> is accepted, I will add a proper sysfs DOC file describing the new files >> (being lazy in the RFC). >> >> For patch 5 ("hwmon: axi-fan-control: clear the fan fault irq at >> startup"), >> it's also arguable if we really need it. The main reason I have it is >> because of some userland apps that might take some drastic measures >> by >> just reading 1 fan_fault alarm. Obviously, we can argue that the >> problem >> is in the app and not in the driver. Though it's such a minimal change >> that I decided to include it (I'm more than fine in dropping the patch). >> >> [1]: https://wiki.analog.com/resources/fpga/docs/axi_fan_control >> [2]: >> https://urldefense.com/v3/__https://www.kernel.org/doc/Documen >> tation/hwmon/sysfs- >> interface__;!!A3Ni8CS0y2Y!uwjpaOT8QEBVfKTCWELJNbjJJ69iR7S3tKS >> WV4B0K742CtcARkTtAqMxknnpPw$ >> >> Nuno Sá (6): >> hwmon: axi-fan-control: make sure the clock is enabled >> hwmon: axi-fan-control: add tacho devicetree properties >> dt-bindings: axi-fan-control: add tacho properties >> hwmon: axi-fan-control: handle irqs in natural order >> hwmon: axi-fan-control: clear the fan fault irq at startup >> hwmon: axi-fan-control: support temperature vs pwm points >> > > The HW guy is willing to change how the core works. This means, > that all that unstable pwm - rpm points will go away and we will > have a register where we can set the minimum fan speed for > evaluating the FAN. He also said that the default value for the > this setting will be pretty low so that we should only have _real_ > faults at startup which means patch 5 should not be needed > anymore... > > Anyways, I will send a new pull with patches 1,3 and 5 and That kind of contradicts what you say above, that patch 5 won't be needed anymore. Am I missing something ? > as soon as I have some HW ready to test, I will send the other > patches. With the new mechanism, we can also simplify the IRQ > handling [1]... > > For the new devicetree property, I think now it really is a fan > property which makes me wonder if the property will be accepted > in the controller bindings or if I need to send a fan.yaml... > Not really sure myself. At he very least we'll have sysfs properties, so the minimum speed could also be updated from userspace. Ultimately we'll need some set of devicetree properties, not only for fans but for pretty much everything supported by hwmon, but I have no idea what is acceptable and what isn't - if I did I might have proposed something by now. Guenter
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter > Roeck > Sent: Wednesday, July 28, 2021 8:38 PM > To: Sa, Nuno <Nuno.Sa@analog.com>; linux-hwmon@vger.kernel.org; > devicetree@vger.kernel.org > Cc: Rob Herring <robh+dt@kernel.org>; Jean Delvare > <jdelvare@suse.com> > Subject: Re: [RFC PATCH 0/6] AXI FAN new features and > improvements > > [External] > > Hi, > > On 7/27/21 1:42 AM, Sa, Nuno wrote: > > > > Hi Guenter, > > > >> From: Nuno Sá <nuno.sa@analog.com> > >> Sent: Thursday, July 8, 2021 2:01 PM > >> To: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org > >> Cc: Guenter Roeck <linux@roeck-us.net>; Rob Herring > >> <robh+dt@kernel.org>; Jean Delvare <jdelvare@suse.com> > >> Subject: [RFC PATCH 0/6] AXI FAN new features and improvements > >> > >> This series adds some new features to the axi-fan-control driver. > On > >> top > >> of that, the HW had some changes (basically it now starts > automatically > >> out of reset) so that the driver needed some minor refactoring. The > >> reason I'm sending this as RFC, is mainly because of the last patch > >> ("hwmon: axi-fan-control: support temperature vs pwm points"). > The > >> core > >> has some predefined values which define a temperature vs pwm > >> curve [1]. > >> It also exposes registers so that users can change it according to > their > >> needs. As I could not find standard attributes in the subsystem, I'm > >> proposing some "raw" sysfs files. Looking at [2], the > pwm_auto_point > >> stuff looked to be what I want. Obviously I might be wrong :). If this > >> is accepted, I will add a proper sysfs DOC file describing the new > files > >> (being lazy in the RFC). > >> > >> For patch 5 ("hwmon: axi-fan-control: clear the fan fault irq at > >> startup"), > >> it's also arguable if we really need it. The main reason I have it is > >> because of some userland apps that might take some drastic > measures > >> by > >> just reading 1 fan_fault alarm. Obviously, we can argue that the > >> problem > >> is in the app and not in the driver. Though it's such a minimal change > >> that I decided to include it (I'm more than fine in dropping the > patch). > >> > >> [1]: https://wiki.analog.com/resources/fpga/docs/axi_fan_control > >> [2]: > >> > https://urldefense.com/v3/__https://www.kernel.org/doc/Documen > >> tation/hwmon/sysfs- > >> > interface__;!!A3Ni8CS0y2Y!uwjpaOT8QEBVfKTCWELJNbjJJ69iR7S3tKS > >> WV4B0K742CtcARkTtAqMxknnpPw$ > >> > >> Nuno Sá (6): > >> hwmon: axi-fan-control: make sure the clock is enabled > >> hwmon: axi-fan-control: add tacho devicetree properties > >> dt-bindings: axi-fan-control: add tacho properties > >> hwmon: axi-fan-control: handle irqs in natural order > >> hwmon: axi-fan-control: clear the fan fault irq at startup > >> hwmon: axi-fan-control: support temperature vs pwm points > >> > > > > The HW guy is willing to change how the core works. This means, > > that all that unstable pwm - rpm points will go away and we will > > have a register where we can set the minimum fan speed for > > evaluating the FAN. He also said that the default value for the > > this setting will be pretty low so that we should only have _real_ > > faults at startup which means patch 5 should not be needed > > anymore... > > > > Anyways, I will send a new pull with patches 1,3 and 5 and > > That kind of contradicts what you say above, that patch 5 won't be > needed anymore. Am I missing something ? My bad... I meant patch 6 and not 5. - Nuno Sá
This series adds some new features to the axi-fan-control driver. On top of that, the HW had some changes (basically it now starts automatically out of reset) so that the driver needed some minor refactoring. The reason I'm sending this as RFC, is mainly because of the last patch ("hwmon: axi-fan-control: support temperature vs pwm points"). The core has some predefined values which define a temperature vs pwm curve [1]. It also exposes registers so that users can change it according to their needs. As I could not find standard attributes in the subsystem, I'm proposing some "raw" sysfs files. Looking at [2], the pwm_auto_point stuff looked to be what I want. Obviously I might be wrong :). If this is accepted, I will add a proper sysfs DOC file describing the new files (being lazy in the RFC). For patch 5 ("hwmon: axi-fan-control: clear the fan fault irq at startup"), it's also arguable if we really need it. The main reason I have it is because of some userland apps that might take some drastic measures by just reading 1 fan_fault alarm. Obviously, we can argue that the problem is in the app and not in the driver. Though it's such a minimal change that I decided to include it (I'm more than fine in dropping the patch). [1]: https://wiki.analog.com/resources/fpga/docs/axi_fan_control [2]: https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface Nuno Sá (6): hwmon: axi-fan-control: make sure the clock is enabled hwmon: axi-fan-control: add tacho devicetree properties dt-bindings: axi-fan-control: add tacho properties hwmon: axi-fan-control: handle irqs in natural order hwmon: axi-fan-control: clear the fan fault irq at startup hwmon: axi-fan-control: support temperature vs pwm points .../bindings/hwmon/adi,axi-fan-control.yaml | 12 ++ drivers/hwmon/axi-fan-control.c | 160 ++++++++++++++++-- 2 files changed, 156 insertions(+), 16 deletions(-)