[v4,12/24] PM / devfreq: tegra30: Inline all one-line functions
diff mbox series

Message ID 20190707223303.6755-13-digetx@gmail.com
State New
Headers show
Series
  • More improvements for Tegra30 devfreq driver
Related show

Commit Message

Dmitry Osipenko July 7, 2019, 10:32 p.m. UTC
Depending on a kernel's configuration, a single line functions may not be
inlined by compiler (like enabled ftracing for example). Let's inline such
functions explicitly for consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Chanwoo Choi July 16, 2019, 12:26 p.m. UTC | #1
Hi Dmitry,

I'm not sure that it is necessary.
As I knew, usally, the 'inline' is used on header file
to define the empty functions.

Do we have to change it with 'inline' keyword?

On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> Depending on a kernel's configuration, a single line functions may not be
> inlined by compiler (like enabled ftracing for example). Let's inline such
> functions explicitly for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index c6c4a07d3e07..1a10df5dbbed 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -181,28 +181,29 @@ static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
>  	{  250000,    100000 },
>  };
>  
> -static u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
> +static inline u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
>  {
>  	return readl_relaxed(tegra->regs + offset);
>  }
>  
> -static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
> +static inline void actmon_writel(struct tegra_devfreq *tegra,
> +				 u32 val, u32 offset)
>  {
>  	writel_relaxed(val, tegra->regs + offset);
>  }
>  
> -static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
> +static inline u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
>  {
>  	return readl_relaxed(dev->regs + offset);
>  }
>  
> -static void device_writel(struct tegra_devfreq_device *dev, u32 val,
> -			  u32 offset)
> +static inline void device_writel(struct tegra_devfreq_device *dev,
> +				 u32 val, u32 offset)
>  {
>  	writel_relaxed(val, dev->regs + offset);
>  }
>  
> -static unsigned long do_percent(unsigned long val, unsigned int pct)
> +static inline unsigned long do_percent(unsigned long val, unsigned int pct)
>  {
>  	return val * pct / 100;
>  }
>
Dmitry Osipenko July 16, 2019, 1:35 p.m. UTC | #2
16.07.2019 15:26, Chanwoo Choi пишет:
> Hi Dmitry,
> 
> I'm not sure that it is necessary.
> As I knew, usally, the 'inline' is used on header file
> to define the empty functions.
> 
> Do we have to change it with 'inline' keyword?

The 'inline' attribute tells compiler that instead of jumping into the
function, it should take the function's code and replace the function's
invocation with that code. This is done in order to help compiler
optimize code properly, please see [1]. There is absolutely no need to
create a function call into a function that consists of a single
instruction.

