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

[Guenter Roeck]

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