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

[Bastien Curutchet]

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