Le 18/04/2023 à 12:09, Ryan Chen a écrit :
Hello Christophe,
Subject: Re: [PATCH v10 2/2] i2c: aspeed: support ast2600 i2c new register
mode driver
(resending, my mail client removed some addresses. Sorry for the duplicated
message for the others)
> + dev_dbg(i2c_bus->dev, "Recovery done [%x]\n",
> + readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
> + if (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) &
AST2600_I2CC_BUS_BUSY_STS) {
> + dev_dbg(i2c_bus->dev, "Can't recovery bus [%x]\n",
recover?
Sorry, Do you mean modify the wording "Can’t recover bus"?
Yes.
English is not my native language but "Can't recovery bus" sounds
strange to me.
> +master_out:
> + if (i2c_bus->mode == DMA_MODE) {
> + kfree(i2c_bus->master_safe_buf);
> + i2c_bus->master_safe_buf = NULL;
Alignement.
Sorry, I can't catch the alignment could you help point out or give me example?
2 tabs in front of kfree
1 tab + 4 spaces in front of i2c_bus->master_safe_buf
when tabs are configures as 8 spaces, it looks strange.
> +
> + writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> + i2c_bus->slave = client;
> + /* Set slave addr. */
> + writel(client->addr | AST2600_I2CS_ADDR1_ENABLE,
> + i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
Nit: alignement
Sorry, what should I do for alignment.
1 tab in front of writel
3 tabs in front of i2c_bus->reg_base
when tabs are configures as 8 spaces, it looks strange.
Look at the writel just a lines below. These ones look nice :)
> + /* i2c timeout counter: use base clk4 1Mhz,
> + * per unit: 1/(1000/4096) = 4096us
> + */
> + ret = of_property_read_u32(dev->of_node,
> + "i2c-scl-clk-low-timeout-us",
> + &i2c_bus->timeout);
Strange layout.
Sorry, could you point out the strange? It just property read for timeout.
"i2c-scl-clk-low-timeout-us" could fit on the previous line (but up to
you to do it or not).
The 2 last lines are way to much on the right when tabs are 8 spaces.
> + if (ret < 0)
> + i2c_bus->timeout = 0;
Nit: This is not really needed since i2c_bus is kzalloc()'ed.
Got it, will remove it.
I would say that it is a matter of taste. If it improves reading, I
think that it is fine as-is. If you want to save a few lines of code, it
can be removed.
CJ
