[v3,8/8] media: qcom: camss: Decouple VFE from CSID

Message ID 20240411124543.199-9-quic_grosikop@quicinc.com (mailing list archive)
State Superseded
Delegated to: Hans Verkuil
Headers
Series Move camss version related defs in to resources |

Commit Message

Gjorgji Rosikopulos April 11, 2024, 12:45 p.m. UTC
  From: Milen Mitkov <quic_mmitkov@quicinc.com>

Decouple the direct calls to VFE's vfe_get/put in the CSID subdev
in order to prepare for the introduction of IFE subdev.

Also decouple CSID base address from VFE since on the Titan platform
CSID register base address resides within VFE's base address.

Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
Signed-off-by: Gjorgji Rosikopulos <quic_grosikop@quicinc.com>
---
 .../media/platform/qcom/camss/camss-csid.c    | 16 +++--
 .../media/platform/qcom/camss/camss-csid.h    |  1 +
 drivers/media/platform/qcom/camss/camss.c     | 69 +++++++++++++++++++
 drivers/media/platform/qcom/camss/camss.h     |  8 +++
 4 files changed, 89 insertions(+), 5 deletions(-)
  

Comments

Bryan O'Donoghue May 10, 2024, 6:29 p.m. UTC | #1
On 11/04/2024 13:45, Gjorgji Rosikopulos wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> 
> Decouple the direct calls to VFE's vfe_get/put in the CSID subdev
> in order to prepare for the introduction of IFE subdev.
> 
> Also decouple CSID base address from VFE since on the Titan platform
> CSID register base address resides within VFE's base address.
> 
> Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
> Signed-off-by: Gjorgji Rosikopulos <quic_grosikop@quicinc.com>
> ---

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # 
sc8280xp/sm8250/sdm845/apq8016
  
