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

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

Hi Krzysztof,

On 11/21/24 11:33 AM, Krzysztof Kozlowski wrote:
> 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.

Ok, I'll use EXPORT_SYMBOL_GPL in next iteration.

> 
> 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.
> 

Do you know a tool that could help me checking the kernel doc style ? I 
have no warning when running ```make W=1n drivers/memory/ti-aemif.o```. 
The warnings I get when running ```make W=2 drivers/memory/ti-aemif.o``` 
aren't linked to these comments. And the output I get with 
```./scripts/kernel-doc -export  drivers/memory/ti-aemif.c``` seems fine 
to me.

>>   
>>   /**
>>    * 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
> 

Ok, I'll do it in next iteration.


Best regards,
Bastien