sensor: bq274xx: add support for bq27427 and various improvements #60117

fabiobaltieri · 2023-07-06T22:00:05Z

Add support for BQ27247 and do various improvements to the driver while at it.

Add the bq274xx_ back to the static function prefixes for the bq274xx driver. These have been removed recently but every other sensor and most Zephyr driver have static function prefixed, this helps avoiding ambiguity in list files, stack traces, setting debugging breakpoints etc. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Drop unnecessary casting from uint16_t to uint8_t. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Move bq274xx_gauge_configure up in the code, remove the forward declaration. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

drivers/sensor/bq274xx/bq274xx.c

fabiobaltieri · 2023-07-08T18:02:15Z

Excellent comments @henrikbrixandersen, all done.

Drop various unnecessarily initialized variables from the driver. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Control reg write seems to support two bytes mode (the technical reference shows example of that), so use a single i2c_write_dt there. Also drop a couple alias variables from bq274xx_cmd_reg_write. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

fabiobaltieri · 2023-07-09T11:20:32Z

dropped the block zeroing/memset entirely, they are just re-read from the flash anyway, no point zeroing that data
realized the -247 variant that I'm adding has different configuration register offsets, so I've reworked the support patch to add offset support, ported over the original one and added the ones for the variant I'm working on, cross checked between the technical reference manuals [1][2] and the Linux driver [3].

Notes: I've opted for doing the device detection and set the offset table at runtime, the table is just 4 bytes of flash so I think no harm doing that and it's convenient to have a single compatible. If it was bigger it would have made sense to have the part specific compatible and only load the right table at build time, but from what I see there's not that many variants to deal with.

Now bq274xx_gauge_configure is a bit messed up, tmp_checksum is calculated and then not used and discarded down the code, read again and still not used. checksum_old is just read and never used. I tried to clean that part, deleting the stale code but then I was getting erroneous data from the fuel gauge (seems like the configuration change was not being loaded or gets corrupted). I suspect that those stale checksum code were an initial implementation of a setup code that does not require reading the whole block (which is described in the technical reference manual), then it got changed for a full recalculation of the checksum but the previous code was left over. Removing those read though appear to break the setup code, and I think it's because it's lacking some sleep in the right places, and those extra i2c operations happen to compensate for that. The delay story is also inconsistent, the driver defines BQ274XX_SUBCLASS_DELAY_MS but then uses it for every sleep but the subclass selection. Also the Linux driver has some scary comments in it: https://lxr.missinglinkelectronics.com/linux/drivers/power/supply/bq27xxx_battery.c#L1404.

So at this point the driver seems to work, I think I'll leave this PR at this: cleaning up the coding style errors and adding the support for the new variant. Then on a second stage I'll (maybe) look into studying the Linux driver a bit more and fixing up the setup sequence.

[1] https://www.ti.com/lit/ug/sluuac5c/sluuac5c.pdf
[2] https://www.ti.com/lit/ug/sluucd5/sluucd5.pdf
[3] https://lxr.missinglinkelectronics.com/linux/drivers/power/supply/bq27xxx_battery.c#L931

Add a couple of define for the data memory block size and config flag bit. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

drivers/sensor/bq274xx/bq274xx.c

nixward · 2023-07-10T03:05:11Z

drivers/sensor/bq274xx/bq274xx.c

