Date: Mon, 4 Dec 2017 13:17:37 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Simon =?iso-8859-1?Q?Sandstr=F6m?= <simon@nikanor.nu>
Cc: gregkh@linuxfoundation.org, devel@driverdev.osuosl.org,
        linux@Wolf-Entwicklungen.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in
 rf69_enum.h
Message-ID: <20171204101737.xv4eyglsrbmiagtn@mwanda>
References: <20171203151726.16639-1-simon@nikanor.nu>
 <20171203151726.16639-5-simon@nikanor.nu>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20171203151726.16639-5-simon@nikanor.nu>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2829
Lines: 81

On Sun, Dec 03, 2017 at 04:17:24PM +0100, Simon Sandstr?m wrote:
> Renames the enum optionOnOff and its values optionOn, optionOff to enum
> option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
> "Avoid CamelCase: <optionOnOff>, <optionOn>, <optionOff>".
> 
> Signed-off-by: Simon Sandstr?m <simon@nikanor.nu>
> ---
>  drivers/staging/pi433/pi433_if.c  | 34 ++++++++++++++---------------
>  drivers/staging/pi433/pi433_if.h  | 16 +++++++-------
>  drivers/staging/pi433/rf69.c      | 45 ++++++++++++++++++++++-----------------
>  drivers/staging/pi433/rf69.h      | 15 ++++++++-----
>  drivers/staging/pi433/rf69_enum.h |  6 +++---
>  5 files changed, 63 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index b8efe6a74a2a..4f6830f369e9 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
>  	/* packet config */
>  	/* enable */
>  	SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
> -	if (rx_cfg->enable_sync == optionOn)
> +	if (rx_cfg->enable_sync == OPTION_ON)

I haven't told anyone yet, but I wish someone would just get rid of
optionOn entirely.  This should just be:

	if (rx_cfg->enable_sync) {


> diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> index 6b31c1584b3a..34ff0d4807bd 100644
> --- a/drivers/staging/pi433/pi433_if.h
> +++ b/drivers/staging/pi433/pi433_if.h
> @@ -73,11 +73,11 @@ struct pi433_tx_cfg {
>  
>  
>  	/* packet format */
> -	enum optionOnOff	enable_preamble;
> -	enum optionOnOff	enable_sync;
> -	enum optionOnOff	enable_length_byte;
> -	enum optionOnOff	enable_address_byte;
> -	enum optionOnOff	enable_crc;
> +	enum option_on_off	enable_preamble;
> +	enum option_on_off	enable_sync;
> +	enum option_on_off	enable_length_byte;
> +	enum option_on_off	enable_address_byte;
> +	enum option_on_off	enable_crc;

These should be bool.

> -int rf69_set_amplifier_0(struct spi_device *spi, enum optionOnOff optionOnOff)
> +int rf69_set_amplifier_0(struct spi_device *spi,
> +			 enum option_on_off option_on_off)


This should be two functions:

int rf69_enable_amplifier_0(struct spi_device *spi)
{
	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |  MASK_PALEVEL_PA0));
}

int rf69_disable_amplifier_0(struct spi_device *spi)
{
	return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & ~MASK_PALEVEL_PA0));
}

Perhaps choose different function names if you want?  You could do it
as several patches:

patch 1: change types to bool
patch 2: sed -e '/ == optionOn//'
patch 3: split the functions into two functions
patch 4: delete optionOnOff enum

patches 1 and 2 could be merged together (your choice).

regards,
dan carpenter