Skip to content

Commit

Permalink
terrible-bsp: kernel: move code quality slides to appendix
Browse files Browse the repository at this point in the history 
There are too many examples for a 50 minutes talk. Keep them anyway
for the curious.
  • Loading branch information
@lucaceresoli
lucaceresoli committed Feb 5, 2017
1 parent 58377a8 commit 5d026bd
Showing 1 changed file with 117 additions and 117 deletions.
234 changes: 117 additions & 117 deletions terrible-bsp/terrible-bsp.tex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,123 +249,6 @@ \section{Linux kernel}
\end{itemize}
\end{frame}

\begin{frame}[fragile]{Code quality /1}
Changes to \texttt{Makefile}:

\begin{minted}[fontsize=\scriptsize]{diff}
-ARCH?= $(SUBARCH)
-CROSS_COMPILE?=
-CROSS_COMPILE?= $(CONFIG_CROSS_COMPILE:''%''=%)
+#ARCH?= $(SUBARCH)
+ARCH= arm
+#CROSS_COMPILE?=
+#CROSS_COMPILE?= $(CONFIG_CROSS_COMPILE:''%''=%)
+CROSS_COMPILE= arm-linux-
\end{minted}

\begin{itemize}
\item Prevents using toolchains with a different prefix
\item Any advantage?
\end{itemize}
\end{frame}

\begin{frame}[fragile]{Code quality /2}
Changes to \texttt{arch/arm/boot/Makefile}:

\begin{minted}[fontsize=\scriptsize]{udiff}
$(obj)/Image: vmlinux FORCE
$(call if_changed,objcopy)
@echo ' Kernel: $@ is ready'

+ifeq ($(CONFIG_ARCH_W55FA92),y)
+ cp $@ ../image/conprog.bin
+endif
\end{minted}

\begin{itemize}
\item \texttt{../image/} does not make sense in any buildsystem
\end{itemize}
\end{frame}

\begin{frame}[fragile]{Code quality /3}
\texttt{sound/soc/w55fa92/w55fa92\_spu.c}:

\begin{minted}[fontsize=\scriptsize]{c}
if (nChannels ==1)
{
DrvSPU_EnableInt(_u8Channel0, DRVSPU_ENDADDRESS_INT);
DrvSPU_EnableInt(_u8Channel0, DRVSPU_THADDRESS_INT);
}
else
{ /* just open one channel interrupt */
DrvSPU_EnableInt(_u8Channel0, DRVSPU_ENDADDRESS_INT);
DrvSPU_EnableInt(_u8Channel0, DRVSPU_THADDRESS_INT);
}
\end{minted}

\begin{itemize}
\item Find the differences between the {\em then} and the {\em else} branch
\end{itemize}
\end{frame}

\begin{frame}[fragile]{Code quality /4}
\texttt{sound/soc/w55fa92/w55fa92\_spu.c}:

\linespread{0.5}
\begin{minted}[fontsize=\scriptsize]{c}
static int DrvSPU_EnableInt(u32 u32Channel, u32 u32InterruptFlag)
{
if ( (u32Channel >=eDRVSPU_CHANNEL_0) && (u32Channel <=eDRVSPU_CHANNEL_31) )
{
/* ... */
if (u32InterruptFlag & DRVSPU_USER_INT)
{
AUDIO_WRITE(REG_SPU_CH_EVENT, AUDIO_READ(REG_SPU_CH_EVENT) | EV_USR_EN);
}
if (u32InterruptFlag & DRVSPU_SILENT_INT)
{
AUDIO_WRITE(REG_SPU_CH_EVENT, AUDIO_READ(REG_SPU_CH_EVENT) | EV_SLN_EN);
}
/* ...a few more times... */

/* ... */
return E_SUCCESS;
}
else
return E_DRVSPU_WRONG_CHANNEL;
}
\end{minted}
\end{frame}

\begin{frame}[fragile]{Code quality /5}
\texttt{arch/arm/mach-w55fa92/include/mach/w55fa92\_gpio.h}:

\linespread{1}
\begin{minted}[fontsize=\scriptsize]{c}
static inline int w55fa92_gpio_configure(int group, int num) {
/* ... */
case GPIO_GROUP_B:
if(num <= 7)
writel(readl(REG_GPBFUN0) &~ (0xF << (num<<2)), REG_GPBFUN0);
else
writel(readl(REG_GPBFUN1) &~ (0xF << ((num%8)<<2)), REG_GPBFUN1);
break;

case GPIO_GROUP_C:
if(num <= 7)
writel(readl(REG_GPCFUN0) &~ (0xF << (num<<2)), REG_GPCFUN0);
else
writel(readl(REG_GPCFUN1) &~ (0xF << ((num%8)<<2)), REG_GPCFUN1);
break;
/* ...similarly fo other GPIO ports... */
}
\end{minted}

