[PATCH 00/10] Few improvements on the Broadcom iProc

[PATCH 00/10] Few improvements on the Broadcom iProc

[Andi Shyti]

Hi,

just some little improvements on the Broadcom iProc I2C Driver.

Thanks,
Andi

Andi Shyti (10):
  i2c: iproc: Drop unnecessary initialisation of 'ret'
  i2c: iproc: Use dev_err_probe in probe
  i2c: iproc: Use u32 instead of uint32_t
  i2c: iproc: Fix alignment to match the open parenthesis
  i2c: iproc: Remove stray blank line in slave ISR
  i2c: iproc: Replace udelay() with usleep_range()
  i2c: iproc: Fix indentation of bcm_iproc_i2c_slave_init()
  i2c: iproc: Move function and avoid prototypes
  i2c: iproc: When there's an error treat it as an error
  i2c: iproc: Remove unnecessary double negation

 drivers/i2c/busses/i2c-bcm-iproc.c | 222 ++++++++++++++---------------
 1 file changed, 104 insertions(+), 118 deletions(-)

-- 
2.49.0

[PATCH 01/10] i2c: iproc: Drop unnecessary initialisation of 'ret'

[Andi Shyti]

The 'ret' variable doesn't need to be initialised, as it is
always assigned before use.

While here, reorder the variable declarations in reverse
Christmas tree style, by line length.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 332a0fcca28d..5d846ab91e6f 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -1039,9 +1039,9 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 
 static int bcm_iproc_i2c_probe(struct platform_device *pdev)
 {
-	int irq, ret = 0;
 	struct bcm_iproc_i2c_dev *iproc_i2c;
 	struct i2c_adapter *adap;
+	int irq, ret;
 
 	iproc_i2c = devm_kzalloc(&pdev->dev, sizeof(*iproc_i2c),
 				 GFP_KERNEL);
-- 
2.49.0

[PATCH 02/10] i2c: iproc: Use dev_err_probe in probe

[Andi Shyti]

Use dev_err_probe() instead of dev_err() and then return.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 31 ++++++++++++------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 5d846ab91e6f..2e24959bc9af 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -1014,17 +1014,14 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 		bus_speed = I2C_MAX_STANDARD_MODE_FREQ;
 	}
 
-	if (bus_speed < I2C_MAX_STANDARD_MODE_FREQ) {
-		dev_err(iproc_i2c->device, "%d Hz bus speed not supported\n",
-			bus_speed);
-		dev_err(iproc_i2c->device,
-			"valid speeds are 100khz and 400khz\n");
-		return -EINVAL;
-	} else if (bus_speed < I2C_MAX_FAST_MODE_FREQ) {
+	if (bus_speed < I2C_MAX_STANDARD_MODE_FREQ)
+		return dev_err_probe(iproc_i2c->device, -EINVAL,
+				     "%d Hz not supported (out of 100-400 kHz range)\n",
+				     bus_speed);
+	else if (bus_speed < I2C_MAX_FAST_MODE_FREQ)
 		bus_speed = I2C_MAX_STANDARD_MODE_FREQ;
-	} else {
+	else
 		bus_speed = I2C_MAX_FAST_MODE_FREQ;
-	}
 
 	iproc_i2c->bus_speed = bus_speed;
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
@@ -1066,11 +1063,9 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
 		ret = of_property_read_u32(iproc_i2c->device->of_node,
 					   "brcm,ape-hsls-addr-mask",
 					   &iproc_i2c->ape_addr_mask);
-		if (ret < 0) {
-			dev_err(iproc_i2c->device,
-				"'brcm,ape-hsls-addr-mask' missing\n");
-			return -EINVAL;
-		}
+		if (ret < 0)
+			return dev_err_probe(iproc_i2c->device, ret,
+					     "'brcm,ape-hsls-addr-mask' missing\n");
 
 		spin_lock_init(&iproc_i2c->idm_lock);
 
@@ -1090,11 +1085,9 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
 		ret = devm_request_irq(iproc_i2c->device, irq,
 				       bcm_iproc_i2c_isr, 0, pdev->name,
 				       iproc_i2c);
