| Message ID | 20200908224006.25636-30-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improvements for Tegra I2C driver | expand |
On Wed, Sep 09, 2020 at 01:40:01AM +0300, Dmitry Osipenko wrote: > Reorder definition of variables in the code to have them sorted by length > and grouped logically, also replace "unsigned long" with "u32". Do this in > order to make code easier to read. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 97 ++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 52 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index ac40c87f1c21..2376f502d299 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -259,42 +259,48 @@ struct tegra_i2c_hw_feature { > */ > struct tegra_i2c_dev { > struct device *dev; > - const struct tegra_i2c_hw_feature *hw; > struct i2c_adapter adapter; > - struct clk *div_clk; > - struct clk_bulk_data *clocks; > - unsigned int nclocks; > + > + const struct tegra_i2c_hw_feature *hw; > struct reset_control *rst; > - void __iomem *base; > - phys_addr_t base_phys; > unsigned int cont_id; > unsigned int irq; > - bool is_dvc; > - bool is_vi; > + > + phys_addr_t base_phys; > + void __iomem *base; > + > + struct clk_bulk_data *clocks; > + unsigned int nclocks; > + > + struct clk *div_clk; > + u32 bus_clk_rate; > + > struct completion msg_complete; > + size_t msg_buf_remaining; > int msg_err; > u8 *msg_buf; > - size_t msg_buf_remaining; > - bool msg_read; > - u32 bus_clk_rate; > - bool is_multimaster_mode; > + > + struct completion dma_complete; > struct dma_chan *tx_dma_chan; > struct dma_chan *rx_dma_chan; > + unsigned int dma_buf_size; > dma_addr_t dma_phys; > u32 *dma_buf; > - unsigned int dma_buf_size; > - bool is_curr_dma_xfer; > - struct completion dma_complete; > + > + bool is_multimaster_mode; > bool is_curr_atomic_xfer; > + bool is_curr_dma_xfer; > + bool msg_read; > + bool is_dvc; > + bool is_vi; > }; > > -static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, > - unsigned long reg) > +static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, u32 reg) I actually prefer unsigned long/int over u32 for offsets because it makes it clearer that this is not in fact a 32-bit value that we're writing into a register. This is especially true for these register accessors where the "offset" is called "reg" and may be easily mistaken for a register value. Thierry
17.09.2020 15:16, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:40:01AM +0300, Dmitry Osipenko wrote: >> Reorder definition of variables in the code to have them sorted by length >> and grouped logically, also replace "unsigned long" with "u32". Do this in >> order to make code easier to read. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 97 ++++++++++++++++------------------ >> 1 file changed, 45 insertions(+), 52 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index ac40c87f1c21..2376f502d299 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -259,42 +259,48 @@ struct tegra_i2c_hw_feature { >> */ >> struct tegra_i2c_dev { >> struct device *dev; >> - const struct tegra_i2c_hw_feature *hw; >> struct i2c_adapter adapter; >> - struct clk *div_clk; >> - struct clk_bulk_data *clocks; >> - unsigned int nclocks; >> + >> + const struct tegra_i2c_hw_feature *hw; >> struct reset_control *rst; >> - void __iomem *base; >> - phys_addr_t base_phys; >> unsigned int cont_id; >> unsigned int irq; >> - bool is_dvc; >> - bool is_vi; >> + >> + phys_addr_t base_phys; >> + void __iomem *base; >> + >> + struct clk_bulk_data *clocks; >> + unsigned int nclocks; >> + >> + struct clk *div_clk; >> + u32 bus_clk_rate; >> + >> struct completion msg_complete; >> + size_t msg_buf_remaining; >> int msg_err; >> u8 *msg_buf; >> - size_t msg_buf_remaining; >> - bool msg_read; >> - u32 bus_clk_rate; >> - bool is_multimaster_mode; >> + >> + struct completion dma_complete; >> struct dma_chan *tx_dma_chan; >> struct dma_chan *rx_dma_chan; >> + unsigned int dma_buf_size; >> dma_addr_t dma_phys; >> u32 *dma_buf; >> - unsigned int dma_buf_size; >> - bool is_curr_dma_xfer; >> - struct completion dma_complete; >> + >> + bool is_multimaster_mode; >> bool is_curr_atomic_xfer; >> + bool is_curr_dma_xfer; >> + bool msg_read; >> + bool is_dvc; >> + bool is_vi; >> }; >> >> -static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, >> - unsigned long reg) >> +static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, u32 reg) > > I actually prefer unsigned long/int over u32 for offsets because it > makes it clearer that this is not in fact a 32-bit value that we're > writing into a register. This is especially true for these register > accessors where the "offset" is called "reg" and may be easily > mistaken for a register value. That is a bit questionable, at least it definitely won't save me from a mistake :)
On Wed, 09 Sep 2020 01:40:01 +0300, Dmitry Osipenko wrote: > Reorder definition of variables in the code to have them sorted by length > and grouped logically, also replace "unsigned long" with "u32". Do this in > order to make code easier to read. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 97 ++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 52 deletions(-) Tested-by: Thierry Reding <treding@nvidia.com>
On Thu, Sep 17, 2020 at 06:13:36PM +0300, Dmitry Osipenko wrote: > 17.09.2020 15:16, Thierry Reding пишет: > > On Wed, Sep 09, 2020 at 01:40:01AM +0300, Dmitry Osipenko wrote: > >> Reorder definition of variables in the code to have them sorted by length > >> and grouped logically, also replace "unsigned long" with "u32". Do this in > >> order to make code easier to read. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/i2c/busses/i2c-tegra.c | 97 ++++++++++++++++------------------ > >> 1 file changed, 45 insertions(+), 52 deletions(-) > >> > >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > >> index ac40c87f1c21..2376f502d299 100644 > >> --- a/drivers/i2c/busses/i2c-tegra.c > >> +++ b/drivers/i2c/busses/i2c-tegra.c > >> @@ -259,42 +259,48 @@ struct tegra_i2c_hw_feature { > >> */ > >> struct tegra_i2c_dev { > >> struct device *dev; > >> - const struct tegra_i2c_hw_feature *hw; > >> struct i2c_adapter adapter; > >> - struct clk *div_clk; > >> - struct clk_bulk_data *clocks; > >> - unsigned int nclocks; > >> + > >> + const struct tegra_i2c_hw_feature *hw; > >> struct reset_control *rst; > >> - void __iomem *base; > >> - phys_addr_t base_phys; > >> unsigned int cont_id; > >> unsigned int irq; > >> - bool is_dvc; > >> - bool is_vi; > >> + > >> + phys_addr_t base_phys; > >> + void __iomem *base; > >> + > >> + struct clk_bulk_data *clocks; > >> + unsigned int nclocks; > >> + > >> + struct clk *div_clk; > >> + u32 bus_clk_rate; > >> + > >> struct completion msg_complete; > >> + size_t msg_buf_remaining; > >> int msg_err; > >> u8 *msg_buf; > >> - size_t msg_buf_remaining; > >> - bool msg_read; > >> - u32 bus_clk_rate; > >> - bool is_multimaster_mode; > >> + > >> + struct completion dma_complete; > >> struct dma_chan *tx_dma_chan; > >> struct dma_chan *rx_dma_chan; > >> + unsigned int dma_buf_size; > >> dma_addr_t dma_phys; > >> u32 *dma_buf; > >> - unsigned int dma_buf_size; > >> - bool is_curr_dma_xfer; > >> - struct completion dma_complete; > >> + > >> + bool is_multimaster_mode; > >> bool is_curr_atomic_xfer; > >> + bool is_curr_dma_xfer; > >> + bool msg_read; > >> + bool is_dvc; > >> + bool is_vi; > >> }; > >> > >> -static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, > >> - unsigned long reg) > >> +static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, u32 reg) > > > > I actually prefer unsigned long/int over u32 for offsets because it > > makes it clearer that this is not in fact a 32-bit value that we're > > writing into a register. This is especially true for these register > > accessors where the "offset" is called "reg" and may be easily > > mistaken for a register value. > > That is a bit questionable, at least it definitely won't save me from a > mistake :) There's obviously no way of guaranteeing that nobody makes a mistake. But especially with accessors the value and offset parameters are inconsistently ordered, so any help we can provide at the API level may help avoid mistakes. If you only look at the prototype, having two u32 parameters doesn't give you *any* clue about what they are. But a 32-bit accessor that takes a u32 and an unsigned int is pretty obviously expecting the 32-bit value in the u32 parameter and then the unsigned int must obviously be the offset. Thierry
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index ac40c87f1c21..2376f502d299 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -259,42 +259,48 @@ struct tegra_i2c_hw_feature { */ struct tegra_i2c_dev { struct device *dev; - const struct tegra_i2c_hw_feature *hw; struct i2c_adapter adapter; - struct clk *div_clk; - struct clk_bulk_data *clocks; - unsigned int nclocks; + + const struct tegra_i2c_hw_feature *hw; struct reset_control *rst; - void __iomem *base; - phys_addr_t base_phys; unsigned int cont_id; unsigned int irq; - bool is_dvc; - bool is_vi; + + phys_addr_t base_phys; + void __iomem *base; + + struct clk_bulk_data *clocks; + unsigned int nclocks; + + struct clk *div_clk; + u32 bus_clk_rate; + struct completion msg_complete; + size_t msg_buf_remaining; int msg_err; u8 *msg_buf; - size_t msg_buf_remaining; - bool msg_read; - u32 bus_clk_rate; - bool is_multimaster_mode; + + struct completion dma_complete; struct dma_chan *tx_dma_chan; struct dma_chan *rx_dma_chan; + unsigned int dma_buf_size; dma_addr_t dma_phys; u32 *dma_buf; - unsigned int dma_buf_size; - bool is_curr_dma_xfer; - struct completion dma_complete; + + bool is_multimaster_mode; bool is_curr_atomic_xfer; + bool is_curr_dma_xfer; + bool msg_read; + bool is_dvc; + bool is_vi; }; -static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, - unsigned long reg) +static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, u32 reg) { writel_relaxed(val, i2c_dev->base + reg); } -static u32 dvc_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg) +static u32 dvc_readl(struct tegra_i2c_dev *i2c_dev, u32 reg) { return readl_relaxed(i2c_dev->base + reg); } @@ -303,8 +309,7 @@ static u32 dvc_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg) * i2c_writel and i2c_readl will offset the register if necessary to talk * to the I2C block inside the DVC block */ -static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev, - unsigned long reg) +static u32 tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev, u32 reg) { if (i2c_dev->is_dvc) reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40; @@ -313,8 +318,7 @@ static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev, return reg; } -static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val, - unsigned long reg) +static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val, u32 reg) { writel_relaxed(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg)); @@ -323,19 +327,19 @@ static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val, readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg)); } -static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg) +static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, u32 reg) { return readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg)); } static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data, - unsigned long reg, unsigned int len) + u32 reg, unsigned 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, unsigned int len) + u32 reg, unsigned int len) { readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len); } @@ -410,8 +414,8 @@ static void tegra_i2c_release_dma(struct tegra_i2c_dev *i2c_dev) static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) { struct dma_chan *chan; - u32 *dma_buf; dma_addr_t dma_phys; + u32 *dma_buf; int err; if (!i2c_dev->hw->has_apb_dma || i2c_dev->is_vi) @@ -577,12 +581,8 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) { - u32 val; + u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; int err; - u32 clk_divisor, clk_multiplier; - u32 non_hs_mode; - u32 tsu_thd; - u8 tlow, thigh; /* * The reset shouldn't ever fail in practice. The failure will be a @@ -704,11 +704,10 @@ static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev) static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) { - u32 val; - unsigned int rx_fifo_avail; - u8 *buf = i2c_dev->msg_buf; size_t buf_remaining = i2c_dev->msg_buf_remaining; - unsigned int words_to_transfer; + unsigned int words_to_transfer, rx_fifo_avail; + u8 *buf = i2c_dev->msg_buf; + u32 val; /* * Catch overflow due to message fully sent @@ -765,11 +764,10 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) { - u32 val; - unsigned int tx_fifo_avail; - u8 *buf = i2c_dev->msg_buf; size_t buf_remaining = i2c_dev->msg_buf_remaining; - unsigned int words_to_transfer; + unsigned int words_to_transfer, tx_fifo_avail; + u8 *buf = i2c_dev->msg_buf; + u32 val; if (i2c_dev->hw->has_mst_fifo) { val = i2c_readl(i2c_dev, I2C_MST_FIFO_STATUS); @@ -830,9 +828,9 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) static irqreturn_t tegra_i2c_isr(int irq, void *dev_id) { - u32 status; const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST; struct tegra_i2c_dev *i2c_dev = dev_id; + u32 status; status = i2c_readl(i2c_dev, I2C_INT_STATUS); @@ -935,12 +933,10 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id) static int tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, size_t len) { - u32 val, reg; - u8 dma_burst; struct dma_slave_config slv_config = {0}; + u32 val, reg, dma_burst, reg_offset; struct dma_chan *chan; int ret; - unsigned long reg_offset; if (i2c_dev->hw->has_mst_fifo) reg = I2C_MST_FIFO_CONTROL; @@ -1062,9 +1058,8 @@ static unsigned long tegra_i2c_wait_completion(struct tegra_i2c_dev *i2c_dev, static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap) { struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap); + u32 reg, time_left; int err; - unsigned long time_left; - u32 reg; reinit_completion(&i2c_dev->msg_complete); reg = FIELD_PREP(I2C_BC_SCLK_THRESHOLD, 9) | I2C_BC_STOP_COND | @@ -1178,11 +1173,10 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, struct i2c_msg *msg, enum msg_end_type end_state) { - u32 int_mask; - unsigned long time_left; + unsigned long time_left, xfer_time = 100; size_t xfer_size; - int err = 0; - u16 xfer_time = 100; + u32 int_mask; + int err; err = tegra_i2c_flush_fifos(i2c_dev); if (err) @@ -1340,8 +1334,7 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) { struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap); - int i; - int ret; + int i, ret; ret = pm_runtime_get_sync(i2c_dev->dev); if (ret < 0) { @@ -1601,8 +1594,8 @@ MODULE_DEVICE_TABLE(of, tegra_i2c_of_match); static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev) { struct device_node *np = i2c_dev->dev->of_node; - int ret; bool multi_mode; + int ret; ret = of_property_read_u32(np, "clock-frequency", &i2c_dev->bus_clk_rate);
Reorder definition of variables in the code to have them sorted by length and grouped logically, also replace "unsigned long" with "u32". Do this in order to make code easier to read. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/i2c/busses/i2c-tegra.c | 97 ++++++++++++++++------------------ 1 file changed, 45 insertions(+), 52 deletions(-)