Messages in this thread | | | Date | Wed, 20 Mar 2024 16:08:44 +0000 | Subject | Re: [PATCH v2 2/8] media: qcom: camss: Add subdev notify support | From | Bryan O'Donoghue <> |
| |
On 20/03/2024 14:11, Depeng Shao wrote: > From: Yongsheng Li <quic_yon@quicinc.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
| |