-		if (ret < 0) {
-			dev_err(iproc_i2c->device,
-				"unable to request irq %i\n", irq);
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(iproc_i2c->device, ret,
+					     "unable to request irq %i\n", irq);
 
 		iproc_i2c->irq = irq;
 	} else {
-- 
2.49.0

[PATCH 03/10] i2c: iproc: Use u32 instead of uint32_t

[Andi Shyti]

In the kernel u32 should be used instead of unit32_t.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 2e24959bc9af..3816ceb713d9 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -542,7 +542,7 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 static void bcm_iproc_i2c_read_valid_bytes(struct bcm_iproc_i2c_dev *iproc_i2c)
 {
 	struct i2c_msg *msg = iproc_i2c->msg;
-	uint32_t val;
+	u32 val;
 
 	/* Read valid data from RX FIFO */
 	while (iproc_i2c->rx_bytes < msg->len) {
@@ -977,7 +977,7 @@ static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
 	return num;
 }
 
-static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
+static u32 bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
 {
 	u32 val;
 
-- 
2.49.0

[PATCH 04/10] i2c: iproc: Fix alignment to match the open parenthesis

[Andi Shyti]

Alignment should match the open parenthesis but in some places it
didn't

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 3816ceb713d9..3e76120c23ab 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -833,7 +833,7 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
  * The i2c quirks are set to enforce this rule.
  */
 static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,
-					struct i2c_msg *msgs, bool process_call)
+				       struct i2c_msg *msgs, bool process_call)
 {
 	int i;
 	u8 addr;
@@ -1010,7 +1010,7 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 				       "clock-frequency", &bus_speed);
 	if (ret < 0) {
 		dev_info(iproc_i2c->device,
-			"unable to interpret clock-frequency DT property\n");
+			 "unable to interpret clock-frequency DT property\n");
 		bus_speed = I2C_MAX_STANDARD_MODE_FREQ;
 	}
 
@@ -1099,9 +1099,8 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
 
 	adap = &iproc_i2c->adapter;
 	i2c_set_adapdata(adap, iproc_i2c);
-	snprintf(adap->name, sizeof(adap->name),
-		"Broadcom iProc (%s)",
-		of_node_full_name(iproc_i2c->device->of_node));
+	snprintf(adap->name, sizeof(adap->name), "Broadcom iProc (%s)",
+		 of_node_full_name(iproc_i2c->device->of_node));
 	adap->algo = &bcm_iproc_algo;
 	adap->quirks = &bcm_iproc_i2c_quirks;
 	adap->dev.parent = &pdev->dev;
-- 
2.49.0

[PATCH 05/10] i2c: iproc: Remove stray blank line in slave ISR

[Andi Shyti]

Drop an unnecessary blank line in bcm_iproc_i2c_slave_isr().

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 3e76120c23ab..44083698121a 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -438,7 +438,6 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 	u32 val;
 	u8 value;
 
-
 	if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
 		iproc_i2c->tx_underrun++;
 		if (iproc_i2c->tx_underrun == 1)
-- 
2.49.0

[PATCH 06/10] i2c: iproc: Replace udelay() with usleep_range()

[Andi Shyti]

Replace udelay(100) with usleep_range(100, 200) as recommended
by kernel documentation. The delay is not in atomic context, so
busy-waiting is unnecessary.

Also update the comment for clarity.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 44083698121a..2e04ea157e8e 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -276,8 +276,8 @@ static void bcm_iproc_i2c_slave_init(
 		val |= BIT(CFG_RESET_SHIFT);
 		iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
 
-		/* wait 100 usec per spec */
-		udelay(100);
+		/* wait approximately 100 usec as per spec */
+		usleep_range(100, 200);
 
 		/* bring controller out of reset */
 		val &= ~(BIT(CFG_RESET_SHIFT));
@@ -687,8 +687,8 @@ static void bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
 	val &= ~(BIT(CFG_EN_SHIFT));
 	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
 
-	/* wait 100 usec per spec */
-	udelay(100);
+	/* wait approximately 100 usec as per spec */
+	usleep_range(100, 200);
 
 	/* bring controller out of reset */
 	val &= ~(BIT(CFG_RESET_SHIFT));
-- 
2.49.0

[PATCH 07/10] i2c: iproc: Fix indentation of bcm_iproc_i2c_slave_init()

[Andi Shyti]

Adjust the indentation of the bcm_iproc_i2c_slave_init() function
definition to match standard kernel coding style. Don't end the
line with an open parenthesis.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 2e04ea157e8e..d25b393f456b 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -264,8 +264,8 @@ static inline void iproc_i2c_wr_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	}
 }
 
