Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 19 Apr 2021 08:20:01 +0900
From:   Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
To:     Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Lee Jones <lee.jones@linaro.org>, devicetree@vger.kernel.org,
        linux-pwm@vger.kernel.org, punit1.agrawal@toshiba.co.jp,
        yuji2.ishikawa@toshiba.co.jp, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM
 support
Message-ID: <20210418232001.lvx7ho2qo2ac2khy@toshiba.co.jp>
References: <20210418110904.1942806-1-nobuhiro1.iwamatsu@toshiba.co.jp>
 <20210418110904.1942806-3-nobuhiro1.iwamatsu@toshiba.co.jp>
 <20210418134411.vfltokielrwuygqa@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210418134411.vfltokielrwuygqa@pengutronix.de>
Precedence: bulk

Hi Uwe,

Thanks for your review.

On Sun, Apr 18, 2021 at 03:44:11PM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> just a few smaller issues left to fix.
> 
> On Sun, Apr 18, 2021 at 08:09:04PM +0900, Nobuhiro Iwamatsu wrote:
> > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> > new file mode 100644
> > index 000000000000..166b18ac1a3a
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-visconti.c
> > @@ -0,0 +1,188 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Toshiba Visconti pulse-width-modulation controller driver
> > + *
> > + * Copyright (c) 2020 TOSHIBA CORPORATION
> > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> 
> We're in 2021, so you might want to adapt the year in the copy right
> notice.

OK, I will update.

> 
> > + *
> > + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > + *
> > + * Limitations:
> > + * - PIPGM_PWMC is a 2-bit divider (00: 1, 01: 2, 10: 4, 11: 8).
> 
> This is too detailed for the purpose of this section. Please either drop
> it or make this:
> 
>  - The fixed input clock is running at 1 MHz and is divided by either 1,
>    2, 4 or 8.


OK, I will add your sugggestion.

> 
> > + * - Fixed input clock running at 1 MHz.
> > + * - When the settings of the PWM are modified, the new values are shadowed
> > + *   in hardware until the PIPGM_PCSR register is written and the currently
> > + *   running period is completed. This way the hardware switches atomically
> > + *   from the old setting to the new.
> > + * - Disabling the hardware completes the currently running period and keeps
> > + *   the output at low level at all times.
> > + */
> > +
> > [...]
> > +	/*
> > +	 * PWMC controls a divider that divides the input clk by a
> > +	 * power of two between 1 and 8. As a smaller divider yields
> > +	 * higher precision, pick the smallest possible one.
> > +	 */
> > +	if (period > 0xffff) {
> > +		pwmc0 = ilog2(period >> 16);
> > +		BUG_ON(pwmc0 > 3);
> > +	} else
> > +		pwmc0 = 0;
> 
> The linux coding style mandates that you should use braces for both
> branches. (i.e.
> 
> +	if (period > 0xffff) {
> +		pwmc0 = ilog2(period >> 16);
> +		BUG_ON(pwmc0 > 3);
> +	} else {
> +		pwmc0 = 0;
> +	}
> )

Oh, I fotgot it, I will fix this. Thanks you.

> 
> > +	period >>= pwmc0;
> > +	duty_cycle >>= pwmc0;
> > +
> > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > +		pwmc0 |= PIPGM_PWMC_PWMACT;
> > +	writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm));
> > +	writel(duty_cycle, priv->base + PIPGM_PDUT(pwm->hwpwm));
> > +	writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm));
> > +
> > +	return 0;
> > +}
> 
> Best regards
> Uwe


Best regards,
  Nobuhiro