On 12/09/2024 20:25, Arturs Artamonovs via B4 Relay wrote:
> From: Arturs Artamonovs <arturs.artamonovs@xxxxxxxxxx>
>
> Add support for I2C on SC5xx
>
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@xxxxxxxxxx>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@xxxxxxxxxxx>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@xxxxxxxxxxx>
> Co-developed-by: Greg Malysa <greg.malysa@xxxxxxxxxxx>
> Signed-off-by: Greg Malysa <greg.malysa@xxxxxxxxxxx>
As in all patches - chain looks wrong.
> ---
> drivers/i2c/busses/Kconfig | 17 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-adi-twi.c | 940 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 958 insertions(+)
> +static SIMPLE_DEV_PM_OPS(i2c_adi_twi_pm,
> + i2c_adi_twi_suspend, i2c_adi_twi_resume);
> +#define I2C_ADI_TWI_PM_OPS (&i2c_adi_twi_pm)
> +#else
> +#define I2C_ADI_TWI_PM_OPS NULL
> +#endif
> +
> +#ifdef CONFIG_OF
Drop
> +static const struct of_device_id adi_twi_of_match[] = {
> + {
> + .compatible = "adi,twi",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, adi_twi_of_match);
> +#endif
> +
> +static int i2c_adi_twi_probe(struct platform_device *pdev)
> +{
> + struct adi_twi_iface *iface;
> + struct i2c_adapter *p_adap;
> + struct resource *res;
> + const struct of_device_id *match;
> + struct device_node *node = pdev->dev.of_node;
> + int rc;
> + unsigned int clkhilow;
> + u16 writeValue;
> +
> + iface = devm_kzalloc(&pdev->dev, sizeof(*iface), GFP_KERNEL);
> + if (!iface)
> + return -ENOMEM;
> +
> + spin_lock_init(&(iface->lock));
> +
> + match = of_match_device(of_match_ptr(adi_twi_of_match), &pdev->dev);
Drop of_mathc_ptr
> + if (match) {
> + if (of_property_read_u32(node, "clock-khz",
Uh? I really do not get what is this.
> + &iface->twi_clk))
Really odd alignment.
> + iface->twi_clk = 50;
> + } else
> + iface->twi_clk = CONFIG_I2C_ADI_TWI_CLK_KHZ;
> +
> + iface->sclk = devm_clk_get(&pdev->dev, "sclk0");
> + if (IS_ERR(iface->sclk)) {
> + if (PTR_ERR(iface->sclk) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Missing i2c clock\n");
Eh... there is nowhere such code. Please work with upstream code, not
downstream. When writing drivers take UPSTREAM driver as template.
Whatever you have in downstream is not a good to send to us.
Syntax is return dev_err_probe.
> + return PTR_ERR(iface->sclk);
> + }
> +
> + /* Find and map our resources */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "Cannot get IORESOURCE_MEM\n");
> + return -ENOENT;
> + }
> +
> + iface->regs_base = devm_ioremap_resource(&pdev->dev, res);
Combine these two calls with proper helper.
> + if (IS_ERR(iface->regs_base)) {
> + dev_err(&pdev->dev, "Cannot map IO\n");
> + return PTR_ERR(iface->regs_base);
> + }
> +
> + iface->irq = platform_get_irq(pdev, 0);
> + if (iface->irq < 0) {
Here you have correct, other patch has a bug. That makes me wonder about
consistency of this code. There are several other hints that people
wrote it with quite different coding style.
> + dev_err(&pdev->dev, "No IRQ specified\n");
> + return -ENOENT;
No. return the error. Anyway, that's never a correct errno. Read
description of this errno: no such file. This is not a file you are
getting here.
This comment applies to all your code.
> + }
> +
> + p_adap = &iface->adap;
> + p_adap->nr = pdev->id;
> + strscpy(p_adap->name, pdev->name, sizeof(p_adap->name));
> + p_adap->algo = &adi_twi_algorithm;
> + p_adap->algo_data = iface;
> + p_adap->class = I2C_CLASS_DEPRECATED;
> + p_adap->dev.parent = &pdev->dev;
> + p_adap->dev.of_node = node;
> + p_adap->timeout = 5 * HZ;
> + p_adap->retries = 3;
> +
> + rc = devm_request_irq(&pdev->dev, iface->irq, adi_twi_interrupt_entry,
> + 0, pdev->name, iface);
> + if (rc) {
> + dev_err(&pdev->dev, "Can't get IRQ %d !\n", iface->irq);
> + rc = -ENODEV;
???
Sorry, this driver is in really poor shape.
> + goto out_error;
> + }
> +
> + /* Set TWI internal clock as 10MHz */
> + clk_prepare_enable(iface->sclk);
> + if (rc) {
> + dev_err(&pdev->dev, "Could not enable sclk\n");
> + goto out_error;
return
> + }
> +
> + writeValue = ((clk_get_rate(iface->sclk) / 1000 / 1000 + 5) / 10) & 0x7F;
No camelCase. Please follow Linux coding style.
> + iowrite16(writeValue, &iface->regs_base->control);
> +
> + /*
> + * We will not end up with a CLKDIV=0 because no one will specify
> + * 20kHz SCL or less in Kconfig now. (5 * 1000 / 20 = 250)
> + */
> + clkhilow = ((10 * 1000 / iface->twi_clk) + 1) / 2;
> +
> + /* Set Twi interface clock as specified */
> + writeValue = (clkhilow << 8) | clkhilow;
> + iowrite16(writeValue, &iface->regs_base->clkdiv);
> +
> + /* Enable TWI */
> + writeValue = ioread16(&iface->regs_base->control) | TWI_ENA;
> + iowrite16(writeValue, &iface->regs_base->control);
> +
> + rc = i2c_add_numbered_adapter(p_adap);
> + if (rc < 0)
> + goto disable_clk;
> +
> + platform_set_drvdata(pdev, iface);
> +
> + dev_info(&pdev->dev, "ADI on-chip I2C TWI Controller, regs_base@%p\n",
> + iface->regs_base);
Drop. Driver should be silent on success.
> +
> + return 0;
> +
> +disable_clk:
> + clk_disable_unprepare(iface->sclk);
devm_clk_get_enabled
> +
> +out_error:
Drop
> + return rc;
> +}
> +
> +static void i2c_adi_twi_remove(struct platform_device *pdev)
> +{
> + struct adi_twi_iface *iface = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(iface->sclk);
> + i2c_del_adapter(&(iface->adap));
> +}
> +
> +static struct platform_driver i2c_adi_twi_driver = {
> + .probe = i2c_adi_twi_probe,
> + .remove = i2c_adi_twi_remove,
> + .driver = {
> + .name = "i2c-adi-twi",
> + .pm = I2C_ADI_TWI_PM_OPS,
> + .of_match_table = of_match_ptr(adi_twi_of_match),
Drop of_match_ptr. None of your other code has it, right? This should
make you wonder.
> + },
> +};
> +
> +static int __init i2c_adi_twi_init(void)
> +{
> + return platform_driver_register(&i2c_adi_twi_driver);
> +}
> +
> +static void __exit i2c_adi_twi_exit(void)
> +{
> + platform_driver_unregister(&i2c_adi_twi_driver);
> +}
> +
> +subsys_initcall(i2c_adi_twi_init);
No, i2c driver can be just module platform driver.
> +module_exit(i2c_adi_twi_exit);
> +
> +MODULE_AUTHOR("Bryan Wu, Sonic Zhang");
> +MODULE_DESCRIPTION("ADI on-chip I2C TWI Controller Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:i2c-adi-twi");
You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.
> \ No newline at end of file
>
Best regards,
Krzysztof
