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
