[PATCH 02/39] drm/imx: Add i.MX95 Display Controller DomainBlend

[Marek Vasut]

On 10/13/25 6:38 PM, Frank Li wrote:

Hello Frank,

>> +#define STATICCONTROL				0x8
>> +#define  SHDTOKSEL_MASK				GENMASK(6, 4)
>> +#define  SHDTOKSEL(x)				FIELD_PREP(SHDTOKSEL_MASK, (x))
>> +#define  SHDLDSEL_MASK				GENMASK(3, 1)
>> +#define  SHDLDSEL(x)				FIELD_PREP(SHDLDSEL_MASK, (x))
> 
> Can you keep bit fields as consistent order,  from 31..0 or 0..31.

I sent a separate fix for that, so it can go in before these patches:

[PATCH] drm/imx: dc: Sort bits and bitfields in descending order

[...]

>> +static const struct dc_subdev_info dc_db_info[] = {
>> +	{ .reg_start = 0x4b6a0000, .id = 0, },
>> +	{ .reg_start = 0x4b720000, .id = 1, },
>> +};
> 
> Not sure why need map register address to id? Does graphic link or use
> dt cells pass it as argument.

The display engine component (de) does use it to figure out which 
matching domainblend component (db) to access , since there are multiple 
instances of each.

Is there anything I should change ?

>> +static inline void dc_db_alphamaskmode_disable(struct dc_db *db)
>> +{
>> +	regmap_write_bits(db->reg_cfg, ALPHACONTROL, ALPHAMASKENABLE, 0);
>> +}
> 
> This helper function just write value to one register, not helper much
> at all.

I agree, but it also makes dc_db_init() a bit more readable, so I don't 
think inlining this is going to improve the driver much.

>> +static inline void dc_db_blendcontrol(struct dc_db *db)
>> +{
>> +	u32 val = PRIM_A_BLD_FUNC(DC_DOMAINBLEND_BLEND_ZERO) |
>> +		  SEC_A_BLD_FUNC(DC_DOMAINBLEND_BLEND_ZERO) |
>> +		  PRIM_C_BLD_FUNC(DC_DOMAINBLEND_BLEND_ZERO) |
>> +		  SEC_C_BLD_FUNC(DC_DOMAINBLEND_BLEND_ONE);
>> +
>> +	regmap_write(db->reg_cfg, BLENDCONTROL, val);
>> +}
>> +
>> +void dc_db_init(struct dc_db *db)
>> +{
>> +	dc_db_enable_shden(db);
>> +	dc_db_shdtoksel(db, SW);
>> +	dc_db_shdldsel(db, SW);
>> +	dc_db_mode(db, DB_PRIMARY);
>> +	dc_db_alphamaskmode_disable(db);
>> +	dc_db_blendcontrol(db);
>> +}
>> +
> ...
>>
>> +struct dc_db {
>> +	struct device *dev;
>> +	struct regmap *reg_cfg;
>> +	int id;
> 
> where actually use this id?
Please see dc_db_bind() .

I will be handling the rest of the feedback on this series piece by 
piece, thank you for your input.