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

[Amit Barzilai <amit.barzilai22@gmail.com>]

Javier Martinez Canillas <javierm@redhat.com> 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