Message-ID: <eb4112aa-21d0-4537-a18c-940d8832711a@kernel.org>
Date: Tue, 15 Oct 2024 10:08:30 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Troy Mitchell <troymitchell988@...il.com>, andi.shyti@...nel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Cc: linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

On 15/10/2024 09:51, 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.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@...il.com>
> ---
>  drivers/i2c/busses/Kconfig  |  18 +
>  drivers/i2c/busses/Makefile |   1 +
>  drivers/i2c/busses/i2c-k1.c | 694 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 713 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-k1.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 53f18b351f53..cfa5cda9c8d8 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -800,6 +800,24 @@ config I2C_KEMPLD
>  	  This driver can also be built as a module. If so, the module
>  	  will be called i2c-kempld.
>  
> +config I2C_K1
> +	tristate "Spacemit K1 I2C adapter"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST

There is no such thing as ARCH_SPACEMIT. You never mentioned any dependency.


> +	help
> +	  This option enables support for the I2C interface on the Spacemit K1
> +	  platform.
> +
> +	  If you enable this configuration, the kernel will include support for
> +	  the I2C adapter specific to the Spacemit K1 platform. This driver ca
> +	  be used to manage I2C bus transactions, which are necessary for
> +	  interfacing with I2C peripherals such as sensors, EEPROMs, and other
> +	  devices.
> +
> +	  This driver can also be compiled as a module. If you choose to build
> +	  it as a module, the resulting kernel module will be named `i2c-k1`.
> +	  Loading this module will enable the I2C functionality for the K1
> +	  platform dynamically, without requiring a rebuild of the kernel.
> +
>  config I2C_LPC2K
>  	tristate "I2C bus support for NXP LPC2K/LPC178x/18xx/43xx"
>  	depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index ecc07c50f2a0..682e10435a7f 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -78,6 +78,7 @@ obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
>  obj-$(CONFIG_I2C_JZ4780)	+= i2c-jz4780.o
>  obj-$(CONFIG_I2C_KEBA)		+= i2c-keba.o
>  obj-$(CONFIG_I2C_KEMPLD)	+= i2c-kempld.o
> +obj-$(CONFIG_I2C_K1)		+= i2c-k1.o

Are you sure you ordered it alphabetically? The same in Kconfig.

>  obj-$(CONFIG_I2C_LPC2K)		+= i2c-lpc2k.o
>  obj-$(CONFIG_I2C_LS2X)		+= i2c-ls2x.o
>  obj-$(CONFIG_I2C_MESON)		+= i2c-meson.o
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> new file mode 100644
> index 000000000000..85053530d9b0
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -0,0 +1,694 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Troy Mitchell <troymitchell988@...il.com>
> + */


...

