2017-12-02 15:14:16

by Marcus Wolf

[permalink] [raw]
Subject: [PATCH] staging: pi433: Removed some obsolete or duplicated defines; moved two defines to better locations

The define FIFO_SIZE was moved to rf69_registers.h. Although it is not a register,
it is a value, that is given by hardware (like the registers).

The define FIFO_THRESHOLD was moved to pi433_if.c, since it is a value, that is
freely choosen by the interface implementation. The better the response time of
the driver, the lower threshold can be set.

Signed-off-by: Marcus Wolf <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 1 +
drivers/staging/pi433/rf69.c | 1 -
drivers/staging/pi433/rf69.h | 5 -----
drivers/staging/pi433/rf69_registers.h | 5 +++++
4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 2a205c6..292f121 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -59,6 +59,7 @@
#define MAX_MSG_SIZE 900 /* min: FIFO_SIZE! */
#define MSG_FIFO_SIZE 65536 /* 65536 = 2^16 */
#define NUM_DIO 2
+#define FIFO_THRESHOLD 15 /* in byte */

static dev_t pi433_dev;
static DEFINE_IDR(pi433_idr);
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 767b565..ec4b540 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -31,7 +31,6 @@
#include "rf69_registers.h"

#define F_OSC 32000000 /* in Hz */
-#define FIFO_SIZE 66 /* in byte */

/*-------------------------------------------------------------------------*/

diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 5c0c956..645c8df 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -20,11 +20,6 @@
#include "rf69_enum.h"
#include "rf69_registers.h"

-#define F_OSC 32000000 /* in Hz */
-#define FREQUENCY 433920000 /* in Hz, modifying this value impacts CE certification */
-#define FIFO_SIZE 66 /* in byte */
-#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_modulation(struct spi_device *spi, enum modulation modulation);
diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
index 6335d42..cffafd1 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -16,6 +16,11 @@
*/

/*******************************************/
+/* size of the hardware fifo */
+/*******************************************/
+#define FIFO_SIZE 66
+
+/*******************************************/
/* RF69 register addresses */
/*******************************************/
#define REG_FIFO 0x00
--
1.7.10.4


2017-12-02 15:56:08

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] staging: pi433: Removed some obsolete or duplicated defines; moved two defines to better locations

On Sat, Dec 02, 2017 at 05:14:08PM +0200, Marcus Wolf wrote:
> The define FIFO_SIZE was moved to rf69_registers.h. Although it is not a register,
> it is a value, that is given by hardware (like the registers).
>
> The define FIFO_THRESHOLD was moved to pi433_if.c, since it is a value, that is
> freely choosen by the interface implementation. The better the response time of
> the driver, the lower threshold can be set.

Shouldn't this be two separate patches?

Remember, each patch just does one thing.

thanks,

greg k-h

2017-12-02 16:00:30

by Marcus Wolf

[permalink] [raw]
Subject: Re: [PATCH] staging: pi433: Removed some obsolete or duplicated defines; moved two defines to better locations

Hi Greg,

for me the action was "clean up the defines in rf69.c". So for me it was
fine, that two defines were moved and several other deleted at the same.

Is it ok for you, or should I split the patch belated?

Cheers,

Marcus

Am 02.12.2017 um 17:55 schrieb Greg KH:
> On Sat, Dec 02, 2017 at 05:14:08PM +0200, Marcus Wolf wrote:
>> The define FIFO_SIZE was moved to rf69_registers.h. Although it is not a register,
>> it is a value, that is given by hardware (like the registers).
>>
>> The define FIFO_THRESHOLD was moved to pi433_if.c, since it is a value, that is
>> freely choosen by the interface implementation. The better the response time of
>> the driver, the lower threshold can be set.
>
> Shouldn't this be two separate patches?
>
> Remember, each patch just does one thing.
>
> thanks,
>
> greg k-h
>

--
Smarthome-Wolf UG (haftungsbeschränkt)
Helene-Lange-Weg 23
80637 München
Amtsgericht München, HRB 223529
Umastzsteuer-ID: DE304719911
Geschäftsführer: Marcus Wolf

2017-12-02 16:09:25

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] staging: pi433: Removed some obsolete or duplicated defines; moved two defines to better locations


A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Sat, Dec 02, 2017 at 06:00:28PM +0200, Marcus Wolf wrote:
> Hi Greg,
>
> for me the action was "clean up the defines in rf69.c". So for me it was
> fine, that two defines were moved and several other deleted at the same.
>
> Is it ok for you, or should I split the patch belated?

Please split it up, and make a patch series. Don't dribble patches out
over many hours as that just ensures that I will apply them in the wrong
order :(

Take the time, make a patch series, test that they pass the scripts that
we have, and then send them all out at once.

thanks,

greg k-h

2017-12-02 16:35:26

by Marcus Wolf

[permalink] [raw]
Subject: Re: [PATCH] staging: pi433: Removed some obsolete or duplicated defines; moved two defines to better locations



Am 02.12.2017 um 18:09 schrieb Greg KH:
>
> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
> A: No.
> Q: Should I include quotations after my reply?
>
> http://daringfireball.net/2007/07/on_top
>
> On Sat, Dec 02, 2017 at 06:00:28PM +0200, Marcus Wolf wrote:
>> Hi Greg,
>>
>> for me the action was "clean up the defines in rf69.c". So for me it was
>> fine, that two defines were moved and several other deleted at the same.
>>
>> Is it ok for you, or should I split the patch belated?
>
> Please split it up, and make a patch series. Don't dribble patches out
> over many hours as that just ensures that I will apply them in the wrong
> order :(
>
> Take the time, make a patch series, test that they pass the scripts that
> we have, and then send them all out at once.
>
> thanks,
>
> greg k-h
>

Hi Greg,

thanks for the info on patch series. I thought series are to be used if
the patches are logicaly linked together. Didn't know, that patch series
even is ok, if the patches deal with different topics.

I am working with kernel quite rarely. So it's quite hard to do
everything 100% correct. I already took a lot of time today, to try to
make everything perfect. That's why I was working on these five patches
for all the day (aka 7h). And there was no need to implement a single
line - everything was already implemented and tested on HW in July.
But 7h weren't enough or wasted: Again not a single patch was ok :-(

I can understand, your time is limited, too. But with constantly having
no success and constant redo from start, the motivation is fading. Don't
know, whether there could be a better way, how to build up a volunteer.
This way - I am sorry - is frustrating.

Since I have another peace of software, that needs to be implemented
(not just moved into patches) for a customer (paying for it) until
tomorrow, I will leave / have to leave the hobby topic pi433 for now.

Please skip all my patches and I'll give them a third try - maybe
arround Christmas...

Thanks,

Marcus