> Subject: Re: [PATCH v13 2/3] i2c: aspeed: support AST2600 i2c new register > mode driver > > On Thu, Aug 22, 2024 at 02:24:26AM +0000, Ryan Chen wrote: > > > On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > > > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: > > ... > > > > > > > + /* Check 0x14's SDA and SCL status */ > > > > > > + state = readl(i2c_bus->reg_base + > AST2600_I2CC_STS_AND_BUFF); > > > > > > + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & > > > > > AST2600_I2CC_SCL_LINE_STS)) { > > > > > > + writel(AST2600_I2CM_RECOVER_CMD_EN, > i2c_bus->reg_base > > > + > > > > > AST2600_I2CM_CMD_STS); > > > > > > + r = > wait_for_completion_timeout(&i2c_bus->cmd_complete, > > > > > i2c_bus->adap.timeout); > > > > > > + if (r == 0) { > > > > > > + dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > > > > + ret = -ETIMEDOUT; > > > > > > + } else { > > > > > > + if (i2c_bus->cmd_err) { > > > > > > + dev_dbg(i2c_bus->dev, "recovery error\n"); > > > > > > + ret = -EPROTO; > > > > > > + } > > > > > > + } > > > > > > + } > > > > > > > > > > ret is set but maybe overridden. > > > > > > > > If will modify by following. > > > > if (r == 0) { > > > > dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > > ret = -ETIMEDOUT; > > > > } else if (i2c_bus->cmd_err) { > > > > dev_dbg(i2c_bus->dev, "recovery error\n"); > > > > ret = -EPROTO; > > > > } > > > > If no error keep ret = 0; > > > > > > It doesn't change the behaviour. Still ret can be overridden below... > > > > Yes, it is expectable, previous is issue recovery command out then the > > following is double confirm the bus status. > > If bus still busy, the function still return recovery fail. > > > > Or should I modify by following? > > /* Check 0x14's SDA and SCL status */ > > state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & > AST2600_I2CC_SCL_LINE_STS)) { > > writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + > AST2600_I2CM_CMD_STS); > > r = wait_for_completion_timeout(&i2c_bus->cmd_complete, > i2c_bus->adap.timeout); > > if (r == 0) { > > dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > ret = -ETIMEDOUT; > > This assignment doesn't make sense. > > > } else if (i2c_bus->cmd_err) { > > dev_dbg(i2c_bus->dev, "recovery error\n"); > > ret = -EPROTO; > > } > > /* check bus status */ > > state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > if (state & AST2600_I2CC_BUS_BUSY_STS) { > > dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > > ret = -EPROTO; > > } > > } > > > > > > > + /* Recovery done */ > > > > > > > > > > Even if it fails above? > > > > > > > > This will keep check the bus status, if bus busy, will give ret = > > > > -EPROTO; > > > > > > > > > > + state = readl(i2c_bus->reg_base + > AST2600_I2CC_STS_AND_BUFF); > > > > > > + if (state & AST2600_I2CC_BUS_BUSY_STS) { > > > > > > + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > > > > > > + ret = -EPROTO; > > > > > > ...here. > > See above. OH, I understand now. I will modify following if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) { writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout); if (r == 0) { dev_dbg(i2c_bus->dev, "recovery timed out\n"); ++ writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); ++ return -ETIMEDOUT; } else { if (i2c_bus->cmd_err) { dev_dbg(i2c_bus->dev, "recovery error\n"); ret = -EPROTO; } } > > > > > > + } > > > > > > + > > > > > > + /* restore original master/slave setting */ > > > > > > + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > > > > > > + return ret; > > ... > > > > > > > + i2c_bus->master_dma_addr = > > > > > > + dma_map_single(i2c_bus->dev, > i2c_bus->master_safe_buf, > > > > > > + msg->len, DMA_TO_DEVICE); > > > > > > > > > > > + if (dma_mapping_error(i2c_bus->dev, > > > i2c_bus->master_dma_addr)) > > > > > { > > > > > > + > i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, > > > msg, > > > > > false); > > > > > > + i2c_bus->master_safe_buf = NULL; > > > > > > > > > > > + return -ENOMEM; > > > > > > > > > > Why is the dma_mapping_error() returned error code shadowed? > > > > > > > > Sorry, please point me why you are think it is shadowed? > > > > As I know dma_mapping_error() will return 0 or -ENOMEM. So I check > > > > if it > > > is !=0. > > > > Than return -ENOMEM. > > > > > > First of all, it is a bad style to rely on the implementation > > > details where it's not crucial. Second, today it may return only > > > ENOMEM, tomorrow it can return a different code or codes. And in > > > general, one should not shadow an error code without justification. > > > > > Understood, The following is better, am I right? (if yest, those will > > update in driver) > > Yes. Thanks. > > > Int ret; > > ..... > > ret = dma_mapping_error(i2c_bus->dev, > i2c_bus->master_dma_addr) > > if (ret) { > > i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, > false); > > i2c_bus->master_safe_buf = NULL; > > return ret; > > } > > > > > > > > + } > > ... > > > > > > > + if (i2c_bus->mode == BUFF_MODE) { > > > > > > + i2c_bus->buf_base = > > > > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > > > > + else > > > > > > + i2c_bus->mode = BYTE_MODE; > > > > > > > > > > What's wrong with positive conditional? And is it even possible > > > > > to have NULL here? > > > > > > > > > Yes, if dtsi fill not following yaml example have reg 1, that will > > > > failure at buffer > > > mode. > > > > And I can swith to byte mode. > > > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; > > > > > > I was asking about if (!IS_ERR_OR_NULL(...)) line: > > > 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? > > I will update to following. > > if (IS_ERR(i2c_bus->buf_base)) > > i2c_bus->mode = BYTE_MODE; > > else > > i2c_bus->buf_size = resource_size(res) / 2; > > > > > 2) Why _NULL? > > If dtsi file is claim only 1 reg offset. reg = <0x80 0x80>; that will goto byte > mode. > > reg = <0x80 0x80>, <0xc00 0x20>; can support buffer mode. > > due to 2nd is buffer register offset. > > I have asked why IS_ERR_OR_NULL() and not IS_ERR(). > OH, I will doing by this. if (IS_ERR_OR_NULL(i2c_bus->buf_base)) i2c_bus->mode = BYTE_MODE; else i2c_bus->buf_size = resource_size(res) / 2; > > -- > With Best Regards, > Andy Shevchenko >