dev: ti: mspm0: l: Add common pinmux for MSPM0L111x - #80
Conversation
ssekar15
left a comment
There was a problem hiding this comment.
as far I checked most of configuration matches with existing l222x dtsi. Please resuse the existing dtsi file as include and add delta on high variants.
use name as per datasheet e.g . tima_fault1_pa0 not tima_fal1_pa0.
|
I didn't notice that l222x and l111x are that similar. I agree with your suggestion to have a common base for both with delta where appropriate. As for the naming conventions, I'll use the format of l222x. But I do have a question here. For the SPI CS pins it has the name of |
|
@ssekar15 while in the process of creating the common base, I discover that after PINCM20 the configurations are no longer shared. Do you still want to have this part shared? Also I notice that in my configuration, IOMUX REG is off, it seems I used PT PIN by mistake, but when I tested this on the EV board, the led was working. I guess it was just a happy coincidence. Anyway, I'll be waiting for your feedback before continuing the work on this. |
The naming conviction is followed from the
yes, please create common base until PINCM20. |
| pinmux = <MSP_PINMUX(74, MSPM0_PIN_FUNCTION_9)>; | ||
| }; | ||
| }; | ||
| /omit-if-no-ref/ comp0_out_pa27: comp0_out_pa27 { |
There was a problem hiding this comment.
I think most of the diffs in this file are due to the change in formatting.
There was a problem hiding this comment.
the formatting/indentation change is due to the fact that the root and soc nodes were moved into the common dtsi file, and the pinctrl node is reference here only for the additions needed for mspm0l222x variant.
| pinmux = <MSP_PINMUX(1, MSPM0_PIN_FUNCTION_2)>; | ||
| }; | ||
|
|
||
| /omit-if-no-ref/ ic20_sda_pa0: ic20_sda_pa0 { |
Since MSPM0L111x and MSPM0L222x have a common pin configuration for some of the pins, the mspm0l111x-222x-common-pinctrl.dtsi file is added with the shared pin configuration. The MSPM0L222x dtsi is modified to take advantage of this. The naming convetion was kept from the original 222x variant, with some modifications for SPI CS pins which didn't correctly reflect their function. For example, there was: `spi0_cs3_cd_poci3_pa01: spi0_cs3_cd_poci3_pa01` node for the PINCM2 function 10 which in data sheet is named only SPI0_CS3, and now the node is named: `spi0_cs3_pa01: spi0_cs3_pa01`. As for MSPM0L111x: - All pins support being used as GPIO. - Use input-enable for all PIN TYPE I on datasheet + SPI POCI pins. - Define MSPM0_PIN_FUNCTION_ANALOG for every pin which is unconnected state (essentially needed for low power consumption). Link: https://www.ti.com/lit/ds/symlink/mspm0l1116.pdf Link: https://www.ti.com/lit/ds/symlink/mspm0l2228.pdf Signed-off-by: Razvan Heghedus <razvan.heghedus@protonmail.com>
| pinmux = <MSP_PINMUX(1, MSPM0_PIN_FUNCTION_3)>; | ||
| input-enable; | ||
| drive-open-drain; | ||
| bias-disable; |
There was a problem hiding this comment.
Please add only mux setting in common dtsi, driver setting depends on board.
There was a problem hiding this comment.
I agree with this. Furthermore, I have concerns about maintainability and tradeoffs with other mspm0l variants. while MSPM0L122 and MSPM0L222X are the same family, MSPM0L111X devices are fundamentally a different family, and there's not a planned compatibility and continuity from the TI side, so I think we're better off keeping these files separate even if there are some similarities.
There was a problem hiding this comment.
Please add only mux setting in common dtsi, driver setting depends on board.
I can do that, but for reference, these were added with #66 and changing them might require updates to zephyrproject-rtos/zephyr#94627
| pinmux = <MSP_PINMUX(3, MSPM0_PIN_FUNCTION_5)>; | ||
| input-enable; | ||
| }; | ||
|
|
There was a problem hiding this comment.
It's not missing. For mspm0l222x PA28 MUX 6 is TIMG5_C0 and for mspm0l111x is TIMA0_C1. These both are in their specific dtsi
JFMSP
left a comment
There was a problem hiding this comment.
One note about structure, I don't know if the common file is the right approach here across different families.
There are other MSPM0L devices that can get added such as L1306 where deriving a clean common file will become really tricky.
| pinmux = <MSP_PINMUX(1, MSPM0_PIN_FUNCTION_3)>; | ||
| input-enable; | ||
| drive-open-drain; | ||
| bias-disable; |
There was a problem hiding this comment.
I agree with this. Furthermore, I have concerns about maintainability and tradeoffs with other mspm0l variants. while MSPM0L122 and MSPM0L222X are the same family, MSPM0L111X devices are fundamentally a different family, and there's not a planned compatibility and continuity from the TI side, so I think we're better off keeping these files separate even if there are some similarities.
@JFMSP |
My point here is I think mistakes are actually more likely because this common file won't scale to other MPSM0L devices. The presence of other pin functions apart from analog and GPIO as the first two is not shared because the rest of the peripheral elements are very different. So if we were to add MSPM0L1306, for example, the PA0 functions for that are:
and this starts to break the commonality in more places. Then either we ignore the common file for additional devices which then becomes clunky or we have to re-work the common file with each new device, neither of which I'm a big fan of. I think we have much more maintainable code long term if we deal with some duplication across different devices, because there's not any shared commonality we're enforcing from the TI side. |
|
@heghe Are you still working on this? |
|
Hi @Ayush1325, at the moment I don't have the time to work further on this. |
|
@ssekar15 @JFMSP Can you please explain what should be done to move this PR forward? I personally lean towards @JFMSP point of not trying to find commonality between families that exist by chance and not by overarching TI planning. Specifically, the various L series mspm0 socs seem to have wildly different capabilities. I have found a lot of incorrect nodes in zephyr soc dt for mspm0 due to the effort to extract common things. |
Link: https://www.ti.com/lit/ds/symlink/mspm0l1116.pdf