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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ff887aaf-966a-41ff-a905-f791820136fd@kernel.org>
Date: Sat, 5 Jul 2025 10:35:08 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Kaustabh Chakraborty <kauschluss@...root.org>,
 Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Alim Akhtar <alim.akhtar@...sung.com>,
 Neil Armstrong <neil.armstrong@...aro.org>
Cc: linux-phy@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
 linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] phy: exynos-mipi-video: allow skipping absent PHYs

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.

Best regards,
Krzysztof