[1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
Chanwoo Choi July 18, 2019, 9:09 a.m. UTC | #3
On 19. 7. 16. 오후 10:35, Dmitry Osipenko wrote:
> 16.07.2019 15:26, Chanwoo Choi пишет:
>> Hi Dmitry,
>>
>> I'm not sure that it is necessary.
>> As I knew, usally, the 'inline' is used on header file
>> to define the empty functions.
>>
>> Do we have to change it with 'inline' keyword?
> 
> The 'inline' attribute tells compiler that instead of jumping into the
> function, it should take the function's code and replace the function's
> invocation with that code. This is done in order to help compiler
> optimize code properly, please see [1]. There is absolutely no need to
> create a function call into a function that consists of a single
> instruction.
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
> 

If you want to add 'inline' keyword, I recommend that 
you better to remove the modified function in this patch
and then just call the 'write_relaxed or read_relaxed' function
directly. It is same result when using inline keyword.
Dmitry Osipenko July 19, 2019, 1:22 a.m. UTC | #4
В Thu, 18 Jul 2019 18:09:05 +0900
Chanwoo Choi <cw00.choi@samsung.com> пишет:

> On 19. 7. 16. 오후 10:35, Dmitry Osipenko wrote:
> > 16.07.2019 15:26, Chanwoo Choi пишет:  
> >> Hi Dmitry,
> >>
> >> I'm not sure that it is necessary.
> >> As I knew, usally, the 'inline' is used on header file
> >> to define the empty functions.
> >>
> >> Do we have to change it with 'inline' keyword?  
> > 
> > The 'inline' attribute tells compiler that instead of jumping into
> > the function, it should take the function's code and replace the
> > function's invocation with that code. This is done in order to help
> > compiler optimize code properly, please see [1]. There is
> > absolutely no need to create a function call into a function that
> > consists of a single instruction.
> > 
> > [1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
> >   
> 
> If you want to add 'inline' keyword, I recommend that 
> you better to remove the modified function in this patch
> and then just call the 'write_relaxed or read_relaxed' function
> directly. It is same result when using inline keyword.

That could be done, but it makes code less readable.

See the difference:

device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);

writel_relaxed(ACTMON_INTR_STATUS_CLEAR,
	       dev->regs + ACTMON_DEV_INTR_STATUS);
Chanwoo Choi July 19, 2019, 1:24 a.m. UTC | #5
On 19. 7. 19. 오전 10:22, Dmitry Osipenko wrote:
> В Thu, 18 Jul 2019 18:09:05 +0900
> Chanwoo Choi <cw00.choi@samsung.com> пишет:
> 
>> On 19. 7. 16. 오후 10:35, Dmitry Osipenko wrote:
>>> 16.07.2019 15:26, Chanwoo Choi пишет:  
>>>> Hi Dmitry,
>>>>
>>>> I'm not sure that it is necessary.
>>>> As I knew, usally, the 'inline' is used on header file
>>>> to define the empty functions.
>>>>
>>>> Do we have to change it with 'inline' keyword?  
>>>
>>> The 'inline' attribute tells compiler that instead of jumping into
>>> the function, it should take the function's code and replace the
>>> function's invocation with that code. This is done in order to help
>>> compiler optimize code properly, please see [1]. There is
>>> absolutely no need to create a function call into a function that
>>> consists of a single instruction.
>>>
>>> [1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
>>>   
>>
>> If you want to add 'inline' keyword, I recommend that 
>> you better to remove the modified function in this patch
>> and then just call the 'write_relaxed or read_relaxed' function
>> directly. It is same result when using inline keyword.
> 
> That could be done, but it makes code less readable.
> 
> See the difference:
> 
> device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> 
> writel_relaxed(ACTMON_INTR_STATUS_CLEAR,
> 	       dev->regs + ACTMON_DEV_INTR_STATUS);

No problem if you add the detailed comment and you want to use
the 'inline' keyword.

> 
> 
>
Chanwoo Choi July 19, 2019, 1:27 a.m. UTC | #6
On 19. 7. 19. 오전 10:24, Chanwoo Choi wrote:
> On 19. 7. 19. 오전 10:22, Dmitry Osipenko wrote:
>> В Thu, 18 Jul 2019 18:09:05 +0900
>> Chanwoo Choi <cw00.choi@samsung.com> пишет:
>>
>>> On 19. 7. 16. 오후 10:35, Dmitry Osipenko wrote:
>>>> 16.07.2019 15:26, Chanwoo Choi пишет:  
>>>>> Hi Dmitry,
>>>>>
>>>>> I'm not sure that it is necessary.
>>>>> As I knew, usally, the 'inline' is used on header file
>>>>> to define the empty functions.
>>>>>
>>>>> Do we have to change it with 'inline' keyword?  
>>>>
>>>> The 'inline' attribute tells compiler that instead of jumping into
>>>> the function, it should take the function's code and replace the
>>>> function's invocation with that code. This is done in order to help
>>>> compiler optimize code properly, please see [1]. There is
>>>> absolutely no need to create a function call into a function that
>>>> consists of a single instruction.
>>>>
>>>> [1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
>>>>   
>>>
>>> If you want to add 'inline' keyword, I recommend that 
>>> you better to remove the modified function in this patch
>>> and then just call the 'write_relaxed or read_relaxed' function
>>> directly. It is same result when using inline keyword.
>>
>> That could be done, but it makes code less readable.
>>
>> See the difference:
>>
>> device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
>>
>> writel_relaxed(ACTMON_INTR_STATUS_CLEAR,
>> 	       dev->regs + ACTMON_DEV_INTR_STATUS);
> 
> No problem if you add the detailed comment and you want to use
> the 'inline' keyword.

Basically, I think that 'inline' keyword is not necessary.
But if you want to use 'inline' keyword, I recommend
that call the 'write_relaxed or read_relaxed' function directly
with detailed description. 

> 
>>
>>
>>
> 
>
Dmitry Osipenko July 19, 2019, 2:14 a.m. UTC | #7
В Fri, 19 Jul 2019 10:27:16 +0900
Chanwoo Choi <cw00.choi@samsung.com> пишет:

> On 19. 7. 19. 오전 10:24, Chanwoo Choi wrote:
> > On 19. 7. 19. 오전 10:22, Dmitry Osipenko wrote:  
> >> В Thu, 18 Jul 2019 18:09:05 +0900
> >> Chanwoo Choi <cw00.choi@samsung.com> пишет:
> >>  
> >>> On 19. 7. 16. 오후 10:35, Dmitry Osipenko wrote:  
> >>>> 16.07.2019 15:26, Chanwoo Choi пишет:    
> >>>>> Hi Dmitry,
> >>>>>
> >>>>> I'm not sure that it is necessary.
> >>>>> As I knew, usally, the 'inline' is used on header file
> >>>>> to define the empty functions.
> >>>>>
> >>>>> Do we have to change it with 'inline' keyword?    
> >>>>
> >>>> The 'inline' attribute tells compiler that instead of jumping
> >>>> into the function, it should take the function's code and
> >>>> replace the function's invocation with that code. This is done
> >>>> in order to help compiler optimize code properly, please see
> >>>> [1]. There is absolutely no need to create a function call into
> >>>> a function that consists of a single instruction.
> >>>>
> >>>> [1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
> >>>>     
> >>>
> >>> If you want to add 'inline' keyword, I recommend that 
> >>> you better to remove the modified function in this patch
> >>> and then just call the 'write_relaxed or read_relaxed' function
> >>> directly. It is same result when using inline keyword.  
> >>
> >> That could be done, but it makes code less readable.
> >>
> >> See the difference:
> >>
> >> device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
> >> ACTMON_DEV_INTR_STATUS);
> >>
> >> writel_relaxed(ACTMON_INTR_STATUS_CLEAR,
> >> 	       dev->regs + ACTMON_DEV_INTR_STATUS);  
> > 
> > No problem if you add the detailed comment and you want to use
> > the 'inline' keyword.  
> 
> Basically, I think that 'inline' keyword is not necessary.

Sure, but I'm finding that it's always nicer to explicitly inline a very
simple functions because compiler may not do it properly itself in some
cases.

> But if you want to use 'inline' keyword, I recommend
> that call the 'write_relaxed or read_relaxed' function directly
> with detailed description. 

Could you please reword this sentence? Not sure that I'm understanding
it correctly.
Chanwoo Choi July 19, 2019, 6:01 a.m. UTC | #8
On 19. 7. 19. 오전 11:14, Dmitry Osipenko wrote:
> В Fri, 19 Jul 2019 10:27:16 +0900
> Chanwoo Choi <cw00.choi@samsung.com> пишет:
> 
>> On 19. 7. 19. 오전 10:24, Chanwoo Choi wrote:
>>> On 19. 7. 19. 오전 10:22, Dmitry Osipenko wrote:  
>>>> В Thu, 18 Jul 2019 18:09:05 +0900
>>>> Chanwoo Choi <cw00.choi@samsung.com> пишет:
>>>>  
>>>>> On 19. 7. 16. 오후 10:35, Dmitry Osipenko wrote:  
>>>>>> 16.07.2019 15:26, Chanwoo Choi пишет:    
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> I'm not sure that it is necessary.
>>>>>>> As I knew, usally, the 'inline' is used on header file
>>>>>>> to define the empty functions.
>>>>>>>
>>>>>>> Do we have to change it with 'inline' keyword?    
>>>>>>
>>>>>> The 'inline' attribute tells compiler that instead of jumping
>>>>>> into the function, it should take the function's code and
>>>>>> replace the function's invocation with that code. This is done
>>>>>> in order to help compiler optimize code properly, please see
>>>>>> [1]. There is absolutely no need to create a function call into
>>>>>> a function that consists of a single instruction.
>>>>>>
>>>>>> [1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
>>>>>>     
>>>>>
>>>>> If you want to add 'inline' keyword, I recommend that 
>>>>> you better to remove the modified function in this patch
>>>>> and then just call the 'write_relaxed or read_relaxed' function
>>>>> directly. It is same result when using inline keyword.  
>>>>
>>>> That could be done, but it makes code less readable.
>>>>
>>>> See the difference:
>>>>
>>>> device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
>>>> ACTMON_DEV_INTR_STATUS);
>>>>
>>>> writel_relaxed(ACTMON_INTR_STATUS_CLEAR,
>>>> 	       dev->regs + ACTMON_DEV_INTR_STATUS);  
>>>
>>> No problem if you add the detailed comment and you want to use
>>> the 'inline' keyword.  
>>
>> Basically, I think that 'inline' keyword is not necessary.
> 
> Sure, but I'm finding that it's always nicer to explicitly inline a very
> simple functions because compiler may not do it properly itself in some
> cases.
> 
>> But if you want to use 'inline' keyword, I recommend
>> that call the 'write_relaxed or read_relaxed' function directly
>> with detailed description. 
> 
> Could you please reword this sentence? Not sure that I'm understanding
> it correctly.
> 

If you want to used 'inline' keyword,
Instead, I recommend that remove 'actmon_readl/writel' wrapper functions
and then you calls 'write_relaxed or read_relaxed' function directly
with detailed description.
Dmitry Osipenko July 19, 2019, 4:52 p.m. UTC | #9
19.07.2019 9:01, Chanwoo Choi пишет:
> On 19. 7. 19. 오전 11:14, Dmitry Osipenko wrote:
>> В Fri, 19 Jul 2019 10:27:16 +0900
>> Chanwoo Choi <cw00.choi@samsung.com> пишет:
>>
>>> On 19. 7. 19. 오전 10:24, Chanwoo Choi wrote:
>>>> On 19. 7. 19. 오전 10:22, Dmitry Osipenko wrote:  
>>>>> В Thu, 18 Jul 2019 18:09:05 +0900
>>>>> Chanwoo Choi <cw00.choi@samsung.com> пишет:
>>>>>  
>>>>>> On 19. 7. 16. 오후 10:35, Dmitry Osipenko wrote:  
>>>>>>> 16.07.2019 15:26, Chanwoo Choi пишет:    
>>>>>>>> Hi Dmitry,
>>>>>>>>
>>>>>>>> I'm not sure that it is necessary.
>>>>>>>> As I knew, usally, the 'inline' is used on header file
>>>>>>>> to define the empty functions.
>>>>>>>>
>>>>>>>> Do we have to change it with 'inline' keyword?    
>>>>>>>
>>>>>>> The 'inline' attribute tells compiler that instead of jumping
>>>>>>> into the function, it should take the function's code and
>>>>>>> replace the function's invocation with that code. This is done
>>>>>>> in order to help compiler optimize code properly, please see
>>>>>>> [1]. There is absolutely no need to create a function call into
>>>>>>> a function that consists of a single instruction.
>>>>>>>
>>>>>>> [1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
>>>>>>>     
>>>>>>
>>>>>> If you want to add 'inline' keyword, I recommend that 
>>>>>> you better to remove the modified function in this patch
>>>>>> and then just call the 'write_relaxed or read_relaxed' function
>>>>>> directly. It is same result when using inline keyword.  
>>>>>
>>>>> That could be done, but it makes code less readable.
>>>>>
>>>>> See the difference:
>>>>>
>>>>> device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
>>>>> ACTMON_DEV_INTR_STATUS);
>>>>>
>>>>> writel_relaxed(ACTMON_INTR_STATUS_CLEAR,
>>>>> 	       dev->regs + ACTMON_DEV_INTR_STATUS);  
>>>>
>>>> No problem if you add the detailed comment and you want to use
>>>> the 'inline' keyword.  
>>>
>>> Basically, I think that 'inline' keyword is not necessary.
>>
>> Sure, but I'm finding that it's always nicer to explicitly inline a very
>> simple functions because compiler may not do it properly itself in some
>> cases.
>>
>>> But if you want to use 'inline' keyword, I recommend
>>> that call the 'write_relaxed or read_relaxed' function directly
>>> with detailed description. 
>>
>> Could you please reword this sentence? Not sure that I'm understanding
>> it correctly.
>>
> 
> If you want to used 'inline' keyword,
> Instead, I recommend that remove 'actmon_readl/writel' wrapper functions
> and then you calls 'write_relaxed or read_relaxed' function directly
> with detailed description.
> 

This is a step into a wrong direction. Look, there is no need for extra
comments and the code is clean with the variant I'm proposing, while you
are asking to make code less readable and then paper that over with
comments.

I'll probably just drop this, #11 and #17 for now. Since these patches
and not essential for the functionality of the driver and they are
raising more questions than should be. Maybe we could get back to them
at some point later.

Patch
diff mbox series

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index c6c4a07d3e07..1a10df5dbbed 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -181,28 +181,29 @@  static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
 	{  250000,    100000 },
 };
 
-static u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
+static inline u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
 {
 	return readl_relaxed(tegra->regs + offset);
 }
 
-static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
+static inline void actmon_writel(struct tegra_devfreq *tegra,
+				 u32 val, u32 offset)
 {
 	writel_relaxed(val, tegra->regs + offset);
 }
 
-static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
+static inline u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
 {
 	return readl_relaxed(dev->regs + offset);
 }
 
-static void device_writel(struct tegra_devfreq_device *dev, u32 val,
-			  u32 offset)
+static inline void device_writel(struct tegra_devfreq_device *dev,
+				 u32 val, u32 offset)
 {
 	writel_relaxed(val, dev->regs + offset);
 }
 
-static unsigned long do_percent(unsigned long val, unsigned int pct)
+static inline unsigned long do_percent(unsigned long val, unsigned int pct)
 {
 	return val * pct / 100;
 }