Re: [PATCH V2 1/5] staging: wilc1000: #ifdef conditionals cover entire functions

["tony.cho" <tony.cho@xxxxxxxxx>]

On 2015년 07월 30일 20:56, Sudip Mukherjee wrote:
> On Thu, Jul 30, 2015 at 06:10:10PM +0900, Tony Cho wrote:
> > This patch lets preprocessor conditionals (#ifdef) related to
> > WILC_SDIO_IRQ_GPIO to compile out the entire functions. Compiling out
> > the entire functions is preferred rather than portions of functions or
> > expressions becausue doing so makes code harder to read.
> >
> > Signed-off-by: Tony Cho <tony.cho@xxxxxxxxx>
> > ---
> <snip>
> >   
> > +#ifdef WILC_SDIO_IRQ_GPIO
> >   static int sdio_clear_int(void)
> >   {
> > -#ifndef WILC_SDIO_IRQ_GPIO
> > -	/* uint32_t sts; */
> > -	sdio_cmd52_t cmd;
> > -
> > -	cmd.read_write = 0;
> > -	cmd.function = 1;
> > -	cmd.raw = 0;
> > -	cmd.address = 0x4;
> > -	cmd.data = 0;
> > -	g_sdio.sdio_cmd52(&cmd);
> > -	int_clrd++;
> > -
> > -	return cmd.data;
> > -#else
> >   	uint32_t reg;
> >   
> >   	if (!sdio_read_reg(WILC_HOST_RX_CTRL_0, &reg)) {
> > @@ -181,9 +168,23 @@ static int sdio_clear_int(void)
> >   	sdio_write_reg(WILC_HOST_RX_CTRL_0, reg);
> >   	int_clrd++;
> >   	return 1;
> > -#endif
> > +}
> > +#else
> > +static int sdio_clear_int(void)
> > +{
> > +	sdio_cmd52_t cmd;
> > +
> > +	cmd.read_write = 0;
> > +	cmd.function = 1;
> > +	cmd.raw = 0;
> > +	cmd.address = 0x4;
> > +	cmd.data = 0;
> > +	g_sdio.sdio_cmd52(&cmd);
> > +	int_clrd++;
> >   
> > +	return cmd.data;
> >   }
> > +#endif /* WILC_SDIO_IRQ_GPIO */
> instead of changing #ifndef to #ifdef i think the following would have
> been easier:

Yes, I agree with you. I will rewrite them after a big patch is accepted.
Thanks for your advice.
Tony.

> diff --git a/drivers/staging/wilc1000/wilc_sdio.c b/drivers/staging/wilc1000/wilc_sdio.c
> index 5a18148..5cd4d45 100644
> --- a/drivers/staging/wilc1000/wilc_sdio.c
> +++ b/drivers/staging/wilc1000/wilc_sdio.c
> @@ -155,9 +155,9 @@ _fail_:
>   	return 0;
>   }
>   
> +#ifndef WILC_SDIO_IRQ_GPIO
>   static int sdio_clear_int(void)
>   {
> -#ifndef WILC_SDIO_IRQ_GPIO
>   	/* uint32_t sts; */
>   	sdio_cmd52_t cmd;
>   
> @@ -170,7 +170,10 @@ static int sdio_clear_int(void)
>   	int_clrd++;
>   
>   	return cmd.data;
> +}
>   #else
> +static int sdio_clear_int(void)
> +{
>   	uint32_t reg;
>   
>   	if (!sdio_read_reg(WILC_HOST_RX_CTRL_0, &reg)) {
> @@ -181,9 +184,8 @@ static int sdio_clear_int(void)
>   	sdio_write_reg(WILC_HOST_RX_CTRL_0, reg);
>   	int_clrd++;
>   	return 1;
> -#endif
> -
>   }
> +#endif
>   
>   uint32_t sdio_xfer_cnt(void)
>   {
>
> >   
> >   uint32_t sdio_xfer_cnt(void)
> <snip>
> > +#ifdef WILC_SDIO_IRQ_GPIO
> >   static int sdio_clear_int_ext(uint32_t val)
> >   {
> >   	int ret;
> >   
> > -	if (g_sdio.has_thrpt_enh3) {
> > +	if(g_sdio.has_thrpt_enh3) {
> why changing this? The original style is according to the kernel coding
> style.
>
> regards
> sudip

--
Cho, Tony
Manager, Staff Engineer | Connectivity System Software Team | Atmel Korea (Wireless solutions BU)
#409, Kins Tower, Jeongja-Dong, Bundang-Gu, Seongnam-Si, Gyeonggi-Do, 463-782, Korea
Phone:82 31 784 8400(Ext. 317); Mobile: 82 10 7232 1523
email: tony.cho@xxxxxxxxx

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel