Re: [PATCH v5 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support

[Nobuhiro Iwamatsu]

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@xxxxxxxxxxxxx>
> > + *
> > + * 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