Messages in this thread | | | Date | Fri, 18 Mar 2022 16:15:26 +0000 | Subject | Re: [PATCH] i2c: add support for microchip fpga i2c controllers | From | Conor Dooley <> |
| |
On 18/03/2022 14:56, Ben Dooks wrote: > On 15/03/2022 15:32, Conor Dooley wrote: >> Add Microchip CoreI2C i2c controller support. This driver supports the >> "hard" i2c controller on the Microchip PolarFire SoC & the basic feature >> set for "soft" i2c controller implemtations in the FPGA fabric. >> >> Co-developed-by: Daire McNamara <daire.mcnamara@microchip.com> >> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com> >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> drivers/i2c/busses/Kconfig | 11 + >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-microchip-core.c | 487 ++++++++++++++++++++++++ >> 3 files changed, 499 insertions(+) >> create mode 100644 drivers/i2c/busses/i2c-microchip-core.c >> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index a1bae59208e3..3d4d8e0e9de7 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -781,6 +781,17 @@ config I2C_MESON >> If you say yes to this option, support will be included for the >> I2C interface on the Amlogic Meson family of SoCs. > > snip > >> + >> +static void mchp_corei2c_core_disable(struct mchp_corei2c_dev *idev) >> +{ >> + u8 ctrl = readl(idev->base + CORE_I2C_CTRL); >> + >> + ctrl &= ~CTRL_ENS1; >> + writel(ctrl, idev->base + CORE_I2C_CTRL); >> +} >> + >> +static void mchp_corei2c_core_enable(struct mchp_corei2c_dev *idev) >> +{ >> + u8 ctrl = readl(idev->base + CORE_I2C_CTRL); >> + >> + ctrl |= CTRL_ENS1; >> + writel(ctrl, idev->base + CORE_I2C_CTRL); >> +} > > Not sure why you would use readl/writel with an u8, surely an > readb/writeb be better?
Sure, can try drop it to b.
> > >> +static irqreturn_t mchp_corei2c_handle_isr(struct mchp_corei2c_dev >> *idev) >> +{ >> + u32 status = idev->isr_status; >> + u8 ctrl; >> + >> + if (!idev->buf) { >> + dev_warn(idev->dev, "unexpected interrupt\n"); >> + return IRQ_HANDLED; >> + } > > is IRQ_HANDLED correct here?
Hmm, the dev_warn is probably incorrect too. For the mpfs, the interrupt line isn't shared, but the generic fabric core could be. Ill change this.
> >> + >> +static int mchp_corei2c_probe(struct platform_device *pdev) >> +{ >> + struct mchp_corei2c_dev *idev = NULL; >> + struct resource *res; >> + int irq, ret; >> + u32 val; >> + >> + idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL); >> + if (!idev) >> + return -ENOMEM; >> + >> + idev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); >> + if (IS_ERR(idev->base)) >> + return PTR_ERR(idev->base); >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) >> + return dev_err_probe(&pdev->dev, irq, >> + "missing interrupt resource\n"); >> + >> + idev->i2c_clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(idev->i2c_clk)) >> + return dev_err_probe(&pdev->dev, PTR_ERR(idev->i2c_clk), >> + "missing clock\n"); >> + >> + idev->dev = &pdev->dev; >> + init_completion(&idev->msg_complete); >> + spin_lock_init(&idev->lock); >> + >> + val = device_property_read_u32(idev->dev, "clock-frequency", >> + &idev->bus_clk_rate); >> + if (val) { >> + dev_info(&pdev->dev, "default to 100kHz\n"); >> + idev->bus_clk_rate = 100000; >> + } >> + >> + if (idev->bus_clk_rate > 400000) >> + return dev_err_probe(&pdev->dev, -EINVAL, >> + "clock-frequency too high: %d\n", >> + idev->bus_clk_rate); >> + >> + ret = devm_request_irq(&pdev->dev, irq, mchp_corei2c_isr, >> IRQF_SHARED, >> + pdev->name, idev); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, >> + "failed to claim irq %d\n", irq); >> + >> + ret = clk_prepare_enable(idev->i2c_clk); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, >> + "failed to enable clock\n"); >> + >> + ret = mchp_corei2c_init(idev); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, >> + "failed to program clock divider\n"); > > going to leak a prepared clock from here on?
Aye, good spot. Thanks.
>> + i2c_set_adapdata(&idev->adapter, idev); >> + snprintf(idev->adapter.name, sizeof(idev->adapter.name), >> + "Microchip I2C hw bus"); >> + idev->adapter.owner = THIS_MODULE; >> + idev->adapter.algo = &mchp_corei2c_algo; >> + idev->adapter.dev.parent = &pdev->dev; >> + idev->adapter.dev.of_node = pdev->dev.of_node; >> + >> + platform_set_drvdata(pdev, idev); >> + >> + ret = i2c_add_adapter(&idev->adapter); >> + if (ret) { >> + clk_disable_unprepare(idev->i2c_clk); >> + return ret; >> + } >> + >> + dev_info(&pdev->dev, "Microchip I2C Probe Complete\n"); > > not sure if necessary, doesn't the i2c core also announce?
The only thing I see on boot is this, but I'll take a look and see if I can do this via the core.
> >> + return 0; >> +} >> + >> +static int mchp_corei2c_remove(struct platform_device *pdev) >> +{ >> + struct mchp_corei2c_dev *idev = platform_get_drvdata(pdev); >> + >> + clk_disable_unprepare(idev->i2c_clk); >> + i2c_del_adapter(&idev->adapter); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id mchp_corei2c_of_match[] = { >> + { .compatible = "microchip,mpfs-i2c" }, >> + { .compatible = "microchip,corei2c-rtl-v7" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, mchp_corei2c_of_match); >> + >> +static struct platform_driver mchp_corei2c_driver = { >> + .probe = mchp_corei2c_probe, >> + .remove = mchp_corei2c_remove, >> + .driver = { >> + .name = "microchip-corei2c", >> + .of_match_table = mchp_corei2c_of_match, >> + }, >> +}; >> + >> +module_platform_driver(mchp_corei2c_driver); >> + >> +MODULE_DESCRIPTION("Microchip CoreI2C bus driver"); >> +MODULE_AUTHOR("Daire McNamara <daire.mcnamara@microchip.com>"); >> +MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>"); >> +MODULE_LICENSE("GPL v2"); > >
| |