Skip to contentSkip to conversation

media: imx7-media-csi: Use functions instead of macros

PATCH In review Fabio Estevam 2 participants 4 messages Jun 1, 2019 – Jun 3, 2019

Subsystems (4)
Media (V4L/DVB) Arm i.MX and Layerscape Staging Media drivers for Freescale i.MX
lore
Share & tools
lore

Fabio Estevam

author reviewer
#0 lore raw

PATCH 1/3 media: imx7-media-csi: Use functions instead of macros

Currently there is a macro for reading and another macro for writing to the CSI registers.

Functions can do parameter type checking, which leads to a safer code, so switch from macro to function implementation.

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/staging/media/imx/imx7-media-csi.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/staging/media/imx/imx7-media-csi.c +10 -4
index b1af8694899e..8abdf253696d 100644
@@ -195,10 +195,16 @@ struct imx7_csi {
 	struct completion last_eof_completion;
 };
 
-#define imx7_csi_reg_read(_csi, _offset) \
-	__raw_readl((_csi)->regbase + (_offset))
-#define imx7_csi_reg_write(_csi, _val, _offset) \
-	__raw_writel(_val, (_csi)->regbase + (_offset))
+static u32 imx7_csi_reg_read(struct imx7_csi *csi, unsigned int offset)
+{
+	return readl(csi->regbase + offset);
+}
+
+static void imx7_csi_reg_write(struct imx7_csi *csi, unsigned int value,
+			       unsigned int offset)
+{
+	writel(value, csi->regbase + offset);
+}
 
 static void imx7_csi_hw_reset(struct imx7_csi *csi)
 {
Signature · 1 line
2.17.1
author reviewer
#1 lore raw

PATCH 2/3 media: imx7-media-csi: Use u32 for storing register reads

The CSI registers are 32-bit, so using u32 type is more suitable for storing the values from register reads.

Switch from 'unsigned long' to 'u32' type.

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/staging/media/imx/imx7-media-csi.c | 26 +++++++++++-----------
 1 file changed, 13 insertions(+), 13 deletions(-)
drivers/staging/media/imx/imx7-media-csi.c +13 -13
index 8abdf253696d..7abea8681062 100644
@@ -217,9 +217,9 @@ static void imx7_csi_hw_reset(struct imx7_csi *csi)
 	imx7_csi_reg_write(csi, CSICR3_RESET_VAL, CSI_CSICR3);
 }
 
