Re: [PATCH 1/2] i2c: keba: Add KEBA I2C controller support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Gerhard,

On Sat, Jun 01, 2024 at 09:28:45PM +0200, Gerhard Engleder wrote:
> From: Gerhard Engleder <eg@xxxxxxxx>
> 
> The KEBA I2C controller is found in the system FPGA of KEBA PLC devices.
> It is used to connect EEPROMs and hardware monitoring chips.

can you please add more information about the device, please?

...

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) KEBA AG 2012

can we update the date here?

> + * Copyright (C) KEBA Industrial Automation Gmbh 2024
> + *
> + * Driver for KEBA I2C controller FPGA IP core
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_data/i2c-keba.h>

Can you sort them in alphabetical order, please?

> +#define KI2C "i2c-keba"
> +
> +#define KI2C_CAPABILITY_REG	0x02
> +#define KI2C_CONTROL_REG	0x04
> +#define KI2C_CONTROL_DC_REG	0x05
> +#define KI2C_STATUS_REG		0x08
> +#define KI2C_STATUS_DC_REG	0x09
> +#define KI2C_DATA_REG		0x0c
> +
> +#define KI2C_CAPABILITY_CRYPTO	0x01

This crypto is not used anywhere, did you add it for completness
or have you forgotten to use it?

> +#define KI2C_CAPABILITY_DC	0x02
> +
> +#define KI2C_CONTROL_MEN	0x01
> +#define KI2C_CONTROL_MSTA	0x02
> +#define KI2C_CONTROL_RSTA	0x04
> +#define KI2C_CONTROL_MTX	0x08
> +#define KI2C_CONTROL_TXAK	0x10
> +
> +#define KI2C_STATUS_IN_USE	0x01
> +#define KI2C_STATUS_ACK_CYC	0x02
> +#define KI2C_STATUS_RXAK	0x04
> +#define KI2C_STATUS_MCF		0x08
> +
> +#define KI2C_DC_SDA		0x01
> +#define KI2C_DC_SCL		0x02

You could eventually make it as:

#define REG1_ADDR	0xXX
#define   REG1_VAL_1	0xXX
#define   REG1_VAL_2	0xXX
#define   REG1_VAL_3	0xXX

#define REG2_ADDR	0xXX
#define   REG2_VAL_1	0xXX
#define   REG2_VAL_2	0xXX
#define   REG2_VAL_3	0xXX

So that it's clear what belongs to what. Not a binding comment,
just an aesthetic note.

> +
> +#define KI2C_INUSE_SLEEP_US	(2 * USEC_PER_MSEC)
> +#define KI2C_INUSE_TIMEOUT_US	(10 * USEC_PER_SEC)
> +
> +#define KI2C_POLL_DELAY_US	5
> +
> +struct ki2c {
> +	struct platform_device *pdev;
> +	void __iomem *base;
> +	struct i2c_adapter adapter;
> +
> +	struct i2c_client **client;
> +	int client_size;
> +};
> +
> +static int ki2c_inuse_lock(struct ki2c *ki2c)
> +{
> +	u8 sts;
> +	int ret;
> +
> +	/* The I2C controller has an IN_USE bit for locking access to the
> +	 * controller. This enables the use of I2C controller by other none
> +	 * Linux processors.

Please use the proper commenting style:

	/*
	 * Comment line 1
	 * Comment line 2
	 * ...
	 * Comment line N
	 */

> +	 *
> +	 * If the I2C controller is free, then the first read returns
> +	 * IN_USE == 0. After that the I2C controller is locked and further
> +	 * reads of IN_USE return 1.
> +	 *
> +	 * The I2C controller is unlocked by writing 1 into IN_USE.
> +	 */

Basically this is a semaphore.

> +	ret = readb_poll_timeout(ki2c->base + KI2C_STATUS_REG,
> +				 sts, (sts & KI2C_STATUS_IN_USE) == 0,
> +				 KI2C_INUSE_SLEEP_US, KI2C_INUSE_TIMEOUT_US);

we are waiting too long here... the documentaition recommends to
use the readb_poll_timeout for less than 10us, while we are
waiting 2ms.

> +	if (ret != 0)
> +		dev_warn(&ki2c->pdev->dev, "%s err!\n", __func__);
> +
> +	return ret;
> +}
> +
> +static void ki2c_inuse_unlock(struct ki2c *ki2c)
> +{
> +	/* unlock the controller by writing 1 into IN_USE */
> +	iowrite8(KI2C_STATUS_IN_USE, ki2c->base + KI2C_STATUS_REG);
> +}
> +
> +static int ki2c_wait_for_bit(u8 mask, void __iomem *addr, unsigned long timeout)

It looks more natural to have "addr" as a first argument.

> +{
> +	u8 val;
> +
> +	return readb_poll_timeout(addr, val, (val & mask), KI2C_POLL_DELAY_US,
> +				  jiffies_to_usecs(timeout));
> +}
> +static int ki2c_get_sda(struct ki2c *ki2c)
> +{
> +	/* capability KI2C_CAPABILITY_DC required */
> +	return (ioread8(ki2c->base + KI2C_STATUS_DC_REG) & KI2C_DC_SDA) != 0;

Please avoid using such compact style.

> +}
> +	/* generate clock cycles */
> +	ki2c_set_scl(ki2c, val);
> +	ndelay(KI2C_RECOVERY_NDELAY);
> +	while (count++ < KI2C_RECOVERY_CLK_CNT * 2) {
> +		if (val) {
> +			/* SCL shouldn't be low here */
> +			if (!ki2c_get_scl(ki2c)) {
> +				dev_err(&ki2c->pdev->dev,
> +					"SCL is stuck low!\n");
> +				ret = -EBUSY;
> +				break;
> +			}
> +
> +			/* break if SDA is high */
> +			if (ki2c_get_sda(ki2c))
> +				break;
> +		}
> +
> +		val = !val;
> +		ki2c_set_scl(ki2c, val);
> +		ndelay(KI2C_RECOVERY_NDELAY);

I don't know how much sense it makes to wait in ndelays, this is
not going to be precise and... are we sure we want to wait
atomically here?

> +	}
> +
> +	if (!ki2c_get_sda(ki2c)) {
> +		dev_err(&ki2c->pdev->dev, "SDA is still low!\n");

To me this and the above dev_err's are just spamming the dmesg as
we are already printing up in the probe function. If we want to
have more precision printing, then we need to chose where the
dev_err's need to be.

> +		ret = -EBUSY;
> +	}
> +
> +	/* reenable controller */
> +	iowrite8(KI2C_CONTROL_MEN, ki2c->base + KI2C_CONTROL_REG);

...

> +	ret = ki2c_wait_for_data_ack(ki2c);
> +	if (ret < 0)
> +		/* For EEPROMs this is normal behavior during internal write
> +		 * operation.

Please, mind the coding style.

> +		 */
> +		dev_dbg(&ki2c->pdev->dev, "%s wait for ACK err at 0x%02x!\n",
> +			__func__, m->addr);
> +
> +	return ret;
> +}

...

> +static int ki2c_probe(struct platform_device *pdev)
> +{
> +	struct i2c_keba_platform_data *pdata;
> +	struct device *dev = &pdev->dev;
> +	struct i2c_adapter *adap;
> +	struct resource *io;
> +	struct ki2c *ki2c;
> +	int ret;
> +
> +	pdata = dev->platform_data;
> +	if (pdata == 0) {
> +		dev_err(dev, "Platform data not found!\n");
> +		return -ENODEV;

please use dev_err_probe()

Andi

...




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux