lkml.org 
[lkml]   [2023]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v10 2/2] drm: add kms driver for loongson display controller
From
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.

\
 
 \ /
  Last update: 2023-04-11 10:50    [W:0.375 / U:1.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site