Re: [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

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