On Wed, Aug 19, 2015 at 01:13:59PM +0900, Yoshihiro Shimoda wrote:
> This patch adds support for R-Car SoCs PWM Timer. The PWM timer of
> R-Car H2 has 7 channels. So, we can use the channels if we describe
> device tree nodes.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> Reviewed-by: Simon Horman <horms+renesas@xxxxxxxxxxxx>
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-rcar.c | 265 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 277 insertions(+)
> create mode 100644 drivers/pwm/pwm-rcar.c
Found a couple more things. No need to respin for any of these, I can
make the changes when applying, but I'd like confirmation on a couple
of things below.
[...]
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
[...]
> +static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
> + int period_ns)
> +{
> + unsigned long long one_cycle, tmp; /* 0.01 nanoseconds */
I'm not quite sure why you need the extra multiplication and division by
100 here. Is this for extra accuracy?
> + unsigned long clk_rate = clk_get_rate(rp->clk);
> + u32 cyc, ph;
> +
> + one_cycle = (unsigned long long)NSEC_PER_SEC * 100ULL * (1 << div);
> + do_div(one_cycle, clk_rate);
> +
> + tmp = period_ns * 100ULL;
> + do_div(tmp, one_cycle);
> + cyc = (tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
> +
> + tmp = duty_ns * 100ULL;
> + do_div(tmp, one_cycle);
> + ph = tmp & RCAR_PWMCNT_PH0_MASK;
> +
> + /* Avoid prohibited setting */
> + if (cyc != 0 && ph != 0) {
> + rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> + return 0;
> + } else {
> + return -EINVAL;
> + }
I think the ordering here is unintuitive, better would be:
if (cyc == 0 || ph == 0)
return -EINVAL;
rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
return 0;
> +static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
> +{
> + struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> + int div = rcar_pwm_get_clock_division(rp, period_ns);
> + int ret;
> +
> + if (div < 0)
> + return div;
> +
> + /* Let the core driver set pwm->period if disabled and duty_ns == 0 */
> + if (!test_bit(PWMF_ENABLED, &pwm->flags) && !duty_ns)
> + return 0;
> +
> + rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> + ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
> + rcar_pwm_set_clock_control(rp, div);
> + rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
Just making sure: is it correct to execute the above two lines even if
ret < 0?
> +static int rcar_pwm_probe(struct platform_device *pdev)
> +{
> + struct rcar_pwm_chip *rcar_pwm;
> + struct resource *res;
> + int ret;
> +
> + rcar_pwm = devm_kzalloc(&pdev->dev, sizeof(*rcar_pwm), GFP_KERNEL);
> + if (rcar_pwm == NULL)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + rcar_pwm->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(rcar_pwm->base))
> + return PTR_ERR(rcar_pwm->base);
> +
> + rcar_pwm->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(rcar_pwm->clk)) {
> + dev_err(&pdev->dev, "cannot get clock\n");
> + return PTR_ERR(rcar_pwm->clk);
> + }
> +
> + platform_set_drvdata(pdev, rcar_pwm);
> +
> + rcar_pwm->chip.dev = &pdev->dev;
> + rcar_pwm->chip.ops = &rcar_pwm_ops;
> + rcar_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
This seems to be missing a:
rcar_pwm->chip.of_pwm_n_cells = 3;
?
Thierry
Attachment:
signature.asc
Description: PGP signature
