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

[Depeng Shao <quic_depengs@quicinc.com>]

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