-static unsigned long imx7_csi_irq_clear(struct imx7_csi *csi)
+static u32 imx7_csi_irq_clear(struct imx7_csi *csi)
 {
-	unsigned long isr;
+	u32 isr;
 
 	isr = imx7_csi_reg_read(csi, CSI_CSISR);
 	imx7_csi_reg_write(csi, isr, CSI_CSISR);

@@ -245,7 +245,7 @@ static void imx7_csi_init_interface(struct imx7_csi *csi)
 
 static void imx7_csi_hw_enable_irq(struct imx7_csi *csi)
 {
-	unsigned long cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
+	u32 cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
 
 	cr1 |= BIT_SOF_INTEN;
 	cr1 |= BIT_RFF_OR_INT;

@@ -261,7 +261,7 @@ static void imx7_csi_hw_enable_irq(struct imx7_csi *csi)
 
 static void imx7_csi_hw_disable_irq(struct imx7_csi *csi)
 {
-	unsigned long cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
+	u32 cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
 
 	cr1 &= ~BIT_SOF_INTEN;
 	cr1 &= ~BIT_RFF_OR_INT;

@@ -274,7 +274,7 @@ static void imx7_csi_hw_disable_irq(struct imx7_csi *csi)
 
 static void imx7_csi_hw_enable(struct imx7_csi *csi)
 {
-	unsigned long cr = imx7_csi_reg_read(csi, CSI_CSICR18);
+	u32 cr = imx7_csi_reg_read(csi, CSI_CSICR18);
 
 	cr |= BIT_CSI_HW_ENABLE;
 

@@ -283,7 +283,7 @@ static void imx7_csi_hw_enable(struct imx7_csi *csi)
 
 static void imx7_csi_hw_disable(struct imx7_csi *csi)
 {
-	unsigned long cr = imx7_csi_reg_read(csi, CSI_CSICR18);
+	u32 cr = imx7_csi_reg_read(csi, CSI_CSICR18);
 
 	cr &= ~BIT_CSI_HW_ENABLE;
 

@@ -292,7 +292,7 @@ static void imx7_csi_hw_disable(struct imx7_csi *csi)
 
 static void imx7_csi_dma_reflash(struct imx7_csi *csi)
 {
-	unsigned long cr3 = imx7_csi_reg_read(csi, CSI_CSICR18);
+	u32 cr3 = imx7_csi_reg_read(csi, CSI_CSICR18);
 
 	cr3 = imx7_csi_reg_read(csi, CSI_CSICR3);
 	cr3 |= BIT_DMA_REFLASH_RFF;

@@ -301,7 +301,7 @@ static void imx7_csi_dma_reflash(struct imx7_csi *csi)
 
 static void imx7_csi_rx_fifo_clear(struct imx7_csi *csi)
 {
-	unsigned long cr1;
+	u32 cr1;
 
 	cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
 	imx7_csi_reg_write(csi, cr1 & ~BIT_FCC, CSI_CSICR1);

@@ -319,7 +319,7 @@ static void imx7_csi_buf_stride_set(struct imx7_csi *csi, u32 stride)
 
 static void imx7_csi_deinterlace_enable(struct imx7_csi *csi, bool enable)
 {
-	unsigned long cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
+	u32 cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
 
 	if (enable)
 		cr18 |= BIT_DEINTERLACE_EN;

@@ -331,8 +331,8 @@ static void imx7_csi_deinterlace_enable(struct imx7_csi *csi, bool enable)
 
 static void imx7_csi_dmareq_rff_enable(struct imx7_csi *csi)
 {
-	unsigned long cr3 = imx7_csi_reg_read(csi, CSI_CSICR3);
-	unsigned long cr2 = imx7_csi_reg_read(csi, CSI_CSICR2);
+	u32 cr3 = imx7_csi_reg_read(csi, CSI_CSICR3);
+	u32 cr2 = imx7_csi_reg_read(csi, CSI_CSICR2);
 
 	/* Burst Type of DMA Transfer from RxFIFO. INCR16 */
 	cr2 |= 0xC0000000;

@@ -348,7 +348,7 @@ static void imx7_csi_dmareq_rff_enable(struct imx7_csi *csi)
 
 static void imx7_csi_dmareq_rff_disable(struct imx7_csi *csi)
 {
-	unsigned long cr3 = imx7_csi_reg_read(csi, CSI_CSICR3);
+	u32 cr3 = imx7_csi_reg_read(csi, CSI_CSICR3);
 
 	cr3 &= ~BIT_DMA_REQ_EN_RFF;
 	cr3 &= ~BIT_HRESP_ERR_EN;

@@ -647,7 +647,7 @@ static void imx7_csi_vb2_buf_done(struct imx7_csi *csi)
 static irqreturn_t imx7_csi_irq_handler(int irq, void *data)
 {
 	struct imx7_csi *csi =  data;
-	unsigned long status;
+	u32 status;
 
 	spin_lock(&csi->irqlock);
Signature · 1 line
2.17.1
author reviewer
#2 lore raw

PATCH 3/3 media: imx7-media-csi: Remove unneeded error message

In reply to Fabio Estevam

In case of ioremap failure, the core code will take care of printing the error message, so there is no need for having a local error message in the driver.

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/staging/media/imx/imx7-media-csi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/staging/media/imx/imx7-media-csi.c +1 -3
index 7abea8681062..fead72952aba 100644
@@ -1194,10 +1194,8 @@ static int imx7_csi_probe(struct platform_device *pdev)
 	}
 
 	csi->regbase = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(csi->regbase)) {
-		dev_err(dev, "Failed platform resources map\n");
+	if (IS_ERR(csi->regbase))
 		return PTR_ERR(csi->regbase);
-	}
 
 	spin_lock_init(&csi->irqlock);
 	mutex_init(&csi->lock);
Signature · 1 line
2.17.1
Reviewed-by on v1 1/3
#3 lore raw
In reply to Fabio Estevam

Oi Fabio,

Quoted 8 lines · Fabio Estevam

Currently there is a macro for reading and another macro for writing to the CSI registers.

Functions can do parameter type checking, which leads to a safer code, so switch from macro to function implementation.

Signed-off-by Fabio Estevam <[email protected]>

Thanks for the patches. for all of them:

Reviewed-by Rui Miguel Silva <[email protected]>

---
Cheers,
Rui

Quoted 29 lines
---
 drivers/staging/media/imx/imx7-media-csi.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/staging/media/imx/imx7-media-csi.c +10 -4
index b1af8694899e..8abdf253696d 100644
@@ -195,10 +195,16 @@ struct imx7_csi {
 	struct completion last_eof_completion;
 };
 
-#define imx7_csi_reg_read(_csi, _offset) \
-	__raw_readl((_csi)->regbase + (_offset))
-#define imx7_csi_reg_write(_csi, _val, _offset) \
-	__raw_writel(_val, (_csi)->regbase + (_offset))
+static u32 imx7_csi_reg_read(struct imx7_csi *csi, unsigned int offset)
+{
+	return readl(csi->regbase + offset);
+}
+
+static void imx7_csi_reg_write(struct imx7_csi *csi, unsigned int value,
+			       unsigned int offset)
+{
+	writel(value, csi->regbase + offset);
+}
 
 static void imx7_csi_hw_reset(struct imx7_csi *csi)
 {