Amit Barzilai <[email protected]> writes: Hello Amit,
[...] >> >> 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. > >>> + /* >>> + * 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. > Ok, great. >> 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. > Yeah, I agree with you here. >> 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. > That works for me. Or we could keep the family check for now, knowing that a follow-up series from you are going to remove it and properly handle it. > -- > Thanks, > Amit > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
