Hi Tomasz,
[...]
> > > +Required properties:
> > > +- compatible : should be one of following:
> > > + samsung,s3c24xx-pwm-timer - for 16-bit timers present on S3C24xx
> > > + samsung,s3c64xx-pwm-timer - for 32-bit timers present on S3C64xx
> > > and newer +- reg: base address and size of register area
> > > +- interrupts: interrupt list for all five PWM timers.
> >
> > Is this a set of combined interrupts, or one per timer?
>
> It is one per timer, however the node represents a single PWM timer block
> that contains several timers (usually 5).
>
> > Which order are they in?
> >
> > Assuming they're one per timer, in order, how about something like:
> >
> > "- interrupts: one interrupt per timer, starting at timer 0".
>
> Sounds fine to me. I will modify the description in next version.
Great.
> > > +- samsung,source-timer: index of timer to be used as clocksource
> > > +- samsung,event-timer: index of timer to be used as clock event
> >
> > Is there any reason this needs to be specified in the dt?
>
> Yes. On some SoCs selected channels of PWM block can be used as PWM
> outputs and so thsoe cannot be used for system timers. This property makes
> it possible to specify channels used as system timers on particular
> platform (board).
>
> > Can the driver not just select two arbitrary timers from the hardware?
>
> In most cases I could statically choose channel 3 and 4 as it was done
> before my patches on S3C24xx and S3C64xx. I can make those channels
> default if others are not specified in properties.
That would be nice, it would mean we could have some less verbose dts :)
[...]
> > > - timer_base = ioremap_nocache(timer_variant.reg_base, SZ_4K);
> > > - if (!timer_base)
> > > - panic("failed to map timer registers");
> > > + if (!timer_base) {
> > > + timer_base = ioremap_nocache(timer_variant.reg_base, SZ_4K);
> > > + if (!timer_base)
> > > + panic("failed to map timer registers");
> > > + }
> >
> > You map 4K here, but the binding didn't mention that 4K is always the
> > expected size of the reg.
>
> This is a compatibility mapping for legacy (non-DT) platforms that will be
> removed once all platforms get moved to DT.
Sounds reasonable.
[...]
> > > + if (of_property_read_u32(np, "samsung,source-timer", &val))
> > > + panic("no samsung,source-timer property provided");
> > > + if (val > ARRAY_SIZE(timer_variant.irqs))
> > > + panic("samsung,source-timer property out of range");
> >
> > This check doesn't tell you if you actually had an irq in the dt for the
> > timer at that index.
>
> Hmm, this line is probably a bit confusing. I will add a define with
> maximum number of channels and use it here. This is only a check whether
> the index given is not out of range.
Ok.
> > Do you really need to panic here? Can you not just warn?
> >
> > What if a future platform has another timer driver that can at least get
> > the board to boot?
>
> It's rather unlikely that new platforms using samsung-time will show up.
> This clocksource is used only for non-SMP platforms based on S3C and S5P
> SoCs, where it is the only possible supported clocksource.
Ok.
[...]
> > > + if (!of_property_read_u32(np, "samsung,divisor", &val)) {
> > > + if (val > 16 || (1 << (fls(val) - 1)) != val)
> > > + panic("divsor must be 1, 2, 4, 8 or 16");
> > > + timer_variant.divisor = timer_variant.prescale * val;
> >
> > Maybe it's just me, but I find this somewhat difficult to read.
> >
> > How about something like:
> >
> > switch (val) {
> > case 1:
> > case 2:
> > case 4:
> > case 8:
> > case 16:
> > timer_variant.divisor = timer_variant.prescale * val;
> > break;
> > default:
> > warn("no divisor specified");
> > };
>
> It looks better indeed. I will change this part in next version.
Great.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
