From: Colin Ian King <[email protected]>
The functions pi433_receive and pi433_tx_thread are local to the source
and do not need to be in global scope, so make them static
Cleans up sparse warnings:
symbol 'pi433_receive' was not declared. Should it be static?
symbol 'pi433_tx_thread' was not declared. Should it be static?
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index d9328ce5ec1d..95930a192de4 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -313,7 +313,7 @@ pi433_start_rx(struct pi433_device *dev)
/*-------------------------------------------------------------------------*/
-int
+static int
pi433_receive(void *data)
{
struct pi433_device *dev = data;
@@ -463,7 +463,7 @@ pi433_receive(void *data)
return bytes_total;
}
-int
+static int
pi433_tx_thread(void *data)
{
struct pi433_device *device = data;
--
2.11.0
From: Colin Ian King <[email protected]>
The function rf69_set_bandwidth_intern is local to the source
and do not need to be in global scope, so make it static. Also
break overly wide line.
Cleans up sparse warning:
symbol 'update_share_count' was not declared. Should it be static?
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/staging/pi433/rf69.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e391ce777bc7..04af906476e3 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -433,7 +433,8 @@ int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum dccPer
return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
}
-int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse mantisse, u8 exponent)
+static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
+ enum mantisse mantisse, u8 exponent)
{
u8 newValue;
--
2.11.0
From: Colin Ian King <[email protected]>
The check for thread_run failure is incorrect, use IS_ERR instead.
Cleans up sparse error message:
"error: incompatible types for operation (<)"
Fixes: 874bcba65f9a ("staging: pi433: New driver")
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 95930a192de4..266e572ae66e 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1152,8 +1152,7 @@ static int pi433_probe(struct spi_device *spi)
device->tx_task_struct = kthread_run(pi433_tx_thread,
device,
"pi433_tx_task");
- if (device->tx_task_struct < 0)
- {
+ if (IS_ERR(device->tx_task_struct)) {
dev_dbg(device->dev, "start of send thread failed");
goto send_thread_failed;
}
--
2.11.0
Hi Colin,
thanks for your patches.
#1 is fine. Same fix was provided by Joseph Wright. I tested it and it works
fine.
#2 looks fine, too.
Conerning #3, I would suggest to declare rf69_set_dc_cut_off_frequency static,
as well. Would you prefer to remove rf69_set_dc_cut_off_frequency from the
header (rf69.h) or would you prefer to add a static there?
If you prefer to keep the line in the header, we should spend a line for
rf69_set_bandwidth_intern in the header, too.
Again thank you :-)
Marcus
> Colin King <[email protected]> hat am 18. Juli 2017 um 15:03
> geschrieben:
>
>
> From: Colin Ian King <[email protected]>
>
> The function rf69_set_bandwidth_intern is local to the source
> and do not need to be in global scope, so make it static. Also
> break overly wide line.
>
> Cleans up sparse warning:
> symbol 'update_share_count' was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/staging/pi433/rf69.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e391ce777bc7..04af906476e3 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -433,7 +433,8 @@ int rf69_set_dc_cut_off_frequency_during_afc(struct
> spi_device *spi, enum dccPer
> return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
> }
>
> -int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse
> mantisse, u8 exponent)
> +static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
> + enum mantisse mantisse, u8 exponent)
> {
> u8 newValue;
>
> --
> 2.11.0
>
>
Declare rf69_set_dc_cut_off_frequency_intern as static since it
is used internaly only
Fixes: 874bcba65f9a ("staging: pi433: New driver")
Signed-off-by: Marcus Wolf <[email protected]>
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -433,7 +433,7 @@
return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
}
-int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse mantisse, u8 exponent)
+static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse mantisse, u8 exponent)
{
u8 newValue;
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -41,7 +41,6 @@
int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance antennaImpedance);
int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain);
enum lnaGain rf69_get_lna_gain(struct spi_device *spi);
-int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dccPercent dccPercent);
int rf69_set_dc_cut_off_frequency(struct spi_device *spi, enum dccPercent dccPercent);
int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum dccPercent dccPercent);
int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8 exponent);
Reviewed-by: Marcus Wolf <[email protected]>
Tested-by: Marcus Wolf <[email protected]>
Am Di, 18.07.2017, 15:04 schrieb Colin King:
> From: Colin Ian King <[email protected]>
>
> The check for thread_run failure is incorrect, use IS_ERR instead.
>
> Cleans up sparse error message:
> "error: incompatible types for operation (<)"
>
> Fixes: 874bcba65f9a ("staging: pi433: New driver")
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/staging/pi433/pi433_if.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 95930a192de4..266e572ae66e 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -1152,8 +1152,7 @@ static int pi433_probe(struct spi_device *spi)
> device->tx_task_struct = kthread_run(pi433_tx_thread,
> device,
> "pi433_tx_task");
> - if (device->tx_task_struct < 0)
> - {
> + if (IS_ERR(device->tx_task_struct)) {
> dev_dbg(device->dev, "start of send thread failed");
> goto send_thread_failed;
> }
> --
> 2.11.0
>
>
>
Reviewed-by: Marcus Wolf <[email protected]>
Am Di, 18.07.2017, 15:03 schrieb Colin King:
> From: Colin Ian King <[email protected]>
>
> The functions pi433_receive and pi433_tx_thread are local to the source
> and do not need to be in global scope, so make them static
>
> Cleans up sparse warnings:
> symbol 'pi433_receive' was not declared. Should it be static?
> symbol 'pi433_tx_thread' was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/staging/pi433/pi433_if.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index d9328ce5ec1d..95930a192de4 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -313,7 +313,7 @@ pi433_start_rx(struct pi433_device *dev)
>
> /*-------------------------------------------------------------------------*/
>
> -int
> +static int
> pi433_receive(void *data)
> {
> struct pi433_device *dev = data;
> @@ -463,7 +463,7 @@ pi433_receive(void *data)
> return bytes_total;
> }
>
> -int
> +static int
> pi433_tx_thread(void *data)
> {
> struct pi433_device *device = data;
> --
> 2.11.0
>
>
>
Reviewed-by: Marcus Wolf <[email protected]>
Am Di, 18.07.2017, 15:03 schrieb Colin King:
> From: Colin Ian King <[email protected]>
>
> The function rf69_set_bandwidth_intern is local to the source
> and do not need to be in global scope, so make it static. Also
> break overly wide line.
>
> Cleans up sparse warning:
> symbol 'update_share_count' was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/staging/pi433/rf69.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e391ce777bc7..04af906476e3 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -433,7 +433,8 @@ int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum dccPer
> return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
> }
>
> -int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse mantisse, u8 exponent)
> +static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
> + enum mantisse mantisse, u8 exponent)
> {
> u8 newValue;
>
> --
> 2.11.0
>
>
>
On 20/07/17 12:01, Wolf Entwicklungen wrote:
> Declare rf69_set_dc_cut_off_frequency_intern as static since it
> is used internaly only
>
> Fixes: 874bcba65f9a ("staging: pi433: New driver")
> Signed-off-by: Marcus Wolf <[email protected]>
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -433,7 +433,7 @@
> return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
> }
>
> -int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse mantisse, u8 exponent)
> +static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse mantisse, u8 exponent)
> {
> u8 newValue;
> diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> --- a/drivers/staging/pi433/rf69.h
> +++ b/drivers/staging/pi433/rf69.h
> @@ -41,7 +41,6 @@
> int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance antennaImpedance);
> int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain);
> enum lnaGain rf69_get_lna_gain(struct spi_device *spi);
> -int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dccPercent dccPercent);
> int rf69_set_dc_cut_off_frequency(struct spi_device *spi, enum dccPercent dccPercent);
> int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum dccPercent dccPercent);
> int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8 exponent);
>
This is better than my original patch, so ignore my patch "staging:
pi433: Make functions rf69_set_bandwidth_intern static"
Colin
Hi Colin,
mine is an aditional patch for an other function, that's pretty similar to the
one, you improoved. So we need both patches, yours and mine!
Cheers,
Marcus
> Colin Ian King <[email protected]> hat am 20. Juli 2017 um 14:58
> geschrieben:
>
>
> On 20/07/17 12:01, Wolf Entwicklungen wrote:
> > Declare rf69_set_dc_cut_off_frequency_intern as static since it
> > is used internaly only
> >
> > Fixes: 874bcba65f9a ("staging: pi433: New driver")
> > Signed-off-by: Marcus Wolf <[email protected]>
> >
> > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > --- a/drivers/staging/pi433/rf69.c
> > +++ b/drivers/staging/pi433/rf69.c
> > @@ -433,7 +433,7 @@
> > return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
> > }
> >
> > -int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse
> > mantisse, u8 exponent)
> > +static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum
> > mantisse mantisse, u8 exponent)
> > {
> > u8 newValue;
> > diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> > --- a/drivers/staging/pi433/rf69.h
> > +++ b/drivers/staging/pi433/rf69.h
> > @@ -41,7 +41,6 @@
> > int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance
> > antennaImpedance);
> > int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain);
> > enum lnaGain rf69_get_lna_gain(struct spi_device *spi);
> > -int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg,
> > enum dccPercent dccPercent);
> > int rf69_set_dc_cut_off_frequency(struct spi_device *spi, enum dccPercent
> > dccPercent);
> > int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum
> > dccPercent dccPercent);
> > int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8
> > exponent);
> >
>
> This is better than my original patch, so ignore my patch "staging:
> pi433: Make functions rf69_set_bandwidth_intern static"
>
> Colin
>
On Tue, Jul 18, 2017 at 02:03:58PM +0100, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> The functions pi433_receive and pi433_tx_thread are local to the source
> and do not need to be in global scope, so make them static
>
> Cleans up sparse warnings:
> symbol 'pi433_receive' was not declared. Should it be static?
> symbol 'pi433_tx_thread' was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King <[email protected]>
> Reviewed-by: Marcus Wolf <[email protected]>
Only one of the patches in this series applied :(
On Thu, Jul 20, 2017 at 01:01:46PM +0200, Wolf Entwicklungen wrote:
> Declare rf69_set_dc_cut_off_frequency_intern as static since it
> is used internaly only
>
> Fixes: 874bcba65f9a ("staging: pi433: New driver")
> Signed-off-by: Marcus Wolf <[email protected]>
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -433,7 +433,7 @@
> return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
> }
>
> -int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse mantisse, u8 exponent)
> +static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse mantisse, u8 exponent)
> {
> u8 newValue;
> diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> --- a/drivers/staging/pi433/rf69.h
> +++ b/drivers/staging/pi433/rf69.h
> @@ -41,7 +41,6 @@
> int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance antennaImpedance);
> int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain);
> enum lnaGain rf69_get_lna_gain(struct spi_device *spi);
> -int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dccPercent dccPercent);
> int rf69_set_dc_cut_off_frequency(struct spi_device *spi, enum dccPercent dccPercent);
> int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum dccPercent dccPercent);
> int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8 exponent);
> _______________________________________________
> devel mailing list
> [email protected]
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Patch does not apply :(
Hi Greg,
also had a very close look to this patch. Even in your reply I can't find any
problems with line wraps or other corruptions :-/
But we have alternative patches, solving these problems as well.
You e.g. could use the patch
[PATCH] Make functions rf69_set_bandwidth_intern and
rf69_set_dc_cut_off_frequency_intern static
from Colin King 21/07/2017.
It's doing exactly the same, my patch should have done.
But be careful - tonight you added patch
staging: pi433: Make functions rf69_set_bandwidth_intern static"
Clins patch includes that changes as well!
Once again sorry for my crappy patches,
Marcus
On Sat, Jul 29, 2017 at 11:09:23AM +0200, Marcus Wolf wrote:
> Hi Greg,
>
> also had a very close look to this patch. Even in your reply I can't find any
> problems with line wraps or other corruptions :-/
>
Here are the relevant lines from Greg's email:
> +++ b/drivers/staging/pi433/rf69.c
> @@ -221,7 +221,7 @@ int rf69_set_frequency(struct spi_device *spi, u32
> frequency)
The "frequency)" bit is supposed to be on the line before.
regards,
dan carpenter