-static void bcm_iproc_i2c_slave_init(
-	struct bcm_iproc_i2c_dev *iproc_i2c, bool need_reset)
+static void bcm_iproc_i2c_slave_init(struct bcm_iproc_i2c_dev *iproc_i2c,
+				     bool need_reset)
 {
 	u32 val;
 
-- 
2.49.0

[PATCH 08/10] i2c: iproc: Move function and avoid prototypes

[Andi Shyti]

Shuffle a bit the code in order to avoid prototypes.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 143 ++++++++++++++---------------
 1 file changed, 69 insertions(+), 74 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index d25b393f456b..74903edb4925 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -224,11 +224,6 @@ static void slave_rx_tasklet_fn(unsigned long);
 		| BIT(IS_S_TX_UNDERRUN_SHIFT) | BIT(IS_S_RX_FIFO_FULL_SHIFT)\
 		| BIT(IS_S_RX_THLD_SHIFT))
 
-static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave);
-static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave);
-static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
-					 bool enable);
-
 static inline u32 iproc_i2c_rd_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
 				   u32 offset)
 {
@@ -316,6 +311,19 @@ static void bcm_iproc_i2c_slave_init(struct bcm_iproc_i2c_dev *iproc_i2c,
 	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
 }
 
+static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
+					 bool enable)
+{
+	u32 val;
+
+	val = iproc_i2c_rd_reg(iproc_i2c, CFG_OFFSET);
+	if (enable)
+		val |= BIT(CFG_EN_SHIFT);
+	else
+		val &= ~BIT(CFG_EN_SHIFT);
+	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
+}
+
 static bool bcm_iproc_i2c_check_slave_status
 	(struct bcm_iproc_i2c_dev *iproc_i2c, u32 status)
 {
@@ -707,19 +715,6 @@ static void bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
 	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, 0xffffffff);
 }
 
-static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
-					 bool enable)
-{
-	u32 val;
-
-	val = iproc_i2c_rd_reg(iproc_i2c, CFG_OFFSET);
-	if (enable)
-		val |= BIT(CFG_EN_SHIFT);
-	else
-		val &= ~BIT(CFG_EN_SHIFT);
-	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
-}
-
 static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
 				      struct i2c_msg *msg)
 {
@@ -988,6 +983,62 @@ static u32 bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
 	return val;
 }
 
