Re: [PATCH v4 06/10] memory: ti-aemif: Export aemif_*_cs_timings()

[Krzysztof Kozlowski <krzk@kernel.org>]

On 15/11/2024 14:26, Bastien Curutchet wrote:
>  	return 0;
>  }
> +EXPORT_SYMBOL(aemif_check_cs_timings);
>  
>  /**
>   * aemif_set_cs_timings - Set the timing configuration of a given chip select.
> @@ -173,7 +158,7 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
>   *
>   * @return: 0 on success, else negative errno.
>   */
> -static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_cs_timings *timings)
> +int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_cs_timings *timings)
>  {
>  	unsigned int offset;
>  	u32 val, set;
> @@ -195,13 +180,16 @@ static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_
>  
>  	offset = A1CR_OFFSET + cs * 4;
>  
> +	mutex_lock(&aemif->config_cs_lock);
>  	val = readl(aemif->base + offset);
>  	val &= ~TIMINGS_MASK;
>  	val |= set;
>  	writel(val, aemif->base + offset);
> +	mutex_unlock(&aemif->config_cs_lock);
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(aemif_set_cs_timings);


EXPORT_SYMBOL_GPL everywhere, these are quite specific to driver's
internals, so internal implementation.

Also, all of exported functions need to have correct kerneldoc but I
think they don't. At least missing(), so maybe rest also did not conform
to kernel doc style.

>  
>  /**
>   * aemif_calc_rate - calculate timing data.
> @@ -257,10 +245,12 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
>  	if (data->enable_ss)
>  		set |= ACR_SSTROBE_MASK;
>  
> +	mutex_lock(&aemif->config_cs_lock);
>  	val = readl(aemif->base + offset);
>  	val &= ~CONFIG_MASK;
>  	val |= set;
>  	writel(val, aemif->base + offset);
> +	mutex_unlock(&aemif->config_cs_lock);
>  
>  	return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &data->timings);
>  }
> @@ -399,6 +389,7 @@ static int aemif_probe(struct platform_device *pdev)
>  	if (IS_ERR(aemif->base))
>  		return PTR_ERR(aemif->base);
>  
> +	mutex_init(&aemif->config_cs_lock);
>  	if (np) {
>  		/*
>  		 * For every controller device node, there is a cs device node
> diff --git a/include/linux/memory/ti-aemif.h b/include/linux/memory/ti-aemif.h
> new file mode 100644
> index 000000000000..0640d30f6321
> --- /dev/null
> +++ b/include/linux/memory/ti-aemif.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __TI_AEMIF_H
> +#define __TI_AEMIF_H


Use some longer header guard, e.g. __MEMORY_TI_AEMIF_H

> +
> +/**



Best regards,
Krzysztof