RE: [PATCH v2 2/2] clocksource: Add Intel Keem Bay Timer Support

[Ayyathurai, Vijayakannan]

Hi Daniel,

> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@xxxxxxxxx>
> 
> [ ... ]
> 
> > +static struct timer_of keembay_ce_to = {
> > +	.flags	= TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
> > +	.clkevt = {
> > +		.name			= "keembay_sys_clkevt",
> > +		.features		= CLOCK_EVT_FEAT_PERIODIC |
> > +					  CLOCK_EVT_FEAT_ONESHOT  |
> > +					  CLOCK_EVT_FEAT_DYNIRQ,
> > +		.rating			= TIM_RATING,
> > +		.set_next_event		=
> keembay_timer_set_next_event,
> > +		.set_state_periodic	= keembay_timer_periodic,
> > +		.set_state_shutdown	= keembay_timer_shutdown,
> > +	},
> > +	.of_base = {
> > +		.index = 0,
> > +	},
> > +	.of_irq = {
> > +		.handler = keembay_timer_isr,
> > +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
> 
> Is the IRQPOLL flag really needed here ?
> 

Not really needed. I will remove this redundant Flag in my next version.

> > +static int __init keembay_timer_init(struct device_node *np)
> > +{
> > +	struct keembay_init_data data;
> > +	int ret;
> > +
> > +	data.base = of_iomap(np, 2);
> > +	if (!data.base)
> > +		return -ENXIO;
> > +
> > +	ret = keembay_clocksource_init(np, &data);
> > +	if (ret)
> > +		goto exit;
> > +
> > +	ret = keembay_clockevent_init(np, &data);
> 
> Is this missing ?
> 

Yes. Either case it goes to the exit path. So I thought of avoiding this error handling code.

> 	if (ret)
> 		goto exit;
> 
> 	return 0;
> 
> > +
> > +exit:
> > +	keembay_timer_cleanup(np, &data);
> > +
> > +	return ret;
> > +}
> > +
> > +TIMER_OF_DECLARE(keembay_timer, "intel,keembay-timer",
> keembay_timer_init);
> >
> 

Thanks,
Vijay