> +
> +static inline u32 spacemit_i2c_read_reg(struct spacemit_i2c_dev *i2c, int reg)
> +{
> +	return readl(i2c->base + reg);

So basically short and obvious code like this:

	readl(i2c->base + reg);

you replace with:

	spacemit_i2c_read_reg(i2c, reg)

how is this helpful?



> +}
> +
> +static inline void
> +spacemit_i2c_write_reg(struct spacemit_i2c_dev *i2c, int reg, u32 val)
> +{
> +	writel(val, i2c->base + reg);
> +}
> +
> +static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	val = spacemit_i2c_read_reg(i2c, ICR);
> +	spacemit_i2c_write_reg(i2c, ICR, val | CR_IUE);
> +}
> +
> +static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	val = spacemit_i2c_read_reg(i2c, ICR);
> +	val &= ~CR_IUE;
> +	spacemit_i2c_write_reg(i2c, ICR, val);
> +}
> +
> +static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> +{
> +	spacemit_i2c_write_reg(i2c, ICR, CR_UR);
> +	udelay(5);
> +	spacemit_i2c_write_reg(i2c, ICR, 0);
> +}
> +
> +static void spacemit_i2c_bus_reset(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 status;
> +
> +	/* if bus is locked, reset unit. 0: locked */
> +	status = spacemit_i2c_read_reg(i2c, IBMR);
> +
> +	if ((status & BMR_SDA) && (status & BMR_SCL))
> +		return;
> +
> +	spacemit_i2c_reset(i2c);
> +	usleep_range(10, 20);
> +
> +	/* check scl status again */
> +	status = spacemit_i2c_read_reg(i2c, IBMR);
> +
> +	if (!(status & BMR_SCL))
> +		dev_alert(i2c->dev, "unit reset failed\n");

dev_warn_ratelimited

> +}
> +
> +static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c)
> +{
> +	int ret = 0;
> +	u32 val;
> +

...

> +static inline int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c)
> +{
> +	int ret = 0;
> +
> +	spacemit_i2c_reset(i2c);
> +
> +	spacemit_i2c_calc_timeout(i2c);
> +
> +	spacemit_i2c_init(i2c);
> +
> +	spacemit_i2c_enable(i2c);
> +	enable_irq(i2c->irq);
> +
> +	/* i2c wait for bus busy */
> +	ret = spacemit_i2c_recover_bus_busy(i2c);
> +
> +	if (unlikely(ret))
> +		return ret;
> +
> +	ret = spacemit_i2c_xfer_msg(i2c);
> +

This coding style is poor... further on this in probe function.

> +	if (unlikely(ret < 0)) {
> +		dev_dbg(i2c->dev, "i2c transfer error\n");
> +		/* timeout error should not be overridden, and the transfer
> +		 * error will be confirmed by err handle function latter,
> +		 * the reset should be invalid argument error.
> +		 */
> +		if (ret != -ETIMEDOUT)
> +			ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int
> +spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg msgs[], int num)
> +{
> +	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
> +	int ret;
> +
> +	i2c->msgs = msgs;
> +	i2c->msg_num = num;
> +
> +	ret = spacemit_i2c_xfer_core(i2c);
> +
> +	if (likely(!ret))
> +		spacemit_i2c_check_bus_release(i2c);
> +
> +	disable_irq(i2c->irq);
> +
> +	spacemit_i2c_disable(i2c);
> +
> +	if (unlikely((ret == -ETIMEDOUT || ret == -EAGAIN)))
> +		dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n",
> +			  ret, i2c->err);
> +
> +	return ret < 0 ? ret : num;
> +}
> +
> +static u32 spacemit_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +}
> +
> +static const struct i2c_algorithm spacemit_i2c_algo = {
> +	.xfer = spacemit_i2c_xfer,
> +	.functionality = spacemit_i2c_func,
> +};
> +
> +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(struct spacemit_i2c_dev),

sizeof(*) and stop unnecessary wrapping.

> +			   GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;
> +
> +	i2c->dev = &pdev->dev;
> +
> +	i2c->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (!i2c->base) {
> +		ret = PTR_ERR(i2c->base);

Drop

> +		return dev_err_probe(&pdev->dev, ret, "failed to do ioremap");

Just use here PTR_ERR


> +	}
> +
> +	i2c->irq = platform_get_irq(pdev, 0);
> +	if (i2c->irq < 0) {
> +		ret = i2c->irq;

Drop, really useless.

> +		return dev_err_probe(&pdev->dev, ret, "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);
> +

There is never a blank line between call and it's condition check if()
statement.

> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to request irq");
> +
> +	disable_irq(i2c->irq);

Why?

> +
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);

drop

> +		return dev_err_probe(&pdev->dev, ret, "failed to enable clock");
> +	}
> +
> +	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);
> +

Drop stray blank line

> +	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_dt_match[] = {
> +	{ .compatible = "spacemit,k1-i2c", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, spacemit_i2c_dt_match);
> +
> +static struct platform_driver spacemit_i2c_driver = {
> +	.probe = spacemit_i2c_probe,
> +	.remove_new = spacemit_i2c_remove,
> +	.driver = {
> +		.name = "i2c-k1",
> +		.of_match_table = spacemit_i2c_dt_match,
> +	},
> +};
> +module_platform_driver(spacemit_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC");

Best regards,
Krzysztof

Powered by blists - more mailing lists