\begin{itemize}
\item A little refactoring would help
\end{itemize}
\end{frame}

\begin{frame}[fragile]{Code quality: driver model}
\texttt{drivers/video/w55fa92\_fb.c}:

Expand Down Expand Up @@ -813,4 +696,121 @@ \section{Extra Slides}
\texttt{ error: expected primary-expression before '.' token}
\end{frame}

\begin{frame}[fragile]{Kernel code quality --- example 1}
Changes to \texttt{Makefile}:

\begin{minted}[fontsize=\scriptsize]{diff}
-ARCH?= $(SUBARCH)
-CROSS_COMPILE?=
-CROSS_COMPILE?= $(CONFIG_CROSS_COMPILE:''%''=%)
+#ARCH?= $(SUBARCH)
+ARCH= arm
+#CROSS_COMPILE?=
+#CROSS_COMPILE?= $(CONFIG_CROSS_COMPILE:''%''=%)
+CROSS_COMPILE= arm-linux-
\end{minted}

\begin{itemize}
\item Prevents using toolchains with a different prefix
\item Any advantage?
\end{itemize}
\end{frame}

\begin{frame}[fragile]{Kernel code quality --- example 2}
Changes to \texttt{arch/arm/boot/Makefile}:

\begin{minted}[fontsize=\scriptsize]{udiff}
$(obj)/Image: vmlinux FORCE
$(call if_changed,objcopy)
@echo ' Kernel: $@ is ready'

+ifeq ($(CONFIG_ARCH_W55FA92),y)
+ cp $@ ../image/conprog.bin
+endif
\end{minted}

\begin{itemize}
\item \texttt{../image/} does not make sense in any buildsystem
\end{itemize}
\end{frame}

\begin{frame}[fragile]{Kernel code quality --- example 3}
\texttt{sound/soc/w55fa92/w55fa92\_spu.c}:

\begin{minted}[fontsize=\scriptsize]{c}
if (nChannels ==1)
{
DrvSPU_EnableInt(_u8Channel0, DRVSPU_ENDADDRESS_INT);
DrvSPU_EnableInt(_u8Channel0, DRVSPU_THADDRESS_INT);
}
else
{ /* just open one channel interrupt */
DrvSPU_EnableInt(_u8Channel0, DRVSPU_ENDADDRESS_INT);
DrvSPU_EnableInt(_u8Channel0, DRVSPU_THADDRESS_INT);
}
\end{minted}

\begin{itemize}
\item Find the differences between the {\em then} and the {\em else} branch
\end{itemize}
\end{frame}

\begin{frame}[fragile]{Kernel code quality --- example 4}
\texttt{sound/soc/w55fa92/w55fa92\_spu.c}:

\linespread{0.5}
\begin{minted}[fontsize=\scriptsize]{c}
static int DrvSPU_EnableInt(u32 u32Channel, u32 u32InterruptFlag)
{
if ( (u32Channel >=eDRVSPU_CHANNEL_0) && (u32Channel <=eDRVSPU_CHANNEL_31) )
{
/* ... */
if (u32InterruptFlag & DRVSPU_USER_INT)
{
AUDIO_WRITE(REG_SPU_CH_EVENT, AUDIO_READ(REG_SPU_CH_EVENT) | EV_USR_EN);
}
if (u32InterruptFlag & DRVSPU_SILENT_INT)
{
AUDIO_WRITE(REG_SPU_CH_EVENT, AUDIO_READ(REG_SPU_CH_EVENT) | EV_SLN_EN);
}
/* ...a few more times... */

/* ... */
return E_SUCCESS;
}
else
return E_DRVSPU_WRONG_CHANNEL;
}
\end{minted}
\end{frame}

\begin{frame}[fragile]{Kernel code quality --- example 5}
\texttt{arch/arm/mach-w55fa92/include/mach/w55fa92\_gpio.h}:

\linespread{1}
\begin{minted}[fontsize=\scriptsize]{c}
static inline int w55fa92_gpio_configure(int group, int num) {
/* ... */
case GPIO_GROUP_B:
if(num <= 7)
writel(readl(REG_GPBFUN0) &~ (0xF << (num<<2)), REG_GPBFUN0);
else
writel(readl(REG_GPBFUN1) &~ (0xF << ((num%8)<<2)), REG_GPBFUN1);
break;

case GPIO_GROUP_C:
if(num <= 7)
writel(readl(REG_GPCFUN0) &~ (0xF << (num<<2)), REG_GPCFUN0);
else
writel(readl(REG_GPCFUN1) &~ (0xF << ((num%8)<<2)), REG_GPCFUN1);
break;
/* ...similarly fo other GPIO ports... */
}
\end{minted}

\begin{itemize}
\item A little refactoring would help
\end{itemize}
\end{frame}

\end{document}

0 comments on commit 5d026bd

Please sign in to comment.