[04/13] media: qcom: camss: csiphy: Add an init callback to CSI PHY devices
Commit Message
From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Add a nop init callback to CSIPHY devices, this callback is used to add
some HW register offset and register configuration for specific platform,
then different platform can reuse the same CSIPHY driver. Later changes
will enumerate with enabling code.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
---
drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c | 6 ++++++
drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c | 6 ++++++
drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++++
drivers/media/platform/qcom/camss/camss-csiphy.h | 1 +
4 files changed, 17 insertions(+)
Comments
On 8/12/24 17:41, Depeng Shao wrote:
> From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>
> Add a nop init callback to CSIPHY devices, this callback is used to add
> some HW register offset and register configuration for specific platform,
> then different platform can reuse the same CSIPHY driver. Later changes
> will enumerate with enabling code.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> ---
> drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c | 6 ++++++
> drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c | 6 ++++++
> drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++++
> drivers/media/platform/qcom/camss/camss-csiphy.h | 1 +
> 4 files changed, 17 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
> index cd4a8c369234..9d67e7fa6366 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
> @@ -180,6 +180,11 @@ static irqreturn_t csiphy_isr(int irq, void *dev)
> return IRQ_HANDLED;
> }
>
> +static int csiphy_init(struct csiphy_device *csiphy)
> +{
> + return 0;
> +}
As far as I see from the patchset there is no intention to populate this function,
see a comment below.
> const struct csiphy_hw_ops csiphy_ops_2ph_1_0 = {
> .get_lane_mask = csiphy_get_lane_mask,
> .hw_version_read = csiphy_hw_version_read,
> @@ -187,4 +192,5 @@ const struct csiphy_hw_ops csiphy_ops_2ph_1_0 = {
> .lanes_enable = csiphy_lanes_enable,
> .lanes_disable = csiphy_lanes_disable,
> .isr = csiphy_isr,
> + .init = csiphy_init,
> };
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index bc4834ee2dcc..b60c32a195df 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> @@ -581,6 +581,11 @@ static void csiphy_lanes_disable(struct csiphy_device *csiphy,
> CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(6));
> }
>
> +static int csiphy_init(struct csiphy_device *csiphy)
> +{
> + return 0;
> +}
> +
> const struct csiphy_hw_ops csiphy_ops_3ph_1_0 = {
> .get_lane_mask = csiphy_get_lane_mask,
> .hw_version_read = csiphy_hw_version_read,
> @@ -588,4 +593,5 @@ const struct csiphy_hw_ops csiphy_ops_3ph_1_0 = {
> .lanes_enable = csiphy_lanes_enable,
> .lanes_disable = csiphy_lanes_disable,
> .isr = csiphy_isr,
> + .init = csiphy_init,
> };
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index 2f7361dfd461..ea5c7078ec8e 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -576,6 +576,10 @@ int msm_csiphy_subdev_init(struct camss *camss,
> csiphy->cfg.combo_mode = 0;
> csiphy->res = &res->csiphy;
>
> + ret = csiphy->res->hw_ops->init(csiphy);
> + if (ret)
> + return ret;
I've already expressed concerns about a necessity of this function, since it
adds runtime burden of work, which can be successfully done at compile time,
but okay...
Since it is needed for 3PH case only, it may make sense to remove it from 2PH
and call it here conditionally like
if (csiphy->res->hw_ops->init)
ret = csiphy->res->hw_ops->init(csiphy);
But it's up to you, I hope the callback will be removed in short future.
> /* Memory */
>
> csiphy->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
> index 47f0b6b09eba..bdf9a9c8bacc 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
> @@ -71,6 +71,7 @@ struct csiphy_hw_ops {
> void (*lanes_disable)(struct csiphy_device *csiphy,
> struct csiphy_config *cfg);
> irqreturn_t (*isr)(int irq, void *dev);
> + int (*init)(struct csiphy_device *csiphy);
> };
>
> struct csiphy_subdev_resources {
--
Best wishes,
Vladimir
Hi Bryan,
On 8/19/2024 8:17 AM, Vladimir Zapolskiy wrote:
> On 8/12/24 17:41, Depeng Shao wrote:
>> From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> I've already expressed concerns about a necessity of this function,
> since it
> adds runtime burden of work, which can be successfully done at compile
> time,
> but okay...
>
> Since it is needed for 3PH case only, it may make sense to remove it
> from 2PH
> and call it here conditionally like
>
> if (csiphy->res->hw_ops->init)
> ret = csiphy->res->hw_ops->init(csiphy);
>
> But it's up to you, I hope the callback will be removed in short future.
>
Could you please comment on if it is fine to remove the init from 2PH
driver?
Thanks,
Depeng
On 04/09/2024 15:20, Depeng Shao wrote:
> Hi Bryan,
>
> On 8/19/2024 8:17 AM, Vladimir Zapolskiy wrote:
>> On 8/12/24 17:41, Depeng Shao wrote:
>>> From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>
>> I've already expressed concerns about a necessity of this function,
>> since it
>> adds runtime burden of work, which can be successfully done at compile
>> time,
>> but okay...
>>
>> Since it is needed for 3PH case only, it may make sense to remove it
>> from 2PH
>> and call it here conditionally like
>>
>> if (csiphy->res->hw_ops->init)
>> ret = csiphy->res->hw_ops->init(csiphy);
>>
>> But it's up to you, I hope the callback will be removed in short future.
>>
>
> Could you please comment on if it is fine to remove the init from 2PH
> driver?
>
>
> Thanks,
> Depeng
Yes, its dead code ATM we can enumerate this callback when/if its needed.
Agreed.
---
bod
@@ -180,6 +180,11 @@ static irqreturn_t csiphy_isr(int irq, void *dev)
return IRQ_HANDLED;
}
+static int csiphy_init(struct csiphy_device *csiphy)
+{
+ return 0;
+}
+
const struct csiphy_hw_ops csiphy_ops_2ph_1_0 = {
.get_lane_mask = csiphy_get_lane_mask,
.hw_version_read = csiphy_hw_version_read,
@@ -187,4 +192,5 @@ const struct csiphy_hw_ops csiphy_ops_2ph_1_0 = {
.lanes_enable = csiphy_lanes_enable,
.lanes_disable = csiphy_lanes_disable,
.isr = csiphy_isr,
+ .init = csiphy_init,
};
@@ -581,6 +581,11 @@ static void csiphy_lanes_disable(struct csiphy_device *csiphy,
CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(6));
}
+static int csiphy_init(struct csiphy_device *csiphy)
+{
+ return 0;
+}
+
const struct csiphy_hw_ops csiphy_ops_3ph_1_0 = {
.get_lane_mask = csiphy_get_lane_mask,
.hw_version_read = csiphy_hw_version_read,
@@ -588,4 +593,5 @@ const struct csiphy_hw_ops csiphy_ops_3ph_1_0 = {
.lanes_enable = csiphy_lanes_enable,
.lanes_disable = csiphy_lanes_disable,
.isr = csiphy_isr,
+ .init = csiphy_init,
};
@@ -576,6 +576,10 @@ int msm_csiphy_subdev_init(struct camss *camss,
csiphy->cfg.combo_mode = 0;
csiphy->res = &res->csiphy;
+ ret = csiphy->res->hw_ops->init(csiphy);
+ if (ret)
+ return ret;
+
/* Memory */
csiphy->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
@@ -71,6 +71,7 @@ struct csiphy_hw_ops {
void (*lanes_disable)(struct csiphy_device *csiphy,
struct csiphy_config *cfg);
irqreturn_t (*isr)(int irq, void *dev);
+ int (*init)(struct csiphy_device *csiphy);
};
struct csiphy_subdev_resources {