linux-kernel - Re: [PATCH 2/2] i2c: atciic100: add Andes I2C driver support

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <neeqqifb3urhtchaqowa6pkq3s3v3xwzlz4bujmiuq7tsvhz4i@a3v4vbxqozgr>
Date: Thu, 20 Mar 2025 11:44:08 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Ben Zong-You Xie <ben717@...estech.com>
Cc: linux-i2c@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Subject: Re: [PATCH 2/2] i2c: atciic100: add Andes I2C driver support

Hi Ben,

I glanced throught the patch and looks OK, just few things with
little importance. Please, do not forget to run checkpatch.pl
before sendint the patch.

Once you send a v2, I will read it more carefully.

...

> +	if (status & ATCIIC100_STATUS_CMPL) {
> +		/* Write 1 to clear all status */
> +		writel(ATCIIC100_STATUS_W1C, i2c->base + ATCIIC100_STATUS_REG);
> +
> +		i2c->xfer_done = true;
> +		if (status & ATCIIC100_STATUS_ADDR_HIT)
> +			i2c->addr_hit = true;
> +
> +		/* For the last read, retrieve all remaining data in FIFO. */
> +		while (i2c->buf_len--)
> +			*i2c->buf++ = readl(i2c->base + ATCIIC100_DATA_REG);
> +

please, remove this blank space here

> +	}
> +
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +}

...

> +static int atciic100_xfer_wait(struct atciic100 *i2c, struct i2c_msg *msg)
> +{
> +	u32 val;
> +
> +	/*
> +	 * Set the data count.
> +	 * If there are 256 bytes to be transmitted/received, write 0 to the
> +	 * data count field.
> +	 */
> +	val = readl(i2c->base + ATCIIC100_CTRL_REG) |
> +	      (i2c->buf_len & ATCIIC100_CTRL_DATA_CNT);

please checkt the alignment here.

> +	/* Set the transaction direction */
> +	if (msg->flags & I2C_M_RD)
> +		val |= ATCIIC100_CTRL_DIR;
> +	else
> +		val &= ~ATCIIC100_CTRL_DIR;

...

> +/* I2C may be needed to bring up other drivers */
> +static int __init atciic100_init_driver(void)
> +{
> +	return platform_driver_register(&atciic100_platform_driver);
> +}
> +subsys_initcall(atciic100_init_driver);
> +
> +static void __exit atciic100_exit_driver(void)
> +{
> +	platform_driver_unregister(&atciic100_platform_driver);
> +}
> +module_exit(atciic100_exit_driver);

can you please use module_platform_driver()?

Andi

> +
> +MODULE_AUTHOR("Ben Zong-You Xie <ben717@...estech.com>");
> +MODULE_DESCRIPTION("Andes ATCIIC100 I2C bus adapter");
> +MODULE_LICENSE("GPL");
> +
> -- 
> 2.34.1
>