Hi Troy:
On 12:11 Thu 21 Nov , Troy Mitchell wrote:
> Hi, Yixun. thanks for ur review.
>
> On 2024/11/14 14:49, Yixun Lan wrote:
> > On 11:07 Tue 12 Nov , Troy Mitchell wrote:
> >> From: Troy Mitchell <troymitchell988@xxxxxxxxx>
> >>
> >> This patch introduces basic I2C support for the SpacemiT K1 SoC,
> >> utilizing interrupts for transfers.
> >>
> >> The driver has been tested using i2c-tools on a Bananapi-F3 board,
> >> and basic I2C read/write operations have been confirmed to work.
> >>
> >> Signed-off-by: Troy Mitchell <TroyMitchell988@xxxxxxxxx>
> >> ---
> >> drivers/i2c/busses/Kconfig | 19 ++
> >> drivers/i2c/busses/Makefile | 1 +
> >> drivers/i2c/busses/i2c-k1.c | 656 ++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 676 insertions(+)
> >>
...
> >> +
> >> +static int spacemit_i2c_probe(struct platform_device *pdev)
> >> +{
> >> + struct spacemit_i2c_dev *i2c;
> >> + struct device_node *of_node = pdev->dev.of_node;
> >> + struct clk *clk;
> >> + int ret = 0;
> >> +
> >> + i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> >> + if (!i2c)
> >> + return -ENOMEM;
> >> +
> >> + i2c->dev = &pdev->dev;
> >> +
> >> + i2c->base = devm_platform_ioremap_resource(pdev, 0);
> >> + if (IS_ERR(i2c->base))
> >> + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->base), "failed to do ioremap");
> >> +
> >> + i2c->irq = platform_get_irq(pdev, 0);
> >> + if (i2c->irq < 0)
> >> + return dev_err_probe(&pdev->dev, i2c->irq, "failed to get irq resource");
> >> +
> >> + ret = devm_request_irq(i2c->dev, i2c->irq, spacemit_i2c_irq_handler,
> >> + IRQF_NO_SUSPEND | IRQF_ONESHOT, dev_name(i2c->dev), i2c);
> >> + if (ret)
> >> + return dev_err_probe(&pdev->dev, ret, "failed to request irq");
> >> +
> >> + disable_irq(i2c->irq);
> >> +
> >> + clk = devm_clk_get_enabled(&pdev->dev, NULL);
> >> + if (IS_ERR(clk))
> >> + return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to enable clock");
> >> +
> > I'd suggest also to handle pin request here, since pinctrl driver is merged
> >
> > https://lore.kernel.org/all/CACRpkdYnaJsKKfcdhHeMGTTp86M+wNODzZx2e=OYbxQ4Jc4Rjw@xxxxxxxxxxxxxx/
> sry, I don't understand what pin I need to request?
> >
I was suggesting to add devm_pinctrl_get(), but after checking,
this is not needed anymore since it's already handled by device core,
so no need to change here..
check drivers/core/pinctrl.c -> pinctrl_bind_pins()
> >> + i2c_set_adapdata(&i2c->adapt, i2c);
> >> + i2c->adapt.owner = THIS_MODULE;
> >> + i2c->adapt.algo = &spacemit_i2c_algo;
> >> + i2c->adapt.dev.parent = i2c->dev;
> >> + i2c->adapt.nr = pdev->id;
> >> +
> >> + i2c->adapt.dev.of_node = of_node;
> >> + i2c->adapt.algo_data = i2c;
> >> +
> >> + strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name));
> >> +
> >> + init_completion(&i2c->complete);
> >> +
> >> + ret = i2c_add_numbered_adapter(&i2c->adapt);
> >> + if (ret)
> >> + return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
> >> +
> >> + platform_set_drvdata(pdev, i2c);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void spacemit_i2c_remove(struct platform_device *pdev)
> >> +{
> >> + struct spacemit_i2c_dev *i2c = platform_get_drvdata(pdev);
> >> +
> >> + i2c_del_adapter(&i2c->adapt);
> >> +}
> >> +
> >> +static const struct of_device_id spacemit_i2c_of_match[] = {
> >> + { .compatible = "spacemit,k1-i2c", },
> >> + { /* sentinel */ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, spacemit_i2c_of_match);
> >> +
> >> +static struct platform_driver spacemit_i2c_driver = {
> >> + .probe = spacemit_i2c_probe,
> >> + .remove = spacemit_i2c_remove,
> >> + .driver = {
> >> + .name = "i2c-k1",
> >> + .of_match_table = spacemit_i2c_of_match,
> >> + },
> >> +};
> >> +module_platform_driver(spacemit_i2c_driver);
> >> +
> >> +MODULE_LICENSE("GPL");
> >> +MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC");
> >>
> >> --
> >> 2.34.1
> >>
> >
>
> --
> Troy Mitchell
--
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55
