In Exynos7870 MIPI PHY, DSIM1 PHY is not present. The num_phys field of
mipi_phy_device_desc when set to a value, say n, always unconditionally
initializes the first n phys, so there is no mechanism to skip over
these absent PHY blocks.
Introduce a "present" flag in all PHY descriptors which needs to be set
to true. And instead of checking the first n PHYs, check all of them. If
a certain PHY is not defined, the flag remains false and is skipped
during the init phase.
The num_phys field has to go, it has no purpose as now we're checking
all elements of the exynos_mipi_phy_desc array.
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
drivers/phy/samsung/phy-exynos-mipi-video.c | 100 ++++++++++++++++------------
1 file changed, 57 insertions(+), 43 deletions(-)
diff --git a/drivers/phy/samsung/phy-exynos-mipi-video.c b/drivers/phy/samsung/phy-exynos-mipi-video.c
index 1c9c340f708da0ca214639880d067706aaea8fc7..c120c4f3531661a45374793ed7d9e9f622ff3c5d 100644
--- a/drivers/phy/samsung/phy-exynos-mipi-video.c
+++ b/drivers/phy/samsung/phy-exynos-mipi-video.c
@@ -37,10 +37,10 @@ enum exynos_mipi_phy_regmap_id {
};
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] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_DSIM0,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS4_MIPI_PHY_CONTROL(0),
@@ -65,8 +64,9 @@ static const struct mipi_phy_device_desc s5pv210_mipi_phy = {
.resetn_val = EXYNOS4_MIPI_PHY_SRESETN,
.resetn_reg = EXYNOS4_MIPI_PHY_CONTROL(0),
.resetn_map = EXYNOS_MIPI_REGMAP_PMU,
- }, {
- /* EXYNOS_MIPI_PHY_ID_DSIM0 */
+ },
+ [EXYNOS_MIPI_PHY_ID_DSIM0] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_CSIS0,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS4_MIPI_PHY_CONTROL(0),
@@ -74,8 +74,9 @@ static const struct mipi_phy_device_desc s5pv210_mipi_phy = {
.resetn_val = EXYNOS4_MIPI_PHY_MRESETN,
.resetn_reg = EXYNOS4_MIPI_PHY_CONTROL(0),
.resetn_map = EXYNOS_MIPI_REGMAP_PMU,
- }, {
- /* EXYNOS_MIPI_PHY_ID_CSIS1 */
+ },
+ [EXYNOS_MIPI_PHY_ID_CSIS1] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_DSIM1,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS4_MIPI_PHY_CONTROL(1),
@@ -83,8 +84,9 @@ static const struct mipi_phy_device_desc s5pv210_mipi_phy = {
.resetn_val = EXYNOS4_MIPI_PHY_SRESETN,
.resetn_reg = EXYNOS4_MIPI_PHY_CONTROL(1),
.resetn_map = EXYNOS_MIPI_REGMAP_PMU,
- }, {
- /* EXYNOS_MIPI_PHY_ID_DSIM1 */
+ },
+ [EXYNOS_MIPI_PHY_ID_DSIM1] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_CSIS1,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS4_MIPI_PHY_CONTROL(1),
@@ -99,10 +101,9 @@ static const struct mipi_phy_device_desc s5pv210_mipi_phy = {
static const struct mipi_phy_device_desc exynos5420_mipi_phy = {
.num_regmaps = 1,
.regmap_names = {"syscon"},
- .num_phys = 5,
.phys = {
- {
- /* EXYNOS_MIPI_PHY_ID_CSIS0 */
+ [EXYNOS_MIPI_PHY_ID_CSIS0] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_DSIM0,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS5420_MIPI_PHY_CONTROL(0),
@@ -110,8 +111,9 @@ static const struct mipi_phy_device_desc exynos5420_mipi_phy = {
.resetn_val = EXYNOS4_MIPI_PHY_SRESETN,
.resetn_reg = EXYNOS5420_MIPI_PHY_CONTROL(0),
.resetn_map = EXYNOS_MIPI_REGMAP_PMU,
- }, {
- /* EXYNOS_MIPI_PHY_ID_DSIM0 */
+ },
+ [EXYNOS_MIPI_PHY_ID_DSIM0] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_CSIS0,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS5420_MIPI_PHY_CONTROL(0),
@@ -119,8 +121,9 @@ static const struct mipi_phy_device_desc exynos5420_mipi_phy = {
.resetn_val = EXYNOS4_MIPI_PHY_MRESETN,
.resetn_reg = EXYNOS5420_MIPI_PHY_CONTROL(0),
.resetn_map = EXYNOS_MIPI_REGMAP_PMU,
- }, {
- /* EXYNOS_MIPI_PHY_ID_CSIS1 */
+ },
+ [EXYNOS_MIPI_PHY_ID_CSIS1] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_DSIM1,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS5420_MIPI_PHY_CONTROL(1),
@@ -128,8 +131,9 @@ static const struct mipi_phy_device_desc exynos5420_mipi_phy = {
.resetn_val = EXYNOS4_MIPI_PHY_SRESETN,
.resetn_reg = EXYNOS5420_MIPI_PHY_CONTROL(1),
.resetn_map = EXYNOS_MIPI_REGMAP_PMU,
- }, {
- /* EXYNOS_MIPI_PHY_ID_DSIM1 */
+ },
+ [EXYNOS_MIPI_PHY_ID_DSIM1] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_CSIS1,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS5420_MIPI_PHY_CONTROL(1),
@@ -137,8 +141,9 @@ static const struct mipi_phy_device_desc exynos5420_mipi_phy = {
.resetn_val = EXYNOS4_MIPI_PHY_MRESETN,
.resetn_reg = EXYNOS5420_MIPI_PHY_CONTROL(1),
.resetn_map = EXYNOS_MIPI_REGMAP_PMU,
- }, {
- /* EXYNOS_MIPI_PHY_ID_CSIS2 */
+ },
+ [EXYNOS_MIPI_PHY_ID_CSIS2] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_NONE,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS5420_MIPI_PHY_CONTROL(2),
@@ -162,10 +167,9 @@ static const struct mipi_phy_device_desc exynos5433_mipi_phy = {
"samsung,cam0-sysreg",
"samsung,cam1-sysreg"
},
- .num_phys = 5,
.phys = {
- {
- /* EXYNOS_MIPI_PHY_ID_CSIS0 */
+ [EXYNOS_MIPI_PHY_ID_CSIS0] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_DSIM0,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS4_MIPI_PHY_CONTROL(0),
@@ -173,8 +177,9 @@ static const struct mipi_phy_device_desc exynos5433_mipi_phy = {
.resetn_val = BIT(0),
.resetn_reg = EXYNOS5433_SYSREG_CAM0_MIPI_DPHY_CON,
.resetn_map = EXYNOS_MIPI_REGMAP_CAM0,
- }, {
- /* EXYNOS_MIPI_PHY_ID_DSIM0 */
+ },
+ [EXYNOS_MIPI_PHY_ID_DSIM0] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_CSIS0,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS4_MIPI_PHY_CONTROL(0),
@@ -182,8 +187,9 @@ static const struct mipi_phy_device_desc exynos5433_mipi_phy = {
.resetn_val = BIT(0),
.resetn_reg = EXYNOS5433_SYSREG_DISP_MIPI_PHY,
.resetn_map = EXYNOS_MIPI_REGMAP_DISP,
- }, {
- /* EXYNOS_MIPI_PHY_ID_CSIS1 */
+ },
+ [EXYNOS_MIPI_PHY_ID_CSIS1] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_NONE,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS4_MIPI_PHY_CONTROL(1),
@@ -191,8 +197,9 @@ static const struct mipi_phy_device_desc exynos5433_mipi_phy = {
.resetn_val = BIT(1),
.resetn_reg = EXYNOS5433_SYSREG_CAM0_MIPI_DPHY_CON,
.resetn_map = EXYNOS_MIPI_REGMAP_CAM0,
- }, {
- /* EXYNOS_MIPI_PHY_ID_DSIM1 */
+ },
+ [EXYNOS_MIPI_PHY_ID_DSIM1] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_NONE,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS4_MIPI_PHY_CONTROL(1),
@@ -200,8 +207,9 @@ static const struct mipi_phy_device_desc exynos5433_mipi_phy = {
.resetn_val = BIT(1),
.resetn_reg = EXYNOS5433_SYSREG_DISP_MIPI_PHY,
.resetn_map = EXYNOS_MIPI_REGMAP_DISP,
- }, {
- /* EXYNOS_MIPI_PHY_ID_CSIS2 */
+ },
+ [EXYNOS_MIPI_PHY_ID_CSIS2] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_NONE,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS4_MIPI_PHY_CONTROL(2),
@@ -220,10 +228,9 @@ static const struct mipi_phy_device_desc exynos7870_mipi_phy = {
"samsung,disp-sysreg",
"samsung,cam0-sysreg",
},
- .num_phys = 4,
.phys = {
- {
- /* EXYNOS_MIPI_PHY_ID_CSIS0 */
+ [EXYNOS_MIPI_PHY_ID_CSIS0] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_DSIM0,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS7870_MIPI_PHY_CONTROL0,
@@ -231,8 +238,9 @@ static const struct mipi_phy_device_desc exynos7870_mipi_phy = {
.resetn_val = BIT(0),
.resetn_reg = 0,
.resetn_map = EXYNOS_MIPI_REGMAP_CAM0,
- }, {
- /* EXYNOS_MIPI_PHY_ID_DSIM0 */
+ },
+ [EXYNOS_MIPI_PHY_ID_DSIM0] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_CSIS0,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS7870_MIPI_PHY_CONTROL0,
@@ -240,8 +248,9 @@ static const struct mipi_phy_device_desc exynos7870_mipi_phy = {
.resetn_val = BIT(0),
.resetn_reg = 0,
.resetn_map = EXYNOS_MIPI_REGMAP_DISP,
- }, {
- /* EXYNOS_MIPI_PHY_ID_CSIS1 */
+ },
+ [EXYNOS_MIPI_PHY_ID_CSIS1] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_NONE,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS7870_MIPI_PHY_CONTROL1,
@@ -249,8 +258,9 @@ static const struct mipi_phy_device_desc exynos7870_mipi_phy = {
.resetn_val = BIT(1),
.resetn_reg = 0,
.resetn_map = EXYNOS_MIPI_REGMAP_CAM0,
- }, {
- /* EXYNOS_MIPI_PHY_ID_CSIS2 */
+ },
+ [EXYNOS_MIPI_PHY_ID_CSIS2] = {
+ .present = true,
.coupled_phy_id = EXYNOS_MIPI_PHY_ID_NONE,
.enable_val = EXYNOS4_PHY_ENABLE,
.enable_reg = EXYNOS7870_MIPI_PHY_CONTROL2,
@@ -365,12 +375,15 @@ static int exynos_mipi_video_phy_probe(struct platform_device *pdev)
if (IS_ERR(state->regmaps[i]))
return PTR_ERR(state->regmaps[i]);
}
- state->num_phys = phy_dev->num_phys;
+ state->num_phys = 0;
spin_lock_init(&state->slock);
dev_set_drvdata(dev, state);
- for (i = 0; i < state->num_phys; i++) {
+ for (i = 0; i < EXYNOS_MIPI_PHYS_NUM; i++) {
+ if (!phy_dev->phys[i].present)
+ continue;
+
struct phy *phy = devm_phy_create(dev, NULL,
&exynos_mipi_video_phy_ops);
if (IS_ERR(phy)) {
@@ -378,6 +391,7 @@ static int exynos_mipi_video_phy_probe(struct platform_device *pdev)
return PTR_ERR(phy);
}
+ state->num_phys++;
state->phys[i].phy = phy;
state->phys[i].index = i;
state->phys[i].data = &phy_dev->phys[i];
--
2.49.0On 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
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
© 2016 - 2025 Red Hat, Inc.