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 <[email protected]>
---
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 = 1000000; // to improve precision of calculation
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: deviation");
#endif
- if (deviation < 600 || deviation > 500000) { //TODO: Abh?ngigkeit von Bitrate beachten!!
+ if (deviation < 600 || deviation > 500000) { //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 = 1000000; // 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
// check input value
if (powerLevel > 0x1f) {
@@ -337,7 +339,7 @@ int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel)
int rf69_set_pa_ramp(struct spi_device *spi, enum paRamp paRamp)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: pa ramp");
#endif
@@ -366,7 +368,7 @@ int rf69_set_pa_ramp(struct spi_device *spi, enum paRamp paRamp)
int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance antennaImpedance)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: antenna impedance");
#endif
@@ -381,7 +383,7 @@ int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance ant
int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: lna gain");
#endif
@@ -403,7 +405,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
{
u8 currentValue;
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "get: lna gain");
#endif
@@ -440,7 +442,7 @@ int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dc
int rf69_set_dc_cut_off_frequency(struct spi_device *spi, enum dccPercent dccPercent)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: cut off freq");
#endif
@@ -449,7 +451,7 @@ int rf69_set_dc_cut_off_frequency(struct spi_device *spi, enum dccPercent dccPer
int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum dccPercent dccPercent)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: cut off freq during afc");
#endif
@@ -502,7 +504,7 @@ static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8 exponent)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: band width");
#endif
@@ -511,7 +513,7 @@ int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8 expone
int rf69_set_bandwidth_during_afc(struct spi_device *spi, enum mantisse mantisse, u8 exponent)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: band width during afc");
#endif
@@ -520,7 +522,7 @@ int rf69_set_bandwidth_during_afc(struct spi_device *spi, enum mantisse mantisse
int rf69_set_ook_threshold_type(struct spi_device *spi, enum thresholdType thresholdType)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: threshold type");
#endif
@@ -536,7 +538,7 @@ int rf69_set_ook_threshold_type(struct spi_device *spi, enum thresholdType thres
int rf69_set_ook_threshold_step(struct spi_device *spi, enum thresholdStep thresholdStep)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: threshold step");
#endif
@@ -557,7 +559,7 @@ int rf69_set_ook_threshold_step(struct spi_device *spi, enum thresholdStep thres
int rf69_set_ook_threshold_dec(struct spi_device *spi, enum thresholdDecrement thresholdDecrement)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: threshold decrement");
#endif
@@ -583,7 +585,7 @@ int rf69_set_dio_mapping(struct spi_device *spi, u8 DIONumber, u8 value)
u8 regaddr;
u8 regValue;
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: DIO mapping");
#endif
@@ -623,7 +625,7 @@ int rf69_set_dio_mapping(struct spi_device *spi, u8 DIONumber, u8 value)
bool rf69_get_flag(struct spi_device *spi, enum flag flag)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "get: flag");
#endif
@@ -651,7 +653,7 @@ bool rf69_get_flag(struct spi_device *spi, enum flag flag)
int rf69_reset_flag(struct spi_device *spi, enum flag flag)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "reset: flag");
#endif
@@ -667,7 +669,7 @@ int rf69_reset_flag(struct spi_device *spi, enum flag flag)
int rf69_set_rssi_threshold(struct spi_device *spi, u8 threshold)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: rssi threshold");
#endif
@@ -678,7 +680,7 @@ int rf69_set_rssi_threshold(struct spi_device *spi, u8 threshold)
int rf69_set_rx_start_timeout(struct spi_device *spi, u8 timeout)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: start timeout");
#endif
@@ -689,7 +691,7 @@ int rf69_set_rx_start_timeout(struct spi_device *spi, u8 timeout)
int rf69_set_rssi_timeout(struct spi_device *spi, u8 timeout)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: rssi timeout");
#endif
@@ -703,7 +705,7 @@ int rf69_set_preamble_length(struct spi_device *spi, u16 preambleLength)
int retval;
u8 msb, lsb;
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: preamble length");
#endif
@@ -716,13 +718,13 @@ int rf69_set_preamble_length(struct spi_device *spi, u16 preambleLength)
/* transmit to chip */
retval = WRITE_REG(REG_PREAMBLE_MSB, msb);
if (retval)
return WRITE_REG(REG_PREAMBLE_LSB, lsb);
}
int rf69_set_sync_enable(struct spi_device *spi, enum optionOnOff optionOnOff)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: sync enable");
#endif
@@ -737,7 +739,7 @@ int rf69_set_sync_enable(struct spi_device *spi, enum optionOnOff optionOnOff)
int rf69_set_fifo_fill_condition(struct spi_device *spi, enum fifoFillCondition fifoFillCondition)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: fifo fill condition");
#endif
@@ -752,7 +754,7 @@ int rf69_set_fifo_fill_condition(struct spi_device *spi, enum fifoFillCondition
int rf69_set_sync_size(struct spi_device *spi, u8 syncSize)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: sync size");
#endif
@@ -768,7 +770,7 @@ int rf69_set_sync_size(struct spi_device *spi, u8 syncSize)
int rf69_set_sync_tolerance(struct spi_device *spi, u8 syncTolerance)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: sync tolerance");
#endif
@@ -786,7 +788,7 @@ int rf69_set_sync_values(struct spi_device *spi, u8 syncValues[8])
{
int retval = 0;
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: sync values");
#endif
@@ -804,7 +806,7 @@ int rf69_set_sync_values(struct spi_device *spi, u8 syncValues[8])
int rf69_set_packet_format(struct spi_device *spi, enum packetFormat packetFormat)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: packet format");
#endif
@@ -819,7 +821,7 @@ int rf69_set_packet_format(struct spi_device *spi, enum packetFormat packetForma
int rf69_set_crc_enable(struct spi_device *spi, enum optionOnOff optionOnOff)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: crc enable");
#endif
@@ -834,7 +836,7 @@ int rf69_set_crc_enable(struct spi_device *spi, enum optionOnOff optionOnOff)
int rf69_set_adressFiltering(struct spi_device *spi, enum addressFiltering addressFiltering)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: address filtering");
#endif
@@ -850,7 +852,7 @@ int rf69_set_adressFiltering(struct spi_device *spi, enum addressFiltering addre
int rf69_set_payload_length(struct spi_device *spi, u8 payloadLength)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: payload length");
#endif
@@ -859,7 +861,7 @@ int rf69_set_payload_length(struct spi_device *spi, u8 payloadLength)
u8 rf69_get_payload_length(struct spi_device *spi)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "get: payload length");
#endif
@@ -868,7 +870,7 @@ u8 rf69_get_payload_length(struct spi_device *spi)
int rf69_set_node_address(struct spi_device *spi, u8 nodeAddress)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: node address");
#endif
@@ -877,7 +879,7 @@ int rf69_set_node_address(struct spi_device *spi, u8 nodeAddress)
int rf69_set_broadcast_address(struct spi_device *spi, u8 broadcastAddress)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: broadcast address");
#endif
@@ -886,7 +888,7 @@ int rf69_set_broadcast_address(struct spi_device *spi, u8 broadcastAddress)
int rf69_set_tx_start_condition(struct spi_device *spi, enum txStartCondition txStartCondition)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: start condition");
#endif
@@ -903,7 +905,7 @@ int rf69_set_fifo_threshold(struct spi_device *spi, u8 threshold)
{
int retval;
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: fifo threshold");
#endif
@@ -924,7 +926,7 @@ int rf69_set_fifo_threshold(struct spi_device *spi, u8 threshold)
int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: dagc");
#endif
@@ -950,11 +952,9 @@ int rf69_read_fifo (struct spi_device *spi, u8 *buffer, unsigned int size)
int retval;
if (size > FIFO_SIZE) {
- #ifdef DEBUG
- dev_dbg(&spi->dev, "read fifo: passed in buffer bigger then internal buffer \n");
- #endif
+ dev_dbg(&spi->dev, "read fifo: passed in buffer bigger then internal buffer \n");
return -EMSGSIZE;
}
@@ -970,7 +970,7 @@ int rf69_read_fifo (struct spi_device *spi, u8 *buffer, unsigned int size)
dev_dbg(&spi->dev, "%d - 0x%x\n", i, local_buffer[i+1]);
#endif
- memcpy(buffer, &local_buffer[1], size); // TODO: ohne memcopy w?re sch?ner
+ memcpy(buffer, &local_buffer[1], size); // TODO: ohne memcopy waere schoener
return retval;
}
@@ -984,14 +984,12 @@ int rf69_write_fifo(struct spi_device *spi, u8 *buffer, unsigned int size)
u8 local_buffer[FIFO_SIZE + 1];
if (size > FIFO_SIZE) {
- #ifdef DEBUG
- dev_dbg(&spi->dev, "read fifo: passed in buffer bigger then internal buffer \n");
- #endif
+ dev_dbg(&spi->dev, "read fifo: passed in buffer bigger then internal buffer \n");
return -EMSGSIZE;
}
local_buffer[0] = spi_address;
- memcpy(&local_buffer[1], buffer, size); // TODO: ohne memcopy w?re sch?ner
+ memcpy(&local_buffer[1], buffer, size); // TODO: ohne memcopy waere schoener
#ifdef DEBUG_FIFO_ACCESS
for (i = 0; i < size; i++)
--
1.7.10.4
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> > > ---
> > > 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