kywwilson11 commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353714144
########## arch/arm/src/stm32h5/stm32_adc.h: ########## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: @acassis I agree with you. I understand the need for consistency across the STM32 family. I really do and I am not doing this to be difficult. Before I implement any driver for the STM32H5, I always look at other implementations for consistency sake but also because I do not need to re-do what has already been done. But to accomplish what I need, I thought that the stm32_adc_channel_s structure was the best and cleanest way to pass information I need for the board side to the lower-level driver side. My goal with this pull request was to do 2 things primarily: 1. Add support for differential ADC inputs. 2. Add support for the ADC Watchdogs. I'm adding only code for watchdog1 because the upper level driver does not currently support more than 1 watchdog with the ioctls available. I am remaining steadfast in my position to keep this stm32_adc_channel_s structure. Below is why: 1. The main reason for adding the stm32_adc_channel_s structure was for the differential mode implementation. There is no precedent for this in the stm32 architectures that I found. So nothing to reference. Also, I'm not sure of a better way to pass information that is completely board related. Just like the gpios, which channels are differential is specific to the board implementation, not the architecture. Passing an array of uint8_t types that define the channel is just not enough. How else would I tell the lower driver that a given channel, defined on the board side, is differential. Kconfig is possible but makes less sense than this. 2. The only code actually affected by this structure is code related to the STM32H5. There is currently 1 board supported by the STM32H5, which is the nucleo-h563zi. This pull request updates that board to use this structure. 3. The structure allows me to easily configure sampletime for each channel on a per-channel basis. I find this to be a huge improvement over the implementation in the F7. 4. Other than it being different than what has been done before, I have not seen any valid criticisms of the structure or given potential alternative solutions. It works and does have precedent with the nrf52 at least. Ideally, I do update the stm32f7, stm32h7, and probably other architectures to use this implementation. But as I said before, I am not paid by Apache NuttX. I'm not saying this to be a jerk. I am contributing to NuttX with an application/product in mind for the company that pays me. So in summary, I would like to leave the PR as is with the stm32_adc_channel_s structure. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
