linux-kernel - Re: [PATCH V2] i2c: imx-lpi2c: add target mode 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <qcoguhxtkwn2aowtccfybutn6xgzrqvhdob4tzericerpfntfh@q6f5upgegba7>
Date: Fri, 6 Sep 2024 00:09:56 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: carlos.song@....com
Cc: aisheng.dong@....com, shawnguo@...nel.org, s.hauer@...gutronix.de, 
	kernel@...gutronix.de, festevam@...il.com, linux-i2c@...r.kernel.org, 
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] i2c: imx-lpi2c: add target mode support

Hi Carlos,

Looks good, just few little comments.

On Thu, Aug 29, 2024 at 06:54:44PM GMT, carlos.song@....com wrote:
> From: Carlos Song <carlos.song@....com>
> 
> Add target mode support for LPI2C.

Can you please be a bit more descriptive? What is this mode
doing, how it works, what are you adding, etc. etc. etc.

> Signed-off-by: Carlos Song <carlos.song@....com>
> ---
> Change for V2:
> - remove unused variable ‘lpi2c_imx’ in lpi2c_suspend_noirq.

this was part of a 5 patches series. Is that series superseded?

(please, next time when you send a series with more than one
patch, include a cover letter).

...

> +#define SCR_SEN		BIT(0)
> +#define SCR_RST		BIT(1)
> +#define SCR_FILTEN	BIT(4)
> +#define SCR_RTF		BIT(8)
> +#define SCR_RRF		BIT(9)
> +#define SCFGR1_RXSTALL	BIT(1)
> +#define SCFGR1_TXDSTALL	BIT(2)
> +#define SCFGR2_FILTSDA_SHIFT	24
> +#define SCFGR2_FILTSCL_SHIFT	16
> +#define SCFGR2_CLKHOLD(x)	(x)
> +#define SCFGR2_FILTSDA(x)	((x) << SCFGR2_FILTSDA_SHIFT)
> +#define SCFGR2_FILTSCL(x)	((x) << SCFGR2_FILTSCL_SHIFT)
> +#define SSR_TDF		BIT(0)
> +#define SSR_RDF		BIT(1)
> +#define SSR_AVF		BIT(2)
> +#define SSR_TAF		BIT(3)
> +#define SSR_RSF		BIT(8)
> +#define SSR_SDF		BIT(9)
> +#define SSR_BEF		BIT(10)
> +#define SSR_FEF		BIT(11)
> +#define SSR_SBF		BIT(24)
> +#define SSR_BBF		BIT(25)
> +#define SSR_CLEAR_BITS	(SSR_RSF | SSR_SDF | SSR_BEF | SSR_FEF)
> +#define SIER_TDIE	BIT(0)
> +#define SIER_RDIE	BIT(1)
> +#define SIER_AVIE	BIT(2)
> +#define SIER_TAIE	BIT(3)
> +#define SIER_RSIE	BIT(8)
> +#define SIER_SDIE	BIT(9)
> +#define SIER_BEIE	BIT(10)
> +#define SIER_FEIE	BIT(11)
> +#define SIER_AM0F	BIT(12)
> +#define SASR_READ_REQ	0x1
> +#define SLAVE_INT_FLAG	(SIER_TDIE | SIER_RDIE | SIER_AVIE | \
> +						SIER_SDIE | SIER_BEIE)

I'm not happy about the alignment here, can we have the columns
well aligned, please?

> +
>  #define I2C_CLK_RATIO	2
>  #define CHUNK_DATA	256

...

> +	if (sier_filter & SSR_SDF) {
> +		/* STOP */
> +		i2c_slave_event(lpi2c_imx->target, I2C_SLAVE_STOP, &value);
> +	}

nit: no need for brackets here.

...

> +static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
> +{
> +	struct lpi2c_imx_struct *lpi2c_imx = dev_id;
> +	u32 ssr, sier_filter;
> +	unsigned int scr;

why is ssr unsigned int and not u32?

Can we define ssr, sier_filter and scr in the innermost section?
i.e. inside the "if () { ... }"

> +
> +	if (lpi2c_imx->target) {
> +		scr = readl(lpi2c_imx->base + LPI2C_SCR);
> +		ssr = readl(lpi2c_imx->base + LPI2C_SSR);
> +		sier_filter = ssr & readl(lpi2c_imx->base + LPI2C_SIER);
> +		if ((scr & SCR_SEN) && sier_filter)
> +			return lpi2c_imx_target_isr(lpi2c_imx, ssr, sier_filter);
> +		else
> +			return lpi2c_imx_master_isr(lpi2c_imx);
> +	} else {
> +		return lpi2c_imx_master_isr(lpi2c_imx);
> +	}

this can be simplified a bit:

	if (...) {
		scr = ...
		ssr = ...
		sier_filter = ...

		/* a nice comment */
		if (...)
			return lpi2c_imx_target_isr(...)
	}

	return lpi2c_imx_master_isr(lpi2c_imx);

Not a binding comment. Your choice.

> +}
> +
> +static void lpi2c_imx_target_init(struct lpi2c_imx_struct *lpi2c_imx)
> +{
> +	int temp;

u32?

Thanks,
Andi