| Message ID | 20241217140656.965235-2-quic_vikramsa@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: qcom: camss: Add sc7280 support | expand |
On 17/12/2024 15:06, Vikram Sharma wrote: > This patch change clock names to make it consistent with > existing platforms as gcc_cam_hf_axi -> gcc_axi_hf. > This also adds gcc_axi_sf and remove gcc_camera_ahb. Don't combine ABI changes with some less important things. You miss here explanation why doing the ABI change in the first place. Without that explanation I find it rather churn. But anyway, reason for ABI break and impact should be documented in commit msg. > > Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> > --- Best regards, Krzysztof
On 17/12/2024 14:10, Krzysztof Kozlowski wrote: > On 17/12/2024 15:06, Vikram Sharma wrote: >> This patch change clock names to make it consistent with >> existing platforms as gcc_cam_hf_axi -> gcc_axi_hf. >> This also adds gcc_axi_sf and remove gcc_camera_ahb. > > Don't combine ABI changes with some less important things. > > You miss here explanation why doing the ABI change in the first place. > Without that explanation I find it rather churn. But anyway, reason for > ABI break and impact should be documented in commit msg. > >> >> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >> --- > > Best regards, > Krzysztof This change should be fine since we haven't committed and upstream dtsi, so there's no ABI to break yet. Agree the cover letter should have been explicit in explaining. --- bod
On 17/12/2024 17:12, Bryan O'Donoghue wrote: > On 17/12/2024 14:10, Krzysztof Kozlowski wrote: >> On 17/12/2024 15:06, Vikram Sharma wrote: >>> This patch change clock names to make it consistent with >>> existing platforms as gcc_cam_hf_axi -> gcc_axi_hf. >>> This also adds gcc_axi_sf and remove gcc_camera_ahb. >> >> Don't combine ABI changes with some less important things. >> >> You miss here explanation why doing the ABI change in the first place. >> Without that explanation I find it rather churn. But anyway, reason for >> ABI break and impact should be documented in commit msg. >> >>> >>> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >>> --- >> >> Best regards, >> Krzysztof > > This change should be fine since we haven't committed and upstream dtsi, > so there's no ABI to break yet. > > Agree the cover letter should have been explicit in explaining. So these are recently added bindings (sc7280 is not particularly new)? If so mention in the commit msg that no users are affected because of this. Best regards, Krzysztof
On 17/12/2024 16:23, Krzysztof Kozlowski wrote: > On 17/12/2024 17:12, Bryan O'Donoghue wrote: >> On 17/12/2024 14:10, Krzysztof Kozlowski wrote: >>> On 17/12/2024 15:06, Vikram Sharma wrote: >>>> This patch change clock names to make it consistent with >>>> existing platforms as gcc_cam_hf_axi -> gcc_axi_hf. >>>> This also adds gcc_axi_sf and remove gcc_camera_ahb. >>> >>> Don't combine ABI changes with some less important things. >>> >>> You miss here explanation why doing the ABI change in the first place. >>> Without that explanation I find it rather churn. But anyway, reason for >>> ABI break and impact should be documented in commit msg. >>> >>>> >>>> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >>>> --- >>> >>> Best regards, >>> Krzysztof >> >> This change should be fine since we haven't committed and upstream dtsi, >> so there's no ABI to break yet. >> >> Agree the cover letter should have been explicit in explaining. > > So these are recently added bindings (sc7280 is not particularly new)? > If so mention in the commit msg that no users are affected because of this. > > Best regards, > Krzysztof Agreed. The commit log should make clear that the ABI hasn't been cemented yet. 20241217140656.965235-4-quic_vikramsa@quicinc.com <- is still pending --- bod
On 12/17/2024 10:00 PM, Bryan O'Donoghue wrote: > On 17/12/2024 16:23, Krzysztof Kozlowski wrote: >> On 17/12/2024 17:12, Bryan O'Donoghue wrote: >>> On 17/12/2024 14:10, Krzysztof Kozlowski wrote: >>>> On 17/12/2024 15:06, Vikram Sharma wrote: >>>>> This patch change clock names to make it consistent with >>>>> existing platforms as gcc_cam_hf_axi -> gcc_axi_hf. >>>>> This also adds gcc_axi_sf and remove gcc_camera_ahb. >>>> >>>> Don't combine ABI changes with some less important things. >>>> >>>> You miss here explanation why doing the ABI change in the first place. >>>> Without that explanation I find it rather churn. But anyway, reason >>>> for >>>> ABI break and impact should be documented in commit msg. >>>> >>>>> >>>>> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >>>>> --- >>>> >>>> Best regards, >>>> Krzysztof >>> >>> This change should be fine since we haven't committed and upstream >>> dtsi, >>> so there's no ABI to break yet. >>> >>> Agree the cover letter should have been explicit in explaining. >> >> So these are recently added bindings (sc7280 is not particularly new)? >> If so mention in the commit msg that no users are affected because of >> this. >> >> Best regards, >> Krzysztof > > Agreed. > > The commit log should make clear that the ABI hasn't been cemented yet. > > 20241217140656.965235-4-quic_vikramsa@quicinc.com <- is still pending Thanks Krzysztof and Bryan. I will update commit text to explain why ABI is not braking with this change. > > --- > bod
On Tue, Dec 17, 2024 at 04:30:37PM +0000, Bryan O'Donoghue wrote: > On 17/12/2024 16:23, Krzysztof Kozlowski wrote: > > On 17/12/2024 17:12, Bryan O'Donoghue wrote: > > > On 17/12/2024 14:10, Krzysztof Kozlowski wrote: > > > > On 17/12/2024 15:06, Vikram Sharma wrote: > > > > > This patch change clock names to make it consistent with > > > > > existing platforms as gcc_cam_hf_axi -> gcc_axi_hf. > > > > > This also adds gcc_axi_sf and remove gcc_camera_ahb. > > > > > > > > Don't combine ABI changes with some less important things. > > > > > > > > You miss here explanation why doing the ABI change in the first place. > > > > Without that explanation I find it rather churn. But anyway, reason for > > > > ABI break and impact should be documented in commit msg. > > > > > > > > > > > > > > Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> > > > > > --- > > > > > > > > Best regards, > > > > Krzysztof > > > > > > This change should be fine since we haven't committed and upstream dtsi, > > > so there's no ABI to break yet. > > > > > > Agree the cover letter should have been explicit in explaining. > > > > So these are recently added bindings (sc7280 is not particularly new)? > > If so mention in the commit msg that no users are affected because of this. > > > > Best regards, > > Krzysztof > > Agreed. > > The commit log should make clear that the ABI hasn't been cemented yet. > > 20241217140656.965235-4-quic_vikramsa@quicinc.com <- is still pending If it hasn't been comitted yet, isn't it better to post a fixed version rather than committing a version which has known issues? > > --- > bod
On 18/12/2024 11:36, Dmitry Baryshkov wrote: >> Agreed. >> >> The commit log should make clear that the ABI hasn't been cemented yet. >> >> 20241217140656.965235-4-quic_vikramsa@quicinc.com <- is still pending > If it hasn't been comitted yet, isn't it better to post a fixed version > rather than committing a version which has known issues? Yes. - yaml - driver are merged but - dtsi is not So we can still change yaml and driver prior to fixing dtsi. --- bod
On 18/12/2024 12:17, Bryan O'Donoghue wrote: > On 18/12/2024 11:36, Dmitry Baryshkov wrote: >>> Agreed. >>> >>> The commit log should make clear that the ABI hasn't been cemented yet. >>> >>> 20241217140656.965235-4-quic_vikramsa@quicinc.com <- is still pending >> If it hasn't been comitted yet, isn't it better to post a fixed version >> rather than committing a version which has known issues? > > Yes. > > - yaml > - driver > > are merged but > > - dtsi is not > > So we can still change yaml and driver prior to fixing dtsi. > > --- > bod Excuse me; prior to _committing_ the dtsi
diff --git a/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml index e11141b812a0..ee35e3bc97ff 100644 --- a/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml +++ b/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml @@ -55,8 +55,8 @@ properties: - const: csiphy3_timer - const: csiphy4 - const: csiphy4_timer - - const: gcc_camera_ahb - - const: gcc_cam_hf_axi + - const: gcc_axi_hf + - const: gcc_axi_sf - const: icp_ahb - const: vfe0 - const: vfe0_axi @@ -310,8 +310,8 @@ examples: <&camcc CAM_CC_CSI3PHYTIMER_CLK>, <&camcc CAM_CC_CSIPHY4_CLK>, <&camcc CAM_CC_CSI4PHYTIMER_CLK>, - <&gcc GCC_CAMERA_AHB_CLK>, <&gcc GCC_CAMERA_HF_AXI_CLK>, + <&gcc GCC_CAMERA_SF_AXI_CLK>, <&camcc CAM_CC_ICP_AHB_CLK>, <&camcc CAM_CC_IFE_0_CLK>, <&camcc CAM_CC_IFE_0_AXI_CLK>, @@ -343,8 +343,8 @@ examples: "csiphy3_timer", "csiphy4", "csiphy4_timer", - "gcc_camera_ahb", - "gcc_cam_hf_axi", + "gcc_axi_hf", + "gcc_axi_sf", "icp_ahb", "vfe0", "vfe0_axi",
This patch change clock names to make it consistent with existing platforms as gcc_cam_hf_axi -> gcc_axi_hf. This also adds gcc_axi_sf and remove gcc_camera_ahb. Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> --- .../devicetree/bindings/media/qcom,sc7280-camss.yaml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)