Hi Guenter, One question/doubt that I just noticed now...
On Sun, 2019-10-06 at 08:32 -0700, Guenter Roeck wrote: > > On Thu, Sep 26, 2019 at 12:39:24PM +0200, Nuno Sá wrote: > > The purpose of this IP Core is to control the fan used for the > > cooling of a > > Xilinx Zynq Ultrascale+ MPSoC without the need of any external > > temperature > > sensors. To achieve this, the IP core uses the PL SYSMONE4 > > primitive to > > obtain the PL temperature and, based on those readings, it then > > outputs > > a PWM signal to control the fan rotation accordingly. > > > > Signed-off-by: Nuno Sá <nuno...@analog.com> > > --- > > MAINTAINERS | 7 + > > drivers/hwmon/Kconfig | 9 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/axi-fan-control.c | 452 > > ++++++++++++++++++++++++++++++++ > > 4 files changed, 469 insertions(+) > > create mode 100644 drivers/hwmon/axi-fan-control.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index c7035ce2460b..d775c923d23b 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -2853,6 +2853,13 @@ S: Maintained > > F: Documentation/devicetree/bindings/sound/axentia,* > > F: sound/soc/atmel/tse850-pcm5142.c > > > > +AXI-FAN-CONTROL HARDWARE MONITOR DRIVER > > +M: Nuno Sá <nuno...@analog.com> > > +W: http://ez.analog.com/community/linux-device-drivers > > +L: linux-hwmon@vger.kernel.org > > +S: Supported > > +F: drivers/hwmon/axi-fan-control.c > > + > > AXXIA I2C CONTROLLER > > M: Krzysztof Adamski <krzysztof.adam...@nokia.com> > > L: linux-...@vger.kernel.org > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index 2ca5668bdb62..2396cc347c47 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -269,6 +269,15 @@ config SENSORS_ASC7621 > > This driver can also be built as a module. If so, the module > > will be called asc7621. > > > > +config SENSORS_AXI_FAN_CONTROL > > + tristate "Analog Devices FAN Control HDL Core driver" > > + help > > + If you say yes here you get support for the Analog Devices > > + AXI HDL FAN monitoring core. > > + > > + This driver can also be built as a module. If so, the module > > + will be called axi-fan-control > > + > > config SENSORS_K8TEMP > > tristate "AMD Athlon64/FX or Opteron temperature sensor" > > depends on X86 && PCI > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > > index c86ce4d3d36b..ebb1fd2ea82b 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_AS370) += as370- > > hwmon.o > > obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o > > obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o > > obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o > > +obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o > > obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o > > obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o > > obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o > > diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi- > > fan-control.c > > new file mode 100644 > > index 000000000000..f86efc444753 > > --- /dev/null > > +++ b/drivers/hwmon/axi-fan-control.c > > @@ -0,0 +1,452 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Fan Control HDL CORE driver > > + * > > + * Copyright 2019 Analog Devices Inc. > > + */ > > +#include <linux/clk.h> > > +#include <linux/fpga/adi-axi-common.h> > > +#include <linux/hwmon.h> > > +#include <linux/interrupt.h> > > +#include <linux/io.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > + > > +/* register map */ > > +#define ADI_REG_RSTN 0x0080 > > +#define ADI_REG_PWM_WIDTH 0x0084 > > +#define ADI_REG_TACH_PERIOD 0x0088 > > +#define ADI_REG_TACH_TOLERANCE 0x008c > > +#define ADI_REG_PWM_PERIOD 0x00c0 > > +#define ADI_REG_TACH_MEASUR 0x00c4 > > +#define ADI_REG_TEMPERATURE 0x00c8 > > + > > +#define ADI_REG_IRQ_MASK 0x0040 > > +#define ADI_REG_IRQ_PENDING 0x0044 > > +#define ADI_REG_IRQ_SRC 0x0048 > > + > > +/* IRQ sources */ > > +#define ADI_IRQ_SRC_PWM_CHANGED BIT(0) > > include linux/bits.h > > > +#define ADI_IRQ_SRC_TACH_ERR BIT(1) > > +#define ADI_IRQ_SRC_TEMP_INCREASE BIT(2) > > +#define ADI_IRQ_SRC_NEW_MEASUR BIT(3) > > +#define ADI_IRQ_SRC_MASK GENMASK(3, 0) > > +#define ADI_IRQ_MASK_OUT_ALL 0xFFFFFFFFU > > + > > +#define SYSFS_PWM_MAX 255 > > + > > +struct axi_fan_control_data { > > + struct clk *clk; > > + void __iomem *base; > > + /* tacho period */ > > + atomic_t tach; > > Why exactly is this an atomic ? The value is only set, and it is set > in > a single operation. Why does it matter if reqading it catches the old > or > the new value ? > > > + int irq; > > + /* pulses per revolution */ > > + u32 ppr; > > + bool hw_pwm_req; > > + bool update_tacho_params; > > + u8 fan_fault; > > +}; > > + > > +static inline void axi_fan_control_iowrite(const u32 val, const > > u32 reg, > > + const struct axi_fan_control_data *ctl) > > Multi-line alignment. Also, please consider shorter function names. > An "axi_fan_control_" prefix for static functions is really a bit > excessive > and doesn't add value. "axi_" would do it just as well. > > > +{ > > + iowrite32(val, ctl->base + reg); > > +} > > + > > +static inline u32 axi_fan_control_ioread(const u32 reg, > > + const struct > > axi_fan_control_data *ctl) > > +{ > > + return ioread32(ctl->base + reg); > > +} > > + > > +static long axi_fan_control_get_pwm_duty(const struct > > axi_fan_control_data *ctl) > > +{ > > + u32 pwm_width = axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl); > > + u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD, > > ctl); > > + > > + return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX, > > pwm_period); > > Is pwm_period guaranteed to be != 0 ? > > > +} > > + > > +static int axi_fan_control_set_pwm_duty(const long val, > > + struct axi_fan_control_data > > *ctl) > > +{ > > + u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD, > > ctl); > > + u32 new_width; > > + long __val = clamp_val(val, 0, SYSFS_PWM_MAX); > > + > > + new_width = DIV_ROUND_CLOSEST(__val * pwm_period, > > SYSFS_PWM_MAX); > > + > > + axi_fan_control_iowrite(new_width, ADI_REG_PWM_WIDTH, ctl); > > + > > + return 0; > > +} > > + > > +static long axi_fan_control_get_fan_rpm(const struct > > axi_fan_control_data *ctl) > > +{ > > + unsigned long clk_rate = clk_get_rate(ctl->clk); > > + > > + if (!clk_rate) > > + return -EINVAL; > > Unless the clock rate changes dynamically, it might make sense to > read it once > in the probe function. > > > + /* > > + * The tacho period should be: > > + * TACH = 60/(ppr * rpm), where rpm is revolutions per > > second > > + * and ppr is pulses per revolution. > > + * Given the tacho period, we can multiply it by the input > > clock > > + * so that we know how many clocks we need to have this period. > > + * From this, we can derive the RPM value. > > + */ > > + return DIV_ROUND_CLOSEST(60 * clk_rate, > > + ctl->ppr * atomic_read(&ctl->tach)); > > Are ppr and tach guaranteed to be != 0 ? I don't think so, > since neither is ever evaluated. > > > +} > > + > > +static int axi_fan_control_read_temp(struct device *dev, u32 attr, > > long *val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + long raw_temp; > > + > > + switch (attr) { > > + case hwmon_temp_input: > > + raw_temp = axi_fan_control_ioread(ADI_REG_TEMPERATURE, > > ctl); > > + /* > > + * The formula for the temperature is: > > + * T = (ADC * 501.3743 / 2^bits) - 273.6777 > > + * It's multiplied by 1000 to have milidegrees as > > s/milidegrees/millidegrees/ > > > + * specified by the hwmon sysfs interface. > > + */ > > + *val = ((raw_temp * 501374) >> 16) - 273677; > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read_fan(struct device *dev, u32 attr, > > long *val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_fan_fault: > > + *val = ctl->fan_fault; > > + return 0; > > + case hwmon_fan_input: > > + *val = axi_fan_control_get_fan_rpm(ctl); > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read_pwm(struct device *dev, u32 attr, > > long *val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_pwm_input: > > + *val = axi_fan_control_get_pwm_duty(ctl); > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_write_pwm(struct device *dev, u32 attr, > > long val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_pwm_input: > > + return axi_fan_control_set_pwm_duty(val, ctl); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read_labels(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, const > > char **str) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + *str = "FAN"; > > + return 0; > > + case hwmon_temp: > > + *str = "SYSMON4"; > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + return axi_fan_control_read_fan(dev, attr, val); > > + case hwmon_pwm: > > + return axi_fan_control_read_pwm(dev, attr, val); > > + case hwmon_temp: > > + return axi_fan_control_read_temp(dev, attr, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_write(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, long val) > > +{ > > + switch (type) { > > + case hwmon_pwm: > > + return axi_fan_control_write_pwm(dev, attr, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static umode_t axi_fan_control_fan_is_visible(const u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_fan_input: > > + case hwmon_fan_fault: > > + case hwmon_fan_label: > > + return 0444; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t axi_fan_control_pwm_is_visible(const u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_pwm_input: > > + return 0644; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t axi_fan_control_temp_is_visible(const u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_temp_input: > > + case hwmon_temp_label: > > + return 0444; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t axi_fan_control_is_visible(const void *data, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + return axi_fan_control_fan_is_visible(attr); > > + case hwmon_pwm: > > + return axi_fan_control_pwm_is_visible(attr); > > + case hwmon_temp: > > + return axi_fan_control_temp_is_visible(attr); > > + default: > > + return 0; > > + } > > +} > > + > > +static irqreturn_t axi_fan_control_irq_handler(int irq, void > > *data) > > +{ > > + struct axi_fan_control_data *ctl = (struct axi_fan_control_data > > *)data; > > + u32 irq_pending = axi_fan_control_ioread(ADI_REG_IRQ_PENDING, > > ctl); > > + u32 clear_mask; > > + > > + if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) { > > + u32 new_tach = > > axi_fan_control_ioread(ADI_REG_TACH_MEASUR, > > + ctl); > > + > > + if (ctl->update_tacho_params) { > > + /* get 25% tolerance */ > > + u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25, > > 100); > > + /* set new tacho parameters */ > > + axi_fan_control_iowrite(new_tach, > > ADI_REG_TACH_PERIOD, > > + ctl); > > + axi_fan_control_iowrite(tach_tol, > > + ADI_REG_TACH_TOLERANCE, > > ctl); > > + ctl->update_tacho_params = false; > > + } > > + > > + atomic_set(&ctl->tach, new_tach); > > + } > > + > > + if (irq_pending & ADI_IRQ_SRC_PWM_CHANGED) { > > + /* > > + * if the pwm changes on behalf of software, > > + * we need to provide new tacho parameters to the core. > > + * Wait for the next measurement for that... > > + */ > > + if (!ctl->hw_pwm_req) > > + ctl->update_tacho_params = true; > > + > > + /* just set this to false even if it is already... */ > > + ctl->hw_pwm_req = false; > > + } > > + > > + if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE) > > + /* hardware requested a new pwm */ > > + ctl->hw_pwm_req = true; > > + > I don't really understand the logic here. If > ADI_IRQ_SRC_TEMP_INCREASE means > that hardware wants a new pwm, how is userspace informed about that > request ? > And why are the tacho paramaters _not_ updated in this case later on > (unless > ADI_IRQ_SRC_PWM_CHANGED and ADI_IRQ_SRC_TEMP_INCREASE are both set) ? > It might be useful to describe the expected sequence of events. > > > + if (irq_pending & ADI_IRQ_SRC_TACH_ERR) > > + ctl->fan_fault = 1; > > Is it on purpose that this bit is never reset ? > > > + > > + /* clear all interrupts */ > > + clear_mask = irq_pending & ADI_IRQ_SRC_MASK; > > + axi_fan_control_iowrite(clear_mask, ADI_REG_IRQ_PENDING, ctl); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int axi_fan_control_init(struct axi_fan_control_data *ctl, > > + const struct device_node *np) > > +{ > > + int ret; > > + > > + /* get fan pulses per revolution */ > > + ret = of_property_read_u32(np, "adi,pulses-per-revolution", > > &ctl->ppr); > > + if (ret) > > + return ret; > > So all random values are acceptable, including 0 and 0xffffffff ? > > > + /* > > + * Enable all IRQs > > + */ > > + axi_fan_control_iowrite((ADI_IRQ_MASK_OUT_ALL & > > + ~(ADI_IRQ_SRC_NEW_MEASUR | ADI_IRQ_SRC_TACH_ERR > > | > > + ADI_IRQ_SRC_PWM_CHANGED | > > ADI_IRQ_SRC_TEMP_INCREASE)), > > One set of ( ) is unnecessary. > > > + ADI_REG_IRQ_MASK, ctl); > > + > > + /* bring the device out of reset */ > > + axi_fan_control_iowrite(0x01, ADI_REG_RSTN, ctl); > > + > > + return ret; > > +} > > + > > +static const struct hwmon_channel_info *axi_fan_control_info[] = { > > + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT), > > + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT | > > HWMON_F_LABEL), > > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL), > > + NULL > > +}; > > + > > +static const struct hwmon_ops axi_fan_control_hwmon_ops = { > > + .is_visible = axi_fan_control_is_visible, > > + .read = axi_fan_control_read, > > + .write = axi_fan_control_write, > > + .read_string = axi_fan_control_read_labels, > > +}; > > + > > +static const struct hwmon_chip_info axi_fan_control_chip_info = { > > + .ops = &axi_fan_control_hwmon_ops, > > + .info = axi_fan_control_info, > > +}; > > + > > +static const u32 version_1_0_0 = ADI_AXI_PCORE_VER(1, 0, 'a'); > > + > > +static const struct of_device_id axi_fan_control_of_match[] = { > > + { .compatible = "adi,axi-fan-control-1.00.a", > > + .data = (void *)&version_1_0_0}, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, axi_fan_control_of_match); > > + > > +static int axi_fan_control_probe(struct platform_device *pdev) > > +{ > > + struct device *hwmon_dev; > > + struct axi_fan_control_data *ctl; > > + const struct of_device_id *id; > > + struct resource *res; > > + u32 version; > > + int ret; > > + > > + id = of_match_node(axi_fan_control_of_match, pdev- > > >dev.of_node); > > + if (!id) > > + return -EINVAL; > > + > > + ctl = devm_kzalloc(&pdev->dev, sizeof(*ctl), GFP_KERNEL); > > + if (!ctl) > > + return -ENOMEM; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ctl->base = devm_ioremap_resource(&pdev->dev, res); > > devm_platform_ioremap_resource() > > > + if (IS_ERR(ctl->base)) { > > + dev_err(&pdev->dev, "ioremap failed with %ld\n", > > + PTR_ERR(ctl->base)); > > + return PTR_ERR(ctl->base); > > + } > > This does not require an extra error message. > > > + > > + ctl->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(ctl->clk)) { > > + dev_err(&pdev->dev, "clk_get failed with %ld\n", > > + PTR_ERR(ctl->clk)); > > + return PTR_ERR(ctl->clk); > > + } > > + > > + dev_dbg(&pdev->dev, "Re-mapped from 0x%08llX to %p\n", > > + (unsigned long long)res->start, ctl->base); > > + > > + version = axi_fan_control_ioread(ADI_AXI_REG_VERSION, ctl); > > + if (ADI_AXI_PCORE_VER_MAJOR(version) != > > + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data))) { > > + dev_err(&pdev->dev, "Major version mismatch. Expected > > %d.%.2d.%c, Reported %d.%.2d.%c\n", > > + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data)), > > + ADI_AXI_PCORE_VER_MINOR((*(u32 *)id->data)), > > + ADI_AXI_PCORE_VER_PATCH((*(u32 *)id->data)), > > + ADI_AXI_PCORE_VER_MAJOR(version), > > + ADI_AXI_PCORE_VER_MINOR(version), > > + ADI_AXI_PCORE_VER_PATCH(version)); > > + return -ENODEV; > > + } > > + > > + ctl->irq = platform_get_irq(pdev, 0); > > + if (ctl->irq < 0) { > > This can return -EPROBE_DEFER. On top of that, it already generates > an error > message, meaning another one here is unnecessary. Why returning -EPROBE_DEFER? platform_get_irq() already seems to handle EPROBE_DEFER... > > + dev_err(&pdev->dev, "platform_get_irq failed with > > %d\n", > > + ctl->irq); > > + return ctl->irq; > > + } > > + > > + ret = devm_request_threaded_irq(&pdev->dev, ctl->irq, NULL, > > + axi_fan_control_irq_handler, > > + IRQF_ONESHOT | > > IRQF_TRIGGER_HIGH, > > + pdev->driver_override, ctl); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to request an irq, %d", > > ret); > > + return ret; > > + } > > + > > + ret = axi_fan_control_init(ctl, pdev->dev.of_node); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to initialize device\n"); > > + return ret; > > + } > > + > > + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > > + "axi_fan_control", > > + ctl, > > + &axi_fan_control_chip_i > > nfo, > > + NULL); > > Another alignment issue. Shorter function and global variable names > would help. > > > + > > + return PTR_ERR_OR_ZERO(hwmon_dev); > > +} > > + > > +static struct platform_driver axi_fan_control_driver = { > > + .driver = { > > + .name = "axi_fan_control_driver", > > + .of_match_table = axi_fan_control_of_match, > > + }, > > + .probe = axi_fan_control_probe, > > +}; > > +module_platform_driver(axi_fan_control_driver); > > + > > +MODULE_AUTHOR("Nuno Sa <nuno...@analog.com>"); > > +MODULE_DESCRIPTION("Analog Devices Fan Control HDL CORE driver"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.23.0 > >
