Hey Javier, I recently returned from a trip abroad and resumed work on v4. I am making good progress, but I would like to propose a change to the agreed-upon plan.
Javier Martinez Canillas <[email protected]> wrote: >>> Can we move this to the ssd130x-spi driver? For example, something like the >>> following might work: >>> >>> 1. Make ssd130x_write_cmds() to just be a static inline wrapper that calls >>> to regmap_raw_write(ssd130x->regmap, SSD13XX_COMMAND, cmd, len). >>> >>> 2. Make ssd130x_write_cmd() be a variadic wrapper around >>> ssd130x_write_cmds(). >>> >>> 3. Add your logic to ssd130x_spi_write() instead of ssd130x_write_cmds(), >>> that >>> way it stays in the correct layer rather than having a leaking >>> abstraction. >> >> I agree, in hindsight this code goes against the transport abstraction. >> I'd propose keeping 1 and 2 in a single patch, though. Making >> ssd130x_write_cmds() >> a wrapper around regmap_raw_write() and making ssd130x_write_cmd() a >> variadic wrapper >> around ssd130x_write_cmds() are two halves of the same change: routing >> command buffers >> through regmap_raw_write(). Splitting them would leave an intermediate state >> that isn't >> independently meaningful. Happy to split if you'd still prefer it. >> > > Yeah, as one patch is OK I think. > >>> Also, instead of checking for info->family_id == SSD135X_FAMILY, we could >>> add >>> a dc_high_params member (or whatever name is more suitable) to the struct >>> ssd130x_spi_transport Then other families that might use the same can just >>> reuse this option instead of checking for specific families. >> >> I agree that hard-coding a check for the family isn't open for extension and >> should be changed. >> Adding a member for this in ssd130x_spi_transport is a fitting solution, >> I'll populate it in >> ssd130x_spi_probe() using a static array that will describe which families >> need dc_high_params. >> I'd keep that table in ssd130x-spi.c rather than adding a flag to >> ssd130x_deviceinfo, so the >> D/C# concern stays in the SPI layer instead of leaking into the >> transport-agnostic device info. >> > > That works too. I don't have a strong preference on how should be > handled. As long as the logic remains in the SPI part of the driver. While implementing v4 I noticed a problem with step 1 above, and it made me reconsider the layering argument as well. First, the concrete issue. SSD13XX_COMMAND is 0x80, which as an I2C control byte is Co=1, D/C#=0. Per section 8.1.5.2 of the SSD1306 datasheet, Co=1 means exactly one payload byte follows and then another control byte - control and payload strictly alternate. So regmap_raw_write(ssd130x->regmap, SSD13XX_COMMAND, cmd, len) puts len bytes behind a control byte that promises one, and the controller parses cmd[1] as a control byte instead of passing it to the command decoder. This would break the code paths already using the I2C transport: most of ssd132x_init(), and also the per-frame path via ssd130x_set_col_range(), ssd130x_set_page_range() and ssd132x_update_rect(), so it is a runtime regression rather than only an init-time one. It is fixable: 0x00 is Co=0, D/C#=0, i.e. "the rest of this transaction is command bytes", which is exactly the semantics a burst needs. That is also why ssd130x_write_data() can already burst today - 0x40 is Co=0. But it changes I2C command framing for every existing chip, from one transfer per byte to one transfer per command, and I'd rather not do that as a side effect of adding a new controller. Second, the layering. Looking at it again, I think the comment I wrote was the misleading part: it described the behaviour as "D/C# HIGH" and "D/C# LOW", which makes it read as an SPI concern. The code under it only chose between SSD13XX_COMMAND and SSD13XX_DATA - the core's existing transport-neutral naming for the two paths - and left it to the transport to turn that into a pin level or a control byte. I should have described it in those terms to begin with. Furthermore, moving this logic to the transport layer could lead to code duplication. If a new controller that supports I2C is released with the same "parameters are considered data" requirement, the logic in ssd130x-spi.c would have to be duplicated in ssd130x-i2c.c - which today has no .write handler at all, since it uses the stock regmap_i2c bus. So for v4 I propose: - Keep ssd130x_write_cmds() writing SSD13XX_COMMAND per byte as it does today, so there is no change to I2C or SPI wire behaviour anywhere in the series. - Still do the ssd130x_write_cmd()/ssd130x_write_cmds() unification as its own prep patch, as you asked. It removes the duplicated variadic loop and gives a single place for the check below. - Add a bool cmd_params_are_data to ssd130x_deviceinfo, checked after the first byte is sent to decide how to send the parameters. If it is set, the parameters go through ssd130x_write_data(); otherwise the existing byte-by-byte loop is used. - Drop the separate SPI transport patch entirely. The comment in the core will be phrased in the core's own terms this time: parameters go on the data path rather than as further command bytes, without mentioning D/C or any other transport-specific information. Apologies for going back on something I already agreed to. If you still prefer it in the SPI layer I will do it that way and fold in the 0x00 control byte change, with the I2C framing change called out in the commit message, but I wanted to flag the breakage before building on it. I have already implemented the fixes for most of the other comments locally. Once we settle this, I will submit the finished v4. -- Thanks, Amit