+static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
+
+	if (iproc_i2c->slave)
+		return -EBUSY;
+
+	if (slave->flags & I2C_CLIENT_TEN)
+		return -EAFNOSUPPORT;
+
+	iproc_i2c->slave = slave;
+
+	tasklet_init(&iproc_i2c->slave_rx_tasklet, slave_rx_tasklet_fn,
+		     (unsigned long)iproc_i2c);
+
+	bcm_iproc_i2c_slave_init(iproc_i2c, false);
+	return 0;
+}
+
+static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
+{
+	u32 tmp;
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
+
+	if (!iproc_i2c->slave)
+		return -EINVAL;
+
+	disable_irq(iproc_i2c->irq);
+
+	tasklet_kill(&iproc_i2c->slave_rx_tasklet);
+
+	/* disable all slave interrupts */
+	tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
+	tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
+			IE_S_ALL_INTERRUPT_SHIFT);
+	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
+
+	/* Erase the slave address programmed */
+	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
+	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
+	iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);
+
+	/* flush TX/RX FIFOs */
+	tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
+	iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);
+
+	/* clear all pending slave interrupts */
+	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);
+
+	iproc_i2c->slave = NULL;
+
+	enable_irq(iproc_i2c->irq);
+
+	return 0;
+}
+
 static struct i2c_algorithm bcm_iproc_algo = {
 	.master_xfer = bcm_iproc_i2c_xfer,
 	.functionality = bcm_iproc_i2c_functionality,
@@ -1173,62 +1224,6 @@ static const struct dev_pm_ops bcm_iproc_i2c_pm_ops = {
 	.resume_early = &bcm_iproc_i2c_resume
 };
 
-static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
-{
-	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
-
-	if (iproc_i2c->slave)
-		return -EBUSY;
-
-	if (slave->flags & I2C_CLIENT_TEN)
-		return -EAFNOSUPPORT;
-
-	iproc_i2c->slave = slave;
-
-	tasklet_init(&iproc_i2c->slave_rx_tasklet, slave_rx_tasklet_fn,
-		     (unsigned long)iproc_i2c);
-
-	bcm_iproc_i2c_slave_init(iproc_i2c, false);
-	return 0;
-}
-
-static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
-{
-	u32 tmp;
-	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
-
-	if (!iproc_i2c->slave)
-		return -EINVAL;
-
-	disable_irq(iproc_i2c->irq);
-
-	tasklet_kill(&iproc_i2c->slave_rx_tasklet);
-
-	/* disable all slave interrupts */
-	tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
-	tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
-			IE_S_ALL_INTERRUPT_SHIFT);
-	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
-
-	/* Erase the slave address programmed */
-	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
-	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
-	iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);
-
-	/* flush TX/RX FIFOs */
-	tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
-	iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);
-
-	/* clear all pending slave interrupts */
-	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);
-
-	iproc_i2c->slave = NULL;
-
-	enable_irq(iproc_i2c->irq);
-
-	return 0;
-}
-
 static const struct of_device_id bcm_iproc_i2c_of_match[] = {
 	{
 		.compatible = "brcm,iproc-i2c",
-- 
2.49.0

Re: [PATCH 08/10] i2c: iproc: Move function and avoid prototypes

[Mukesh Kumar Savaliya]

On 4/19/2025 2:46 AM, Andi Shyti wrote:
> Shuffle a bit the code in order to avoid prototypes.
> 
> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> ---
>   drivers/i2c/busses/i2c-bcm-iproc.c | 143 ++++++++++++++---------------
>   1 file changed, 69 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index d25b393f456b..74903edb4925 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -224,11 +224,6 @@ static void slave_rx_tasklet_fn(unsigned long);
[...]
> +static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
> +{
> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
> +
> +	if (iproc_i2c->slave)
> +		return -EBUSY;
> +
> +	if (slave->flags & I2C_CLIENT_TEN)
> +		return -EAFNOSUPPORT;
> +
> +	iproc_i2c->slave = slave;
> +
> +	tasklet_init(&iproc_i2c->slave_rx_tasklet, slave_rx_tasklet_fn,
> +		     (unsigned long)iproc_i2c);
> +
> +	bcm_iproc_i2c_slave_init(iproc_i2c, false);
one line gap/space before return.
> +	return 0;
> +}
> +
> +static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
> +{
> +	u32 tmp;
> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
Reverse xmas tree alignment during decalarations please ?
> +
> +	if (!iproc_i2c->slave)
> +		return -EINVAL;
> +
[...]

[PATCH v2 08/10] i2c: iproc: Move function and avoid prototypes

[Andi Shyti]

Shuffle a bit the code in order to avoid prototypes.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
Hi,

as I am moving the functions, I can take the chance to fix the
two style issues that Mukesh has pointed out:

 - Leave a blank line before the return.
 - Arrange variable declaration in a reverse xmas tree alignment.

Thanks Mukesh,
Andi

 drivers/i2c/busses/i2c-bcm-iproc.c | 144 ++++++++++++++---------------
 1 file changed, 70 insertions(+), 74 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index d25b393f456b..4553225d7cdd 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -224,11 +224,6 @@ static void slave_rx_tasklet_fn(unsigned long);
 		| BIT(IS_S_TX_UNDERRUN_SHIFT) | BIT(IS_S_RX_FIFO_FULL_SHIFT)\
 		| BIT(IS_S_RX_THLD_SHIFT))
 
-static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave);
-static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave);
-static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
-					 bool enable);
-
 static inline u32 iproc_i2c_rd_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
 				   u32 offset)
 {
@@ -316,6 +311,19 @@ static void bcm_iproc_i2c_slave_init(struct bcm_iproc_i2c_dev *iproc_i2c,
 	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
 }
 
+static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
+					 bool enable)
+{
+	u32 val;
+
+	val = iproc_i2c_rd_reg(iproc_i2c, CFG_OFFSET);
+	if (enable)
+		val |= BIT(CFG_EN_SHIFT);
+	else
+		val &= ~BIT(CFG_EN_SHIFT);
+	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
+}
+
 static bool bcm_iproc_i2c_check_slave_status
 	(struct bcm_iproc_i2c_dev *iproc_i2c, u32 status)
 {
@@ -707,19 +715,6 @@ static void bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
 	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, 0xffffffff);
 }
 
