lkml.org 
[lkml]   [2022]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 4/4] hwmon: add driver for the Microchip LAN966x SoC
From
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

\
 
 \ /
  Last update: 2022-04-01 17:55    [W:0.404 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site