Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e4267787-4e71-6122-db3e-ce34110cccbb@roeck-us.net>
Date:   Fri, 1 Apr 2022 08:23:19 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v3 4/4] hwmon: add driver for the Microchip LAN966x SoC
Content-Language: en-US
To:     Michael Walle <michael@walle.cc>
Cc:     Jean Delvare <jdelvare@suse.com>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220331162431.3648535-1-michael@walle.cc>
 <20220331162431.3648535-5-michael@walle.cc>
 <06ff3ca0-9d2b-205f-064d-24bcb86e5362@roeck-us.net>
 <415334e3278a610bc603b316d4af6502@walle.cc>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <415334e3278a610bc603b316d4af6502@walle.cc>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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