On 2025/3/18 05:42, Alex Elder wrote: > On 3/16/25 2:43 AM, Troy Mitchell wrote: >> 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. >> >> Reviewed-by: Alex Elder <elder@xxxxxxxxxxxx> >> Link: >> https://lore.kernel.org/all/20250128-k1-maintainer-1-v1-1-e5dec4f379eb@xxxxxxxxxx [1] >> Signed-off-by: Troy Mitchell <troymitchell988@xxxxxxxxx> > > I know I said it was fine, but I'm going to reiterate two comments in > the probe function. Hi, Alex. Thanks for your review! > >> --- >> drivers/i2c/busses/Kconfig | 17 ++ >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-k1.c | 605 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 623 insertions(+) >> > > . . . > >> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c >> new file mode 100644 >> index >> 0000000000000000000000000000000000000000..ae43dcd31e8aa480766b44be91656657c7aaaf4a >> --- /dev/null >> +++ b/drivers/i2c/busses/i2c-k1.c >> @@ -0,0 +1,605 @@ > > . . . > >> +static int spacemit_i2c_probe(struct platform_device *pdev) >> +{ >> + struct clk *clk; >> + struct device *dev = &pdev->dev; >> + struct device_node *of_node = pdev->dev.of_node; >> + struct spacemit_i2c_dev *i2c; >> + int ret; >> + >> + i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL); >> + if (!i2c) >> + return -ENOMEM; >> + >> + ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq); >> + if (ret) >> + dev_warn(dev, "failed to read clock-frequency property\n"); > > If the property doesn't exist, I don't think this warrants a warning, > because it's optional. Perhaps if a different error (something other > than -EINVAL) is returned it would warrant a warning. > >> + >> + /* For now, this driver doesn't support high-speed. */ >> + if (!i2c->clock_freq || i2c->clock_freq < 1 || > > For an unsigned value, !i2c->clock_freq is *the same as* > i2c->clock_freq < 1. Get rid of the latter. > > I'll leave it up to the maintainer to decide whether these > comments can just be ignored--my Reviewed-by is fine, even > if you don't change these. > > -Alex I know it's right what you said. But I don't know if it's worth to send v8? Maybe I can fix it when I add FIFO function? If I'm wrong, let me know. > >> + i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) { >> + dev_warn(dev, "unsupported clock frequency %u; using %u\n", >> + i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ); >> + i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ; >> + } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) { >> + dev_warn(dev, "unsupported clock frequency %u; using %u\n", >> + i2c->clock_freq, SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ); >> + i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ; >> + } >> + >> + i2c->dev = &pdev->dev; >> + >> + i2c->base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(i2c->base)) >> + return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap"); >> + >> + i2c->irq = platform_get_irq(pdev, 0); >> + if (i2c->irq < 0) >> + return dev_err_probe(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(dev, ret, "failed to request irq"); >> + >> + clk = devm_clk_get_enabled(dev, "func"); >> + if (IS_ERR(clk)) >> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock"); >> + >> + clk = devm_clk_get_enabled(dev, "bus"); >> + if (IS_ERR(clk)) >> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock"); >> + >> + spacemit_i2c_reset(i2c); >> + >> + 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; >> + >> + strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name)); >> + >> + init_completion(&i2c->complete); >> + >> + platform_set_drvdata(pdev, i2c); >> + >> + ret = i2c_add_numbered_adapter(&i2c->adapt); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter"); >> + >> + 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"); >> > -- Troy Mitchell
