Re: [PATCH v4 1/5] i2c: hpe: Add GXP SoC I2C Controller

[Wolfram Sang <wsa@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7270 bytes --]

Hi Nick,

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index e50f9603d189..8b3951e0ca5c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1457,4 +1457,11 @@ config I2C_VIRTIO
>            This driver can also be built as a module. If so, the module
>            will be called i2c-virtio.
>  
> +config I2C_GXP
> +	tristate "GXP I2C Interface"
> +	depends on ARCH_HPE_GXP || COMPILE_TEST
> +	help
> +	  This enables support for GXP I2C interface. The I2C engines can be
> +	  either I2C master or I2C slaves.
> +

Shouldn't this be in the "I2C system bus drivers (mostly embedded /
system-on-chip)" section? (alphabetically sorted)

> +static bool i2c_global_init_done;

Can't we avoid this static by checking if i2cg_map is NULL/non-NULL?

> +
> +enum {
> +	GXP_I2C_IDLE = 0,
> +	GXP_I2C_ADDR_PHASE,
> +	GXP_I2C_RDATA_PHASE,
> +	GXP_I2C_WDATA_PHASE,
> +	GXP_I2C_ADDR_NACK,
> +	GXP_I2C_DATA_NACK,
> +	GXP_I2C_ERROR,
> +	GXP_I2C_COMP
> +};
> +
> +struct gxp_i2c_drvdata {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct i2c_timings t;
> +	u32 engine;
> +	int irq;
> +	struct completion completion;
> +	struct i2c_adapter adapter;
> +	struct i2c_msg *curr_msg;
> +	int msgs_remaining;
> +	int msgs_num;
> +	u8 *buf;
> +	size_t buf_remaining;
> +	unsigned char state;
> +	struct i2c_client *slave;
> +	unsigned char stopped;
> +};
> +
> +static struct regmap *i2cg_map;
> +
> +static void gxp_i2c_start(struct gxp_i2c_drvdata *drvdata)
> +{
> +	u16 value;
> +
> +	drvdata->buf = drvdata->curr_msg->buf;
> +	drvdata->buf_remaining = drvdata->curr_msg->len;
> +
> +	/* Note: Address in struct i2c_msg is 7 bits */
> +	value = drvdata->curr_msg->addr << 9;
> +
> +	if (drvdata->curr_msg->flags & I2C_M_RD) {
> +		/* Read */
> +		value |= 0x05;
> +	} else {
> +		/* Write */
> +		value |= 0x01;
> +	}

I'd write it more concise as:

	value |= drvdata->curr_msg->flags & I2C_M_RD ? 0x05 : 0x01;

But this is a matter of taste.

> +
> +	drvdata->state = GXP_I2C_ADDR_PHASE;
> +	writew(value, drvdata->base + GXP_I2CMCMD);
> +}
> +
> +static int gxp_i2c_master_xfer(struct i2c_adapter *adapter,
> +			       struct i2c_msg *msgs, int num)
> +{
> +	int ret;
> +	struct gxp_i2c_drvdata *drvdata = i2c_get_adapdata(adapter);
> +	unsigned long time_left;
> +
> +	drvdata->msgs_remaining = num;
> +	drvdata->curr_msg = msgs;
> +	drvdata->msgs_num = num;
> +	reinit_completion(&drvdata->completion);
> +
> +	gxp_i2c_start(drvdata);
> +
> +	time_left = wait_for_completion_timeout(&drvdata->completion,
> +						adapter->timeout);
> +	ret = num - drvdata->msgs_remaining;
> +	if (time_left == 0) {
> +		switch (drvdata->state) {
> +		case GXP_I2C_WDATA_PHASE:
> +			dev_err(drvdata->dev,
> +				"gxp_i2c_start:write Data phase timeout at msg[%d]\n",
> +			ret);

Please don't write error message to the log on timeouts. They can
happen. Think of an EEPROM which is busy because it needs to erase a
page. Upper layers need to handle this correctly, the user doesn't need
to know about it.

> +			break;
> +		case GXP_I2C_RDATA_PHASE:
> +			dev_err(drvdata->dev,
> +				"gxp_i2c_start:read Data phase timeout at msg[%d]\n",
> +			ret);
> +			break;
> +		case GXP_I2C_ADDR_PHASE:
> +			dev_err(drvdata->dev,
> +				"gxp_i2c_start:Addr phase timeout\n");
> +			break;
> +		default:
> +			dev_err(drvdata->dev,
> +				"gxp_i2c_start:i2c transfer timeout state=%d\n",
> +			drvdata->state);
> +			break;
> +		}
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (drvdata->state == GXP_I2C_ADDR_NACK) {
> +		dev_err(drvdata->dev,
> +			"gxp_i2c_start:No ACK for address phase\n");
> +		return -EIO;
> +	} else if (drvdata->state == GXP_I2C_DATA_NACK) {
> +		dev_err(drvdata->dev, "gxp_i2c_start:No ACK for data phase\n");
> +		return -EIO;

Same here for NACK. Otherwise i2cdetect will flood your logfile.

> +	}
> +
> +	return ret;
> +}
> +
> +static u32 gxp_i2c_func(struct i2c_adapter *adap)
> +{
> +	if (IS_ENABLED(CONFIG_I2C_SLAVE))
> +		return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
> +
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int gxp_i2c_reg_slave(struct i2c_client *slave)
> +{
> +	struct gxp_i2c_drvdata *drvdata = i2c_get_adapdata(slave->adapter);
> +
> +	if (drvdata->slave)
> +		return -EBUSY;
> +
> +	if (slave->flags & I2C_CLIENT_TEN)
> +		return -EAFNOSUPPORT;
> +
> +	drvdata->slave = slave;
> +
> +	writeb(slave->addr << 1, drvdata->base + GXP_I2COWNADR);
> +	writeb(0x69, drvdata->base + GXP_I2CSCMD);

Does it make sense to have #defines for the magic values for I2CSCMD?
BTW, is the datasheet public?

...

> +static void gxp_i2c_init(struct gxp_i2c_drvdata *drvdata)
> +{
> +	drvdata->state = GXP_I2C_IDLE;
> +	writeb(2000000 / drvdata->t.bus_freq_hz,
> +	       drvdata->base + GXP_I2CFREQDIV);
> +	writeb(0x32, drvdata->base + GXP_I2CFLTFAIR);
> +	writeb(0x0a, drvdata->base + GXP_I2CTMOEDG);
> +	writeb(0x00, drvdata->base + GXP_I2CCYCTIM);
> +	writeb(0x00, drvdata->base + GXP_I2CSNPAA);
> +	writeb(0x00, drvdata->base + GXP_I2CADVFEAT);
> +	writeb(0xF0, drvdata->base + GXP_I2CSCMD);
> +	writeb(0x80, drvdata->base + GXP_I2CMCMD);
> +	writeb(0x00, drvdata->base + GXP_I2CEVTERR);
> +	writeb(0x00, drvdata->base + GXP_I2COWNADR);

Also here, lots of magic values. Can we do something about it?

> +}
> +
> +static int gxp_i2c_probe(struct platform_device *pdev)
> +{
> +	struct gxp_i2c_drvdata *drvdata;
> +	int rc;
> +	struct i2c_adapter *adapter;
> +
> +	if (!i2c_global_init_done) {
> +		i2cg_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +							   "hpe,sysreg");
> +		if (IS_ERR(i2cg_map)) {
> +			return dev_err_probe(&pdev->dev, IS_ERR(i2cg_map),
> +					     "failed to map i2cg_handle\n");
> +		}
> +
> +		/* Disable interrupt */
> +		regmap_update_bits(i2cg_map, GXP_I2CINTEN, 0x00000FFF, 0);
> +		i2c_global_init_done = true;
> +	}
> +
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata),
> +			       GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, drvdata);
> +	drvdata->dev = &pdev->dev;
> +	init_completion(&drvdata->completion);
> +
> +	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(drvdata->base))
> +		return PTR_ERR(drvdata->base);
> +
> +	/* Use physical memory address to determine which I2C engine this is. */
> +	drvdata->engine = ((u32)drvdata->base & 0xf00) >> 8;

This breaks on my 64-bit test-build, so it will also fail with
COMPILE_TEST.

drivers/i2c/busses/i2c-gxp.c: In function ‘gxp_i2c_probe’:
drivers/i2c/busses/i2c-gxp.c:533:28: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
  533 |         drvdata->engine = ((u32)drvdata->base & 0xf00) >> 8;

The rest looks good to me.

Except for removing the hardcoded values, the other fixes should be
fairly simple, I think. So, hardcoded values could be changed
incrementally maybe. If this is possible at all. I still plan to have
this driver in 6.3.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]