Add ASPEED AST2600 I2Cv2 controller driver
Subsystems (4)
Ryan Chen
maintainerv11 cover Add ASPEED AST2600 I2Cv2 controller driver
This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new register set that have new clock divider option for more flexiable generation. And also have separate i2c master and slave register set for control.
The legacy register layout is mix master/slave register control together. The following is add more detail description about new register layout. And new feature set add for register.
-Add new clock divider option for more flexible and accurate clock rate generation -Add tCKHighMin timing to guarantee SCL high pulse width. -Add support dual pool buffer mode, split 32 bytes pool buffer of each device into 2 x 16 bytes for Tx and Rx individually. -Increase DMA buffer size to 4096 bytes and support byte alignment. -Re-define the base address of BUS1 ~ BUS16 and Pool buffer. -Re-define registers for separating master and slave mode control. -Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx.
And following is new register set for package transfer sequence. -New Master operation mode:
S -> Aw -> P
S -> Aw -> TxD -> P
S -> Ar -> RxD -> P
S -> Aw -> RxD -> Sr -> Ar -> TxD -> P
-Bus SDA lock auto-release capability for new master DMA command mode. -Bus auto timeout for new master/slave DMA mode.
The following is two versus register layout.
Old:
{I2CD00}: Function Control Register
{I2CD04}: Clock and AC Timing Control Register {I2CD08}: Clock and AC Timing Control Register {I2CD0C}: Interrupt Control Register
{I2CD10}: Interrupt Status Register
{I2CD14}: Command/Status Register
{I2CD18}: Slave Device Address Register
{I2CD1C}: Pool Buffer Control Register
{I2CD20}: Transmit/Receive Byte Buffer Register {I2CD24}: DMA Mode Buffer Address Register
{I2CD28}: DMA Transfer Length Register
{I2CD2C}: Original DMA Mode Buffer Address Setting {I2CD30}: Original DMA Transfer Length Setting and Final Status
New Register mode
{I2CC00}: Master/Slave Function Control Register {I2CC04}: Master/Slave Clock and AC Timing Control Register {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register {I2CC0C}: Master/Slave Pool Buffer Control Register {I2CM10}: Master Interrupt Control Register {I2CM14}: Master Interrupt Status Register {I2CM18}: Master Command/Status Register
{I2CM1C}: Master DMA Buffer Length Register {I2CS20}: Slave~ Interrupt Control Register {I2CS24}: Slave~ Interrupt Status Register {I2CS28}: Slave~ Command/Status Register
{I2CS2C}: Slave~ DMA Buffer Length Register {I2CM30}: Master DMA Mode Tx Buffer Base Address {I2CM34}: Master DMA Mode Rx Buffer Base Address {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address {I2CS40}: Slave Device Address Register
{I2CM48}: Master DMA Length Status Register {I2CS4C}: Slave DMA Length Status Register {I2CC50}: Current DMA Operating Address Status {I2CC54}: Current DMA Operating Length Status
aspeed,global-regs:
This global register is needed, global register is setting for new clock divide control, and new register set control.
ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug. The following is board-specific design example.
Board A Board B
------------------------- ------------------------
|i2c bus#1(master/slave) <===fingerprint ===> i2c bus#x (master/slave)| |i2c bus#2(master)-> tmp i2c device | | | |i2c bus#3(master)-> adc i2c device | | | ------------------------- ------------------------
i2c-scl-clk-low-timeout-us:
For example I2C controller as slave mode, and suddenly disconnected. Slave state machine will keep waiting for master clock in for rx/tx transmit. So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL.
aspeed,enable-dma:
For example The bus#1 have trunk data needed for transfer, it can enable bus dma mode transfer, it can reduce cpu utilized. Others bus bus#2/3 use defautl buffer mode.
v11:
-aspeed,i2c.yaml
-no change, the same with v10.-i2c-ast2600.c
-modify alert_enable from int -> boolean.
-modify dbg string recovery -> recover.
-remove no need to init 0.
-remove new line after break.
-remove unneeded empty line.
-modify dma_alloc_coherent to dmam_alloc_coherent
-modify probe nomem return dev_err_probe
-modify i2c_add_adapter to devm_i2c_adapter
-modify checkpatch: Alignment should match open parenthesis -modify checkpatch: braces {} should be used on all arms of this statement -modify checkpatch: Unbalanced braces around else statement
v10:
-aspeed,i2c.yaml
-move unevaluatedProperties after allOf.
-remove extra one blank line.-i2c-ast2600.c
-no change, the same with v8.
v9:
-aspeed,i2c.yaml
-backoff to v7.
-no fix typo in maintainer's name and email. this would be another patch. -no remove address-cells, size-cells, this would be another patch.
-use aspeed,enable-dma property instead of aspeed,xfer-mode selection. -fix allOf and else false properties for aspeed,ast2600-i2cv2.-i2c-ast2600.c
-no change, the same with v8
v8:
-aspeed,i2c.yaml
-modify commit message.
-Fix typo in maintainer's name and email.
-remove address-cells, size-cells.-i2c-ast2600.c
-move "i2c timeout counter" comment description before property_read. -remove redundant code "return ret" in probe end.
v7:
-aspeed,i2c.yaml
-Update ASPEED I2C maintainers email.
-use aspeed,enable-dma property instead of aspeed,xfer-mode selection. -fix allOf and else false properties for aspeed,ast2600-i2cv2.-i2c-ast2600.c
-remove aspeed,xfer-mode instead of aspeed,enable-dma mode. buffer mode
is default.
-remove aspeed,timeout instead of i2c-scl-clk-low-timeout-us for
timeout setting.
v6:
-remove aspeed,i2cv2.yaml, merge to aspeed,i2c.yaml -add support for
i2cv2 properites.-i2c-ast2600.c
-fix ast2600_i2c_remove ordering.
-remove ast2600_i2c_probe goto labels, and add dev_err_probe -remove
redundant deb_dbg debug message.
-rename gr_regmap -> global_regs
v5:
-remove ast2600-i2c-global.yaml, i2c-ast2600-global.c. -i2c-ast2600.c
-remove legacy clock divide, all go for new clock divide. -remove duplicated read isr.
-remove no used driver match
-fix probe return for each labels return.
-global use mfd driver, driver use phandle to regmap read/write.
-rename aspeed,i2c-ast2600.yaml to aspeed,i2cv2.yaml -remove bus-frequency. -add required aspeed,gr
-add timeout, byte-mode, buff-mode properites.
v4:
-fix i2c-ast2600.c driver buffer mode use single buffer conflit in
master slave mode both enable.
-fix kmemleak issue when use dma mode.
-fix typo aspeed,i2c-ast2600.yaml compatible is "aspeed,ast2600-i2c" -fix typo aspeed,i2c-ast2600.ymal to aspeed,i2c-ast2600.yaml
v3:
-fix i2c global clock divide default value.
-remove i2c slave no used dev_dbg info.
v2:
-add i2c global ymal file commit.
-rename file name from new to ast2600.
aspeed-i2c-new-global.c -> i2c-ast2600-global.c
aspeed-i2c-new-global.h -> i2c-ast2600-global.h
i2c-new-aspeed.c -> i2c-ast2600.c
-rename all driver function name to ast2600.
Ryan Chen (2):
dt-bindings: i2c: aspeed: support for AST2600-i2cv2
i2c: aspeed: support ast2600 i2c new register mode driver
.../devicetree/bindings/i2c/aspeed,i2c.yaml | 51 +- MAINTAINERS | 9 + drivers/i2c/busses/Kconfig | 11 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-ast2600.c | 1596 +++++++++++++++++ 5 files changed, 1665 insertions(+), 3 deletions(-) create mode 100644 drivers/i2c/busses/i2c-ast2600.c
Signature · 1 line
2.34.1
Ryan Chen
maintainerPATCH v11 1/2 dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma and description for ast2600-i2cv2.
--- .../devicetree/bindings/i2c/aspeed,i2c.yaml | 51 +++++++++++++++++-- 1 file changed, 48 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +48 -3
@@ -9,9 +9,6 @@ title: ASPEED I2C on the AST24XX, AST25XX, and AST26XX SoCs Device Tree Bindings maintainers: - Rayn Chen <[email protected]> -allOf: - - $ref: /schemas/i2c/i2c-controller.yaml# - properties: compatible: enum:
@@ -49,12 +46,50 @@ properties: description: states that there is another master active on this bus + aspeed,enable-dma: + type: boolean + description: | + I2C bus enable dma mode transfer. + + ASPEED ast2600 platform equipped with 16 I2C controllers that share a + single DMA engine. DTS files can specify the data transfer mode to/from + the device, either DMA or programmed I/O. However, hardware limitations + may require a DTS to manually allocate which controller can use DMA mode. + The "aspeed,enable-dma" property allows control of this. + + In cases where one the hardware design results in a specific + controller handling a larger amount of data, a DTS would likely + enable DMA mode for that one controller. + + aspeed,global-regs: + $ref: /schemas/types.yaml#/definitions/phandle + description: The phandle of i2c global register node. + required: - reg - compatible - clocks - resets +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + - if: + properties: + compatible: + contains: + const: aspeed,ast2600-i2cv2 + + then: + properties: + reg: + minItems: 2 + required: + - aspeed,global-regs + else: + properties: + aspeed,global-regs: false + aspeed,enable-dma: false + unevaluatedProperties: false examples:
@@ -71,3 +106,13 @@ examples: interrupts = <0>; interrupt-parent = <&i2c_ic>; }; + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + i2c1: i2c at 80 { + compatible = "aspeed,ast2600-i2cv2"; + reg = <0x80 0x80>, <0xc00 0x20>; + aspeed,global-regs = <&i2c_global>; + clocks = <&syscon ASPEED_CLK_APB>; + resets = <&syscon ASPEED_RESET_I2C>; + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>; + };
Signature · 1 line
2.34.1
Re: [PATCH v11 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 4 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma and description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
This is a friendly reminder during the review process.
It looks like you received a tag and forgot to add it.
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply.
https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submitting-patches.rst#L540
If a tag was not added on purpose, please state why and what changed.
Best regards,
Krzysztof
Ryan Chen
maintainerRE: [PATCH v11 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Hello Krzysztof,
Quoted 18 lines
Subject: Re: [PATCH v11 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 4 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma and description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>This is a friendly reminder during the review process.
It looks like you received a tag and forgot to add it.
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply.
Quoted 5 lines
https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submittin
g-patches.rst#L540If a tag was not added on purpose, please state why and what changed.
Sorry, My mistake. May I ask a question?
In this situation, should I submit with new version? Or I just add Review-by and send it again?
Best regards,
Ryan Chen
Re: [PATCH v11 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 20 lines · Ryan Chen
Hello Krzysztof,
Quoted 18 lines
Subject: Re: [PATCH v11 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 4 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma and description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>This is a friendly reminder during the review process.
It looks like you received a tag and forgot to add it.
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply.
Quoted 8 lines
Quoted 5 lines
https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submittin
g-patches.rst#L540If a tag was not added on purpose, please state why and what changed.
Sorry, My mistake. May I ask a question?
In this situation, should I submit with new version? Or I just add Review-by and send it again?
Please send a new version.
Best regards,
Krzysztof
Ryan Chen
maintainerPATCH v11 2/2 i2c: aspeed: support ast2600 i2c new register mode driver
In reply to Ryan ChenAdd i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.
--- MAINTAINERS | 9 + drivers/i2c/busses/Kconfig | 11 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-ast2600.c | 1596 ++++++++++++++++++++++++++++++ 4 files changed, 1617 insertions(+) create mode 100644 drivers/i2c/busses/i2c-ast2600.c
MAINTAINERS +9 -0
@@ -1924,6 +1924,15 @@ F: Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic. F: drivers/i2c/busses/i2c-aspeed.c F: drivers/irqchip/irq-aspeed-i2c-ic.c +ARM/ASPEED I2CV2 DRIVER +M: Ryan Chen <[email protected]> +R: Andrew Jeffery <[email protected]> +L: linux-i2c at vger.kernel.org +L: openbmc at lists.ozlabs.org (moderated for non-subscribers) +S: Maintained +F: Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +F: drivers/i2c/busses/i2c-ast2600.c + ARM/ASPEED MACHINE SUPPORT M: Joel Stanley <[email protected]> R: Andrew Jeffery <[email protected]>
drivers/i2c/busses/Kconfig +11 -0
@@ -389,6 +389,17 @@ config I2C_ALTERA This driver can also be built as a module. If so, the module will be called i2c-altera. +config I2C_AST2600 + tristate "Aspeed I2C v2 Controller" + depends on ARCH_ASPEED || COMPILE_TEST + select I2C_SMBUS + help + If you say yes to this option, support will be included for the + Aspeed I2C controller with new register set. + + This driver can also be built as a module. If so, the module + will be called i2c-ast2600. + config I2C_ASPEED tristate "Aspeed I2C Controller" depends on ARCH_ASPEED || COMPILE_TEST
drivers/i2c/busses/Makefile +1 -0
@@ -38,6 +38,7 @@ obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o obj-$(CONFIG_I2C_ALTERA) += i2c-altera.o obj-$(CONFIG_I2C_AMD_MP2) += i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o +obj-$(CONFIG_I2C_AST2600) += i2c-ast2600.o obj-$(CONFIG_I2C_AT91) += i2c-at91.o i2c-at91-objs := i2c-at91-core.o i2c-at91-master.o ifeq ($(CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL),y)
drivers/i2c/busses/i2c-ast2600.c new file +1596 -0
1,596 lines not on this page · raw message
Signature · 1 line
2.34.1
Re: [PATCH v11 2/2] i2c: aspeed: support ast2600 i2c new register mode driver
Quoted 6 lines · Ryan Chen
Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.
...
Quoted 7 lines
drivers/i2c/busses/i2c-ast2600.c new file +7
-0+#include <linux/clk.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/reset.h>
drivers/i2c/busses/i2c-ast2600.c line 23
Quoted 10 lines
drivers/i2c/busses/i2c-ast2600.c new file +10
-0+#include <linux/module.h> +#include <linux/interrupt.h> +#include <linux/completion.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> +#include <linux/of_device.h> +#include <linux/dma-mapping.h> +#include <linux/i2c-smbus.h>
Ordered?
...
drivers/i2c/busses/i2c-ast2600.c line 53
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+#define AST2600_GLOBAL_INIT \ + (AST2600_I2CG_CTRL_NEW_REG | \ + AST2600_I2CG_CTRL_NEW_CLK_DIV)
Make just a one TAB and put the last two lines on the single one.
...
Quoted 1 line
+#define
I2CCG_DIV_CTRL0xC6411208
Is it decimal? Is it combination of bitfields? Can you add a comment what is this magic?
...
drivers/i2c/busses/i2c-ast2600.c line 246
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +4
-0+struct ast2600_i2c_timing_table { +u32 divisor; +u32 timing; +};
Is it even used?
...
drivers/i2c/busses/i2c-ast2600.c line 249
Quoted 2 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+enum xfer_mode { +BYTE_MODE =0 ,
Why explicit assignment?
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+BUFF_MODE , + DMA_MODE, +};
...
drivers/i2c/busses/i2c-ast2600.c line 319
Quoted 7 lines
drivers/i2c/busses/i2c-ast2600.c new file +7
-0+ base_clk1 = (i2c_bus->apb_clk *10 ) / ((((clk_div_reg &0xff ) +2 ) *10 ) /2 ); + base_clk2 = (i2c_bus->apb_clk *10 ) / + (((((clk_div_reg >>8 ) &0xff ) +2 ) *10 ) /2 ); + base_clk3 = (i2c_bus->apb_clk *10 ) / + (((((clk_div_reg >>16 ) &0xff ) +2 ) *10 ) /2 ); + base_clk4 = (i2c_bus->apb_clk *10 ) / + (((((clk_div_reg >>24 ) &0xff ) +2 ) *10 ) /2 );
The same equation is used per each byte of clk_div_reg? Can it be rewritten to avoid this and using loop, so you will have an array of base_clk to be filled?
Don't forget to use GENMASK().
...
drivers/i2c/busses/i2c-ast2600.c line 332
Quoted 12 lines
drivers/i2c/busses/i2c-ast2600.c new file +12
-0+if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <=32 ) { + baseclk_idx =0 ; + divisor =DIV_ROUND_UP (i2c_bus->apb_clk, i2c_bus->bus_frequency); + }else if ((base_clk1 / i2c_bus->bus_frequency) <=32 ) { + baseclk_idx =1 ; + divisor =DIV_ROUND_UP (base_clk1, i2c_bus->bus_frequency); + }else if ((base_clk2 / i2c_bus->bus_frequency) <=32 ) { + baseclk_idx =2 ; + divisor =DIV_ROUND_UP (base_clk2, i2c_bus->bus_frequency); + }else if ((base_clk3 / i2c_bus->bus_frequency) <=32 ) { + baseclk_idx =3 ; + divisor =DIV_ROUND_UP (base_clk3, i2c_bus->bus_frequency);
Will be optimized with above suggestion, I believe.
drivers/i2c/busses/i2c-ast2600.c line 342
Quoted 10 lines
drivers/i2c/busses/i2c-ast2600.c new file +10
-0+ }else { + baseclk_idx =4 ; + divisor =DIV_ROUND_UP (base_clk4, i2c_bus->bus_frequency); + inc =0 ; +while ((divisor + inc) >32 ) { + inc |= divisor &0x1 ; + divisor >>=1 ; + baseclk_idx++; + } + divisor += inc;
I think the above loop can be rewritten to have better representation.
Quoted 1 line
(quoted diff) +1
-0+ }
...
drivers/i2c/busses/i2c-ast2600.c line 345
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ baseclk_idx &=0xf ;
GENMASK()?
...
drivers/i2c/busses/i2c-ast2600.c line 347
Quoted 2 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+ scl_low = ((divisor *9 ) /16 ) -1 ; + scl_low =min_t (u32, scl_low,0xf );
This can be done in one line. Also, why not 15?
...
drivers/i2c/busses/i2c-ast2600.c line 348
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ scl_high = (divisor - scl_low -2 ) &0xf ;
GENMASK()?
...
drivers/i2c/busses/i2c-ast2600.c line 350
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ data = ((scl_high -1 ) <<20 ) | (scl_high <<16 ) | (scl_low <<12 ) | (baseclk_idx);
Too many parentheses.
...
drivers/i2c/busses/i2c-ast2600.c line 549
Quoted 2 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+/* due to master slave is common buffer, so need force the master stop not issue */ +if (readl (i2c_bus->reg_base +AST2600_I2CM_CMD_STS ) &0xffff ) {
GENMASK() ?
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c new file +5
-0+writel (0 , i2c_bus->reg_base +AST2600_I2CM_CMD_STS ); + i2c_bus->cmd_err = -EBUSY ; +writel (0 , i2c_bus->reg_base +AST2600_I2CC_BUFF_CTRL ); +complete (&i2c_bus->cmd_complete); + }
...
drivers/i2c/busses/i2c-ast2600.c line 796
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+/* send start */ + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", + i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write",
str_read_write() ?
Quoted 2 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+ msg->len, msg->len >1 ?"s" :"" , + msg->flags & I2C_M_RD ? "from" : "to", msg->addr);
...
drivers/i2c/busses/i2c-ast2600.c line 914
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +4
-0+for (i =0 ; i < xfer_len; i++) { + wbuf[i %4 ] = msg->buf[i]; +if (i %4 ==3 ) +writel (*(u32 *)wbuf, i2c_bus->buf_base + i -3 );
Err.. There can be alignment issues.
drivers/i2c/busses/i2c-ast2600.c line 917
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+ } +if (--i %4 !=3 ) +writel (*(u32 *)wbuf, i2c_bus->buf_base + i - (i %4 ));
The above code is ugly. Can you think about it and write in a better way?
...
drivers/i2c/busses/i2c-ast2600.c line 1047
Quoted 7 lines
drivers/i2c/busses/i2c-ast2600.c new file +7
-0+for (i =0 ; i < xfer_len; i++) { + wbuf[i %4 ] = msg->buf[i2c_bus->master_xfer_cnt + i]; +if (i %4 ==3 ) +writel (*(u32 *)wbuf, i2c_bus->buf_base + i -3 ); + } +if (--i %4 !=3 ) +writel (*(u32 *)wbuf, i2c_bus->buf_base + i - (i %4 ));
Ditto.
...
Do you have similar code pieces? Why not doing it in a separate function with parameters?
...
drivers/i2c/busses/i2c-ast2600.c line 1225
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+return ast2600_i2c_master_irq (i2c_bus) ?IRQ_HANDLED :IRQ_NONE ;
IRQ_RETVAL() ?
...
drivers/i2c/busses/i2c-ast2600.c line 1338
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+writel (0xfffffff , i2c_bus->reg_base +AST2600_I2CM_ISR );
GENMASK()
...
drivers/i2c/busses/i2c-ast2600.c line 1350
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+writel (0xfffffff , i2c_bus->reg_base +AST2600_I2CS_ISR );
Ditto.
drivers/i2c/busses/i2c-ast2600.c line 1353
Quoted 2 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+if (i2c_bus->mode ==BYTE_MODE ) { +writel (0xffff , i2c_bus->reg_base +AST2600_I2CS_IER );
Ditto.
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +4
-0+ }else { +/* Set interrupt generation of I2C slave controller */ +writel (AST2600_I2CS_PKT_DONE , i2c_bus->reg_base +AST2600_I2CS_IER ); + }
...
drivers/i2c/busses/i2c-ast2600.c line 1402
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+WARN_ON (!i2c_bus->slave);
Why?
...
drivers/i2c/busses/i2c-ast2600.c line 1436
Quoted 8 lines
drivers/i2c/busses/i2c-ast2600.c new file +7
-0+static const struct of_device_id ast2600_i2c_bus_of_table[] = { + { + .compatible ="aspeed,ast2600-i2cv2" , + }, + {} +}; +
Redundant blank line.
Quoted 1 line
+MODULE_DEVICE_TABLE(of,
ast2600_i2c_bus_of_table);
...
drivers/i2c/busses/i2c-ast2600.c line 1450
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+ i2c_bus =devm_kzalloc (dev,sizeof (*i2c_bus),GFP_KERNEL ); +if (!i2c_bus) +return dev_err_probe (dev, -ENOMEM ,"no memory allocated\n" );
No. We do not print error message for ENOMEM.
You homework to find why.
...
drivers/i2c/busses/i2c-ast2600.c line 1476
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+if (of_property_read_bool (pdev->dev.of_node,"aspeed,enable-dma" ))
device_property_read_bool() ?
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ i2c_bus->mode =DMA_MODE ;
...
drivers/i2c/busses/i2c-ast2600.c line 1489
Quoted 11 lines
drivers/i2c/busses/i2c-ast2600.c new file +11
-0+if (i2c_bus->mode ==BUFF_MODE ) { + res =platform_get_resource (pdev,IORESOURCE_MEM ,1 ); +if (res &&resource_size (res) >=2 ) { + i2c_bus->buf_base =devm_ioremap_resource (dev, res); + +if (!IS_ERR_OR_NULL (i2c_bus->buf_base)) + i2c_bus->buf_size =resource_size (res) /2 ; + }else { + i2c_bus->mode =BYTE_MODE ; + } + }
Can be done without additional checks and with a simple call to devm_platform_ioremap_resource(). No?
...
drivers/i2c/busses/i2c-ast2600.c line 1493
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+/* i2c timeout counter: use base clk4 1Mhz, +* per unit: 1/(1000/4096) = 4096us +*/
Wrong multi-line style of the comment.
...
drivers/i2c/busses/i2c-ast2600.c line 1498
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c new file +5
-0+ ret =of_property_read_u32 (dev->of_node, +"i2c-scl-clk-low-timeout-us" , + &i2c_bus->timeout); +if (!ret) + i2c_bus->timeout /=4096 ;
What is this and why I2C core timings (standard) can't be utilized here?
...
drivers/i2c/busses/i2c-ast2600.c line 1518
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c new file +5
-0+ ret =of_property_read_u32 (dev->of_node,"clock-frequency" , &i2c_bus->bus_frequency); +if (ret <0 ) { +dev_warn (dev,"Could not read clock-frequency property\n" ); + i2c_bus->bus_frequency =100000 ; + }
There are macro for standard speeds. Moreover, there is a function to parse properties, no need to open code.
...
drivers/i2c/busses/i2c-ast2600.c line 1525
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ i2c_bus->adap.dev.of_node = dev->of_node;
device_set_node()
...
drivers/i2c/busses/i2c-ast2600.c line 1537
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+if (of_property_read_bool (dev->of_node,"smbus-alert" )) {
Doesn't core have already support for this?
Quoted 13 lines
drivers/i2c/busses/i2c-ast2600.c new file +13
-0+ i2c_bus->alert_enable = true; + i2c_bus->ara =i2c_new_smbus_alert_device (&i2c_bus->adap, &i2c_bus->alert_data); +if (!i2c_bus->ara) +dev_warn (dev,"Failed to register ARA client\n" ); + +writel (AST2600_I2CM_PKT_DONE |AST2600_I2CM_BUS_RECOVER |AST2600_I2CM_SMBUS_ALT , + i2c_bus->reg_base +AST2600_I2CM_IER ); + }else { + i2c_bus->alert_enable = false; +/* Set interrupt generation of I2C master controller */ +writel (AST2600_I2CM_PKT_DONE |AST2600_I2CM_BUS_RECOVER , + i2c_bus->reg_base +AST2600_I2CM_IER ); + }
...
drivers/i2c/busses/i2c-ast2600.c line 1558
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+dev_info (dev,"%s [%d]: adapter [%d khz] mode [%d]\n" , + dev->of_node->name, i2c_bus->adap.nr, i2c_bus->bus_frequency /1000 , + i2c_bus->mode);
Useless noise.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Ryan Chen
maintainerRE: [PATCH v11 2/2] i2c: aspeed: support ast2600 i2c new register mode driver
Hello Andy,
Quoted 21 lines · Ryan Chen
Quoted 5 lines
Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.
...
Quoted 11 lines
drivers/i2c/busses/i2c-ast2600.c new file +11
-0+#include <linux/clk.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/reset.h> +#include <linux/module.h> +#include <linux/interrupt.h> +#include <linux/completion.h> +#include <linux/of.h>
Quoted 8 lines
drivers/i2c/busses/i2c-ast2600.c line 23
Quoted 6 lines
drivers/i2c/busses/i2c-ast2600.c new file +6
-0+#include <linux/of_irq.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> +#include <linux/of_device.h> +#include <linux/dma-mapping.h> +#include <linux/i2c-smbus.h> Ordered?
Update by alphabetical order
#include <linux/clk.h>
#include <linux/completion.h>
#include <linux/delay.h>
#include <linux/dma-mapping.h>
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/i2c-smbus.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/mfd/syscon.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/of_irq.h>
#include <linux/regmap.h>
#include <linux/reset.h>
#include <linux/slab.h>
Quoted 7 lines
...
drivers/i2c/busses/i2c-ast2600.c line 53
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+#define AST2600_GLOBAL_INIT \ + (AST2600_I2CG_CTRL_NEW_REG | \ + AST2600_I2CG_CTRL_NEW_CLK_DIV)Make just a one TAB and put the last two lines on the single one.
Update by following.
#define AST2600_GLOBAL_INIT \ (AST2600_I2CG_CTRL_NEW_REG | \ AST2600_I2CG_CTRL_NEW_CLK_DIV)
Quoted 7 lines
...
Quoted 1 line
+#define
I2CCG_DIV_CTRL0xC6411208Is it decimal? Is it combination of bitfields? Can you add a comment what is this magic?
This have been comment in previous line. I will move above of this magic code. /*
* APB clk : 100Mhz
* div : scl : baseclk [APB/((div/2) + 1)] : tBuf [1/bclk * 16]
* I2CG10[31:24] base clk4 for i2c auto recovery timeout counter (0xC6)
* I2CG10[23:16] base clk3 for Standard-mode (100Khz) min tBuf 4.7us
* 0x3c : 100.8Khz : 3.225Mhz : 4.96us
* 0x3d : 99.2Khz : 3.174Mhz : 5.04us
* 0x3e : 97.65Khz : 3.125Mhz : 5.12us
* 0x40 : 97.75Khz : 3.03Mhz : 5.28us
* 0x41 : 99.5Khz : 2.98Mhz : 5.36us (default)
* I2CG10[15:8] base clk2 for Fast-mode (400Khz) min tBuf 1.3us
* 0x12 : 400Khz : 10Mhz : 1.6us
* I2CG10[7:0] base clk1 for Fast-mode Plus (1Mhz) min tBuf 0.5us
* 0x08 : 1Mhz : 20Mhz : 0.8us
*/
Quoted 9 lines
...
drivers/i2c/busses/i2c-ast2600.c line 246
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +4
-0+struct ast2600_i2c_timing_table { +u32 divisor; +u32 timing; +};Is it even used?
Will remove.
Quoted 6 lines
...
drivers/i2c/busses/i2c-ast2600.c line 249
Quoted 2 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+enum xfer_mode { +BYTE_MODE =0 ,Why explicit assignment?
Will remove.
Quoted 19 lines
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+BUFF_MODE , + DMA_MODE, +};...
Review
Quoted 1 line
(quoted diff) +1
-0+ base_clk1 = (i2c_bus->apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) * 10) /2);
drivers/i2c/busses/i2c-ast2600.c line 319
Quoted 6 lines
drivers/i2c/busses/i2c-ast2600.c new file +6
-0+ base_clk2 = (i2c_bus->apb_clk *10 ) / + (((((clk_div_reg >>8 ) &0xff ) +2 ) *10 ) /2 ); + base_clk3 = (i2c_bus->apb_clk *10 ) / + (((((clk_div_reg >>16 ) &0xff ) +2 ) *10 ) /2 ); + base_clk4 = (i2c_bus->apb_clk *10 ) / + (((((clk_div_reg >>24 ) &0xff ) +2 ) *10 ) /2 );The same equation is used per each byte of
clk_div_reg? Can it be rewritten to avoid this and using loop, so you will have an array ofbase_clkto be filled?
Quoted 2 lines
Don't forget to use
GENMASK().
Will update for loop.
for (i = 0; i < AST2600_NUM_CLK_DIV_BYTES; i++) {
u32 byte_val = (clk_div_reg >> (i * 8)) & GENMASK(7, 0);
base_clk[i] = (i2c_bus->apb_clk * 10) / ((byte_val + 2) * 5);
}Quoted 6 lines
...
drivers/i2c/busses/i2c-ast2600.c
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <=32 ) { + baseclk_idx =0 ; + divisor = DIV_ROUND_UP(i2c_bus->apb_clk,
i2c_bus->bus_frequency);
Quoted 11 lines
drivers/i2c/busses/i2c-ast2600.c line 332
Quoted 9 lines
drivers/i2c/busses/i2c-ast2600.c new file +9
-0+ }else if ((base_clk1 / i2c_bus->bus_frequency) <=32 ) { + baseclk_idx =1 ; + divisor =DIV_ROUND_UP (base_clk1, i2c_bus->bus_frequency); + }else if ((base_clk2 / i2c_bus->bus_frequency) <=32 ) { + baseclk_idx =2 ; + divisor =DIV_ROUND_UP (base_clk2, i2c_bus->bus_frequency); + }else if ((base_clk3 / i2c_bus->bus_frequency) <=32 ) { + baseclk_idx =3 ; + divisor =DIV_ROUND_UP (base_clk3, i2c_bus->bus_frequency);Will be optimized with above suggestion, I believe.
Will up with previous
Quoted 14 lines
drivers/i2c/busses/i2c-ast2600.c line 342
Quoted 11 lines
drivers/i2c/busses/i2c-ast2600.c new file +10
-0+ }else { + baseclk_idx =4 ; + divisor =DIV_ROUND_UP (base_clk4, i2c_bus->bus_frequency); + inc =0 ; +while ((divisor + inc) >32 ) { + inc |= divisor &0x1 ; + divisor >>=1 ; + baseclk_idx++; + } + divisor += inc;I think the above loop can be rewritten to have better representation.
Will up with previous
Quoted 8 lines
Quoted 1 line
(quoted diff) +1
-0+ }...
drivers/i2c/busses/i2c-ast2600.c line 345
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ baseclk_idx &=0xf ;
GENMASK()?
Will update
Quoted 7 lines
...
drivers/i2c/busses/i2c-ast2600.c line 347
Quoted 2 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+ scl_low = ((divisor *9 ) /16 ) -1 ; + scl_low =min_t (u32, scl_low,0xf );This can be done in one line. Also, why not 15?
Quoted 5 lines
...
drivers/i2c/busses/i2c-ast2600.c line 348
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ scl_high = (divisor - scl_low -2 ) &0xf ;
GENMASK()?
Will update
Quoted 7 lines
...
Review
Quoted 2 lines
(quoted diff) +2
-0+ data = ((scl_high - 1) << 20) | (scl_high << 16) | (scl_low << 12) | +(baseclk_idx);Too many parentheses.
Update to
data = ((scl_high - 1) << 20) | (scl_high << 16) | (scl_low << 12) | baseclk_idx;
Quoted 17 lines
...
Review
Quoted 1 line
(quoted diff) +1
-0+ /* due to master slave is common buffer, so need force the master stopnot issue */
drivers/i2c/busses/i2c-ast2600.c line 549
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+if (readl (i2c_bus->reg_base +AST2600_I2CM_CMD_STS ) &0xffff ) {
GENMASK()?Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c new file +5
-0+writel (0 , i2c_bus->reg_base +AST2600_I2CM_CMD_STS ); + i2c_bus->cmd_err = -EBUSY ; +writel (0 , i2c_bus->reg_base +AST2600_I2CC_BUFF_CTRL ); +complete (&i2c_bus->cmd_complete); + }...
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c line 796
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+/* send start */ + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", + i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write",
str_read_write()?
Sorry do you mean there have a function call str_read_write? Can you point me where it is for refer?
Quoted 6 lines
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+ msg->len, msg->len >1 ?"s" :"" , + msg->flags & I2C_M_RD ? "from" : "to", msg->addr);...
Quoted 6 lines
drivers/i2c/busses/i2c-ast2600.c line 914
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +4
-0+for (i =0 ; i < xfer_len; i++) { + wbuf[i %4 ] = msg->buf[i]; +if (i %4 ==3 ) +writel (*(u32 *)wbuf, i2c_bus->buf_base + i -3 );Err.. There can be alignment issues.
Will update
Quoted 6 lines
drivers/i2c/busses/i2c-ast2600.c line 917
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+ } +if (--i %4 !=3 ) +writel (*(u32 *)wbuf, i2c_bus->buf_base + i - (i %4 ));The above code is ugly. Can you think about it and write in a better way?
Sorry, that is because the register only support for 4 byte align write. That the reason I need put for byte write to 4 byte align write.
Quoted 12 lines
...
drivers/i2c/busses/i2c-ast2600.c line 1047
Quoted 7 lines
drivers/i2c/busses/i2c-ast2600.c new file +7
-0+for (i =0 ; i < xfer_len; i++) { + wbuf[i %4 ] = msg->buf[i2c_bus->master_xfer_cnt + i]; +if (i %4 ==3 ) +writel (*(u32 *)wbuf, i2c_bus->buf_base + i -3 ); + } +if (--i %4 !=3 ) +writel (*(u32 *)wbuf, i2c_bus->buf_base + i - (i %4 ));Ditto.
Will update
Quoted 6 lines
...
Do you have similar code pieces? Why not doing it in a separate function with parameters?
I will think it to be MICRO function call.
Quoted 5 lines
...
drivers/i2c/busses/i2c-ast2600.c line 1225
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+return ast2600_i2c_master_irq (i2c_bus) ?IRQ_HANDLED :IRQ_NONE ;
IRQ_RETVAL()?
Sorry, most return is handled or not handled. Do you mean replace it just " return IRQ_RETVAL(ret);"
Quoted 7 lines
...
drivers/i2c/busses/i2c-ast2600.c line 1338
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+writel (0xfffffff , i2c_bus->reg_base +AST2600_I2CM_ISR );
GENMASK()
Will Update
Quoted 5 lines
...
drivers/i2c/busses/i2c-ast2600.c line 1350
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+writel (0xfffffff , i2c_bus->reg_base +AST2600_I2CS_ISR );Ditto.
Will Update
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c line 1353
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+if (i2c_bus->mode ==BYTE_MODE ) { +writel (0xffff , i2c_bus->reg_base +AST2600_I2CS_IER );Ditto.
Will Update
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+ }else { +/* Set interrupt generation of I2C slave controller */ + writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
AST2600_I2CS_IER);
Quoted 7 lines
Quoted 1 line
(quoted diff) +1
-0+ }...
drivers/i2c/busses/i2c-ast2600.c line 1402
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+WARN_ON (!i2c_bus->slave);Why?
It can be removed.
Quoted 10 lines
...
Quoted 6 lines
drivers/i2c/busses/i2c-ast2600.c new file +6
-0+static const struct of_device_id ast2600_i2c_bus_of_table[] = { + { + .compatible ="aspeed,ast2600-i2cv2" , + }, + {} +};
Quoted 3 lines
Quoted 1 line
+
Redundant blank line.
Will removed.
Quoted 12 lines
Quoted 2 lines
+MODULE_DEVICE_TABLE(of,
ast2600_i2c_bus_of_table);...
drivers/i2c/busses/i2c-ast2600.c line 1450
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+ i2c_bus =devm_kzalloc (dev,sizeof (*i2c_bus),GFP_KERNEL ); +if (!i2c_bus) +return dev_err_probe (dev, -ENOMEM ,"no memory allocated\n" );No. We do not print error message for ENOMEM.
You homework to find why.
Got it, will update.
Quoted 10 lines
...
drivers/i2c/busses/i2c-ast2600.c line 1476
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+if (of_property_read_bool (pdev->dev.of_node,"aspeed,enable-dma" ))
device_property_read_bool()?Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ i2c_bus->mode =DMA_MODE ;...
Quoted 15 lines
drivers/i2c/busses/i2c-ast2600.c line 1489
Quoted 11 lines
drivers/i2c/busses/i2c-ast2600.c new file +11
-0+if (i2c_bus->mode ==BUFF_MODE ) { + res =platform_get_resource (pdev,IORESOURCE_MEM ,1 ); +if (res &&resource_size (res) >=2 ) { + i2c_bus->buf_base =devm_ioremap_resource (dev, res); + +if (!IS_ERR_OR_NULL (i2c_bus->buf_base)) + i2c_bus->buf_size =resource_size (res) /2 ; + }else { + i2c_bus->mode =BYTE_MODE ; + } + }Can be done without additional checks and with a simple call to
devm_platform_ioremap_resource(). No?
Sorry, I can't catch your point, can you guide me more about it?
Quoted 8 lines
...
drivers/i2c/busses/i2c-ast2600.c line 1493
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+/* i2c timeout counter: use base clk4 1Mhz, +* per unit: 1/(1000/4096) = 4096us +*/ Wrong multi-line style of the comment.
Will update to
/*
* i2c timeout counter: use base clk4 1Mhz,
* per unit: 1/(1000/4096) = 4096us
*/
Quoted 9 lines
...
drivers/i2c/busses/i2c-ast2600.c line 1498
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c new file +5
-0+ ret =of_property_read_u32 (dev->of_node, +"i2c-scl-clk-low-timeout-us" , + &i2c_bus->timeout); +if (!ret) + i2c_bus->timeout /=4096 ;What is this and why I2C core timings (standard) can't be utilized here?
It is scl clk timout setting. That have been discuss with following.
https://lore.kernel.org/lkml/20230314221614.24205-1-andi.shyti at kernel.org/T/
Quoted 5 lines
...
Review
Quoted 1 line
(quoted diff) +1
-0+ ret = of_property_read_u32(dev->of_node, "clock-frequency",
&i2c_bus->bus_frequency);
Quoted 8 lines
drivers/i2c/busses/i2c-ast2600.c line 1518
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +4
-0+if (ret <0 ) { +dev_warn (dev,"Could not read clock-frequency property\n" ); + i2c_bus->bus_frequency =100000 ; + }There are macro for standard speeds. Moreover, there is a function to parse properties, no need to open code.
Will update
ret = of_property_read_u32(dev->of_node, "clock-frequency", &bus_freq);
if (ret < 0) {
dev_warn(dev, "Could not read clock-frequency property\n");
i2c_bus->bus_frequency = I2C_SPEED_STANDARD;
} else {
i2c_bus->bus_frequency = bus_freq;
}Quoted 11 lines
...
drivers/i2c/busses/i2c-ast2600.c line 1525
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ i2c_bus->adap.dev.of_node = dev->of_node;
device_set_node()...
drivers/i2c/busses/i2c-ast2600.c line 1537
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+if (of_property_read_bool (dev->of_node,"smbus-alert" )) {Doesn't core have already support for this?
Will remove it.
Quoted 19 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ i2c_bus->alert_enable = true; + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap,
&i2c_bus->alert_data);drivers/i2c/busses/i2c-ast2600.c
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +4
-0+if (!i2c_bus->ara) +dev_warn (dev,"Failed to register ARA client\n" ); + + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER |
AST2600_I2CM_SMBUS_ALT,Quoted 7 lines
drivers/i2c/busses/i2c-ast2600.c new file +7
-0+ i2c_bus->reg_base + AST2600_I2CM_IER); + }else { + i2c_bus->alert_enable = false; +/* Set interrupt generation of I2C master controller */ +writel (AST2600_I2CM_PKT_DONE |AST2600_I2CM_BUS_RECOVER , + i2c_bus->reg_base +AST2600_I2CM_IER ); + }...
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ dev_info(dev, "%s [%d]: adapter [%d khz] mode [%d]\n", + dev->of_node->name, i2c_bus->adap.nr, i2c_bus->bus_frequency /1000,
drivers/i2c/busses/i2c-ast2600.c line 1558
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+ i2c_bus->mode);Useless noise.
Will remove it.
Thanks your review.
Best Regards,
Ryan Chen.
PATCH v11 2/2 i2c: aspeed: support ast2600 i2c new register mode driver
Quoted 1 line · Ryan Chen
Quoted 1 line · Ryan Chen
...
Quoted 11 lines
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c line 53
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+#define AST2600_GLOBAL_INIT \ + (AST2600_I2CG_CTRL_NEW_REG | \ + AST2600_I2CG_CTRL_NEW_CLK_DIV)Make just a one TAB and put the last two lines on the single one.
Update by following.
#define AST2600_GLOBAL_INIT \ (AST2600_I2CG_CTRL_NEW_REG | \ AST2600_I2CG_CTRL_NEW_CLK_DIV)
As I mentioned the last two can occupy a single line.
...
Quoted 7 lines
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c line 796
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c new file +3
-0+/* send start */ + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", + i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write",
str_read_write()?Sorry do you mean there have a function call
str_read_write? Can you point me where it is for refer?
string_helpers.h.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/i2c/busses/i2c-ast2600.c new file +2
-0+ msg->len, msg->len >1 ?"s" :"" , + msg->flags & I2C_M_RD ? "from" : "to", msg->addr);
...
Quoted 6 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (--i % 4 != 3) + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));The above code is ugly. Can you think about it and write in a better way?
Sorry, that is because the register only support for 4 byte align write. That the reason I need put for byte write to 4 byte align write.
Yes, that's fine. The problem is in _how_ the driver does it. We have a lot of helpers in the kernel to access unaligned data.
...
Quoted 5 lines
Quoted 3 lines
drivers/i2c/busses/i2c-ast2600.c line 1225
Quoted 1 line
drivers/i2c/busses/i2c-ast2600.c new file +1
-0+return ast2600_i2c_master_irq (i2c_bus) ?IRQ_HANDLED :IRQ_NONE ;
IRQ_RETVAL()?Sorry, most return is handled or not handled. Do you mean replace it just " return
IRQ_RETVAL(ret);"
Have you had a chance to look in the implementation of IRQ_RETVAL() ? I believe if you do, you will find the answer to your question.
...
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
drivers/i2c/busses/i2c-ast2600.c new file +6
-0+if (i2c_bus->mode ==BUFF_MODE ) { + res =platform_get_resource (pdev,IORESOURCE_MEM ,1 ); +if (res &&resource_size (res) >=2 ) { + i2c_bus->buf_base =devm_ioremap_resource (dev, res); + +if (!IS_ERR_OR_NULL (i2c_bus->buf_base))Quoted 9 lines
drivers/i2c/busses/i2c-ast2600.c line 1489
Quoted 5 lines
drivers/i2c/busses/i2c-ast2600.c new file +5
-0+ i2c_bus->buf_size =resource_size (res) /2 ; + }else { + i2c_bus->mode =BYTE_MODE ; + } + }Can be done without additional checks and with a simple call to
devm_platform_ioremap_resource(). No?
Quoted 1 line
Sorry, I can't catch your point, can you guide me more about it?
if (BUFF_MODE) {
void __iomem buf_base;
buf_base = devm_platform_ioremap_and_get_resource(pdev, 1, &res);
if (IS_ERR(buf_base))
mode = BYTE_MODE;
else {
->buf_base = buf_base;
->buf_size = ...
}
}...
Quoted 10 lines
Quoted 10 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ ret = of_property_read_u32(dev->of_node, "clock-frequency",
&i2c_bus->bus_frequency);drivers/i2c/busses/i2c-ast2600.c line 1518
Quoted 4 lines
drivers/i2c/busses/i2c-ast2600.c new file +4
-0+if (ret <0 ) { +dev_warn (dev,"Could not read clock-frequency property\n" ); + i2c_bus->bus_frequency =100000 ; + }There are macro for standard speeds. Moreover, there is a function to parse properties, no need to open code.
Quoted 8 lines
Will update
ret =of_property_read_u32(dev->of_node, "clock-frequency", &bus_freq);if (ret < 0) { dev_warn(dev, "Could not read clock-frequency property\n"); i2c_bus->bus_frequency = I2C_SPEED_STANDARD; } else { i2c_bus->bus_frequency = bus_freq; }
No, just use the I2C core API to fill this property in the specific i2c_timings structure.
Signature · 2 lines
With Best Regards, Andy Shevchenko