tmedicci commented on PR #11428: URL: https://github.com/apache/nuttx/pull/11428#issuecomment-2539694262
> The point is that the entire RMT driver belongs to `arch` and should be moved there. Let's avoid using `RMT` generically. I suppose you didn't mean to move [drivers/rmt/rmtchar.c](https://github.com/apache/incubator-nuttx/blob/286d37026c3b7f3923a45d7c114200a6546ba030/drivers/rmt/rmtchar.c) to arch folder, right? It's clearly an upper-half driver and totally optional. It's just an interface to the lower-half. > > It isn't, it's used by either RISC-V and xtensa devices with the RMT peripheral. > > I can't agree here. It's vendor specific driver, only used by ESP chips, so it should be in `arch`. You've said earlier: > RMT upper-half itself seems to be wrong, as it's architecture specific driver. And I just said that it isn't **architecture specific**. It's vendor-specific (at least for now), not arch-specific. > RMT shouldn't be in `drivers/` in the first place, it belongs to `arch` (yes, I know, I'm repeating myself :P) and IR feature should be provided with already present driver that provides good and standardized abstraction: `drivers/rc/lirc_dev.c`. I suppose you are referring to [include/nuttx/rmt/rmt.h](https://github.com/apache/incubator-nuttx/blob/cd2fcf52520314bf375b4ffbf2ee527f53a0d23a/include/nuttx/rmt/rmt.h), right? If so, as I've said earlier: it isn't related to the upper-half driver and it can be moved to the arch. I'm not that convinced that it's in the wrong place because it's totally decoupled with the upper-half RMT driver ([drivers/rmt/rmtchar.c](https://github.com/apache/incubator-nuttx/blob/286d37026c3b7f3923a45d7c114200a6546ba030/drivers/rmt/rmtchar.c)) and its header file [(include/nuttx/rmt/rmtchar.h)](https://github.com/apache/incubator-nuttx/blob/cd2fcf52520314bf375b4ffbf2ee527f53a0d23a/include/nuttx/rmt/rmtchar.h), but we can move it to arch folder (duplicating it for xtensa and RISC-V). > I don't see a problem with WS2812, the problem is in RMT. Now I suppose you are referring to [drivers/rmt/rmtchar.c](https://github.com/apache/incubator-nuttx/blob/286d37026c3b7f3923a45d7c114200a6546ba030/drivers/rmt/rmtchar.c), right? If so, I said earlier that this upper-half driver isn't needed for WS2812. > and ws2812 as it's implemented now, but with the difference that esp-specific ws2812 implementation use esp-specific API for RMT. I'm not sure if I got your point here. The "common" API it uses is defined at [include/nuttx/rmt/rmt.h](https://github.com/apache/incubator-nuttx/blob/cd2fcf52520314bf375b4ffbf2ee527f53a0d23a/include/nuttx/rmt/rmt.h). If we simply move it to arch-folder without any modification it'd be called "esp-specific API for RMT"? If so, again, I'm not against it... > There is no feature that RMT implements that cannot be implemented with existing portable drivers. IR with `drivers/rc/lirc_dev.c` I agree, and there are a bunch of other features that could be implemented with the RMT peripheral (PWM, audio, DACs etc). Our main goal is to provide support for the peripherals. Unfortunately, we have limited resources to create bindings to all of those upper-half driver, so we implemented the RMT lower-half driver and bound it with an existing upper-half driver (ws2812). Then, we provided a generic API for users to deal directly with the RMT peripheral ([drivers/rmt/rmtchar.c](https://github.com/apache/incubator-nuttx/blob/286d37026c3b7f3923a45d7c114200a6546ba030/drivers/rmt/rmtchar.c)). As soon as people bind the lower-half driver with the existing upper-half drivers, RMT upper-half wouldn't be necessary anymore and can be removed. There is a difference between the good and the needed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
