Message-ID: <ad7ac580-3593-4b44-b6c9-5c43b80b135e@kernel.org>
Date: Mon, 16 Sep 2024 09:13:14 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: arturs.artamonovs@...log.com, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>, Greg Malysa <greg.malysa@...esys.com>,
 Philipp Zabel <p.zabel@...gutronix.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Utsav Agarwal <Utsav.Agarwal@...log.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>, Thomas Gleixner <tglx@...utronix.de>,
 Andi Shyti <andi.shyti@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Jiri Slaby <jirislaby@...nel.org>, Arnd Bergmann <arnd@...db.de>,
 Olof Johansson <olof@...om.net>, soc@...nel.org
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
 linux-serial@...r.kernel.org, adsp-linux@...log.com,
 Nathan Barrett-Morrison <nathan.morrison@...esys.com>
Subject: Re: [PATCH 15/21] i2c: Add driver for ADI ADSP-SC5xx platforms

On 12/09/2024 20:25, Arturs Artamonovs via B4 Relay wrote:
> From: Arturs Artamonovs <arturs.artamonovs@...log.com>
> 
> Add support for I2C on SC5xx
> 
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@...log.com>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@...esys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@...esys.com>
> Co-developed-by: Greg Malysa <greg.malysa@...esys.com>
> Signed-off-by: Greg Malysa <greg.malysa@...esys.com>

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

Powered by blists - more mailing lists