| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c7dc26a0-7cbc-4909-b2ac-582d108fc5e7@gmail.com>
Date: Tue, 18 Mar 2025 13:44:06 +0800
From: Troy Mitchell <troymitchell988@...il.com>
To: Alex Elder <elder@...e.org>, Andi Shyti <andi.shyti@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Yixun Lan <dlan@...too.org>
Cc: troymitchell988@...il.com, linux-riscv@...ts.infradead.org,
linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, spacemit@...ts.linux.dev,
Alex Elder <elder@...cstar.com>
Subject: Re: [PATCH v7 2/2] i2c: spacemit: add support for SpacemiT K1 SoC
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@...cstar.com>
>> Link:
>> https://lore.kernel.org/all/20250128-k1-maintainer-1-v1-1-e5dec4f379eb@gentoo.org [1]
>> Signed-off-by: Troy Mitchell <troymitchell988@...il.com>
>
> 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
Powered by blists - more mailing lists