Re: [PATCH v5 1/6] ARM: at91: add TCB registers definitions

[Alexandre Belloni]

On 28/06/2018 17:15:39+0200, Daniel Lezcano wrote:
> On 19/06/2018 23:19, Alexandre Belloni wrote:
> > Add registers and bits definitions for the timer counter blocks found on
> > Atmel ARM SoCs.
> > 
> > Tested-by: Alexander Dahl <ada@xxxxxxxxxxx>
> > Tested-by: Andras Szemzo <szemzo.andras@xxxxxxxxx>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> > ---
> >  include/soc/at91/atmel_tcb.h | 216 +++++++++++++++++++++++++++++++++++
> 
> Is the header necessary ? Can it be moved in the .c ?
> 

Ultimately, the clocksource driver will not be the only one to use it.
There is the pwm driver that will be converted (it was converted in the
first version of the series). and then there is a counter driver that
will be submitted once the subsystem is upstreamed.

> >  1 file changed, 216 insertions(+)
> >  create mode 100644 include/soc/at91/atmel_tcb.h
> > 
> > diff --git a/include/soc/at91/atmel_tcb.h b/include/soc/at91/atmel_tcb.h
> > new file mode 100644
> > index 000000000000..3ed66031fc76
> > --- /dev/null
> 
> [ ... ]
> 
> > +static inline struct clk *tcb_clk_get(struct device_node *node, int channel)
> > +{
> > +	struct clk *clk;
> > +	char clk_name[] = "t0_clk";
> > +
> > +	clk_name[1] += channel;
> 
> clever :)
> 
> > +	clk = of_clk_get_by_name(node->parent, clk_name);
> > +	if (!IS_ERR(clk))
> > +		return clk;
> > +
> > +	return of_clk_get_by_name(node->parent, "t0_clk");
> 
> Why do you want to return clk from t0_clk if another channel is
> requested ? This is prone to error.

The newer TCBs only have one peripheral clocks. The current DT binding only
have t0_clk in that case so whatever the channel, t0_clk is the correct
one.

> 
> I would clarify that at the caller level, if tcb_clk_get fails then try
> with channel zero.

This was hidden from the individual drivers by tclib but this can be
open coded in the drivers.

> 
> > +}
> > +
> > +static inline int tcb_irq_get(struct device_node *node, int channel)
> 
> no inline
> 

IIRC, removing the inline will make linking the kernel fail when there
is more than 2 drivers using the TCBs but I'll try again. Or I can
remove both those functions and open code as you suggest.

> > +{
> > +	int irq;
> > +
> > +	irq = of_irq_get(node->parent, channel);
> > +	if (irq > 0)
> > +		return irq;
> > +
> > +	return of_irq_get(node->parent, 0);
> 
> Same comment than above.
> 
> > +}
> > +
> > +static const u8 atmel_tc_divisors[5] = { 2, 8, 32, 128, 0, };
> > +
> > +struct atmel_tcb_info {
> > +	int bits;
> > +};
> > +
> > +static const struct atmel_tcb_info atmel_tcb_infos[] = {
> > +	{ .bits = 16 },
> > +	{ .bits = 32 },
> > +};
> 
> Structuring the code with structure is a good practice. However, this is
> too much :)
> 

I was going to add the divisor there but as AVR32 is gone, this is
indeed unnecessary.

> > +static const struct of_device_id atmel_tcb_dt_ids[] = {
> > +	{
> > +		.compatible = "atmel,at91rm9200-tcb",
> > +		.data = &atmel_tcb_infos[0],
> 
> 		.data = (void *)16;
> 
> > +	}, {
> > +		.compatible = "atmel,at91sam9x5-tcb",
> > +		.data = &atmel_tcb_infos[1],
> > +	}, {
> > +		/* sentinel */
> > +	}
> > +};
> > +
> 
> 
> 
> > +#endif /* __SOC_ATMEL_TCB_H */
> > 
> 
> 
> -- 
>  <http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com