+	if (id == BQ27421_DEVICE_ID) {
+		data->regs = &bq27421_regs;
+	} else if (id == BQ27427_DEVICE_ID) {
+		data->regs = &bq27427_regs;


Would it be better to do this at compile time to reduce flash usage?

There seems to be quite a few variations to potentially support: https://lxr.missinglinkelectronics.com/linux/drivers/power/supply/bq27xxx_battery.c#L136

Yeah I'm unsure about it, with these two we can get away with just 4 bytes of flash. If we introduce build time variations we'd have to go for part number specific compatibles and rework the driver a bit more and have downstream user change it in their application. I was thinking about trying to go with this, keep it simple for now and see if there's demand for the other chips in the family.

It could easily be done based on compatible at instantiation time, but can be done later, this PR is already doing a good cleanup

Actually I was starting to like the idea of keeping this in runtime: the two p/n right now are a pin-to-pin compatible, as it is now you can drop the new one and it should work with the same firmware image, if the cost for that is only few bytes of flash I'd say it may be worth it.

drivers/sensor/bq274xx/bq274xx.c

fabiobaltieri · 2023-07-10T09:45:31Z

Thanks folks, all addressed, great feedback.

fabiobaltieri · 2023-07-10T11:19:45Z

fixed the delay definitions properly

nixward

Great job! I'm loving with the new variant support and I2C improvements, all achieved with less lines of code!

drivers/sensor/bq274xx/bq274xx.c

fabiobaltieri · 2023-07-10T13:16:38Z

One more thought about this: the configuration function currently issue a soft reset every time, the right way to do would be to check if the configuration changed and leave the device alone if it's been set up already. The Linux driver has that logic, that'll be good to keep in mind for followup work on this driver.

drivers/sensor/bq274xx/bq274xx.c

Fix the calculation for designenergy_mwh, as right now it's using a float casted straight to an int, which results in the factor rounded from 3.7 to 3. Also rework both that and taperrate so that they don't use floating point. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Add a retry count limit to config update mode loops, this way the system can still boot if there's an issue with the device. The normal sleep should be enough for correct operation, adding a conservative limit. Rework the delays to be unambiguous while at it. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

The device technical reference manual says "The Sealed to Unsealed key has two identical words". Use two different defines with the same value in the code so it's somewhat less ambiguous that the double write is intentional. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Change bq274xx_sample_fetch to support SENSOR_CHAN_ALL. This makes it possible to get the sensor data using the sensor shell, besides being generally convenient. Also drop a redundant comment. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Use i2c_reg_write_byte_dt instead of bq274xx_cmd_reg_write. The wrapper does not add anything anyway. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

The current ID for BQ274XX_DEVICE_ID is actually the one for the BQ27421. The driver seems to work with the BQ27427 as well, at least the common and extended commands are the same, so add that variant as well, rename the existing one and print the currently read ID when the ID check fails. The configuration registers have a different offset though, so add a register offset table and make the device rcognize the right one un runtime based on the device ID. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Use a combination of sys_cpu_to_be16 and i2c_burst_write_dt for setting 16 bits registers. Get rid of a bunch of temporary variables, custom conversions and few bus writes. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

fabiobaltieri · 2023-07-13T11:11:28Z

@marco85mars20 @nixward hey you both have quite few contributions kicking around, I'd suggest filing https://github.com/zephyrproject-rtos/zephyr/issues/new?assignees=&labels=Role+Nomination&projects=&template=006_nomination.md&title= to get triage permissions.

gmarull · 2023-07-21T10:38:48Z

drivers/sensor/bq274xx/bq274xx.c

+		subcommand & 0xff,
+		subcommand >> 8,


note: we should likely be using sys/byteorder API

I'll followup on this, I forsee another stack of fixes anyway.

gmarull · 2023-07-21T10:41:58Z

drivers/sensor/bq274xx/bq274xx.c

+	if (id == BQ27421_DEVICE_ID) {
+		data->regs = &bq27421_regs;
+	} else if (id == BQ27427_DEVICE_ID) {
+		data->regs = &bq27427_regs;


It could easily be done based on compatible at instantiation time, but can be done later, this PR is already doing a good cleanup

fabiobaltieri added 3 commits July 6, 2023 22:32

sensor: bq274xx: drop unnecessary casting

6fefb03

Drop unnecessary casting from uint16_t to uint8_t. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

sensor: bq274xx: remove forward declaration

7dae5b9

Move bq274xx_gauge_configure up in the code, remove the forward declaration. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

fabiobaltieri requested a review from MaureenHelm as a code owner July 6, 2023 22:00

zephyrbot added the area: Sensors Sensors label Jul 6, 2023

fabiobaltieri changed the title Bq sensor: bq274xx: add support for bq27427 and various improvements Jul 6, 2023

zephyrbot requested review from avisconti, teburd and yperess July 6, 2023 22:00

zephyrbot assigned MaureenHelm Jul 6, 2023

fabiobaltieri requested review from henrikbrixandersen and gmarull July 6, 2023 22:06

henrikbrixandersen requested changes Jul 7, 2023

View reviewed changes

fabiobaltieri force-pushed the bq branch from 8af3f95 to fce4a17 Compare July 8, 2023 21:12

fabiobaltieri added 2 commits July 9, 2023 11:31

sensor: bq274xx: drop a bunch of redundant initialization

6bcc4a8

Drop various unnecessarily initialized variables from the driver. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

fabiobaltieri force-pushed the bq branch from fce4a17 to 542a931 Compare July 9, 2023 11:04

fabiobaltieri force-pushed the bq branch 2 times, most recently from 0e4066f to f34590d Compare July 9, 2023 15:01

sensor: bq274xx: add few defines for magic numbers

b218615

Add a couple of define for the data memory block size and config flag bit. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

fabiobaltieri force-pushed the bq branch 2 times, most recently from a72fae5 to fb2ca67 Compare July 9, 2023 15:39

fabiobaltieri mentioned this pull request Jul 9, 2023

drivers: sensor: bq274xx: add support for fetch SENSOR_CHAN_ALL #58387

Closed

marco85mars20 suggested changes Jul 10, 2023

View reviewed changes

drivers/sensor/bq274xx/bq274xx.c Outdated Show resolved Hide resolved

drivers/sensor/bq274xx/bq274xx.c Outdated Show resolved Hide resolved

drivers/sensor/bq274xx/bq274xx.c Show resolved Hide resolved

nixward reviewed Jul 10, 2023

View reviewed changes

drivers/sensor/bq274xx/bq274xx.c Outdated Show resolved Hide resolved

nixward reviewed Jul 10, 2023

View reviewed changes

drivers/sensor/bq274xx/bq274xx.c Outdated Show resolved Hide resolved

nixward reviewed Jul 10, 2023

View reviewed changes

drivers/sensor/bq274xx/bq274xx.c Outdated Show resolved Hide resolved

nixward reviewed Jul 10, 2023

View reviewed changes

drivers/sensor/bq274xx/bq274xx.c Show resolved Hide resolved

nixward reviewed Jul 10, 2023

View reviewed changes

drivers/sensor/bq274xx/bq274xx.c Show resolved Hide resolved

fabiobaltieri force-pushed the bq branch 3 times, most recently from b65c31c to 4c31e9c Compare July 10, 2023 11:18

nixward approved these changes Jul 10, 2023

View reviewed changes

drivers/sensor/bq274xx/bq274xx.c Outdated Show resolved Hide resolved

fabiobaltieri force-pushed the bq branch from 4c31e9c to b298326 Compare July 10, 2023 12:43

nixward reviewed Jul 11, 2023

View reviewed changes

drivers/sensor/bq274xx/bq274xx.c Outdated Show resolved Hide resolved

fabiobaltieri added 7 commits July 11, 2023 10:32

sensor: bq274xx: drop bq274xx_ctrl_reg_write

8003c85

Use i2c_reg_write_byte_dt instead of bq274xx_cmd_reg_write. The wrapper does not add anything anyway. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

sensor: bq274xx: use two bytes write for settings

5a3b15d

Use a combination of sys_cpu_to_be16 and i2c_burst_write_dt for setting 16 bits registers. Get rid of a bunch of temporary variables, custom conversions and few bus writes. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

fabiobaltieri force-pushed the bq branch from b298326 to 5a3b15d Compare July 11, 2023 09:33

fabiobaltieri requested review from henrikbrixandersen and marco85mars20 July 12, 2023 15:05

marco85mars20 approved these changes Jul 13, 2023

View reviewed changes

henrikbrixandersen approved these changes Jul 17, 2023

View reviewed changes

gmarull approved these changes Jul 21, 2023

View reviewed changes

MaureenHelm approved these changes Jul 21, 2023

View reviewed changes

MaureenHelm merged commit bb0135b into zephyrproject-rtos:main Jul 21, 2023

fabiobaltieri deleted the bq branch July 21, 2023 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sensor: bq274xx: add support for bq27427 and various improvements #60117

sensor: bq274xx: add support for bq27427 and various improvements #60117

fabiobaltieri commented Jul 6, 2023

fabiobaltieri commented Jul 8, 2023

fabiobaltieri commented Jul 9, 2023 •

edited

Loading

nixward Jul 10, 2023

nixward Jul 10, 2023

fabiobaltieri Jul 10, 2023

gmarull Jul 21, 2023

fabiobaltieri Jul 21, 2023 •

edited

Loading

fabiobaltieri commented Jul 10, 2023

fabiobaltieri commented Jul 10, 2023

nixward left a comment

fabiobaltieri commented Jul 10, 2023

fabiobaltieri commented Jul 13, 2023 •

edited

Loading

gmarull Jul 21, 2023

fabiobaltieri Jul 21, 2023

gmarull Jul 21, 2023

sensor: bq274xx: add support for bq27427 and various improvements #60117

sensor: bq274xx: add support for bq27427 and various improvements #60117

Conversation

fabiobaltieri commented Jul 6, 2023

fabiobaltieri commented Jul 8, 2023

fabiobaltieri commented Jul 9, 2023 • edited Loading

nixward Jul 10, 2023

Choose a reason for hiding this comment

nixward Jul 10, 2023

Choose a reason for hiding this comment

fabiobaltieri Jul 10, 2023

Choose a reason for hiding this comment

gmarull Jul 21, 2023

Choose a reason for hiding this comment

fabiobaltieri Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

fabiobaltieri commented Jul 10, 2023

fabiobaltieri commented Jul 10, 2023

nixward left a comment

Choose a reason for hiding this comment

fabiobaltieri commented Jul 10, 2023

fabiobaltieri commented Jul 13, 2023 • edited Loading

gmarull Jul 21, 2023

Choose a reason for hiding this comment

fabiobaltieri Jul 21, 2023

Choose a reason for hiding this comment

gmarull Jul 21, 2023

Choose a reason for hiding this comment

fabiobaltieri commented Jul 9, 2023 •

edited

Loading

fabiobaltieri Jul 21, 2023 •

edited

Loading

fabiobaltieri commented Jul 13, 2023 •

edited

Loading