Re: [PATCH v3 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Yixun Lan <dlan@gentoo.org>]

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@gmail.com>
> >>
> >> 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@gmail.com>
> >> ---
> >>  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@mail.gmail.com/
> 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