Re: [PATCH v2 2/2] powerpc/legacy_serial: Use early_ioremap()
Hi Christophe,
On 21/04/21 1:32 am, Christophe Leroy wrote:
> From: Christophe Leroy
>
> [0.00] ioremap() called early from
> find_legacy_serial_ports+0x3cc/0x474. Use early_ioremap() instead
>
> find_legacy_serial_ports() is called early from setup_arch(), before
> paging_init(). vmalloc is not available yet, ioremap shouldn't be
> used that early.
>
> Use early_ioremap() and switch to a regular ioremap() later.
>
> Signed-off-by: Christophe Leroy
> Signed-off-by: Christophe Leroy
> ---
I gave it a spin on my T2080RDB. The error message is gone and I still
get console output.
Tested-by: Chris Packham
> arch/powerpc/kernel/legacy_serial.c | 33 +
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/legacy_serial.c
> b/arch/powerpc/kernel/legacy_serial.c
> index f061e06e9f51..8b2c1a8553a0 100644
> --- a/arch/powerpc/kernel/legacy_serial.c
> +++ b/arch/powerpc/kernel/legacy_serial.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #undef DEBUG
>
> @@ -34,6 +35,7 @@ static struct legacy_serial_info {
> unsigned intclock;
> int irq_check_parent;
> phys_addr_t taddr;
> + void __iomem*early_addr;
> } legacy_serial_infos[MAX_LEGACY_SERIAL_PORTS];
>
> static const struct of_device_id legacy_serial_parents[] __initconst = {
> @@ -325,17 +327,16 @@ static void __init setup_legacy_serial_console(int
> console)
> {
> struct legacy_serial_info *info = _serial_infos[console];
> struct plat_serial8250_port *port = _serial_ports[console];
> - void __iomem *addr;
> unsigned int stride;
>
> stride = 1 << port->regshift;
>
> /* Check if a translated MMIO address has been found */
> if (info->taddr) {
> - addr = ioremap(info->taddr, 0x1000);
> - if (addr == NULL)
> + info->early_addr = early_ioremap(info->taddr, 0x1000);
> + if (info->early_addr == NULL)
> return;
> - udbg_uart_init_mmio(addr, stride);
> + udbg_uart_init_mmio(info->early_addr, stride);
> } else {
> /* Check if it's PIO and we support untranslated PIO */
> if (port->iotype == UPIO_PORT && isa_io_special)
> @@ -353,6 +354,30 @@ static void __init setup_legacy_serial_console(int
> console)
> udbg_uart_setup(info->speed, info->clock);
> }
>
> +static int __init ioremap_legacy_serial_console(void)
> +{
> + struct legacy_serial_info *info =
> _serial_infos[legacy_serial_console];
> + struct plat_serial8250_port *port =
> _serial_ports[legacy_serial_console];
> + void __iomem *vaddr;
> +
> + if (legacy_serial_console < 0)
> + return 0;
> +
> + if (!info->early_addr)
> + return 0;
> +
> + vaddr = ioremap(info->taddr, 0x1000);
> + if (WARN_ON(!vaddr))
> + return -ENOMEM;
> +
> + udbg_uart_init_mmio(vaddr, 1 << port->regshift);
> + early_iounmap(info->early_addr, 0x1000);
> + info->early_addr = NULL;
> +
> + return 0;
> +}
> +early_initcall(ioremap_legacy_serial_console);
> +
> /*
>* This is called very early, as part of setup_system() or eventually
>* setup_arch(), basically before anything else in this file. This function
[PATCH v4 6/6] i2c: mpc: Drop duplicate message from devm_platform_ioremap_resource()
From: Andy Shevchenko
devm_platform_ioremap_resource() prints a message in case of error.
Drop custom one.
Signed-off-by: Andy Shevchenko
Reviewed-by: Chris Packham
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 7a9abeeb6da0..30d9e89a3db2 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -711,10 +711,8 @@ static int fsl_i2c_probe(struct platform_device *op)
spin_lock_init(>lock);
i2c->base = devm_platform_ioremap_resource(op, 0);
- if (IS_ERR(i2c->base)) {
- dev_err(i2c->dev, "failed to map controller\n");
+ if (IS_ERR(i2c->base))
return PTR_ERR(i2c->base);
- }
i2c->irq = platform_get_irq(op, 0);
if (i2c->irq < 0)
--
2.31.1
[PATCH v4 3/6] i2c: mpc: Use devm_clk_get_optional()
From: Andy Shevchenko
The peripheral clock is optional and we may get an -EPROBE_DEFER error code
which would not be propagated correctly, fix this by using
devm_clk_get_optional().
Signed-off-by: Andy Shevchenko
Reviewed-by: Chris Packham
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 37244138875a..d2209c04f67a 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -737,17 +737,18 @@ static int fsl_i2c_probe(struct platform_device *op)
* enable clock for the I2C peripheral (non fatal),
* keep a reference upon successful allocation
*/
- clk = devm_clk_get(>dev, NULL);
- if (!IS_ERR(clk)) {
- err = clk_prepare_enable(clk);
- if (err) {
- dev_err(>dev, "failed to enable clock\n");
- return err;
- } else {
- i2c->clk_per = clk;
- }
+ clk = devm_clk_get_optional(>dev, NULL);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ err = clk_prepare_enable(clk);
+ if (err) {
+ dev_err(>dev, "failed to enable clock\n");
+ return err;
}
+ i2c->clk_per = clk;
+
if (of_property_read_bool(op->dev.of_node, "fsl,preserve-clocking")) {
clock = MPC_I2C_CLOCK_PRESERVE;
} else {
@@ -791,8 +792,7 @@ static int fsl_i2c_probe(struct platform_device *op)
return 0;
fail_add:
- if (i2c->clk_per)
- clk_disable_unprepare(i2c->clk_per);
+ clk_disable_unprepare(i2c->clk_per);
return result;
};
@@ -803,8 +803,7 @@ static int fsl_i2c_remove(struct platform_device *op)
i2c_del_adapter(>adap);
- if (i2c->clk_per)
- clk_disable_unprepare(i2c->clk_per);
+ clk_disable_unprepare(i2c->clk_per);
return 0;
};
--
2.31.1
[PATCH v4 5/6] i2c: mpc: Use device_get_match_data() helper
From: Andy Shevchenko
Use the device_get_match_data() helper instead of open coding.
Signed-off-by: Andy Shevchenko
Reviewed-by: Chris Packham
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 0865f7ac80bd..7a9abeeb6da0 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
@@ -689,10 +690,9 @@ static struct i2c_bus_recovery_info fsl_i2c_recovery_info
= {
.recover_bus = fsl_i2c_bus_recovery,
};
-static const struct of_device_id mpc_i2c_of_match[];
static int fsl_i2c_probe(struct platform_device *op)
{
- const struct of_device_id *match;
+ const struct mpc_i2c_data *data;
struct mpc_i2c *i2c;
const u32 *prop;
u32 clock = MPC_I2C_CLOCK_LEGACY;
@@ -701,10 +701,6 @@ static int fsl_i2c_probe(struct platform_device *op)
struct clk *clk;
int err;
- match = of_match_device(mpc_i2c_of_match, >dev);
- if (!match)
- return -EINVAL;
-
i2c = devm_kzalloc(>dev, sizeof(*i2c), GFP_KERNEL);
if (!i2c)
return -ENOMEM;
@@ -756,8 +752,8 @@ static int fsl_i2c_probe(struct platform_device *op)
clock = *prop;
}
- if (match->data) {
- const struct mpc_i2c_data *data = match->data;
+ data = device_get_match_data(>dev);
+ if (data) {
data->setup(op->dev.of_node, i2c, clock);
} else {
/* Backwards compatibility */
--
2.31.1
[PATCH v4 1/6] i2c: mpc: Interrupt driven transfer
The fsl-i2c controller will generate an interrupt after every byte
transferred. Make use of this interrupt to drive a state machine which
allows the next part of a transfer to happen as soon as the interrupt is
received. This is particularly helpful with SMBUS devices like the LM81
which will timeout if we take too long between bytes in a transfer.
Signed-off-by: Chris Packham
---
Notes:
Changes in v4:
- Split license/copyright change to separate patch
- Remove MPC_I2C_ACTION_INVALID
- make action_str const
- Remove __func__ from dev_dbg output
- Use Tx ACK/Rx ACK consistently in comments and messages
- Use spin_lock() instead of spin_lock_irqsave() in hardirq context
- Typo fix arbiritration -> arbitration
- Make mpc_i2c_wait_for_completion return an error and use that to set
i2c->rc
- Minor style fixes in mpc_i2c_isr and mpc_i2c_wait_for_completion
- I haven't deduplicated the read/write debug in mpc_i2c_do_action() as
doing so seems to make things overly complex
Changes in v3:
- use WARN/WARN_ON instead of BUG/BUG_ON
Changes in v2:
- add static_assert for state debug strings
- remove superfluous space
drivers/i2c/busses/i2c-mpc.c | 426 ---
1 file changed, 239 insertions(+), 187 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 3c8bcdfff7e7..0a80fafbe6c6 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -58,11 +58,35 @@
#define CSR_MIF 0x02
#define CSR_RXAK 0x01
+enum mpc_i2c_action {
+ MPC_I2C_ACTION_START = 1,
+ MPC_I2C_ACTION_RESTART,
+ MPC_I2C_ACTION_READ_BEGIN,
+ MPC_I2C_ACTION_READ_BYTE,
+ MPC_I2C_ACTION_WRITE,
+ MPC_I2C_ACTION_STOP,
+
+ __MPC_I2C_ACTION_CNT
+};
+
+static const char * const action_str[] = {
+ "invalid",
+ "start",
+ "restart",
+ "read begin",
+ "read",
+ "write",
+ "stop",
+};
+
+static_assert(ARRAY_SIZE(action_str) == __MPC_I2C_ACTION_CNT);
+
struct mpc_i2c {
struct device *dev;
void __iomem *base;
u32 interrupt;
- wait_queue_head_t queue;
+ wait_queue_head_t waitq;
+ spinlock_t lock;
struct i2c_adapter adap;
int irq;
u32 real_clk;
@@ -70,6 +94,16 @@ struct mpc_i2c {
u8 fdr, dfsrr;
#endif
struct clk *clk_per;
+ u32 cntl_bits;
+ enum mpc_i2c_action action;
+ struct i2c_msg *msgs;
+ int num_msgs;
+ int curr_msg;
+ u32 byte_posn;
+ u32 block;
+ int rc;
+ int expect_rxack;
+
};
struct mpc_i2c_divider {
@@ -86,19 +120,6 @@ static inline void writeccr(struct mpc_i2c *i2c, u32 x)
writeb(x, i2c->base + MPC_I2C_CR);
}
-static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
-{
- struct mpc_i2c *i2c = dev_id;
- if (readb(i2c->base + MPC_I2C_SR) & CSR_MIF) {
- /* Read again to allow register to stabilise */
- i2c->interrupt = readb(i2c->base + MPC_I2C_SR);
- writeb(0, i2c->base + MPC_I2C_SR);
- wake_up(>queue);
- return IRQ_HANDLED;
- }
- return IRQ_NONE;
-}
-
/* Sometimes 9th clock pulse isn't generated, and slave doesn't release
* the bus, because it wants to send ACK.
* Following sequence of enabling/disabling and sending start/stop generates
@@ -121,45 +142,6 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
}
}
-static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
-{
- u32 cmd_err;
- int result;
-
- result = wait_event_timeout(i2c->queue,
- (i2c->interrupt & CSR_MIF), timeout);
-
- if (unlikely(!(i2c->interrupt & CSR_MIF))) {
- dev_dbg(i2c->dev, "wait timeout\n");
- writeccr(i2c, 0);
- result = -ETIMEDOUT;
- }
-
- cmd_err = i2c->interrupt;
- i2c->interrupt = 0;
-
- if (result < 0)
- return result;
-
- if (!(cmd_err & CSR_MCF)) {
- dev_dbg(i2c->dev, "unfinished\n");
- return -EIO;
- }
-
- if (cmd_err & CSR_MAL) {
- dev_dbg(i2c->dev, "MAL\n");
- return -EAGAIN;
- }
-
- if (writing && (cmd_err & CSR_RXAK)) {
- dev_dbg(i2c->dev, "No RXAK\n");
- /* generate stop */
- writeccr(i2c, CCR_MEN);
- return -ENXIO;
- }
- return 0;
-}
-
#if defined(CONFIG_PPC_MPC52xx) || defined(CONFIG_PPC_MPC512x)
static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = {
{20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
@@ -434,168 +416,209 @@ static void mpc_i2c_setup_8xxx(struct device_node *
[PATCH v4 4/6] i2c: mpc: Remove CONFIG_PM_SLEEP ifdeffery
From: Andy Shevchenko
Use __maybe_unused for the suspend()/resume() hooks and get rid of
the CONFIG_PM_SLEEP ifdeffery to improve the code.
Signed-off-by: Andy Shevchenko
Reviewed-by: Chris Packham
Signed-off-by: Chris Packham
---
Notes:
Changes in v4:
- Fix compile error due to MPC_I2C_PM_OPS not being defined
drivers/i2c/busses/i2c-mpc.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d2209c04f67a..0865f7ac80bd 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -85,9 +85,7 @@ struct mpc_i2c {
struct i2c_adapter adap;
int irq;
u32 real_clk;
-#ifdef CONFIG_PM_SLEEP
u8 fdr, dfsrr;
-#endif
struct clk *clk_per;
u32 cntl_bits;
enum mpc_i2c_action action;
@@ -808,8 +806,7 @@ static int fsl_i2c_remove(struct platform_device *op)
return 0;
};
-#ifdef CONFIG_PM_SLEEP
-static int mpc_i2c_suspend(struct device *dev)
+static int __maybe_unused mpc_i2c_suspend(struct device *dev)
{
struct mpc_i2c *i2c = dev_get_drvdata(dev);
@@ -819,7 +816,7 @@ static int mpc_i2c_suspend(struct device *dev)
return 0;
}
-static int mpc_i2c_resume(struct device *dev)
+static int __maybe_unused mpc_i2c_resume(struct device *dev)
{
struct mpc_i2c *i2c = dev_get_drvdata(dev);
@@ -828,12 +825,7 @@ static int mpc_i2c_resume(struct device *dev)
return 0;
}
-
static SIMPLE_DEV_PM_OPS(mpc_i2c_pm_ops, mpc_i2c_suspend, mpc_i2c_resume);
-#define MPC_I2C_PM_OPS (_i2c_pm_ops)
-#else
-#define MPC_I2C_PM_OPS NULL
-#endif
static const struct mpc_i2c_data mpc_i2c_data_512x = {
.setup = mpc_i2c_setup_512x,
@@ -876,7 +868,7 @@ static struct platform_driver mpc_i2c_driver = {
.driver = {
.name = DRV_NAME,
.of_match_table = mpc_i2c_of_match,
- .pm = MPC_I2C_PM_OPS,
+ .pm = _i2c_pm_ops,
},
};
--
2.31.1
[PATCH v4 2/6] i2c: mpc: Update license and copyright
Use SPDX-License-Identifier and add copyright for Allied Telesis because of the reasonably large rewrite in the preceding patch. Signed-off-by: Chris Packham --- Notes: Changes in v4: - New, split out from "i2c: mpc: Interrupt driven transfer" drivers/i2c/busses/i2c-mpc.c | 11 +++ 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index 0a80fafbe6c6..37244138875a 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -1,16 +1,11 @@ +// SPDX-License-Identifier: GPL-2.0 /* - * (C) Copyright 2003-2004 - * Humboldt Solutions Ltd, adr...@humboldt.co.uk. - * This is a combined i2c adapter and algorithm driver for the * MPC107/Tsi107 PowerPC northbridge and processors that include * the same I2C unit (8240, 8245, 85xx). * - * Release 0.8 - * - * This file is licensed under the terms of the GNU General Public - * License version 2. This program is licensed "as is" without any - * warranty of any kind, whether express or implied. + * Copyright (C) 2003-2004 Humboldt Solutions Ltd, adr...@humboldt.co.uk + * Copyright (C) 2021 Allied Telesis Labs */ #include -- 2.31.1
[PATCH v4 0/6] i2c: mpc: Refactor to improve responsiveness
I've tested on T2081 and P2041 based systems with a number of i2c and smbus devices. I've included some clean ups provided by Andy Shevchenko to make applying the series easier. Andy Shevchenko (4): i2c: mpc: Use devm_clk_get_optional() i2c: mpc: Remove CONFIG_PM_SLEEP ifdeffery i2c: mpc: Use device_get_match_data() helper i2c: mpc: Drop duplicate message from devm_platform_ioremap_resource() Chris Packham (2): i2c: mpc: Interrupt driven transfer i2c: mpc: Update license and copyright drivers/i2c/busses/i2c-mpc.c | 492 +++ 1 file changed, 262 insertions(+), 230 deletions(-) -- 2.31.1
Re: [PATCH v1 2/4] i2c: mpc: Remove CONFIG_PM_SLEEP ifdeffery
On 15/04/21 12:14 am, Andy Shevchenko wrote:
> On Tue, Apr 13, 2021 at 11:43:25PM +0000, Chris Packham wrote:
>> On 14/04/21 2:37 am, Andy Shevchenko wrote:
>>> Use __maybe_unused for the suspend()/resume() hooks and get rid of
>>> the CONFIG_PM_SLEEP ifdeffery to improve the code.
>> This has a trivial conflict with my series because I'm also touching
>> struct mpc_i2c. git am -3 seems to deal with it but would it be easier
>> if I picked up these 4 changes and included them with my next submission?
> It would be ideal to me!
OK I've picked them up.
>>> -#define MPC_I2C_PM_OPS (_i2c_pm_ops)
>>> -#else
>>> -#define MPC_I2C_PM_OPS NULL
>>> -#endif
>>>
>>>static const struct mpc_i2c_data mpc_i2c_data_512x = {
>>> .setup = mpc_i2c_setup_512x,
>> There's a reference to MPC_I2C_PM_OPS in mpc_i2c_driver which needs
>> changing I think the following is needed
> True. sorry that my build test had been broken.
> Tell me if you want v2 with this fixed or you may fold that change since the
> above agreement.
>
I can fold the fix below in. No need for a v2 from you.
>> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
>> index 1308f749dc75..7fde13472c09 100644
>> --- a/drivers/i2c/busses/i2c-mpc.c
>> +++ b/drivers/i2c/busses/i2c-mpc.c
>> @@ -862,7 +862,7 @@ static struct platform_driver mpc_i2c_driver = {
>> .driver = {
>> .name = DRV_NAME,
>> .of_match_table = mpc_i2c_of_match,
>> - .pm = MPC_I2C_PM_OPS,
>> + .pm = _i2c_pm_ops,
>> },
>> };
>>
>>
Re: [PATCH v1 4/4] i2c: mpc: Drop duplicate message from devm_platform_ioremap_resource()
On 14/04/21 2:37 am, Andy Shevchenko wrote:
> devm_platform_ioremap_resource() prints a message in case of error.
> Drop custom one.
>
> Signed-off-by: Andy Shevchenko
Reviewed-by: Chris Packham
> ---
> drivers/i2c/busses/i2c-mpc.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index ec9d7d93e80f..684a8cd17efd 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -664,10 +664,8 @@ static int fsl_i2c_probe(struct platform_device *op)
> init_waitqueue_head(>queue);
>
> i2c->base = devm_platform_ioremap_resource(op, 0);
> - if (IS_ERR(i2c->base)) {
> - dev_err(i2c->dev, "failed to map controller\n");
> + if (IS_ERR(i2c->base))
> return PTR_ERR(i2c->base);
> - }
>
> i2c->irq = platform_get_irq(op, 0);
> if (i2c->irq < 0)
Re: [PATCH v1 3/4] i2c: mpc: Use device_get_match_data() helper
On 14/04/21 2:37 am, Andy Shevchenko wrote:
> Use the device_get_match_data() helper instead of open coding.
>
> Signed-off-by: Andy Shevchenko
Reviewed-by: Chris Packham
> ---
> drivers/i2c/busses/i2c-mpc.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 2376accd4e8e..ec9d7d93e80f 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -643,10 +644,9 @@ static struct i2c_bus_recovery_info
> fsl_i2c_recovery_info = {
> .recover_bus = fsl_i2c_bus_recovery,
> };
>
> -static const struct of_device_id mpc_i2c_of_match[];
> static int fsl_i2c_probe(struct platform_device *op)
> {
> - const struct of_device_id *match;
> + const struct mpc_i2c_data *data;
> struct mpc_i2c *i2c;
> const u32 *prop;
> u32 clock = MPC_I2C_CLOCK_LEGACY;
> @@ -655,10 +655,6 @@ static int fsl_i2c_probe(struct platform_device *op)
> struct clk *clk;
> int err;
>
> - match = of_match_device(mpc_i2c_of_match, >dev);
> - if (!match)
> - return -EINVAL;
> -
> i2c = devm_kzalloc(>dev, sizeof(*i2c), GFP_KERNEL);
> if (!i2c)
> return -ENOMEM;
> @@ -709,8 +705,8 @@ static int fsl_i2c_probe(struct platform_device *op)
> clock = *prop;
> }
>
> - if (match->data) {
> - const struct mpc_i2c_data *data = match->data;
> + data = device_get_match_data(>dev);
> + if (data) {
> data->setup(op->dev.of_node, i2c, clock);
> } else {
> /* Backwards compatibility */
Re: [PATCH v1 2/4] i2c: mpc: Remove CONFIG_PM_SLEEP ifdeffery
On 14/04/21 2:37 am, Andy Shevchenko wrote:
> Use __maybe_unused for the suspend()/resume() hooks and get rid of
> the CONFIG_PM_SLEEP ifdeffery to improve the code.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/i2c/busses/i2c-mpc.c | 12 ++--
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 6dc029a31d36..2376accd4e8e 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -66,9 +66,7 @@ struct mpc_i2c {
> struct i2c_adapter adap;
> int irq;
> u32 real_clk;
> -#ifdef CONFIG_PM_SLEEP
> u8 fdr, dfsrr;
> -#endif
> struct clk *clk_per;
> };
This has a trivial conflict with my series because I'm also touching
struct mpc_i2c. git am -3 seems to deal with it but would it be easier
if I picked up these 4 changes and included them with my next submission?
> @@ -761,8 +759,7 @@ static int fsl_i2c_remove(struct platform_device *op)
> return 0;
> };
>
> -#ifdef CONFIG_PM_SLEEP
> -static int mpc_i2c_suspend(struct device *dev)
> +static int __maybe_unused mpc_i2c_suspend(struct device *dev)
> {
> struct mpc_i2c *i2c = dev_get_drvdata(dev);
>
> @@ -772,7 +769,7 @@ static int mpc_i2c_suspend(struct device *dev)
> return 0;
> }
>
> -static int mpc_i2c_resume(struct device *dev)
> +static int __maybe_unused mpc_i2c_resume(struct device *dev)
> {
> struct mpc_i2c *i2c = dev_get_drvdata(dev);
>
> @@ -781,12 +778,7 @@ static int mpc_i2c_resume(struct device *dev)
>
> return 0;
> }
> -
> static SIMPLE_DEV_PM_OPS(mpc_i2c_pm_ops, mpc_i2c_suspend, mpc_i2c_resume);
> -#define MPC_I2C_PM_OPS (_i2c_pm_ops)
> -#else
> -#define MPC_I2C_PM_OPS NULL
> -#endif
>
> static const struct mpc_i2c_data mpc_i2c_data_512x = {
> .setup = mpc_i2c_setup_512x,
There's a reference to MPC_I2C_PM_OPS in mpc_i2c_driver which needs
changing I think the following is needed
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 1308f749dc75..7fde13472c09 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -862,7 +862,7 @@ static struct platform_driver mpc_i2c_driver = {
.driver = {
.name = DRV_NAME,
.of_match_table = mpc_i2c_of_match,
- .pm = MPC_I2C_PM_OPS,
+ .pm = _i2c_pm_ops,
},
};
Re: [PATCH v1 1/4] i2c: mpc: Use devm_clk_get_optional()
On 14/04/21 2:37 am, Andy Shevchenko wrote:
> The peripheral clock is optional and we may get an -EPROBE_DEFER error code
> which would not be propagated correctly, fix this by using
> devm_clk_get_optional().
>
> Signed-off-by: Andy Shevchenko
Reviewed-by: Chris Packham
None of the systems I have access to make use of the clocks property so
I can't do much testing. I can say it still handles the absence of a
clocks property correctly.
> ---
> drivers/i2c/busses/i2c-mpc.c | 25 -
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 3c8bcdfff7e7..6dc029a31d36 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -690,17 +690,18 @@ static int fsl_i2c_probe(struct platform_device *op)
>* enable clock for the I2C peripheral (non fatal),
>* keep a reference upon successful allocation
>*/
> - clk = devm_clk_get(>dev, NULL);
> - if (!IS_ERR(clk)) {
> - err = clk_prepare_enable(clk);
> - if (err) {
> - dev_err(>dev, "failed to enable clock\n");
> - return err;
> - } else {
> - i2c->clk_per = clk;
> - }
> + clk = devm_clk_get_optional(>dev, NULL);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + err = clk_prepare_enable(clk);
> + if (err) {
> + dev_err(>dev, "failed to enable clock\n");
> + return err;
> }
>
> + i2c->clk_per = clk;
> +
> if (of_property_read_bool(op->dev.of_node, "fsl,preserve-clocking")) {
> clock = MPC_I2C_CLOCK_PRESERVE;
> } else {
> @@ -744,8 +745,7 @@ static int fsl_i2c_probe(struct platform_device *op)
> return 0;
>
>fail_add:
> - if (i2c->clk_per)
> - clk_disable_unprepare(i2c->clk_per);
> + clk_disable_unprepare(i2c->clk_per);
>
> return result;
> };
> @@ -756,8 +756,7 @@ static int fsl_i2c_remove(struct platform_device *op)
>
> i2c_del_adapter(>adap);
>
> - if (i2c->clk_per)
> - clk_disable_unprepare(i2c->clk_per);
> + clk_disable_unprepare(i2c->clk_per);
>
> return 0;
> };
Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer
On 14/04/21 10:28 am, Chris Packham wrote: > > On 14/04/21 1:52 am, Andy Shevchenko wrote: >> On Tue, Apr 13, 2021 at 8:10 AM Chris Packham >> wrote: >>> The fsl-i2c controller will generate an interrupt after every byte >>> transferred. Make use of this interrupt to drive a state machine which >>> allows the next part of a transfer to happen as soon as the >>> interrupt is >>> received. This is particularly helpful with SMBUS devices like the LM81 >>> which will timeout if we take too long between bytes in a transfer. >> Also see my other comments below. >> >> ... >> ... >> >>> + dev_dbg(i2c->dev, "%s: %s %02x\n", __func__, >>> + action_str[i2c->action], byte); >> You already printed action. Anything changed? > It's mainly the addition of the byte read. I couldn't figure out a > sensible way of always printing the action then appending the data in > the read/write case. Open to suggestions. >> >>> + dev_dbg(i2c->dev, "%s: %s %02x\n", __func__, >>> + action_str[i2c->action], >>> msg->buf[i2c->byte_posn]); >> Deduplicate this. Perhaps at the end of switch-case print once with >> whatever temporary variable value you want to. >> >> ... > I thought about this but decided not to because in the write case it's > printed before going to hardware and in the read case it's after. If I > moved it after the case I'd have to use something other than > i2c->byte_posn which seemed error prone. I looked at a few options for this. Avoiding repeating the action is doable but I feel it needs to be replaced by something otherwise it's just a random byte value in the output (e.g. "buf = "/"byte = "). Rolling everything into a single line of output seems really hard to do to cover all the possible actions. Completely deduplicating this means I need to add code to store the action before the case and check it afterward which will run all the time. This seems overkill for the sake of avoiding duplicate code which is usually compiled out. I'm erring on the side of just removing __func__ and leaving the rest as-is. Unless you feel really strongly that something else should be done. One option not mentioned is to remove these two debug statements. I'd be OK with that.
Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer
On 14/04/21 1:52 am, Andy Shevchenko wrote:
> On Tue, Apr 13, 2021 at 8:10 AM Chris Packham
> wrote:
>> The fsl-i2c controller will generate an interrupt after every byte
>> transferred. Make use of this interrupt to drive a state machine which
>> allows the next part of a transfer to happen as soon as the interrupt is
>> received. This is particularly helpful with SMBUS devices like the LM81
>> which will timeout if we take too long between bytes in a transfer.
> Also see my other comments below.
>
> ...
>
>> +// SPDX-License-Identifier: GPL-2.0
> I think it is better to split this with a removal of old stuff and
> updating a copyright notice and go as a last one in the series.
>
> ...
Have split out into new patch.
>> +static char *action_str[] = {
> static const char * const action_str[]
Ack.
>> + "invalid",
>> + "start",
>> + "restart",
>> + "read begin",
>> + "read",
>> + "write",
>> + "stop",
>> +};
> ...
>
>> + dev_dbg(i2c->dev, "%s: action = %s\n", __func__,
>> + action_str[i2c->action]);
> Drop useless __func__. With Dynamic Debug enabled it can be turned on
> and off at run time.
Ack. Other instances of __func__ also.
>
> ...
>
>> + /* Generate txack on next to last byte */
> Tx ACK ? Ditto for other comments.
>
> ...
ACK.
>
>> + dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
>> + action_str[i2c->action], byte);
> You already printed action. Anything changed?
It's mainly the addition of the byte read. I couldn't figure out a
sensible way of always printing the action then appending the data in
the read/write case. Open to suggestions.
>
>> + dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
>> + action_str[i2c->action], msg->buf[i2c->byte_posn]);
> Deduplicate this. Perhaps at the end of switch-case print once with
> whatever temporary variable value you want to.
>
> ...
I thought about this but decided not to because in the write case it's
printed before going to hardware and in the read case it's after. If I
moved it after the case I'd have to use something other than
i2c->byte_posn which seemed error prone.
>
>> + case MPC_I2C_ACTION_INVALID:
>> + default:
> Does the first one deserve loud WARN?
> Otherwise, why is it defined at all?
I added MPC_I2C_ACTION_INVALID to make sure that a value of 0 was not
something that would naturally happen via a zeroed initialization. I
could probably achieve the same thing by making MPC_I2C_ACTION_START = 1.
>> + WARN(1, "Unexpected action %d\n", i2c->action);
>> + break;
> ...
>
>> +static void mpc_i2c_do_intr(struct mpc_i2c *i2c, u8 status)
>> {
>> + spin_lock_irqsave(>lock, flags);
> Why _irqsave?
>
> ...
Primarily because it's the only one I've ever used and it was the one
similar i2c drivers used when I started this work. I see they've now
been updated so I don't think there will be a problem switching to
spin_lock().
>> + dev_dbg(i2c->dev, "arbiritration lost\n");
> arbitration
Ack.
> ...
>
>> + if (i2c->expect_rxack && (status & CSR_RXAK)) {
>> + dev_dbg(i2c->dev, "no RXAK\n");
> You see, you have to be consistent in comments and messages.
> Either use TXAK/RXAK, or more verbose 'Tx ACK/Rx ACK' everywhere.
>
> ...
Updated to "Rx ACK". I think I've got them all now.
>
>> +out:
> out_unlock:
>
>> + spin_unlock_irqrestore(>lock, flags);
> ...
>
>> +static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
>> +{
>> + struct mpc_i2c *i2c = dev_id;
>> + u8 status = readb(i2c->base + MPC_I2C_SR);
> I would split this assignment, so it will be closer to its user.
Ack.
>> + if (status & CSR_MIF) {
>> + writeb(0, i2c->base + MPC_I2C_SR);
>> + mpc_i2c_do_intr(i2c, status);
>> + return IRQ_HANDLED;
>> }
>> + return IRQ_NONE;
>> +}
> ...
>
>> + time_left = wait_event_timeout(i2c->waitq, !i2c->block,
>> i2c->adap.timeout);
>> +
> No need for a blank line here.
Ack.
>> + if (!time_left)
>> + i2c->rc = -ETIMEDOUT;
>> + else if (time_left < 0)
> Redundant 'else'
Ack.
>> + i2c->rc = time_left;
> Can't you return an error code from here, rather than injecting it
> somewhere where it doesn't belong to?
Yes I think so. If I make mpc_i2c_wait_for_completion() return an int
then have mpc_i2c_execute_msg() check it and set i2c->rc if needed.
>> }
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH v3 3/3] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer
On 13/04/21 5:09 pm, Chris Packham wrote: > Add Chris Packham as FREESCALE MPC I2C maintainer. > > Signed-off-by: Chris Packham Sorry for the duplicate. I had existing output from an earlier invocation of git format-patch lying around. "[PATCH v3 4/4] MAINTAINERS: ..." is the one I intended to send (although the content is the same). > --- > MAINTAINERS | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 56e9e4d777d8..3bc77ba8cd05 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7135,6 +7135,13 @@ S: Maintained > F: Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml > F: drivers/i2c/busses/i2c-imx-lpi2c.c > > +FREESCALE MPC I2C DRIVER > +M: Chris Packham > +L: linux-...@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/i2c/i2c-mpc.yaml > +F: drivers/i2c/busses/i2c-mpc.c > + > FREESCALE QORIQ DPAA ETHERNET DRIVER > M: Madalin Bucur > L: net...@vger.kernel.org
[PATCH v3 4/4] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer
Add Chris Packham as FREESCALE MPC I2C maintainer. Signed-off-by: Chris Packham --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 56e9e4d777d8..3bc77ba8cd05 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7135,6 +7135,13 @@ S: Maintained F: Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml F: drivers/i2c/busses/i2c-imx-lpi2c.c +FREESCALE MPC I2C DRIVER +M: Chris Packham +L: linux-...@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/i2c/i2c-mpc.yaml +F: drivers/i2c/busses/i2c-mpc.c + FREESCALE QORIQ DPAA ETHERNET DRIVER M: Madalin Bucur L: net...@vger.kernel.org -- 2.31.1
[PATCH v3 1/4] i2c: mpc: use device managed APIs
Use device managed functions an clean up error handling.
Signed-off-by: Chris Packham
Signed-off-by: Wolfram Sang
---
Notes:
Changes in v3:
- Assuming 09aab7add7bf is reverted I've folded in the fix from Wei
Yongjun[1] into the original patch. If Wei's patch is applied on top
of whats already in i2c/for-next then this patch can be ignored.
[1] -
https://lore.kernel.org/linux-i2c/20210412160026.194423-1-weiyongj...@huawei.com/
drivers/i2c/busses/i2c-mpc.c | 52 +---
1 file changed, 18 insertions(+), 34 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 5b746a898e8e..6e5614acebac 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -654,7 +654,6 @@ static int fsl_i2c_probe(struct platform_device *op)
u32 clock = MPC_I2C_CLOCK_LEGACY;
int result = 0;
int plen;
- struct resource res;
struct clk *clk;
int err;
@@ -662,7 +661,7 @@ static int fsl_i2c_probe(struct platform_device *op)
if (!match)
return -EINVAL;
- i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+ i2c = devm_kzalloc(>dev, sizeof(*i2c), GFP_KERNEL);
if (!i2c)
return -ENOMEM;
@@ -670,24 +669,21 @@ static int fsl_i2c_probe(struct platform_device *op)
init_waitqueue_head(>queue);
- i2c->base = of_iomap(op->dev.of_node, 0);
- if (!i2c->base) {
+ i2c->base = devm_platform_ioremap_resource(op, 0);
+ if (IS_ERR(i2c->base)) {
dev_err(i2c->dev, "failed to map controller\n");
- result = -ENOMEM;
- goto fail_map;
+ return PTR_ERR(i2c->base);
}
- i2c->irq = irq_of_parse_and_map(op->dev.of_node, 0);
- if (i2c->irq < 0) {
- result = i2c->irq;
- goto fail_map;
- }
+ i2c->irq = platform_get_irq(op, 0);
+ if (i2c->irq < 0)
+ return i2c->irq;
- result = request_irq(i2c->irq, mpc_i2c_isr,
+ result = devm_request_irq(>dev, i2c->irq, mpc_i2c_isr,
IRQF_SHARED, "i2c-mpc", i2c);
if (result < 0) {
dev_err(i2c->dev, "failed to attach interrupt\n");
- goto fail_request;
+ return result;
}
/*
@@ -699,7 +695,7 @@ static int fsl_i2c_probe(struct platform_device *op)
err = clk_prepare_enable(clk);
if (err) {
dev_err(>dev, "failed to enable clock\n");
- goto fail_request;
+ return err;
} else {
i2c->clk_per = clk;
}
@@ -731,32 +727,26 @@ static int fsl_i2c_probe(struct platform_device *op)
}
dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 100 / HZ);
- platform_set_drvdata(op, i2c);
-
i2c->adap = mpc_ops;
- of_address_to_resource(op->dev.of_node, 0, );
scnprintf(i2c->adap.name, sizeof(i2c->adap.name),
- "MPC adapter at 0x%llx", (unsigned long long)res.start);
- i2c_set_adapdata(>adap, i2c);
+ "MPC adapter (%s)", of_node_full_name(op->dev.of_node));
i2c->adap.dev.parent = >dev;
+ i2c->adap.nr = op->id;
i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
i2c->adap.bus_recovery_info = _i2c_recovery_info;
+ platform_set_drvdata(op, i2c);
+ i2c_set_adapdata(>adap, i2c);
- result = i2c_add_adapter(>adap);
- if (result < 0)
+ result = i2c_add_numbered_adapter(>adap);
+ if (result)
goto fail_add;
- return result;
+ return 0;
fail_add:
if (i2c->clk_per)
clk_disable_unprepare(i2c->clk_per);
- free_irq(i2c->irq, i2c);
- fail_request:
- irq_dispose_mapping(i2c->irq);
- iounmap(i2c->base);
- fail_map:
- kfree(i2c);
+
return result;
};
@@ -769,12 +759,6 @@ static int fsl_i2c_remove(struct platform_device *op)
if (i2c->clk_per)
clk_disable_unprepare(i2c->clk_per);
- if (i2c->irq)
- free_irq(i2c->irq, i2c);
-
- irq_dispose_mapping(i2c->irq);
- iounmap(i2c->base);
- kfree(i2c);
return 0;
};
--
2.31.1
[PATCH v3 3/4] i2c: mpc: Remove redundant NULL check
In mpc_i2c_get_fdr_8xxx div is assigned as we iterate through the mpc_i2c_dividers_8xxx array. By the time we exit the loop div will either have the value that matches the requested speed or be pointing at the last entry in mpc_i2c_dividers_8xxx. Checking for div being NULL after the loop is redundant so remove the check. Reported-by: Wolfram Sang Signed-off-by: Chris Packham --- drivers/i2c/busses/i2c-mpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index 9818f9f6a553..c30687483147 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -377,7 +377,7 @@ static int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock, } *real_clk = fsl_get_sys_freq() / prescaler / div->divider; - return div ? (int)div->fdr : -EINVAL; + return (int)div->fdr; } static void mpc_i2c_setup_8xxx(struct device_node *node, -- 2.31.1
[PATCH v3 0/4] i2c: mpc: Refactor to improve responsiveness
This is an update to what is already in i2c/for-next. I've included "i2c: mpc: use device managed APIs" which had some problems in the remove code path which Wei Yongjun kindly pointed out with a fix. I've incorporated those changes into this version in case the original is reverted. I've tested on T2081 and P2041 based systems with a number of i2c and smbus devices. Also this time I included a few iterations of module insert/remove which would have caught the earlier errors. Chris Packham (4): i2c: mpc: use device managed APIs i2c: mpc: Interrupt driven transfer i2c: mpc: Remove redundant NULL check MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer MAINTAINERS | 7 + drivers/i2c/busses/i2c-mpc.c | 488 +++ 2 files changed, 267 insertions(+), 228 deletions(-) -- 2.31.1
[PATCH v3 2/4] i2c: mpc: Interrupt driven transfer
The fsl-i2c controller will generate an interrupt after every byte
transferred. Make use of this interrupt to drive a state machine which
allows the next part of a transfer to happen as soon as the interrupt is
received. This is particularly helpful with SMBUS devices like the LM81
which will timeout if we take too long between bytes in a transfer.
Signed-off-by: Chris Packham
---
Notes:
Changes in v3:
- use WARN/WARN_ON instead of BUG/BUG_ON
Changes in v2:
- add static_assert for state debug strings
- remove superfluous space
drivers/i2c/busses/i2c-mpc.c | 434 +++
1 file changed, 241 insertions(+), 193 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 6e5614acebac..9818f9f6a553 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -1,16 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
/*
- * (C) Copyright 2003-2004
- * Humboldt Solutions Ltd, adr...@humboldt.co.uk.
-
* This is a combined i2c adapter and algorithm driver for the
* MPC107/Tsi107 PowerPC northbridge and processors that include
* the same I2C unit (8240, 8245, 85xx).
*
- * Release 0.8
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
+ * Copyright (C) 2003-2004 Humboldt Solutions Ltd, adr...@humboldt.co.uk
+ * Copyright (C) 2021 Allied Telesis Labs
*/
#include
@@ -58,11 +53,36 @@
#define CSR_MIF 0x02
#define CSR_RXAK 0x01
+enum mpc_i2c_action {
+ MPC_I2C_ACTION_INVALID = 0,
+ MPC_I2C_ACTION_START,
+ MPC_I2C_ACTION_RESTART,
+ MPC_I2C_ACTION_READ_BEGIN,
+ MPC_I2C_ACTION_READ_BYTE,
+ MPC_I2C_ACTION_WRITE,
+ MPC_I2C_ACTION_STOP,
+
+ __MPC_I2C_ACTION_CNT
+};
+
+static char *action_str[] = {
+ "invalid",
+ "start",
+ "restart",
+ "read begin",
+ "read",
+ "write",
+ "stop",
+};
+
+static_assert(ARRAY_SIZE(action_str) == __MPC_I2C_ACTION_CNT);
+
struct mpc_i2c {
struct device *dev;
void __iomem *base;
u32 interrupt;
- wait_queue_head_t queue;
+ wait_queue_head_t waitq;
+ spinlock_t lock;
struct i2c_adapter adap;
int irq;
u32 real_clk;
@@ -70,6 +90,16 @@ struct mpc_i2c {
u8 fdr, dfsrr;
#endif
struct clk *clk_per;
+ u32 cntl_bits;
+ enum mpc_i2c_action action;
+ struct i2c_msg *msgs;
+ int num_msgs;
+ int curr_msg;
+ u32 byte_posn;
+ u32 block;
+ int rc;
+ int expect_rxack;
+
};
struct mpc_i2c_divider {
@@ -86,19 +116,6 @@ static inline void writeccr(struct mpc_i2c *i2c, u32 x)
writeb(x, i2c->base + MPC_I2C_CR);
}
-static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
-{
- struct mpc_i2c *i2c = dev_id;
- if (readb(i2c->base + MPC_I2C_SR) & CSR_MIF) {
- /* Read again to allow register to stabilise */
- i2c->interrupt = readb(i2c->base + MPC_I2C_SR);
- writeb(0, i2c->base + MPC_I2C_SR);
- wake_up(>queue);
- return IRQ_HANDLED;
- }
- return IRQ_NONE;
-}
-
/* Sometimes 9th clock pulse isn't generated, and slave doesn't release
* the bus, because it wants to send ACK.
* Following sequence of enabling/disabling and sending start/stop generates
@@ -121,45 +138,6 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
}
}
-static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
-{
- u32 cmd_err;
- int result;
-
- result = wait_event_timeout(i2c->queue,
- (i2c->interrupt & CSR_MIF), timeout);
-
- if (unlikely(!(i2c->interrupt & CSR_MIF))) {
- dev_dbg(i2c->dev, "wait timeout\n");
- writeccr(i2c, 0);
- result = -ETIMEDOUT;
- }
-
- cmd_err = i2c->interrupt;
- i2c->interrupt = 0;
-
- if (result < 0)
- return result;
-
- if (!(cmd_err & CSR_MCF)) {
- dev_dbg(i2c->dev, "unfinished\n");
- return -EIO;
- }
-
- if (cmd_err & CSR_MAL) {
- dev_dbg(i2c->dev, "MAL\n");
- return -EAGAIN;
- }
-
- if (writing && (cmd_err & CSR_RXAK)) {
- dev_dbg(i2c->dev, "No RXAK\n");
- /* generate stop */
- writeccr(i2c, CCR_MEN);
- return -ENXIO;
- }
- return 0;
-}
-
#if defined(CONFIG_PPC_MPC52xx) || defined(CONFIG_PPC_MPC512x)
static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = {
{20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
@@ -434,168 +412,2
[PATCH v3 3/3] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer
Add Chris Packham as FREESCALE MPC I2C maintainer. Signed-off-by: Chris Packham --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 56e9e4d777d8..3bc77ba8cd05 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7135,6 +7135,13 @@ S: Maintained F: Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml F: drivers/i2c/busses/i2c-imx-lpi2c.c +FREESCALE MPC I2C DRIVER +M: Chris Packham +L: linux-...@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/i2c/i2c-mpc.yaml +F: drivers/i2c/busses/i2c-mpc.c + FREESCALE QORIQ DPAA ETHERNET DRIVER M: Madalin Bucur L: net...@vger.kernel.org -- 2.31.1
Re: [PATCH v2 5/6] i2c: mpc: use device managed APIs
On 13/04/21 11:21 am, Chris Packham wrote: > > On 13/04/21 10:52 am, Andy Shevchenko wrote: >> On Mon, Mar 29, 2021 at 4:54 AM Chris Packham >> wrote: >>> Use device managed functions an clean up error handling. >> For the god sake how have you tested this? >> The patch is broken. > I've clearly missed the remove path in my testing. I was focused on > the interrupt bevhaviour not the probe/remove which I'll make sure to > check for the next round. Should I send a revert or leave it for Wolfram?
Re: [PATCH v2 5/6] i2c: mpc: use device managed APIs
On 13/04/21 10:52 am, Andy Shevchenko wrote: > On Mon, Mar 29, 2021 at 4:54 AM Chris Packham > wrote: >> Use device managed functions an clean up error handling. > For the god sake how have you tested this? > The patch is broken. I've clearly missed the remove path in my testing. I was focused on the interrupt bevhaviour not the probe/remove which I'll make sure to check for the next round.
Re: [PATCH v2 4/6] i2c: mpc: make interrupt mandatory and remove polling code
On 11/04/21 8:16 am, Wolfram Sang wrote: > On Mon, Mar 29, 2021 at 02:52:04PM +1300, Chris Packham wrote: >> All the in-tree dts files that use one of the compatible strings from >> i2c-mpc.c provide an interrupt property. By making this mandatory we >> can simplify the code. >> >> Signed-off-by: Chris Packham > After I applied this patch, cppcheck reports: > > CPPCHECK > drivers/i2c/busses/i2c-mpc.c:401:47: warning: Either the condition > 'div?(int)div->fdr:-EINVAL' is redundant or there is possible null pointer > dereference: div. [nullPointerRedundantCheck] > *real_clk = fsl_get_sys_freq() / prescaler / div->divider; >^ > drivers/i2c/busses/i2c-mpc.c:402:13: note: Assuming that condition > 'div?(int)div->fdr:-EINVAL' is not redundant > return div ? (int)div->fdr : -EINVAL; > ^ > drivers/i2c/busses/i2c-mpc.c:401:47: note: Null pointer dereference > *real_clk = fsl_get_sys_freq() / prescaler / div->divider; >^ > Can you check this? I'd think we can fix it incrementally... > What are the arguments passed to cppcheck? I've tried two versions I have easy access to (1.82 and 1.86) neither report problems when invoked as `cppcheck drivers/i2c/busses/i2c-mpc.c` nor do they complain about this with `--enable=all`. Looking at the code I can see what it's complaining about, div should have a value since mpc_i2c_dividers_8xxx does not have a sentinel value so I think the div check is unnecessary.
Re: [PATCH v2 6/6] i2c: mpc: Interrupt driven transfer
On 11/04/21 8:13 am, Wolfram Sang wrote: > On Mon, Mar 29, 2021 at 02:52:06PM +1300, Chris Packham wrote: >> The fsl-i2c controller will generate an interrupt after every byte >> transferred. Make use of this interrupt to drive a state machine which >> allows the next part of a transfer to happen as soon as the interrupt is >> received. This is particularly helpful with SMBUS devices like the LM81 >> which will timeout if we take too long between bytes in a transfer. >> >> Signed-off-by: Chris Packham > Okay, this change is too large and HW specific for a detailed review. > But I trust you and hope you will be around to fix regressions if I > apply it for 5.13? Yep I plan on being around. I've got access to a couple of designs with P2040 and T2081 so hopefully that's sufficient to deal with any regressions. One issue is a lack of different i2c devices (the systems we have tend to use the same devices) but hopefully any reports of regression will be from people with access to such devices. > That kind of leads to the question if you want to > step up as the maintainer for this driver? Sure can do. It'd be nice if it was someone from NXP but I think they've lost interest in the PowerPC based SoCs. Should I send a patch for MAINTAINERS? If so does that go through the i2c tree? > Only thing I noticed was a "BUG" and "BUG_ON" and wonder if we really > need to halt the kernel in that case. Maybe WARN is enough? Yeah I think they can both be WARN variants. The one in mpc_xfer() can happily continue. It's a little less clear what I should do in mpc_i2c_do_action() if the WARN is ever hit but in theory it should be an unreachable case anyway so the only thing that could get there is some kind of memory corruption which would likely cause a crash elsewhere. Do you want me to send a V3 of just that patch? > I'll apply the first five patches now, they look good to me. >
Re: linux-next: build warning after merge of the hwmon-staging tree
My bad. I'll send a patch shortly
On 30/03/21 8:27 pm, Stephen Rothwell wrote:
> Hi all,
>
> After merging the hwmon-staging tree, today's linux-next build (htmldocs)
> produced this warning:
>
> Documentation/hwmon/bpa-rs600.rst: WARNING: document isn't included in any
> toctree
>
> Introduced by commit
>
>9a8210575cde ("hwmon: (pmbus) Add driver for BluTek BPA-RS600")
>
Linux include/uapi/linux/libc-compat.h and Musl include/netinet/in.h incompatibility for __UAPI_DEF_IN6_ADDR_ALT
Hi, I've come over from https://github.com/strace/strace/issues/177 there's a bit of context there. Crosstool-ng has hit a problem when building a recent enough version of strace in a configuration that uses musl libc. The error is [ALL ]In file included from /home/x-tool/.build/arm-unknown-linux-musleabi/src/strace/bundled/linux/include/uapi/linux/in6.h:26, [ALL ] from /home/x-tool/.build/arm-unknown-linux-musleabi/src/strace/bundled/linux/include/uapi/linux/if_bridge.h:19, [ALL ] from /home/x-tool/.build/arm-unknown-linux-musleabi/src/strace/src/rtnl_mdb.c:16: [ERROR] /home/x-tool/.build/arm-unknown-linux-musleabi/src/strace/bundled/linux/include/uapi/linux/libc-compat.h:109: error: "__UAPI_DEF_IN6_ADDR_ALT" redefined [-Werror] [ALL ] 109 | #define __UAPI_DEF_IN6_ADDR_ALT 1 [ALL ] | [ALL ]In file included from /home/x-tool/.build/arm-unknown-linux-musleabi/src/strace/src/rtnl_mdb.c:15: [ALL ] /home/x-tool/x-tools/arm-unknown-linux-musleabi/arm-unknown-linux-musleabi/sysroot/usr/include/netinet/in.h:401: note: this is the location of the previous definition [ALL ] 401 | #define __UAPI_DEF_IN6_ADDR_ALT 0 [ALL ] | [ALL ]cc1: all warnings being treated as errors [ERROR]make[4]: *** [Makefile:6660: libstrace_a-rtnl_mdb.o] Error 1 [ALL ]make[4]: Leaving directory '/home/x-tool/.build/arm-unknown-linux-musleabi/build/build-strace/src' [ERROR]make[3]: *** [Makefile:2404: all] Error 2 [ALL ]rm ioctlsort0.o ioctls_all0.h ioctlsort0 [ALL ]make[3]: Leaving directory '/home/x-tool/.build/arm-unknown-linux-musleabi/build/build-strace/src' [ERROR]make[2]: *** [Makefile:601: all-recursive] Error 1 [ALL ]make[2]: Leaving directory '/home/x-tool/.build/arm-unknown-linux-musleabi/build/build-strace' [ERROR]make[1]: *** [Makefile:506: all] Error 2 [ALL ]make[1]: Leaving directory '/home/x-tool/.build/arm-unknown-linux-musleabi/build/build-strace' It appears that the bundled uapi headers definition of __UAPI_DEF_IN6_ADDR_ALT conflicts with the musl libc definition. It looks like libc-compat.h tries to co-exists with GNU libc but this isn't working for musl. I've identified an egregious hack that I think should make things work (CFLAGS+=-D__USE_MISC) but I wanted to know if there was a nicer way to make this work. Regards, Chris
[PATCH v2 6/6] i2c: mpc: Interrupt driven transfer
The fsl-i2c controller will generate an interrupt after every byte
transferred. Make use of this interrupt to drive a state machine which
allows the next part of a transfer to happen as soon as the interrupt is
received. This is particularly helpful with SMBUS devices like the LM81
which will timeout if we take too long between bytes in a transfer.
Signed-off-by: Chris Packham
---
Notes:
Changes in v2:
- add static_assert for state debug strings
- remove superfluous space
drivers/i2c/busses/i2c-mpc.c | 434 +++
1 file changed, 241 insertions(+), 193 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 46cdb36e2f9b..f5c155125de9 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -1,16 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
/*
- * (C) Copyright 2003-2004
- * Humboldt Solutions Ltd, adr...@humboldt.co.uk.
-
* This is a combined i2c adapter and algorithm driver for the
* MPC107/Tsi107 PowerPC northbridge and processors that include
* the same I2C unit (8240, 8245, 85xx).
*
- * Release 0.8
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
+ * Copyright (C) 2003-2004 Humboldt Solutions Ltd, adr...@humboldt.co.uk
+ * Copyright (C) 2021 Allied Telesis Labs
*/
#include
@@ -58,11 +53,36 @@
#define CSR_MIF 0x02
#define CSR_RXAK 0x01
+enum mpc_i2c_action {
+ MPC_I2C_ACTION_INVALID = 0,
+ MPC_I2C_ACTION_START,
+ MPC_I2C_ACTION_RESTART,
+ MPC_I2C_ACTION_READ_BEGIN,
+ MPC_I2C_ACTION_READ_BYTE,
+ MPC_I2C_ACTION_WRITE,
+ MPC_I2C_ACTION_STOP,
+
+ __MPC_I2C_ACTION_CNT
+};
+
+static char *action_str[] = {
+ "invalid",
+ "start",
+ "restart",
+ "read begin",
+ "read",
+ "write",
+ "stop",
+};
+
+static_assert(ARRAY_SIZE(action_str) == __MPC_I2C_ACTION_CNT);
+
struct mpc_i2c {
struct device *dev;
void __iomem *base;
u32 interrupt;
- wait_queue_head_t queue;
+ wait_queue_head_t waitq;
+ spinlock_t lock;
struct i2c_adapter adap;
int irq;
u32 real_clk;
@@ -70,6 +90,16 @@ struct mpc_i2c {
u8 fdr, dfsrr;
#endif
struct clk *clk_per;
+ u32 cntl_bits;
+ enum mpc_i2c_action action;
+ struct i2c_msg *msgs;
+ int num_msgs;
+ int curr_msg;
+ u32 byte_posn;
+ u32 block;
+ int rc;
+ int expect_rxack;
+
};
struct mpc_i2c_divider {
@@ -86,19 +116,6 @@ static inline void writeccr(struct mpc_i2c *i2c, u32 x)
writeb(x, i2c->base + MPC_I2C_CR);
}
-static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
-{
- struct mpc_i2c *i2c = dev_id;
- if (readb(i2c->base + MPC_I2C_SR) & CSR_MIF) {
- /* Read again to allow register to stabilise */
- i2c->interrupt = readb(i2c->base + MPC_I2C_SR);
- writeb(0, i2c->base + MPC_I2C_SR);
- wake_up(>queue);
- return IRQ_HANDLED;
- }
- return IRQ_NONE;
-}
-
/* Sometimes 9th clock pulse isn't generated, and slave doesn't release
* the bus, because it wants to send ACK.
* Following sequence of enabling/disabling and sending start/stop generates
@@ -121,45 +138,6 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
}
}
-static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
-{
- u32 cmd_err;
- int result;
-
- result = wait_event_timeout(i2c->queue,
- (i2c->interrupt & CSR_MIF), timeout);
-
- if (unlikely(!(i2c->interrupt & CSR_MIF))) {
- dev_dbg(i2c->dev, "wait timeout\n");
- writeccr(i2c, 0);
- result = -ETIMEDOUT;
- }
-
- cmd_err = i2c->interrupt;
- i2c->interrupt = 0;
-
- if (result < 0)
- return result;
-
- if (!(cmd_err & CSR_MCF)) {
- dev_dbg(i2c->dev, "unfinished\n");
- return -EIO;
- }
-
- if (cmd_err & CSR_MAL) {
- dev_dbg(i2c->dev, "MAL\n");
- return -EAGAIN;
- }
-
- if (writing && (cmd_err & CSR_RXAK)) {
- dev_dbg(i2c->dev, "No RXAK\n");
- /* generate stop */
- writeccr(i2c, CCR_MEN);
- return -ENXIO;
- }
- return 0;
-}
-
#if defined(CONFIG_PPC_MPC52xx) || defined(CONFIG_PPC_MPC512x)
static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = {
{20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
@@ -434,168 +412,209 @@ static void mpc_i2c_setup_8xxx(struct device_node *node,
}
#endif /*
[PATCH v2 3/6] i2c: mpc: Make use of i2c_recover_bus()
Move the existing calls of mpc_i2c_fixup() to a recovery function
registered via bus_recovery_info. This makes it more obvious that
recovery is supported and allows for a future where recovery is
triggered by the i2c core.
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d94f05c8b8b7..6a0d55e9e8e3 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -586,7 +586,7 @@ static int mpc_xfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
writeb(status & ~CSR_MAL,
i2c->base + MPC_I2C_SR);
- mpc_i2c_fixup(i2c);
+ i2c_recover_bus(>adap);
}
return -EIO;
}
@@ -622,7 +622,7 @@ static int mpc_xfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
writeb(status & ~CSR_MAL,
i2c->base + MPC_I2C_SR);
- mpc_i2c_fixup(i2c);
+ i2c_recover_bus(>adap);
}
return -EIO;
}
@@ -637,6 +637,15 @@ static u32 mpc_functionality(struct i2c_adapter *adap)
| I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL;
}
+static int fsl_i2c_bus_recovery(struct i2c_adapter *adap)
+{
+ struct mpc_i2c *i2c = i2c_get_adapdata(adap);
+
+ mpc_i2c_fixup(i2c);
+
+ return 0;
+}
+
static const struct i2c_algorithm mpc_algo = {
.master_xfer = mpc_xfer,
.functionality = mpc_functionality,
@@ -648,6 +657,10 @@ static struct i2c_adapter mpc_ops = {
.timeout = HZ,
};
+static struct i2c_bus_recovery_info fsl_i2c_recovery_info = {
+ .recover_bus = fsl_i2c_bus_recovery,
+};
+
static const struct of_device_id mpc_i2c_of_match[];
static int fsl_i2c_probe(struct platform_device *op)
{
@@ -740,6 +753,7 @@ static int fsl_i2c_probe(struct platform_device *op)
i2c_set_adapdata(>adap, i2c);
i2c->adap.dev.parent = >dev;
i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
+ i2c->adap.bus_recovery_info = _i2c_recovery_info;
result = i2c_add_adapter(>adap);
if (result < 0)
--
2.31.0
[PATCH v2 0/6] i2c: mpc: Refactor to improve responsiveness
The "meat" of this series is in the last patch which is the change that actually starts making use of the interrupts to drive a state machine. The dt-bindings patches can probably go in at any time, the rest of the series isn't dependent on them. I've tested on T2081 and P2041 based systems with a number of i2c and smbus devices. Chris Packham (6): dt-bindings: i2c-mpc: Document interrupt property as required dt-bindings: i2c: convert i2c-mpc to json-schema i2c: mpc: Make use of i2c_recover_bus() i2c: mpc: make interrupt mandatory and remove polling code i2c: mpc: use device managed APIs i2c: mpc: Interrupt driven transfer .../devicetree/bindings/i2c/i2c-mpc.txt | 62 --- .../devicetree/bindings/i2c/i2c-mpc.yaml | 91 +++ drivers/i2c/busses/i2c-mpc.c | 517 ++ 3 files changed, 369 insertions(+), 301 deletions(-) delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml -- 2.31.0
[PATCH v2 4/6] i2c: mpc: make interrupt mandatory and remove polling code
All the in-tree dts files that use one of the compatible strings from
i2c-mpc.c provide an interrupt property. By making this mandatory we
can simplify the code.
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 51 ++--
1 file changed, 19 insertions(+), 32 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 6a0d55e9e8e3..5b746a898e8e 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -123,37 +123,21 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
{
- unsigned long orig_jiffies = jiffies;
u32 cmd_err;
- int result = 0;
+ int result;
- if (!i2c->irq) {
- while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
- schedule();
- if (time_after(jiffies, orig_jiffies + timeout)) {
- dev_dbg(i2c->dev, "timeout\n");
- writeccr(i2c, 0);
- result = -ETIMEDOUT;
- break;
- }
- }
- cmd_err = readb(i2c->base + MPC_I2C_SR);
- writeb(0, i2c->base + MPC_I2C_SR);
- } else {
- /* Interrupt mode */
- result = wait_event_timeout(i2c->queue,
+ result = wait_event_timeout(i2c->queue,
(i2c->interrupt & CSR_MIF), timeout);
- if (unlikely(!(i2c->interrupt & CSR_MIF))) {
- dev_dbg(i2c->dev, "wait timeout\n");
- writeccr(i2c, 0);
- result = -ETIMEDOUT;
- }
-
- cmd_err = i2c->interrupt;
- i2c->interrupt = 0;
+ if (unlikely(!(i2c->interrupt & CSR_MIF))) {
+ dev_dbg(i2c->dev, "wait timeout\n");
+ writeccr(i2c, 0);
+ result = -ETIMEDOUT;
}
+ cmd_err = i2c->interrupt;
+ i2c->interrupt = 0;
+
if (result < 0)
return result;
@@ -694,13 +678,16 @@ static int fsl_i2c_probe(struct platform_device *op)
}
i2c->irq = irq_of_parse_and_map(op->dev.of_node, 0);
- if (i2c->irq) { /* no i2c->irq implies polling */
- result = request_irq(i2c->irq, mpc_i2c_isr,
-IRQF_SHARED, "i2c-mpc", i2c);
- if (result < 0) {
- dev_err(i2c->dev, "failed to attach interrupt\n");
- goto fail_request;
- }
+ if (i2c->irq < 0) {
+ result = i2c->irq;
+ goto fail_map;
+ }
+
+ result = request_irq(i2c->irq, mpc_i2c_isr,
+ IRQF_SHARED, "i2c-mpc", i2c);
+ if (result < 0) {
+ dev_err(i2c->dev, "failed to attach interrupt\n");
+ goto fail_request;
}
/*
--
2.31.0
[PATCH v2 5/6] i2c: mpc: use device managed APIs
Use device managed functions an clean up error handling.
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 46 ++--
1 file changed, 18 insertions(+), 28 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 5b746a898e8e..46cdb36e2f9b 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -654,7 +654,6 @@ static int fsl_i2c_probe(struct platform_device *op)
u32 clock = MPC_I2C_CLOCK_LEGACY;
int result = 0;
int plen;
- struct resource res;
struct clk *clk;
int err;
@@ -662,7 +661,7 @@ static int fsl_i2c_probe(struct platform_device *op)
if (!match)
return -EINVAL;
- i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+ i2c = devm_kzalloc(>dev, sizeof(*i2c), GFP_KERNEL);
if (!i2c)
return -ENOMEM;
@@ -670,24 +669,21 @@ static int fsl_i2c_probe(struct platform_device *op)
init_waitqueue_head(>queue);
- i2c->base = of_iomap(op->dev.of_node, 0);
- if (!i2c->base) {
+ i2c->base = devm_platform_ioremap_resource(op, 0);
+ if (IS_ERR(i2c->base)) {
dev_err(i2c->dev, "failed to map controller\n");
- result = -ENOMEM;
- goto fail_map;
+ return PTR_ERR(i2c->base);
}
- i2c->irq = irq_of_parse_and_map(op->dev.of_node, 0);
- if (i2c->irq < 0) {
- result = i2c->irq;
- goto fail_map;
- }
+ i2c->irq = platform_get_irq(op, 0);
+ if (i2c->irq < 0)
+ return i2c->irq;
- result = request_irq(i2c->irq, mpc_i2c_isr,
+ result = devm_request_irq(>dev, i2c->irq, mpc_i2c_isr,
IRQF_SHARED, "i2c-mpc", i2c);
if (result < 0) {
dev_err(i2c->dev, "failed to attach interrupt\n");
- goto fail_request;
+ return result;
}
/*
@@ -699,7 +695,7 @@ static int fsl_i2c_probe(struct platform_device *op)
err = clk_prepare_enable(clk);
if (err) {
dev_err(>dev, "failed to enable clock\n");
- goto fail_request;
+ return err;
} else {
i2c->clk_per = clk;
}
@@ -731,32 +727,26 @@ static int fsl_i2c_probe(struct platform_device *op)
}
dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 100 / HZ);
- platform_set_drvdata(op, i2c);
-
i2c->adap = mpc_ops;
- of_address_to_resource(op->dev.of_node, 0, );
scnprintf(i2c->adap.name, sizeof(i2c->adap.name),
- "MPC adapter at 0x%llx", (unsigned long long)res.start);
- i2c_set_adapdata(>adap, i2c);
+ "MPC adapter (%s)", of_node_full_name(op->dev.of_node));
i2c->adap.dev.parent = >dev;
+ i2c->adap.nr = op->id;
i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
i2c->adap.bus_recovery_info = _i2c_recovery_info;
+ platform_set_drvdata(op, i2c);
+ i2c_set_adapdata(>adap, i2c);
- result = i2c_add_adapter(>adap);
- if (result < 0)
+ result = i2c_add_numbered_adapter(>adap);
+ if (result)
goto fail_add;
- return result;
+ return 0;
fail_add:
if (i2c->clk_per)
clk_disable_unprepare(i2c->clk_per);
- free_irq(i2c->irq, i2c);
- fail_request:
- irq_dispose_mapping(i2c->irq);
- iounmap(i2c->base);
- fail_map:
- kfree(i2c);
+
return result;
};
--
2.31.0
[PATCH v2 1/6] dt-bindings: i2c-mpc: Document interrupt property as required
All of the in-tree device-trees that use the one of the compatible strings from i2c-mpc.c supply an interrupts property. Make this property mandatory to aid refactoring the driver. Signed-off-by: Chris Packham --- Documentation/devicetree/bindings/i2c/i2c-mpc.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt index 42a390526957..b15acb43d84d 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt @@ -7,14 +7,14 @@ Required properties : compatible processor, e.g. mpc8313, mpc8543, mpc8544, mpc5121, mpc5200 or mpc5200b. For the mpc5121, an additional node "fsl,mpc5121-i2c-ctrl" is required as shown in the example below. - -Recommended properties : - - interrupts : where a is the interrupt number and b is a field that represents an encoding of the sense and level information for the interrupt. This should be encoded based on the information in section 2) depending on the type of interrupt controller you have. + +Recommended properties : + - fsl,preserve-clocking : boolean; if defined, the clock settings from the bootloader are preserved (not touched). - clock-frequency : desired I2C bus clock frequency in Hz. -- 2.31.0
[PATCH v2 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
Convert i2c-mpc to YAML.
Signed-off-by: Chris Packham
---
Notes:
Changes in v2:
- Rework compatible validation
- Remove irrelevant i2ccontrol from example
.../devicetree/bindings/i2c/i2c-mpc.txt | 62 -
.../devicetree/bindings/i2c/i2c-mpc.yaml | 91 +++
2 files changed, 91 insertions(+), 62 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
deleted file mode 100644
index b15acb43d84d..
--- a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
+++ /dev/null
@@ -1,62 +0,0 @@
-* I2C
-
-Required properties :
-
- - reg : Offset and length of the register set for the device
- - compatible : should be "fsl,CHIP-i2c" where CHIP is the name of a
- compatible processor, e.g. mpc8313, mpc8543, mpc8544, mpc5121,
- mpc5200 or mpc5200b. For the mpc5121, an additional node
- "fsl,mpc5121-i2c-ctrl" is required as shown in the example below.
- - interrupts : where a is the interrupt number and b is a
- field that represents an encoding of the sense and level
- information for the interrupt. This should be encoded based on
- the information in section 2) depending on the type of interrupt
- controller you have.
-
-Recommended properties :
-
- - fsl,preserve-clocking : boolean; if defined, the clock settings
- from the bootloader are preserved (not touched).
- - clock-frequency : desired I2C bus clock frequency in Hz.
- - fsl,timeout : I2C bus timeout in microseconds.
-
-Examples :
-
- /* MPC5121 based board */
- i2c@1740 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "fsl,mpc5121-i2c", "fsl-i2c";
- reg = <0x1740 0x20>;
- interrupts = <11 0x8>;
- interrupt-parent = <>;
- clock-frequency = <10>;
- };
-
- i2ccontrol@1760 {
- compatible = "fsl,mpc5121-i2c-ctrl";
- reg = <0x1760 0x8>;
- };
-
- /* MPC5200B based board */
- i2c@3d00 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "fsl,mpc5200b-i2c","fsl,mpc5200-i2c","fsl-i2c";
- reg = <0x3d00 0x40>;
- interrupts = <2 15 0>;
- interrupt-parent = <_pic>;
- fsl,preserve-clocking;
- };
-
- /* MPC8544 base board */
- i2c@3100 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "fsl,mpc8544-i2c", "fsl-i2c";
- reg = <0x3100 0x100>;
- interrupts = <43 2>;
- interrupt-parent = <>;
- clock-frequency = <40>;
- fsl,timeout = <1>;
- };
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
new file mode 100644
index ..7b553d559c83
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/i2c-mpc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C-Bus adapter for MPC824x/83xx/85xx/86xx/512x/52xx SoCs
+
+maintainers:
+ - Chris Packham
+
+allOf:
+ - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+ compatible:
+oneOf:
+ - items:
+ - enum:
+ - mpc5200-i2c
+ - fsl,mpc5200-i2c
+ - fsl,mpc5121-i2c
+ - fsl,mpc8313-i2c
+ - fsl,mpc8543-i2c
+ - fsl,mpc8544-i2c
+ - const: fsl-i2c
+ - items:
+ - const: fsl,mpc5200b-i2c
+ - const: fsl,mpc5200-i2c
+ - const: fsl-i2c
+
+ reg:
+maxItems: 1
+
+ interrupts:
+maxItems: 1
+
+ fsl,preserve-clocking:
+$ref: /schemas/types.yaml#/definitions/flag
+description: |
+ if defined, the clock settings from the bootloader are
+ preserved (not touched)
+
+ fsl,timeout:
+$ref: /schemas/types.yaml#/definitions/uint32
+description: |
+ I2C bus timeout in microseconds
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+unevaluatedProperties: false
+
+examples:
+ - |
+/* MPC5121 based board */
+i2c@1740 {
+#address-cells = <1>;
+#size-cells = <0>;
+compatible = "fsl,mpc5121-i2c", "fsl-i2c";
+reg = <0x1740 0x20>;
+interrupts = <11 0x8>;
+interrupt-parent = <>;
+clock-freque
Re: [PATCH 0/6] i2c: mpc: Refactor to improve responsiveness
On 23/03/21 5:33 pm, Chris Packham wrote: > The "meat" of this series is in the last patch which is the change that > actually starts making use of the interrupts to drive a state machine. > The dt-bindings patches can probably go in at any time. The rest of the > series isn't dependent on them. > > I've tested it on a T2081 based system with a number of i2c and smbus > devices. Its the end of my work day so I figured I'd get this out now > but I'll do some more testing on a P2041 board and a few different i2c > devices tomorrow. I've done more testing on a T2081 and P2041 board. Both look good. I've had some feedback from Rob on the dt-bindings which I think I've got sorted now. I've got a couple of minor cosmetic changes to 6/6 but I'll hold fire on sending a v2 to give people a chance to look at the functional changes. > Chris Packham (6): >dt-bindings: i2c-mpc: Document interrupt property as required >dt-bindings: i2c: convert i2c-mpc to json-schema >i2c: mpc: Make use of i2c_recover_bus() >i2c: mpc: make interrupt mandatory and remove polling code >i2c: mpc: use device managed APIs >i2c: mpc: Interrupt driven transfer > > .../devicetree/bindings/i2c/i2c-mpc.txt | 62 --- > .../devicetree/bindings/i2c/i2c-mpc.yaml | 99 > drivers/i2c/busses/i2c-mpc.c | 513 ++ > 3 files changed, 373 insertions(+), 301 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml >
Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
On 24/03/21 10:59 am, Chris Packham wrote: > > On 24/03/21 10:15 am, Rob Herring wrote: >> On Tue, Mar 23, 2021 at 05:33:27PM +1300, Chris Packham wrote: >>> Convert i2c-mpc to YAML. >>> >>> Signed-off-by: Chris Packham >>> --- >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml >>> @@ -0,0 +1,99 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/i2c/i2c-mpc.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: I2C-Bus adapter for MPC824x/83xx/85xx/86xx/512x/52xx SoCs >>> + >>> +maintainers: >>> + - Chris Packham >>> + >>> +allOf: >>> + - $ref: /schemas/i2c/i2c-controller.yaml# >>> + >>> +properties: >>> + compatible: >>> + anyOf: >>> + - items: >>> + - enum: >>> + - mpc5200-i2c >>> + - fsl,mpc5200b-i2c >>> + - fsl,mpc5200-i2c >>> + - fsl,mpc5121-i2c >>> + - fsl,mpc8313-i2c >>> + - fsl,mpc8543-i2c >>> + - fsl,mpc8544-i2c >>> + >>> + - const: fsl-i2c >>> + >>> + - contains: >>> + const: fsl-i2c >>> + minItems: 1 >>> + maxItems: 4 >> Can't we drop this and list out any other compatibles? > > I'm struggling a little bit with how to get the schema right to allow > one or more of a set of compatible values. > > Basically I want to allow 'compatible = "fsl-i2c";' or 'compatible = > "fsl,mpc8544-i2c", "fsl-i2c";' but disallow 'compatible = "foobar", > "fsl-i2c";' This is what I've ended up with properties: compatible: oneOf: - items: - enum: - mpc5200-i2c - fsl,mpc5200-i2c - fsl,mpc5121-i2c - fsl,mpc8313-i2c - fsl,mpc8543-i2c - fsl,mpc8544-i2c - fsl-i2c - const: fsl-i2c - items: - const: fsl,mpc5200b-i2c - const: fsl,mpc5200-i2c - const: fsl-i2c It passes `make dt_binding_check` and rejects things when I add other non-documented values to the compatible property. I did struggle with it so I'm not confident it's the best approach but it seems to work.
Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
On 24/03/21 10:15 am, Rob Herring wrote:
> On Tue, Mar 23, 2021 at 05:33:27PM +1300, Chris Packham wrote:
>> Convert i2c-mpc to YAML.
>>
>> Signed-off-by: Chris Packham
>> ---
>> .../devicetree/bindings/i2c/i2c-mpc.txt | 62
>> .../devicetree/bindings/i2c/i2c-mpc.yaml | 99 +++
>> 2 files changed, 99 insertions(+), 62 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
>> b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
>> deleted file mode 100644
>> index b15acb43d84d..
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
>> +++ /dev/null
>> @@ -1,62 +0,0 @@
>> -* I2C
>> -
>> -Required properties :
>> -
>> - - reg : Offset and length of the register set for the device
>> - - compatible : should be "fsl,CHIP-i2c" where CHIP is the name of a
>> - compatible processor, e.g. mpc8313, mpc8543, mpc8544, mpc5121,
>> - mpc5200 or mpc5200b. For the mpc5121, an additional node
>> - "fsl,mpc5121-i2c-ctrl" is required as shown in the example below.
>> - - interrupts : where a is the interrupt number and b is a
>> - field that represents an encoding of the sense and level
>> - information for the interrupt. This should be encoded based on
>> - the information in section 2) depending on the type of interrupt
>> - controller you have.
>> -
>> -Recommended properties :
>> -
>> - - fsl,preserve-clocking : boolean; if defined, the clock settings
>> - from the bootloader are preserved (not touched).
>> - - clock-frequency : desired I2C bus clock frequency in Hz.
>> - - fsl,timeout : I2C bus timeout in microseconds.
>> -
>> -Examples :
>> -
>> -/* MPC5121 based board */
>> -i2c@1740 {
>> -#address-cells = <1>;
>> -#size-cells = <0>;
>> -compatible = "fsl,mpc5121-i2c", "fsl-i2c";
>> -reg = <0x1740 0x20>;
>> -interrupts = <11 0x8>;
>> -interrupt-parent = <>;
>> -clock-frequency = <10>;
>> -};
>> -
>> -i2ccontrol@1760 {
>> -compatible = "fsl,mpc5121-i2c-ctrl";
>> -reg = <0x1760 0x8>;
>> -};
>> -
>> -/* MPC5200B based board */
>> -i2c@3d00 {
>> -#address-cells = <1>;
>> -#size-cells = <0>;
>> -compatible = "fsl,mpc5200b-i2c","fsl,mpc5200-i2c","fsl-i2c";
>> -reg = <0x3d00 0x40>;
>> -interrupts = <2 15 0>;
>> -interrupt-parent = <_pic>;
>> -fsl,preserve-clocking;
>> -};
>> -
>> -/* MPC8544 base board */
>> -i2c@3100 {
>> -#address-cells = <1>;
>> -#size-cells = <0>;
>> -compatible = "fsl,mpc8544-i2c", "fsl-i2c";
>> -reg = <0x3100 0x100>;
>> -interrupts = <43 2>;
>> -interrupt-parent = <>;
>> -clock-frequency = <40>;
>> -fsl,timeout = <1>;
>> -};
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
>> b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
>> new file mode 100644
>> index ..97cea8a817ea
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
>> @@ -0,0 +1,99 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/i2c/i2c-mpc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: I2C-Bus adapter for MPC824x/83xx/85xx/86xx/512x/52xx SoCs
>> +
>> +maintainers:
>> + - Chris Packham
>> +
>> +allOf:
>> + - $ref: /schemas/i2c/i2c-controller.yaml#
>> +
>> +properties:
>> + compatible:
>> +anyOf:
>> + - items:
>> +- enum:
>> + - mpc5200-i2c
>> + - fsl,mpc5200b-i2c
>> + - fsl,mpc5200-i2c
>> + - fsl,mpc5121-i2c
>> + - fsl,mpc8313-i2c
>> + - fsl,mpc8543-i2c
>> + - fsl,mpc8544-i2c
>> +
>> +- const: fsl-i2c
>>
Re: [PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
Hi Rob, On 24/03/21 9:16 am, Rob Herring wrote: > On Tue, 23 Mar 2021 17:33:27 +1300, Chris Packham wrote: >> Convert i2c-mpc to YAML. >> >> Signed-off-by: Chris Packham >> --- >> .../devicetree/bindings/i2c/i2c-mpc.txt | 62 >> .../devicetree/bindings/i2c/i2c-mpc.yaml | 99 +++ >> 2 files changed, 99 insertions(+), 62 deletions(-) >> delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt >> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml >> > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > ./Documentation/devicetree/bindings/i2c/i2c-mpc.yaml:19:9: [warning] wrong > indentation: expected 10 but found 8 (indentation) > ./Documentation/devicetree/bindings/i2c/i2c-mpc.yaml:20:11: [warning] wrong > indentation: expected 12 but found 10 (indentation) Hmm I did run 'make dt_binding_check' is yamllint run separately (or not run if it's not installed?). > dtschema/dtc warnings/errors: > > See https://patchwork.ozlabs.org/patch/1457053 > > This check can fail if there are any dependencies. The base for a patch > series is generally the most recent rc1. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit. Should be easy to fix the binding but I'll spend a bit of time trying to get my tooling sorted.
[PATCH 5/6] i2c: mpc: use device managed APIs
Use device managed functions an clean up error handling.
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 46 ++--
1 file changed, 18 insertions(+), 28 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 5b746a898e8e..46cdb36e2f9b 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -654,7 +654,6 @@ static int fsl_i2c_probe(struct platform_device *op)
u32 clock = MPC_I2C_CLOCK_LEGACY;
int result = 0;
int plen;
- struct resource res;
struct clk *clk;
int err;
@@ -662,7 +661,7 @@ static int fsl_i2c_probe(struct platform_device *op)
if (!match)
return -EINVAL;
- i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+ i2c = devm_kzalloc(>dev, sizeof(*i2c), GFP_KERNEL);
if (!i2c)
return -ENOMEM;
@@ -670,24 +669,21 @@ static int fsl_i2c_probe(struct platform_device *op)
init_waitqueue_head(>queue);
- i2c->base = of_iomap(op->dev.of_node, 0);
- if (!i2c->base) {
+ i2c->base = devm_platform_ioremap_resource(op, 0);
+ if (IS_ERR(i2c->base)) {
dev_err(i2c->dev, "failed to map controller\n");
- result = -ENOMEM;
- goto fail_map;
+ return PTR_ERR(i2c->base);
}
- i2c->irq = irq_of_parse_and_map(op->dev.of_node, 0);
- if (i2c->irq < 0) {
- result = i2c->irq;
- goto fail_map;
- }
+ i2c->irq = platform_get_irq(op, 0);
+ if (i2c->irq < 0)
+ return i2c->irq;
- result = request_irq(i2c->irq, mpc_i2c_isr,
+ result = devm_request_irq(>dev, i2c->irq, mpc_i2c_isr,
IRQF_SHARED, "i2c-mpc", i2c);
if (result < 0) {
dev_err(i2c->dev, "failed to attach interrupt\n");
- goto fail_request;
+ return result;
}
/*
@@ -699,7 +695,7 @@ static int fsl_i2c_probe(struct platform_device *op)
err = clk_prepare_enable(clk);
if (err) {
dev_err(>dev, "failed to enable clock\n");
- goto fail_request;
+ return err;
} else {
i2c->clk_per = clk;
}
@@ -731,32 +727,26 @@ static int fsl_i2c_probe(struct platform_device *op)
}
dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 100 / HZ);
- platform_set_drvdata(op, i2c);
-
i2c->adap = mpc_ops;
- of_address_to_resource(op->dev.of_node, 0, );
scnprintf(i2c->adap.name, sizeof(i2c->adap.name),
- "MPC adapter at 0x%llx", (unsigned long long)res.start);
- i2c_set_adapdata(>adap, i2c);
+ "MPC adapter (%s)", of_node_full_name(op->dev.of_node));
i2c->adap.dev.parent = >dev;
+ i2c->adap.nr = op->id;
i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
i2c->adap.bus_recovery_info = _i2c_recovery_info;
+ platform_set_drvdata(op, i2c);
+ i2c_set_adapdata(>adap, i2c);
- result = i2c_add_adapter(>adap);
- if (result < 0)
+ result = i2c_add_numbered_adapter(>adap);
+ if (result)
goto fail_add;
- return result;
+ return 0;
fail_add:
if (i2c->clk_per)
clk_disable_unprepare(i2c->clk_per);
- free_irq(i2c->irq, i2c);
- fail_request:
- irq_dispose_mapping(i2c->irq);
- iounmap(i2c->base);
- fail_map:
- kfree(i2c);
+
return result;
};
--
2.30.2
[PATCH 6/6] i2c: mpc: Interrupt driven transfer
The fsl-i2c controller will generate an interrupt after every byte
transferred. Make use of this interrupt to drive a state machine which
allows the next part of a transfer to happen as soon as the interrupt is
received. This is particularly helpful with SMBUS devices like the LM81
which will timeout if we take too long between bytes in a transfer.
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 430 +++
1 file changed, 237 insertions(+), 193 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 46cdb36e2f9b..5ffde3428232 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -1,16 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
/*
- * (C) Copyright 2003-2004
- * Humboldt Solutions Ltd, adr...@humboldt.co.uk.
-
* This is a combined i2c adapter and algorithm driver for the
* MPC107/Tsi107 PowerPC northbridge and processors that include
* the same I2C unit (8240, 8245, 85xx).
*
- * Release 0.8
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
+ * Copyright (C) 2003-2004 Humboldt Solutions Ltd, adr...@humboldt.co.uk
+ * Copyright (C) 2021 Allied Telesis Labs
*/
#include
@@ -58,11 +53,32 @@
#define CSR_MIF 0x02
#define CSR_RXAK 0x01
+enum mpc_i2c_action {
+ MPC_I2C_ACTION_INVALID = 0,
+ MPC_I2C_ACTION_START,
+ MPC_I2C_ACTION_RESTART,
+ MPC_I2C_ACTION_READ_BEGIN,
+ MPC_I2C_ACTION_READ_BYTE,
+ MPC_I2C_ACTION_WRITE,
+ MPC_I2C_ACTION_STOP,
+};
+
+static char *action_str[] = {
+ "invalid",
+ "start",
+ "restart",
+ "read begin",
+ "read",
+ "write",
+ "stop",
+};
+
struct mpc_i2c {
struct device *dev;
void __iomem *base;
u32 interrupt;
- wait_queue_head_t queue;
+ wait_queue_head_t waitq;
+ spinlock_t lock;
struct i2c_adapter adap;
int irq;
u32 real_clk;
@@ -70,6 +86,16 @@ struct mpc_i2c {
u8 fdr, dfsrr;
#endif
struct clk *clk_per;
+ u32 cntl_bits;
+ enum mpc_i2c_action action;
+ struct i2c_msg *msgs;
+ int num_msgs;
+ int curr_msg;
+ u32 byte_posn;
+ u32 block;
+ int rc;
+ int expect_rxack;
+
};
struct mpc_i2c_divider {
@@ -86,19 +112,6 @@ static inline void writeccr(struct mpc_i2c *i2c, u32 x)
writeb(x, i2c->base + MPC_I2C_CR);
}
-static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
-{
- struct mpc_i2c *i2c = dev_id;
- if (readb(i2c->base + MPC_I2C_SR) & CSR_MIF) {
- /* Read again to allow register to stabilise */
- i2c->interrupt = readb(i2c->base + MPC_I2C_SR);
- writeb(0, i2c->base + MPC_I2C_SR);
- wake_up(>queue);
- return IRQ_HANDLED;
- }
- return IRQ_NONE;
-}
-
/* Sometimes 9th clock pulse isn't generated, and slave doesn't release
* the bus, because it wants to send ACK.
* Following sequence of enabling/disabling and sending start/stop generates
@@ -121,45 +134,6 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
}
}
-static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
-{
- u32 cmd_err;
- int result;
-
- result = wait_event_timeout(i2c->queue,
- (i2c->interrupt & CSR_MIF), timeout);
-
- if (unlikely(!(i2c->interrupt & CSR_MIF))) {
- dev_dbg(i2c->dev, "wait timeout\n");
- writeccr(i2c, 0);
- result = -ETIMEDOUT;
- }
-
- cmd_err = i2c->interrupt;
- i2c->interrupt = 0;
-
- if (result < 0)
- return result;
-
- if (!(cmd_err & CSR_MCF)) {
- dev_dbg(i2c->dev, "unfinished\n");
- return -EIO;
- }
-
- if (cmd_err & CSR_MAL) {
- dev_dbg(i2c->dev, "MAL\n");
- return -EAGAIN;
- }
-
- if (writing && (cmd_err & CSR_RXAK)) {
- dev_dbg(i2c->dev, "No RXAK\n");
- /* generate stop */
- writeccr(i2c, CCR_MEN);
- return -ENXIO;
- }
- return 0;
-}
-
#if defined(CONFIG_PPC_MPC52xx) || defined(CONFIG_PPC_MPC512x)
static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = {
{20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
@@ -434,168 +408,209 @@ static void mpc_i2c_setup_8xxx(struct device_node *node,
}
#endif /* CONFIG_FSL_SOC */
-static void mpc_i2c_start(struct mpc_i2c *i2c)
+static void mpc_i2c_finish(struct mpc_i2c *i2c, int rc)
{
- /* Clear arbitration */
- writeb(0, i2
[PATCH 3/6] i2c: mpc: Make use of i2c_recover_bus()
Move the existing calls of mpc_i2c_fixup() to a recovery function
registered via bus_recovery_info. This makes it more obvious that
recovery is supported and allows for a future where recovery is
triggered by the i2c core.
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d94f05c8b8b7..6a0d55e9e8e3 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -586,7 +586,7 @@ static int mpc_xfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
writeb(status & ~CSR_MAL,
i2c->base + MPC_I2C_SR);
- mpc_i2c_fixup(i2c);
+ i2c_recover_bus(>adap);
}
return -EIO;
}
@@ -622,7 +622,7 @@ static int mpc_xfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
writeb(status & ~CSR_MAL,
i2c->base + MPC_I2C_SR);
- mpc_i2c_fixup(i2c);
+ i2c_recover_bus(>adap);
}
return -EIO;
}
@@ -637,6 +637,15 @@ static u32 mpc_functionality(struct i2c_adapter *adap)
| I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL;
}
+static int fsl_i2c_bus_recovery(struct i2c_adapter *adap)
+{
+ struct mpc_i2c *i2c = i2c_get_adapdata(adap);
+
+ mpc_i2c_fixup(i2c);
+
+ return 0;
+}
+
static const struct i2c_algorithm mpc_algo = {
.master_xfer = mpc_xfer,
.functionality = mpc_functionality,
@@ -648,6 +657,10 @@ static struct i2c_adapter mpc_ops = {
.timeout = HZ,
};
+static struct i2c_bus_recovery_info fsl_i2c_recovery_info = {
+ .recover_bus = fsl_i2c_bus_recovery,
+};
+
static const struct of_device_id mpc_i2c_of_match[];
static int fsl_i2c_probe(struct platform_device *op)
{
@@ -740,6 +753,7 @@ static int fsl_i2c_probe(struct platform_device *op)
i2c_set_adapdata(>adap, i2c);
i2c->adap.dev.parent = >dev;
i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
+ i2c->adap.bus_recovery_info = _i2c_recovery_info;
result = i2c_add_adapter(>adap);
if (result < 0)
--
2.30.2
[PATCH 0/6] i2c: mpc: Refactor to improve responsiveness
The "meat" of this series is in the last patch which is the change that actually starts making use of the interrupts to drive a state machine. The dt-bindings patches can probably go in at any time. The rest of the series isn't dependent on them. I've tested it on a T2081 based system with a number of i2c and smbus devices. Its the end of my work day so I figured I'd get this out now but I'll do some more testing on a P2041 board and a few different i2c devices tomorrow. Chris Packham (6): dt-bindings: i2c-mpc: Document interrupt property as required dt-bindings: i2c: convert i2c-mpc to json-schema i2c: mpc: Make use of i2c_recover_bus() i2c: mpc: make interrupt mandatory and remove polling code i2c: mpc: use device managed APIs i2c: mpc: Interrupt driven transfer .../devicetree/bindings/i2c/i2c-mpc.txt | 62 --- .../devicetree/bindings/i2c/i2c-mpc.yaml | 99 drivers/i2c/busses/i2c-mpc.c | 513 ++ 3 files changed, 373 insertions(+), 301 deletions(-) delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml -- 2.30.2
[PATCH 2/6] dt-bindings: i2c: convert i2c-mpc to json-schema
Convert i2c-mpc to YAML.
Signed-off-by: Chris Packham
---
.../devicetree/bindings/i2c/i2c-mpc.txt | 62
.../devicetree/bindings/i2c/i2c-mpc.yaml | 99 +++
2 files changed, 99 insertions(+), 62 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.txt
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
deleted file mode 100644
index b15acb43d84d..
--- a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt
+++ /dev/null
@@ -1,62 +0,0 @@
-* I2C
-
-Required properties :
-
- - reg : Offset and length of the register set for the device
- - compatible : should be "fsl,CHIP-i2c" where CHIP is the name of a
- compatible processor, e.g. mpc8313, mpc8543, mpc8544, mpc5121,
- mpc5200 or mpc5200b. For the mpc5121, an additional node
- "fsl,mpc5121-i2c-ctrl" is required as shown in the example below.
- - interrupts : where a is the interrupt number and b is a
- field that represents an encoding of the sense and level
- information for the interrupt. This should be encoded based on
- the information in section 2) depending on the type of interrupt
- controller you have.
-
-Recommended properties :
-
- - fsl,preserve-clocking : boolean; if defined, the clock settings
- from the bootloader are preserved (not touched).
- - clock-frequency : desired I2C bus clock frequency in Hz.
- - fsl,timeout : I2C bus timeout in microseconds.
-
-Examples :
-
- /* MPC5121 based board */
- i2c@1740 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "fsl,mpc5121-i2c", "fsl-i2c";
- reg = <0x1740 0x20>;
- interrupts = <11 0x8>;
- interrupt-parent = <>;
- clock-frequency = <10>;
- };
-
- i2ccontrol@1760 {
- compatible = "fsl,mpc5121-i2c-ctrl";
- reg = <0x1760 0x8>;
- };
-
- /* MPC5200B based board */
- i2c@3d00 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "fsl,mpc5200b-i2c","fsl,mpc5200-i2c","fsl-i2c";
- reg = <0x3d00 0x40>;
- interrupts = <2 15 0>;
- interrupt-parent = <_pic>;
- fsl,preserve-clocking;
- };
-
- /* MPC8544 base board */
- i2c@3100 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "fsl,mpc8544-i2c", "fsl-i2c";
- reg = <0x3100 0x100>;
- interrupts = <43 2>;
- interrupt-parent = <>;
- clock-frequency = <40>;
- fsl,timeout = <1>;
- };
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
new file mode 100644
index ..97cea8a817ea
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/i2c-mpc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C-Bus adapter for MPC824x/83xx/85xx/86xx/512x/52xx SoCs
+
+maintainers:
+ - Chris Packham
+
+allOf:
+ - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+ compatible:
+anyOf:
+ - items:
+- enum:
+ - mpc5200-i2c
+ - fsl,mpc5200b-i2c
+ - fsl,mpc5200-i2c
+ - fsl,mpc5121-i2c
+ - fsl,mpc8313-i2c
+ - fsl,mpc8543-i2c
+ - fsl,mpc8544-i2c
+
+- const: fsl-i2c
+
+ - contains:
+ const: fsl-i2c
+minItems: 1
+maxItems: 4
+
+ reg:
+maxItems: 1
+
+ interrupts:
+maxItems: 1
+
+ fsl,preserve-clocking:
+$ref: /schemas/types.yaml#/definitions/flag
+description: |
+ if defined, the clock settings from the bootloader are
+ preserved (not touched)
+
+ fsl,timeout:
+$ref: /schemas/types.yaml#/definitions/uint32
+description: |
+ I2C bus timeout in microseconds
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+unevaluatedProperties: false
+
+examples:
+ - |
+/* MPC5121 based board */
+i2c@1740 {
+#address-cells = <1>;
+#size-cells = <0>;
+compatible = "fsl,mpc5121-i2c", "fsl-i2c";
+reg = <0x1740 0x20>;
+interrupts = <11 0x8>;
+interrupt-parent = <>;
+clock-frequency = <10>;
+};
+
+i2ccontrol@1760 {
+compatible = "fsl,mpc5121-i2c-ctrl";
+reg = <0x1760 0x8>
[PATCH 1/6] dt-bindings: i2c-mpc: Document interrupt property as required
All of the in-tree device-trees that use the one of the compatible strings from i2c-mpc.c supply an interrupts property. Make this property mandatory to aid refactoring the driver. Signed-off-by: Chris Packham --- Documentation/devicetree/bindings/i2c/i2c-mpc.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt index 42a390526957..b15acb43d84d 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mpc.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.txt @@ -7,14 +7,14 @@ Required properties : compatible processor, e.g. mpc8313, mpc8543, mpc8544, mpc5121, mpc5200 or mpc5200b. For the mpc5121, an additional node "fsl,mpc5121-i2c-ctrl" is required as shown in the example below. - -Recommended properties : - - interrupts : where a is the interrupt number and b is a field that represents an encoding of the sense and level information for the interrupt. This should be encoded based on the information in section 2) depending on the type of interrupt controller you have. + +Recommended properties : + - fsl,preserve-clocking : boolean; if defined, the clock settings from the bootloader are preserved (not touched). - clock-frequency : desired I2C bus clock frequency in Hz. -- 2.30.2
[PATCH 4/6] i2c: mpc: make interrupt mandatory and remove polling code
All the in-tree dts files that use one of the compatible strings from
i2c-mpc.c provide an interrupt property. By making this mandatory we
can simplify the code.
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 51 ++--
1 file changed, 19 insertions(+), 32 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 6a0d55e9e8e3..5b746a898e8e 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -123,37 +123,21 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
{
- unsigned long orig_jiffies = jiffies;
u32 cmd_err;
- int result = 0;
+ int result;
- if (!i2c->irq) {
- while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
- schedule();
- if (time_after(jiffies, orig_jiffies + timeout)) {
- dev_dbg(i2c->dev, "timeout\n");
- writeccr(i2c, 0);
- result = -ETIMEDOUT;
- break;
- }
- }
- cmd_err = readb(i2c->base + MPC_I2C_SR);
- writeb(0, i2c->base + MPC_I2C_SR);
- } else {
- /* Interrupt mode */
- result = wait_event_timeout(i2c->queue,
+ result = wait_event_timeout(i2c->queue,
(i2c->interrupt & CSR_MIF), timeout);
- if (unlikely(!(i2c->interrupt & CSR_MIF))) {
- dev_dbg(i2c->dev, "wait timeout\n");
- writeccr(i2c, 0);
- result = -ETIMEDOUT;
- }
-
- cmd_err = i2c->interrupt;
- i2c->interrupt = 0;
+ if (unlikely(!(i2c->interrupt & CSR_MIF))) {
+ dev_dbg(i2c->dev, "wait timeout\n");
+ writeccr(i2c, 0);
+ result = -ETIMEDOUT;
}
+ cmd_err = i2c->interrupt;
+ i2c->interrupt = 0;
+
if (result < 0)
return result;
@@ -694,13 +678,16 @@ static int fsl_i2c_probe(struct platform_device *op)
}
i2c->irq = irq_of_parse_and_map(op->dev.of_node, 0);
- if (i2c->irq) { /* no i2c->irq implies polling */
- result = request_irq(i2c->irq, mpc_i2c_isr,
-IRQF_SHARED, "i2c-mpc", i2c);
- if (result < 0) {
- dev_err(i2c->dev, "failed to attach interrupt\n");
- goto fail_request;
- }
+ if (i2c->irq < 0) {
+ result = i2c->irq;
+ goto fail_map;
+ }
+
+ result = request_irq(i2c->irq, mpc_i2c_isr,
+ IRQF_SHARED, "i2c-mpc", i2c);
+ if (result < 0) {
+ dev_err(i2c->dev, "failed to attach interrupt\n");
+ goto fail_request;
}
/*
--
2.30.2
Re: Errant readings on LM81 with T2080 SoC
On 12/03/21 10:34 am, Guenter Roeck wrote: > On 3/11/21 1:17 PM, Chris Packham wrote: >> On 11/03/21 9:18 pm, Wolfram Sang wrote: >>>> Bummer. What is really weird is that you see clock stretching under >>>> CPU load. Normally clock stretching is triggered by the device, not >>>> by the host. >>> One example: Some hosts need an interrupt per byte to know if they >>> should send ACK or NACK. If that interrupt is delayed, they stretch the >>> clock. >>> >> It feels like something like that is happening. Looking at the T2080 >> Reference manual there is an interesting timing diagram (Figure 14-2 if >> someone feels like looking it up). It shows SCL low between the ACK for >> the address and the data byte. I think if we're delayed in sending the >> next byte we could violate Ttimeout or Tlow:mext from the SMBUS spec. >> > I think that really leaves you only two options that I can see: > Rework the driver to handle critical actions (such as setting TXAK, > and everything else that might result in clock stretching) in the > interrupt handler, or rework the driver to handle everything in > a high priority kernel thread. I've made some reasonable progress on making i2c-mpc more interrupt driven. Assuming it works out for my use-case is there an opinion on making interrupt support mandatory? Looking at all the in-tree dts files that use one of the compatible strings from i2c-mpc.c they all have interrupt properties so in theory nothing is using the polling mode. But there may be some out-of-tree boards or boards using an old dtb that would be affected?
[PATCH v3 3/3] hwmon: (pmbus): Add driver for BluTek BPA-RS600
The BPA-RS600 is a compact 600W AC to DC removable power supply module. Signed-off-by: Chris Packham --- Notes: Changes in v3: - Fix typo BPD -> BPA - Allow for NUL terminator in buf Changes in v2: - Whitespace and line length cleanup - Add comments about commands that return data but shouldn't be used Documentation/hwmon/bpa-rs600.rst | 74 + drivers/hwmon/pmbus/Kconfig | 9 ++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/bpa-rs600.c | 172 ++ 4 files changed, 256 insertions(+) create mode 100644 Documentation/hwmon/bpa-rs600.rst create mode 100644 drivers/hwmon/pmbus/bpa-rs600.c diff --git a/Documentation/hwmon/bpa-rs600.rst b/Documentation/hwmon/bpa-rs600.rst new file mode 100644 index ..28313995d4ae --- /dev/null +++ b/Documentation/hwmon/bpa-rs600.rst @@ -0,0 +1,74 @@ +.. SPDX-License-Identifier: GPL-2.0 + +Kernel driver bpa-rs600 +=== + +Supported chips: + + * BPA-RS600-120 + +Datasheet: Publicly available at the BluTek website + http://blutekpower.com/wp-content/uploads/2019/01/BPA-RS600-120-07-19-2018.pdf + +Authors: + - Chris Packham + +Description +--- + +The BPA-RS600 is a compact 600W removable power supply module. + +Usage Notes +--- + +This driver does not probe for PMBus devices. You will have to instantiate +devices explicitly. + +Sysfs attributes + + +=== +curr1_label "iin" +curr1_inputMeasured input current +curr1_max Maximum input current +curr1_max_alarmInput current high alarm + +curr2_label"iout1" +curr2_inputMeasured output current +curr2_max Maximum output current +curr2_max_alarmOutput current high alarm + +fan1_input Measured fan speed +fan1_alarm Fan warning +fan1_fault Fan fault + +in1_label "vin" +in1_input Measured input voltage +in1_maxMaximum input voltage +in1_max_alarm Input voltage high alarm +in1_minMinimum input voltage +in1_min_alarm Input voltage low alarm + +in2_label "vout1" +in2_input Measured output voltage +in2_maxMaximum output voltage +in2_max_alarm Output voltage high alarm +in2_minMaximum output voltage +in2_min_alarm Output voltage low alarm + +power1_label "pin" +power1_input Measured input power +power1_alarm Input power alarm +power1_max Maximum input power + +power2_label "pout1" +power2_input Measured output power +power2_max Maximum output power +power2_max_alarm Output power high alarm + +temp1_inputMeasured temperature around input connector +temp1_alarmTemperature alarm + +temp2_inputMeasured temperature around output connector +temp2_alarmTemperature alarm +=== diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index 32d2fc850621..512d6f656dca 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -56,6 +56,15 @@ config SENSORS_BEL_PFE This driver can also be built as a module. If so, the module will be called bel-pfe. +config SENSORS_BPA_RS600 + tristate "BluTek BPA-RS600 Power Supplies" + help + If you say yes here you get hardware monitoring support for BluTek + BPA-RS600 Power Supplies. + + This driver can also be built as a module. If so, the module will + be called bpa-rs600. + config SENSORS_IBM_CFFPS tristate "IBM Common Form Factor Power Supply" depends on LEDS_CLASS diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile index 6a4ba0fdc1db..80a437060dc4 100644 --- a/drivers/hwmon/pmbus/Makefile +++ b/drivers/hwmon/pmbus/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o +obj-$(CONFIG_SENSORS_BPA_RS600)+= bpa-rs600.o obj-$(CONFIG_SENSORS_IBM_CFFPS)+= ibm-cffps.o obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o obj-$(CONFIG_SENSORS_IR35221) += ir35221.o diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c new file mode 100644 index ..c4ede68b3e26 --- /dev/null +++ b/drivers/hwmon/pmbus/bpa-rs600.c @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Hardware monitoring driver for BluTek BPA-RS600 Power Supplies + * + * Copyright 20
[PATCH v3 1/3] dt-bindings: Add vendor prefix and trivial device for BluTek BPA-RS600
Add vendor prefix "blutek" for BluTek Power. Add trivial device entry for BPA-RS600. Signed-off-by: Chris Packham --- Notes: Changes in v3: - None Changes in v2: - None Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index a327130d1faa..569236e9bed0 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -50,6 +50,8 @@ properties: - atmel,atsha204a # i2c h/w elliptic curve crypto module - atmel,atecc508a +# BPA-RS600: Power Supply + - blutek,bpa-rs600 # Bosch Sensortec pressure, temperature, humididty and VOC sensor - bosch,bme680 # CM32181: Ambient Light Sensor diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index f6064d84a424..d9d7226f5dfe 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -169,6 +169,8 @@ patternProperties: description: Beckhoff Automation GmbH & Co. KG "^bitmain,.*": description: Bitmain Technologies + "^blutek,.*": +description: BluTek Power "^boe,.*": description: BOE Technology Group Co., Ltd. "^bosch,.*": -- 2.30.2
[PATCH v3 2/3] hwmon: (pmbus): Replace - with _ in device names before registration
The hwmon sysfs ABI requires that the `name` property doesn't include
any dashes. But when the pmbus code picks the name up from the device
tree it quite often does. Replace '-' with '_' before registering the
device.
Signed-off-by: Chris Packham
---
Notes:
Changes in v3:
- None
Changes in v2:
- New
drivers/hwmon/pmbus/pmbus_core.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index aadea85fe630..7d2f8f032314 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2557,6 +2557,7 @@ int pmbus_do_probe(struct i2c_client *client, struct
pmbus_driver_info *info)
struct pmbus_data *data;
size_t groups_num = 0;
int ret;
+ char *name;
if (!info)
return -ENODEV;
@@ -2606,10 +2607,15 @@ int pmbus_do_probe(struct i2c_client *client, struct
pmbus_driver_info *info)
return -ENODEV;
}
+ name = devm_kstrdup(dev, client->name, GFP_KERNEL);
+ if (!name)
+ return -ENOMEM;
+ strreplace(name, '-', '_');
+
data->groups[0] = >group;
memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num);
data->hwmon_dev = devm_hwmon_device_register_with_groups(dev,
- client->name, data, data->groups);
+ name, data, data->groups);
if (IS_ERR(data->hwmon_dev)) {
dev_err(dev, "Failed to register hwmon device\n");
return PTR_ERR(data->hwmon_dev);
--
2.30.2
Re: [PATCH v2 3/3] hwmon: (pmbus): Add driver for BluTek BPA-RS600
On 17/03/21 9:30 am, Chris Packham wrote: > The BPA-RS600 is a compact 600W AC to DC removable power supply module. > > Signed-off-by: Chris Packham > --- > > Notes: > Changes in v2: > - Whitespace and line length cleanup > - Add comments about commands that return data but shouldn't be used > > Documentation/hwmon/bpa-rs600.rst | 74 + > drivers/hwmon/pmbus/Kconfig | 9 ++ > drivers/hwmon/pmbus/Makefile | 1 + > drivers/hwmon/pmbus/bpa-rs600.c | 172 ++ > 4 files changed, 256 insertions(+) > create mode 100644 Documentation/hwmon/bpa-rs600.rst > create mode 100644 drivers/hwmon/pmbus/bpa-rs600.c > > > +config SENSORS_BPA_RS600 > + tristate "BluTek BPD-RS600 Power Supplies" > + help > + If you say yes here you get hardware monitoring support for BluTek > + BPD-RS600 Power Supplies. > + > + This driver can also be built as a module. If so, the module will > + be called bpd-rs600. > + I've used BPD here but it should be BPA (A == AC, D == DC). I'll get that ready for a v3.
[PATCH v2 3/3] hwmon: (pmbus): Add driver for BluTek BPA-RS600
The BPA-RS600 is a compact 600W AC to DC removable power supply module. Signed-off-by: Chris Packham --- Notes: Changes in v2: - Whitespace and line length cleanup - Add comments about commands that return data but shouldn't be used Documentation/hwmon/bpa-rs600.rst | 74 + drivers/hwmon/pmbus/Kconfig | 9 ++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/bpa-rs600.c | 172 ++ 4 files changed, 256 insertions(+) create mode 100644 Documentation/hwmon/bpa-rs600.rst create mode 100644 drivers/hwmon/pmbus/bpa-rs600.c diff --git a/Documentation/hwmon/bpa-rs600.rst b/Documentation/hwmon/bpa-rs600.rst new file mode 100644 index ..28313995d4ae --- /dev/null +++ b/Documentation/hwmon/bpa-rs600.rst @@ -0,0 +1,74 @@ +.. SPDX-License-Identifier: GPL-2.0 + +Kernel driver bpa-rs600 +=== + +Supported chips: + + * BPA-RS600-120 + +Datasheet: Publicly available at the BluTek website + http://blutekpower.com/wp-content/uploads/2019/01/BPA-RS600-120-07-19-2018.pdf + +Authors: + - Chris Packham + +Description +--- + +The BPA-RS600 is a compact 600W removable power supply module. + +Usage Notes +--- + +This driver does not probe for PMBus devices. You will have to instantiate +devices explicitly. + +Sysfs attributes + + +=== +curr1_label "iin" +curr1_inputMeasured input current +curr1_max Maximum input current +curr1_max_alarmInput current high alarm + +curr2_label"iout1" +curr2_inputMeasured output current +curr2_max Maximum output current +curr2_max_alarmOutput current high alarm + +fan1_input Measured fan speed +fan1_alarm Fan warning +fan1_fault Fan fault + +in1_label "vin" +in1_input Measured input voltage +in1_maxMaximum input voltage +in1_max_alarm Input voltage high alarm +in1_minMinimum input voltage +in1_min_alarm Input voltage low alarm + +in2_label "vout1" +in2_input Measured output voltage +in2_maxMaximum output voltage +in2_max_alarm Output voltage high alarm +in2_minMaximum output voltage +in2_min_alarm Output voltage low alarm + +power1_label "pin" +power1_input Measured input power +power1_alarm Input power alarm +power1_max Maximum input power + +power2_label "pout1" +power2_input Measured output power +power2_max Maximum output power +power2_max_alarm Output power high alarm + +temp1_inputMeasured temperature around input connector +temp1_alarmTemperature alarm + +temp2_inputMeasured temperature around output connector +temp2_alarmTemperature alarm +=== diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index 32d2fc850621..c9f08725d201 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -56,6 +56,15 @@ config SENSORS_BEL_PFE This driver can also be built as a module. If so, the module will be called bel-pfe. +config SENSORS_BPA_RS600 + tristate "BluTek BPD-RS600 Power Supplies" + help + If you say yes here you get hardware monitoring support for BluTek + BPD-RS600 Power Supplies. + + This driver can also be built as a module. If so, the module will + be called bpd-rs600. + config SENSORS_IBM_CFFPS tristate "IBM Common Form Factor Power Supply" depends on LEDS_CLASS diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile index 6a4ba0fdc1db..80a437060dc4 100644 --- a/drivers/hwmon/pmbus/Makefile +++ b/drivers/hwmon/pmbus/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o +obj-$(CONFIG_SENSORS_BPA_RS600)+= bpa-rs600.o obj-$(CONFIG_SENSORS_IBM_CFFPS)+= ibm-cffps.o obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o obj-$(CONFIG_SENSORS_IR35221) += ir35221.o diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c new file mode 100644 index ..bdfdef86bf1e --- /dev/null +++ b/drivers/hwmon/pmbus/bpa-rs600.c @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Hardware monitoring driver for BluTek BPA-RS600 Power Supplies + * + * Copyright 2021 Allied Telesis Labs + */ + +#include +#include +#include +#include
[PATCH v2 2/3] hwmon: (pmbus): Replace - with _ in device names before registration
The hwmon sysfs ABI requires that the `name` property doesn't include
any dashes. But when the pmbus code picks the name up from the device
tree it quite often does. Replace '-' with '_' before registering the
device.
Signed-off-by: Chris Packham
---
Notes:
Changes in v2:
- New
drivers/hwmon/pmbus/pmbus_core.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index aadea85fe630..7d2f8f032314 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2557,6 +2557,7 @@ int pmbus_do_probe(struct i2c_client *client, struct
pmbus_driver_info *info)
struct pmbus_data *data;
size_t groups_num = 0;
int ret;
+ char *name;
if (!info)
return -ENODEV;
@@ -2606,10 +2607,15 @@ int pmbus_do_probe(struct i2c_client *client, struct
pmbus_driver_info *info)
return -ENODEV;
}
+ name = devm_kstrdup(dev, client->name, GFP_KERNEL);
+ if (!name)
+ return -ENOMEM;
+ strreplace(name, '-', '_');
+
data->groups[0] = >group;
memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num);
data->hwmon_dev = devm_hwmon_device_register_with_groups(dev,
- client->name, data, data->groups);
+ name, data, data->groups);
if (IS_ERR(data->hwmon_dev)) {
dev_err(dev, "Failed to register hwmon device\n");
return PTR_ERR(data->hwmon_dev);
--
2.30.2
[PATCH v2 1/3] dt-bindings: Add vendor prefix and trivial device for BluTek BPA-RS600
Add vendor prefix "blutek" for BluTek Power. Add trivial device entry for BPA-RS600. Signed-off-by: Chris Packham --- Notes: Changes in v2: - None Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index a327130d1faa..569236e9bed0 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -50,6 +50,8 @@ properties: - atmel,atsha204a # i2c h/w elliptic curve crypto module - atmel,atecc508a +# BPA-RS600: Power Supply + - blutek,bpa-rs600 # Bosch Sensortec pressure, temperature, humididty and VOC sensor - bosch,bme680 # CM32181: Ambient Light Sensor diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index f6064d84a424..d9d7226f5dfe 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -169,6 +169,8 @@ patternProperties: description: Beckhoff Automation GmbH & Co. KG "^bitmain,.*": description: Bitmain Technologies + "^blutek,.*": +description: BluTek Power "^boe,.*": description: BOE Technology Group Co., Ltd. "^bosch,.*": -- 2.30.2
Re: [PATCH 2/2] hwmon: (pmbus): Add driver for BluTek BPA-RS600
On 16/03/21 4:43 pm, Guenter Roeck wrote:
> On 3/15/21 7:35 PM, Chris Packham wrote:
>> The BPA-RS600 is a compact 600W AC to DC removable power supply module.
>>
>> Signed-off-by: Chris Packham
>> ---
>> +
>> +static int bpa_rs600_read_word_data(struct i2c_client *client, int page,
>> +int phase, int reg)+{
>> +int ret;
>> +
>> +if (page > 0)
>> +return -ENXIO;
>> +
>> +switch (reg) {
>> +case PMBUS_VIN_UV_FAULT_LIMIT:
>> +case PMBUS_VIN_OV_FAULT_LIMIT:
>> +case PMBUS_VOUT_UV_FAULT_LIMIT:
>> +case PMBUS_VOUT_OV_FAULT_LIMIT:
>> +ret = -ENXIO;
> Is that needed ? Why not -ENODATA ?
Basically these commands get responses on the bus but they don't have
valid data (nor are they documented in the datasheet). I'll add a
comment to that effect.
If I'm reading things correctly -ENODATA is a signal to
_pmbus_read_word_data use the "normal" read operation. So I need to
return something other than that. I found another driver (mp2975.c)
doing the same thing for what I assume are similar reasons so I went
with -ENXIO.
>
>> +break;
> +
>> +static const struct i2c_device_id bpa_rs600_id[] = {
>> +{ "bpa_rs600", 0 },
> Hmm, no, this has an underscore. Guess you'll have to use the trick from
> iio_hwmon.c or similar to generate a valid name.
>
> Oh, wait, this is a pmbus driver, and the pmbus core uses client->name.
> Maybe we need to add an optional strreplace() to the pmbus core.
Looking into this now.
Re: [PATCH 2/2] hwmon: (pmbus): Add driver for BluTek BPA-RS600
On 16/03/21 4:35 pm, Guenter Roeck wrote:
> On 3/15/21 8:30 PM, Chris Packham wrote:
>> On 16/03/21 3:35 pm, Chris Packham wrote:
>>> The BPA-RS600 is a compact 600W AC to DC removable power supply module.
>>>
>>> Signed-off-by: Chris Packham
>>>
>>> +static const struct of_device_id __maybe_unused bpa_rs600_of_match[] = {
>>> + { .compatible = "blutek,bpa-rs600" },
>>> + {},
>>> +};
>> I see this will fall foul of the name check in
>> __hwmon_device_register(). How should I name things to avoid this?
>>
> It isn't the binding. The driver name should not have a '-' in it.
> You could just name it bpa_rs600 instead.
Sold.
I'll give the world a chance to turn so people can look at the rest of
the patch before I send a v2.
Re: [PATCH 2/2] hwmon: (pmbus): Add driver for BluTek BPA-RS600
On 16/03/21 3:35 pm, Chris Packham wrote:
> The BPA-RS600 is a compact 600W AC to DC removable power supply module.
>
> Signed-off-by: Chris Packham
>
> +static const struct of_device_id __maybe_unused bpa_rs600_of_match[] = {
> + { .compatible = "blutek,bpa-rs600" },
> + {},
> +};
I see this will fall foul of the name check in
__hwmon_device_register(). How should I name things to avoid this?
[PATCH 2/2] hwmon: (pmbus): Add driver for BluTek BPA-RS600
The BPA-RS600 is a compact 600W AC to DC removable power supply module. Signed-off-by: Chris Packham --- Documentation/hwmon/bpa-rs600.rst | 74 drivers/hwmon/pmbus/Kconfig | 10 ++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/bpa-rs600.c | 179 ++ 4 files changed, 264 insertions(+) create mode 100644 Documentation/hwmon/bpa-rs600.rst create mode 100644 drivers/hwmon/pmbus/bpa-rs600.c diff --git a/Documentation/hwmon/bpa-rs600.rst b/Documentation/hwmon/bpa-rs600.rst new file mode 100644 index ..28313995d4ae --- /dev/null +++ b/Documentation/hwmon/bpa-rs600.rst @@ -0,0 +1,74 @@ +.. SPDX-License-Identifier: GPL-2.0 + +Kernel driver bpa-rs600 +=== + +Supported chips: + + * BPA-RS600-120 + +Datasheet: Publicly available at the BluTek website + http://blutekpower.com/wp-content/uploads/2019/01/BPA-RS600-120-07-19-2018.pdf + +Authors: + - Chris Packham + +Description +--- + +The BPA-RS600 is a compact 600W removable power supply module. + +Usage Notes +--- + +This driver does not probe for PMBus devices. You will have to instantiate +devices explicitly. + +Sysfs attributes + + +=== +curr1_label "iin" +curr1_inputMeasured input current +curr1_max Maximum input current +curr1_max_alarmInput current high alarm + +curr2_label"iout1" +curr2_inputMeasured output current +curr2_max Maximum output current +curr2_max_alarmOutput current high alarm + +fan1_input Measured fan speed +fan1_alarm Fan warning +fan1_fault Fan fault + +in1_label "vin" +in1_input Measured input voltage +in1_maxMaximum input voltage +in1_max_alarm Input voltage high alarm +in1_minMinimum input voltage +in1_min_alarm Input voltage low alarm + +in2_label "vout1" +in2_input Measured output voltage +in2_maxMaximum output voltage +in2_max_alarm Output voltage high alarm +in2_minMaximum output voltage +in2_min_alarm Output voltage low alarm + +power1_label "pin" +power1_input Measured input power +power1_alarm Input power alarm +power1_max Maximum input power + +power2_label "pout1" +power2_input Measured output power +power2_max Maximum output power +power2_max_alarm Output power high alarm + +temp1_inputMeasured temperature around input connector +temp1_alarmTemperature alarm + +temp2_inputMeasured temperature around output connector +temp2_alarmTemperature alarm +=== diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index 32d2fc850621..d1f0f1cd8247 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -56,6 +56,16 @@ config SENSORS_BEL_PFE This driver can also be built as a module. If so, the module will be called bel-pfe. +config SENSORS_BPA_RS600 + tristate "BluTek BPD-RS600 Power Supplies" + help + If you say yes here you get hardware monitoring support for BluTek + BPD-RS600 Power Supplies. + + This driver can also be built as a module. If so, the module will + be called bpd-rs600. + + config SENSORS_IBM_CFFPS tristate "IBM Common Form Factor Power Supply" depends on LEDS_CLASS diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile index 6a4ba0fdc1db..80a437060dc4 100644 --- a/drivers/hwmon/pmbus/Makefile +++ b/drivers/hwmon/pmbus/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o +obj-$(CONFIG_SENSORS_BPA_RS600)+= bpa-rs600.o obj-$(CONFIG_SENSORS_IBM_CFFPS)+= ibm-cffps.o obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o obj-$(CONFIG_SENSORS_IR35221) += ir35221.o diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c new file mode 100644 index ..94cbf42816f0 --- /dev/null +++ b/drivers/hwmon/pmbus/bpa-rs600.c @@ -0,0 +1,179 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Hardware monitoring driver for BluTek BPA-RS600 Power Supplies + * + * Copyright 2021 Allied Telesis Labs + */ + +#include +#include +#include +#include +#include +#include "pmbus.h" + +#define BPARS600_MFR_VIN_MIN 0xa0 +#define BPARS600_MFR_VIN_MAX 0xa1 +#define BPARS600_MFR_IIN_MAX
[PATCH 1/2] dt-bindings: Add vendor prefix and trivial device for BluTek BPA-RS600
Add vendor prefix "blutek" for BluTek Power. Add trivial device entry for BPA-RS600. Signed-off-by: Chris Packham --- Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index a327130d1faa..569236e9bed0 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -50,6 +50,8 @@ properties: - atmel,atsha204a # i2c h/w elliptic curve crypto module - atmel,atecc508a +# BPA-RS600: Power Supply + - blutek,bpa-rs600 # Bosch Sensortec pressure, temperature, humididty and VOC sensor - bosch,bme680 # CM32181: Ambient Light Sensor diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index f6064d84a424..d9d7226f5dfe 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -169,6 +169,8 @@ patternProperties: description: Beckhoff Automation GmbH & Co. KG "^bitmain,.*": description: Bitmain Technologies + "^blutek,.*": +description: BluTek Power "^boe,.*": description: BOE Technology Group Co., Ltd. "^bosch,.*": -- 2.30.2
Re: Errant readings on LM81 with T2080 SoC
On 12/03/21 10:25 pm, David Laight wrote: > From: Linuxppc-dev Guenter Roeck >> Sent: 11 March 2021 21:35 >> >> On 3/11/21 1:17 PM, Chris Packham wrote: >>> On 11/03/21 9:18 pm, Wolfram Sang wrote: >>>>> Bummer. What is really weird is that you see clock stretching under >>>>> CPU load. Normally clock stretching is triggered by the device, not >>>>> by the host. >>>> One example: Some hosts need an interrupt per byte to know if they >>>> should send ACK or NACK. If that interrupt is delayed, they stretch the >>>> clock. >>>> >>> It feels like something like that is happening. Looking at the T2080 >>> Reference manual there is an interesting timing diagram (Figure 14-2 if >>> someone feels like looking it up). It shows SCL low between the ACK for >>> the address and the data byte. I think if we're delayed in sending the >>> next byte we could violate Ttimeout or Tlow:mext from the SMBUS spec. >>> >> I think that really leaves you only two options that I can see: >> Rework the driver to handle critical actions (such as setting TXAK, >> and everything else that might result in clock stretching) in the >> interrupt handler, or rework the driver to handle everything in >> a high priority kernel thread. > I'm not sure a high priority kernel thread will help. > Without CONFIG_PREEMPT (which has its own set of nasties) > a RT process won't be scheduled until the processor it last > ran on does a reschedule. > I don't think a kernel thread will be any different from a > user process running under the RT scheduler. > > I'm trying to remember the smbus spec (without remembering the I2C one). For those following along the spec is available here[0]. I know there's a 3.0 version[1] as well but the devices I'm dealing with are from a 2.0 vintage. > While basically a clock+data bit-bang the slave is allowed to drive > the clock low to extend a cycle. > It may be allowed to do this at any point? From what I can see it's actually the master extending the clock. Or more accurately holding it low between the address and data bytes (which from the T2080 reference manual looks expected). I think this may cause a strictly compliant SMBUS device to determine that Tlow:mext has been violated. > The master can generate the data at almost any rate (below the maximum) > but I don't think it can go down to zero. > But I do remember one of the specs having a timeout. > > But I'd have thought the slave should answer the cycle correctly > regardless of any 'random' delays the master adds in. Probably depends on the device implementation. I've got multiple other I2C/SMBUS devices and the LM81 seems to be the one that objects. > Unless you are getting away with de-asserting chipselect? > > The only implementation I've done is one an FPGA so doesn't have > worry about interrupt latencies. > It doesn't actually support clock stretching; it wasn't in the > code I started from and none of the slaves we need to connect to > ever does it. > > David [0] - http://www.smbus.org/specs/smbus20.pdf [1] - https://pmbus.org/Assets/PDFS/Public/SMBus_3_0_20141220.pdf > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales) >
[PATCH] hwmon: (pmbus): Fix Documentation kernel-doc warning
Fix Documentation/hwmon/ kernel-doc warning:
Documentation/hwmon/ir36021.rst:34: WARNING: Malformed table.
No bottom table border found.
Fixes: 0be9fee30ff9 ("hwmon: (pmbus) Add driver for Infineon IR36021")
Signed-off-by: Chris Packham
---
Documentation/hwmon/ir36021.rst | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/hwmon/ir36021.rst b/Documentation/hwmon/ir36021.rst
index 36ef8d518b81..ca3436b04e20 100644
--- a/Documentation/hwmon/ir36021.rst
+++ b/Documentation/hwmon/ir36021.rst
@@ -60,3 +60,4 @@ temp1_alarm Temperature alarm
temp2_input Measured other loop temperature
temp2_alarm Temperature alarm
+=== ===
--
2.30.2
Re: linux-next: build warning after merge of the hwmon-staging tree
On 12/03/21 4:50 pm, Stephen Rothwell wrote:
> Hi all,
>
> After merging the hwmon-staging tree, today's linux-next build (htmldocs)
> produced this warning:
>
> Documentation/hwmon/ir36021.rst:34: WARNING: Malformed table.
> No bottom table border found.
>
> === ===
> curr1_label "iin"
> curr1_input Measured input current
> curr1_alarm Input fault alarm
>
> curr2_label "iout1"
> curr2_input Measured output current
> curr2_alarm Output over-current alarm
>
> in1_label "vin"
> in1_input Measured input voltage
> in1_alarm Input under-voltage alarm
>
> in2_label "vout1"
> in2_input Measured output voltage
> in2_alarm Output over-voltage alarm
>
> power1_label"pin"
> power1_inputMeasured input power
> power1_alarmInput under-voltage alarm
>
> power2_label"pout1"
> power2_inputMeasured output power
>
> temp1_input Measured temperature
> temp1_alarm Temperature alarm
>
> temp2_input Measured other loop temperature
> temp2_alarm Temperature alarm
>
> Caused by commit
>
>0be9fee30ff9 ("hwmon: (pmbus) Add driver for Infineon IR36021")
>
Easy enough to fix. Who should I send the patch to?
Re: Errant readings on LM81 with T2080 SoC
On 12/03/21 1:07 pm, Guenter Roeck wrote: > On 3/11/21 3:47 PM, Chris Packham wrote: >> On 12/03/21 10:34 am, Guenter Roeck wrote: >>> On 3/11/21 1:17 PM, Chris Packham wrote: >>>> On 11/03/21 9:18 pm, Wolfram Sang wrote: >>>>>> Bummer. What is really weird is that you see clock stretching under >>>>>> CPU load. Normally clock stretching is triggered by the device, not >>>>>> by the host. >>>>> One example: Some hosts need an interrupt per byte to know if they >>>>> should send ACK or NACK. If that interrupt is delayed, they stretch the >>>>> clock. >>>>> >>>> It feels like something like that is happening. Looking at the T2080 >>>> Reference manual there is an interesting timing diagram (Figure 14-2 if >>>> someone feels like looking it up). It shows SCL low between the ACK for >>>> the address and the data byte. I think if we're delayed in sending the >>>> next byte we could violate Ttimeout or Tlow:mext from the SMBUS spec. >>>> >>> I think that really leaves you only two options that I can see: >>> Rework the driver to handle critical actions (such as setting TXAK, >>> and everything else that might result in clock stretching) in the >>> interrupt handler, or rework the driver to handle everything in >>> a high priority kernel thread. >> One thing I've found that does seem to avoid the problem is to disable >> preemption, use polling and replace the schedule() in i2c_wait() with >> udelay(50). That's kind of like the kernel thread option. > It is kind of hackish, though, especially since it makes the "loaded system" > situation even worse by adding even more active wait loops. No -ish about it :). But it might put out one fire for me while I'm looking at doing some kind of interrupt driven state machine.
Re: Errant readings on LM81 with T2080 SoC
On 12/03/21 10:34 am, Guenter Roeck wrote: > On 3/11/21 1:17 PM, Chris Packham wrote: >> On 11/03/21 9:18 pm, Wolfram Sang wrote: >>>> Bummer. What is really weird is that you see clock stretching under >>>> CPU load. Normally clock stretching is triggered by the device, not >>>> by the host. >>> One example: Some hosts need an interrupt per byte to know if they >>> should send ACK or NACK. If that interrupt is delayed, they stretch the >>> clock. >>> >> It feels like something like that is happening. Looking at the T2080 >> Reference manual there is an interesting timing diagram (Figure 14-2 if >> someone feels like looking it up). It shows SCL low between the ACK for >> the address and the data byte. I think if we're delayed in sending the >> next byte we could violate Ttimeout or Tlow:mext from the SMBUS spec. >> > I think that really leaves you only two options that I can see: > Rework the driver to handle critical actions (such as setting TXAK, > and everything else that might result in clock stretching) in the > interrupt handler, or rework the driver to handle everything in > a high priority kernel thread. One thing I've found that does seem to avoid the problem is to disable preemption, use polling and replace the schedule() in i2c_wait() with udelay(50). That's kind of like the kernel thread option. > Guenter
Re: Errant readings on LM81 with T2080 SoC
On 11/03/21 9:18 pm, Wolfram Sang wrote: >> Bummer. What is really weird is that you see clock stretching under >> CPU load. Normally clock stretching is triggered by the device, not >> by the host. > One example: Some hosts need an interrupt per byte to know if they > should send ACK or NACK. If that interrupt is delayed, they stretch the > clock. > It feels like something like that is happening. Looking at the T2080 Reference manual there is an interesting timing diagram (Figure 14-2 if someone feels like looking it up). It shows SCL low between the ACK for the address and the data byte. I think if we're delayed in sending the next byte we could violate Ttimeout or Tlow:mext from the SMBUS spec.
Re: Errant readings on LM81 with T2080 SoC
On 10/03/21 6:06 pm, Guenter Roeck wrote: > On 3/9/21 6:19 PM, Chris Packham wrote: >> On 9/03/21 9:27 am, Chris Packham wrote: >>> On 8/03/21 5:59 pm, Guenter Roeck wrote: >>>> Other than that, the only other real idea I have would be to monitor >>>> the i2c bus. >>> I am in the fortunate position of being able to go into the office and >>> even happen to have the expensive scope at the moment. Now I just need >>> to find a tame HW engineer so I don't burn myself trying to attach the >>> probes. >> One thing I see on the scope is that when there is a CPU load there >> appears to be some clock stretching going on (SCL is held low some >> times). I don't see it without the CPU load. It's hard to correlate a >> clock stretching event with a bad read or error but it is one area where >> the SMBUS spec has a maximum that might cause the device to give up waiting. >> > Do you have CONFIG_PREEMPT enabled in your kernel ? But even without > that it is possible that the hot loops at the beginning and end of > each operation mess up the driver and cause it to sleep longer > than intended. Did you try usleep_range() ? I've been running with and without CONFIG_PREEMPT. The failures happen with both. I did try usleep_range() and still saw failures. > On a side note, can you send me a register dump for the lm81 ? > It would be useful for my module test code. Here you go this is from a largely unconfigured LM81 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef 00: 47 47 47 47 47 47 47 47 47 47 47 47 47 47 47 47 10: 47 81 24 03 94 00 00 00 00 ff ff ff ff ff ff ff G?$??... 20: bf cb c1 00 c0 47 ec 24 ff ff 65 ff 00 ff 00 ff ???.?G?$..e. 30: 00 ff 00 ff 00 ff 00 71 a9 7f 7f ff ff 58 01 04 ...q???..X?? 40: 01 08 00 00 00 00 00 50 2f 80 80 01 44 00 00 00 ??.P/???D... 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 90: 00 81 24 03 94 00 00 00 00 ff ff ff ff ff ff ff .?$??... a0: bf cb c1 00 c0 47 ec 24 ff ff 65 ff 00 ff 00 ff ???.?G?$..e. b0: 00 ff 00 ff 00 ff 00 71 a9 7f 7f ff ff 58 01 04 ...q???..X?? c0: 01 00 00 00 00 00 00 50 2f 80 80 01 44 00 00 00 ?..P/???D... d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 This is from a LM81 that's been configured by our application SW with limits appropriate for the platform. 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef 00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 10: ff 81 24 03 94 00 00 00 00 ff ff ff ff ff ff ff ..$. 20: bf cc c1 00 c0 47 ec 1c ff ff 65 dc b4 ff c0 d3 .Ge. 30: ad ff 00 d3 ad 4e 40 71 a9 4b 46 ff ff 58 01 04 .n...@q.kf..X.. 40: 01 08 00 00 00 00 00 f0 2f 80 80 81 44 80 80 80 /...D... 50: 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 60: 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 70: 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80: 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 90: 80 81 24 03 94 00 00 00 00 ff ff ff ff ff ff ff ..$. a0: bf cc c1 00 c0 47 ec 1c ff ff 65 dc b4 ff c0 d3 .Ge. b0: ad ff 00 d3 ad 4e 40 71 a9 4b 46 ff ff 58 01 04 .n...@q.kf..X.. c0: 01 00 00 00 00 00 00 f0 2f 80 80 81 44 80 80 80 /...D... d0: 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 e0: 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 f0: 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80
[PATCH] hwmon: (adm9240): Don't re-read config/limits
The hwmon chip is configured either via sysfs or by an earlier boot
stage (e.g. bootloader/bios). In the sysfs case we already store the
configuration values before it's written to the device. Reading in the
configuration only needs to be done once at probe time to cover the
second case.
Signed-off-by: Chris Packham
---
This doesn't resolve my ongoing i2c issues[0] but avoiding unnecessary
i2c reads will help a bit (it'll certainly avoid errors where the
threshold spontaneously changes).
[0] -
https://lore.kernel.org/lkml/8e0a88ba-01e9-9bc1-c78b-20f26ce27...@alliedtelesis.co.nz/
drivers/hwmon/adm9240.c | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
index cc3e0184e720..7e1258b20b35 100644
--- a/drivers/hwmon/adm9240.c
+++ b/drivers/hwmon/adm9240.c
@@ -128,7 +128,6 @@ struct adm9240_data {
struct mutex update_lock;
char valid;
unsigned long last_updated_measure;
- unsigned long last_updated_config;
u8 in[6]; /* ro in0_input */
u8 in_max[6]; /* rw in0_max */
@@ -282,21 +281,11 @@ static struct adm9240_data *adm9240_update_device(struct
device *dev)
return ERR_PTR(err);
}
data->last_updated_measure = jiffies;
- }
-
- /* minimum config reading cycle: 300 seconds */
- if (time_after(jiffies, data->last_updated_config + (HZ * 300))
- || !data->valid) {
- err = adm9240_update_config(data);
- if (err < 0) {
- data->valid = 0;
- mutex_unlock(>update_lock);
- return ERR_PTR(err);
- }
- data->last_updated_config = jiffies;
data->valid = 1;
}
+
mutex_unlock(>update_lock);
+
return data;
}
@@ -855,7 +844,15 @@ static int adm9240_probe(struct i2c_client *new_client)
new_client->name,
data,
adm9240_groups);
- return PTR_ERR_OR_ZERO(hwmon_dev);
+ if (IS_ERR(hwmon_dev))
+ return PTR_ERR(hwmon_dev);
+
+ /* pull in configuration from an earlier boot stage */
+ err = adm9240_update_config(data);
+ if (err < 0)
+ return err;
+
+ return 0;
}
static const struct i2c_device_id adm9240_id[] = {
--
2.30.1
Re: Errant readings on LM81 with T2080 SoC
On 9/03/21 9:27 am, Chris Packham wrote: > On 8/03/21 5:59 pm, Guenter Roeck wrote: >> Other than that, the only other real idea I have would be to monitor >> the i2c bus. > I am in the fortunate position of being able to go into the office and > even happen to have the expensive scope at the moment. Now I just need > to find a tame HW engineer so I don't burn myself trying to attach the > probes. One thing I see on the scope is that when there is a CPU load there appears to be some clock stretching going on (SCL is held low some times). I don't see it without the CPU load. It's hard to correlate a clock stretching event with a bad read or error but it is one area where the SMBUS spec has a maximum that might cause the device to give up waiting.
Re: Errant readings on LM81 with T2080 SoC
On 8/03/21 1:31 pm, Guenter Roeck wrote: > On 3/7/21 2:52 PM, Chris Packham wrote: >> Fundamentally I think this is a problem with the fact that the LM81 is >> an SMBus device but the T2080 (and other Freescale SoCs) uses i2c and we >> emulate SMBus. I suspect the errant readings are when we don't get round >> to completing the read within the timeout specified by the SMBus >> specification. Depending on when that happens we either fail the >> transfer or interpret the result as all-1s. > That is quite unlikely. Many sensor chips are SMBus chips connected to > i2c busses. It is much more likely that there is a bug in the T2080 i2c > driver, > that the chip doesn't like the bulk read command issued through regmap, that > the chip has problems with the i2c bus speed, or that the i2c bus is noisy. I have noticed that with the switch to regmap we end up using plain i2c instead of SMBUS. There appears to be no way of saying use SMBUS semantics if the i2c adapter reports I2C_FUNC_I2C.
Re: Errant readings on LM81 with T2080 SoC
On 9/03/21 11:10 am, Chris Packham wrote: > > On 8/03/21 5:59 pm, Guenter Roeck wrote: >> On 3/7/21 8:37 PM, Chris Packham wrote: >> [ ... ] >>>> That's from -ENXIO which is used in only one place in i2c-mpc.c. I'll >>>> enable some debug and see what we get. >>> For the errant readings there was nothing abnormal reported by the >>> driver. >>> >>> For the "No such device or address" I saw "mpc-i2c ffe119000.i2c: No >>> RXAK" which matches up with the -ENXIO return. >>> >> Id suggest to check the time until not busy and stop in mpc_xfer(). >> Those hot loops are unusual, and may well mess up the code especially >> if preempt is enabled. > Reworking those loops seems to have had a positive result. I'll do a > bit more testing and hopefully get a patch out later today. D'oh my "fix" was to replace the cond_reshed() with msleep(10) which did "fix" the problem but made every i2c read slow. I didn't notice when testing just the lm81 but as soon as I booted the system with more i2c devices I saw stupidly slow boot times. >> Also, are you using interrupts or polling in >> your system ? The interrupt handler looks a bit odd, with "Read again >> to allow register to stabilise". >> >> Do you have fsl,timeout set in the devicetree properties and, if so, >> have you played with it ? >> >> Other than that, the only other real idea I have would be to monitor >> the i2c bus. >> >> Guenter
Re: Errant readings on LM81 with T2080 SoC
On 8/03/21 5:59 pm, Guenter Roeck wrote: > On 3/7/21 8:37 PM, Chris Packham wrote: > [ ... ] >>> That's from -ENXIO which is used in only one place in i2c-mpc.c. I'll >>> enable some debug and see what we get. >> For the errant readings there was nothing abnormal reported by the driver. >> >> For the "No such device or address" I saw "mpc-i2c ffe119000.i2c: No >> RXAK" which matches up with the -ENXIO return. >> > Id suggest to check the time until not busy and stop in mpc_xfer(). > Those hot loops are unusual, and may well mess up the code especially > if preempt is enabled. Reworking those loops seems to have had a positive result. I'll do a bit more testing and hopefully get a patch out later today. > Also, are you using interrupts or polling in > your system ? The interrupt handler looks a bit odd, with "Read again > to allow register to stabilise". > > Do you have fsl,timeout set in the devicetree properties and, if so, > have you played with it ? > > Other than that, the only other real idea I have would be to monitor > the i2c bus. > > Guenter
Re: Errant readings on LM81 with T2080 SoC
On 8/03/21 5:59 pm, Guenter Roeck wrote: > On 3/7/21 8:37 PM, Chris Packham wrote: > [ ... ] >>> That's from -ENXIO which is used in only one place in i2c-mpc.c. I'll >>> enable some debug and see what we get. >> For the errant readings there was nothing abnormal reported by the driver. >> >> For the "No such device or address" I saw "mpc-i2c ffe119000.i2c: No >> RXAK" which matches up with the -ENXIO return. >> > Id suggest to check the time until not busy and stop in mpc_xfer(). > Those hot loops are unusual, and may well mess up the code especially > if preempt is enabled. Also, are you using interrupts or polling in > your system ? I'm using interrupts but I see the same issue if I comment out the interrupts in the dtsi file (i.e. force it to use polling). > The interrupt handler looks a bit odd, with "Read again > to allow register to stabilise". Yeah that stuck out to me too. The code in question predates git, I went spelunking in history.git and the "Read again" seems to be in the initial version[0]. I did try to alter the interrupt handler so that it only does one read but that didn't seem to change anything. > Do you have fsl,timeout set in the devicetree properties and, if so, > have you played with it ? Haven't got it set but I'll have a go at tweaking it. > Other than that, the only other real idea I have would be to monitor > the i2c bus. I am in the fortunate position of being able to go into the office and even happen to have the expensive scope at the moment. Now I just need to find a tame HW engineer so I don't burn myself trying to attach the probes. -- [0] - https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=11b3235dc04a306f6a9ba14c1ab621b2d54f2c56
Re: Errant readings on LM81 with T2080 SoC
On 8/03/21 3:27 pm, Chris Packham wrote: > > On 8/03/21 1:31 pm, Guenter Roeck wrote: >> On 3/7/21 2:52 PM, Chris Packham wrote: >>> Hi, >>> >>> I've got a system using a PowerPC T2080 SoC and among other things has >>> an LM81 hwmon chip. >>> >>> Under a high CPU load we see errant readings from the LM81 as well as >>> actual failures. It's the errant readings that cause the most concern >>> since we can easily ignore the read errors in our monitoring >>> application >>> (although it would be better if they weren't there at all). >>> >>> I'm able to reproduce this with a test application[0] that artificially >>> creates a high CPU load then by repeatedly checking for the all-1s >>> values from the LM81 datasheet[1](page 17). The all-1s readings stick >>> out as they are obviously higher than the voltage rails that are >>> connected and disagree with measurements taken with a multimeter. >>> >>> Here's the output from my device >>> >>> [root@linuxbox ~]# cpuload 90& >>> [root@linuxbox ~]# (while true; do cat >>> /sys/class/hwmon/hwmon0/in*_input >>> | grep '3320\|4383\|6641\|15930\|3586'; sleep 1; done)& >>> 3586 >>> 3586 >>> cat: read error: No such device or address >>> cat: read error: No such device or address >>> 3320 >>> 3320 >>> 3586 >>> 3586 >>> 6641 >>> 6641 >>> 4383 >>> 4383 >>> >>> Fundamentally I think this is a problem with the fact that the LM81 is >>> an SMBus device but the T2080 (and other Freescale SoCs) uses i2c >>> and we >>> emulate SMBus. I suspect the errant readings are when we don't get >>> round >>> to completing the read within the timeout specified by the SMBus >>> specification. Depending on when that happens we either fail the >>> transfer or interpret the result as all-1s. >>> >> That is quite unlikely. Many sensor chips are SMBus chips connected to >> i2c busses. It is much more likely that there is a bug in the T2080 >> i2c driver, >> that the chip doesn't like the bulk read command issued through >> regmap, that >> the chip has problems with the i2c bus speed, or that the i2c bus is >> noisy. > Perhaps something gets upset when interrupt processing is delayed > because of CPU load. I don't see the problem when there isn't a CPU > load so I think that eliminates board issues. >> In this context, the "No such device or address" responses are very >> suspicious. >> Those are reported by the i2c driver, not by the hwmon driver, and >> suggest >> that the chip did not respond to a read request. Maybe it helps to >> enable >> debugging to the i2c driver to see if it reports anything useful. Even >> better might be to connect an i2c bus analyzer to the i2c bus and check >> what is going on. > That's from -ENXIO which is used in only one place in i2c-mpc.c. I'll > enable some debug and see what we get. For the errant readings there was nothing abnormal reported by the driver. For the "No such device or address" I saw "mpc-i2c ffe119000.i2c: No RXAK" which matches up with the -ENXIO return.
Re: Errant readings on LM81 with T2080 SoC
On 8/03/21 1:31 pm, Guenter Roeck wrote: > On 3/7/21 2:52 PM, Chris Packham wrote: >> Hi, >> >> I've got a system using a PowerPC T2080 SoC and among other things has >> an LM81 hwmon chip. >> >> Under a high CPU load we see errant readings from the LM81 as well as >> actual failures. It's the errant readings that cause the most concern >> since we can easily ignore the read errors in our monitoring application >> (although it would be better if they weren't there at all). >> >> I'm able to reproduce this with a test application[0] that artificially >> creates a high CPU load then by repeatedly checking for the all-1s >> values from the LM81 datasheet[1](page 17). The all-1s readings stick >> out as they are obviously higher than the voltage rails that are >> connected and disagree with measurements taken with a multimeter. >> >> Here's the output from my device >> >> [root@linuxbox ~]# cpuload 90& >> [root@linuxbox ~]# (while true; do cat /sys/class/hwmon/hwmon0/in*_input >> | grep '3320\|4383\|6641\|15930\|3586'; sleep 1; done)& >> 3586 >> 3586 >> cat: read error: No such device or address >> cat: read error: No such device or address >> 3320 >> 3320 >> 3586 >> 3586 >> 6641 >> 6641 >> 4383 >> 4383 >> >> Fundamentally I think this is a problem with the fact that the LM81 is >> an SMBus device but the T2080 (and other Freescale SoCs) uses i2c and we >> emulate SMBus. I suspect the errant readings are when we don't get round >> to completing the read within the timeout specified by the SMBus >> specification. Depending on when that happens we either fail the >> transfer or interpret the result as all-1s. >> > That is quite unlikely. Many sensor chips are SMBus chips connected to > i2c busses. It is much more likely that there is a bug in the T2080 i2c > driver, > that the chip doesn't like the bulk read command issued through regmap, that > the chip has problems with the i2c bus speed, or that the i2c bus is noisy. Perhaps something gets upset when interrupt processing is delayed because of CPU load. I don't see the problem when there isn't a CPU load so I think that eliminates board issues. > In this context, the "No such device or address" responses are very > suspicious. > Those are reported by the i2c driver, not by the hwmon driver, and suggest > that the chip did not respond to a read request. Maybe it helps to enable > debugging to the i2c driver to see if it reports anything useful. Even > better might be to connect an i2c bus analyzer to the i2c bus and check > what is going on. That's from -ENXIO which is used in only one place in i2c-mpc.c. I'll enable some debug and see what we get. > > Guenter
Errant readings on LM81 with T2080 SoC
Hi, I've got a system using a PowerPC T2080 SoC and among other things has an LM81 hwmon chip. Under a high CPU load we see errant readings from the LM81 as well as actual failures. It's the errant readings that cause the most concern since we can easily ignore the read errors in our monitoring application (although it would be better if they weren't there at all). I'm able to reproduce this with a test application[0] that artificially creates a high CPU load then by repeatedly checking for the all-1s values from the LM81 datasheet[1](page 17). The all-1s readings stick out as they are obviously higher than the voltage rails that are connected and disagree with measurements taken with a multimeter. Here's the output from my device [root@linuxbox ~]# cpuload 90& [root@linuxbox ~]# (while true; do cat /sys/class/hwmon/hwmon0/in*_input | grep '3320\|4383\|6641\|15930\|3586'; sleep 1; done)& 3586 3586 cat: read error: No such device or address cat: read error: No such device or address 3320 3320 3586 3586 6641 6641 4383 4383 Fundamentally I think this is a problem with the fact that the LM81 is an SMBus device but the T2080 (and other Freescale SoCs) uses i2c and we emulate SMBus. I suspect the errant readings are when we don't get round to completing the read within the timeout specified by the SMBus specification. Depending on when that happens we either fail the transfer or interpret the result as all-1s. [0] - https://gist.github.com/cpackham/6356a3a943accebb228135dc10daf721 [1] - https://www.ti.com/lit/ds/symlink/lm81.pdf
[PATCH] i2c: mpc: Make use of i2c_recover_bus()
Move the existing calls of mpc_i2c_fixup() to a recovery function
registered via bus_recovery_info. This makes it more obvious that
recovery is supported and allows for a future where recover is triggered
by the i2c core.
Signed-off-by: Chris Packham
---
drivers/i2c/busses/i2c-mpc.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d94f05c8b8b7..6a0d55e9e8e3 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -586,7 +586,7 @@ static int mpc_xfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
writeb(status & ~CSR_MAL,
i2c->base + MPC_I2C_SR);
- mpc_i2c_fixup(i2c);
+ i2c_recover_bus(>adap);
}
return -EIO;
}
@@ -622,7 +622,7 @@ static int mpc_xfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
writeb(status & ~CSR_MAL,
i2c->base + MPC_I2C_SR);
- mpc_i2c_fixup(i2c);
+ i2c_recover_bus(>adap);
}
return -EIO;
}
@@ -637,6 +637,15 @@ static u32 mpc_functionality(struct i2c_adapter *adap)
| I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL;
}
+static int fsl_i2c_bus_recovery(struct i2c_adapter *adap)
+{
+ struct mpc_i2c *i2c = i2c_get_adapdata(adap);
+
+ mpc_i2c_fixup(i2c);
+
+ return 0;
+}
+
static const struct i2c_algorithm mpc_algo = {
.master_xfer = mpc_xfer,
.functionality = mpc_functionality,
@@ -648,6 +657,10 @@ static struct i2c_adapter mpc_ops = {
.timeout = HZ,
};
+static struct i2c_bus_recovery_info fsl_i2c_recovery_info = {
+ .recover_bus = fsl_i2c_bus_recovery,
+};
+
static const struct of_device_id mpc_i2c_of_match[];
static int fsl_i2c_probe(struct platform_device *op)
{
@@ -740,6 +753,7 @@ static int fsl_i2c_probe(struct platform_device *op)
i2c_set_adapdata(>adap, i2c);
i2c->adap.dev.parent = >dev;
i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
+ i2c->adap.bus_recovery_info = _i2c_recovery_info;
result = i2c_add_adapter(>adap);
if (result < 0)
--
2.30.1
[PATCH v2 2/2] hwmon: (pmbus): Add driver for Infineon IR36021
The IR36021 is a dual‐loop digital multi‐phase buck controller.
Signed-off-by: Chris Packham
---
Changes in v2:
- update against latest kernel for pmbus API changes
- avoid double negation
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/ir36021.rst | 62 ++
drivers/hwmon/pmbus/Kconfig | 9
drivers/hwmon/pmbus/Makefile| 1 +
drivers/hwmon/pmbus/ir36021.c | 79 +
5 files changed, 152 insertions(+)
create mode 100644 Documentation/hwmon/ir36021.rst
create mode 100644 drivers/hwmon/pmbus/ir36021.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 8d5a2df1ecb6..b34894403c2b 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -77,6 +77,7 @@ Hardware Monitoring Kernel Drivers
intel-m10-bmc-hwmon
ir35221
ir38064
+ ir36021
isl68137
it87
jc42
diff --git a/Documentation/hwmon/ir36021.rst b/Documentation/hwmon/ir36021.rst
new file mode 100644
index ..36ef8d518b81
--- /dev/null
+++ b/Documentation/hwmon/ir36021.rst
@@ -0,0 +1,62 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver ir36021
+=
+
+Supported chips:
+
+ * Infineon IR36021
+
+Prefix: ir36021
+Addresses scanned: -
+
+Datasheet: Publicly available at the Infineon website
+
https://www.infineon.com/dgdl/ir36021.pdf?fileId=5546d462533600a4015355d0aa2d1775
+
+Authors:
+ - Chris Packham
+
+Description
+---
+
+The IR36021 is a dual‐loop digital multi‐phase buck controller designed for
+point of load applications.
+
+Usage Notes
+---
+
+This driver does not probe for PMBus devices. You will have to instantiate
+devices explicitly.
+
+Sysfs attributes
+
+
+=== ===
+curr1_label "iin"
+curr1_input Measured input current
+curr1_alarm Input fault alarm
+
+curr2_label "iout1"
+curr2_input Measured output current
+curr2_alarm Output over-current alarm
+
+in1_label "vin"
+in1_input Measured input voltage
+in1_alarm Input under-voltage alarm
+
+in2_label "vout1"
+in2_input Measured output voltage
+in2_alarm Output over-voltage alarm
+
+power1_label"pin"
+power1_inputMeasured input power
+power1_alarmInput under-voltage alarm
+
+power2_label"pout1"
+power2_inputMeasured output power
+
+temp1_input Measured temperature
+temp1_alarm Temperature alarm
+
+temp2_input Measured other loop temperature
+temp2_alarm Temperature alarm
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 32d2fc850621..ee8c27b3b83d 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -84,6 +84,15 @@ config SENSORS_IR35221
This driver can also be built as a module. If so, the module will
be called ir35221.
+config SENSORS_IR36021
+ tristate "Infineon IR36021"
+ help
+ If you say yes here you get hardware monitoring support for Infineon
+ IR36021.
+
+ This driver can also be built as a module. If so, the module will
+ be called ir36021.
+
config SENSORS_IR38064
tristate "Infineon IR38064"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..685a6bc2b15f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
obj-$(CONFIG_SENSORS_IBM_CFFPS)+= ibm-cffps.o
obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
+obj-$(CONFIG_SENSORS_IR36021) += ir36021.o
obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
obj-$(CONFIG_SENSORS_ISL68137) += isl68137.o
diff --git a/drivers/hwmon/pmbus/ir36021.c b/drivers/hwmon/pmbus/ir36021.c
new file mode 100644
index ..4767e39cc965
--- /dev/null
+++ b/drivers/hwmon/pmbus/ir36021.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Hardware monitoring driver for Infineon IR36021
+ *
+ * Copyright (c) 2021 Allied Telesis
+ */
+#include
+#include
+#include
+#include
+#include
+#include "pmbus.h"
+
+static struct pmbus_driver_info ir36021_info = {
+ .pages = 1,
+ .format[PSC_VOLTAGE_IN] = linear,
+ .format[PSC_VOLTAGE_OUT] = linear,
+ .format[PSC_CURRENT_IN] = linear,
+ .format[PSC_CURRENT_OUT] = linear,
+ .format[PSC_POWER] = linear,
+ .format[PSC_TEMPERATURE] = linear,
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT
+ | PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT
+
[PATCH v2 1/2] dt-bindings: trivial-devices: Add infineon,ir36021
Add infineon,ir36021 to trivial-devices.yaml. Signed-off-by: Chris Packham --- Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index a327130d1faa..19bc4c301f5b 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -102,6 +102,8 @@ properties: - mps,mp2975 # G751: Digital Temperature Sensor and Thermal Watchdog with Two-Wire Interface - gmt,g751 +# Infineon IR36021 digital POL buck controller + - infineon,ir36021 # Infineon IR38064 Voltage Regulator - infineon,ir38064 # Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz) -- 2.30.0
[PATCH v2 0/2] hwmon: (pmbus): Infineon IR36021 driver
This adds a basic driver for the IR36021. This chip as both a PMBUS and I2C interface that is available simultaenously. This driver is just the PMBUS interface. Chris Packham (2): dt-bindings: trivial-devices: Add infineon,ir36021 hwmon: (pmbus): Add driver for Infineon IR36021 .../devicetree/bindings/trivial-devices.yaml | 2 + Documentation/hwmon/index.rst | 1 + Documentation/hwmon/ir36021.rst | 62 +++ drivers/hwmon/pmbus/Kconfig | 9 +++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/ir36021.c | 79 +++ 6 files changed, 154 insertions(+) create mode 100644 Documentation/hwmon/ir36021.rst create mode 100644 drivers/hwmon/pmbus/ir36021.c -- 2.30.0
[PATCH 2/2] hwmon: (pmbus): Add driver for Infineon IR36021
The IR36021 is a dual‐loop digital multi‐phase buck controller.
Signed-off-by: Chris Packham
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/ir36021.rst | 62 +
drivers/hwmon/pmbus/Kconfig | 9
drivers/hwmon/pmbus/Makefile| 1 +
drivers/hwmon/pmbus/ir36021.c | 81 +
5 files changed, 154 insertions(+)
create mode 100644 Documentation/hwmon/ir36021.rst
create mode 100644 drivers/hwmon/pmbus/ir36021.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 8d5a2df1ecb6..b34894403c2b 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -77,6 +77,7 @@ Hardware Monitoring Kernel Drivers
intel-m10-bmc-hwmon
ir35221
ir38064
+ ir36021
isl68137
it87
jc42
diff --git a/Documentation/hwmon/ir36021.rst b/Documentation/hwmon/ir36021.rst
new file mode 100644
index ..36ef8d518b81
--- /dev/null
+++ b/Documentation/hwmon/ir36021.rst
@@ -0,0 +1,62 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver ir36021
+=
+
+Supported chips:
+
+ * Infineon IR36021
+
+Prefix: ir36021
+Addresses scanned: -
+
+Datasheet: Publicly available at the Infineon website
+
https://www.infineon.com/dgdl/ir36021.pdf?fileId=5546d462533600a4015355d0aa2d1775
+
+Authors:
+ - Chris Packham
+
+Description
+---
+
+The IR36021 is a dual‐loop digital multi‐phase buck controller designed for
+point of load applications.
+
+Usage Notes
+---
+
+This driver does not probe for PMBus devices. You will have to instantiate
+devices explicitly.
+
+Sysfs attributes
+
+
+=== ===
+curr1_label "iin"
+curr1_input Measured input current
+curr1_alarm Input fault alarm
+
+curr2_label "iout1"
+curr2_input Measured output current
+curr2_alarm Output over-current alarm
+
+in1_label "vin"
+in1_input Measured input voltage
+in1_alarm Input under-voltage alarm
+
+in2_label "vout1"
+in2_input Measured output voltage
+in2_alarm Output over-voltage alarm
+
+power1_label"pin"
+power1_inputMeasured input power
+power1_alarmInput under-voltage alarm
+
+power2_label"pout1"
+power2_inputMeasured output power
+
+temp1_input Measured temperature
+temp1_alarm Temperature alarm
+
+temp2_input Measured other loop temperature
+temp2_alarm Temperature alarm
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 32d2fc850621..ee8c27b3b83d 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -84,6 +84,15 @@ config SENSORS_IR35221
This driver can also be built as a module. If so, the module will
be called ir35221.
+config SENSORS_IR36021
+ tristate "Infineon IR36021"
+ help
+ If you say yes here you get hardware monitoring support for Infineon
+ IR36021.
+
+ This driver can also be built as a module. If so, the module will
+ be called ir36021.
+
config SENSORS_IR38064
tristate "Infineon IR38064"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..685a6bc2b15f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
obj-$(CONFIG_SENSORS_IBM_CFFPS)+= ibm-cffps.o
obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
+obj-$(CONFIG_SENSORS_IR36021) += ir36021.o
obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
obj-$(CONFIG_SENSORS_ISL68137) += isl68137.o
diff --git a/drivers/hwmon/pmbus/ir36021.c b/drivers/hwmon/pmbus/ir36021.c
new file mode 100644
index ..99b5c7311afd
--- /dev/null
+++ b/drivers/hwmon/pmbus/ir36021.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Hardware monitoring driver for Infineon IR36021
+ *
+ * Copyright (c) 2021 Allied Telesis
+ */
+#include
+#include
+#include
+#include
+#include
+#include "pmbus.h"
+
+static struct pmbus_driver_info ir36021_info = {
+ .pages = 1,
+ .format[PSC_VOLTAGE_IN] = linear,
+ .format[PSC_VOLTAGE_OUT] = linear,
+ .format[PSC_CURRENT_IN] = linear,
+ .format[PSC_CURRENT_OUT] = linear,
+ .format[PSC_POWER] = linear,
+ .format[PSC_TEMPERATURE] = linear,
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT
+ | PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT
+ | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT
+ | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2
+
[PATCH 1/2] dt-bindings: trivial-devices: Add infineon,ir36021
Add infineon,ir36021 to trivial-devices.yaml. Signed-off-by: Chris Packham --- Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index a327130d1faa..19bc4c301f5b 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -102,6 +102,8 @@ properties: - mps,mp2975 # G751: Digital Temperature Sensor and Thermal Watchdog with Two-Wire Interface - gmt,g751 +# Infineon IR36021 digital POL buck controller + - infineon,ir36021 # Infineon IR38064 Voltage Regulator - infineon,ir38064 # Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz) -- 2.30.0
[PATCH 0/2] hwmon: (pmbus): Infineon IR36021 driver
This adds a basic driver for the IR36021. This chip as both a PMBUS and I2C interface that is available simultaenously. This driver is just the PMBUS interface. Chris Packham (2): dt-bindings: trivial-devices: Add infineon,ir36021 hwmon: (pmbus): Add driver for Infineon IR36021 .../devicetree/bindings/trivial-devices.yaml | 2 + Documentation/hwmon/index.rst | 1 + Documentation/hwmon/ir36021.rst | 62 ++ drivers/hwmon/pmbus/Kconfig | 9 +++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/ir36021.c | 81 +++ 6 files changed, 156 insertions(+) create mode 100644 Documentation/hwmon/ir36021.rst create mode 100644 drivers/hwmon/pmbus/ir36021.c -- 2.30.0
Re: [PATCH v2] of/fdt: Remove redundant kbasename function call
On 26/01/21 3:06 pm, Rob Herring wrote:
> +LAKML given it's an Arm issue
>
> On Mon, Jan 25, 2021 at 6:47 PM Chris Packham
> wrote:
>> Hi All,
>>
>> On 29/05/20 1:25 am, Qi Zheng wrote:
>>> For version 1 to 3 of the device tree, this is the node full
>>> path as a zero terminated string, starting with "/". The
>>> following equation will not hold, since the node name has
>>> been processed in the fdt_get_name().
>>>
>>>*pathp == '/'
>>>
>>> For version 16 and later, this is the node unit name only
>>> (or an empty string for the root node). So the above
>>> equation will still not hold.
>>>
>>> So the kbasename() is redundant, just remove it.
>>>
>>> Signed-off-by: Qi Zheng
>>> ---
>>>
>>> Change in v2:
>>>remove another kbasename() also.
>>>
>>>drivers/of/fdt.c | 4
>>>1 file changed, 4 deletions(-)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index 38619e9ef6b2..4602e467ca8b 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -643,8 +643,6 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
>>> offset = fdt_next_node(blob, offset, )) {
>>>
>>>pathp = fdt_get_name(blob, offset, NULL);
>>> - if (*pathp == '/')
>>> - pathp = kbasename(pathp);
>>>rc = it(offset, pathp, depth, data);
>>>}
>>>return rc;
>>> @@ -671,8 +669,6 @@ int __init of_scan_flat_dt_subnodes(unsigned long
>>> parent,
>>>int rc;
>>>
>>>pathp = fdt_get_name(blob, node, NULL);
>>> - if (*pathp == '/')
>>> - pathp = kbasename(pathp);
>>>rc = it(node, pathp, data);
>>>if (rc)
>>>return rc;
>> I'm trying to keep our boards up to date with newer kernels.
>>
>> I've just hit a problem on an older board that uses
>> CONFIG_ARM_APPENDED_DTB and has a number of command line args passed up
>> from the bootloader that are required for a successful boot.
>>
>> I'm stepping through kernel versions in the hope that keeping things
>> running is easier in smaller increments I'm up to v5.8. I'm not
>> currently able to check a newer kernel on this board but looking at the
>> code the problem still seems to exist in the latest tree.
>>
>> early_init_dt_scan_chosen() searches for "chosen" prior to this change
>> the "/chosen" node that gets inserted by atags_to_fdt.c but with this
>> change it can't find it and fails to boot.
> Given this code works for normal cases, I'm guessing the problem is in
> atags_to_fdt.c or libfdt.
It might be related tot this snippet of libfdt
const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
{
...
if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
/*
* For old FDT versions, match the naming conventions
of V16:
* give only the leaf name (after all /). The actual tree
* contents are loosely checked.
*/
const char *leaf;
leaf = strrchr(nameptr, '/');
if (leaf == NULL) {
err = -FDT_ERR_BADSTRUCTURE;
goto fail;
}
nameptr = leaf+1;
}
...
}
On my system that if evaluates to 0
> Is it possible to add an empty chosen node
> to the DT and see if that makes any difference.
I'll give that a try.
[PATCH] of/fdt: Check against '/chosen' in early_init_dt_scan_chosen
of_scan_flat_dt() passes the name of the visited node to the iterator.
In the case of '/chosen' this includes the leading '/'. Update
early_init_dt_scan_chosen() to expect this.
Fixes: 7536c7e03e74 ("of/fdt: Remove redundant kbasename function call")
Signed-off-by: Chris Packham
---
drivers/of/fdt.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index feb0f2d67fc5..861aedf0bb7c 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1043,7 +1043,10 @@ int __init early_init_dt_scan_chosen(unsigned long node,
const char *uname,
pr_debug("search \"chosen\", depth: %d, uname: %s\n", depth, uname);
if (depth != 1 || !data ||
- (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
+ (strcmp(uname, "chosen") != 0 &&
+strcmp(uname, "chosen@0") != 0 &&
+strcmp(uname, "/chosen") != 0 &&
+strcmp(uname, "/chosen@0") != 0))
return 0;
early_init_dt_check_for_initrd(node);
--
2.30.0
Re: [PATCH v2] of/fdt: Remove redundant kbasename function call
Hi All,
On 29/05/20 1:25 am, Qi Zheng wrote:
> For version 1 to 3 of the device tree, this is the node full
> path as a zero terminated string, starting with "/". The
> following equation will not hold, since the node name has
> been processed in the fdt_get_name().
>
> *pathp == '/'
>
> For version 16 and later, this is the node unit name only
> (or an empty string for the root node). So the above
> equation will still not hold.
>
> So the kbasename() is redundant, just remove it.
>
> Signed-off-by: Qi Zheng
> ---
>
> Change in v2:
> remove another kbasename() also.
>
> drivers/of/fdt.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 38619e9ef6b2..4602e467ca8b 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -643,8 +643,6 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
>offset = fdt_next_node(blob, offset, )) {
>
> pathp = fdt_get_name(blob, offset, NULL);
> - if (*pathp == '/')
> - pathp = kbasename(pathp);
> rc = it(offset, pathp, depth, data);
> }
> return rc;
> @@ -671,8 +669,6 @@ int __init of_scan_flat_dt_subnodes(unsigned long parent,
> int rc;
>
> pathp = fdt_get_name(blob, node, NULL);
> - if (*pathp == '/')
> - pathp = kbasename(pathp);
> rc = it(node, pathp, data);
> if (rc)
> return rc;
I'm trying to keep our boards up to date with newer kernels.
I've just hit a problem on an older board that uses
CONFIG_ARM_APPENDED_DTB and has a number of command line args passed up
from the bootloader that are required for a successful boot.
I'm stepping through kernel versions in the hope that keeping things
running is easier in smaller increments I'm up to v5.8. I'm not
currently able to check a newer kernel on this board but looking at the
code the problem still seems to exist in the latest tree.
early_init_dt_scan_chosen() searches for "chosen" prior to this change
the "/chosen" node that gets inserted by atags_to_fdt.c but with this
change it can't find it and fails to boot.
Re: [PATCH] bus: mvebu-mbus: make iounmap() symmetric with ioremap()
Hi All, On 12/11/20 9:02 pm, Thomas Petazzoni wrote: > On Thu, 12 Nov 2020 16:21:49 +1300 > Chris Packham wrote: > >> make coccicheck complains: >> >>./drivers/bus/mvebu-mbus.c:1113:2-8: ERROR: missing iounmap; ioremap on >> line 1106 and execution via conditional on line >> >> It took some staring but I don't think there is a problem because the >> file global `mbus_state` is passed mvebu_mbus_common_init() as the >> `mbus` parameter so `mbus_state.mbuswins_base` and `mbus->mbuswins_base` >> are the same thing. But this is confusing for anyone reading the code >> and one less complaint from coccicheck would be nice so lets fix it. >> >> Signed-off-by: Chris Packham > Acked-by: Thomas Petazzoni Just going through some old branches. This doesn't seem to have been picked up. Have I missed a maintainer?
Re: orion-nand: uncorrectable ECC error on v5.10-rc6
Hi Miquel,
On 2/12/20 9:31 pm, Miquel Raynal wrote:
> Hi Chris,
>
> Chris Packham wrote on Wed, 2 Dec
> 2020 08:23:13 +:
>
>> Hi Miquel,
>>
>> On 2/12/20 8:59 pm, Miquel Raynal wrote:
>>> Hi Chris,
>>>
>>> Chris Packham wrote on Wed, 2 Dec
>>> 2020 07:47:32 +:
>>>
>>>> Hi,
>>>>
>>>> I've just booted v5.10-rc6 on a kirkwood based board (which uses the
>>>> orion-nand driver) and I get the following errors reported. I haven't
>>>> started bisecting yet but v5.7.19 mounts the nand flash without any issue.
>>>>
>>>> ubi0: attaching mtd0
>>>> __nand_correct_data: uncorrectable ECC error
>>>> ubi0 warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes
>>>> from PEB 0:0, read only 64 bytes, retry
>>>> __nand_correct_data: uncorrectable ECC error
>>>> ubi0 warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes
>>>> from PEB 0:0, read only 64 bytes, retry
>>>> __nand_correct_data: uncorrectable ECC error
>>>> ubi0 warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes
>>>> from PEB 0:0, read only 64 bytes, retry
>>>> __nand_correct_data: uncorrectable ECC error
>>>> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
>>>> from PEB 0:0, read 64 bytes
>>>> CPU: 0 PID: 101 Comm: ubiattach Not tainted 5.10.0-rc6+ #1
>>>> Hardware name: Marvell Kirkwood (Flattened Device Tree)
>>>> [<8010ca64>] (unwind_backtrace) from [<80109bd0>] (show_stack+0x10/0x14)
>>>> [<80109bd0>] (show_stack) from [<8045f10c>] (ubi_io_read+0x184/0x304)
>>>> [<8045f10c>] (ubi_io_read) from [<8045f4ac>]
>>>> (ubi_io_read_ec_hdr+0x44/0x240)
>>>> [<8045f4ac>] (ubi_io_read_ec_hdr) from [<80464db0>]
>>>> (ubi_attach+0x178/0x15fc)
>>>> [<80464db0>] (ubi_attach) from [<80458d8c>]
>>>> (ubi_attach_mtd_dev+0x538/0xb48)
>>>> [<80458d8c>] (ubi_attach_mtd_dev) from [<8045a114>]
>>>> (ctrl_cdev_ioctl+0x170/0x1e0)
>>>> [<8045a114>] (ctrl_cdev_ioctl) from [<80203094>] (sys_ioctl+0x1f8/0x990)
>>>> [<80203094>] (sys_ioctl) from [<80100060>] (ret_fast_syscall+0x0/0x50)
>>>> Exception stack(0x87633fa8 to 0x87633ff0)
>>>> 3fa0: 0003 7e9b0c30 0003 40186f40 7e9b0c30
>>>>
>>>> 3fc0: 0003 7e9b0c30 000148f8 0036 00014770 00013f90 76f3dfa4
>>>>
>>>> 3fe0: 76e936f0 7e9b0c1c 00011f68 76e936fc
>>> I recently contributed a pile of fixes to ensure DT parsing was not
>>> broken and this applies to Orion. Can you please check
>>>
>>> mtd: rawnand: orion: Move the ECC initialization to ->attach_chip()
>> That looks to be it. In Linus's tree commit 76dc2bfc2e1b ("Merge tag
>> 'mtd/fixes-for-5.10-rc6' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux") seems to be
>> the difference between working and not working.
>>> And tell me if you see something wrong there? I assumed this driver was
>>> not supporting on host ECC engines and only soft Hamming was used, is
>>> this assumption wrong?
>> Our dts has
>>
>> nand-ecc-mode = "soft";
>> nand-ecc-algo = "bch";
>> nand-on-flash-bbt;
>>
> I assumed Hamming was the only possible algorithm, this is the error.
>
> I have several drivers in this case then.
>
> We need to default to Hamming but let the user decide then. Can you try
> something like the below change please?
>
>
> Thanks,
> Miquèl
>
>
> ---8<---
>
> Author: Miquel Raynal
> Date: Wed Dec 2 09:31:14 2020 +0100
>
> mtd: rawnand: orion: Fix soft ECC algo selection
>
> Signed-off-by: Miquel Raynal
>
> diff --git a/drivers/mtd/nand/raw/orion_nand.c
> b/drivers/mtd/nand/raw/orion_nand.c
> index e3bb65fd3ab2..66211c9311d2 100644
> --- a/drivers/mtd/nand/raw/orion_nand.c
> +++ b/drivers/mtd/nand/raw/orion_nand.c
> @@ -86,7 +86,9 @@ static void orion_nand_read_buf(struct nand_chip *chip,
> uint8_t *buf, int len)
> static int orion_nand_attach_chip(struct nand_chip *chip)
> {
> chip->ecc.engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
> - chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
> +
> + if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN)
> + chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
>
> return 0;
> }
>
Thanks, that seems to have fixed it.
Tested-by: Chris Packham
Re: orion-nand: uncorrectable ECC error on v5.10-rc6
Hi Miquel,
On 2/12/20 8:59 pm, Miquel Raynal wrote:
> Hi Chris,
>
> Chris Packham wrote on Wed, 2 Dec
> 2020 07:47:32 +:
>
>> Hi,
>>
>> I've just booted v5.10-rc6 on a kirkwood based board (which uses the
>> orion-nand driver) and I get the following errors reported. I haven't
>> started bisecting yet but v5.7.19 mounts the nand flash without any issue.
>>
>> ubi0: attaching mtd0
>> __nand_correct_data: uncorrectable ECC error
>> ubi0 warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes
>> from PEB 0:0, read only 64 bytes, retry
>> __nand_correct_data: uncorrectable ECC error
>> ubi0 warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes
>> from PEB 0:0, read only 64 bytes, retry
>> __nand_correct_data: uncorrectable ECC error
>> ubi0 warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes
>> from PEB 0:0, read only 64 bytes, retry
>> __nand_correct_data: uncorrectable ECC error
>> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
>> from PEB 0:0, read 64 bytes
>> CPU: 0 PID: 101 Comm: ubiattach Not tainted 5.10.0-rc6+ #1
>> Hardware name: Marvell Kirkwood (Flattened Device Tree)
>> [<8010ca64>] (unwind_backtrace) from [<80109bd0>] (show_stack+0x10/0x14)
>> [<80109bd0>] (show_stack) from [<8045f10c>] (ubi_io_read+0x184/0x304)
>> [<8045f10c>] (ubi_io_read) from [<8045f4ac>] (ubi_io_read_ec_hdr+0x44/0x240)
>> [<8045f4ac>] (ubi_io_read_ec_hdr) from [<80464db0>]
>> (ubi_attach+0x178/0x15fc)
>> [<80464db0>] (ubi_attach) from [<80458d8c>] (ubi_attach_mtd_dev+0x538/0xb48)
>> [<80458d8c>] (ubi_attach_mtd_dev) from [<8045a114>]
>> (ctrl_cdev_ioctl+0x170/0x1e0)
>> [<8045a114>] (ctrl_cdev_ioctl) from [<80203094>] (sys_ioctl+0x1f8/0x990)
>> [<80203094>] (sys_ioctl) from [<80100060>] (ret_fast_syscall+0x0/0x50)
>> Exception stack(0x87633fa8 to 0x87633ff0)
>> 3fa0: 0003 7e9b0c30 0003 40186f40 7e9b0c30
>>
>> 3fc0: 0003 7e9b0c30 000148f8 0036 00014770 00013f90 76f3dfa4
>>
>> 3fe0: 76e936f0 7e9b0c1c 00011f68 76e936fc
> I recently contributed a pile of fixes to ensure DT parsing was not
> broken and this applies to Orion. Can you please check
>
> mtd: rawnand: orion: Move the ECC initialization to ->attach_chip()
That looks to be it. In Linus's tree commit 76dc2bfc2e1b ("Merge tag
'mtd/fixes-for-5.10-rc6' of
git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux") seems to be
the difference between working and not working.
> And tell me if you see something wrong there? I assumed this driver was
> not supporting on host ECC engines and only soft Hamming was used, is
> this assumption wrong?
Our dts has
nand-ecc-mode = "soft";
nand-ecc-algo = "bch";
nand-on-flash-bbt;
orion-nand: uncorrectable ECC error on v5.10-rc6
Hi, I've just booted v5.10-rc6 on a kirkwood based board (which uses the orion-nand driver) and I get the following errors reported. I haven't started bisecting yet but v5.7.19 mounts the nand flash without any issue. ubi0: attaching mtd0 __nand_correct_data: uncorrectable ECC error ubi0 warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes from PEB 0:0, read only 64 bytes, retry __nand_correct_data: uncorrectable ECC error ubi0 warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes from PEB 0:0, read only 64 bytes, retry __nand_correct_data: uncorrectable ECC error ubi0 warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes from PEB 0:0, read only 64 bytes, retry __nand_correct_data: uncorrectable ECC error ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes from PEB 0:0, read 64 bytes CPU: 0 PID: 101 Comm: ubiattach Not tainted 5.10.0-rc6+ #1 Hardware name: Marvell Kirkwood (Flattened Device Tree) [<8010ca64>] (unwind_backtrace) from [<80109bd0>] (show_stack+0x10/0x14) [<80109bd0>] (show_stack) from [<8045f10c>] (ubi_io_read+0x184/0x304) [<8045f10c>] (ubi_io_read) from [<8045f4ac>] (ubi_io_read_ec_hdr+0x44/0x240) [<8045f4ac>] (ubi_io_read_ec_hdr) from [<80464db0>] (ubi_attach+0x178/0x15fc) [<80464db0>] (ubi_attach) from [<80458d8c>] (ubi_attach_mtd_dev+0x538/0xb48) [<80458d8c>] (ubi_attach_mtd_dev) from [<8045a114>] (ctrl_cdev_ioctl+0x170/0x1e0) [<8045a114>] (ctrl_cdev_ioctl) from [<80203094>] (sys_ioctl+0x1f8/0x990) [<80203094>] (sys_ioctl) from [<80100060>] (ret_fast_syscall+0x0/0x50) Exception stack(0x87633fa8 to 0x87633ff0) 3fa0: 0003 7e9b0c30 0003 40186f40 7e9b0c30 3fc0: 0003 7e9b0c30 000148f8 0036 00014770 00013f90 76f3dfa4 3fe0: 76e936f0 7e9b0c1c 00011f68 76e936fc
Re: [PATCH v2] ARM: dts: mvebu: Add device tree for ATL-x530 Board
On 1/12/20 12:03 pm, Andrew Lunn wrote: > On Tue, Dec 01, 2020 at 11:35:07AM +1300, Aryan Srivastava wrote: >> Add device tree file for x530 board. This has an Armada 385 SoC. Has >> NAND-flash for user storage and SPI for booting. Covers majority of x530 >> and GS980MX variants. > Hi Aryan > > What exactly does that mean, it covers most variants? We were trying to avoid the patch submission becoming marketing spam and have probably been a little too vague. Between the x530, x530L and GS980MX there are 24 different models. The differences between them are in the type and number of switch ASICs, the number of PSUs and whether PoE is supported. But the CPU block is largely the same between models. > I'm just wondering if this should be a .dtsi file, and then each > variant has a .dts file specific to it? This is what we've done in our kernel fork. But for upstreaming we wanted to start slow and maybe move things into one or more common .dtsi files if/when needed (in particular there are some models in development now that use different i2c mux and hwmon chips). > Or is the hardware itself the > same, and only the software varies between the variants? Kind of both. This patch submission basically represents the common items among all the (currently shipping) variants.
[net-next PATCH] net: freescale: ucc_geth: remove unused SKB_ALLOC_TIMEOUT
This was added in commit ce973b141dfa ("[PATCH] Freescale QE UCC gigabit
ethernet driver") but doesn't appear to have been used. Remove it now.
Signed-off-by: Chris Packham
---
drivers/net/ethernet/freescale/ucc_geth.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.h
b/drivers/net/ethernet/freescale/ucc_geth.h
index 3fe903972195..1a9bdf66a7d8 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.h
+++ b/drivers/net/ethernet/freescale/ucc_geth.h
@@ -882,7 +882,6 @@ struct ucc_geth_hardware_statistics {
addresses */
#define TX_TIMEOUT (1*HZ)
-#define SKB_ALLOC_TIMEOUT 10
#define PHY_INIT_TIMEOUT10
#define PHY_CHANGE_TIME 2
--
2.29.2
[net-next PATCH v5 2/4] net: dsa: mv88e6xxx: Support serdes ports on MV88E6097/6095/6185
Implement serdes_power, serdes_get_lane and serdes_pcs_get_state ops for
the MV88E6097/6095/6185 so that ports 8 & 9 can be supported as serdes
ports and directly connected to other network interfaces or to SFPs
without a PHY.
Signed-off-by: Chris Packham
Reviewed-by: Andrew Lunn
---
Changes in v5:
- None
Changes in v4:
- None
Changes in v3:
- Add comment to mv88e6185_serdes_get_lane
- Add review from Andrew
Changes in v2:
- expand support to cover 6095 and 6185
- move serdes related code to serdes.c
drivers/net/dsa/mv88e6xxx/chip.c | 9 +
drivers/net/dsa/mv88e6xxx/serdes.c | 62 ++
drivers/net/dsa/mv88e6xxx/serdes.h | 5 +++
3 files changed, 76 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 296932b2b80d..545eb9c6c3fc 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3263,6 +3263,9 @@ static const struct mv88e6xxx_ops mv88e6095_ops = {
.stats_get_strings = mv88e6095_stats_get_strings,
.stats_get_stats = mv88e6095_stats_get_stats,
.mgmt_rsvd2cpu = mv88e6185_g2_mgmt_rsvd2cpu,
+ .serdes_power = mv88e6185_serdes_power,
+ .serdes_get_lane = mv88e6185_serdes_get_lane,
+ .serdes_pcs_get_state = mv88e6185_serdes_pcs_get_state,
.ppu_enable = mv88e6185_g1_ppu_enable,
.ppu_disable = mv88e6185_g1_ppu_disable,
.reset = mv88e6185_g1_reset,
@@ -3302,6 +3305,9 @@ static const struct mv88e6xxx_ops mv88e6097_ops = {
.set_egress_port = mv88e6095_g1_set_egress_port,
.watchdog_ops = _watchdog_ops,
.mgmt_rsvd2cpu = mv88e6352_g2_mgmt_rsvd2cpu,
+ .serdes_power = mv88e6185_serdes_power,
+ .serdes_get_lane = mv88e6185_serdes_get_lane,
+ .serdes_pcs_get_state = mv88e6185_serdes_pcs_get_state,
.pot_clear = mv88e6xxx_g2_pot_clear,
.reset = mv88e6352_g1_reset,
.rmu_disable = mv88e6085_g1_rmu_disable,
@@ -3736,6 +3742,9 @@ static const struct mv88e6xxx_ops mv88e6185_ops = {
.set_egress_port = mv88e6095_g1_set_egress_port,
.watchdog_ops = _watchdog_ops,
.mgmt_rsvd2cpu = mv88e6185_g2_mgmt_rsvd2cpu,
+ .serdes_power = mv88e6185_serdes_power,
+ .serdes_get_lane = mv88e6185_serdes_get_lane,
+ .serdes_pcs_get_state = mv88e6185_serdes_pcs_get_state,
.set_cascade_port = mv88e6185_g1_set_cascade_port,
.ppu_enable = mv88e6185_g1_ppu_enable,
.ppu_disable = mv88e6185_g1_ppu_disable,
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c
b/drivers/net/dsa/mv88e6xxx/serdes.c
index 9c07b4f3d345..d4f40a739b17 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -428,6 +428,68 @@ u8 mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip,
int port)
return lane;
}
+int mv88e6185_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
+ bool up)
+{
+ /* The serdes power can't be controlled on this switch chip but we need
+* to supply this function to avoid returning -EOPNOTSUPP in
+* mv88e6xxx_serdes_power_up/mv88e6xxx_serdes_power_down
+*/
+ return 0;
+}
+
+u8 mv88e6185_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
+{
+ /* There are no configurable serdes lanes on this switch chip but we
+* need to return non-zero so that callers of
+* mv88e6xxx_serdes_get_lane() know this is a serdes port.
+*/
+ switch (chip->ports[port].cmode) {
+ case MV88E6185_PORT_STS_CMODE_SERDES:
+ case MV88E6185_PORT_STS_CMODE_1000BASE_X:
+ return 0xff;
+ default:
+ return 0;
+ }
+}
+
+int mv88e6185_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port,
+ u8 lane, struct phylink_link_state *state)
+{
+ int err;
+ u16 status;
+
+ err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, );
+ if (err)
+ return err;
+
+ state->link = !!(status & MV88E6XXX_PORT_STS_LINK);
+
+ if (state->link) {
+ state->duplex = status & MV88E6XXX_PORT_STS_DUPLEX ?
DUPLEX_FULL : DUPLEX_HALF;
+
+ switch (status & MV88E6XXX_PORT_STS_SPEED_MASK) {
+ case MV88E6XXX_PORT_STS_SPEED_1000:
+ state->speed = SPEED_1000;
+ break;
+ case MV88E6XXX_PORT_STS_SPEED_100:
+ state->speed = SPEED_100;
+ break;
+ case MV88E6XXX_PORT_STS_SPEED_10:
+ state->speed = SPEED_10;
+ break;
+ default:
+ dev_err(chip->dev, "invalid PHY speed\n");
+ return -EINVAL;
+ }
+ } else {
+ state->duplex = DUPLEX_UNKNOWN;
+ state->speed = SPEED_UNKNOWN;
+
[net-next PATCH v5 1/4] net: dsa: mv88e6xxx: Don't force link when using in-band-status
When a port is configured with 'managed = "in-band-status"' switch chips
like the 88E6390 need to propagate the SERDES link state to the MAC
because the link state is not correctly detected. This causes problems
on the 88E6185/88E6097 where the link partner won't see link state
changes because we're forcing the link.
To address this introduce a new device specific op port_sync_link() and
push the logic from mv88e6xxx_mac_link_up() into that. Provide an
implementation for the 88E6185 like devices which doesn't force the
link.
Signed-off-by: Chris Packham
---
Changes in v5:
- Update mv88e6xxx_mac_link_down code path
Changes in v4:
- Introduce new device op
- Remove review from Andrew as things have changed a lot
Changes in v3:
- None
Changes in v2:
- Add review from Andrew
drivers/net/dsa/mv88e6xxx/chip.c | 35 +++
drivers/net/dsa/mv88e6xxx/chip.h | 4
drivers/net/dsa/mv88e6xxx/port.c | 36
drivers/net/dsa/mv88e6xxx/port.h | 3 +++
4 files changed, 74 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index e8258db8c21e..296932b2b80d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -727,8 +727,8 @@ static void mv88e6xxx_mac_link_down(struct dsa_switch *ds,
int port,
mv88e6xxx_reg_lock(chip);
if ((!mv88e6xxx_port_ppu_updates(chip, port) ||
-mode == MLO_AN_FIXED) && ops->port_set_link)
- err = ops->port_set_link(chip, port, LINK_FORCED_DOWN);
+mode == MLO_AN_FIXED) && ops->port_sync_link)
+ err = ops->port_sync_link(chip, port, mode, false);
mv88e6xxx_reg_unlock(chip);
if (err)
@@ -768,8 +768,8 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds,
int port,
goto error;
}
- if (ops->port_set_link)
- err = ops->port_set_link(chip, port, LINK_FORCED_UP);
+ if (ops->port_sync_link)
+ err = ops->port_sync_link(chip, port, mode, true);
}
error:
mv88e6xxx_reg_unlock(chip);
@@ -3210,6 +3210,7 @@ static const struct mv88e6xxx_ops mv88e6085_ops = {
.phy_read = mv88e6185_phy_ppu_read,
.phy_write = mv88e6185_phy_ppu_write,
.port_set_link = mv88e6xxx_port_set_link,
+ .port_sync_link = mv88e6xxx_port_sync_link,
.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
.port_tag_remap = mv88e6095_port_tag_remap,
.port_set_frame_mode = mv88e6351_port_set_frame_mode,
@@ -3249,6 +3250,7 @@ static const struct mv88e6xxx_ops mv88e6095_ops = {
.phy_read = mv88e6185_phy_ppu_read,
.phy_write = mv88e6185_phy_ppu_write,
.port_set_link = mv88e6xxx_port_set_link,
+ .port_sync_link = mv88e6185_port_sync_link,
.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
.port_set_frame_mode = mv88e6085_port_set_frame_mode,
.port_set_egress_floods = mv88e6185_port_set_egress_floods,
@@ -3279,6 +3281,7 @@ static const struct mv88e6xxx_ops mv88e6097_ops = {
.phy_read = mv88e6xxx_g2_smi_phy_read,
.phy_write = mv88e6xxx_g2_smi_phy_write,
.port_set_link = mv88e6xxx_port_set_link,
+ .port_sync_link = mv88e6185_port_sync_link,
.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
.port_tag_remap = mv88e6095_port_tag_remap,
.port_set_frame_mode = mv88e6351_port_set_frame_mode,
@@ -3317,6 +3320,7 @@ static const struct mv88e6xxx_ops mv88e6123_ops = {
.phy_read = mv88e6xxx_g2_smi_phy_read,
.phy_write = mv88e6xxx_g2_smi_phy_write,
.port_set_link = mv88e6xxx_port_set_link,
+ .port_sync_link = mv88e6xxx_port_sync_link,
.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
.port_set_frame_mode = mv88e6085_port_set_frame_mode,
.port_set_egress_floods = mv88e6352_port_set_egress_floods,
@@ -3351,6 +3355,7 @@ static const struct mv88e6xxx_ops mv88e6131_ops = {
.phy_read = mv88e6185_phy_ppu_read,
.phy_write = mv88e6185_phy_ppu_write,
.port_set_link = mv88e6xxx_port_set_link,
+ .port_sync_link = mv88e6xxx_port_sync_link,
.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
.port_tag_remap = mv88e6095_port_tag_remap,
.port_set_frame_mode = mv88e6351_port_set_frame_mode,
@@ -3392,6 +3397,7 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
.phy_read = mv88e6xxx_g2_smi_phy_read,
.phy_write = mv88e6xxx_g2_smi_phy_write,
.port_set_link = mv88e6xxx_port_set_link,
+ .port_sync_link = mv88e6xxx_port_sync_link,
.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
.port_set_speed_duplex = mv88e6341_port_set_speed_duplex,
.port_max_speed_mode = mv88e6341_port_max_speed_mode,
@@ -3
[net-next PATCH v5 4/4] net: dsa: mv88e6xxx: Handle error in serdes_get_regs
If the underlying read operation failed we would end up writing stale
data to the supplied buffer. This would end up with the last
successfully read value repeating. Fix this by only writing the data
when we know the read was good. This will mean that failed values will
return 0x.
Signed-off-by: Chris Packham
Reviewed-by: Andrew Lunn
---
Changes in v5:
- Add review from Andrew.
Changes in v4:
- new
drivers/net/dsa/mv88e6xxx/serdes.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c
b/drivers/net/dsa/mv88e6xxx/serdes.c
index e60e8f0d0225..3195936dc5be 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -400,14 +400,16 @@ void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip
*chip, int port, void *_p)
{
u16 *p = _p;
u16 reg;
+ int err;
int i;
if (!mv88e6352_port_has_serdes(chip, port))
return;
for (i = 0 ; i < 32; i++) {
- mv88e6352_serdes_read(chip, i, );
- p[i] = reg;
+ err = mv88e6352_serdes_read(chip, i, );
+ if (!err)
+ p[i] = reg;
}
}
@@ -1096,6 +1098,7 @@ void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip
*chip, int port, void *_p)
u16 *p = _p;
int lane;
u16 reg;
+ int err;
int i;
lane = mv88e6xxx_serdes_get_lane(chip, port);
@@ -1103,8 +1106,9 @@ void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip
*chip, int port, void *_p)
return;
for (i = 0 ; i < ARRAY_SIZE(mv88e6390_serdes_regs); i++) {
- mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
- mv88e6390_serdes_regs[i], );
- p[i] = reg;
+ err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+ mv88e6390_serdes_regs[i], );
+ if (!err)
+ p[i] = reg;
}
}
--
2.29.2
[net-next PATCH v5 3/4] net: dsa: mv88e6xxx: Add serdes interrupt support for MV88E6097
The MV88E6097 presents the serdes interrupts for ports 8 and 9 via the
Switch Global 2 registers. There is no additional layer of
enablinh/disabling the serdes interrupts like other mv88e6xxx switches.
Even though most of the serdes behaviour is the same as the MV88E6185
that chip does not provide interrupts for serdes events so unlike
earlier commits the functions added here are specific to the MV88E6097.
Signed-off-by: Chris Packham
---
Changes in v5:
- New
drivers/net/dsa/mv88e6xxx/chip.c | 3 ++
drivers/net/dsa/mv88e6xxx/serdes.c | 47 ++
drivers/net/dsa/mv88e6xxx/serdes.h | 4 +++
3 files changed, 54 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 545eb9c6c3fc..e7f68ac0c7e3 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3308,6 +3308,9 @@ static const struct mv88e6xxx_ops mv88e6097_ops = {
.serdes_power = mv88e6185_serdes_power,
.serdes_get_lane = mv88e6185_serdes_get_lane,
.serdes_pcs_get_state = mv88e6185_serdes_pcs_get_state,
+ .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
+ .serdes_irq_enable = mv88e6097_serdes_irq_enable,
+ .serdes_irq_status = mv88e6097_serdes_irq_status,
.pot_clear = mv88e6xxx_g2_pot_clear,
.reset = mv88e6352_g1_reset,
.rmu_disable = mv88e6085_g1_rmu_disable,
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c
b/drivers/net/dsa/mv88e6xxx/serdes.c
index d4f40a739b17..e60e8f0d0225 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -490,6 +490,53 @@ int mv88e6185_serdes_pcs_get_state(struct mv88e6xxx_chip
*chip, int port,
return 0;
}
+int mv88e6097_serdes_irq_enable(struct mv88e6xxx_chip *chip, int port, u8 lane,
+ bool enable)
+{
+ u8 cmode = chip->ports[port].cmode;
+
+ /* The serdes interrupts are enabled in the G2_INT_MASK register. We
+* need to return 0 to avoid returning -EOPNOTSUPP in
+* mv88e6xxx_serdes_irq_enable/mv88e6xxx_serdes_irq_disable
+*/
+ switch (cmode) {
+ case MV88E6185_PORT_STS_CMODE_SERDES:
+ case MV88E6185_PORT_STS_CMODE_1000BASE_X:
+ return 0;
+ }
+
+ return -EOPNOTSUPP;
+}
+
+static void mv88e6097_serdes_irq_link(struct mv88e6xxx_chip *chip, int port)
+{
+ u16 status;
+ int err;
+
+ err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, );
+ if (err) {
+ dev_err(chip->dev, "can't read port status: %d\n", err);
+ return;
+ }
+
+ dsa_port_phylink_mac_change(chip->ds, port, !!(status &
MV88E6XXX_PORT_STS_LINK));
+}
+
+irqreturn_t mv88e6097_serdes_irq_status(struct mv88e6xxx_chip *chip, int port,
+ u8 lane)
+{
+ u8 cmode = chip->ports[port].cmode;
+
+ switch (cmode) {
+ case MV88E6185_PORT_STS_CMODE_SERDES:
+ case MV88E6185_PORT_STS_CMODE_1000BASE_X:
+ mv88e6097_serdes_irq_link(chip, port);
+ return IRQ_HANDLED;
+ }
+
+ return IRQ_NONE;
+}
+
u8 mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
{
u8 cmode = chip->ports[port].cmode;
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h
b/drivers/net/dsa/mv88e6xxx/serdes.h
index c24ec4122c9e..93822ef9bab8 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -110,10 +110,14 @@ int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip,
int port, u8 lane,
bool on);
int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
bool on);
+int mv88e6097_serdes_irq_enable(struct mv88e6xxx_chip *chip, int port, u8 lane,
+ bool enable);
int mv88e6352_serdes_irq_enable(struct mv88e6xxx_chip *chip, int port, u8 lane,
bool enable);
int mv88e6390_serdes_irq_enable(struct mv88e6xxx_chip *chip, int port, u8 lane,
bool enable);
+irqreturn_t mv88e6097_serdes_irq_status(struct mv88e6xxx_chip *chip, int port,
+ u8 lane);
irqreturn_t mv88e6352_serdes_irq_status(struct mv88e6xxx_chip *chip, int port,
u8 lane);
irqreturn_t mv88e6390_serdes_irq_status(struct mv88e6xxx_chip *chip, int port,
--
2.29.2
[net-next PATCH v5 0/4] net: dsa: mv88e6xxx: serdes link without phy
This small series gets my hardware into a working state. The key points are to make sure we don't force the link and that we ask the MAC for the link status. I also have updated my dts to say `phy-mode = "1000base-x";` and `managed = "in-band-status";` I've dropped the patch for the 88E6123 as it's a distraction and I lack hardware to do any proper testing with it. Earlier versions are on the mailing list if anyone wants to pick it up in the future. I notice there's a series for mv88e6393x circulating on the netdev mailing list. As patch #1 is adding a new device specific op either this series will need updating to cover the mv88e6393x or the mv88e6393x series will need updating for the new op depenting on which lands first. Chris Packham (4): net: dsa: mv88e6xxx: Don't force link when using in-band-status net: dsa: mv88e6xxx: Support serdes ports on MV88E6097/6095/6185 net: dsa: mv88e6xxx: Add serdes interrupt support for MV88E6097 net: dsa: mv88e6xxx: Handle error in serdes_get_regs drivers/net/dsa/mv88e6xxx/chip.c | 47 ++- drivers/net/dsa/mv88e6xxx/chip.h | 4 + drivers/net/dsa/mv88e6xxx/port.c | 36 + drivers/net/dsa/mv88e6xxx/port.h | 3 + drivers/net/dsa/mv88e6xxx/serdes.c | 123 +++-- drivers/net/dsa/mv88e6xxx/serdes.h | 9 +++ 6 files changed, 213 insertions(+), 9 deletions(-) -- 2.29.2
Re: [PATCH] ARM: dts: mvebu: Add device tree for RD-AC3X-48G4X2XL board
Hi Aryan,
On 23/11/20 4:52 pm, Aryan Srivastava wrote:
> Add device tree for RD-AC3X-48G4X2XL board. This has a Armada 382 SoC on
> a interposer board connected to a baseboard with a Prestera AC3X ASIC
> connected via PCI.
>
> Signed-off-by: Aryan Srivastava
Reviewed-by: Chris Packham
> ---
> arch/arm/boot/dts/Makefile| 1 +
> .../boot/dts/armada-382-rd-ac3x-48g4x2xl.dts | 114 ++
> 2 files changed, 115 insertions(+)
> create mode 100644 arch/arm/boot/dts/armada-382-rd-ac3x-48g4x2xl.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index ce66ffd5a1bb..a60407ad7347 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1319,6 +1319,7 @@ dtb-$(CONFIG_MACH_ARMADA_370) += \
> dtb-$(CONFIG_MACH_ARMADA_375) += \
> armada-375-db.dtb
> dtb-$(CONFIG_MACH_ARMADA_38X) += \
> + armada-382-rd-ac3x-48g4x2xl.dtb \
> armada-385-clearfog-gtr-s4.dtb \
> armada-385-clearfog-gtr-l8.dtb \
> armada-385-db-88f6820-amc.dtb \
> diff --git a/arch/arm/boot/dts/armada-382-rd-ac3x-48g4x2xl.dts
> b/arch/arm/boot/dts/armada-382-rd-ac3x-48g4x2xl.dts
> new file mode 100644
> index ..b08d662a8519
> --- /dev/null
> +++ b/arch/arm/boot/dts/armada-382-rd-ac3x-48g4x2xl.dts
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Device Tree file for Marvell Armada 382 reference board
> + * (RD-AC3X-48G4X2XL)
> + *
> + * Copyright (C) 2020 Allied Telesis Labs
> + */
> +
> +/dts-v1/;
> +#include "armada-385.dtsi"
> +
> +#include
> +
> +/ {
> + model = "Marvell Armada 382 RD-AC3X";
> + compatible = "marvell,rd-ac3x-48g4x2xl", "marvell,rd-ac3x",
> + "marvell,armada385", "marvell,armada380";
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + aliases {
> + ethernet0 =
> + };
> +
> + memory {
> + device_type = "memory";
> + reg = <0x 0x2000>; /* 512MB */
> + };
> +
> + soc {
> + ranges = + MBUS_ID(0x01, 0x1d) 0 0xfff0 0x10>;
> + };
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins>;
> + status = "okay";
> +
> + eeprom@53{
> + compatible = "atmel,24c64";
> + reg = <0x53>;
> + };
> +// cpld@3c{
> +// compatible = "marvell,ac3x-cpld";
> +// reg = <0x3c>;
> +// };
> +};
Question for the mvebu maintainers: We know there is a cpld with that
can be interfaced with over i2c. Other than detecting that it shows up
on the i2c bus we haven't done anything with it. We believe it
interfaces with some discrete IO and might mux the i2c interface towards
the SFP/QSFP cages.
Obviously there isn't a "marvell,ac3x-cpld" driver (yet) so I suggested
to Aryan that we put it in the dts to document its existence but leave
it commented out for now. Is this OK?
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins>;
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> + phy = <>;
> + phy-mode = "rgmii-id";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins>;
> +
> + phy0: ethernet-phy@0 {
> + reg = <0>;
> + };
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + /* Port 0, Lane 0 */
> + status = "okay";
> +};
> +
> +_controller {
> + status = "okay";
> +
> + nand@0 {
> + reg = <0>;
> + label = "pxa3xx_nand-0";
> + nand-rb = <0>;
> + nand-on-flash-bbt;
> +
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + partition@0 {
> + reg = <0x 0x0050>;
> + label = "u-boot";
> + };
> + partition@50{
> + reg = <0x0050 0x0040>;
> + label = "u-boot env";
> + };
> + partition@90{
> + reg = <0x0090 0x3F70>;
> + label = "user";
> + };
> + };
> + };
> +};
> +
> + {
> + clock-frequency = <2>;
> +};
Re: [PATCH v5 2/2] spi: Add generic SPI multiplexer
On 14/11/20 4:46 am, Nicolas Saenz Julienne wrote:
> Upon registering spi-mux's devices through spi_add_device() the kernel gets
> stuck waiting for the 'spi_add_lock' mutex to be released. The mutex happens
> to
> be held by spi-mux's parent SPI bus, which unluckily, is waiting for spi-mux's
> probe to finish before releasing it.
I just re-tested my system with v5.10.0-rc4 and didn't see any problem.
My dts is pretty similar to yours the only obvious thing missing is
`mux-control-names = "spi";` and I also set `#size-cells = <1>;` (let me
know if you want me to post the whole thing).
It might be dependent on the host spi controller. The re-test I just did
was on a board using the spi-orion.c driver and I tested my original
change on a board using spi-bcm-qspi.c (I haven't got the board handy
right now but I could go and find one if necessary).
> I might aswell be doing something wrong. But so far I trust my DT
> implementation:
>
>{
> status = "okay";
> pinctrl-names = "default";
> pinctrl-0 = <_gpio7>;
>
> spi@0 {
> compatible = "spi-mux";
> reg = <0>;
> #address-cells = <1>;
> #size-cells = <0>;
> spi-max-frequency = <1>;
>
> mux-controls = <_mux>;
>
> w5500@0 {
> compatible = "wiznet,w5500";
> reg = <0>;
> pinctrl-names = "default";
> pinctrl-0 = <_pins>;
> interrupt-parent = <>;
> interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
> spi-max-frequency = <3000>;
> };
>
> spi-flash@1 {
> compatible = "jedec,spi-nor";
> reg = <1>;
> #address-cells = <1>;
> #size-cells = <0>;
> spi-max-frequency = <1600>;
> };
> };
> };
>
> Regards,
> Nicolas
[PATCH] bus: mvebu-mbus: make iounmap() symmetric with ioremap()
make coccicheck complains:
./drivers/bus/mvebu-mbus.c:1113:2-8: ERROR: missing iounmap; ioremap on line
1106 and execution via conditional on line
It took some staring but I don't think there is a problem because the
file global `mbus_state` is passed mvebu_mbus_common_init() as the
`mbus` parameter so `mbus_state.mbuswins_base` and `mbus->mbuswins_base`
are the same thing. But this is confusing for anyone reading the code
and one less complaint from coccicheck would be nice so lets fix it.
Signed-off-by: Chris Packham
---
I haven't included
Fixes: fb52a6c4 ("bus: introduce an Marvell EBU MBus driver")
because that commit is quite old and it's probably not worth bothering
the stable trees with what is essentially a no-op.
drivers/bus/mvebu-mbus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 2519ceede64b..dd9e7343a5e3 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -,7 +,7 @@ static int __init mvebu_mbus_common_init(struct
mvebu_mbus_state *mbus,
mbus->sdramwins_base = ioremap(sdramwins_phys_base, sdramwins_size);
if (!mbus->sdramwins_base) {
- iounmap(mbus_state.mbuswins_base);
+ iounmap(mbus->mbuswins_base);
return -ENOMEM;
}
--
2.29.2
Re: [PATCH 2/2] hwmon: (adt7470) Convert to regmap
On 24/10/20 10:40 am, Guenter Roeck wrote:
> On Tue, Oct 20, 2020 at 11:34:23AM +1300, Chris Packham wrote:
>> Convert the adt7470 to using regmap and add error handling.
>>
>> Signed-off-by: Chris Packham
>> ---
>> drivers/hwmon/adt7470.c | 388 ++--
>> 1 file changed, 250 insertions(+), 138 deletions(-)
>>
>> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
>> index 2e8feacccf84..d20aeb3a2fdc 100644
>> --- a/drivers/hwmon/adt7470.c
>> +++ b/drivers/hwmon/adt7470.c
>> @@ -18,6 +18,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -137,7 +138,7 @@ static const unsigned short normal_i2c[] = { 0x2C, 0x2E,
>> 0x2F, I2C_CLIENT_END };
>> #define ADT7470_FREQ_SHIFT 4
>>
>> struct adt7470_data {
>> -struct i2c_client *client;
>> +struct regmap *regmap;
>> struct mutexlock;
>> charsensors_valid;
>> charlimits_valid;
>> @@ -171,51 +172,73 @@ struct adt7470_data {
>>* 16-bit registers on the ADT7470 are low-byte first. The data sheet says
>>* that the low byte must be read before the high byte.
>>*/
>> -static inline int adt7470_read_word_data(struct i2c_client *client, u8 reg)
>> +static inline int adt7470_read_word_data(struct adt7470_data *data, u8 reg)
>> {
>> -u16 foo;
>> -foo = i2c_smbus_read_byte_data(client, reg);
>> -foo |= ((u16)i2c_smbus_read_byte_data(client, reg + 1) << 8);
>> -return foo;
>> +u8 val[2];
>> +int err;
>> +
>> +err = regmap_bulk_read(data->regmap, reg, , 2);
>> +if (err < 0)
>> +return err;
>> +
>> +return val[0] | (val[1] << 8);
>> }
>>
>> -static inline int adt7470_write_word_data(struct i2c_client *client, u8 reg,
>> +static inline int adt7470_write_word_data(struct adt7470_data *data, u8 reg,
>>u16 value)
>> {
>> -return i2c_smbus_write_byte_data(client, reg, value & 0xFF)
>> - || i2c_smbus_write_byte_data(client, reg + 1, value >> 8);
>> +u8 val[2];
>> +
>> +val[0] = value & 0xFF;
>> +val[1] = value >> 8;
>> +
>> +return regmap_bulk_write(data->regmap, reg, , 2);
>> }
>>
>> /* Probe for temperature sensors. Assumes lock is held */
>> -static int adt7470_read_temperatures(struct i2c_client *client,
>> - struct adt7470_data *data)
>> +static int adt7470_read_temperatures(struct adt7470_data *data)
>> {
>> unsigned long res;
>> +int err;
>> int i;
>> u8 cfg, pwm[4], pwm_cfg[2];
>>
>> /* save pwm[1-4] config register */
>> -pwm_cfg[0] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM_CFG(0));
>> -pwm_cfg[1] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM_CFG(2));
>> +err = regmap_raw_read(data->regmap, ADT7470_REG_PWM_CFG(0),
>> _cfg[0], 1);
>> +if (err < 0)
>> +return err;
>> +err = regmap_raw_read(data->regmap, ADT7470_REG_PWM_CFG(2),
>> _cfg[1], 1);
>> +if (err < 0)
>> +return err;
>>
>> /* set manual pwm to whatever it is set to now */
>> -for (i = 0; i < ADT7470_FAN_COUNT; i++)
>> -pwm[i] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM(i));
>> +err = regmap_bulk_read(data->regmap, ADT7470_REG_PWM(0), [0],
>> ADT7470_FAN_COUNT);
>> +if (err < 0)
>> +return err;
>>
>> /* put pwm in manual mode */
>> -i2c_smbus_write_byte_data(client, ADT7470_REG_PWM_CFG(0),
>> -pwm_cfg[0] & ~(ADT7470_PWM_AUTO_MASK));
>> -i2c_smbus_write_byte_data(client, ADT7470_REG_PWM_CFG(2),
>> -pwm_cfg[1] & ~(ADT7470_PWM_AUTO_MASK));
>> +pwm_cfg[0] &= ~ADT7470_PWM_AUTO_MASK;
>> +err = regmap_raw_write(data->regmap, ADT7470_REG_PWM_CFG(0),
>> _cfg[0], 1);
>> +if (err < 0)
>> +return err;
>> +pwm_cfg[1] &= ~ADT7470_PWM_AUTO_MASK;
>> +err = regmap_raw_write(data->regmap, ADT7470_REG_PWM_CFG(2),
>> _cfg[1], 1);
>> +if (err < 0)
>> +return err;
>>
> Any special reason for not using regmap_update_bits() ? Also, why
> regmap_raw_read() and regmap_raw_write() instead of ju
