On lun, ago 18, 2014 at 05:27:26 +0100, Mark Rutland wrote:
> On Sun, Aug 17, 2014 at 11:49:50AM +0100, Carlo Caione wrote:
> > +enum {
> > + A = 0,
> > + B,
> > + C,
> > + D,
> > +};
>
> That's a very terse set of enum names. I would recomment something a
> little longer.
>
> Any reason for missing E?
TIMER_E is a slightly different timer so I preferred to leave it apart.
> > +#define TIMER_ISA_MUX 0
> > +#define TIMER_ISA_E_VAL 0x14
> > +#define TIMER_ISA_t_VAL(t) ((t + 1) << 2)
> > +
> > +#define TIMER_t_INPUT_BIT(t) (2 * t)
> > +#define TIMER_E_INPUT_BIT 8
> > +#define TIMER_t_INPUT_MASK(t) (3UL << TIMER_t_INPUT_BIT(t))
> > +#define TIMER_E_INPUT_MASK (7UL << TIMER_E_INPUT_BIT)
> > +#define TIMER_t_ENABLE_BIT(t) (16 + t)
> > +#define TIMER_E_ENABLE_BIT 20
> > +#define TIMER_t_PERIODIC_BIT(t) (12 + t)
>
> While I don't think it matters for any of these, it's usually good
> practice to add parentheses around arguments, e.g.
>
> #define FOO(x) ((x) * 2)
>
> So you can avoid bad expansions in cases like:
>
> FOO(reg + 4)
Agree. I'll change it.
> > +
> > +#define TIMER_UNIT_1us 0
> > +#define TIMER_E_UNIT_1us 1
> > +
> > +static void __iomem *timer_base;
> > +
> > +static cycle_t cycle_read_timer_e(struct clocksource *cs)
> > +{
> > + return (cycle_t)readl(timer_base + TIMER_ISA_E_VAL);
> > +}
> > +
> > +static struct clocksource clocksource_timer_e = {
> > + .name = "meson6_timerE",
>
> Do we really care which specific timer we're using within the block?
>
> Why not just "meson6_clocksource"?
Just a reminder of which timer we are using. I'll fix it.
> > + .rating = 300,
> > + .read = cycle_read_timer_e,
> > + .mask = CLOCKSOURCE_MASK(32),
> > + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> > +};
>
> [...]
>
> > +static irqreturn_t meson6_timer_interrupt(int irq, void *dev_id)
> > +{
> > + struct clock_event_device *evt = (struct clock_event_device *)dev_id;
> > +
> > + evt->event_handler(evt);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static struct irqaction meson6_timer_irq = {
> > + .name = "meson6_timerA",
>
> Similarly to the clocksource naming, this might be better as
> "meson6_timer", without the 'A'.
Yep.
Thanks for the review,
--
Carlo Caione
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
