From d753cd61fca361cdf0567c28b5ec4684c70ef73b Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 9 Nov 2020 03:24:01 +0100 Subject: [PATCH 1/7] drivers/pijuice.c: replace __FUNCTION__ by __func__ to avoid warnings error: ISO C does not support '__FUNCTION__' predefined identifier [-Werror=pedantic] --- drivers/pijuice.c | 52 +++++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/drivers/pijuice.c b/drivers/pijuice.c index 2211bff82f..bac116b83b 100644 --- a/drivers/pijuice.c +++ b/drivers/pijuice.c @@ -291,9 +291,9 @@ static void get_charge_level_hi_res() uint8_t cmd = CHARGE_LEVEL_HI_RES_CMD; uint16_t data; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_WORD( upsfd, cmd, __FUNCTION__ ) + I2C_READ_WORD( upsfd, cmd, __func__ ) /* * Use an external variable to allow for missed i2c bus @@ -321,11 +321,11 @@ static void get_status() uint8_t data; char status_buf[ST_MAX_VALUE_LEN]; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); memset( status_buf, 0, ST_MAX_VALUE_LEN ); - I2C_READ_BYTE( upsfd, cmd, __FUNCTION__ ) + I2C_READ_BYTE( upsfd, cmd, __func__ ) uint8_t batteryStatus = data >> 2 & 0x03; switch( batteryStatus ) @@ -522,9 +522,9 @@ static void get_battery_temperature() uint8_t cmd = BATTERY_TEMPERATURE_CMD; int16_t data; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_WORD( upsfd, cmd, __FUNCTION__ ) + I2C_READ_WORD( upsfd, cmd, __func__ ) upsdebugx( 1, "Battery Temperature: %d°C", data ); dstate_setinfo( "battery.temperature", "%d", data ); @@ -535,9 +535,9 @@ static void get_battery_voltage() uint8_t cmd = BATTERY_VOLTAGE_CMD; int16_t data; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_WORD( upsfd, cmd, __FUNCTION__ ) + I2C_READ_WORD( upsfd, cmd, __func__ ) upsdebugx( 1, "Battery Voltage: %0.3fV", data / 1000.0 ); dstate_setinfo( "battery.voltage", "%0.3f", data / 1000.0 ); @@ -548,7 +548,7 @@ static void get_battery_current() uint8_t cmd = BATTERY_CURRENT_CMD; int16_t data; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); /* * The reported current can actually be negative, so we cannot @@ -570,9 +570,9 @@ static void get_io_voltage() uint8_t cmd = IO_VOLTAGE_CMD; int16_t data; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_WORD( upsfd, cmd, __FUNCTION__ ) + I2C_READ_WORD( upsfd, cmd, __func__ ) upsdebugx( 1, "Input Voltage: %.3fV", data / 1000.0 ); dstate_setinfo( "input.voltage", "%.3f", data / 1000.0 ); @@ -583,7 +583,7 @@ static void get_io_current() uint8_t cmd = IO_CURRENT_CMD; int16_t data; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); /* * The reported current can actually be negative, so we cannot @@ -606,9 +606,9 @@ static void get_firmware_version() uint16_t data; uint8_t major, minor; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_WORD( upsfd, cmd, __FUNCTION__ ) + I2C_READ_WORD( upsfd, cmd, __func__ ) major = data >> 4; minor = ( data << 4 & 0xf0 ) >> 4; @@ -627,9 +627,9 @@ static void get_battery_profile() uint8_t cmd = BATTERY_PROFILE_CMD; __u8 block[I2C_SMBUS_BLOCK_MAX]; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_BLOCK( upsfd, cmd, 14, block, __FUNCTION__ ) + I2C_READ_BLOCK( upsfd, cmd, 14, block, __func__ ) upsdebugx( 1, "Battery Capacity: %0.3fAh", ( block[1] << 8 | block[0] ) / 1000.0 ); dstate_setinfo( "battery.capacity", "%0.3f", ( block[1] << 8 | block[0] ) / 1000.0 ); @@ -640,9 +640,9 @@ static void get_battery_profile_ext() uint8_t cmd = BATTERY_EXT_PROFILE_CMD; __u8 block[I2C_SMBUS_BLOCK_MAX]; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_BLOCK( upsfd, cmd, 17, block, __FUNCTION__ ) + I2C_READ_BLOCK( upsfd, cmd, 17, block, __func__ ) switch( block[0] & 0xFF00 ) { @@ -665,9 +665,9 @@ static void get_power_off() uint8_t cmd = POWER_OFF_CMD; uint8_t data; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_BYTE( upsfd, cmd, __FUNCTION__ ) + I2C_READ_BYTE( upsfd, cmd, __func__ ) if ( data == 255 ) { @@ -684,7 +684,7 @@ static void set_power_off() uint8_t cmd = POWER_OFF_CMD; uint8_t data; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); /* * Acceptable values for shutdown_delay are 1-250, @@ -709,7 +709,7 @@ static void set_power_off() shutdown_delay = 1; } - I2C_WRITE_BYTE( upsfd, cmd, shutdown_delay, __FUNCTION__ ) + I2C_WRITE_BYTE( upsfd, cmd, shutdown_delay, __func__ ) } static void get_time() @@ -719,9 +719,9 @@ static void get_time() uint8_t second, minute, hour, day, month, subsecond; uint16_t year; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_BLOCK( upsfd, cmd, 9, block, __FUNCTION__ ) + I2C_READ_BLOCK( upsfd, cmd, 9, block, __func__ ) second = (( (block[0] >> 4 ) & 0x07) * 10 ) + ( block[0] & 0x0F ); minute = (( (block[1] >> 4 ) & 0x07) * 10 ) + ( block[1] & 0x0F ); @@ -743,9 +743,9 @@ static void get_i2c_address() uint8_t cmd = I2C_ADDRESS_CMD; uint8_t data; - upsdebugx( 3, __FUNCTION__ ); + upsdebugx( 3, __func__ ); - I2C_READ_BYTE( upsfd, cmd, __FUNCTION__ ) + I2C_READ_BYTE( upsfd, cmd, __func__ ) upsdebugx( 1, "I2C Address: 0x%0x", data ); From ab1275bdfd4e7db59d7590460e4638ee82debf7b Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 9 Nov 2020 03:26:13 +0100 Subject: [PATCH 2/7] drivers/pijuice.c: set_poweroff(): fix limited value range exceeded --- drivers/pijuice.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pijuice.c b/drivers/pijuice.c index bac116b83b..08c574ebc3 100644 --- a/drivers/pijuice.c +++ b/drivers/pijuice.c @@ -691,7 +691,7 @@ static void set_power_off() * use 0/255 to clear a scheduled power off command */ - if ( shutdown_delay > 255 ) + if ( shutdown_delay > 250 ) { upslogx( LOG_WARNING, From dda64eccd42770ac1e45c00b165087ff6dbef2cd Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 9 Nov 2020 04:32:21 +0100 Subject: [PATCH 3/7] drivers/pijuice.c: rearrange I2C_*() macros to catch -1 error codes while fetching unsigned ints --- drivers/pijuice.c | 43 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/drivers/pijuice.c b/drivers/pijuice.c index 08c574ebc3..36306f3288 100644 --- a/drivers/pijuice.c +++ b/drivers/pijuice.c @@ -245,22 +245,47 @@ upsdrv_info_t upsdrv_info = { { NULL } }; +/* The macros below all write into a "data" variable defined by the routine + * scope which calls them, with respective type of uint8_t for "byte" and + * uint16_t for "word" macros. Native i2c functions operate with s32 type + * (currently, signed 32-bit ints?) with negative values for error returns. + * Code below was fixed to convert the valid values and avoid compiler + * warnings about comparing whether unsigned ints happened to be negative. + */ #define I2C_READ_BYTE(fd, cmd, label) \ - if ((data = i2c_smbus_read_byte_data(upsfd, cmd)) < 0 ) { \ - upsdebugx(2, "Failure reading the i2c bus [%s]", label); \ - return; \ + { \ + s32 sData; \ + if ((sData = i2c_smbus_read_byte_data(upsfd, cmd)) < 0 ) { \ + upsdebugx(2, "Failure reading the i2c bus [%s]", label); \ + return; \ + } ; \ + data = (uint8_t) sData; \ } +/* FIXME? This code before fixing contained assignment to "data" so the fix + * retains that. Not sure this is logically right; maybe just checking for + * anonymous negative result should suffice. + * For the one currently existing use-case below this does not matter anyway, + * it is the last operation in a routine. + */ #define I2C_WRITE_BYTE(fd, cmd, value, label) \ - if ((data = i2c_smbus_write_byte_data(upsfd, cmd, value)) < 0 ) { \ - upsdebugx(2, "Failure writing to the i2c bus [%s]", label); \ - return; \ + { \ + s32 sData; \ + if ((sData = i2c_smbus_write_byte_data(upsfd, cmd, value)) < 0 ) { \ + upsdebugx(2, "Failure writing to the i2c bus [%s]", label); \ + return; \ + } ; \ + data = (uint8_t) sData; \ } #define I2C_READ_WORD(fd, cmd, label) \ - if ((data = i2c_smbus_read_word_data(upsfd, cmd)) < 0 ) { \ - upsdebugx(2, "Failure reading the i2c bus [%s]", label); \ - return; \ + { \ + s32 sData; \ + if ((sData = i2c_smbus_read_word_data(upsfd, cmd)) < 0 ) { \ + upsdebugx(2, "Failure reading the i2c bus [%s]", label); \ + return; \ + } ; \ + data = (uint16_t) sData; \ } #define I2C_READ_BLOCK(fd, cmd, size, block, label) \ From 944b9e40c300199c5fdece78178d5ec26a6727d3 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 9 Nov 2020 16:58:55 +0100 Subject: [PATCH 4/7] drivers/pijuice.c: use "__s32" de-facto in headers instead of "s32" from manpages --- drivers/pijuice.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/pijuice.c b/drivers/pijuice.c index 36306f3288..99897f3d54 100644 --- a/drivers/pijuice.c +++ b/drivers/pijuice.c @@ -247,14 +247,16 @@ upsdrv_info_t upsdrv_info = { /* The macros below all write into a "data" variable defined by the routine * scope which calls them, with respective type of uint8_t for "byte" and - * uint16_t for "word" macros. Native i2c functions operate with s32 type + * uint16_t for "word" macros. Native i2c functions operate with __s32 type * (currently, signed 32-bit ints?) with negative values for error returns. + * Note: some manpages refer to "s32" while headers on my and CI systems use + * a "__s32" type. Maybe this is something to determine in configure script? * Code below was fixed to convert the valid values and avoid compiler * warnings about comparing whether unsigned ints happened to be negative. */ #define I2C_READ_BYTE(fd, cmd, label) \ { \ - s32 sData; \ + __s32 sData; \ if ((sData = i2c_smbus_read_byte_data(upsfd, cmd)) < 0 ) { \ upsdebugx(2, "Failure reading the i2c bus [%s]", label); \ return; \ @@ -270,7 +272,7 @@ upsdrv_info_t upsdrv_info = { */ #define I2C_WRITE_BYTE(fd, cmd, value, label) \ { \ - s32 sData; \ + __s32 sData; \ if ((sData = i2c_smbus_write_byte_data(upsfd, cmd, value)) < 0 ) { \ upsdebugx(2, "Failure writing to the i2c bus [%s]", label); \ return; \ @@ -280,7 +282,7 @@ upsdrv_info_t upsdrv_info = { #define I2C_READ_WORD(fd, cmd, label) \ { \ - s32 sData; \ + __s32 sData; \ if ((sData = i2c_smbus_read_word_data(upsfd, cmd)) < 0 ) { \ upsdebugx(2, "Failure reading the i2c bus [%s]", label); \ return; \ From abd8b51448a29e63e35f74630b1874c7653b3056 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 9 Nov 2020 17:07:33 +0100 Subject: [PATCH 5/7] drivers/pijuice.c: avoid setting "data" during a write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compiler warnings resolved this puzzle: [2020-11-09 15:10:38] [build-err] pijuice.c:737:2: note: in expansion of macro ‘I2C_WRITE_BYTE’ [2020-11-09 15:10:38] [build-err] 737 | I2C_WRITE_BYTE( upsfd, cmd, shutdown_delay, __func__ ) [2020-11-09 15:10:38] [build-err] | ^~~~~~~~~~~~~~ [2020-11-09 15:10:38] [build-err] pijuice.c:710:10: warning: variable ‘data’ set but not used [-Wunused-but-set-variable] [2020-11-09 15:10:38] [build-err] 710 | uint8_t data; [2020-11-09 15:10:38] [build-err] | ^~~~ So we did not need to set it at all, can go to simple anonymous negative-return check. --- drivers/pijuice.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/pijuice.c b/drivers/pijuice.c index 99897f3d54..1facf6ceaa 100644 --- a/drivers/pijuice.c +++ b/drivers/pijuice.c @@ -264,20 +264,12 @@ upsdrv_info_t upsdrv_info = { data = (uint8_t) sData; \ } -/* FIXME? This code before fixing contained assignment to "data" so the fix - * retains that. Not sure this is logically right; maybe just checking for - * anonymous negative result should suffice. - * For the one currently existing use-case below this does not matter anyway, - * it is the last operation in a routine. - */ #define I2C_WRITE_BYTE(fd, cmd, value, label) \ { \ - __s32 sData; \ - if ((sData = i2c_smbus_write_byte_data(upsfd, cmd, value)) < 0 ) { \ + if ( i2c_smbus_write_byte_data(upsfd, cmd, value) < 0 ) { \ upsdebugx(2, "Failure writing to the i2c bus [%s]", label); \ return; \ } ; \ - data = (uint8_t) sData; \ } #define I2C_READ_WORD(fd, cmd, label) \ From c5a2ff7dfbf75cd4ca34e7977f451820f7fa5601 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 9 Nov 2020 19:07:48 +0100 Subject: [PATCH 6/7] drivers/pijuice.c: set_power_off(): drop unused "data" --- drivers/pijuice.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/pijuice.c b/drivers/pijuice.c index 1facf6ceaa..8214771260 100644 --- a/drivers/pijuice.c +++ b/drivers/pijuice.c @@ -701,7 +701,6 @@ static void get_power_off() static void set_power_off() { uint8_t cmd = POWER_OFF_CMD; - uint8_t data; upsdebugx( 3, __func__ ); From 08af6932372c94f71f4a5950d631d1a733916af9 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 9 Nov 2020 17:04:34 +0100 Subject: [PATCH 7/7] drivers/al175.c: update Linux i2c include mesh (thanks to pijuice.c) --- drivers/asem.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/drivers/asem.c b/drivers/asem.c index 30c193f9c3..fa88210732 100644 --- a/drivers/asem.c +++ b/drivers/asem.c @@ -35,8 +35,28 @@ #include #include #include -/* Depends on i2c-dev.h, Linux only */ -#include + +/* Depends on i2c-dev.h, Linux only + * Linux I2C userland is a bit of a mess until distros refresh to + * the i2c-tools 4.x release that profides i2c/smbus.h for userspace + * instead of (re)using linux/i2c-dev.h, which conflicts with a + * kernel header of the same name. + * + * See: + * https://i2c.wiki.kernel.org/index.php/Plans_for_I2C_Tools_4 + */ +#if HAVE_LINUX_SMBUS_H +# include +#endif +#if HAVE_LINUX_I2C_DEV_H +# include /* for I2C_SLAVE */ +# if !HAVE_LINUX_SMBUS_H +# ifndef I2C_FUNC_I2C +# include +# endif +# endif +#endif + #include #ifndef __STR__