Re: [PATCH v2 08/11] staging: pi433: Remove enum data_mode

[Marcus Wolf]

Am 06.12.2017 um 00:08 schrieb Simon SandstrÃm:
> Call rf69_set_data_mode with DATAMODUL_MODE value directly.
>
> Signed-off-by: Simon SandstrÃm <simon@xxxxxxxxxx>
> ---
>   drivers/staging/pi433/pi433_if.c  |  2 +-
>   drivers/staging/pi433/rf69.c      | 15 ++-------------
>   drivers/staging/pi433/rf69.h      |  2 +-
>   drivers/staging/pi433/rf69_enum.h |  6 ------
>   4 files changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index fb500d062df8..3b4170b9ba94 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -1125,7 +1125,7 @@ static int pi433_probe(struct spi_device *spi)
>   
>   	/* setup the radio module */
>   	SET_CHECKED(rf69_set_mode		(spi, standby));
> -	SET_CHECKED(rf69_set_data_mode		(spi, packet));
> +	SET_CHECKED(rf69_set_data_mode		(spi, DATAMODUL_MODE_PACKET));
>   	SET_CHECKED(rf69_set_amplifier_0	(spi, OPTION_ON));
>   	SET_CHECKED(rf69_set_amplifier_1	(spi, OPTION_OFF));
>   	SET_CHECKED(rf69_set_amplifier_2	(spi, OPTION_OFF));
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e5b29bed35ef..4c9eb97978ef 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -61,20 +61,9 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
>   
>   }
>   
> -int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode)
> +int rf69_set_data_mode(struct spi_device *spi, u8 data_mode)
>   {
> -	#ifdef DEBUG
> -		dev_dbg(&spi->dev, "set: data mode");
> -	#endif
> -
> -	switch (dataMode) {
> -	case packet:		return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
> -	case continuous:	return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
> -	case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS_NOSYNC);
> -	default:
> -		dev_dbg(&spi->dev, "set: illegal input param");
> -		return -EINVAL;
> -	}
> +	return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | data_mode);
>   }
>   
>   int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
> diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> index 177223451c87..18296b4502f3 100644
> --- a/drivers/staging/pi433/rf69.h
> +++ b/drivers/staging/pi433/rf69.h
> @@ -26,7 +26,7 @@
>   #define FIFO_THRESHOLD	15		/* in byte */
>   
>   int rf69_set_mode(struct spi_device *spi, enum mode mode);
> -int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode);
> +int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
>   int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);
>   enum modulation rf69_get_modulation(struct spi_device *spi);
>   int rf69_set_modulation_shaping(struct spi_device *spi, enum mod_shaping mod_shaping);
> diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
> index 97f615effca3..b0715b4eb6ac 100644
> --- a/drivers/staging/pi433/rf69_enum.h
> +++ b/drivers/staging/pi433/rf69_enum.h
> @@ -31,12 +31,6 @@ enum mode {
>   	receive
>   };
>   
> -enum dataMode {
> -	packet,
> -	continuous,
> -	continuousNoSync
> -};
> -
>   enum modulation {
>   	OOK,
>   	FSK

Hi Simon,

this change is "closing a door".

The rf69 is able to work in an advanced packet mode or in a simple 
continuous mode.

The driver so far is using the advanced packet mode, only. But if it 
will support continuous mode some day, it will be necessary to configure 
this.

The open source projects fhem and domoticz already asked for such a 
change, since for their architecture, they'll need a pi433 working in 
continuous mode. But so far I am not planning to implement such a 
functionality.

Since the rule for kernel development seems to be, not to care about 
future, most probably you patch is fine, anyway.

Marcus