On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote: > On 4/29/25 8:48 AM, Jorge Marques wrote: > > Hi David, > > > > I didn't went through your's and Jonathan's ad4052.c review yet, > > but for the trigger-source-cells I need to dig deeper and make > > considerable changes to the driver, as well as hardware tests. > > My idea was to have a less customizable driver, but I get that it is > > more interesting to make it user-definable. > > We don't need to make the driver support all possibilities, but the devicetree > needs to be as complete as possible since it can't be as easily changed in the > future. > Ack. I see that the node goes in the spi controller (the parent). To use the same information in the driver I need to look-up the parent node, then the node. I don't plan to do that in the version of the driver, just an observation. There is something else I want to discuss on the dt-bindings actually. According to the schema, the spi-max-frequency is: > Maximum SPI clocking speed of the device in Hz. The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode (higher, depends on VIO). The solution I came up, to not require a custom regmap spi bus, is to have spi-max-frequency bound the Configuration mode speed, and have ADC Mode set by VIO regulator voltage, through spi_transfer.speed_hz. At the end of the day, both are bounded by the spi controller maximum speed. My concern is that having ADC mode speed higher than spi-max-frequency may be counter-intuitive, still, it allows to achieve the max data sheet speed considering VIO voltage with the lowest code boilerplate. Let me know if I can proceed this way before submitting V3. > ... > > >> > >> Assuming the diagram at [1] is correct, for SPI offload use, we are missing: > >> > >> #trigger-source-cells: > >> const: 2 > >> description: | > >> Output pins used as trigger source. > >> > >> Cell 0 defines which pin: > >> * 0 = GP0 > >> * 1 = GP1 > >> > >> Cell 1 defines the event: > >> * 0 = Data ready > >> * 1 = Min threshold > >> * 2 = Max threshold > >> * 3 = Either threshold > >> * 4 = Device ready > >> * 5 = Device enable > >> * 6 = Chop control > >> > >> Bonus points for adding a header with macros for the arbitrary event values. > > > > In the sense of describing the device and not what the driver does, I > > believe the proper mapping would be: > > > > Cell 1 defines the event: > > * 0 = Disabled > > * 1 = Data ready > > * 2 = Min threshold > > * 3 = Max threshold > > * 4 = Either threshold > > * 5 = CHOP control > > * 6 = Device enable > > * 7 = Device ready (only GP1) > > > > I will investigate further this. > > > >> > > 0 = Disabled doesn't make sense to me. One would just not wire up a > trigger-source in that case. Ack. Regards, Jorge
