Javier Martinez Canillas <[email protected]> wrote: >> */ >> -static int ssd130x_write_data(struct ssd130x_device *ssd130x, u8 *values, >> int count) >> +static int ssd130x_write_data(struct ssd130x_device *ssd130x, const u8 >> *values, int count) >> { >> return regmap_bulk_write(ssd130x->regmap, SSD13XX_DATA, values, count); >> } > >> > > This change is correct but need to be split in a separate preparatory > patch. When you do that, feel free to add my Reviewed-by tag.
Agreed, adding to v4. > Please split these ssd130x_write_cmds() and ssd1330x_write_cmd() refactoring > as separate preparatory patches. This patch is changing too many things at > once, it is better to have each logical changes as a separate patch. This > makes reviewer much easier. Ditto. > I don't like that this logic is in the ssd130x core part of the driver. This > is supposed to be transport agonistic and should not be aware of the D/C# > behaviour that is specific to the SPI transport. > > 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. > 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. >> + /* >> + * No long post-reset delay: the controller datasheets only specify >> + * microsecond-scale reset timing. The lengthy delay seen in some other >> + * drivers comes from fbtft's generic reset helper and targets slow >> + * parallel-bus GPIOs, which do not apply here. >> + */ >> } >> > > I don't understand the point of this comment. You are not changing the logic > of this function nor I see how this comment is useful. I originally added a redundant delay that I saw in fb_ssd1351.c, when Andy noticed it he told me to add a comment so others could understand the difference between this driver and the fb counterpart. I agree that it shouldn't be included in v4, it was a specific mistake and I highly doubt others will see similar situations and if so they can easily research the subject. > Instead of ssd133x_write_pixels() and having per family logic in the primary > place update path, I prefer to have a little bit more of code duplication > and add an .atomic_update, .atomic_disable, etc for the SSD1351. > > Yes, I know that your current approach reduces code duplication but also > makes it harder to change the primary plane update path for a family without > affecting another one. This is particularly important in my opinion, given > that contributors usually just test on their display family when posting > the patches. So I prefer to have per family callbacks if possible. Agreed, I'll add per-family .atomic_update/.atomic_disable for SSD135X in v4 and drop the shared ssd133x_write_pixels() path. The one thing I'd keep shared is the atomic_check callbacks (ssd133x_primary_plane_atomic_check and ssd133x_crtc_atomic_check): they have no family-specific branching - the plane check is generic and the CRTC check only allocates the RGB565 data_array, which SSD135X needs identically. So I'd reuse them via the table rather than duplicate. Let me know if you'd rather those were split too. > Again I wonder if is better to have a device_info member to decide whether to > register the backlight, instead of harcoding the check for a specific family. I already have a draft for splitting ssd130x_update_bl() to be per family, removing the need for this code. I prefer to first submit that patch, and then mention it as a dependency for v4 of this series. Feel free to suggest a different solution. -- Thanks, Amit
