Re: [Outreachy kernel] [PATCH v2 1/4] staging: comedi: amplc_pci230: Convert macro CLK_CONFIG to static inline function

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Mar 21, 2016 at 12:41:27AM +0530, Bhaktipriya Shridhar wrote:
> Convert macro CLK_CONFIG to static inline function as static inline
> functions are preferred over macros. Since every single user of this
> macro passes the argument into the outb() function, with
> (dev->iobase + ...) as the address, the two calls have been merged
> into one.
> 
> This was done using Coccinelle:
> 
> @r@
> identifier f;
> expression e;
> @@
> - #define CLK_CONFIG(chan, src) e
> + 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);
> +}
> 
> @r1@
> expression dev,reg,chan,src;
> @@
> -outb(CLK_CONFIG(chan, src), dev->iobase + reg);
> +pci230_clk_config(dev, reg, chan, src);
> 
> Also, the comment describing the macro has been removed manually.
> 
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
> ---
>  Changes in v2:
> 	- Since every single usage of this macro passes the argument into
> 	  the outb() function, with (dev->iobase + ...) as the address,
> 	  the two calls have been merged into one.
> 
>  drivers/staging/comedi/drivers/amplc_pci230.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/amplc_pci230.c b/drivers/staging/comedi/drivers/amplc_pci230.c
> index 907c39c..635cd46 100644
> --- a/drivers/staging/comedi/drivers/amplc_pci230.c
> +++ b/drivers/staging/comedi/drivers/amplc_pci230.c
> @@ -369,8 +369,12 @@
>  #define CLK_1KHZ	5	/* internal 1 kHz clock */
>  #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 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);
> +}
> 
>  /*
>   * Counter/timer gate input configuration sources.
> @@ -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);
> +	pci230_clk_config(dev, PCI230_ZCLK_SCE, ct, clk_src);

This is only done in one place, just "open code" it, no need for a
separate function at all here.

Actually, the original code is nicer, it makes more sense, and a macro
is just fine, no need to change it.

thanks,

greg k-h