-static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
-					 bool enable)
-{
-	u32 val;
-
-	val = iproc_i2c_rd_reg(iproc_i2c, CFG_OFFSET);
-	if (enable)
-		val |= BIT(CFG_EN_SHIFT);
-	else
-		val &= ~BIT(CFG_EN_SHIFT);
-	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
-}
-
 static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
 				      struct i2c_msg *msg)
 {
@@ -988,6 +983,63 @@ static u32 bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
 	return val;
 }
 
+static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
+
+	if (iproc_i2c->slave)
+		return -EBUSY;
+
+	if (slave->flags & I2C_CLIENT_TEN)
+		return -EAFNOSUPPORT;
+
+	iproc_i2c->slave = slave;
+
+	tasklet_init(&iproc_i2c->slave_rx_tasklet, slave_rx_tasklet_fn,
+		     (unsigned long)iproc_i2c);
+
+	bcm_iproc_i2c_slave_init(iproc_i2c, false);
+
+	return 0;
+}
+
+static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
+	u32 tmp;
+
+	if (!iproc_i2c->slave)
+		return -EINVAL;
+
+	disable_irq(iproc_i2c->irq);
+
+	tasklet_kill(&iproc_i2c->slave_rx_tasklet);
+
+	/* disable all slave interrupts */
+	tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
+	tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
+			IE_S_ALL_INTERRUPT_SHIFT);
+	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
+
+	/* Erase the slave address programmed */
+	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
+	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
+	iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);
+
+	/* flush TX/RX FIFOs */
+	tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
+	iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);
+
+	/* clear all pending slave interrupts */
+	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);
+
+	iproc_i2c->slave = NULL;
+
+	enable_irq(iproc_i2c->irq);
+
+	return 0;
+}
+
 static struct i2c_algorithm bcm_iproc_algo = {
 	.master_xfer = bcm_iproc_i2c_xfer,
 	.functionality = bcm_iproc_i2c_functionality,
@@ -1173,62 +1225,6 @@ static const struct dev_pm_ops bcm_iproc_i2c_pm_ops = {
 	.resume_early = &bcm_iproc_i2c_resume
 };
 
-static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
-{
-	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
-
-	if (iproc_i2c->slave)
-		return -EBUSY;
-
-	if (slave->flags & I2C_CLIENT_TEN)
-		return -EAFNOSUPPORT;
-
-	iproc_i2c->slave = slave;
-
-	tasklet_init(&iproc_i2c->slave_rx_tasklet, slave_rx_tasklet_fn,
-		     (unsigned long)iproc_i2c);
-
-	bcm_iproc_i2c_slave_init(iproc_i2c, false);
-	return 0;
-}
-
-static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
-{
-	u32 tmp;
-	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
-
-	if (!iproc_i2c->slave)
-		return -EINVAL;
-
-	disable_irq(iproc_i2c->irq);
-
-	tasklet_kill(&iproc_i2c->slave_rx_tasklet);
-
-	/* disable all slave interrupts */
-	tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
-	tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
-			IE_S_ALL_INTERRUPT_SHIFT);
-	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
-
-	/* Erase the slave address programmed */
-	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
-	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
-	iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);
-
-	/* flush TX/RX FIFOs */
-	tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
-	iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);
-
-	/* clear all pending slave interrupts */
-	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);
-
-	iproc_i2c->slave = NULL;
-
-	enable_irq(iproc_i2c->irq);
-
-	return 0;
-}
-
 static const struct of_device_id bcm_iproc_i2c_of_match[] = {
 	{
 		.compatible = "brcm,iproc-i2c",
-- 
2.49.0

[PATCH 09/10] i2c: iproc: When there's an error treat it as an error

[Andi Shyti]

If the xfer fails, it indicates a real error. Log it with an
error message instead of a debug message to reflect its severity.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 74903edb4925..9356a16422a3 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -728,31 +728,31 @@ static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
 		return 0;
 
 	case M_CMD_STATUS_LOST_ARB:
-		dev_dbg(iproc_i2c->device, "lost bus arbitration\n");
+		dev_err(iproc_i2c->device, "lost bus arbitration\n");
 		return -EAGAIN;
 
 	case M_CMD_STATUS_NACK_ADDR:
-		dev_dbg(iproc_i2c->device, "NAK addr:0x%02x\n", msg->addr);
+		dev_err(iproc_i2c->device, "NAK addr:0x%02x\n", msg->addr);
 		return -ENXIO;
 
 	case M_CMD_STATUS_NACK_DATA:
-		dev_dbg(iproc_i2c->device, "NAK data\n");
+		dev_err(iproc_i2c->device, "NAK data\n");
 		return -ENXIO;
 
 	case M_CMD_STATUS_TIMEOUT:
-		dev_dbg(iproc_i2c->device, "bus timeout\n");
+		dev_err(iproc_i2c->device, "bus timeout\n");
 		return -ETIMEDOUT;
 
 	case M_CMD_STATUS_FIFO_UNDERRUN:
-		dev_dbg(iproc_i2c->device, "FIFO under-run\n");
+		dev_err(iproc_i2c->device, "FIFO under-run\n");
 		return -ENXIO;
 
 	case M_CMD_STATUS_RX_FIFO_FULL:
-		dev_dbg(iproc_i2c->device, "RX FIFO full\n");
+		dev_err(iproc_i2c->device, "RX FIFO full\n");
 		return -ETIMEDOUT;
 
 	default:
-		dev_dbg(iproc_i2c->device, "unknown error code=%d\n", val);
+		dev_err(iproc_i2c->device, "unknown error code=%d\n", val);
 
 		/* re-initialize i2c for recovery */
 		bcm_iproc_i2c_enable_disable(iproc_i2c, false);
@@ -964,7 +964,7 @@ static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
 
 	ret = bcm_iproc_i2c_xfer_internal(iproc_i2c, msgs, process_call);
 	if (ret) {
-		dev_dbg(iproc_i2c->device, "xfer failed\n");
+		dev_err(iproc_i2c->device, "xfer failed\n");
 		return ret;
 	}
 
-- 
2.49.0

[PATCH 10/10] i2c: iproc: Remove unnecessary double negation

[Andi Shyti]

True is true when greater than '0', no need for double negation
inside the if statement.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 9356a16422a3..2d117f242875 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -836,8 +836,8 @@ static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,
 	struct i2c_msg *msg = &msgs[0];
 
 	/* check if bus is busy */
-	if (!!(iproc_i2c_rd_reg(iproc_i2c,
-				M_CMD_OFFSET) & BIT(M_CMD_START_BUSY_SHIFT))) {
+	if (iproc_i2c_rd_reg(iproc_i2c,
+			     M_CMD_OFFSET) & BIT(M_CMD_START_BUSY_SHIFT)) {
 		dev_warn(iproc_i2c->device, "bus is busy\n");
 		return -EBUSY;
 	}
-- 
2.49.0

Re: [PATCH 00/10] Few improvements on the Broadcom iProc

[Andi Shyti]

> Andi Shyti (10):
>   i2c: iproc: Drop unnecessary initialisation of 'ret'
>   i2c: iproc: Use dev_err_probe in probe
>   i2c: iproc: Use u32 instead of uint32_t
>   i2c: iproc: Fix alignment to match the open parenthesis
>   i2c: iproc: Remove stray blank line in slave ISR
>   i2c: iproc: Replace udelay() with usleep_range()
>   i2c: iproc: Fix indentation of bcm_iproc_i2c_slave_init()
>   i2c: iproc: Move function and avoid prototypes
>   i2c: iproc: When there's an error treat it as an error
>   i2c: iproc: Remove unnecessary double negation

merged to i2c/i2c-host.

Andi