linux-kernel - Re: [PATCH v3 4/4] hwmon: add driver for the Microchip LAN966x SoC

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e4267787-4e71-6122-db3e-ce34110cccbb@roeck-us.net>
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