linux-kernel - Re: [PATCH v2 2/8] media: qcom: camss: Add subdev notify support

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: <21d2139f-8547-466a-9be1-fbeae1194869@linaro.org>
Date: Wed, 20 Mar 2024 16:08:44 +0000
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Depeng Shao <quic_depengs@...cinc.com>, rfoss@...nel.org,
 todor.too@...il.com, bryan.odonoghue@...aro.org, andersson@...nel.org,
 konrad.dybcio@...aro.org, mchehab@...nel.org, quic_yon@...cinc.com
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 2/8] media: qcom: camss: Add subdev notify support

On 20/03/2024 14:11, Depeng Shao wrote:
> From: Yongsheng Li <quic_yon@...cinc.com>
> 
> The buf done irq and register update register are moved
> to CSID in SM8550, so but the write master configuration
> in VFE, in case adapt existing code logic. So add buf
> done and register related subdev event, and use the notify
> interface in the v4l2_device structure to communicate
> between CSID and VFE driver.


Shouldn't it be possible to just have a function to write internally ?

You know the indexes of the CSID -> VFE connection.

The subdev notify is I think not the right fit for this purpose within 
our driver.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index fddccb69da13..4a9e5a2d1f92 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -147,6 +147,13 @@ struct csid_hw_ops {
>   	 * @csid: CSID device
>   	 */
>   	void (*subdev_init)(struct csid_device *csid);
> +
> +	/*
> +	 * event - receive event from parent v4l2 device
> +	 * @csid: CSID device
> +	 */
> +	void (*event)(struct csid_device *csid,
> +			unsigned int evt_type, void *arg);
>   };
>   
>   struct csid_device {
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
> index c9b7fe82b1f0..ffe1b95eea98 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
> @@ -61,6 +61,8 @@ struct csiphy_hw_ops {
>   	void (*lanes_disable)(struct csiphy_device *csiphy,
>   			      struct csiphy_config *cfg);
>   	irqreturn_t (*isr)(int irq, void *dev);
> +	void (*event)(struct csiphy_device *csiphy,
> +			unsigned int evt_type, void *arg);
>   };
>   
>   struct csiphy_device {
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
> index 0572c9b08e11..9919fe0ff101 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
> @@ -115,6 +115,8 @@ struct vfe_hw_ops {
>   	int (*vfe_halt)(struct vfe_device *vfe);
>   	void (*violation_read)(struct vfe_device *vfe);
>   	void (*vfe_wm_stop)(struct vfe_device *vfe, u8 wm);
> +	void (*event)(struct vfe_device *vfe,
> +			unsigned int evt_type, void *arg);
>   };
>   
>   struct vfe_isr_ops {
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 1923615f0eea..b57cd25bf6c7 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1904,6 +1904,55 @@ static void camss_genpd_cleanup(struct camss *camss)
>   	dev_pm_domain_detach(camss->genpd, true);
>   }
>   
> +static void camss_v4l2_subdev_notify(struct v4l2_subdev *sd,
> +			unsigned int cmd, void *arg)
> +{
> +	struct v4l2_device *v4l2_dev = sd->v4l2_dev;
> +	struct camss *camss = to_camss(v4l2_dev);
> +	struct vfe_device *vfe;
> +	struct vfe_line *vfe_line;
> +	struct csid_device *csid;
> +	int evt_data = *(int *)arg;
> +
> +	if (camss->res->version != CAMSS_8550)
> +		return;
> +
> +	switch (cmd) {
> +	case NOTIFY_BUF_DONE:
> +		csid = v4l2_get_subdevdata(sd);
> +		vfe = &(camss->vfe[csid->id]);
> +		if (vfe->ops->event)
> +			vfe->ops->event(vfe,
> +				NOTIFY_BUF_DONE, (void *)&evt_data);
> +		break;
> +
> +	case NOTIFY_RUP:
> +		vfe_line = v4l2_get_subdevdata(sd);
> +		vfe = to_vfe(vfe_line);
> +		csid = &(camss->csid[vfe->id]);
> +
> +		if (csid->ops->event)
> +			csid->ops->event(csid,
> +				NOTIFY_RUP, (void *)&evt_data);
> +		break;
> +
> +	case NOTIFY_RUP_CLEAR:
> +		vfe_line = v4l2_get_subdevdata(sd);
> +		vfe = to_vfe(vfe_line);
> +		csid = &(camss->csid[vfe->id]);
> +
> +		if (csid->ops->event)
> +			csid->ops->event(csid,
> +				NOTIFY_RUP_CLEAR, (void *)&evt_data);
> +
> +		break;
> +
> +	default:
> +		dev_err(camss->dev, "Not supported evt type\n");
> +		break;
> +	}
> +}

I'm really not sure I see a good reason for this.

Why can't we just define calls between vfe and csid similar to

drivers/media/platform/qcom/camss/camss-csid.c:		ret = vfe_get(vfe);

---
bod