From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Rahul Tanwar <rahul.tanwar@linux.intel.com>
Cc: linux-pwm@vger.kernel.org, thierry.reding@gmail.com,
p.zabel@pengutronix.de, robh+dt@kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
andriy.shevchenko@intel.com, songjun.Wu@intel.com,
cheol.yong.kim@intel.com, qi-ming.wu@intel.com,
rahul.tanwar.linux@gmail.com
Subject: Re: [PATCH v3 2/2] Add PWM fan controller driver for LGM SoC
Date: Mon, 29 Jun 2020 15:43:46 +0200 [thread overview]
Message-ID: <20200629134346.yjol6xk2iafs2mgj@pengutronix.de> (raw)
In-Reply-To: <df22a642083474e71f9f8274c033a6ef9757af5f.1593420979.git.rahul.tanwar@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 7281 bytes --]
Hello Rahul,
On Mon, Jun 29, 2020 at 05:03:47PM +0800, Rahul Tanwar wrote:
> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
> new file mode 100644
> index 000000000000..661fa7d9145d
> --- /dev/null
> +++ b/drivers/pwm/pwm-intel-lgm.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Intel Corporation.
> + *
> + * Notes & Limitations:
> + * - The hardware supports fixed period which is dependent on 2/3 or 4
> + * wire fan mode.
> + * - Supports normal polarity. Does not support changing polarity.
> + * - When PWM is disabled, output of PWM will become 0(inactive). It doesn't
> + * keep track of running period.
> + * - When duty cycle is changed, PWM output may be a mix of previous setting
> + * and new setting for the first period. From second period, the output is
> + * based on new setting.
> + * - Supports 100% duty cycle.
This would be worth mentioning if it didn't support that. IMHO you can
drop this one.
> + * - It is a dedicated PWM fan controller. There are no other consumers for
> + * this PWM controller.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +#define PWM_FAN_CON0 0x0
> +#define PWM_FAN_EN_EN BIT(0)
> +#define PWM_FAN_EN_DIS 0x0
> +#define PWM_FAN_EN_MSK BIT(0)
> +#define PWM_FAN_MODE_2WIRE 0x0
> +#define PWM_FAN_MODE_4WIRE 0x1
> +#define PWM_FAN_MODE_MSK BIT(1)
> +#define PWM_FAN_DC_MSK GENMASK(23, 16)
> +
> +#define PWM_FAN_CON1 0x4
> +#define PWM_FAN_MAX_RPM_MSK GENMASK(15, 0)
> +
> +#define MAX_RPM (BIT(16) - 1)
> +#define DFAULT_RPM 4000
DEFAULT_RPM?
> +#define MAX_DUTY_CYCLE (BIT(8) - 1)
> +
> +#define DC_BITS 8
> +
> +#define PERIOD_2WIRE_NSECS 40000000
> +#define PERIOD_4WIRE_NSECS 40000
Please add a common prefix to these definitions.
> +#define LGM_PWM_DIV_ROUND_DOWN(n, d) (((n) + ((d) / 2) - 1) / (d))
> +
> +struct lgm_pwm_chip {
> + struct pwm_chip chip;
> + struct regmap *regmap;
> + struct clk *clk;
> + struct reset_control *rst;
> + u32 period;
> +};
> +
> +static inline struct lgm_pwm_chip *to_lgm_pwm_chip(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct lgm_pwm_chip, chip);
> +}
> +
> +static int lgm_pwm_enable(struct pwm_chip *chip, bool enable)
> +{
> + struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
> + struct regmap *regmap = pc->regmap;
> +
> + if (enable)
> + regmap_update_bits(regmap, PWM_FAN_CON0,
> + PWM_FAN_EN_MSK, PWM_FAN_EN_EN);
> + else
> + regmap_update_bits(regmap, PWM_FAN_CON0,
> + PWM_FAN_EN_MSK, PWM_FAN_EN_DIS);
Is it easier to understand what happens here if you write this as:
+ regmap_update_bits(regmap, PWM_FAN_CON0, PWM_FAN_EN_MSK,
+ enable ? PWM_FAN_EN_EN : PWM_FAN_EN_DIS);
? (This is what I'd prefer, but maybe this is subjective?)
> +
> + return 0;
> +}
> +
> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
> + u32 duty_cycle, val;
> + unsigned int period;
> +
> + period = min_t(unsigned int, state->period, pc->period);
Given that state->period will most probably change type (from unsigned
int to u64) soon, it would be nice to use this already here.
> + if (state->polarity != PWM_POLARITY_NORMAL ||
> + period < pc->period)
> + return -EINVAL;
> +
> + duty_cycle = min_t(u32, state->duty_cycle, period);
> +
> + /* reg_value = duty_ns * MAX_REG_VAL(0xff) / period_ns */
s/MAX_REG_VAL/MAX_DUTY_CYCLE/
> + val = LGM_PWM_DIV_ROUND_DOWN(duty_cycle << DC_BITS, period);
The rounding is wrong here, you have to round down. I think you need to
do:
val = duty_cycle * MAX_DUTY_CYCLE / period;
> + val = min_t(u32, val, MAX_DUTY_CYCLE);
Then as duty_cycle <= period this is a noop and can be dropped.
> + regmap_update_bits(pc->regmap, PWM_FAN_CON0, PWM_FAN_DC_MSK,
> + FIELD_PREP(PWM_FAN_DC_MSK, val));
> +
> + if (state->enabled != regmap_test_bits(pc->regmap, PWM_FAN_CON0,
> + PWM_FAN_EN_EN))
> + lgm_pwm_enable(chip, state->enabled);
Here a spike can happen that you can prevent:
pwm_apply_state(pwm, { .enabled = 1, .duty_cycle = 0ms, .period = 40ms })
pwm_apply_state(pwm, { .enabled = 0, .duty_cycle = 40ms, .period = 40ms })
As apply first configures the duty_cycle, the output goes high before
disabling. So better do something like:
if (!state->enabled) {
lgm_pwm_enable(chip, 0);
return;
}
configure_duty_cycle();
if ( state->enabled)
lgm_pwm_enable(chip, 1);
> + return 0;
> +}
> +
> +static void lgm_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
> + u32 duty, val;
> +
> + state->enabled = regmap_test_bits(pc->regmap, PWM_FAN_CON0,
> + PWM_FAN_EN_EN);
> + state->polarity = PWM_POLARITY_NORMAL;
> + state->period = pc->period; /* fixed period */
> +
> + regmap_read(pc->regmap, PWM_FAN_CON0, &val);
> + duty = FIELD_GET(PWM_FAN_DC_MSK, val);
> + state->duty_cycle = duty * pc->period >> DC_BITS;
If PWM_FAN_DC = 255 means 100% the calculation is wrong. You said in the
v2 thread: 0 = disabled (0%) and 255 = 100%, so we need here:
state->duty_cycle = DIV_ROUND_UP(duty * pc->period, 255);
.
> + state->duty_cycle = roundup_pow_of_two(state->duty_cycle);
> +}
> +[..]
> +static int lgm_pwm_probe(struct platform_device *pdev)
> +{
> + struct lgm_pwm_chip *pc;
> + struct device *dev = &pdev->dev;
> + void __iomem *io_base;
> + int ret;
> +
> + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> + if (!pc)
> + return -ENOMEM;
> +
> + io_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(io_base))
> + return PTR_ERR(io_base);
> +
> + pc->regmap = devm_regmap_init_mmio(dev, io_base, &lgm_pwm_regmap_config);
> + if (IS_ERR(pc->regmap)) {
> + ret = PTR_ERR(pc->regmap);
> + dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
> + return ret;
> + }
> +
> + pc->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pc->clk)) {
> + ret = PTR_ERR(pc->clk);
> + dev_err(dev, "failed to get clock: %pe\n", pc->clk);
> + return ret;
> + }
> +
> + pc->rst = devm_reset_control_get_exclusive(dev, NULL);
> + if (IS_ERR(pc->rst)) {
> + ret = PTR_ERR(pc->rst);
> + dev_err(dev, "failed to get reset control: %pe\n", pc->rst);
Please skip the error messages if ret = -EPROBE_DEFER.
> + return ret;
> + }
> +
> + ret = reset_control_deassert(pc->rst);
> + if (ret) {
> + dev_err(dev, "cannot deassert reset control: %pe\n",
> + ERR_PTR(ret));
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(pc->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable clock\n");
reset_control_assert(pc->rst);
> + return ret;
> + }
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2020-06-29 19:43 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-29 9:03 [PATCH v3 0/2] pwm: intel: Add PWM driver for a new SoC Rahul Tanwar
2020-06-29 9:03 ` [PATCH v3 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC Rahul Tanwar
2020-06-29 22:08 ` Rob Herring
2020-06-29 9:03 ` [PATCH v3 2/2] Add PWM fan controller driver for " Rahul Tanwar
2020-06-29 13:43 ` Uwe Kleine-König [this message]
2020-06-29 19:58 ` Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200629134346.yjol6xk2iafs2mgj@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=andriy.shevchenko@intel.com \
--cc=cheol.yong.kim@intel.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=qi-ming.wu@intel.com \
--cc=rahul.tanwar.linux@gmail.com \
--cc=rahul.tanwar@linux.intel.com \
--cc=robh+dt@kernel.org \
--cc=songjun.Wu@intel.com \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.