Re: [PATCH 2/2] phy: exynos-mipi-video: allow skipping absent PHYs

[Kaustabh Chakraborty <kauschluss@disroot.org>]

On 2025-07-05 08:35, Krzysztof Kozlowski wrote:
> On 26/06/2025 22:01, Kaustabh Chakraborty wrote:
>> 
>>  struct mipi_phy_device_desc {
>> -	int num_phys;
>>  	int num_regmaps;
>>  	const char *regmap_names[EXYNOS_MIPI_REGMAPS_NUM];
>>  	struct exynos_mipi_phy_desc {
>> +		bool present;
>>  		enum exynos_mipi_phy_id	coupled_phy_id;
>>  		u32 enable_val;
>>  		unsigned int enable_reg;
>> @@ -54,10 +54,9 @@ struct mipi_phy_device_desc {
>>  static const struct mipi_phy_device_desc s5pv210_mipi_phy = {
>>  	.num_regmaps = 1,
>>  	.regmap_names = {"syscon"},
>> -	.num_phys = 4,
>>  	.phys = {
>> -		{
>> -			/* EXYNOS_MIPI_PHY_ID_CSIS0 */
>> +		[EXYNOS_MIPI_PHY_ID_CSIS0] = {
> 
> 
> This should be a separate change... but overall I don't like existing
> idea and I think your change is a reason to fix actual code style 
> issue:
> 
> It is expected that each variant will define static const array and 
> then
> you assign in:
> 
> static const struct mipi_phy_device_desc exynos5420_mipi_phy = {
> 	.phys = exynos5420_mipi_phys_data
> }
> 
> which means:
> 1. You don't waste space for unused entries (now you always allocate 5
> entries, even if you have one phy)
> 2. You can count them easily - ARRAY_SIZE
> 3. Index in the array won't the the phy ID, so you need a separate ID
> member for that
> 4. You do not need this odd 'present' field, because really code which
> is not initalized should mean 'not present' and it should be never
> needed to initialize additionally to indicate 'yes, I do exist' beyond
> basic initializations.

Weird, I don't know why had I even developed this patch. The 'issue' it
fixes isn't even an issue. Oversight, I'll drop it I guess...

> 
> Best regards,
> Krzysztof