| 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
| ||
|
Message-ID: <Y+1Alvn3eepZ6yAC@shikoro>
Date: Wed, 15 Feb 2023 21:29:10 +0100
From: Wolfram Sang <wsa@...nel.org>
To: nick.hawkins@....com
Cc: verdun@....com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, linux@...linux.org.uk,
linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
joel@....id.au
Subject: Re: [PATCH v4 1/5] i2c: hpe: Add GXP SoC I2C Controller
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
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists