lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 1 Apr 2022 08:23:19 -0700 From: Guenter Roeck <linux@...ck-us.net> To: Michael Walle <michael@...le.cc> Cc: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v3 4/4] hwmon: add driver for the Microchip LAN966x SoC On 4/1/22 05:57, Michael Walle wrote: > Am 2022-03-31 19:28, schrieb Guenter Roeck: > >>> +static int lan966x_hwmon_write_pwm_freq(struct device *dev, long val) >>> +{ >>> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev); >>> + >>> + val = DIV_ROUND_CLOSEST(hwmon->clk_rate, val); >> >> I must have looked at this for an hour, but I didn't see the problem. >> Sorry for that. Try writing "0" as new pwm frequency. > > Ohh, and negative values.. > > I'll add a > > if (val <= 0) > return -EINVAL; > >>> +static int lan966x_hwmon_enable(struct lan966x_hwmon *hwmon) >>> +{ >>> + unsigned int mask = SENSOR_CFG_SAMPLE_ENA | >>> + SENSOR_CFG_START_CAPTURE | >>> + SENSOR_CFG_CONTINIOUS_MODE | >>> + SENSOR_CFG_PSAMPLE_ENA; >>> + unsigned int val; >>> + >>> + /* enable continuous mode */ >>> + val = SENSOR_CFG_SAMPLE_ENA | SENSOR_CFG_CONTINIOUS_MODE; >>> + >> >> I am curious: Why not as part of the assignment, similar to 'mask' ? > > There was code to set the clock divider, but I've removed > it as the hardware has a sane default. That left just that one > line, but moving the comment above the declaration looked > weird. > > Now thinking about it again, it might make sense to bring > back the code to set the clock divider in case someone > will fiddle around with it before the driver is probed. > > #define LAN966X_PVT_CLK 1200000 /* 1.2 MHz */ > > /* set PVT clock to be between 1.15 and 1.25 MHz */ > div = DIV_ROUND_CLOSEST(hwmon->clk_rate, LAN966X_PVT_CLK); > val |= FIELD_PREP(SENSOR_CFG_CLK_CFG, div); > Yes, that would be a good idea. Thanks, Guenter >>> +static int lan966x_hwmon_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct lan966x_hwmon *hwmon; >>> + struct device *hwmon_dev; >>> + int ret; >>> + >>> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL); >>> + if (!hwmon) >>> + return -ENOMEM; >>> + >>> + hwmon->clk = devm_clk_get(dev, NULL); >>> + if (IS_ERR(hwmon->clk)) >>> + return dev_err_probe(dev, PTR_ERR(hwmon->clk), >>> + "failed to get clock\n"); >>> + >>> + ret = lan966x_clk_enable(dev, hwmon); >>> + if (ret) >>> + return dev_err_probe(dev, ret, "failed to enable clock\n"); >>> + >>> + hwmon->clk_rate = clk_get_rate(hwmon->clk); >>> + >>> + hwmon->regmap_pvt = lan966x_init_regmap(pdev, "pvt"); >>> + if (IS_ERR(hwmon->regmap_pvt)) >>> + return dev_err_probe(dev, PTR_ERR(hwmon->regmap_pvt), >>> + "failed to get regmap for PVT registers\n"); >>> + >>> + hwmon->regmap_fan = lan966x_init_regmap(pdev, "fan"); >>> + if (IS_ERR(hwmon->regmap_fan)) >>> + return dev_err_probe(dev, PTR_ERR(hwmon->regmap_fan), >>> + "failed to get regmap for fan registers\n"); >>> + >>> + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, >>> + "lan966x_hwmon", hwmon, >>> + &lan966x_hwmon_chip_info, NULL); >>> + if (IS_ERR(hwmon_dev)) >>> + return dev_err_probe(dev, PTR_ERR(hwmon_dev), >>> + "failed to register hwmon device\n"); >>> + >>> + return lan966x_hwmon_enable(hwmon); >> >> Since I am nitpicking: It may make sense to call this function before >> registering the hwmon device, and it may make sense to disable sampling >> when unloading the driver (you could trigger that by calling >> devm_add_action_or_reset() from lan966x_hwmon_enable). > > sure > > -michael
Powered by blists - more mailing lists