Re: [PATCH 2/3] ARM: at91/tclib: move initialization from alloc to probe

[Boris BREZILLON <boris.brezillon@free-electrons.com>]

Hi Gael,

On Wed, 20 Aug 2014 00:07:51 +0200
Gaël PORTAY <gael.portay@gmail.com> wrote:

> Move resource retrieval from atmel_tc_alloc to tc_probe to avoid lately
> reporting resource related issues when a TC block user request a TC block.
> 
> Moreover, resources retrieval are usually done in the probe function,
> thus moving them add some consistency with other drivers.
> 
> Initialization is done once, ie not every time a tc block is requested.
> If it fails, the device is not appended to the list of tc blocks.
> 
> Furhermore, the device id is retrieved at probe as well, avoiding parsing
> DT every time the user requests of tc block.
> 
> Signed-off-by: Gaël PORTAY <gael.portay@gmail.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/clocksource/tcb_clksrc.c |  2 +-
>  drivers/misc/atmel_tclib.c       | 71 +++++++++++++---------------------------
>  drivers/pwm/pwm-atmel-tcb.c      |  2 +-
>  include/linux/atmel_tc.h         |  8 +++--
>  4 files changed, 29 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
> index a8d7ea1..f922e81 100644
> --- a/drivers/clocksource/tcb_clksrc.c
> +++ b/drivers/clocksource/tcb_clksrc.c
> @@ -279,7 +279,7 @@ static int __init tcb_clksrc_init(void)
>  	int i;
>  	int ret;
>  
> -	tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK, clksrc.name);
> +	tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK);
>  	if (!tc) {
>  		pr_debug("can't alloc TC for clocksource\n");
>  		return -ENODEV;
> diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c
> index b514a2d..d505d1e 100644
> --- a/drivers/misc/atmel_tclib.c
> +++ b/drivers/misc/atmel_tclib.c
> @@ -35,60 +35,31 @@ static LIST_HEAD(tc_list);
>  /**
>   * atmel_tc_alloc - allocate a specified TC block
>   * @block: which block to allocate
> - * @name: name to be associated with the iomem resource
>   *
>   * Caller allocates a block.  If it is available, a pointer to a
>   * pre-initialized struct atmel_tc is returned. The caller can access
>   * the registers directly through the "regs" field.
>   */
> -struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name)
> +struct atmel_tc *atmel_tc_alloc(unsigned block)
>  {
>  	struct atmel_tc		*tc;
>  	struct platform_device	*pdev = NULL;
> -	struct resource		*r;
> -	size_t			size;
>  
>  	spin_lock(&tc_list_lock);
>  	list_for_each_entry(tc, &tc_list, node) {
> -		if (tc->pdev->dev.of_node) {
> -			if (of_alias_get_id(tc->pdev->dev.of_node, "tcb")
> -					== block) {
> -				pdev = tc->pdev;
> -				break;
> -			}
> -		} else if (tc->pdev->id == block) {
> +		if (tc->allocated)
> +			continue;
> +
> +		if ((tc->pdev->dev.of_node && tc->id == block) ||
> +		    (tc->pdev->id == block)) {
>  			pdev = tc->pdev;
> +			tc->allocated = true;
>  			break;
>  		}
>  	}
> -
> -	if (!pdev || tc->iomem)
> -		goto fail;
> -
> -	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!r)
> -		goto fail;
> -
> -	size = resource_size(r);
> -	r = request_mem_region(r->start, size, name);
> -	if (!r)
> -		goto fail;
> -
> -	tc->regs = ioremap(r->start, size);
> -	if (!tc->regs)
> -		goto fail_ioremap;
> -
> -	tc->iomem = r;
> -
> -out:
>  	spin_unlock(&tc_list_lock);
> -	return tc;
>  
> -fail_ioremap:
> -	release_mem_region(r->start, size);
> -fail:
> -	tc = NULL;
> -	goto out;
> +	return pdev ? tc : NULL;
>  }
>  EXPORT_SYMBOL_GPL(atmel_tc_alloc);
>  
> @@ -96,19 +67,14 @@ EXPORT_SYMBOL_GPL(atmel_tc_alloc);
>   * atmel_tc_free - release a specified TC block
>   * @tc: Timer/counter block that was returned by atmel_tc_alloc()
>   *
> - * This reverses the effect of atmel_tc_alloc(), unmapping the I/O
> - * registers, invalidating the resource returned by that routine and
> - * making the TC available to other drivers.
> + * This reverses the effect of atmel_tc_alloc(), invalidating the resource
> + * returned by that routine and making the TC available to other drivers.
>   */
>  void atmel_tc_free(struct atmel_tc *tc)
>  {
>  	spin_lock(&tc_list_lock);
> -	if (tc->regs) {
> -		iounmap(tc->regs);
> -		release_mem_region(tc->iomem->start, resource_size(tc->iomem));
> -		tc->regs = NULL;
> -		tc->iomem = NULL;
> -	}
> +	if (tc->allocated)
> +		tc->allocated = false;
>  	spin_unlock(&tc_list_lock);
>  }
>  EXPORT_SYMBOL_GPL(atmel_tc_free);
> @@ -142,9 +108,7 @@ static int __init tc_probe(struct platform_device *pdev)
>  	struct atmel_tc *tc;
>  	struct clk	*clk;
>  	int		irq;
> -
> -	if (!platform_get_resource(pdev, IORESOURCE_MEM, 0))
> -		return -EINVAL;
> +	struct resource	*r;
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
> @@ -160,12 +124,21 @@ static int __init tc_probe(struct platform_device *pdev)
>  	if (IS_ERR(clk))
>  		return PTR_ERR(clk);
>  
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	tc->regs = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(tc->regs))
> +		return PTR_ERR(tc->regs);
> +
>  	/* Now take SoC information if available */
>  	if (pdev->dev.of_node) {
>  		const struct of_device_id *match;
>  		match = of_match_node(atmel_tcb_dt_ids, pdev->dev.of_node);
>  		if (match)
>  			tc->tcb_config = match->data;
> +
> +		tc->id = of_alias_get_id(tc->pdev->dev.of_node, "tcb");
> +	} else {
> +		tc->id = pdev->id;
>  	}
>  
>  	tc->clk[0] = clk;
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index f3dcd02..d56e5b7 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -379,7 +379,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	tc = atmel_tc_alloc(tcblock, "tcb-pwm");
> +	tc = atmel_tc_alloc(tcblock);
>  	if (tc == NULL) {
>  		dev_err(&pdev->dev, "failed to allocate Timer Counter Block\n");
>  		return -ENOMEM;
> diff --git a/include/linux/atmel_tc.h b/include/linux/atmel_tc.h
> index 89a931b..d8aa884 100644
> --- a/include/linux/atmel_tc.h
> +++ b/include/linux/atmel_tc.h
> @@ -44,12 +44,13 @@ struct atmel_tcb_config {
>  /**
>   * struct atmel_tc - information about a Timer/Counter Block
>   * @pdev: physical device
> - * @iomem: resource associated with the I/O register
>   * @regs: mapping through which the I/O registers can be accessed
> + * @id: block id
>   * @tcb_config: configuration data from SoC
>   * @irq: irq for each of the three channels
>   * @clk: internal clock source for each of the three channels
>   * @node: list node, for tclib internal use
> + * @allocated: if already used, for tclib internal use
>   *
>   * On some platforms, each TC channel has its own clocks and IRQs,
>   * while on others, all TC channels share the same clock and IRQ.
> @@ -61,15 +62,16 @@ struct atmel_tcb_config {
>   */
>  struct atmel_tc {
>  	struct platform_device	*pdev;
> -	struct resource		*iomem;
>  	void __iomem		*regs;
> +	int                     id;
>  	const struct atmel_tcb_config *tcb_config;
>  	int			irq[3];
>  	struct clk		*clk[3];
>  	struct list_head	node;
> +	bool			allocated;
>  };
>  
> -extern struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name);
> +extern struct atmel_tc *atmel_tc_alloc(unsigned block);
>  extern void atmel_tc_free(struct atmel_tc *tc);
>  
>  /* platform-specific ATMEL_TC_TIMER_CLOCKx divisors (0 means 32KiHz) */



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com