Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On Mon, Jul 02, 2018 at 02:40:11PM -0700, Jae Hyun Yoo wrote: > This patch adjusts spinlock scope to make it wrap the whole irq > handler using a single lock/unlock which covers both master and > slave handlers. > > Signed-off-by: Jae Hyun Yoo Applied to for-next, thanks! Not related to these patches, but there is an issue found with sparse: drivers/i2c/busses/i2c-aspeed.c:875:38: warning: incorrect type in assignment (different modifiers) drivers/i2c/busses/i2c-aspeed.c:875:38:expected unsigned int ( *get_clk_reg_val )( ... ) drivers/i2c/busses/i2c-aspeed.c:875:38:got void const *const data Maybe someone wants to have a go at this... signature.asc Description: PGP signature
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On Mon, Jul 02, 2018 at 02:40:11PM -0700, Jae Hyun Yoo wrote: > This patch adjusts spinlock scope to make it wrap the whole irq > handler using a single lock/unlock which covers both master and > slave handlers. > > Signed-off-by: Jae Hyun Yoo Applied to for-next, thanks! Not related to these patches, but there is an issue found with sparse: drivers/i2c/busses/i2c-aspeed.c:875:38: warning: incorrect type in assignment (different modifiers) drivers/i2c/busses/i2c-aspeed.c:875:38:expected unsigned int ( *get_clk_reg_val )( ... ) drivers/i2c/busses/i2c-aspeed.c:875:38:got void const *const data Maybe someone wants to have a go at this... signature.asc Description: PGP signature
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On 7/13/2018 1:33 PM, Wolfram Sang wrote: BTW, to whom should I ask applying this patch? Should I send v2 after adding your reviewed-by tag? Not needed, thanks. I use 'patchwork' and this tool collects all the tags for me. If I see a patch reviewed by a driver maintainer, I'll pick it up in the next queue. Oh, I see. Thanks a lot Wolfram!
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On 7/13/2018 1:33 PM, Wolfram Sang wrote: BTW, to whom should I ask applying this patch? Should I send v2 after adding your reviewed-by tag? Not needed, thanks. I use 'patchwork' and this tool collects all the tags for me. If I see a patch reviewed by a driver maintainer, I'll pick it up in the next queue. Oh, I see. Thanks a lot Wolfram!
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
> BTW, to whom should I ask applying this patch? Should I send v2 after > adding your reviewed-by tag? Not needed, thanks. I use 'patchwork' and this tool collects all the tags for me. If I see a patch reviewed by a driver maintainer, I'll pick it up in the next queue. signature.asc Description: PGP signature
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
> BTW, to whom should I ask applying this patch? Should I send v2 after > adding your reviewed-by tag? Not needed, thanks. I use 'patchwork' and this tool collects all the tags for me. If I see a patch reviewed by a driver maintainer, I'll pick it up in the next queue. signature.asc Description: PGP signature
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On 7/12/2018 1:41 AM, Brendan Higgins wrote: On Mon, Jul 2, 2018 at 2:40 PM Jae Hyun Yoo wrote: This patch adjusts spinlock scope to make it wrap the whole irq handler using a single lock/unlock which covers both master and slave handlers. Signed-off-by: Jae Hyun Yoo --- drivers/i2c/busses/i2c-aspeed.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 60e4d0e939a3..9f02aa959a3e 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -234,7 +234,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus) bool irq_handled = true; u8 value; - spin_lock(>lock); if (!slave) { irq_handled = false; goto out; @@ -325,7 +324,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus) writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG); out: - spin_unlock(>lock); return irq_handled; } #endif /* CONFIG_I2C_SLAVE */ @@ -389,7 +387,6 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus) u8 recv_byte; int ret; - spin_lock(>lock); irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG); /* Ack all interrupt bits. */ writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG); @@ -547,22 +544,29 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus) dev_err(bus->dev, "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_status, status_ack); - spin_unlock(>lock); return !!irq_status; } static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) { struct aspeed_i2c_bus *bus = dev_id; + bool ret; + + spin_lock(>lock); #if IS_ENABLED(CONFIG_I2C_SLAVE) if (aspeed_i2c_slave_irq(bus)) { dev_dbg(bus->dev, "irq handled by slave.\n"); - return IRQ_HANDLED; + ret = true; + goto out; } #endif /* CONFIG_I2C_SLAVE */ - return aspeed_i2c_master_irq(bus) ? IRQ_HANDLED : IRQ_NONE; + ret = aspeed_i2c_master_irq(bus); + +out: + spin_unlock(>lock); + return ret ? IRQ_HANDLED : IRQ_NONE; } static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, -- 2.17.1 Reviewed-by: Brendan Higgins Thanks! Thanks Brendan! BTW, to whom should I ask applying this patch? Should I send v2 after adding your reviewed-by tag? Thanks, Jae
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On 7/12/2018 1:41 AM, Brendan Higgins wrote: On Mon, Jul 2, 2018 at 2:40 PM Jae Hyun Yoo wrote: This patch adjusts spinlock scope to make it wrap the whole irq handler using a single lock/unlock which covers both master and slave handlers. Signed-off-by: Jae Hyun Yoo --- drivers/i2c/busses/i2c-aspeed.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 60e4d0e939a3..9f02aa959a3e 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -234,7 +234,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus) bool irq_handled = true; u8 value; - spin_lock(>lock); if (!slave) { irq_handled = false; goto out; @@ -325,7 +324,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus) writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG); out: - spin_unlock(>lock); return irq_handled; } #endif /* CONFIG_I2C_SLAVE */ @@ -389,7 +387,6 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus) u8 recv_byte; int ret; - spin_lock(>lock); irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG); /* Ack all interrupt bits. */ writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG); @@ -547,22 +544,29 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus) dev_err(bus->dev, "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_status, status_ack); - spin_unlock(>lock); return !!irq_status; } static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) { struct aspeed_i2c_bus *bus = dev_id; + bool ret; + + spin_lock(>lock); #if IS_ENABLED(CONFIG_I2C_SLAVE) if (aspeed_i2c_slave_irq(bus)) { dev_dbg(bus->dev, "irq handled by slave.\n"); - return IRQ_HANDLED; + ret = true; + goto out; } #endif /* CONFIG_I2C_SLAVE */ - return aspeed_i2c_master_irq(bus) ? IRQ_HANDLED : IRQ_NONE; + ret = aspeed_i2c_master_irq(bus); + +out: + spin_unlock(>lock); + return ret ? IRQ_HANDLED : IRQ_NONE; } static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, -- 2.17.1 Reviewed-by: Brendan Higgins Thanks! Thanks Brendan! BTW, to whom should I ask applying this patch? Should I send v2 after adding your reviewed-by tag? Thanks, Jae
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On Mon, Jul 2, 2018 at 2:40 PM Jae Hyun Yoo wrote: > > This patch adjusts spinlock scope to make it wrap the whole irq > handler using a single lock/unlock which covers both master and > slave handlers. > > Signed-off-by: Jae Hyun Yoo > --- > drivers/i2c/busses/i2c-aspeed.c | 16 ++-- > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 60e4d0e939a3..9f02aa959a3e 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -234,7 +234,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus > *bus) > bool irq_handled = true; > u8 value; > > - spin_lock(>lock); > if (!slave) { > irq_handled = false; > goto out; > @@ -325,7 +324,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus > *bus) > writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG); > > out: > - spin_unlock(>lock); > return irq_handled; > } > #endif /* CONFIG_I2C_SLAVE */ > @@ -389,7 +387,6 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus > *bus) > u8 recv_byte; > int ret; > > - spin_lock(>lock); > irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > /* Ack all interrupt bits. */ > writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG); > @@ -547,22 +544,29 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus > *bus) > dev_err(bus->dev, > "irq handled != irq. expected 0x%08x, but was > 0x%08x\n", > irq_status, status_ack); > - spin_unlock(>lock); > return !!irq_status; > } > > static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) > { > struct aspeed_i2c_bus *bus = dev_id; > + bool ret; > + > + spin_lock(>lock); > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > if (aspeed_i2c_slave_irq(bus)) { > dev_dbg(bus->dev, "irq handled by slave.\n"); > - return IRQ_HANDLED; > + ret = true; > + goto out; > } > #endif /* CONFIG_I2C_SLAVE */ > > - return aspeed_i2c_master_irq(bus) ? IRQ_HANDLED : IRQ_NONE; > + ret = aspeed_i2c_master_irq(bus); > + > +out: > + spin_unlock(>lock); > + return ret ? IRQ_HANDLED : IRQ_NONE; > } > > static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, > -- > 2.17.1 > Reviewed-by: Brendan Higgins Thanks!
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On Mon, Jul 2, 2018 at 2:40 PM Jae Hyun Yoo wrote: > > This patch adjusts spinlock scope to make it wrap the whole irq > handler using a single lock/unlock which covers both master and > slave handlers. > > Signed-off-by: Jae Hyun Yoo > --- > drivers/i2c/busses/i2c-aspeed.c | 16 ++-- > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 60e4d0e939a3..9f02aa959a3e 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -234,7 +234,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus > *bus) > bool irq_handled = true; > u8 value; > > - spin_lock(>lock); > if (!slave) { > irq_handled = false; > goto out; > @@ -325,7 +324,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus > *bus) > writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG); > > out: > - spin_unlock(>lock); > return irq_handled; > } > #endif /* CONFIG_I2C_SLAVE */ > @@ -389,7 +387,6 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus > *bus) > u8 recv_byte; > int ret; > > - spin_lock(>lock); > irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > /* Ack all interrupt bits. */ > writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG); > @@ -547,22 +544,29 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus > *bus) > dev_err(bus->dev, > "irq handled != irq. expected 0x%08x, but was > 0x%08x\n", > irq_status, status_ack); > - spin_unlock(>lock); > return !!irq_status; > } > > static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) > { > struct aspeed_i2c_bus *bus = dev_id; > + bool ret; > + > + spin_lock(>lock); > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > if (aspeed_i2c_slave_irq(bus)) { > dev_dbg(bus->dev, "irq handled by slave.\n"); > - return IRQ_HANDLED; > + ret = true; > + goto out; > } > #endif /* CONFIG_I2C_SLAVE */ > > - return aspeed_i2c_master_irq(bus) ? IRQ_HANDLED : IRQ_NONE; > + ret = aspeed_i2c_master_irq(bus); > + > +out: > + spin_unlock(>lock); > + return ret ? IRQ_HANDLED : IRQ_NONE; > } > > static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, > -- > 2.17.1 > Reviewed-by: Brendan Higgins Thanks!