On Fri, Jun 18, 2021 at 11:53:41AM +0800, Jamin Lin wrote:

The 06/17/2021 10:33, Andy Shevchenko wrote:

quoted

On Thu, Jun 17, 2021 at 05:43:40PM +0800, Jamin Lin wrote:

...

quoted

quoted

+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.

SPDX covers this.

Will change as following what do you think?
// SPDX-License-Identifier: GPL-2.0-or-later
/*
 *  Aspeed I2C Interrupt Controller.
 * Copyright (C) ASPEED Technology Inc.
 * Ryan Chen [off-list ref]
 */

Yes, something like this.

...

quoted

quoted

+static const struct aspeed_i2c_base_clk i2c_base_clk[BASE_CLK_COUNT] = {
+	/* name	target_freq */
+	{  "base_clk0",	1000000 },	/* 1M */
+	{  "base_clk1",	4000000 },	/* 4M */
+	{  "base_clk2",	10000000 },	/* 10M */
+	{  "base_clk3",	40000000 },	/* 40M */
+};

Why it's not provided as the clock provider(s)?

According to the design of ASPEED AST2600 SOC, the I2C bus is connected to PHB bus.
The clock driver provides PHB clock and its default frequency is 100MHZ.
AST2600 support the bus frequency of I2C from 0.1kbps to 5Mbps if PHB clock is 50MHZ.
To meet the different bus frequency of I2C, we use this programmer to calculate the divider to 
change the base clock.
For example, 
It calculates divider to change base_clock 1 to 1M to support I2C bus frequency 100KHZ
It calculates divider to change base_clock 2 to 4M to support I2C bus frequency 400KHZ 

My question is, why don't you provide a clock provider (under drivers/clk) for
this platform and use it instead?

...

quoted

quoted

+	struct clk_hw_onecell_data *onecell;
+	struct clk_hw *hw;
+	int err;
+	u32 clk_divider = 0;
+	int i, j;
+	unsigned long base_freq;

Use reversed xmas tree order for all these blocks.

The rule of thumb, btw, that any comment applies to all similar places by
default (independently on which line it was given against).

Do you mena change as following?
struct clk_hw_onecell_data *onecell;
unsigned long base_freq;
u32 clk_divider = 0;
struct clk_hw *hw;
int err;
int i;
int j;

Yes.

...

quoted

quoted

+		for (i = 0; i < 0xff; i++) {
+			/*
+			 * i maps to div:
+			 * 0x00: div 1
+			 * 0x01: div 1.5
+			 * 0x02: div 2
+			 * 0x03: div 2.5
+			 * 0x04: div 3
+			 * ...
+			 * 0xFE: div 128
+			 * 0xFF: div 128.5
+			 */
+			base_freq = base_clk * 2 / (2 + i);
+			if (base_freq <= i2c_base_clk[j].base_freq)
+				break;
+		}

oAre yuo sure you can't eliminate the entire for-loop? Think about it a bit,
please.

What do you think if we use "lookup table" instaed of above for loop?

Besides that it should be a part of clock provider, no, you may use a formula
(bit operations and so on).

...

quoted

quoted

+	i2c_ic->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);

quoted

quoted

+	if (IS_ERR(i2c_ic->rst)) {

quoted

+		dev_dbg(&pdev->dev,
+			"missing or invalid reset controller device tree entry");

Make it optional.

Can I change to use "dev_err"?

What I meant here is to make the reset optional (there is even specific API for
that) and return an error in that case.

quoted

quoted

+	} else {
+		/* SCU I2C Reset */
+		reset_control_assert(i2c_ic->rst);
+		udelay(3);
+		reset_control_deassert(i2c_ic->rst);
+	}

...

quoted

quoted

+static struct ast_i2c_timing_table aspeed_old_i2c_timing_table[] = {

What the ... is this for?!

AST2600 support old/new register set for I2C controller.
This lookup table is used to find the divisor for the specific I2C bus
frequency for AST2600 I2C controller with old register set.
For example
If I2C bus frequency is 100KHZ and PHB clock is 100MHZ, it will find this table
because 100000000/1024 < 100000
"{ 1024, 0x00000300 | (0x5) | (0xf << 20) | (0xf << 16) | (0xf << 12) }"

Can't you derive it by formula(s)?

quoted

quoted

+	/* Divisor : Base Clock : tCKHighMin : tCK High : tCK Low  */
+	/* Divisor :	  [3:0] : [23: 20]   :   [19:16]:   [15:12] */

quoted

quoted

+};

-- 
With Best Regards,
Andy Shevchenko