Re: [PATCH v7 29/34] i2c: tegra: Improve formatting of variables

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3468 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]