[core] allow I2C_[SDA|SCL]_PAL_MODE to be (p)redefined

[JohSchneider]

some i2c devices might not work properly with the pre-set default values on the i2c pads

allow users to redefine/reconfigure the i2c pads on chibios-based builds by wrapping the define in a 'ifndef'

Description

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[fauxpark]

I'm not really sure why these are being set in chibios_config.h in the first place; they should probably be moved to i2c_master.c: https://github.com/qmk/qmk_firmware/blob/master/platforms/chibios/drivers/i2c_master.c#L33

[JohSchneider]

note that these are in a "#if defined(MCU_RP)" block!
so it might make sense to have them in the chibios_config.h instead of the i2c_master.H

[JohSchneider]

... added to the commit message that this targets RP2040

[fauxpark]

note that these are in a "#if defined(MCU_RP)" block! so it might make sense to have them in the chibios_config.h instead of the i2c_master.H

But now these defaults are fragmented across multiple files. chibios_config.h, as far as I understand it, is meant to be a translation layer (for example the GD32 and WB32 stuff), so it should not contain anything that might need to be overridden.

[JohSchneider]

shuffled things around - like this then?

[sigprof]

But now these defaults are fragmented across multiple files. chibios_config.h, as far as I understand it, is meant to be a translation layer (for example the GD32 and WB32 stuff), so it should not contain anything that might need to be overridden.

But should the supposedly generic i2c_master.c driver contain RP2040-specific code?

Maybe chibios_config.h should unconditionally define something like DEFAULT_I2C_SCL_PAL_MODE, and then i2c_master.c would use it as the default value for the driver config option?

[KarlK90]

some i2c devices might not work properly with the pre-set default values on the i2c pads

@JohSchneider out of curiosity what are the changes in the modes did you had to do in order to get i2c working with you device? Maybe the defaults could need a change.

[KarlK90]

But should the supposedly generic i2c_master.c driver contain RP2040-specific code?

@zvecr requested to move the mcu specific logic/data out of i2c_master.c/h when I first implemented rp2040 support, which I very much agree to. There are ideas to have mcu specific logic in vendor/mcu/mcu family specific configuration files but in the meantime I think chibios_config.h is fine for that purpose.

[fauxpark]

But should the supposedly generic i2c_master.c driver contain RP2040-specific code?

There is already plenty of MCU-specific code in these drivers. It's difficult to avoid when we have to support all these different families, though ChibiOS does help out a lot.

I guess the issue is that the RP2040's default I2C pin configuration is more specific than simply "open drain", therefore we can't just define it for PAL_MODE_ALTERNATE_OPENDRAIN in chibios_config.h as other drivers also use it. This approach is fine when dealing with STM32 parts and "clones" which are effectively the same but with different naming, but it breaks down when you throw something like the 2040 into the mix...

[sigprof]

The problem with RP2040 is that it does not implement the open drain mode for GPIOs at all — it is possible to emulate it by setting the pin to output 0 and manipulating the output enable bit appropriately, but the existing GPIO code does not try to implement that workaround, therefore PAL_OUTPUT_TYPE_OPENDRAIN is defined to something which fails the build.

BTW, a similar problem could appear with WB32 (the code in chibios_config.h unconditionally defines SPI_SCK_FLAGS) and GD32VF103 (AUDIO_PWM_PAL_MODE). Maybe the proper way is to wrap all defines that are also driver config options in #ifndef and live with the resulting verbosity?

Another difference between placing the define in chibios_config.h or the driver code itself is that the define in the driver code won't be visible globally, but the define in chibios_config.h would be visible almost everywhere.

[KarlK90]

some i2c devices might not work properly with the pre-set default values on the i2c pads

@JohSchneider out of curiosity what are the changes in the modes did you had to do in order to get i2c working with you device? Maybe the defaults could need a change.

To answer my own question, in #17902 you wrote:

removing 'PAL_RP_PAD_DRIVE4' which the portexpander in the left keyboard half doesn't like alternative solution would be to add a 100pF ceramic-capacitor between SCL and GND

But the default pad drive strength is already 4ma both in ChibiOS and as the default value after boot-up in the rp2040. So (in theory) that change should have no effect.

[JohSchneider]

empirical practice lead me to the "remove PAD_DRIVE4 == works" conclusion s-:
the i2c_clock speed is also sth a user might want to change - which is probably why it's already in a "if !defined" block?

[sigprof]

But the default pad drive strength is already 4ma both in ChibiOS and as the default value after boot-up in the rp2040.

If the drive strength is not specified, it probably ends up being 2, not 4:

#define PAL_RP_PAD_DRIVE2                   (0U << (24 + 4))
#define PAL_RP_PAD_DRIVE4                   (1U << (24 + 4))
#define PAL_RP_PAD_DRIVE8                   (2U << (24 + 4))
#define PAL_RP_PAD_DRIVE12                  (3U << (24 + 4))

[KarlK90]

If the drive strength is not specified, it probably ends up being 2, not 4:

Oh well, you are completely right. I forgot that we override the whole register with the new values.

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[JohSchneider]

un-stale: rebasing onto current develop

any chance to get a consensus - and possibly approval? :-)

[sigprof]

Does the current code (#ifndef in chibios_config.h, added by #20314) work for you? A config.h override for those modes should work now.

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[github-actions]

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]