| Message ID | 20190707223303.6755-13-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | More improvements for Tegra30 devfreq driver | expand |
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; > } >
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
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.
В 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);
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. > > >
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. > >> >> >> > >
В 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.
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.
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.
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; }
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(-)