Re: [PATCH 1/3] i2c: tegra: implement slave mode

From: Andrey Danin
Date: Tue Mar 31 2015 - 02:26:07 EST


Hi,

Sorry for long delay.

And thanks for the quick review. It helped a lot!

On 29.01.2015 14:41, Wolfram Sang wrote:
Hi,

Initialization code is based on NVEC driver.

There is a HW bug in AP20 that was also mentioned in kernel sources
for Toshiba AC100.

Signed-off-by: Andrey Danin <danindrey@xxxxxxx>

Cool, thanks for the converison. While I usually like to only get the
patches which I need to handle, please CC me to all patches next time. I
am interested what changes were needed for the user of the slave
framework, too.

Done. I sent v2 yesterday evening.

+static bool tegra_i2c_slave_isr(int irq, struct tegra_i2c_dev *i2c_dev)
+{
+ unsigned long status;
+ u8 value;
+
+ if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
+ return false;

Can this happen?

Yes. I call slave ISR without any conditions from main ISR routine.

+ /* i2c master sends data to us */
+ if (is_write(status)) {
+ i2c_slave_event(i2c_dev->slave, I2C_SLAVE_REQ_WRITE_START,
+ NULL);

Can this HW create an interrupt once the address detection + RW bit are
received? Or only if a complete write has been received?

Tegra I2C generates one interrupt per byte (address or data) and one interrupt for stop bit.

+static int tegra_reg_slave(struct i2c_client *slave)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(slave->adapter);
+
+ if (i2c_dev->slave)
+ return -EBUSY;
+
+ i2c_dev->slave = slave;
+
+ tegra_i2c_clock_enable(i2c_dev);
+
+ reset_control_assert(i2c_dev->rst);
+ udelay(2);
+ reset_control_deassert(i2c_dev->rst);

Why do you need a reset when a slave gets registered?

I copied this code from nvec driver. Reset is done during I2C controller initialization. This reset is not needed. Thanks for pointing.

+
+ i2c_writel(i2c_dev, I2C_SL_CNFG_NEWSL, I2C_SL_CNFG);
+ i2c_writel(i2c_dev, 0x1E, I2C_SL_DELAY_COUNT);

What does this magic number mean?

It's a default value. I created a constant for it.

+
+ i2c_writel(i2c_dev, slave->addr, I2C_SL_ADDR1);
+ i2c_writel(i2c_dev, 0, I2C_SL_ADDR2);

Handling 10 bit addresses?

In v2.

+
+ return 0;
+}
+

static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -679,6 +808,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
.has_single_clk_source = false,
.clk_divisor_hs_mode = 3,
.clk_divisor_std_fast_mode = 0,
+ .slave_read_start_delay = 0,

No need to init to 0 IMO.

Ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/