Hi Geert-san,
Thank you for your review!
> Sent: Friday, May 15, 2015 1:07 AM
>
> Hi Shimoda-san,
>
> On Wed, May 13, 2015 at 11:27 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > R-Car SoCs have a seven-channel pulse width modulation (PWM) timer.
> > This driver adds support for the PWM Timer as a single PWM chip and
> > seven PWM devices.
>
> Thanks for your patch!
>
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index b1541f4..7e98175 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -249,6 +249,17 @@ config PWM_PXA
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-pxa.
> >
> > +config PWM_RCAR
> > + tristate "Renesas R-Car PWM support"
> > + depends on ARCH_SHMOBILE || COMPILE_TEST
>
> Could be instead
>
> depends on ARCH_RCAR_GEN1 || ARCH_RCAR_GEN2 || COMPILE_TEST
I will fix this.
> > + depends on HAS_IOMEM
> > + help
> > + This driver exposes the PWM Timer controller found in Renesas
> > + chips through the PWM API.
>
> "Renesas R-Car chips" (e.g. R-Mobile uses pwm-renesas-tpu).
I will fix this.
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rcar.c
< snip >
> > +#define NUM_RCAR_PWM_CHANNELS 7
>
> I'm wondering whether this should be a DT property, to prepare for
> future expansion.
> Alternatively, as these are really 7 individual channels, with 7 individual
> register blocks spaced 0x1000 apart, you could have 7 individual device nodes,
> each driving one pwm output. That would probably incur more overhead, as
> there would be 7 individual pwm controllers. But it does allow for future
> expansion, and allows to cope when the individual register blocks are moved
> in the SoC address space (I can imagine that on old SoCs e.g. all SCIF
> blocks were nicely lay out in the address space, while now they have arbitrary
> base addresses).
>
> What do other people think?
My initial development, I wrote 7 individual device nodes. However, after the driver
was proved, the sysfs for pwm is:
/sys/class/pwm/pwmchip0/pwm0
/sys/class/pwm/pwmchip1/pwm0
...
/sys/class/pwm/pwmchip6/pwm0
I felt this was strange numbering a little.
So, I changed the style to one pwm chip and 7 pwm channels:
/sys/class/pwm/pwmchip0/pwm0
/sys/class/pwm/pwmchip0/pwm1
...
/sys/class/pwm/pwmchip0/pwm6
I'm not sure which style is good for the PWM IP.
> > +static int rcar_pwn_get_clock_division(struct rcar_pwm_chip *rp,
> > + int period_ns)
> > +{
> > + int div;
> > + unsigned long clk_rate = clk_get_rate(rp->clk);
> > + unsigned long long max; /* max cycle / nanoseconds */
> > +
> > + for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> > + max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> > + do_div(max, clk_rate / (1 << div));
>
> Dividing clk_rate by "1 << div" reduces accuracy a lot for large values of div.
> "NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE * (1 << div)" just fits in 64-bit,
> so you can use
>
> max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> (1 << div);
> do_div(max, clk_rate);
Thank you very much for the suggestion. I will fix this in v2.
> > +static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int channel,
> > + int div, int duty_ns, int period_ns)
> > +{
> > + unsigned long long one_cycle, tmp; /* 0.01 nanoseconds */
> > + unsigned long clk_rate = clk_get_rate(rp->clk);
> > + u32 cyc, ph;
> > +
> > + one_cycle = (unsigned long long)NSEC_PER_SEC * 100;
> > + do_div(one_cycle, clk_rate / (1 << div));
>
> Same here:
>
> one_cycle = (unsigned long long)NSEC_PER_SEC * 100 * (1 << div)
> do_div(one_cycle, clk_rate);
I will fix this in v2.
Best regards,
Yoshihiro Shimoda
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
