Re: [Outreachy kernel] [PATCH 2/2] staging: comedi: drivers: amplc_pci230: Convert macros to static inline functions

[Arnd Bergmann <arnd@arndb.de>]

On Saturday 19 March 2016 01:16:32 Bhaktipriya Shridhar wrote:
> 
> diff --git a/drivers/staging/comedi/drivers/amplc_pci230.c b/drivers/staging/comedi/drivers/amplc_pci230.c
> index 907c39c..051787e 100644
> --- a/drivers/staging/comedi/drivers/amplc_pci230.c
> +++ b/drivers/staging/comedi/drivers/amplc_pci230.c
> @@ -370,7 +370,9 @@
>  #define CLK_OUTNM1	6	/* output of channel-1 modulo total */
>  #define CLK_EXT		7	/* external clock */
>  /* Macro to construct clock input configuration register value. */
> -#define CLK_CONFIG(chan, src)	((((chan) & 3) << 3) | ((src) & 7))
> +static inline unsigned int CLK_CONFIG(unsigned int chan, unsigned int src) {
> +	return ((((chan) & 3) << 3) | ((src) & 7));
> +}

We often use macros for cases like this, as this really just puts some bits
in a particular position.

With the conversion above, you should remove the extra () around the arguments.

>  /*
> @@ -687,7 +691,7 @@ static void pci230_ct_setup_ns_mode(struct comedi_device *dev, unsigned int ct,
>  	/* Determine clock source and count. */
>  	clk_src = pci230_choose_clk_count(ns, &count, flags);
>  	/* Program clock source. */
> -	outb(CLK_CONFIG(ct, clk_src), dev->iobase + PCI230_ZCLK_SCE);
> +	outb(clk_config(ct, clk_src), dev->iobase + PCI230_ZCLK_SCE);
>  	/* Set initial count. */
>  	if (count >= 65536)
>  		count = 0;

This won't work: you have changed the user from uppercase to lowercase, but not
the definition. The change is ok, but you have to do it consistently.

It looks like you did not build-test this correctly, otherwise you would have
run into the problem.

I also see that every single user of this macro passes the argument into
the outb() function, with (dev->iobase + ...) as the address. I would
suggest merging the two calls into one when you convert it, and do

static inline void pci230_clk_config(struct comedi_device *dev, unsigned int reg,
				      unsigned int chan, unsigned int src)
{
	outb(((chan & 3) << 3) | (src & 7), dev->iobase + reg);
}

which lets the caller above get simplified to

	pci230_clk_config(dev, PCI230_ZCLK_SCE, ct, clk_src);

	Arnd