Vladimir Zapolskiy May 13, 2024, 3:58 p.m. UTC | #2
On 4/11/24 15:45, Gjorgji Rosikopulos wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> 
> Decouple the direct calls to VFE's vfe_get/put in the CSID subdev
> in order to prepare for the introduction of IFE subdev.
> 
> Also decouple CSID base address from VFE since on the Titan platform
> CSID register base address resides within VFE's base address.
> 
> Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
> Signed-off-by: Gjorgji Rosikopulos <quic_grosikop@quicinc.com>
> ---
>   .../media/platform/qcom/camss/camss-csid.c    | 16 +++--
>   .../media/platform/qcom/camss/camss-csid.h    |  1 +
>   drivers/media/platform/qcom/camss/camss.c     | 69 +++++++++++++++++++
>   drivers/media/platform/qcom/camss/camss.h     |  8 +++
>   4 files changed, 89 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 5b23f5b8746d..858db5d4ca75 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -602,7 +602,6 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>   	struct csid_device *csid = v4l2_get_subdevdata(sd);
>   	struct camss *camss = csid->camss;
>   	struct device *dev = camss->dev;
> -	struct vfe_device *vfe = &camss->vfe[csid->id];
>   	int ret = 0;
>   
>   	if (on) {
> @@ -611,7 +610,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>   		 * switching on the CSID. Do so unconditionally, as there is no
>   		 * drawback in following the same powering order on older SoCs.
>   		 */
> -		ret = vfe_get(vfe);
> +		ret = csid->res->parent_dev_ops->get(camss, csid->id);
>   		if (ret < 0)
>   			return ret;
>   
> @@ -663,7 +662,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>   		regulator_bulk_disable(csid->num_supplies,
>   				       csid->supplies);
>   		pm_runtime_put_sync(dev);
> -		vfe_put(vfe);
> +		csid->res->parent_dev_ops->put(camss, csid->id);
>   	}
>   
>   	return ret;
> @@ -1021,6 +1020,11 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>   	csid->id = id;
>   	csid->res = &res->csid;
>   
> +	if (dev_WARN_ONCE(dev, !csid->res->parent_dev_ops,

Please remove/replace dev_WARN_ONCE() to a lesser dev_warn_once(), wherever it's
possible please do not use/introduce WARN() type of writes to the kernel log buffer...

> +			  "Error: CSID depends on VFE/IFE device ops!\n")) {
> +		return -EINVAL;
> +	}
> +
>   	csid->res->hw_ops->subdev_init(csid);
>   
>   	/* Memory */
> @@ -1031,9 +1035,11 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>   		 * VFE to be initialized before CSID
>   		 */
>   		if (id >= 2) /* VFE/CSID lite */
> -			csid->base = camss->vfe[id].base + VFE_480_LITE_CSID_OFFSET;
> +			csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
> +				+ VFE_480_LITE_CSID_OFFSET;
>   		else
> -			csid->base = camss->vfe[id].base + VFE_480_CSID_OFFSET;
> +			csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
> +				 + VFE_480_CSID_OFFSET;
>   	} else {
>   		csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
>   		if (IS_ERR(csid->base))
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index 0e385d17c250..8cdae98e4dca 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -157,6 +157,7 @@ struct csid_hw_ops {
>   struct csid_subdev_resources {
>   	bool is_lite;
>   	const struct csid_hw_ops *hw_ops;
> +	const struct parent_dev_ops *parent_dev_ops;
>   	const struct csid_formats *formats;
>   };
>   
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 37060eaa0ba5..4d625ef59cf7 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -32,6 +32,8 @@
>   #define CAMSS_CLOCK_MARGIN_NUMERATOR 105
>   #define CAMSS_CLOCK_MARGIN_DENOMINATOR 100
>   
> +static const struct parent_dev_ops vfe_parent_dev_ops;
> +
>   static const struct camss_subdev_resources csiphy_res_8x16[] = {
>   	/* CSIPHY0 */
>   	{
> @@ -87,6 +89,7 @@ static const struct camss_subdev_resources csid_res_8x16[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_1,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_1
>   		}
>   	},
> @@ -109,6 +112,7 @@ static const struct camss_subdev_resources csid_res_8x16[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_1,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_1
>   		}
>   	},
> @@ -226,6 +230,7 @@ static const struct camss_subdev_resources csid_res_8x96[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_7,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_7
>   		}
>   	},
> @@ -248,6 +253,7 @@ static const struct camss_subdev_resources csid_res_8x96[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_7,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_7
>   		}
>   	},
> @@ -270,6 +276,7 @@ static const struct camss_subdev_resources csid_res_8x96[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_7,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_7
>   		}
>   	},
> @@ -292,6 +299,7 @@ static const struct camss_subdev_resources csid_res_8x96[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_7,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_7
>   		}
>   	}
> @@ -445,6 +453,7 @@ static const struct camss_subdev_resources csid_res_660[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_7,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_7
>   		}
>   	},
> @@ -470,6 +479,7 @@ static const struct camss_subdev_resources csid_res_660[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_7,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_7
>   		}
>   	},
> @@ -495,6 +505,7 @@ static const struct camss_subdev_resources csid_res_660[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_7,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_7
>   		}
>   	},
> @@ -520,6 +531,7 @@ static const struct camss_subdev_resources csid_res_660[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_4_7,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_4_7
>   		}
>   	}
> @@ -714,6 +726,7 @@ static const struct camss_subdev_resources csid_res_845[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -739,6 +752,7 @@ static const struct camss_subdev_resources csid_res_845[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -765,6 +779,7 @@ static const struct camss_subdev_resources csid_res_845[] = {
>   		.csid = {
>   			.is_lite = true,
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	}
> @@ -957,6 +972,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -974,6 +990,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.type = CAMSS_SUBDEV_TYPE_CSID,
>   		.csid = {
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -991,6 +1008,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.csid = {
>   			.is_lite = true,
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -1008,6 +1026,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.csid = {
>   			.is_lite = true,
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	}
> @@ -1212,6 +1231,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
>   		.interrupt = { "csid0" },
>   		.csid = {
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -1227,6 +1247,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
>   		.interrupt = { "csid1" },
>   		.csid = {
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -1242,6 +1263,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
>   		.interrupt = { "csid2" },
>   		.csid = {
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -1257,6 +1279,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
>   		.interrupt = { "csid3" },
>   		.csid = {
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -1272,6 +1295,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
>   		.csid = {
>   			.is_lite = true,
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -1287,6 +1311,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
>   		.csid = {
>   			.is_lite = true,
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -1302,6 +1327,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
>   		.csid = {
>   			.is_lite = true,
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	},
> @@ -1317,6 +1343,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
>   		.csid = {
>   			.is_lite = true,
>   			.hw_ops = &csid_ops_gen2,
> +			.parent_dev_ops = &vfe_parent_dev_ops,
>   			.formats = &csid_formats_gen2
>   		}
>   	}
> @@ -1661,6 +1688,48 @@ void camss_pm_domain_off(struct camss *camss, int id)
>   	}
>   }
>   
> +static int vfe_parent_dev_ops_get(struct camss *camss, int id)
> +{
> +	int ret = -EINVAL;
> +
> +	if (id < camss->res->vfe_num) {


if (id >= camss->res->vfe_num)
     return -EINVAL;

> +		struct vfe_device *vfe = &camss->vfe[id];
> +
> +		ret = vfe_get(vfe);
> +	}
> +
> +	return ret;
> +}
> +
> +static int vfe_parent_dev_ops_put(struct camss *camss, int id)
> +{
> +	if (id < camss->res->vfe_num) {
> +		struct vfe_device *vfe = &camss->vfe[id];
> +
> +		vfe_put(vfe);
> +	}
> +
> +	return 0;
> +}
> +
> +static void __iomem
> +*vfe_parent_dev_ops_get_base_address(struct camss *camss, int id)
> +{
> +	if (id < camss->res->vfe_num) {
> +		struct vfe_device *vfe = &camss->vfe[id];
> +
> +		return vfe->base;
> +	}
> +
> +	return NULL;

I can find code snippets above like

	if (IS_ERR(csid->base))
		...

So, is it really a good idea to return NULL on error? Probably it might be better
to return a reasonable error to the caller.

> +}
> +
> +static const struct parent_dev_ops vfe_parent_dev_ops = {
> +	.get = vfe_parent_dev_ops_get,
> +	.put = vfe_parent_dev_ops_put,
> +	.get_base_address = vfe_parent_dev_ops_get_base_address
> +};
> +
>   /*
>    * camss_of_parse_endpoint_node - Parse port endpoint node
>    * @dev: Device
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index a5be9e872992..b3c967bcf8a9 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -143,6 +143,12 @@ struct camss_clock {
>   	u32 nfreqs;
>   };
>   
> +struct parent_dev_ops {
> +	int (*get)(struct camss *camss, int id);
> +	int (*put)(struct camss *camss, int id);
> +	void __iomem *(*get_base_address)(struct camss *camss, int id);
> +};
> +
>   void camss_add_clock_margin(u64 *rate);
>   int camss_enable_clocks(int nclocks, struct camss_clock *clock,
>   			struct device *dev);
> @@ -153,6 +159,8 @@ s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
>   int camss_get_pixel_clock(struct media_entity *entity, u64 *pixel_clock);
>   int camss_pm_domain_on(struct camss *camss, int id);
>   void camss_pm_domain_off(struct camss *camss, int id);
> +int camss_vfe_get(struct camss *camss, int id);
> +void camss_vfe_put(struct camss *camss, int id);
>   void camss_delete(struct camss *camss);
>   
>   #endif /* QC_MSM_CAMSS_H */

--
Best wishes,
Vladimir
  
Gjorgji Rosikopulos May 13, 2024, 4:26 p.m. UTC | #3
Hi Vladimir,

Thanks for the review,

On 5/13/2024 6:58 PM, Vladimir Zapolskiy wrote:
> On 4/11/24 15:45, Gjorgji Rosikopulos wrote:
>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>
>> Decouple the direct calls to VFE's vfe_get/put in the CSID subdev
>> in order to prepare for the introduction of IFE subdev.
>>
>> Also decouple CSID base address from VFE since on the Titan platform
>> CSID register base address resides within VFE's base address.
>>
>> Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
>> Signed-off-by: Gjorgji Rosikopulos <quic_grosikop@quicinc.com>
>> ---
>>   .../media/platform/qcom/camss/camss-csid.c    | 16 +++--
>>   .../media/platform/qcom/camss/camss-csid.h    |  1 +
>>   drivers/media/platform/qcom/camss/camss.c     | 69 +++++++++++++++++++
>>   drivers/media/platform/qcom/camss/camss.h     |  8 +++
>>   4 files changed, 89 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c
>> b/drivers/media/platform/qcom/camss/camss-csid.c
>> index 5b23f5b8746d..858db5d4ca75 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -602,7 +602,6 @@ static int csid_set_power(struct v4l2_subdev *sd,
>> int on)
>>       struct csid_device *csid = v4l2_get_subdevdata(sd);
>>       struct camss *camss = csid->camss;
>>       struct device *dev = camss->dev;
>> -    struct vfe_device *vfe = &camss->vfe[csid->id];
>>       int ret = 0;
>>         if (on) {
>> @@ -611,7 +610,7 @@ static int csid_set_power(struct v4l2_subdev *sd,
>> int on)
>>            * switching on the CSID. Do so unconditionally, as there is no
>>            * drawback in following the same powering order on older SoCs.
>>            */
>> -        ret = vfe_get(vfe);
>> +        ret = csid->res->parent_dev_ops->get(camss, csid->id);
>>           if (ret < 0)
>>               return ret;
>>   @@ -663,7 +662,7 @@ static int csid_set_power(struct v4l2_subdev
>> *sd, int on)
>>           regulator_bulk_disable(csid->num_supplies,
>>                          csid->supplies);
>>           pm_runtime_put_sync(dev);
>> -        vfe_put(vfe);
>> +        csid->res->parent_dev_ops->put(camss, csid->id);
>>       }
>>         return ret;
>> @@ -1021,6 +1020,11 @@ int msm_csid_subdev_init(struct camss *camss,
>> struct csid_device *csid,
>>       csid->id = id;
>>       csid->res = &res->csid;
>>   +    if (dev_WARN_ONCE(dev, !csid->res->parent_dev_ops,
> 
> Please remove/replace dev_WARN_ONCE() to a lesser dev_warn_once(),
> wherever it's
> possible please do not use/introduce WARN() type of writes to the kernel
> log buffer...

The error is fatal and driver probe will fail if this happens,
it is good to have it in kernel log buffer.
However i agree it can be changed to dev_warn_once.

> 
>> +              "Error: CSID depends on VFE/IFE device ops!\n")) {
>> +        return -EINVAL;
>> +    }
>> +
>>       csid->res->hw_ops->subdev_init(csid);
>>         /* Memory */
>> @@ -1031,9 +1035,11 @@ int msm_csid_subdev_init(struct camss *camss,
>> struct csid_device *csid,
>>            * VFE to be initialized before CSID
>>            */
>>           if (id >= 2) /* VFE/CSID lite */
>> -            csid->base = camss->vfe[id].base + VFE_480_LITE_CSID_OFFSET;
>> +            csid->base =
>> csid->res->parent_dev_ops->get_base_address(camss, id)
>> +                + VFE_480_LITE_CSID_OFFSET;
>>           else
>> -            csid->base = camss->vfe[id].base + VFE_480_CSID_OFFSET;
>> +            csid->base =
>> csid->res->parent_dev_ops->get_base_address(camss, id)
>> +                 + VFE_480_CSID_OFFSET;
>>       } else {
>>           csid->base = devm_platform_ioremap_resource_byname(pdev,
>> res->reg[0]);
>>           if (IS_ERR(csid->base))
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h
>> b/drivers/media/platform/qcom/camss/camss-csid.h
>> index 0e385d17c250..8cdae98e4dca 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.h
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
>> @@ -157,6 +157,7 @@ struct csid_hw_ops {
>>   struct csid_subdev_resources {
>>       bool is_lite;
>>       const struct csid_hw_ops *hw_ops;
>> +    const struct parent_dev_ops *parent_dev_ops;
>>       const struct csid_formats *formats;
>>   };
>>   diff --git a/drivers/media/platform/qcom/camss/camss.c
>> b/drivers/media/platform/qcom/camss/camss.c
>> index 37060eaa0ba5..4d625ef59cf7 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -32,6 +32,8 @@
>>   #define CAMSS_CLOCK_MARGIN_NUMERATOR 105
>>   #define CAMSS_CLOCK_MARGIN_DENOMINATOR 100
>>   +static const struct parent_dev_ops vfe_parent_dev_ops;
>> +
>>   static const struct camss_subdev_resources csiphy_res_8x16[] = {
>>       /* CSIPHY0 */
>>       {
>> @@ -87,6 +89,7 @@ static const struct camss_subdev_resources
>> csid_res_8x16[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_1,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_1
>>           }
>>       },
>> @@ -109,6 +112,7 @@ static const struct camss_subdev_resources
>> csid_res_8x16[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_1,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_1
>>           }
>>       },
>> @@ -226,6 +230,7 @@ static const struct camss_subdev_resources
>> csid_res_8x96[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_7,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_7
>>           }
>>       },
>> @@ -248,6 +253,7 @@ static const struct camss_subdev_resources
>> csid_res_8x96[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_7,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_7
>>           }
>>       },
>> @@ -270,6 +276,7 @@ static const struct camss_subdev_resources
>> csid_res_8x96[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_7,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_7
>>           }
>>       },
>> @@ -292,6 +299,7 @@ static const struct camss_subdev_resources
>> csid_res_8x96[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_7,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_7
>>           }
>>       }
>> @@ -445,6 +453,7 @@ static const struct camss_subdev_resources
>> csid_res_660[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_7,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_7
>>           }
>>       },
>> @@ -470,6 +479,7 @@ static const struct camss_subdev_resources
>> csid_res_660[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_7,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_7
>>           }
>>       },
>> @@ -495,6 +505,7 @@ static const struct camss_subdev_resources
>> csid_res_660[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_7,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_7
>>           }
>>       },
>> @@ -520,6 +531,7 @@ static const struct camss_subdev_resources
>> csid_res_660[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_4_7,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_4_7
>>           }
>>       }
>> @@ -714,6 +726,7 @@ static const struct camss_subdev_resources
>> csid_res_845[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -739,6 +752,7 @@ static const struct camss_subdev_resources
>> csid_res_845[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -765,6 +779,7 @@ static const struct camss_subdev_resources
>> csid_res_845[] = {
>>           .csid = {
>>               .is_lite = true,
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       }
>> @@ -957,6 +972,7 @@ static const struct camss_subdev_resources
>> csid_res_8250[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -974,6 +990,7 @@ static const struct camss_subdev_resources
>> csid_res_8250[] = {
>>           .type = CAMSS_SUBDEV_TYPE_CSID,
>>           .csid = {
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -991,6 +1008,7 @@ static const struct camss_subdev_resources
>> csid_res_8250[] = {
>>           .csid = {
>>               .is_lite = true,
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -1008,6 +1026,7 @@ static const struct camss_subdev_resources
>> csid_res_8250[] = {
>>           .csid = {
>>               .is_lite = true,
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       }
>> @@ -1212,6 +1231,7 @@ static const struct camss_subdev_resources
>> csid_res_sc8280xp[] = {
>>           .interrupt = { "csid0" },
>>           .csid = {
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -1227,6 +1247,7 @@ static const struct camss_subdev_resources
>> csid_res_sc8280xp[] = {
>>           .interrupt = { "csid1" },
>>           .csid = {
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -1242,6 +1263,7 @@ static const struct camss_subdev_resources
>> csid_res_sc8280xp[] = {
>>           .interrupt = { "csid2" },
>>           .csid = {
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -1257,6 +1279,7 @@ static const struct camss_subdev_resources
>> csid_res_sc8280xp[] = {
>>           .interrupt = { "csid3" },
>>           .csid = {
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -1272,6 +1295,7 @@ static const struct camss_subdev_resources
>> csid_res_sc8280xp[] = {
>>           .csid = {
>>               .is_lite = true,
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -1287,6 +1311,7 @@ static const struct camss_subdev_resources
>> csid_res_sc8280xp[] = {
>>           .csid = {
>>               .is_lite = true,
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -1302,6 +1327,7 @@ static const struct camss_subdev_resources
>> csid_res_sc8280xp[] = {
>>           .csid = {
>>               .is_lite = true,
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       },
>> @@ -1317,6 +1343,7 @@ static const struct camss_subdev_resources
>> csid_res_sc8280xp[] = {
>>           .csid = {
>>               .is_lite = true,
>>               .hw_ops = &csid_ops_gen2,
>> +            .parent_dev_ops = &vfe_parent_dev_ops,
>>               .formats = &csid_formats_gen2
>>           }
>>       }
>> @@ -1661,6 +1688,48 @@ void camss_pm_domain_off(struct camss *camss,
>> int id)
>>       }
>>   }
>>   +static int vfe_parent_dev_ops_get(struct camss *camss, int id)
>> +{
>> +    int ret = -EINVAL;
>> +
>> +    if (id < camss->res->vfe_num) {
> 
> 
> if (id >= camss->res->vfe_num)
>     return -EINVAL;

:-). I believe this is metter of personal taste. I also like
the code which you have posted. But with function of this size
i dont see that it will make any difference.

> 
>> +        struct vfe_device *vfe = &camss->vfe[id];
>> +
>> +        ret = vfe_get(vfe);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int vfe_parent_dev_ops_put(struct camss *camss, int id)
>> +{
>> +    if (id < camss->res->vfe_num) {
>> +        struct vfe_device *vfe = &camss->vfe[id];
>> +
>> +        vfe_put(vfe);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void __iomem
>> +*vfe_parent_dev_ops_get_base_address(struct camss *camss, int id)
>> +{
>> +    if (id < camss->res->vfe_num) {
>> +        struct vfe_device *vfe = &camss->vfe[id];
>> +
>> +        return vfe->base;
>> +    }
>> +
>> +    return NULL;
> 
> I can find code snippets above like
> 
>     if (IS_ERR(csid->base))
>         ...
> 
> So, is it really a good idea to return NULL on error? Probably it might
> be better
> to return a reasonable error to the caller.

As general rule i agree. But here either we have address or not,
i dont see the reason to return an error code. Also i dont see what
caller will do if he gets error code instead of NULL.
I am refering in particular this case. If we have different error paths
of failiure maybe it will more sense.


> 
>> +}
>> +
>> +static const struct parent_dev_ops vfe_parent_dev_ops = {
>> +    .get = vfe_parent_dev_ops_get,
>> +    .put = vfe_parent_dev_ops_put,
>> +    .get_base_address = vfe_parent_dev_ops_get_base_address
>> +};
>> +
>>   /*
>>    * camss_of_parse_endpoint_node - Parse port endpoint node
>>    * @dev: Device
>> diff --git a/drivers/media/platform/qcom/camss/camss.h
>> b/drivers/media/platform/qcom/camss/camss.h
>> index a5be9e872992..b3c967bcf8a9 100644
>> --- a/drivers/media/platform/qcom/camss/camss.h
>> +++ b/drivers/media/platform/qcom/camss/camss.h
>> @@ -143,6 +143,12 @@ struct camss_clock {
>>       u32 nfreqs;
>>   };
>>   +struct parent_dev_ops {
>> +    int (*get)(struct camss *camss, int id);
>> +    int (*put)(struct camss *camss, int id);
>> +    void __iomem *(*get_base_address)(struct camss *camss, int id);
>> +};
>> +
>>   void camss_add_clock_margin(u64 *rate);
>>   int camss_enable_clocks(int nclocks, struct camss_clock *clock,
>>               struct device *dev);
>> @@ -153,6 +159,8 @@ s64 camss_get_link_freq(struct media_entity
>> *entity, unsigned int bpp,
>>   int camss_get_pixel_clock(struct media_entity *entity, u64
>> *pixel_clock);
>>   int camss_pm_domain_on(struct camss *camss, int id);
>>   void camss_pm_domain_off(struct camss *camss, int id);
>> +int camss_vfe_get(struct camss *camss, int id);
>> +void camss_vfe_put(struct camss *camss, int id);
>>   void camss_delete(struct camss *camss);
>>     #endif /* QC_MSM_CAMSS_H */
> 
> -- 
> Best wishes,
> Vladimir
  
Bryan O'Donoghue May 13, 2024, 5:48 p.m. UTC | #4
On 13/05/2024 17:26, Gjorgji Rosikopulos (Consultant) wrote:
>>> +static void __iomem
>>> +*vfe_parent_dev_ops_get_base_address(struct camss *camss, int id)
>>> +{
>>> +    if (id < camss->res->vfe_num) {
>>> +        struct vfe_device *vfe = &camss->vfe[id];
>>> +
>>> +        return vfe->base;
>>> +    }
>>> +
>>> +    return NULL;
>> I can find code snippets above like
>>
>>      if (IS_ERR(csid->base))
>>          ...
>>
>> So, is it really a good idea to return NULL on error? Probably it might
>> be better
>> to return a reasonable error to the caller.
> As general rule i agree. But here either we have address or not,
> i dont see the reason to return an error code. Also i dont see what
> caller will do if he gets error code instead of NULL.
> I am refering in particular this case. If we have different error paths
> of failiure maybe it will more sense.

I don't see a compelling reason to change the submitted code. I'd leave 
well-enough alone for v4.

Please keep changes for V4 restricted to formatting/line indentation/SPDX.

I don't want to have to reverify all of this code unless a bug is found.

---
bod
  

Patch

diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index 5b23f5b8746d..858db5d4ca75 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -602,7 +602,6 @@  static int csid_set_power(struct v4l2_subdev *sd, int on)
 	struct csid_device *csid = v4l2_get_subdevdata(sd);
 	struct camss *camss = csid->camss;
 	struct device *dev = camss->dev;
-	struct vfe_device *vfe = &camss->vfe[csid->id];
 	int ret = 0;
 
 	if (on) {
@@ -611,7 +610,7 @@  static int csid_set_power(struct v4l2_subdev *sd, int on)
 		 * switching on the CSID. Do so unconditionally, as there is no
 		 * drawback in following the same powering order on older SoCs.
 		 */
-		ret = vfe_get(vfe);
+		ret = csid->res->parent_dev_ops->get(camss, csid->id);
 		if (ret < 0)
 			return ret;
 
@@ -663,7 +662,7 @@  static int csid_set_power(struct v4l2_subdev *sd, int on)
 		regulator_bulk_disable(csid->num_supplies,
 				       csid->supplies);
 		pm_runtime_put_sync(dev);
-		vfe_put(vfe);
+		csid->res->parent_dev_ops->put(camss, csid->id);
 	}
 
 	return ret;
@@ -1021,6 +1020,11 @@  int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
 	csid->id = id;
 	csid->res = &res->csid;
 
+	if (dev_WARN_ONCE(dev, !csid->res->parent_dev_ops,
+			  "Error: CSID depends on VFE/IFE device ops!\n")) {
+		return -EINVAL;
+	}
+
 	csid->res->hw_ops->subdev_init(csid);
 
 	/* Memory */
@@ -1031,9 +1035,11 @@  int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
 		 * VFE to be initialized before CSID
 		 */
 		if (id >= 2) /* VFE/CSID lite */
-			csid->base = camss->vfe[id].base + VFE_480_LITE_CSID_OFFSET;
+			csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
+				+ VFE_480_LITE_CSID_OFFSET;
 		else
-			csid->base = camss->vfe[id].base + VFE_480_CSID_OFFSET;
+			csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
+				 + VFE_480_CSID_OFFSET;
 	} else {
 		csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
 		if (IS_ERR(csid->base))
diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
index 0e385d17c250..8cdae98e4dca 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.h
+++ b/drivers/media/platform/qcom/camss/camss-csid.h
@@ -157,6 +157,7 @@  struct csid_hw_ops {
 struct csid_subdev_resources {
 	bool is_lite;
 	const struct csid_hw_ops *hw_ops;
+	const struct parent_dev_ops *parent_dev_ops;
 	const struct csid_formats *formats;
 };
 
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 37060eaa0ba5..4d625ef59cf7 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -32,6 +32,8 @@ 
 #define CAMSS_CLOCK_MARGIN_NUMERATOR 105
 #define CAMSS_CLOCK_MARGIN_DENOMINATOR 100
 
+static const struct parent_dev_ops vfe_parent_dev_ops;
+
 static const struct camss_subdev_resources csiphy_res_8x16[] = {
 	/* CSIPHY0 */
 	{
@@ -87,6 +89,7 @@  static const struct camss_subdev_resources csid_res_8x16[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_1,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_1
 		}
 	},
@@ -109,6 +112,7 @@  static const struct camss_subdev_resources csid_res_8x16[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_1,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_1
 		}
 	},
@@ -226,6 +230,7 @@  static const struct camss_subdev_resources csid_res_8x96[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_7,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_7
 		}
 	},
@@ -248,6 +253,7 @@  static const struct camss_subdev_resources csid_res_8x96[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_7,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_7
 		}
 	},
@@ -270,6 +276,7 @@  static const struct camss_subdev_resources csid_res_8x96[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_7,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_7
 		}
 	},
@@ -292,6 +299,7 @@  static const struct camss_subdev_resources csid_res_8x96[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_7,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_7
 		}
 	}
@@ -445,6 +453,7 @@  static const struct camss_subdev_resources csid_res_660[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_7,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_7
 		}
 	},
@@ -470,6 +479,7 @@  static const struct camss_subdev_resources csid_res_660[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_7,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_7
 		}
 	},
@@ -495,6 +505,7 @@  static const struct camss_subdev_resources csid_res_660[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_7,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_7
 		}
 	},
@@ -520,6 +531,7 @@  static const struct camss_subdev_resources csid_res_660[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_4_7,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_4_7
 		}
 	}
@@ -714,6 +726,7 @@  static const struct camss_subdev_resources csid_res_845[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -739,6 +752,7 @@  static const struct camss_subdev_resources csid_res_845[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -765,6 +779,7 @@  static const struct camss_subdev_resources csid_res_845[] = {
 		.csid = {
 			.is_lite = true,
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	}
@@ -957,6 +972,7 @@  static const struct camss_subdev_resources csid_res_8250[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -974,6 +990,7 @@  static const struct camss_subdev_resources csid_res_8250[] = {
 		.type = CAMSS_SUBDEV_TYPE_CSID,
 		.csid = {
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -991,6 +1008,7 @@  static const struct camss_subdev_resources csid_res_8250[] = {
 		.csid = {
 			.is_lite = true,
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -1008,6 +1026,7 @@  static const struct camss_subdev_resources csid_res_8250[] = {
 		.csid = {
 			.is_lite = true,
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	}
@@ -1212,6 +1231,7 @@  static const struct camss_subdev_resources csid_res_sc8280xp[] = {
 		.interrupt = { "csid0" },
 		.csid = {
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -1227,6 +1247,7 @@  static const struct camss_subdev_resources csid_res_sc8280xp[] = {
 		.interrupt = { "csid1" },
 		.csid = {
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -1242,6 +1263,7 @@  static const struct camss_subdev_resources csid_res_sc8280xp[] = {
 		.interrupt = { "csid2" },
 		.csid = {
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -1257,6 +1279,7 @@  static const struct camss_subdev_resources csid_res_sc8280xp[] = {
 		.interrupt = { "csid3" },
 		.csid = {
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -1272,6 +1295,7 @@  static const struct camss_subdev_resources csid_res_sc8280xp[] = {
 		.csid = {
 			.is_lite = true,
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -1287,6 +1311,7 @@  static const struct camss_subdev_resources csid_res_sc8280xp[] = {
 		.csid = {
 			.is_lite = true,
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -1302,6 +1327,7 @@  static const struct camss_subdev_resources csid_res_sc8280xp[] = {
 		.csid = {
 			.is_lite = true,
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	},
@@ -1317,6 +1343,7 @@  static const struct camss_subdev_resources csid_res_sc8280xp[] = {
 		.csid = {
 			.is_lite = true,
 			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
 			.formats = &csid_formats_gen2
 		}
 	}
@@ -1661,6 +1688,48 @@  void camss_pm_domain_off(struct camss *camss, int id)
 	}
 }
 
+static int vfe_parent_dev_ops_get(struct camss *camss, int id)
+{
+	int ret = -EINVAL;
+
+	if (id < camss->res->vfe_num) {
+		struct vfe_device *vfe = &camss->vfe[id];
+
+		ret = vfe_get(vfe);
+	}
+
+	return ret;
+}
+
+static int vfe_parent_dev_ops_put(struct camss *camss, int id)
+{
+	if (id < camss->res->vfe_num) {
+		struct vfe_device *vfe = &camss->vfe[id];
+
+		vfe_put(vfe);
+	}
+
+	return 0;
+}
+
+static void __iomem
+*vfe_parent_dev_ops_get_base_address(struct camss *camss, int id)
+{
+	if (id < camss->res->vfe_num) {
+		struct vfe_device *vfe = &camss->vfe[id];
+
+		return vfe->base;
+	}
+
+	return NULL;
+}
+
+static const struct parent_dev_ops vfe_parent_dev_ops = {
+	.get = vfe_parent_dev_ops_get,
+	.put = vfe_parent_dev_ops_put,
+	.get_base_address = vfe_parent_dev_ops_get_base_address
+};
+
 /*
  * camss_of_parse_endpoint_node - Parse port endpoint node
  * @dev: Device
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index a5be9e872992..b3c967bcf8a9 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -143,6 +143,12 @@  struct camss_clock {
 	u32 nfreqs;
 };
 
+struct parent_dev_ops {
+	int (*get)(struct camss *camss, int id);
+	int (*put)(struct camss *camss, int id);
+	void __iomem *(*get_base_address)(struct camss *camss, int id);
+};
+
 void camss_add_clock_margin(u64 *rate);
 int camss_enable_clocks(int nclocks, struct camss_clock *clock,
 			struct device *dev);
@@ -153,6 +159,8 @@  s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
 int camss_get_pixel_clock(struct media_entity *entity, u64 *pixel_clock);
 int camss_pm_domain_on(struct camss *camss, int id);
 void camss_pm_domain_off(struct camss *camss, int id);
+int camss_vfe_get(struct camss *camss, int id);
+void camss_vfe_put(struct camss *camss, int id);
 void camss_delete(struct camss *camss);
 
 #endif /* QC_MSM_CAMSS_H */