Re: [PATCH 11/13] media: qcom: camss: Add notify interface in camss driver

[Depeng Shao]

Hi Bryan,

On 7/10/2024 7:54 PM, Bryan O'Donoghue wrote:
> On 09/07/2024 17:06, Depeng Shao wrote:
> > The main v4l2 process logic in camss vfe subdev driver, so the vfe driver
> > handles the buf done irq and register update configuration. But the buf
> > done irq and register update configuration have been moved CSID HW in
> > Titan 780 and other new platform, so vfe driver needs to call into CSID
> > driver to configure the register update. And CSID driver also needs to
> > call into vfe driver to notify of the buf done irq.
> >
> > Adding this notify interface in camss structure to do the subdevs cross

> >   /*
> >    * csid_isr - CSID module interrupt service routine
> >    * @irq: Interrupt line
> > @@ -341,6 +353,14 @@ static irqreturn_t csid_isr(int irq, void *dev)
> >           if (csid->phy.en_vc & BIT(i)) {
> >               val = readl_relaxed(csid->base + 
> > CSID_CSI2_RDIN_IRQ_STATUS(i));
> >               writel_relaxed(val, csid->base + 
> > CSID_CSI2_RDIN_IRQ_CLEAR(i));
> > +
> > +            if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + 
> > i)) {
> > +                /* For Titan 780, Buf Done IRQ&REG has been moved to 
> > CSID from VFE.
> > +                 * Once CSID received Buf Done, need notify this 
> > event to VFE.
> > +                 * Trigger VFE to handle Buf Done process.
> > +                 */
> > +                csid->camss->notify(&csid->subdev, 
> > CAMSS_MSG_BUF_DONE, (void *)&i);
>
> Instead of a generic notify function with an enum passed to a switch 
> lets just have a dedicated function for each functional callback.
>
> csid->camss->reg_update_cmd();
> csid->camss->reg_clear_cmd();
>
> We can get rid of a switch and an additional enum that way, plus 
> redundant "event not supported" error checking.

Sure, will update the code based on suggestion.


> > +            }
> >           }
> >       val = 1 << IRQ_CMD_CLEAR;
> > @@ -434,6 +454,23 @@ static void csid_subdev_init(struct csid_device 
> > *csid)
> >       csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
> >   }
> > +static void csid_subdev_process_msg(struct csid_device *csid, 
> > unsigned int msg_type, void *arg)
> > +{
> > +    int msg_data = *(int *)arg;
> > +
> > +    switch (msg_type) {
> > +    case CAMSS_MSG_RUP:
> > +        csid_reg_update(csid, msg_data);
> > +        break;
> > +    case CAMSS_MSG_RUP_CLEAR:
> > +        csid_reg_update_clear(csid, msg_data);
> > +        break;
> > +    default:
> > +        dev_err(csid->camss->dev, "NOT Supported EVT Type\n");
> > +        break;
> > +    }
> > +}
> > +
> >   const struct csid_hw_ops csid_ops_gen3 = {
> >       .configure_stream = csid_configure_stream,
> >       .configure_testgen_pattern = csid_configure_testgen_pattern,
> > @@ -442,4 +479,5 @@ const struct csid_hw_ops csid_ops_gen3 = {
> >       .reset = csid_reset,
> >       .src_pad_code = csid_src_pad_code,
> >       .subdev_init = csid_subdev_init,
> > +    .process_msg = csid_subdev_process_msg,


> > diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h 
> > b/drivers/media/platform/qcom/camss/camss-vfe.h
> > index 10e2cc3c0b83..a8b09ce9941b 100644
> > --- a/drivers/media/platform/qcom/camss/camss-vfe.h
> > +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
> > @@ -115,6 +115,7 @@ 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 (*process_msg)(struct vfe_device *vfe, unsigned int 
> > msg_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 1f1f44f6fbb2..abeb0918e47d 100644
> > --- a/drivers/media/platform/qcom/camss/camss.c
> > +++ b/drivers/media/platform/qcom/camss/camss.c
> > @@ -2202,6 +2202,52 @@ static void camss_genpd_cleanup(struct camss 
> > *camss)
> >       dev_pm_domain_detach(camss->genpd, true);
> >   }
> > +static void camss_notify_msg(struct v4l2_subdev *sd,
> > +            unsigned int msg_type, 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;
> > +
> > +    switch (msg_type) {
> > +    case CAMSS_MSG_BUF_DONE:
> > +        csid = v4l2_get_subdevdata(sd);
> > +        vfe = &(camss->vfe[csid->id]);
> > +        if (vfe->res->hw_ops->process_msg)
> > +            vfe->res->hw_ops->process_msg(vfe,
> > +                CAMSS_MSG_BUF_DONE, (void *)&evt_data);
> > +        break;
> > +
> > +    case CAMSS_MSG_RUP:
> > +        vfe_line = v4l2_get_subdevdata(sd);
> > +        vfe = to_vfe(vfe_line);
> > +        csid = &(camss->csid[vfe->id]);
> > +
> > +        if (csid->res->hw_ops->process_msg)
> > +            csid->res->hw_ops->process_msg(csid,
> > +                CAMSS_MSG_RUP, (void *)&evt_data);
> > +        break;
> > +
> > +    case CAMSS_MSG_RUP_CLEAR:
> > +        vfe_line = v4l2_get_subdevdata(sd);
> > +        vfe = to_vfe(vfe_line);
> > +        csid = &(camss->csid[vfe->id]);
> > +
> > +        if (csid->res->hw_ops->process_msg)
> > +            csid->res->hw_ops->process_msg(csid,
> > +                CAMSS_MSG_RUP_CLEAR, (void *)&evt_data);
> > +
> > +        break;
> > +
> > +    default:
> > +        dev_err(camss->dev, "Not supported evt type\n");
> > +        break;
> > +    }
> > +}
>
> Instead of having a central swiss army knife notify() callback these 
> should be CSID or VFE specific callbacks vfe->buf_done() csid->msg_rup() 
> etc.

I will try to add the callback in CSID and VFE driver.

Thanks,
Depeng