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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 01 Apr 2022 14:57:21 +0200
From:   Michael Walle <michael@...le.cc>
To:     Guenter Roeck <linux@...ck-us.net>
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

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);

>> +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