linux-kernel - Re: [PATCH v3 1/4] i2c: machxo2: new driver

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] [day] [month] [year] [list]

Message-ID: <unkntq6gtw7bbunzjj6av7icf6u4kyhommxwqvyaiozaotyzrq@a2n3vzfmvvrg>
Date: Thu, 23 Oct 2025 15:14:00 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Matthias Schiffer <matthias.schiffer@...tq-group.com>
Cc: Lee Jones <lee@...nel.org>, linux-i2c@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux@...tq-group.com
Subject: Re: [PATCH v3 1/4] i2c: machxo2: new driver

Hi Matthias,

...

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index fedf5d31f9035..e270f7d9e0254 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -855,6 +855,16 @@ config I2C_LS2X
>  	  This driver can also be built as a module. If so, the module
>  	  will be called i2c-ls2x.
>  
> +config I2C_MACHXO2
> +	tristate "Lattice MachXO2 I2C Controller"
> +	select REGMAP_MMIO

do we need "depends on HAS_IOPORT" here?

> +	help
> +	  If you say yes to this option, support will be included for the
> +	  "Hardened I2C" controller found on the Lattice MachXO2 PLD family.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-machxo2.
> +

...

> +static enum hrtimer_restart machxo2_restart_timer(struct machxo2_i2c *i2c)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&i2c->lock, flags);
> +	if (!hrtimer_is_queued(&i2c->timer))
> +		hrtimer_forward_now(&i2c->timer, i2c->timer_wait);
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +	/* Exponential backoff for timer */
> +	i2c->timer_wait *= 2;

you could use here

	i2c->timer_wait = ktime_add(i2c->timer_wait, i2c->timer_wait);

> +
> +	return HRTIMER_RESTART;
> +}

...

> +static int machxo2_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct machxo2_i2c *i2c = i2c_get_adapdata(adap);
> +
> +	machxo2_wait_not_busy(i2c, 1000, jiffies_to_usecs(adap->timeout) + 1);
> +
> +	dev_dbg(&adap->dev, "new msg: addr %02x, flags %x, nmsgs %d, len %d\n",
> +		msgs->addr, msgs->flags, num, msgs[0].len);
> +
> +	i2c->msg = msgs;
> +	i2c->pos = 0;
> +	i2c->nmsgs = num;
> +	i2c->error = 0;
> +	i2c->state = MACHXO2_I2C_STATE_START;
> +	/* Ensure that the hrtimer sees the state change */
> +	smp_wmb();
> +
> +	spin_lock_irq(&i2c->lock);

I would use here spin_lock_irq_irqsave()

> +	hrtimer_start(&i2c->timer, 0, HRTIMER_MODE_REL);

nitpick: '0' can be otherwise written as ktime_set(0, 0). Not
important, as you wish.

> +	spin_unlock_irq(&i2c->lock);
> +
> +	if (!wait_event_timeout(i2c->wait, i2c->state == MACHXO2_I2C_STATE_DONE,
> +				adap->timeout)) {

...

> +
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata || !pdata->clock_khz)
> +		return -EINVAL;
> +
> +	i2c->clock_khz = pdata->clock_khz;
> +	i2c->bus_khz = pdata->bus_khz;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (IS_ERR(res))
> +		return PTR_ERR(res);

platform_get_resource() returns "a pointer to the resource or
NULL on failure". So that you need to check for "!res" and return
-ENODEV in case.

Thanks,
Andi

> +
> +	if (!devm_request_region(&pdev->dev, res->start, resource_size(res),
> +				 pdev->name))
> +		return dev_err_probe(&pdev->dev, -EBUSY, "Can't get I/O resource.\n");
> +