Re: [PATCH v10 2/2] drm: add kms driver for loongson display controller

[Sui Jingfeng <15330273260@189.cn>]

Hi,

On 2023/4/4 22:10, Emil Velikov wrote:
>> --- /dev/null
>> +++ b/drivers/gpu/drm/loongson/lsdc_drv.c
>> +static const struct lsdc_desc dc_in_ls7a1000 = {
>> +       .chip = CHIP_LS7A1000,
>> +       .num_of_crtc = LSDC_NUM_CRTC,
>> +       .max_pixel_clk = 200000,
>> +       .max_width = 2048,
>> +       .max_height = 2048,
>> +       .num_of_hw_cursor = 1,
>> +       .hw_cursor_w = 32,
>> +       .hw_cursor_h = 32,
>> +       .pitch_align = 256,
>> +       .mc_bits = 40,
>> +       .has_vblank_counter = false,
>> +       .has_scan_pos = true,
>> +       .has_builtin_i2c = true,
>> +       .has_vram = true,
>> +       .has_hpd_reg = false,
>> +       .is_soc = false,
>> +};
>> +
>> +static const struct lsdc_desc dc_in_ls7a2000 = {
>> +       .chip = CHIP_LS7A2000,
>> +       .num_of_crtc = LSDC_NUM_CRTC,
>> +       .max_pixel_clk = 350000,
>> +       .max_width = 4096,
>> +       .max_height = 4096,
>> +       .num_of_hw_cursor = 2,
>> +       .hw_cursor_w = 64,
>> +       .hw_cursor_h = 64,
>> +       .pitch_align = 64,
>> +       .mc_bits = 40, /* support 48, but use 40 for backward compatibility */
>> +       .has_vblank_counter = true,
>> +       .has_scan_pos = true,
>> +       .has_builtin_i2c = true,
>> +       .has_vram = true,
>> +       .has_hpd_reg = true,
>> +       .is_soc = false,
>> +};
>> +
> Roughly a quarter of the above are identical. It might be better to
> drop them for now and re-introduce as needed with future code.
>
My initial intent here is to give a skeleton of our hardware features to 
reviewers,

Not only for gearing the control, but also for easier the reviewing process.

Without this, other part of this patch may looks questionable again.


We  could remove all of the features look up key in the struct lsdc_desc,

leave the .chip member there only.  Hard coded everything else according 
to the chip only.

But the code looks ugly by doing that way. We want weaken the

If loongson SoC is introduced, the identical will decrease.

It's OK,  I could drop as much as possible at next version.

>> +const char *chip_to_str(enum loongson_chip_family chip)
>> +{
>> +       if (chip == CHIP_LS7A2000)
>> +               return "LS7A2000";
>> +
>> +       if (chip == CHIP_LS7A1000)
>> +               return "LS7A1000";
>> +
> If it were me, I would add the name into the lsdc_desc.
>
Agree, this is acceptable. Will be changed at the next version.