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

[Javier Martinez Canillas <javierm@redhat.com>]

Amit Barzilai <amit.barzilai22@gmail.com> 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