| Message ID | 20241211140738.3835588-1-quic_depengs@quicinc.com |
|---|---|
| Headers | show |
| Series | media: qcom: camss: Add sm8550 support | expand |
@Depeng. Some of the patches at the top of the stack here - won't apply once Vikram's 7280 patches are applied. Could you please rebase your series with Vikram's patches applied and in v7 send a link in your cover-letter to highlight the dependency. You can get fixed up shared patches from my x1e tree here: https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/arm-laptop/wip/x1e80100-6.13-rc1+camss?ref_type=heads --- bod
On 11/12/2024 14:07, Depeng Shao wrote: > There is no CSID TPG in some modern HW, so the v4l2 ctrl in CSID driver "some modern HW" => "on some SoCs" > shouldn't be registered. Checking the supported TPG modes to indicate > if the TPG HW is existing or not, and only register v4l2 ctrl for CSID "TP HW is existing or not, and only register" => "TPG hardware exists or not and oly registering" No need to abbreviate hardware to HW. > only when TPG HW is existing. "when the TPG hardware exists" you could also say "is present" instead of "exists". You have additional whitespace in your log before " only" > > Signed-off-by: Depeng Shao <quic_depengs@quicinc.com> > --- > .../media/platform/qcom/camss/camss-csid.c | 60 +++++++++++-------- > 1 file changed, 35 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c > index 6cf8e434dc05..e26a69a454a7 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.c > +++ b/drivers/media/platform/qcom/camss/camss-csid.c > @@ -760,11 +760,13 @@ static int csid_set_stream(struct v4l2_subdev *sd, int enable) > int ret; > > if (enable) { > - ret = v4l2_ctrl_handler_setup(&csid->ctrls); > - if (ret < 0) { > - dev_err(csid->camss->dev, > - "could not sync v4l2 controls: %d\n", ret); > - return ret; > + if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED) { > + ret = v4l2_ctrl_handler_setup(&csid->ctrls); > + if (ret < 0) { > + dev_err(csid->camss->dev, > + "could not sync v4l2 controls: %d\n", ret); > + return ret; > + } > } > > if (!csid->testgen.enabled && > @@ -838,7 +840,8 @@ static void csid_try_format(struct csid_device *csid, > break; > > case MSM_CSID_PAD_SRC: > - if (csid->testgen_mode->cur.val == 0) { > + if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED || if (csid->ctrls || feels like a more natural test. Less cumbersome and also less typing. I think that change should be feasible, could you please update your logic from if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED) to if (csid->ctrls) Otherwise looks good but, I'll wait to see your next version before giving an RB. --- bod
On 11/12/2024 15:36, Bryan O'Donoghue wrote: > @Depeng. > > Some of the patches at the top of the stack here - won't apply once > Vikram's 7280 patches are applied. > > Could you please rebase your series with Vikram's patches applied and in > v7 send a link in your cover-letter to highlight the dependency. > > You can get fixed up shared patches from my x1e tree here: > > https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/arm-laptop/wip/ > x1e80100-6.13-rc1+camss?ref_type=heads > > --- > bod > https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/arm-laptop/wip/x1e80100-6.13-rc2+camss?ref_type=heads Same patches on rc2. --- bod
On 11/12/2024 14:07, Depeng Shao wrote: > +static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val) > +{ > + return 0; > +} Could we avoid this empty callback by checking csid->ctrl in csid.c ? If so, please make that change. If not, it's fine. For either case. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Hi Depeng and Bryan. On 12/11/24 16:07, Depeng Shao wrote: > The RUP registers and buf done irq are moved from the IFE to CSID register > block on recent CAMSS implementations. Add callbacks structure to wrapper > the location change with minimum logic disruption. > > Signed-off-by: Depeng Shao <quic_depengs@quicinc.com> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> It's unexpected to see two your Signed-off-by: tags, either one is invalid or the authorship of the change shall be changed appropriately. > --- > .../media/platform/qcom/camss/camss-csid.h | 9 ++++++++ > drivers/media/platform/qcom/camss/camss.c | 22 +++++++++++++++++++ > drivers/media/platform/qcom/camss/camss.h | 3 +++ > 3 files changed, 34 insertions(+) > > diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h > index f52209b96583..1369e7ea7219 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.h > +++ b/drivers/media/platform/qcom/camss/camss-csid.h > @@ -152,6 +152,14 @@ struct csid_hw_ops { > * @csid: CSID device > */ > void (*subdev_init)(struct csid_device *csid); > + > + /* > + * reg_update - receive message from other sub device > + * @csid: CSID device > + * @port_id: Port id > + * @is_clear: Indicate if it is clearing reg update or setting reg update > + */ > + void (*reg_update)(struct csid_device *csid, int port_id, bool is_clear); > }; > > struct csid_subdev_resources { > @@ -168,6 +176,7 @@ struct csid_device { > struct media_pad pads[MSM_CSID_PADS_NUM]; > void __iomem *base; > u32 irq; > + u32 reg_update; > char irq_name[30]; > struct camss_clock *clock; > int nclocks; > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index 9fb31f4c18ad..e24084ff88de 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -2087,6 +2087,28 @@ static int camss_link_entities(struct camss *camss) > return 0; > } > > +void camss_reg_update(struct camss *camss, int hw_id, int port_id, bool is_clear) > +{ > + struct csid_device *csid; > + > + if (hw_id < camss->res->csid_num) { > + csid = &camss->csid[hw_id]; > + > + csid->res->hw_ops->reg_update(csid, port_id, is_clear); > + } > +} > + > +void camss_buf_done(struct camss *camss, int hw_id, int port_id) > +{ > + struct vfe_device *vfe; > + > + if (hw_id < camss->res->vfe_num) { > + vfe = &camss->vfe[hw_id]; > + > + vfe->res->hw_ops->vfe_buf_done(vfe, port_id); > + } > +} > + > /* > * camss_register_entities - Register subdev nodes and create links > * @camss: CAMSS device > diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h > index 9da7f48f5dd7..6dceff8ce319 100644 > --- a/drivers/media/platform/qcom/camss/camss.h > +++ b/drivers/media/platform/qcom/camss/camss.h > @@ -161,5 +161,8 @@ void camss_pm_domain_off(struct camss *camss, int id); > int camss_vfe_get(struct camss *camss, int id); > void camss_vfe_put(struct camss *camss, int id); > void camss_delete(struct camss *camss); > +void camss_buf_done(struct camss *camss, int hw_id, int port_id); > +void camss_reg_update(struct camss *camss, int hw_id, > + int port_id, bool is_clear); > > #endif /* QC_MSM_CAMSS_H */ -- Best wishes, Vladimir
On 12/12/2024 01:09, Vladimir Zapolskiy wrote: >> >> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > It's unexpected to see two your Signed-off-by: tags, either one is invalid > or the authorship of the change shall be changed appropriately. TBH I thought nothing of this at all. @Depeng for the record you can add Co-developed-by with my SOB. --- bod
On 12/12/24 03:32, Bryan O'Donoghue wrote: > On 12/12/2024 01:09, Vladimir Zapolskiy wrote: >>> >>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com> >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> >> It's unexpected to see two your Signed-off-by: tags, either one is invalid >> or the authorship of the change shall be changed appropriately. > > TBH I thought nothing of this at all. > > @Depeng for the record you can add Co-developed-by with my SOB. > Thank you, then this will become aligned with Documentation/process/5.Posting.rst -- Best wishes, Vladimir
Hi Vladimir, On 12/12/2024 9:09 AM, Vladimir Zapolskiy wrote: > Hi Depeng and Bryan. > > On 12/11/24 16:07, Depeng Shao wrote: >> The RUP registers and buf done irq are moved from the IFE to CSID >> register >> block on recent CAMSS implementations. Add callbacks structure to wrapper >> the location change with minimum logic disruption. >> >> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > It's unexpected to see two your Signed-off-by: tags, either one is invalid > or the authorship of the change shall be changed appropriately. > Thanks for pointing out this, I will update it based on Bryan's suggestion. Thanks, Depeng
Hi Bryan, On 12/12/2024 12:03 AM, Bryan O'Donoghue wrote: > On 11/12/2024 15:36, Bryan O'Donoghue wrote: >> @Depeng. >> >> Some of the patches at the top of the stack here - won't apply once >> Vikram's 7280 patches are applied. >> >> Could you please rebase your series with Vikram's patches applied and >> in v7 send a link in your cover-letter to highlight the dependency. >> >> You can get fixed up shared patches from my x1e tree here: >> >> https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/arm-laptop/ >> wip/ x1e80100-6.13-rc1+camss?ref_type=heads >> >> --- >> bod >> > > https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/arm-laptop/wip/ > x1e80100-6.13-rc2+camss?ref_type=heads > > Same patches on rc2. > Sure, I will rebase the change based on Vikram's patches. Thanks, Depeng
Hi Bryan, On 12/12/2024 5:57 AM, Bryan O'Donoghue wrote: > On 11/12/2024 14:07, Depeng Shao wrote: >> +static int csid_configure_testgen_pattern(struct csid_device *csid, >> s32 val) >> +{ >> + return 0; >> +} > > Could we avoid this empty callback by checking csid->ctrl in csid.c ? > > If so, please make that change. > > If not, it's fine. > > For either case. > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Vladimir suggested to add a dummy "return 0" function [1] for the unsupported interface. So, I added this empty callback, will keep the empty callback if no other concern. Thanks. [1] https://lore.kernel.org/all/b1e1ff88-5bba-4424-bc85-38caa85b831f@linaro.org/ Thanks, Depeng
On 12/12/2024 11:28, Depeng Shao wrote: >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > Vladimir suggested to add a dummy "return 0" function [1] for the > unsupported interface. So, I added this empty callback, will keep the > empty callback if no other concern. Thanks. > > [1] https://lore.kernel.org/all/b1e1ff88-5bba-4424- > bc85-38caa85b831f@linaro.org/ Go ahead. --- bod
Hi Bryan, On 12/11/2024 11:44 PM, Bryan O'Donoghue wrote: > On 11/12/2024 14:07, Depeng Shao wrote: >> There is no CSID TPG in some modern HW, so the v4l2 ctrl in CSID driver > > "some modern HW" => "on some SoCs" > >> shouldn't be registered. Checking the supported TPG modes to indicate >> if the TPG HW is existing or not, and only register v4l2 ctrl for CSID > > "TP HW is existing or not, and only register" => "TPG hardware exists or > not and oly registering" > > No need to abbreviate hardware to HW. > >> only when TPG HW is existing. > > "when the TPG hardware exists" you could also say "is present" instead > of "exists". > > You have additional whitespace in your log before " only" > >> Thanks for the suggestion, will update in new version. >> } >> if (!csid->testgen.enabled && >> @@ -838,7 +840,8 @@ static void csid_try_format(struct csid_device *csid, >> break; >> case MSM_CSID_PAD_SRC: >> - if (csid->testgen_mode->cur.val == 0) { >> + if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED || > > if (csid->ctrls || > > feels like a more natural test. Less cumbersome and also less typing. > > I think that change should be feasible, could you please update your > logic from if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED) to if > (csid->ctrls) > > Otherwise looks good but, I'll wait to see your next version before > giving an RB. > csid->ctrls is not a pointer, so it is always true. Thanks, Depeng
v6: - Add bus type property in dt-binding which will be limited by a latest change https://lore.kernel.org/all/20241209-camss-dphy-v1-0-5f1b6f25ed92@fairphone.com/ - Add RB for "media: qcom: camss: Add sm8550 compatible" and "media: qcom: camss: Add support for VFE 780" - Uppercase the hex in csiphy register list - Bryan - Add empty function for csid tpg - Vladimir - Set testgen mode to CSID_PAYLOAD_MODE_DISABLED in subdev init interface - encapsulate the guard __thus__ for new header - Bryan - Add a standalone patch for the platform which doesn't support CSID TPG to make sure new platform driver can set CSID_PAYLOAD_MODE_DISABLED to disable TPG - Update the csid for csid and vfe driver - Bryan - Link to v5: https://lore.kernel.org/all/20241205155538.250743-1-quic_depengs@quicinc.com/ v5: - Update dt-bindings required items order - Krzysztof - Sort the reg order based on the comments in sc7280 dt-binding - Vladimir - Change the irq type to IRQ_TYPE_EDGE_RISING - Vladimir - Remove the Krzysztof's RB tag from dt-binding patch due to above updates in dt-binding patch - Move regulator from csid resource to csiphy resource - Bryan, Vladimir - Move the change to add default case in vfe_src_pad_code to a standalone patch. - Bryan - Rename csid-gen3 as csid-780 - Bryan - use macros() to bury bit shifts - Bryan - Sort the macros by register offset order - Vladimir - Redefine a macro for rup_aup in csid driver - Vladimir - Remove the unused macros in vfe 780 driver - Vladimir - Add dummy function for unsupported hw_ops in vfe 780 driver - Vladimir, Bryan - Use a standalone patch for the callback API of RUP and buf done update - Use a standalone patch to make CSID TPG optional - Vladimir - Link to v4: https://lore.kernel.org/all/20240812144131.369378-1-quic_depengs@quicinc.com/ v4: - Update dt-bindings based on comments - Krzysztof, bod, Vladimir - Move common code into csid core and vfe core driver - bod - Remove *_relaxed in the csid and vfe drivers - Krzysztof - Reorganize patches in logical junks, make sure that new added structures have users in current patch - Krzysztof - Remove notify function and add new functions in camss for buf done and reg update - bod - Remove custom code to get csid base - bod - Remove ISR function in vfe780 driver since it is never fired - bod - Move csid_top_base to camss structure since we only have one csid top block, and just need to get base once for csid top - Add Vladimir's RB - Remove prerequisite-patch-id in the cover letter since the changes have been merged - Add dtsi patch link for reference - Krzysztof https://lore.kernel.org/all/20240807123333.2056518-1-quic_depengs@quicinc.com/ - Link to v3: https://lore.kernel.org/all/20240709160656.31146-1-quic_depengs@quicinc.com/ v3: - Rebased the change based on below change which will be merged firstly. "Move camss version related defs in to resources" Link: https://lore.kernel.org/all/20240522154659.510-1-quic_grosikop@quicinc.com/ - Rebased the change based on Bryan's csiphy optimization change and add these changes into this series, so that the new csiphy-3ph driver don't need to add duplicate code. This has got Bryan's permission to add his patches into this series. - Refactor some changes based on the comments to move the random code to patches where they are used. - Remove the vfe780 irq function since it isn't doing the actual work. - Add dt-binding for sm8550 camss driver. Link to V2: https://lore.kernel.org/all/20240320141136.26827-1-quic_depengs@quicinc.com/ v2: - Update some commit messages Link to V1: https://lore.kernel.org/all/20240320134227.16587-1-quic_depengs@quicinc.com/ v1: SM8550 is a Qualcomm flagship SoC. This series adds support to bring up the CSIPHY, CSID, VFE/RDI interfaces in SM8550. SM8550 provides - 3 x VFE, 3 RDI per VFE - 2 x VFE Lite, 4 RDI per VFE - 3 x CSID - 2 x CSID Lite - 8 x CSI PHY Bryan O'Donoghue (6): media: qcom: camss: csiphy-3ph: Fix trivial indentation fault in defines media: qcom: camss: csiphy-3ph: Remove redundant PHY init sequence control loop media: qcom: camss: csiphy-3ph: Rename struct media: qcom: camss: csiphy: Add an init callback to CSI PHY devices media: qcom: camss: csiphy-3ph: Move CSIPHY variables to data field inside csiphy struct media: qcom: camss: csiphy-3ph: Use an offset variable to find common control regs Depeng Shao (10): media: qcom: camss: csid: Move common code into csid core media: qcom: camss: vfe: Move common code into vfe core media: qcom: camss: Add callback API for RUP update and buf done media: qcom: camss: Add default case in vfe_src_pad_code media: qcom: camss: csid: Add v4l2 ctrl if TPG mode isn't disabled dt-bindings: media: camss: Add qcom,sm8550-camss binding media: qcom: camss: Add sm8550 compatible media: qcom: camss: csiphy-3ph: Add Gen2 v2.1.2 two-phase MIPI CSI-2 DPHY support media: qcom: camss: Add CSID 780 support media: qcom: camss: Add support for VFE 780 .../bindings/media/qcom,sm8550-camss.yaml | 604 ++++++++++++++ drivers/media/platform/qcom/camss/Makefile | 2 + .../platform/qcom/camss/camss-csid-4-1.c | 19 - .../platform/qcom/camss/camss-csid-4-7.c | 42 - .../platform/qcom/camss/camss-csid-780.c | 337 ++++++++ .../platform/qcom/camss/camss-csid-780.h | 25 + .../platform/qcom/camss/camss-csid-gen2.c | 60 -- .../media/platform/qcom/camss/camss-csid.c | 137 ++- .../media/platform/qcom/camss/camss-csid.h | 31 + .../qcom/camss/camss-csiphy-2ph-1-0.c | 6 + .../qcom/camss/camss-csiphy-3ph-1-0.c | 789 ++++++++++-------- .../media/platform/qcom/camss/camss-csiphy.c | 4 + .../media/platform/qcom/camss/camss-csiphy.h | 8 + .../media/platform/qcom/camss/camss-vfe-17x.c | 112 +-- .../media/platform/qcom/camss/camss-vfe-4-1.c | 9 - .../media/platform/qcom/camss/camss-vfe-4-7.c | 11 - .../media/platform/qcom/camss/camss-vfe-4-8.c | 11 - .../media/platform/qcom/camss/camss-vfe-480.c | 301 +------ .../media/platform/qcom/camss/camss-vfe-780.c | 159 ++++ drivers/media/platform/qcom/camss/camss-vfe.c | 282 ++++++- drivers/media/platform/qcom/camss/camss-vfe.h | 58 +- drivers/media/platform/qcom/camss/camss.c | 369 ++++++++ drivers/media/platform/qcom/camss/camss.h | 4 + 23 files changed, 2495 insertions(+), 885 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/qcom,sm8550-camss.yaml create mode 100644 drivers/media/platform/qcom/camss/camss-csid-780.c create mode 100644 drivers/media/platform/qcom/camss/camss-csid-780.h create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c base-commit: bcf2acd8f64b0a5783deeeb5fd70c6163ec5acd7