OpenAnnePro / qmk_firmware

This has since been merged back to mainline QMK. Please use qmk/qmk_firmware
https://qmk.fm
GNU General Public License v2.0
195 stars 64 forks source link

Add persistent led support with eeprom #9

Closed tech2077 closed 4 years ago

tech2077 commented 4 years ago

A few things that needed to be added to get persistent led support:

Codetector1374 commented 4 years ago

It seems like this does belong to lld. Maybe I am understanding this code wrong. But if I recall correctly the stm32 lld SPI driver does something quite close. Basically you can call the SPI to shift in / out data and it will just do it. ( ofc you need to give it configuration and tell it to startSpi() or something like that On Aug 23, 2020, 3:37 AM -0400, tech2077 notifications@github.com, wrote:

@tech2077 commented on this pull request. In drivers/chibios/spi_master.c:

@@ -32,6 +32,10 @@ attribute((weak)) void spi_init(void) { palSetPadMode(PAL_PORT(SPI_SCK_PIN), PAL_PAD(SPI_SCK_PIN), PAL_MODE_STM32_ALTERNATE_PUSHPULL); palSetPadMode(PAL_PORT(SPI_MOSI_PIN), PAL_PAD(SPI_MOSI_PIN), PAL_MODE_STM32_ALTERNATE_PUSHPULL); palSetPadMode(PAL_PORT(SPI_MISO_PIN), PAL_PAD(SPI_MISO_PIN), PAL_MODE_STM32_ALTERNATE_PUSHPULL); +#elif defined(HT32_SPI_USE_SPI1) || defined(HT32_SPI_USE_SPI2) Yeah, I don't like how QMK does it, the main reason I did it this was is in the case we ever try to merge into mainline QMK. Where in ChibiOS would I put this, it feels too setup specific for lld, and to low level for chibios main hal. Usually I see gpio mode config done outside peripheral libraries. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

tech2077 commented 4 years ago

I took a look at the lld for the STM32 SPI driver and they don't seem to setup the gpio mode anywhere. All this code does is enable the GPIO, it's not actually related to the SPI peripheral directly. Also the SPI HAL driver expects the config to be setup on the user layer before being passed to the HAL and lld layers.

I think we should keep it here, as ugly as it is here I think it's better than introducing GPIO configuration into any of the more "pure" drivers.

Codetector1374 commented 4 years ago

Okay sounds good to me. I guess this is a very valid point. <3 thanks for you work. On Aug 23, 2020, 9:13 PM -0400, tech2077 notifications@github.com, wrote:

I took a look at the lld for the STM32 SPI driver and they don't seem to setup the gpio mode anywhere. All this code does is enable the GPIO, it's not actually related to the SPI peripheral directly. Also the SPI HAL driver expects the config to be setup on the user layer before being passed to the HAL and lld layers. I think we should keep it here, as ugly as it is here I think it's better than introducing GPIO configuration into any of the more "pure" drivers. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.