Re: [PATCH] staging: pi433: rf69.c: Introduced define DEBUG_FUNC_ENTRY
On Sat, Dec 02, 2017 at 07:00:17PM +0200, Marcus Wolf wrote:
> Am 02.12.2017 um 17:00 schrieb Greg KH:
> > On Sat, Dec 02, 2017 at 01:45:50PM +0200, Marcus Wolf wrote:
> > > Since dev_dbg already depends on define DEBUG, there was no sense, to
> > > enclose
> > > dev_dbg lines with #ifdef DEBUG.
> > > Instead of removing #ifdef DEBUG, I introduced define DEBUG_FUNC_ENTRY.
> > > So now it is
> > > possible to switch of the function entry debug lines even while debug is
> > > switched on.
> >
> > Ick ick ick.
> >
> > No, these lines should just all be deleted. Use ftrace if you want to
> > see what functions are being called, that's what it is there for. Don't
> > create driver-specific defines and functionality for when we already
> > have that functionality for the whole of the kernel. That's really
> > redundant and messy.
> >
> > > Signed-off-by: Marcus Wolf
> > > ---
> > > drivers/staging/pi433/rf69.c | 118
> > > +-
> > > 1 file changed, 58 insertions(+), 60 deletions(-)
> > >
> > > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > > index 12c9df9..0df084e 100644
> > > --- a/drivers/staging/pi433/rf69.c
> > > +++ b/drivers/staging/pi433/rf69.c
> > > @@ -15,8 +15,10 @@
> > >* GNU General Public License for more details.
> > >*/
> > > -/* enable prosa debug info */
> > > +/* generic enable/disable dev_dbg */
> > > #undef DEBUG
> > > +/* enable print function entry */
> > > +#undef DEBUG_FUNC_ENTRY
> > > /* enable print of values on reg access */
> > > #undef DEBUG_VALUES
> > > /* enable print of values on fifo access */
> > > @@ -40,7 +42,7 @@
> > > int rf69_set_mode(struct spi_device *spi, enum mode mode)
> > > {
> > > - #ifdef DEBUG
> > > + #ifdef DEBUG_FUNC_ENTRY
> > > dev_dbg(&spi->dev, "set: mode");
> > > #endif
> >
> > So this whole #ifdef dev_dbg #endif should all be removed.
> >
> > thanks,
> >
> > greg k-h
> >
>
> Hi all,
>
> just for clarification, why I introduced these dev_dbg during development of
> the driver and didn't use ftrace:
>
> Since I wanted the driver to use a single module as sender and receiver at
> (almost) the same time, the module constantly needs to be reconfigured
> (constant switching between rx configuration and tx configuration - see my
> documentation for details on the idea).
>
> The routine, accessing the registers is able to print the register number
> and the value, it reads / writes from / to the register. It's dam hard to
> keep the survey over the use 30...40 register numbers, in 10th of rows of
> register setting and reading, if you see just the numbers in the log.
> Especially this is / was hard, if one register was written several times,
> because it contains different settings. Then decoding of the adress wasn't
> enough, I even need to decode the bits in the value.
>
> Therefore I finally introduced this dev_dbg lines at the enty of the setter
> (and getter): After that in the log I coud see something like this:
> Set gain: 0x43 0x81
> Set threshold: 0x51 0x30
> Get moulation: 0x22 0x7c
> instead of just numbers.
> That eased the debugging a lot.
>
> When using ftrace I would be able to see, in which order the setter were
> called, but the link to the vlaues would be missing.
>
> If those dev_dbg are unwanted, I am ok, if someone removes them.
>
> Hope this helps understanding
Yes, for debugging the code to start with, put loads of messages in
there, that's fine. But now that it all works, they are not needed, so
they can be removed, and ftrace used instead.
thanks,
greg k-h
Re: [PATCH] staging: pi433: rf69.c: Introduced define DEBUG_FUNC_ENTRY
Am 02.12.2017 um 17:00 schrieb Greg KH:
On Sat, Dec 02, 2017 at 01:45:50PM +0200, Marcus Wolf wrote:
Since dev_dbg already depends on define DEBUG, there was no sense, to enclose
dev_dbg lines with #ifdef DEBUG.
Instead of removing #ifdef DEBUG, I introduced define DEBUG_FUNC_ENTRY. So now
it is
possible to switch of the function entry debug lines even while debug is
switched on.
Ick ick ick.
No, these lines should just all be deleted. Use ftrace if you want to
see what functions are being called, that's what it is there for. Don't
create driver-specific defines and functionality for when we already
have that functionality for the whole of the kernel. That's really
redundant and messy.
Signed-off-by: Marcus Wolf
---
drivers/staging/pi433/rf69.c | 118 +-
1 file changed, 58 insertions(+), 60 deletions(-)
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 12c9df9..0df084e 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -15,8 +15,10 @@
* GNU General Public License for more details.
*/
-/* enable prosa debug info */
+/* generic enable/disable dev_dbg */
#undef DEBUG
+/* enable print function entry */
+#undef DEBUG_FUNC_ENTRY
/* enable print of values on reg access */
#undef DEBUG_VALUES
/* enable print of values on fifo access */
@@ -40,7 +42,7 @@
int rf69_set_mode(struct spi_device *spi, enum mode mode)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: mode");
#endif
So this whole #ifdef dev_dbg #endif should all be removed.
thanks,
greg k-h
Hi all,
just for clarification, why I introduced these dev_dbg during
development of the driver and didn't use ftrace:
Since I wanted the driver to use a single module as sender and receiver
at (almost) the same time, the module constantly needs to be
reconfigured (constant switching between rx configuration and tx
configuration - see my documentation for details on the idea).
The routine, accessing the registers is able to print the register
number and the value, it reads / writes from / to the register. It's dam
hard to keep the survey over the use 30...40 register numbers, in 10th
of rows of register setting and reading, if you see just the numbers in
the log. Especially this is / was hard, if one register was written
several times, because it contains different settings. Then decoding of
the adress wasn't enough, I even need to decode the bits in the value.
Therefore I finally introduced this dev_dbg lines at the enty of the
setter (and getter): After that in the log I coud see something like this:
Set gain: 0x43 0x81
Set threshold: 0x51 0x30
Get moulation: 0x22 0x7c
instead of just numbers.
That eased the debugging a lot.
When using ftrace I would be able to see, in which order the setter were
called, but the link to the vlaues would be missing.
If those dev_dbg are unwanted, I am ok, if someone removes them.
Hope this helps understanding
Marcus
Re: [PATCH] staging: pi433: rf69.c: Introduced define DEBUG_FUNC_ENTRY
On Sat, Dec 02, 2017 at 01:45:50PM +0200, Marcus Wolf wrote:
> Since dev_dbg already depends on define DEBUG, there was no sense, to enclose
> dev_dbg lines with #ifdef DEBUG.
> Instead of removing #ifdef DEBUG, I introduced define DEBUG_FUNC_ENTRY. So
> now it is
> possible to switch of the function entry debug lines even while debug is
> switched on.
Ick ick ick.
No, these lines should just all be deleted. Use ftrace if you want to
see what functions are being called, that's what it is there for. Don't
create driver-specific defines and functionality for when we already
have that functionality for the whole of the kernel. That's really
redundant and messy.
> Signed-off-by: Marcus Wolf
> ---
> drivers/staging/pi433/rf69.c | 118
> +-
> 1 file changed, 58 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index 12c9df9..0df084e 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -15,8 +15,10 @@
> * GNU General Public License for more details.
> */
>
> -/* enable prosa debug info */
> +/* generic enable/disable dev_dbg */
> #undef DEBUG
> +/* enable print function entry */
> +#undef DEBUG_FUNC_ENTRY
> /* enable print of values on reg access */
> #undef DEBUG_VALUES
> /* enable print of values on fifo access */
> @@ -40,7 +42,7 @@
>
> int rf69_set_mode(struct spi_device *spi, enum mode mode)
> {
> - #ifdef DEBUG
> + #ifdef DEBUG_FUNC_ENTRY
> dev_dbg(&spi->dev, "set: mode");
> #endif
So this whole #ifdef dev_dbg #endif should all be removed.
thanks,
greg k-h
[PATCH] staging: pi433: rf69.c: Introduced define DEBUG_FUNC_ENTRY
Since dev_dbg already depends on define DEBUG, there was no sense, to enclose
dev_dbg lines with #ifdef DEBUG.
Instead of removing #ifdef DEBUG, I introduced define DEBUG_FUNC_ENTRY. So now
it is
possible to switch of the function entry debug lines even while debug is
switched on.
Signed-off-by: Marcus Wolf
---
drivers/staging/pi433/rf69.c | 118 +-
1 file changed, 58 insertions(+), 60 deletions(-)
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 12c9df9..0df084e 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -15,8 +15,10 @@
* GNU General Public License for more details.
*/
-/* enable prosa debug info */
+/* generic enable/disable dev_dbg */
#undef DEBUG
+/* enable print function entry */
+#undef DEBUG_FUNC_ENTRY
/* enable print of values on reg access */
#undef DEBUG_VALUES
/* enable print of values on fifo access */
@@ -40,7 +42,7 @@
int rf69_set_mode(struct spi_device *spi, enum mode mode)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: mode");
#endif
@@ -63,7 +65,7 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: data mode");
#endif
@@ -79,7 +81,7 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode
dataMode)
int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: modulation");
#endif
@@ -96,7 +98,7 @@ enum modulation rf69_get_modulation(struct spi_device *spi)
{
u8 currentValue;
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "get: mode");
#endif
@@ -111,7 +113,7 @@ enum modulation rf69_get_modulation(struct spi_device *spi)
int rf69_set_modulation_shaping(struct spi_device *spi, enum modShaping
modShaping)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: mod shaping");
#endif
@@ -145,7 +147,7 @@ int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate)
u8 msb;
u8 lsb;
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: bit rate");
#endif
@@ -177,18 +179,18 @@ int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate)
int rf69_set_deviation(struct spi_device *spi, u32 deviation)
{
int retval;
-// u32 f_max; TODO: Abh�ngigkeit von Bitrate beachten!!
+// u32 f_max; TODO: Abhaengigkeit von Bitrate beachten!!
u64 f_reg;
u64 f_step;
u8 msb;
u8 lsb;
u64 factor = 100; // to improve precision of calculation
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: deviation");
#endif
- if (deviation < 600 || deviation > 50) { //TODO: Abh�ngigkeit von
Bitrate beachten!!
+ if (deviation < 600 || deviation > 50) { //TODO: Abhaengigkeit von
Bitrate beachten!!
dev_dbg(&spi->dev, "set_deviation: illegal input param");
return -EINVAL;
}
@@ -233,7 +235,7 @@ int rf69_set_frequency(struct spi_device *spi, u32
frequency)
u8 lsb;
u64 factor = 100; // to improve precision of calculation
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: frequency");
#endif
@@ -274,7 +276,7 @@ int rf69_set_frequency(struct spi_device *spi, u32
frequency)
int rf69_set_amplifier_0(struct spi_device *spi, enum optionOnOff optionOnOff)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: amp #0");
#endif
@@ -289,7 +291,7 @@ int rf69_set_amplifier_0(struct spi_device *spi, enum
optionOnOff optionOnOff)
int rf69_set_amplifier_1(struct spi_device *spi, enum optionOnOff optionOnOff)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: amp #1");
#endif
@@ -304,7 +306,7 @@ int rf69_set_amplifier_1(struct spi_device *spi, enum
optionOnOff optionOnOff)
int rf69_set_amplifier_2(struct spi_device *spi, enum optionOnOff optionOnOff)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: amp #2");
#endif
@@ -319,11 +321,11 @@ int rf69_set_amplifier_2(struct spi_device *spi, enum
optionOnOff optionOnOff)
int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: power level");
#endif
- powerLevel += 18; // TODO Abh�ngigkeit von PA0,1,2 setting
+ powerLevel += 18; // TODO Abhaengigkeit von PA0,1,2 setting
/
