Fix checkpatch.pl warning(s)/error(s)/check(s) in i2c-tegra.c
Remove redundant BUG_ON calls or replace with WARN_ON_ONCE
as needed. Replace BUG() with error handling code.
Define I2C_ERR_UNEXPECTED_STATUS for error handling.
Signed-off-by: Bitan Biswas <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 67 +++++++++++++++++++++++-------------------
1 file changed, 37 insertions(+), 30 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 76b7926..55a5d87 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -78,6 +78,7 @@
#define I2C_ERR_NO_ACK 0x01
#define I2C_ERR_ARBITRATION_LOST 0x02
#define I2C_ERR_UNKNOWN_INTERRUPT 0x04
+#define I2C_ERR_UNEXPECTED_STATUS 0x08
#define PACKET_HEADER0_HEADER_SIZE_SHIFT 28
#define PACKET_HEADER0_PACKET_ID_SHIFT 16
@@ -112,7 +113,7 @@
#define I2C_CLKEN_OVERRIDE 0x090
#define I2C_MST_CORE_CLKEN_OVR BIT(0)
-#define I2C_CONFIG_LOAD_TIMEOUT 1000000
+#define I2C_CONFIG_LOAD_TMOUT 1000000
#define I2C_MST_FIFO_CONTROL 0x0b4
#define I2C_MST_FIFO_CONTROL_RX_FLUSH BIT(0)
@@ -280,6 +281,7 @@ struct tegra_i2c_dev {
u32 bus_clk_rate;
u16 clk_divisor_non_hs_mode;
bool is_multimaster_mode;
+ /* xfer_lock: lock to serialize transfer submission and processing */
spinlock_t xfer_lock;
struct dma_chan *tx_dma_chan;
struct dma_chan *rx_dma_chan;
@@ -306,7 +308,7 @@ static u32 dvc_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
* to the I2C block inside the DVC block
*/
static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
- unsigned long reg)
+ unsigned long reg)
{
if (i2c_dev->is_dvc)
reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
@@ -314,7 +316,7 @@ static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
}
static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
- unsigned long reg)
+ unsigned long reg)
{
writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
@@ -329,13 +331,13 @@ static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
}
static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
- unsigned long reg, int len)
+ unsigned long reg, int len)
{
writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
}
static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
- unsigned long reg, int len)
+ unsigned long reg, int len)
{
readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
}
@@ -486,7 +488,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
return -ETIMEDOUT;
}
- msleep(1);
+ usleep_range(1000, 2000);
}
return 0;
}
@@ -525,7 +527,6 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
* prevent overwriting past the end of buf
*/
if (rx_fifo_avail > 0 && buf_remaining > 0) {
- BUG_ON(buf_remaining > 3);
val = i2c_readl(i2c_dev, I2C_RX_FIFO);
val = cpu_to_le32(val);
memcpy(buf, &val, buf_remaining);
@@ -533,7 +534,6 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
rx_fifo_avail--;
}
- BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
i2c_dev->msg_buf_remaining = buf_remaining;
i2c_dev->msg_buf = buf;
@@ -591,7 +591,6 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
* boundary and fault.
*/
if (tx_fifo_avail > 0 && buf_remaining > 0) {
- BUG_ON(buf_remaining > 3);
memcpy(&val, buf, buf_remaining);
val = le32_to_cpu(val);
@@ -680,10 +679,11 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
if (in_interrupt())
err = readl_poll_timeout_atomic(addr, val, val == 0,
- 1000, I2C_CONFIG_LOAD_TIMEOUT);
+ 1000,
+ I2C_CONFIG_LOAD_TMOUT);
else
- err = readl_poll_timeout(addr, val, val == 0,
- 1000, I2C_CONFIG_LOAD_TIMEOUT);
+ err = readl_poll_timeout(addr, val, val == 0, 1000,
+ I2C_CONFIG_LOAD_TMOUT);
if (err) {
dev_warn(i2c_dev->dev,
@@ -858,16 +858,21 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
if (i2c_dev->msg_buf_remaining)
tegra_i2c_empty_rx_fifo(i2c_dev);
- else
- BUG();
+ else {
+ dev_err(i2c_dev->dev, "unexpected rx data request\n");
+ i2c_dev->msg_err |= I2C_ERR_UNEXPECTED_STATUS;
+ goto err;
+ }
}
if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
- if (i2c_dev->msg_buf_remaining)
- tegra_i2c_fill_tx_fifo(i2c_dev);
- else
+ if (i2c_dev->msg_buf_remaining) {
+ if (tegra_i2c_fill_tx_fifo(i2c_dev))
+ goto err;
+ } else {
tegra_i2c_mask_irq(i2c_dev,
I2C_INT_TX_FIFO_DATA_REQ);
+ }
}
}
@@ -885,7 +890,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
if (status & I2C_INT_PACKET_XFER_COMPLETE) {
if (i2c_dev->is_curr_dma_xfer)
i2c_dev->msg_buf_remaining = 0;
- BUG_ON(i2c_dev->msg_buf_remaining);
+ WARN_ON_ONCE(i2c_dev->msg_buf_remaining);
complete(&i2c_dev->msg_complete);
}
goto done;
@@ -1024,7 +1029,7 @@ static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)
}
static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
- struct i2c_msg *msg, enum msg_end_type end_state)
+ struct i2c_msg *msg, enum msg_end_type end_state)
{
u32 packet_header;
u32 int_mask;
@@ -1034,7 +1039,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
u32 *buffer = NULL;
int err = 0;
bool dma;
- u16 xfer_time = 100;
+ u16 xfer_tm = 100;
tegra_i2c_flush_fifos(i2c_dev);
@@ -1058,7 +1063,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
* Transfer time in mSec = Total bits / transfer rate
* Total bits = 9 bits per byte (including ACK bit) + Start & stop bits
*/
- xfer_time += DIV_ROUND_CLOSEST(((xfer_size * 9) + 2) * MSEC_PER_SEC,
+ xfer_tm += DIV_ROUND_CLOSEST(((xfer_size * 9) + 2) * MSEC_PER_SEC,
i2c_dev->bus_clk_rate);
spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
@@ -1137,7 +1142,8 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
goto unlock;
}
} else {
- tegra_i2c_fill_tx_fifo(i2c_dev);
+ if (tegra_i2c_fill_tx_fifo(i2c_dev))
+ goto unlock;
}
}
@@ -1161,9 +1167,9 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
if (err)
return err;
- time_left = wait_for_completion_timeout(
- &i2c_dev->dma_complete,
- msecs_to_jiffies(xfer_time));
+ time_left =
+ wait_for_completion_timeout(&i2c_dev->dma_complete,
+ msecs_to_jiffies(xfer_tm));
if (time_left == 0) {
dev_err(i2c_dev->dev, "DMA transfer timeout\n");
dmaengine_terminate_sync(i2c_dev->msg_read ?
@@ -1189,7 +1195,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
}
time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
- msecs_to_jiffies(xfer_time));
+ msecs_to_jiffies(xfer_tm));
tegra_i2c_mask_irq(i2c_dev, int_mask);
if (time_left == 0) {
@@ -1225,7 +1231,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
}
static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
- int num)
+ int num)
{
struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
int i;
@@ -1273,12 +1279,12 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
int ret;
ret = of_property_read_u32(np, "clock-frequency",
- &i2c_dev->bus_clk_rate);
+ &i2c_dev->bus_clk_rate);
if (ret)
i2c_dev->bus_clk_rate = 100000; /* default clock rate */
i2c_dev->is_multimaster_mode = of_property_read_bool(np,
- "multi-master");
+ "multi-master");
}
static const struct i2c_algorithm tegra_i2c_algo = {
@@ -1622,7 +1628,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
}
ret = devm_request_irq(&pdev->dev, i2c_dev->irq,
- tegra_i2c_isr, 0, dev_name(&pdev->dev), i2c_dev);
+ tegra_i2c_isr, 0, dev_name(&pdev->dev), i2c_dev);
if (ret) {
dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
goto release_dma;
@@ -1714,6 +1720,7 @@ static const struct dev_pm_ops tegra_i2c_pm = {
SET_RUNTIME_PM_OPS(tegra_i2c_runtime_suspend, tegra_i2c_runtime_resume,
NULL)
};
+
#define TEGRA_I2C_PM (&tegra_i2c_pm)
#else
#define TEGRA_I2C_PM NULL
--
2.7.4
06.06.2019 10:35, Bitan Biswas пишет:
> Fix checkpatch.pl warning(s)/error(s)/check(s) in i2c-tegra.c
>
> Remove redundant BUG_ON calls or replace with WARN_ON_ONCE
> as needed. Replace BUG() with error handling code.
> Define I2C_ERR_UNEXPECTED_STATUS for error handling.
>
> Signed-off-by: Bitan Biswas <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra.c | 67 +++++++++++++++++++++++-------------------
> 1 file changed, 37 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 76b7926..55a5d87 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -78,6 +78,7 @@
> #define I2C_ERR_NO_ACK 0x01
> #define I2C_ERR_ARBITRATION_LOST 0x02
> #define I2C_ERR_UNKNOWN_INTERRUPT 0x04
> +#define I2C_ERR_UNEXPECTED_STATUS 0x08
>
> #define PACKET_HEADER0_HEADER_SIZE_SHIFT 28
> #define PACKET_HEADER0_PACKET_ID_SHIFT 16
> @@ -112,7 +113,7 @@
> #define I2C_CLKEN_OVERRIDE 0x090
> #define I2C_MST_CORE_CLKEN_OVR BIT(0)
>
> -#define I2C_CONFIG_LOAD_TIMEOUT 1000000
> +#define I2C_CONFIG_LOAD_TMOUT 1000000
>
> #define I2C_MST_FIFO_CONTROL 0x0b4
> #define I2C_MST_FIFO_CONTROL_RX_FLUSH BIT(0)
> @@ -280,6 +281,7 @@ struct tegra_i2c_dev {
> u32 bus_clk_rate;
> u16 clk_divisor_non_hs_mode;
> bool is_multimaster_mode;
> + /* xfer_lock: lock to serialize transfer submission and processing */
> spinlock_t xfer_lock;
> struct dma_chan *tx_dma_chan;
> struct dma_chan *rx_dma_chan;
> @@ -306,7 +308,7 @@ static u32 dvc_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
> * to the I2C block inside the DVC block
> */
> static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
> - unsigned long reg)
> + unsigned long reg)
> {
> if (i2c_dev->is_dvc)
> reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
> @@ -314,7 +316,7 @@ static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
> }
>
> static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
> - unsigned long reg)
> + unsigned long reg)
> {
> writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>
> @@ -329,13 +331,13 @@ static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
> }
>
> static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
> - unsigned long reg, int len)
> + unsigned long reg, int len)
> {
> writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
> }
>
> static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
> - unsigned long reg, int len)
> + unsigned long reg, int len)
> {
> readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
> }
> @@ -486,7 +488,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
> dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
> return -ETIMEDOUT;
> }
> - msleep(1);
> + usleep_range(1000, 2000);
> }
> return 0;
> }
> @@ -525,7 +527,6 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
> * prevent overwriting past the end of buf
> */
> if (rx_fifo_avail > 0 && buf_remaining > 0) {
> - BUG_ON(buf_remaining > 3);
> val = i2c_readl(i2c_dev, I2C_RX_FIFO);
> val = cpu_to_le32(val);
> memcpy(buf, &val, buf_remaining);
> @@ -533,7 +534,6 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
> rx_fifo_avail--;
> }
>
> - BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
> i2c_dev->msg_buf_remaining = buf_remaining;
> i2c_dev->msg_buf = buf;
>
> @@ -591,7 +591,6 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
> * boundary and fault.
> */
> if (tx_fifo_avail > 0 && buf_remaining > 0) {
> - BUG_ON(buf_remaining > 3);
> memcpy(&val, buf, buf_remaining);
> val = le32_to_cpu(val);
>
> @@ -680,10 +679,11 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
> i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
> if (in_interrupt())
> err = readl_poll_timeout_atomic(addr, val, val == 0,
> - 1000, I2C_CONFIG_LOAD_TIMEOUT);
> + 1000,
> + I2C_CONFIG_LOAD_TMOUT);
> else
> - err = readl_poll_timeout(addr, val, val == 0,
> - 1000, I2C_CONFIG_LOAD_TIMEOUT);
> + err = readl_poll_timeout(addr, val, val == 0, 1000,
> + I2C_CONFIG_LOAD_TMOUT);
>
> if (err) {
> dev_warn(i2c_dev->dev,
> @@ -858,16 +858,21 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
> if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
> if (i2c_dev->msg_buf_remaining)
> tegra_i2c_empty_rx_fifo(i2c_dev);
> - else
> - BUG();
> + else {
> + dev_err(i2c_dev->dev, "unexpected rx data request\n");
> + i2c_dev->msg_err |= I2C_ERR_UNEXPECTED_STATUS;
> + goto err;
> + }
> }
>
> if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
> - if (i2c_dev->msg_buf_remaining)
> - tegra_i2c_fill_tx_fifo(i2c_dev);
> - else
> + if (i2c_dev->msg_buf_remaining) {
> + if (tegra_i2c_fill_tx_fifo(i2c_dev))
> + goto err;
> + } else {
> tegra_i2c_mask_irq(i2c_dev,
> I2C_INT_TX_FIFO_DATA_REQ);
> + }
> }
> }
>
> @@ -885,7 +890,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
> if (status & I2C_INT_PACKET_XFER_COMPLETE) {
> if (i2c_dev->is_curr_dma_xfer)
> i2c_dev->msg_buf_remaining = 0;
> - BUG_ON(i2c_dev->msg_buf_remaining);
> + WARN_ON_ONCE(i2c_dev->msg_buf_remaining);
> complete(&i2c_dev->msg_complete);
> }
> goto done;
> @@ -1024,7 +1029,7 @@ static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)
> }
>
> static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> - struct i2c_msg *msg, enum msg_end_type end_state)
> + struct i2c_msg *msg, enum msg_end_type end_state)
> {
> u32 packet_header;
> u32 int_mask;
> @@ -1034,7 +1039,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> u32 *buffer = NULL;
> int err = 0;
> bool dma;
> - u16 xfer_time = 100;
> + u16 xfer_tm = 100;
Why xfer_time is renamed? It is much more important to keep code
readable rather than to satisfy checkpatch. You should *not* follow
checkpatch recommendations where they do not make much sense. The
xfer_tm is a less intuitive naming and hence it harms readability of the
code. Hence it is better to have "lines over 80 chars" in this
particular case.
Also, please don't skip review comments. I already pointed out the above
in the answer to previous version of the patch.
On 6/6/19 4:39 AM, Dmitry Osipenko wrote:
> 06.06.2019 10:35, Bitan Biswas пишет:
>> Fix checkpatch.pl warning(s)/error(s)/check(s) in i2c-tegra.c
>>
>> Remove redundant BUG_ON calls or replace with WARN_ON_ONCE
>> as needed. Replace BUG() with error handling code.
>> Define I2C_ERR_UNEXPECTED_STATUS for error handling.
>>
>> Signed-off-by: Bitan Biswas <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-tegra.c | 67 +++++++++++++++++++++++-------------------
>> 1 file changed, 37 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 76b7926..55a5d87 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -78,6 +78,7 @@
>> #define I2C_ERR_NO_ACK 0x01
>> #define I2C_ERR_ARBITRATION_LOST 0x02
>> #define I2C_ERR_UNKNOWN_INTERRUPT 0x04
>> +#define I2C_ERR_UNEXPECTED_STATUS 0x08
>>
>> #define PACKET_HEADER0_HEADER_SIZE_SHIFT 28
>> #define PACKET_HEADER0_PACKET_ID_SHIFT 16
>> @@ -112,7 +113,7 @@
>> #define I2C_CLKEN_OVERRIDE 0x090
>> #define I2C_MST_CORE_CLKEN_OVR BIT(0)
>>
>> -#define I2C_CONFIG_LOAD_TIMEOUT 1000000
>> +#define I2C_CONFIG_LOAD_TMOUT 1000000
>>
>> #define I2C_MST_FIFO_CONTROL 0x0b4
>> #define I2C_MST_FIFO_CONTROL_RX_FLUSH BIT(0)
>> @@ -280,6 +281,7 @@ struct tegra_i2c_dev {
>> u32 bus_clk_rate;
>> u16 clk_divisor_non_hs_mode;
>> bool is_multimaster_mode;
>> + /* xfer_lock: lock to serialize transfer submission and processing */
>> spinlock_t xfer_lock;
>> struct dma_chan *tx_dma_chan;
>> struct dma_chan *rx_dma_chan;
>> @@ -306,7 +308,7 @@ static u32 dvc_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
>> * to the I2C block inside the DVC block
>> */
>> static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
>> - unsigned long reg)
>> + unsigned long reg)
>> {
>> if (i2c_dev->is_dvc)
>> reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
>> @@ -314,7 +316,7 @@ static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
>> }
>>
>> static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
>> - unsigned long reg)
>> + unsigned long reg)
>> {
>> writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>>
>> @@ -329,13 +331,13 @@ static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
>> }
>>
>> static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
>> - unsigned long reg, int len)
>> + unsigned long reg, int len)
>> {
>> writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
>> }
>>
>> static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
>> - unsigned long reg, int len)
>> + unsigned long reg, int len)
>> {
>> readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
>> }
>> @@ -486,7 +488,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>> dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
>> return -ETIMEDOUT;
>> }
>> - msleep(1);
>> + usleep_range(1000, 2000);
>> }
>> return 0;
>> }
>> @@ -525,7 +527,6 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
>> * prevent overwriting past the end of buf
>> */
>> if (rx_fifo_avail > 0 && buf_remaining > 0) {
>> - BUG_ON(buf_remaining > 3);
>> val = i2c_readl(i2c_dev, I2C_RX_FIFO);
>> val = cpu_to_le32(val);
>> memcpy(buf, &val, buf_remaining);
>> @@ -533,7 +534,6 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
>> rx_fifo_avail--;
>> }
>>
>> - BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
>> i2c_dev->msg_buf_remaining = buf_remaining;
>> i2c_dev->msg_buf = buf;
>>
>> @@ -591,7 +591,6 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
>> * boundary and fault.
>> */
>> if (tx_fifo_avail > 0 && buf_remaining > 0) {
>> - BUG_ON(buf_remaining > 3);
>> memcpy(&val, buf, buf_remaining);
>> val = le32_to_cpu(val);
>>
>> @@ -680,10 +679,11 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>> i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
>> if (in_interrupt())
>> err = readl_poll_timeout_atomic(addr, val, val == 0,
>> - 1000, I2C_CONFIG_LOAD_TIMEOUT);
>> + 1000,
>> + I2C_CONFIG_LOAD_TMOUT);
>> else
>> - err = readl_poll_timeout(addr, val, val == 0,
>> - 1000, I2C_CONFIG_LOAD_TIMEOUT);
>> + err = readl_poll_timeout(addr, val, val == 0, 1000,
>> + I2C_CONFIG_LOAD_TMOUT);
>>
>> if (err) {
>> dev_warn(i2c_dev->dev,
>> @@ -858,16 +858,21 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>> if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
>> if (i2c_dev->msg_buf_remaining)
>> tegra_i2c_empty_rx_fifo(i2c_dev);
>> - else
>> - BUG();
>> + else {
>> + dev_err(i2c_dev->dev, "unexpected rx data request\n");
>> + i2c_dev->msg_err |= I2C_ERR_UNEXPECTED_STATUS;
>> + goto err;
>> + }
>> }
>>
>> if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
>> - if (i2c_dev->msg_buf_remaining)
>> - tegra_i2c_fill_tx_fifo(i2c_dev);
>> - else
>> + if (i2c_dev->msg_buf_remaining) {
>> + if (tegra_i2c_fill_tx_fifo(i2c_dev))
>> + goto err;
>> + } else {
>> tegra_i2c_mask_irq(i2c_dev,
>> I2C_INT_TX_FIFO_DATA_REQ);
>> + }
>> }
>> }
>>
>> @@ -885,7 +890,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>> if (status & I2C_INT_PACKET_XFER_COMPLETE) {
>> if (i2c_dev->is_curr_dma_xfer)
>> i2c_dev->msg_buf_remaining = 0;
>> - BUG_ON(i2c_dev->msg_buf_remaining);
>> + WARN_ON_ONCE(i2c_dev->msg_buf_remaining);
>> complete(&i2c_dev->msg_complete);
>> }
>> goto done;
>> @@ -1024,7 +1029,7 @@ static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)
>> }
>>
>> static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>> - struct i2c_msg *msg, enum msg_end_type end_state)
>> + struct i2c_msg *msg, enum msg_end_type end_state)
>> {
>> u32 packet_header;
>> u32 int_mask;
>> @@ -1034,7 +1039,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>> u32 *buffer = NULL;
>> int err = 0;
>> bool dma;
>> - u16 xfer_time = 100;
>> + u16 xfer_tm = 100;
>
> Why xfer_time is renamed? It is much more important to keep code
> readable rather than to satisfy checkpatch. You should *not* follow
> checkpatch recommendations where they do not make much sense. The
> xfer_tm is a less intuitive naming and hence it harms readability of the
> code. Hence it is better to have "lines over 80 chars" in this
> particular case.
Agreed. I shall share updated patch.
>
> Also, please don't skip review comments. I already pointed out the above
> in the answer to previous version of the patch.
>
I apologize for the oversight. I shall be more careful in future.
-regards,
Bitan
06.06.2019 17:02, Bitan Biswas пишет:
>
>
> On 6/6/19 4:39 AM, Dmitry Osipenko wrote:
>> 06.06.2019 10:35, Bitan Biswas пишет:
>>> Fix checkpatch.pl warning(s)/error(s)/check(s) in i2c-tegra.c
>>>
>>> Remove redundant BUG_ON calls or replace with WARN_ON_ONCE
>>> as needed. Replace BUG() with error handling code.
>>> Define I2C_ERR_UNEXPECTED_STATUS for error handling.
>>>
>>> Signed-off-by: Bitan Biswas <[email protected]>
>>> ---
>>> drivers/i2c/busses/i2c-tegra.c | 67
>>> +++++++++++++++++++++++-------------------
>>> 1 file changed, 37 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c
>>> b/drivers/i2c/busses/i2c-tegra.c
>>> index 76b7926..55a5d87 100644
>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>> @@ -78,6 +78,7 @@
>>> #define I2C_ERR_NO_ACK 0x01
>>> #define I2C_ERR_ARBITRATION_LOST 0x02
>>> #define I2C_ERR_UNKNOWN_INTERRUPT 0x04
>>> +#define I2C_ERR_UNEXPECTED_STATUS 0x08
>>> #define PACKET_HEADER0_HEADER_SIZE_SHIFT 28
>>> #define PACKET_HEADER0_PACKET_ID_SHIFT 16
>>> @@ -112,7 +113,7 @@
>>> #define I2C_CLKEN_OVERRIDE 0x090
>>> #define I2C_MST_CORE_CLKEN_OVR BIT(0)
>>> -#define I2C_CONFIG_LOAD_TIMEOUT 1000000
>>> +#define I2C_CONFIG_LOAD_TMOUT 1000000
>>> #define I2C_MST_FIFO_CONTROL 0x0b4
>>> #define I2C_MST_FIFO_CONTROL_RX_FLUSH BIT(0)
>>> @@ -280,6 +281,7 @@ struct tegra_i2c_dev {
>>> u32 bus_clk_rate;
>>> u16 clk_divisor_non_hs_mode;
>>> bool is_multimaster_mode;
>>> + /* xfer_lock: lock to serialize transfer submission and
>>> processing */
>>> spinlock_t xfer_lock;
>>> struct dma_chan *tx_dma_chan;
>>> struct dma_chan *rx_dma_chan;
>>> @@ -306,7 +308,7 @@ static u32 dvc_readl(struct tegra_i2c_dev
>>> *i2c_dev, unsigned long reg)
>>> * to the I2C block inside the DVC block
>>> */
>>> static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
>>> - unsigned long reg)
>>> + unsigned long reg)
>>> {
>>> if (i2c_dev->is_dvc)
>>> reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
>>> @@ -314,7 +316,7 @@ static unsigned long tegra_i2c_reg_addr(struct
>>> tegra_i2c_dev *i2c_dev,
>>> }
>>> static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
>>> - unsigned long reg)
>>> + unsigned long reg)
>>> {
>>> writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>>> @@ -329,13 +331,13 @@ static u32 i2c_readl(struct tegra_i2c_dev
>>> *i2c_dev, unsigned long reg)
>>> }
>>> static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
>>> - unsigned long reg, int len)
>>> + unsigned long reg, int len)
>>> {
>>> writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data,
>>> len);
>>> }
>>> static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
>>> - unsigned long reg, int len)
>>> + unsigned long reg, int len)
>>> {
>>> readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data,
>>> len);
>>> }
>>> @@ -486,7 +488,7 @@ static int tegra_i2c_flush_fifos(struct
>>> tegra_i2c_dev *i2c_dev)
>>> dev_warn(i2c_dev->dev, "timeout waiting for fifo
>>> flush\n");
>>> return -ETIMEDOUT;
>>> }
>>> - msleep(1);
>>> + usleep_range(1000, 2000);
>>> }
>>> return 0;
>>> }
>>> @@ -525,7 +527,6 @@ static int tegra_i2c_empty_rx_fifo(struct
>>> tegra_i2c_dev *i2c_dev)
>>> * prevent overwriting past the end of buf
>>> */
>>> if (rx_fifo_avail > 0 && buf_remaining > 0) {
>>> - BUG_ON(buf_remaining > 3);
>>> val = i2c_readl(i2c_dev, I2C_RX_FIFO);
>>> val = cpu_to_le32(val);
>>> memcpy(buf, &val, buf_remaining);
>>> @@ -533,7 +534,6 @@ static int tegra_i2c_empty_rx_fifo(struct
>>> tegra_i2c_dev *i2c_dev)
>>> rx_fifo_avail--;
>>> }
>>> - BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
>>> i2c_dev->msg_buf_remaining = buf_remaining;
>>> i2c_dev->msg_buf = buf;
>>> @@ -591,7 +591,6 @@ static int tegra_i2c_fill_tx_fifo(struct
>>> tegra_i2c_dev *i2c_dev)
>>> * boundary and fault.
>>> */
>>> if (tx_fifo_avail > 0 && buf_remaining > 0) {
>>> - BUG_ON(buf_remaining > 3);
>>> memcpy(&val, buf, buf_remaining);
>>> val = le32_to_cpu(val);
>>> @@ -680,10 +679,11 @@ static int
>>> tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>>> i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
>>> if (in_interrupt())
>>> err = readl_poll_timeout_atomic(addr, val, val == 0,
>>> - 1000, I2C_CONFIG_LOAD_TIMEOUT);
>>> + 1000,
>>> + I2C_CONFIG_LOAD_TMOUT);
>>> else
>>> - err = readl_poll_timeout(addr, val, val == 0,
>>> - 1000, I2C_CONFIG_LOAD_TIMEOUT);
>>> + err = readl_poll_timeout(addr, val, val == 0, 1000,
>>> + I2C_CONFIG_LOAD_TMOUT);
>>> if (err) {
>>> dev_warn(i2c_dev->dev,
>>> @@ -858,16 +858,21 @@ static irqreturn_t tegra_i2c_isr(int irq, void
>>> *dev_id)
>>> if (i2c_dev->msg_read && (status &
>>> I2C_INT_RX_FIFO_DATA_REQ)) {
>>> if (i2c_dev->msg_buf_remaining)
>>> tegra_i2c_empty_rx_fifo(i2c_dev);
>>> - else
>>> - BUG();
>>> + else {
>>> + dev_err(i2c_dev->dev, "unexpected rx data request\n");
>>> + i2c_dev->msg_err |= I2C_ERR_UNEXPECTED_STATUS;
>>> + goto err;
>>> + }
>>> }
>>> if (!i2c_dev->msg_read && (status &
>>> I2C_INT_TX_FIFO_DATA_REQ)) {
>>> - if (i2c_dev->msg_buf_remaining)
>>> - tegra_i2c_fill_tx_fifo(i2c_dev);
>>> - else
>>> + if (i2c_dev->msg_buf_remaining) {
>>> + if (tegra_i2c_fill_tx_fifo(i2c_dev))
>>> + goto err;
>>> + } else {
>>> tegra_i2c_mask_irq(i2c_dev,
>>> I2C_INT_TX_FIFO_DATA_REQ);
>>> + }
>>> }
>>> }
>>> @@ -885,7 +890,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void
>>> *dev_id)
>>> if (status & I2C_INT_PACKET_XFER_COMPLETE) {
>>> if (i2c_dev->is_curr_dma_xfer)
>>> i2c_dev->msg_buf_remaining = 0;
>>> - BUG_ON(i2c_dev->msg_buf_remaining);
>>> + WARN_ON_ONCE(i2c_dev->msg_buf_remaining);
>>> complete(&i2c_dev->msg_complete);
>>> }
>>> goto done;
>>> @@ -1024,7 +1029,7 @@ static int tegra_i2c_issue_bus_clear(struct
>>> i2c_adapter *adap)
>>> }
>>> static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>>> - struct i2c_msg *msg, enum msg_end_type end_state)
>>> + struct i2c_msg *msg, enum msg_end_type end_state)
>>> {
>>> u32 packet_header;
>>> u32 int_mask;
>>> @@ -1034,7 +1039,7 @@ static int tegra_i2c_xfer_msg(struct
>>> tegra_i2c_dev *i2c_dev,
>>> u32 *buffer = NULL;
>>> int err = 0;
>>> bool dma;
>>> - u16 xfer_time = 100;
>>> + u16 xfer_tm = 100;
>>
>> Why xfer_time is renamed? It is much more important to keep code
>> readable rather than to satisfy checkpatch. You should *not* follow
>> checkpatch recommendations where they do not make much sense. The
>> xfer_tm is a less intuitive naming and hence it harms readability of the
>> code. Hence it is better to have "lines over 80 chars" in this
>> particular case.
> Agreed. I shall share updated patch.
Yes, please.
>>
>> Also, please don't skip review comments. I already pointed out the above
>> in the answer to previous version of the patch.
>>
> I apologize for the oversight. I shall be more careful in future.
No problems ;)
On Thu, Jun 06, 2019 at 12:35:23AM -0700, Bitan Biswas wrote:
> Fix checkpatch.pl warning(s)/error(s)/check(s) in i2c-tegra.c
>
> Remove redundant BUG_ON calls or replace with WARN_ON_ONCE
> as needed. Replace BUG() with error handling code.
> Define I2C_ERR_UNEXPECTED_STATUS for error handling.
>
> Signed-off-by: Bitan Biswas <[email protected]>
I wonder why you didn't fix this checkpatch defect?
WARNING: A patch subject line should describe the change not the tool that found it
On 6/6/19 4:57 AM, Wolfram Sang wrote:
> On Thu, Jun 06, 2019 at 12:35:23AM -0700, Bitan Biswas wrote:
>> Fix checkpatch.pl warning(s)/error(s)/check(s) in i2c-tegra.c
>>
>> Remove redundant BUG_ON calls or replace with WARN_ON_ONCE
>> as needed. Replace BUG() with error handling code.
>> Define I2C_ERR_UNEXPECTED_STATUS for error handling.
>>
>> Signed-off-by: Bitan Biswas <[email protected]>
>
> I wonder why you didn't fix this checkpatch defect?
>
> WARNING: A patch subject line should describe the change not the tool that found it
>
I ran checkpatch.pl on the source file only hence missed this warning. I
shall fix this in updated patch.
-Thanks,
Bitan
On 2019-06-06 09:35, Bitan Biswas wrote:
> Fix checkpatch.pl warning(s)/error(s)/check(s) in i2c-tegra.c
>
> Remove redundant BUG_ON calls or replace with WARN_ON_ONCE
> as needed. Replace BUG() with error handling code.
> Define I2C_ERR_UNEXPECTED_STATUS for error handling.
>
> Signed-off-by: Bitan Biswas <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra.c | 67 +++++++++++++++++++++++-------------------
> 1 file changed, 37 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 76b7926..55a5d87 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -78,6 +78,7 @@
> #define I2C_ERR_NO_ACK 0x01
> #define I2C_ERR_ARBITRATION_LOST 0x02
> #define I2C_ERR_UNKNOWN_INTERRUPT 0x04
> +#define I2C_ERR_UNEXPECTED_STATUS 0x08
Use tabs like the the surrounding code. And perhaps convert all
these flags to use the BIT() macro?
>
> #define PACKET_HEADER0_HEADER_SIZE_SHIFT 28
> #define PACKET_HEADER0_PACKET_ID_SHIFT 16
> @@ -112,7 +113,7 @@
> #define I2C_CLKEN_OVERRIDE 0x090
> #define I2C_MST_CORE_CLKEN_OVR BIT(0)
>
> -#define I2C_CONFIG_LOAD_TIMEOUT 1000000
> +#define I2C_CONFIG_LOAD_TMOUT 1000000
Similar to xfer_tm already mentioned by Dmitry; just keep it as
..._TIMEOUT and ignore checkpatch on this issue. Or juggle the
code in some other way to pacify checkpatch. E.g. abbreviate
CONFIG instead? Or something. CONF is way easier to read than
TMOUT IMHO...
Cheers,
Peter
On 6/6/19 1:45 PM, Peter Rosin wrote:
> On 2019-06-06 09:35, Bitan Biswas wrote:
>> Fix checkpatch.pl warning(s)/error(s)/check(s) in i2c-tegra.c
>>
>> Remove redundant BUG_ON calls or replace with WARN_ON_ONCE
>> as needed. Replace BUG() with error handling code.
>> Define I2C_ERR_UNEXPECTED_STATUS for error handling.
>>
>> Signed-off-by: Bitan Biswas <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-tegra.c | 67 +++++++++++++++++++++++-------------------
>> 1 file changed, 37 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 76b7926..55a5d87 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -78,6 +78,7 @@
>> #define I2C_ERR_NO_ACK 0x01
>> #define I2C_ERR_ARBITRATION_LOST 0x02
>> #define I2C_ERR_UNKNOWN_INTERRUPT 0x04
>> +#define I2C_ERR_UNEXPECTED_STATUS 0x08
>
> Use tabs like the the surrounding code. And perhaps convert all
> these flags to use the BIT() macro?
I shall correct the line and use tabs. I shall convert macros to BIT()
if possible.
>
>>
>> #define PACKET_HEADER0_HEADER_SIZE_SHIFT 28
>> #define PACKET_HEADER0_PACKET_ID_SHIFT 16
>> @@ -112,7 +113,7 @@
>> #define I2C_CLKEN_OVERRIDE 0x090
>> #define I2C_MST_CORE_CLKEN_OVR BIT(0)
>>
>> -#define I2C_CONFIG_LOAD_TIMEOUT 1000000
>> +#define I2C_CONFIG_LOAD_TMOUT 1000000
>
> Similar to xfer_tm already mentioned by Dmitry; just keep it as
> ..._TIMEOUT and ignore checkpatch on this issue. Or juggle the
> code in some other way to pacify checkpatch. E.g. abbreviate
> CONFIG instead? Or something. CONF is way easier to read than
> TMOUT IMHO...
OK. Just for consistency planning to ignore checkpatch warning and shall
keep current macro I2C_CONFIG_LOAD_TIMEOUT.
-Thanks,
Bitan