[PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

Camss drivers spam kernel dmesg with 64 useless messages during boot:

  qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
  qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0

All of these messages are the same, so it makes no sense to print same
information 32 times.

The driver does not use read version at all, so if it was needed for any
real debugging purpose it would be provided via debugfs interface.
However even then printing this is pointless, because version of
hardware block is deducible from the compatible.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../media/platform/qcom/camss/camss-vfe-17x.c |  1 -
 .../media/platform/qcom/camss/camss-vfe-4-1.c |  1 -
 .../media/platform/qcom/camss/camss-vfe-4-7.c |  1 -
 .../media/platform/qcom/camss/camss-vfe-4-8.c |  1 -
 .../media/platform/qcom/camss/camss-vfe-480.c |  1 -
 .../media/platform/qcom/camss/camss-vfe-680.c |  1 -
 .../media/platform/qcom/camss/camss-vfe-780.c |  1 -
 drivers/media/platform/qcom/camss/camss-vfe.c | 22 -------------------
 drivers/media/platform/qcom/camss/camss-vfe.h |  8 -------
 9 files changed, 37 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-17x.c b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
index e5ee7e717b3b..e0d12c3f6015 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-17x.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
@@ -577,7 +577,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 
 const struct vfe_hw_ops vfe_ops_170 = {
 	.global_reset = vfe_global_reset,
-	.hw_version = vfe_hw_version,
 	.isr_read = vfe_isr_read,
 	.isr = vfe_isr,
 	.pm_domain_off = vfe_pm_domain_off,
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
index 901677293d97..7620ce42b49b 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
@@ -993,7 +993,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 
 const struct vfe_hw_ops vfe_ops_4_1 = {
 	.global_reset = vfe_global_reset,
-	.hw_version = vfe_hw_version,
 	.isr_read = vfe_isr_read,
 	.isr = vfe_isr,
 	.pm_domain_off = vfe_4_1_pm_domain_off,
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
index 76729607db02..b3b6ccb4748e 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
@@ -1145,7 +1145,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 
 const struct vfe_hw_ops vfe_ops_4_7 = {
 	.global_reset = vfe_global_reset,
-	.hw_version = vfe_hw_version,
 	.isr_read = vfe_isr_read,
 	.isr = vfe_isr,
 	.pm_domain_off = vfe_pm_domain_off,
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
index b2f7d855d8dd..5a4b4f486aca 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
@@ -1135,7 +1135,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 
 const struct vfe_hw_ops vfe_ops_4_8 = {
 	.global_reset = vfe_global_reset,
-	.hw_version = vfe_hw_version,
 	.isr_read = vfe_isr_read,
 	.isr = vfe_isr,
 	.pm_domain_off = vfe_pm_domain_off,
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
index 4feea590a47b..edd92308af62 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
@@ -278,7 +278,6 @@ static void vfe_buf_done_480(struct vfe_device *vfe, int port_id)
 const struct vfe_hw_ops vfe_ops_480 = {
 	.enable_irq = vfe_enable_irq,
 	.global_reset = vfe_global_reset,
-	.hw_version = vfe_hw_version,
 	.isr = vfe_isr,
 	.isr_read = vfe_isr_read,
 	.reg_update = vfe_reg_update,
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-680.c b/drivers/media/platform/qcom/camss/camss-vfe-680.c
index 99036e7c1e76..96a927acc6bb 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-680.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-680.c
@@ -227,7 +227,6 @@ static inline void vfe_reg_update_clear(struct vfe_device *vfe,
 
 const struct vfe_hw_ops vfe_ops_680 = {
 	.global_reset = vfe_global_reset,
-	.hw_version = vfe_hw_version,
 	.isr = vfe_isr,
 	.pm_domain_off = vfe_pm_domain_off,
 	.pm_domain_on = vfe_pm_domain_on,
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c b/drivers/media/platform/qcom/camss/camss-vfe-780.c
index b9812d70f91b..e5023eb7ad60 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-780.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
@@ -142,7 +142,6 @@ static int vfe_halt(struct vfe_device *vfe)
 
 const struct vfe_hw_ops vfe_ops_780 = {
 	.global_reset = vfe_global_reset,
-	.hw_version = vfe_hw_version,
 	.isr = vfe_isr,
 	.pm_domain_off = vfe_pm_domain_off,
 	.pm_domain_on = vfe_pm_domain_on,
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 4bca6c3abaff..1ae523219525 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -415,26 +415,6 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
 	return 0;
 }
 
-/*
- * vfe_hw_version - Process write master done interrupt
- * @vfe: VFE Device
- *
- * Return vfe hw version
- */
-u32 vfe_hw_version(struct vfe_device *vfe)
-{
-	u32 hw_version = readl_relaxed(vfe->base + VFE_HW_VERSION);
-
-	u32 gen = (hw_version >> HW_VERSION_GENERATION) & 0xF;
-	u32 rev = (hw_version >> HW_VERSION_REVISION) & 0xFFF;
-	u32 step = (hw_version >> HW_VERSION_STEPPING) & 0xFFFF;
-
-	dev_dbg(vfe->camss->dev, "VFE:%d HW Version = %u.%u.%u\n",
-		vfe->id, gen, rev, step);
-
-	return hw_version;
-}
-
 /*
  * vfe_buf_done - Process write master done interrupt
  * @vfe: VFE Device
@@ -1088,8 +1068,6 @@ int vfe_get(struct vfe_device *vfe)
 		vfe_reset_output_maps(vfe);
 
 		vfe_init_outputs(vfe);
-
-		vfe->res->hw_ops->hw_version(vfe);
 	} else {
 		ret = vfe_check_clock_rates(vfe);
 		if (ret < 0)
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index a23f666be753..1553ca89bd86 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h
@@ -101,7 +101,6 @@ struct vfe_device;
 struct vfe_hw_ops {
 	void (*enable_irq)(struct vfe_device *vfe);
 	void (*global_reset)(struct vfe_device *vfe);
-	u32 (*hw_version)(struct vfe_device *vfe);
 	irqreturn_t (*isr)(int irq, void *dev);
 	void (*isr_read)(struct vfe_device *vfe, u32 *value0, u32 *value1);
 	void (*pm_domain_off)(struct vfe_device *vfe);
@@ -259,13 +258,6 @@ void vfe_put(struct vfe_device *vfe);
  */
 bool vfe_is_lite(struct vfe_device *vfe);
 
-/*
- * vfe_hw_version - Process write master done interrupt
- * @vfe: VFE Device
- *
- * Return vfe hw version
- */
-u32 vfe_hw_version(struct vfe_device *vfe);
 /*
  * vfe_enable - Enable streaming on VFE line
  * @line: VFE line
-- 
2.45.2

[PATCH 2/3] media: qcom: camss: csid: Stop spamming logs with version

[Krzysztof Kozlowski]

Camss drivers spam kernel dmesg with 64 useless messages during boot:

  qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
  qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0

All of these messages are the same, so it makes no sense to print same
information 32 times.

The driver does not use read version at all, so if it was needed for any
real debugging purpose it would be provided via debugfs interface.
However even then printing this is pointless, because version of
hardware block is deducible from the compatible.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../platform/qcom/camss/camss-csid-4-1.c      |  1 -
 .../platform/qcom/camss/camss-csid-4-7.c      |  1 -
 .../platform/qcom/camss/camss-csid-680.c      |  1 -
 .../platform/qcom/camss/camss-csid-780.c      |  1 -
 .../platform/qcom/camss/camss-csid-gen2.c     |  1 -
 .../media/platform/qcom/camss/camss-csid.c    | 25 -------------------
 .../media/platform/qcom/camss/camss-csid.h    | 14 -----------
 7 files changed, 44 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csid-4-1.c b/drivers/media/platform/qcom/camss/camss-csid-4-1.c
index 6998e1c52895..8771e20d2a44 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-4-1.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-4-1.c
@@ -179,7 +179,6 @@ static void csid_subdev_init(struct csid_device *csid)
 const struct csid_hw_ops csid_ops_4_1 = {
 	.configure_stream = csid_configure_stream,
 	.configure_testgen_pattern = csid_configure_testgen_pattern,
-	.hw_version = csid_hw_version,
 	.isr = csid_isr,
 	.reset = csid_reset,
 	.src_pad_code = csid_src_pad_code,
diff --git a/drivers/media/platform/qcom/camss/camss-csid-4-7.c b/drivers/media/platform/qcom/camss/camss-csid-4-7.c
index 66054d4872e6..80135e63c595 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-4-7.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-4-7.c
@@ -204,7 +204,6 @@ static void csid_subdev_init(struct csid_device *csid)
 const struct csid_hw_ops csid_ops_4_7 = {
 	.configure_stream = csid_configure_stream,
 	.configure_testgen_pattern = csid_configure_testgen_pattern,
-	.hw_version = csid_hw_version,
 	.isr = csid_isr,
 	.reset = csid_reset,
 	.src_pad_code = csid_src_pad_code,
diff --git a/drivers/media/platform/qcom/camss/camss-csid-680.c b/drivers/media/platform/qcom/camss/camss-csid-680.c
index 3ad3a174bcfb..6eb9a5efa96a 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-680.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-680.c
@@ -413,7 +413,6 @@ static void csid_subdev_init(struct csid_device *csid) {}
 const struct csid_hw_ops csid_ops_680 = {
 	.configure_testgen_pattern = NULL,
 	.configure_stream = csid_configure_stream,
-	.hw_version = csid_hw_version,
 	.isr = csid_isr,
 	.reset = csid_reset,
 	.src_pad_code = csid_src_pad_code,
diff --git a/drivers/media/platform/qcom/camss/camss-csid-780.c b/drivers/media/platform/qcom/camss/camss-csid-780.c
index 4c720d177731..8a8999747905 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-780.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-780.c
@@ -328,7 +328,6 @@ static void csid_subdev_init(struct csid_device *csid)
 const struct csid_hw_ops csid_ops_780 = {
 	.configure_stream = csid_configure_stream,
 	.configure_testgen_pattern = csid_configure_testgen_pattern,
-	.hw_version = csid_hw_version,
 	.isr = csid_isr,
 	.reset = csid_reset,
 	.src_pad_code = csid_src_pad_code,
diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
index 2a1746dcc1c5..9607ebd7fa3c 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
@@ -424,7 +424,6 @@ static void csid_subdev_init(struct csid_device *csid)
 const struct csid_hw_ops csid_ops_gen2 = {
 	.configure_stream = csid_configure_stream,
 	.configure_testgen_pattern = csid_configure_testgen_pattern,
-	.hw_version = csid_hw_version,
 	.isr = csid_isr,
 	.reset = csid_reset,
 	.src_pad_code = csid_src_pad_code,
diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index 5284b5857368..2f5058b681dc 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -596,29 +596,6 @@ static int csid_set_clock_rates(struct csid_device *csid)
 	return 0;
 }
 
-/*
- * csid_hw_version - CSID hardware version query
- * @csid: CSID device
- *
- * Return HW version or error
- */
-u32 csid_hw_version(struct csid_device *csid)
-{
-	u32 hw_version;
-	u32 hw_gen;
-	u32 hw_rev;
-	u32 hw_step;
-
-	hw_version = readl_relaxed(csid->base + CSID_HW_VERSION);
-	hw_gen = (hw_version >> HW_VERSION_GENERATION) & 0xF;
-	hw_rev = (hw_version >> HW_VERSION_REVISION) & 0xFFF;
-	hw_step = (hw_version >> HW_VERSION_STEPPING) & 0xFFFF;
-	dev_dbg(csid->camss->dev, "CSID:%d HW Version = %u.%u.%u\n",
-		csid->id, hw_gen, hw_rev, hw_step);
-
-	return hw_version;
-}
-
 /*
  * csid_src_pad_code - Pick an output/src format based on the input/sink format
  * @csid: CSID device
@@ -732,8 +709,6 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
 			pm_runtime_put_sync(dev);
 			return ret;
 		}
-
-		csid->res->hw_ops->hw_version(csid);
 	} else {
 		disable_irq(csid->irq);
 		camss_disable_clocks(csid->nclocks, csid->clock);
diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
index 9dc826d8c8f6..4b003ec1519c 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.h
+++ b/drivers/media/platform/qcom/camss/camss-csid.h
@@ -88,12 +88,6 @@ struct csid_hw_ops {
 	 */
 	int (*configure_testgen_pattern)(struct csid_device *csid, s32 val);
 
-	/*
-	 * hw_version - Read hardware version register from hardware
-	 * @csid: CSID device
-	 */
-	u32 (*hw_version)(struct csid_device *csid);
-
 	/*
 	 * isr - CSID module interrupt service routine
 	 * @irq: Interrupt line
@@ -225,14 +219,6 @@ extern const struct csid_hw_ops csid_ops_780;
  */
 bool csid_is_lite(struct csid_device *csid);
 
-/*
- * csid_hw_version - CSID hardware version query
- * @csid: CSID device
- *
- * Return HW version or error
- */
-u32 csid_hw_version(struct csid_device *csid);
-
 /*
  * csid_src_pad_code - Pick an output/src format based on the input/sink format
  * @csid: CSID device
-- 
2.45.2

[PATCH 3/3] media: qcom: camss: csiphy: Stop spamming logs with version

[Krzysztof Kozlowski]

Camss drivers spam kernel dmesg with 64 useless messages during boot:

  qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
  qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0

All of these messages are the same, so it makes no sense to print same
information 32 times.

The driver does not use read version at all, so if it was needed for any
real debugging purpose it would be provided via debugfs interface.
However even then printing this is pointless, because version of
hardware block is deducible from the compatible.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../qcom/camss/camss-csiphy-2ph-1-0.c         | 10 ---------
 .../qcom/camss/camss-csiphy-3ph-1-0.c         | 22 -------------------
 .../media/platform/qcom/camss/camss-csiphy.c  |  2 --
 3 files changed, 34 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
index 9d67e7fa6366..09d3b21e222f 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
@@ -40,15 +40,6 @@ static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)
 	return lane_mask;
 }
 
-static void csiphy_hw_version_read(struct csiphy_device *csiphy,
-				   struct device *dev)
-{
-	u8 hw_version = readl_relaxed(csiphy->base +
-				      CAMSS_CSI_PHY_HW_VERSION);
-
-	dev_dbg(dev, "CSIPHY HW Version = 0x%02x\n", hw_version);
-}
-
 /*
  * csiphy_reset - Perform software reset on CSIPHY module
  * @csiphy: CSIPHY device
@@ -187,7 +178,6 @@ static int csiphy_init(struct csiphy_device *csiphy)
 
 const struct csiphy_hw_ops csiphy_ops_2ph_1_0 = {
 	.get_lane_mask = csiphy_get_lane_mask,
-	.hw_version_read = csiphy_hw_version_read,
 	.reset = csiphy_reset,
 	.lanes_enable = csiphy_lanes_enable,
 	.lanes_disable = csiphy_lanes_disable,
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
index f732a76de93e..bc605931278b 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
@@ -541,27 +541,6 @@ csiphy_lane_regs lane_regs_x1e80100[] = {
 	{0x0C64, 0x7F, 0x00, CSIPHY_SKEW_CAL},
 };
 
-static void csiphy_hw_version_read(struct csiphy_device *csiphy,
-				   struct device *dev)
-{
-	struct csiphy_device_regs *regs = csiphy->regs;
-	u32 hw_version;
-
-	writel(CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID, csiphy->base +
-	       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
-
-	hw_version = readl_relaxed(csiphy->base +
-				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 12));
-	hw_version |= readl_relaxed(csiphy->base +
-				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 13)) << 8;
-	hw_version |= readl_relaxed(csiphy->base +
-				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 14)) << 16;
-	hw_version |= readl_relaxed(csiphy->base +
-				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 15)) << 24;
-
-	dev_dbg(dev, "CSIPHY 3PH HW Version = 0x%08x\n", hw_version);
-}
-
 /*
  * csiphy_reset - Perform software reset on CSIPHY module
  * @csiphy: CSIPHY device
@@ -858,7 +837,6 @@ static int csiphy_init(struct csiphy_device *csiphy)
 
 const struct csiphy_hw_ops csiphy_ops_3ph_1_0 = {
 	.get_lane_mask = csiphy_get_lane_mask,
-	.hw_version_read = csiphy_hw_version_read,
 	.reset = csiphy_reset,
 	.lanes_enable = csiphy_lanes_enable,
 	.lanes_disable = csiphy_lanes_disable,
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index c622efcc92ff..111c3a52a6d1 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -243,8 +243,6 @@ static int csiphy_set_power(struct v4l2_subdev *sd, int on)
 		enable_irq(csiphy->irq);
 
 		csiphy->res->hw_ops->reset(csiphy);
-
-		csiphy->res->hw_ops->hw_version_read(csiphy, dev);
 	} else {
 		disable_irq(csiphy->irq);
 
-- 
2.45.2

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

On 29/04/2025 20:08, Krzysztof Kozlowski wrote:
> Camss drivers spam kernel dmesg with 64 useless messages during boot:
> 
>   qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
>   qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
> 
> All of these messages are the same, so it makes no sense to print same
> information 32 times.
> 
> The driver does not use read version at all, so if it was needed for any
> real debugging purpose it would be provided via debugfs interface.
> However even then printing this is pointless, because version of
> hardware block is deducible from the compatible.
This is how the dmesg looks after camss:


qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:3 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2
qcom-camss acb7000.isp: VFE:0 HW Version = 3.0.2


Best regards,
Krzysztof

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Bryan O'Donoghue]

On 29/04/2025 19:08, Krzysztof Kozlowski wrote:
> -	dev_dbg(vfe->camss->dev, "VFE:%d HW Version = %u.%u.%u\n",
> -		vfe->id, gen, rev, step);

Please just change to dev_dbg_once() instead of entirely removing.

Same comment with the other patches.

---
bod

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

On 29/04/2025 21:40, Bryan O'Donoghue wrote:
> On 29/04/2025 19:08, Krzysztof Kozlowski wrote:
>> -	dev_dbg(vfe->camss->dev, "VFE:%d HW Version = %u.%u.%u\n",
>> -		vfe->id, gen, rev, step);
> 
> Please just change to dev_dbg_once() instead of entirely removing.

Why?

This is entirely useless message, isn't it? Version is deducible from
the compatible.

> 
> Same comment with the other patches.
> 
> ---
> bod


Best regards,
Krzysztof

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Johan Hovold]

On Tue, Apr 29, 2025 at 08:08:29PM +0200, Krzysztof Kozlowski wrote:
> Camss drivers spam kernel dmesg with 64 useless messages during boot:
> 
>   qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
>   qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
> 
> All of these messages are the same, so it makes no sense to print same
> information 32 times.

It's even worse then that (several hundred messages during use) and I
sent fixes for these regressions a few weeks ago:

	https://lore.kernel.org/lkml/20250407104828.3833-1-johan+linaro@kernel.org/
	https://lore.kernel.org/lkml/20250407085125.21325-1-johan+linaro@kernel.org/

Unfortunately, it seems Bryan missed that this was a regression that
should be fixed in 6.15 and only included them in a pull request for 6.16:

	https://lore.kernel.org/all/20250410233039.77093-1-bod@kernel.org/

Bryan, has your PR been merged? Can you try to get my fixes into 6.15
since this is a regression in 6.15-rc1?

Johan

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

On 30/04/2025 09:25, Johan Hovold wrote:
> On Tue, Apr 29, 2025 at 08:08:29PM +0200, Krzysztof Kozlowski wrote:
>> Camss drivers spam kernel dmesg with 64 useless messages during boot:
>>
>>   qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
>>   qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
>>
>> All of these messages are the same, so it makes no sense to print same
>> information 32 times.
> 
> It's even worse then that (several hundred messages during use) and I
> sent fixes for these regressions a few weeks ago:
> 
> 	https://lore.kernel.org/lkml/20250407104828.3833-1-johan+linaro@kernel.org/
> 	https://lore.kernel.org/lkml/20250407085125.21325-1-johan+linaro@kernel.org/

Oh damn...

I developed this on top of next, so already with your fixes included,
but - following standard kernel coding practice that drivers should be
silent on success - I think even debug messages are not needed here.

There is really no point in printing (even as debug) version of hw block
EVERY TIME I boot the hardware. It does not change, does it?

If anyone wants to know it and cannot deduce from compatible, then add
debugfs interface.


Best regards,
Krzysztof

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Bryan O'Donoghue]

On 30/04/2025 08:25, Johan Hovold wrote:
> Unfortunately, it seems Bryan missed that this was a regression that
> should be fixed in 6.15 and only included them in a pull request for 6.16:
> 
> 	https://lore.kernel.org/all/20250410233039.77093-1-bod@kernel.org/
> 
> Bryan, has your PR been merged? Can you try to get my fixes into 6.15
> since this is a regression in 6.15-rc1?

Let me see.. there's a -fixes branch, I think I should be able to PR 
anything with a Fixes: tag there.

---
bod

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Bryan O'Donoghue]

On 30/04/2025 09:19, Krzysztof Kozlowski wrote:
> If anyone wants to know it and cannot deduce from compatible, then add
> debugfs interface.

dev_dbg(); isn't too offensive really IMO but if it really bothers you 
switching to debugfs would be fine.

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/e62825fc2ed737ab88085567f0947306a2a0da9b

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/ff0d7d980ec8192b459b5926b85a105917746d91

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/3580ffcbe507036c35e8f21e293f018fbb62d8bf

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/cd88d924eb55f5dfeb2283e6e0eef37d5bd4c1c4

---
bod

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

On 30/04/2025 10:30, Bryan O'Donoghue wrote:
> On 30/04/2025 09:19, Krzysztof Kozlowski wrote:
>> If anyone wants to know it and cannot deduce from compatible, then add
>> debugfs interface.
> 
> dev_dbg(); isn't too offensive really IMO but if it really bothers you 
> switching to debugfs would be fine.

Yes, please. Dmesg should be only contain issues or some useful
debugging data. Probe success is not useful. It duplicates sysfs and
tracing. Version of hardware - well, I am sure it duplicates the compatible.

> 
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/e62825fc2ed737ab88085567f0947306a2a0da9b
> 
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/ff0d7d980ec8192b459b5926b85a105917746d91
> 
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/3580ffcbe507036c35e8f21e293f018fbb62d8bf
> 
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/cd88d924eb55f5dfeb2283e6e0eef37d5bd4c1c4
> 
> ---
> bod


Best regards,
Krzysztof

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Johan Hovold]

On Wed, Apr 30, 2025 at 10:19:13AM +0200, Krzysztof Kozlowski wrote:
> On 30/04/2025 09:25, Johan Hovold wrote:
> > On Tue, Apr 29, 2025 at 08:08:29PM +0200, Krzysztof Kozlowski wrote:
> >> Camss drivers spam kernel dmesg with 64 useless messages during boot:
> >>
> >>   qcom-camss acb7000.isp: VFE:1 HW Version = 3.0.2
> >>   qcom-camss acb7000.isp: VFE:2 HW Version = 2.4.0
> >>
> >> All of these messages are the same, so it makes no sense to print same
> >> information 32 times.
> > 
> > It's even worse then that (several hundred messages during use) and I
> > sent fixes for these regressions a few weeks ago:
> > 
> > 	https://lore.kernel.org/lkml/20250407104828.3833-1-johan+linaro@kernel.org/
> > 	https://lore.kernel.org/lkml/20250407085125.21325-1-johan+linaro@kernel.org/
> 
> Oh damn...
> 
> I developed this on top of next, so already with your fixes included,
> but - following standard kernel coding practice that drivers should be
> silent on success - I think even debug messages are not needed here.

Ah, ok, it's based on my fixes and just removing the debug printks.

I think that should be made more clear in the commit message as debug
printks are not enabled by default and the rules for those are less
strict.

> There is really no point in printing (even as debug) version of hw block
> EVERY TIME I boot the hardware. It does not change, does it?

Well, it doesn't hurt to print it once on probe if this is something
read out from the hardware, but clearly printing it hundreds of times
during use make that debug statement pretty useless.

Johan

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Johan Hovold]

On Wed, Apr 30, 2025 at 09:24:12AM +0100, Bryan O'Donoghue wrote:
> On 30/04/2025 08:25, Johan Hovold wrote:
> > Unfortunately, it seems Bryan missed that this was a regression that
> > should be fixed in 6.15 and only included them in a pull request for 6.16:
> > 
> > 	https://lore.kernel.org/all/20250410233039.77093-1-bod@kernel.org/
> > 
> > Bryan, has your PR been merged? Can you try to get my fixes into 6.15
> > since this is a regression in 6.15-rc1?
> 
> Let me see.. there's a -fixes branch, I think I should be able to PR 
> anything with a Fixes: tag there.

I see now that you added CC stable tags to my commits so the fixes
should trickle back to 6.15 eventually, but for issues introduced in
-rc1 you should try to get them fixed in the same development cycle.

Johan

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

On 30/04/2025 10:33, Krzysztof Kozlowski wrote:
> On 30/04/2025 10:30, Bryan O'Donoghue wrote:
>> On 30/04/2025 09:19, Krzysztof Kozlowski wrote:
>>> If anyone wants to know it and cannot deduce from compatible, then add
>>> debugfs interface.
>>
>> dev_dbg(); isn't too offensive really IMO but if it really bothers you 
>> switching to debugfs would be fine.
> 
> Yes, please. Dmesg should be only contain issues or some useful
> debugging data. Probe success is not useful. It duplicates sysfs and
> tracing. Version of hardware - well, I am sure it duplicates the compatible.

To recall: kernel coding style is also clear here:
"When drivers are working properly they are quiet,"
and kernel debugging guide as well:
"In almost all cases the debug statements shouldn't be upstreamed, as a
working driver is supposed to be silent."

So I really do not get why this driver deserved exception. Nevertheless
I think we agreed that these logs can go away, thus I just sent a v2
with a bit extended commit msg.

Best regards,
Krzysztof

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Johan Hovold]

On Tue, May 20, 2025 at 08:06:22AM +0200, Krzysztof Kozlowski wrote:
> On 30/04/2025 10:33, Krzysztof Kozlowski wrote:
> > On 30/04/2025 10:30, Bryan O'Donoghue wrote:
> >> On 30/04/2025 09:19, Krzysztof Kozlowski wrote:
> >>> If anyone wants to know it and cannot deduce from compatible, then add
> >>> debugfs interface.
> >>
> >> dev_dbg(); isn't too offensive really IMO but if it really bothers you 
> >> switching to debugfs would be fine.
> > 
> > Yes, please. Dmesg should be only contain issues or some useful
> > debugging data. Probe success is not useful. It duplicates sysfs and
> > tracing. Version of hardware - well, I am sure it duplicates the compatible.
> 
> To recall: kernel coding style is also clear here:
> "When drivers are working properly they are quiet,"

That's clear and well known (or should be).

> and kernel debugging guide as well:
> "In almost all cases the debug statements shouldn't be upstreamed, as a
> working driver is supposed to be silent."

But this is a very recent addition and questionable when read in
isolation since debug statements are not printed by default. The
preceding sentences do qualify this:

	Permanent debug statements have to be useful for a developer to
	troubleshoot driver misbehavior. Judging that is a bit more of
	an art than a science...

> So I really do not get why this driver deserved exception. Nevertheless
> I think we agreed that these logs can go away, thus I just sent a v2
> with a bit extended commit msg.

Spamming the logs as the driver currently does is clearly broken and
should be fixed. Keeping a hw version dev_dbg() is generally perfectly
fine, though.

Johan

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

On 20/05/2025 09:53, Johan Hovold wrote:
> On Tue, May 20, 2025 at 08:06:22AM +0200, Krzysztof Kozlowski wrote:
>> On 30/04/2025 10:33, Krzysztof Kozlowski wrote:
>>> On 30/04/2025 10:30, Bryan O'Donoghue wrote:
>>>> On 30/04/2025 09:19, Krzysztof Kozlowski wrote:
>>>>> If anyone wants to know it and cannot deduce from compatible, then add
>>>>> debugfs interface.
>>>>
>>>> dev_dbg(); isn't too offensive really IMO but if it really bothers you 
>>>> switching to debugfs would be fine.
>>>
>>> Yes, please. Dmesg should be only contain issues or some useful
>>> debugging data. Probe success is not useful. It duplicates sysfs and
>>> tracing. Version of hardware - well, I am sure it duplicates the compatible.
>>
>> To recall: kernel coding style is also clear here:
>> "When drivers are working properly they are quiet,"
> 
> That's clear and well known (or should be).
> 
>> and kernel debugging guide as well:
>> "In almost all cases the debug statements shouldn't be upstreamed, as a
>> working driver is supposed to be silent."
> 
> But this is a very recent addition and questionable when read in
> isolation since debug statements are not printed by default. The
> preceding sentences do qualify this:
> 
> 	Permanent debug statements have to be useful for a developer to

Keyword here: useful ------------------------^^^^^^^^^^

> 	troubleshoot driver misbehavior. Judging that is a bit more of
> 	an art than a science...
> 
>> So I really do not get why this driver deserved exception. Nevertheless
>> I think we agreed that these logs can go away, thus I just sent a v2
>> with a bit extended commit msg.
> 
> Spamming the logs as the driver currently does is clearly broken and
> should be fixed. Keeping a hw version dev_dbg() is generally perfectly
> fine, though.
My main argument, expressed in the commit msg to which no one objected,
is that this debug is 100% useless: deducible from the compatible,
always known upfront, always the same.

Best regards,
Krzysztof

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Johan Hovold]

On Tue, May 20, 2025 at 10:02:32AM +0200, Krzysztof Kozlowski wrote:
> On 20/05/2025 09:53, Johan Hovold wrote:

> > Spamming the logs as the driver currently does is clearly broken and
> > should be fixed. Keeping a hw version dev_dbg() is generally perfectly
> > fine, though.

> My main argument, expressed in the commit msg to which no one objected,
> is that this debug is 100% useless: deducible from the compatible,
> always known upfront, always the same.

To me that deduction does not seem straightforward, at least not without
access to internal qualcomm docs, for example:

	compatible = "qcom,sc8280xp-camss";

        qcom-camss ac5a000.camss: VFE:0 HW Version = 1.2.2
	qcom-camss ac5a000.camss: VFE:1 HW Version = 1.2.2
        qcom-camss ac5a000.camss: VFE:2 HW Version = 1.2.2
        qcom-camss ac5a000.camss: VFE:3 HW Version = 1.2.2
        qcom-camss ac5a000.camss: VFE:4 HW Version = 1.3.0
        qcom-camss ac5a000.camss: VFE:5 HW Version = 1.3.0
        qcom-camss ac5a000.camss: VFE:6 HW Version = 1.3.0
        qcom-camss ac5a000.camss: VFE:7 HW Version = 1.3.0

Whether the hw version is actually useful to anyone debugging this
driver I can't say, but keeping it printed *once* seems perfectly
alright if someone wants to keep it (e.g. as we have a long history of
working around hw bugs based on revision information like this).

Johan

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

On 20/05/2025 10:23, Johan Hovold wrote:
> On Tue, May 20, 2025 at 10:02:32AM +0200, Krzysztof Kozlowski wrote:
>> On 20/05/2025 09:53, Johan Hovold wrote:
> 
>>> Spamming the logs as the driver currently does is clearly broken and
>>> should be fixed. Keeping a hw version dev_dbg() is generally perfectly
>>> fine, though.
> 
>> My main argument, expressed in the commit msg to which no one objected,
>> is that this debug is 100% useless: deducible from the compatible,
>> always known upfront, always the same.
> 
> To me that deduction does not seem straightforward, at least not without
> access to internal qualcomm docs, for example:
> 
> 	compatible = "qcom,sc8280xp-camss";
> 
>         qcom-camss ac5a000.camss: VFE:0 HW Version = 1.2.2
> 	qcom-camss ac5a000.camss: VFE:1 HW Version = 1.2.2
>         qcom-camss ac5a000.camss: VFE:2 HW Version = 1.2.2
>         qcom-camss ac5a000.camss: VFE:3 HW Version = 1.2.2
>         qcom-camss ac5a000.camss: VFE:4 HW Version = 1.3.0
>         qcom-camss ac5a000.camss: VFE:5 HW Version = 1.3.0
>         qcom-camss ac5a000.camss: VFE:6 HW Version = 1.3.0
>         qcom-camss ac5a000.camss: VFE:7 HW Version = 1.3.0
> 

I understand that deduction is not straightforward, but it is also a
fixed one, meaning it will be always sc8280xp -> (vFOO, vBAR), thus the
only usefulness of above is to map each compatible to pair of two HW
versions. This can be done via debugfs interface once and stored in
public docs. No need to do that mapping every time driver probes and my
patches drop nice chunk of code, including indirect function calls.

At least so far no one objected that same compatible maps to same pairs
of HW versions.

> Whether the hw version is actually useful to anyone debugging this
> driver I can't say, but keeping it printed *once* seems perfectly
> alright if someone wants to keep it (e.g. as we have a long history of
> working around hw bugs based on revision information like this).

Now if you claim that one needs access to qcom docs to deduce it, I
claim this version would be useful only to qcom people (or
qcom-NDA-access-to-HPG) folks.


Best regards,
Krzysztof

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Johan Hovold]

On Tue, May 20, 2025 at 10:30:16AM +0200, Krzysztof Kozlowski wrote:
> On 20/05/2025 10:23, Johan Hovold wrote:
> > On Tue, May 20, 2025 at 10:02:32AM +0200, Krzysztof Kozlowski wrote:
> >> On 20/05/2025 09:53, Johan Hovold wrote:
> > 
> >>> Spamming the logs as the driver currently does is clearly broken and
> >>> should be fixed. Keeping a hw version dev_dbg() is generally perfectly
> >>> fine, though.
> > 
> >> My main argument, expressed in the commit msg to which no one objected,
> >> is that this debug is 100% useless: deducible from the compatible,
> >> always known upfront, always the same.
> > 
> > To me that deduction does not seem straightforward, at least not without
> > access to internal qualcomm docs, for example:
> > 
> > 	compatible = "qcom,sc8280xp-camss";
> > 
> >         qcom-camss ac5a000.camss: VFE:0 HW Version = 1.2.2
> > 	qcom-camss ac5a000.camss: VFE:1 HW Version = 1.2.2
> >         qcom-camss ac5a000.camss: VFE:2 HW Version = 1.2.2
> >         qcom-camss ac5a000.camss: VFE:3 HW Version = 1.2.2
> >         qcom-camss ac5a000.camss: VFE:4 HW Version = 1.3.0
> >         qcom-camss ac5a000.camss: VFE:5 HW Version = 1.3.0
> >         qcom-camss ac5a000.camss: VFE:6 HW Version = 1.3.0
> >         qcom-camss ac5a000.camss: VFE:7 HW Version = 1.3.0
> > 
> 
> I understand that deduction is not straightforward, but it is also a
> fixed one, meaning it will be always sc8280xp -> (vFOO, vBAR), thus the
> only usefulness of above is to map each compatible to pair of two HW
> versions. This can be done via debugfs interface once and stored in
> public docs. No need to do that mapping every time driver probes and my
> patches drop nice chunk of code, including indirect function calls.

If you still want to export it through debugfs at some point, then it
seems there is still some worth in having it.

I'm sure the implementation is terrible, and maybe that's reason enough
to simply drop it in this case. But again, generally it's perfectly fine
to print a hardware revision at debug level (once) during probe.

Johan

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Bryan O'Donoghue]

On 20/05/2025 09:30, Krzysztof Kozlowski wrote:
> On 20/05/2025 10:23, Johan Hovold wrote:
>> On Tue, May 20, 2025 at 10:02:32AM +0200, Krzysztof Kozlowski wrote:
>>> On 20/05/2025 09:53, Johan Hovold wrote:
>>
>>>> Spamming the logs as the driver currently does is clearly broken and
>>>> should be fixed. Keeping a hw version dev_dbg() is generally perfectly
>>>> fine, though.
>>
>>> My main argument, expressed in the commit msg to which no one objected,
>>> is that this debug is 100% useless: deducible from the compatible,
>>> always known upfront, always the same.
>>
>> To me that deduction does not seem straightforward, at least not without
>> access to internal qualcomm docs, for example:
>>
>> 	compatible = "qcom,sc8280xp-camss";
>>
>>          qcom-camss ac5a000.camss: VFE:0 HW Version = 1.2.2
>> 	qcom-camss ac5a000.camss: VFE:1 HW Version = 1.2.2
>>          qcom-camss ac5a000.camss: VFE:2 HW Version = 1.2.2
>>          qcom-camss ac5a000.camss: VFE:3 HW Version = 1.2.2
>>          qcom-camss ac5a000.camss: VFE:4 HW Version = 1.3.0
>>          qcom-camss ac5a000.camss: VFE:5 HW Version = 1.3.0
>>          qcom-camss ac5a000.camss: VFE:6 HW Version = 1.3.0
>>          qcom-camss ac5a000.camss: VFE:7 HW Version = 1.3.0
>>
> 
> I understand that deduction is not straightforward, but it is also a
> fixed one, meaning it will be always sc8280xp -> (vFOO, vBAR), thus the
> only usefulness of above is to map each compatible to pair of two HW
> versions. This can be done via debugfs interface once and stored in
> public docs. No need to do that mapping every time driver probes and my
> patches drop nice chunk of code, including indirect function calls.
> 
> At least so far no one objected that same compatible maps to same pairs
> of HW versions.
> 
>> Whether the hw version is actually useful to anyone debugging this
>> driver I can't say, but keeping it printed *once* seems perfectly
>> alright if someone wants to keep it (e.g. as we have a long history of
>> working around hw bugs based on revision information like this).
> 
> Now if you claim that one needs access to qcom docs to deduce it, I
> claim this version would be useful only to qcom people (or
> qcom-NDA-access-to-HPG) folks.
> 
> 
> Best regards,
> Krzysztof

I find the debug prints useful in that I know the hardware block has 
been powered on, clocked etc. I agree the number of those prints seems 
excessive.

The reason it is printed out multiple time is the blocks get powered on/off.

Personally I agree with Johan - it would be nice/useful to print it out 
once with DEBUG on, so that we know we have successfully powered-on and 
identified the blocks once.

Doing it over and over again is excessive as failure to power-on will 
surely produce error messages anyway.

---
bod

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

On 20/05/2025 10:44, Bryan O'Donoghue wrote:
> On 20/05/2025 09:30, Krzysztof Kozlowski wrote:
>> On 20/05/2025 10:23, Johan Hovold wrote:
>>> On Tue, May 20, 2025 at 10:02:32AM +0200, Krzysztof Kozlowski wrote:
>>>> On 20/05/2025 09:53, Johan Hovold wrote:
>>>
>>>>> Spamming the logs as the driver currently does is clearly broken and
>>>>> should be fixed. Keeping a hw version dev_dbg() is generally perfectly
>>>>> fine, though.
>>>
>>>> My main argument, expressed in the commit msg to which no one objected,
>>>> is that this debug is 100% useless: deducible from the compatible,
>>>> always known upfront, always the same.
>>>
>>> To me that deduction does not seem straightforward, at least not without
>>> access to internal qualcomm docs, for example:
>>>
>>> 	compatible = "qcom,sc8280xp-camss";
>>>
>>>          qcom-camss ac5a000.camss: VFE:0 HW Version = 1.2.2
>>> 	qcom-camss ac5a000.camss: VFE:1 HW Version = 1.2.2
>>>          qcom-camss ac5a000.camss: VFE:2 HW Version = 1.2.2
>>>          qcom-camss ac5a000.camss: VFE:3 HW Version = 1.2.2
>>>          qcom-camss ac5a000.camss: VFE:4 HW Version = 1.3.0
>>>          qcom-camss ac5a000.camss: VFE:5 HW Version = 1.3.0
>>>          qcom-camss ac5a000.camss: VFE:6 HW Version = 1.3.0
>>>          qcom-camss ac5a000.camss: VFE:7 HW Version = 1.3.0
>>>
>>
>> I understand that deduction is not straightforward, but it is also a
>> fixed one, meaning it will be always sc8280xp -> (vFOO, vBAR), thus the
>> only usefulness of above is to map each compatible to pair of two HW
>> versions. This can be done via debugfs interface once and stored in
>> public docs. No need to do that mapping every time driver probes and my
>> patches drop nice chunk of code, including indirect function calls.
>>
>> At least so far no one objected that same compatible maps to same pairs
>> of HW versions.
>>
>>> Whether the hw version is actually useful to anyone debugging this
>>> driver I can't say, but keeping it printed *once* seems perfectly
>>> alright if someone wants to keep it (e.g. as we have a long history of
>>> working around hw bugs based on revision information like this).
>>
>> Now if you claim that one needs access to qcom docs to deduce it, I
>> claim this version would be useful only to qcom people (or
>> qcom-NDA-access-to-HPG) folks.
>>
>>
>> Best regards,
>> Krzysztof
> 
> I find the debug prints useful in that I know the hardware block has 
> been powered on, clocked etc. I agree the number of those prints seems 
> excessive.
> 
> The reason it is printed out multiple time is the blocks get powered on/off.
> 
> Personally I agree with Johan - it would be nice/useful to print it out 
> once with DEBUG on, so that we know we have successfully powered-on and 

That's opposite to what coding style asks. Success should be silent.

> identified the blocks once.

Last time you suggested to print it once, so this is contradictory. If
you need simple probe success (or component bind) confirmation, then
fortunately we already have infrastructure for that: sysfs and tracing.

This log should either say something useful or not be there at all.
Printing same version 4 times is not useful at all, considering all my
previous arguments that this maps to compatible 1-to-1.

> 
> Doing it over and over again is excessive as failure to power-on will 
> surely produce error messages anyway.


Best regards,
Krzysztof

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Bryan O'Donoghue]

On 20/05/2025 09:51, Krzysztof Kozlowski wrote:
>           qcom-camss ac5a000.camss: VFE:0 HW Version = 1.2.2
>           qcom-camss ac5a000.camss: VFE:1 HW Version = 1.2.2
>           qcom-camss ac5a000.camss: VFE:2 HW Version = 1.2.2
>           qcom-camss ac5a000.camss: VFE:3 HW Version = 1.2.2
>           qcom-camss ac5a000.camss: VFE:4 HW Version = 1.3.0
>           qcom-camss ac5a000.camss: VFE:5 HW Version = 1.3.0
>           qcom-camss ac5a000.camss: VFE:6 HW Version = 1.3.0
>           qcom-camss ac5a000.camss: VFE:7 HW Version = 1.3.0

This prints the hardware version of eight distinct hardware blocks VFE 
index increases.

TBH I still find this useful when debugging hardware.

My personal preference is to print it once on boot and skip subsequent. 
Which I think is perfectly reasonable for DEBUG scenario.

---
bod

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Krzysztof Kozlowski]

On 20/05/2025 10:58, Bryan O'Donoghue wrote:
> On 20/05/2025 09:51, Krzysztof Kozlowski wrote:
>>           qcom-camss ac5a000.camss: VFE:0 HW Version = 1.2.2
>>           qcom-camss ac5a000.camss: VFE:1 HW Version = 1.2.2
>>           qcom-camss ac5a000.camss: VFE:2 HW Version = 1.2.2
>>           qcom-camss ac5a000.camss: VFE:3 HW Version = 1.2.2
>>           qcom-camss ac5a000.camss: VFE:4 HW Version = 1.3.0
>>           qcom-camss ac5a000.camss: VFE:5 HW Version = 1.3.0
>>           qcom-camss ac5a000.camss: VFE:6 HW Version = 1.3.0
>>           qcom-camss ac5a000.camss: VFE:7 HW Version = 1.3.0
> 
> This prints the hardware version of eight distinct hardware blocks VFE 
> index increases.
> 
> TBH I still find this useful when debugging hardware.

How? Can you respond to actual arguments repeated 5 times that this is
fixed and always known?

If this is always known, in what way this is useful?

> 
> My personal preference is to print it once on boot and skip subsequent. 
> Which I think is perfectly reasonable for DEBUG scenario.
What are you responding to? Before you said you find it useful for
knowing each block power up and down?



Best regards,
Krzysztof

Re: [PATCH 1/3] media: qcom: camss: vfe: Stop spamming logs with version

[Johan Hovold]

On Tue, May 20, 2025 at 10:51:56AM +0200, Krzysztof Kozlowski wrote:
> On 20/05/2025 10:44, Bryan O'Donoghue wrote:

> > I find the debug prints useful in that I know the hardware block has 
> > been powered on, clocked etc. I agree the number of those prints seems 
> > excessive.
> > 
> > The reason it is printed out multiple time is the blocks get powered on/off.
> > 
> > Personally I agree with Johan - it would be nice/useful to print it out 
> > once with DEBUG on, so that we know we have successfully powered-on and 
> 
> That's opposite to what coding style asks. Success should be silent.

The coding style says probe should be silent, but debug statements are
not printed by default so that bit does not apply here.

Johan