Re: [PATCH] i2c: Driver to expose PowerNV platform i2c busses
flat view
From: Wolfram Sang <hidden>
Date: 2014-11-13 07:57:06
Also in:
linux-i2c
Hi, I am basically fine if this goes via the powerpc-tree and I was hoping that I could ack it right now. However, the driver looks a bit rushed and definately needs updates before it is ready to go. On Mon, Nov 10, 2014 at 11:35:39AM +0530, Neelesh Gupta wrote:
The patch exposes the available i2c busses on the PowerNV platform to the kernel and implements the bus driver to support i2c and smbus commands. The driver uses the platform device infrastructure to probe the busses on the platform and registers them with the i2c driver framework. Signed-off-by: Neelesh Gupta <redacted> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Review for the I2C parts:
quoted hunk ↗ jump to hunk
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 78d56c5..350aa86 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile@@ -102,5 +102,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o +obj-$(CONFIG_I2C_OPAL) += i2c-opal.o
Please sort it properly, not simply at the end.
quoted hunk ↗ jump to hunk
ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUGdiff --git a/drivers/i2c/busses/i2c-opal.c b/drivers/i2c/busses/i2c-opal.c new file mode 100644 index 0000000..3261716 --- /dev/null +++ b/drivers/i2c/busses/i2c-opal.c@@ -0,0 +1,276 @@ +/* + * IBM OPAL I2C driver + * Copyright (C) 2014 IBM + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. + */ + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/i2c.h> +#include <linux/device.h> +#include <linux/platform_device.h> +#include <linux/of.h> +#include <linux/mm.h> +#include <asm/opal.h> +#include <asm/firmware.h>
Please sort the includes.
quoted hunk ↗ jump to hunk
+ +static int i2c_opal_send_request(u32 bus_id, struct opal_i2c_request *req) +{ + struct opal_msg msg; + int token, rc; + + token = opal_async_get_token_interruptible(); + if (token < 0) { + if (token != -ERESTARTSYS) + pr_err("Failed to get the async token\n"); + + return token; + } + + rc = opal_i2c_request(token, bus_id, req); + if (rc != OPAL_ASYNC_COMPLETION) { + rc = -EIO; + goto exit; + } + + rc = opal_async_wait_response(token, &msg); + if (rc) { + rc = -EIO; + goto exit;
Is it really -EIO? Maybe -ETIMEDOUT?
quoted hunk ↗ jump to hunk
+ } + + rc = be64_to_cpu(msg.params[1]); + if (rc != OPAL_SUCCESS) { + rc = -EIO; + goto exit; + } + +exit: + opal_async_release_token(token); + return rc; +} + +static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, + int num) +{ + unsigned long opal_id = (unsigned long)adap->algo_data; + struct opal_i2c_request req; + int rc, i; + + /* We only support fairly simple combinations here of one + * or two messages + */
I don't think you should offer I2C_FUNC_I2C with those limitations. Is there a case you really needs this?
quoted hunk ↗ jump to hunk
+ memset(&req, 0, sizeof(req)); + switch (num) { + case 0: + return 0; + case 1: + req.type = (msgs[0].flags & I2C_M_RD) ? + OPAL_I2C_RAW_READ : OPAL_I2C_RAW_WRITE; + req.addr = cpu_to_be16(msgs[0].addr); + req.size = cpu_to_be32(msgs[0].len); + req.buffer_ra = cpu_to_be64(__pa(msgs[0].buf)); + break; + case 2: + /* For two messages, we basically support only simple + * smbus transactions of a write plus a read. We might + * want to allow also two writes but we'd have to bounce + * the data into a single buffer. + */ + if ((msgs[0].flags & I2C_M_RD) || !(msgs[1].flags & I2C_M_RD)) + return -EIO; + if (msgs[0].len > 4) + return -EIO; + if (msgs[0].addr != msgs[1].addr) + return -EIO;
-EOPNOTSUPP? Please check Documentation/i2c/fault-codes for the error codes we use.
quoted hunk ↗ jump to hunk
+ req.type = OPAL_I2C_SM_READ; + req.addr = cpu_to_be16(msgs[0].addr); + req.subaddr_sz = msgs[0].len; + for (i = 0; i < msgs[0].len; i++) + req.subaddr = (req.subaddr << 8) | msgs[0].buf[i]; + req.subaddr = cpu_to_be32(req.subaddr); + req.size = cpu_to_be32(msgs[1].len); + req.buffer_ra = cpu_to_be64(__pa(msgs[1].buf)); + break; + default: + return -EIO; + } + + rc = i2c_opal_send_request(opal_id, &req); + if (rc) + return rc; + + return num; +} + +static int i2c_opal_smbus_xfer(struct i2c_adapter *adap, u16 addr, + unsigned short flags, char read_write, + u8 command, int size, union i2c_smbus_data *data) +{ + unsigned long opal_id = (unsigned long)adap->algo_data; + struct opal_i2c_request req; + u8 local[2]; + int rc; + + memset(&req, 0, sizeof(req)); + + req.addr = cpu_to_be16(addr); + switch (size) { + case I2C_SMBUS_BYTE: + req.buffer_ra = cpu_to_be64(__pa(&data->byte)); + req.size = cpu_to_be32(1); + /* Fall through */ + case I2C_SMBUS_QUICK: + req.type = (read_write == I2C_SMBUS_READ) ? + OPAL_I2C_RAW_READ : OPAL_I2C_RAW_WRITE; + break; + case I2C_SMBUS_BYTE_DATA: + req.buffer_ra = cpu_to_be64(__pa(&data->byte)); + req.size = cpu_to_be32(1); + req.subaddr = cpu_to_be32(command); + req.subaddr_sz = 1; + req.type = (read_write == I2C_SMBUS_READ) ? + OPAL_I2C_SM_READ : OPAL_I2C_SM_WRITE; + break; + case I2C_SMBUS_WORD_DATA: + if (!read_write) { + local[0] = data->word & 0xff; + local[1] = (data->word >> 8) & 0xff; + } + req.buffer_ra = cpu_to_be64(__pa(local)); + req.size = cpu_to_be32(2); + req.subaddr = cpu_to_be32(command); + req.subaddr_sz = 1; + req.type = (read_write == I2C_SMBUS_READ) ? + OPAL_I2C_SM_READ : OPAL_I2C_SM_WRITE; + break; + case I2C_SMBUS_I2C_BLOCK_DATA: + req.buffer_ra = cpu_to_be64(__pa(&data->block[1])); + req.size = cpu_to_be32(data->block[0]); + req.subaddr = cpu_to_be32(command); + req.subaddr_sz = 1; + req.type = (read_write == I2C_SMBUS_READ) ? + OPAL_I2C_SM_READ : OPAL_I2C_SM_WRITE; + break; + default: + return -EINVAL; + } + + rc = i2c_opal_send_request(opal_id, &req); + if (!rc && read_write && size == I2C_SMBUS_WORD_DATA) { + data->word = ((u16)local[1]) << 8; + data->word |= local[0]; + } + + return rc; +} + +static u32 i2c_opal_func(struct i2c_adapter *adapter) +{ + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | + I2C_FUNC_SMBUS_I2C_BLOCK; +}
See comment above about I2C_FUNC_I2C?
quoted hunk ↗ jump to hunk
+ +static const struct i2c_algorithm i2c_opal_algo = { + .master_xfer = i2c_opal_master_xfer, + .smbus_xfer = i2c_opal_smbus_xfer, + .functionality = i2c_opal_func, +}; + +static int i2c_opal_probe(struct platform_device *pdev) +{ + struct i2c_adapter *adapter; + const char *pname; + u32 opal_id; + int rc; + + if (!pdev->dev.of_node) + return -ENODEV;
Can this happen? How would the match happen otherwise?
quoted hunk ↗ jump to hunk
+ rc = of_property_read_u32(pdev->dev.of_node, "ibm,opal-id", &opal_id); + if (rc) { + dev_err(&pdev->dev, "Missing ibm,opal-id property !\n"); + return -EIO; + } + adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
devm_kzalloc?
quoted hunk ↗ jump to hunk
+ if (!adapter) + return -ENOMEM; + adapter->algo = &i2c_opal_algo; + adapter->algo_data = (void *)(unsigned long)opal_id;
double cast?
quoted hunk ↗ jump to hunk
+ adapter->dev.parent = &pdev->dev; + adapter->dev.of_node = of_node_get(pdev->dev.of_node); + pname = of_get_property(pdev->dev.of_node, "port-name", NULL);
I have never seen this binding before, it looks fishy. Where is it documented?
quoted hunk ↗ jump to hunk
+ if (pname) + strlcpy(adapter->name, pname, sizeof(adapter->name)); + else + strlcpy(adapter->name, "opal", sizeof(adapter->name)); + + platform_set_drvdata(pdev, adapter); + rc = i2c_add_adapter(adapter); + if (rc) + dev_err(&pdev->dev, "Failed to register the i2c adapter\n");
Leaking 'adapter' here.
quoted hunk ↗ jump to hunk
+ + return rc; +} + +static int i2c_opal_remove(struct platform_device *pdev) +{ + struct i2c_adapter *adapter = platform_get_drvdata(pdev); + + i2c_del_adapter(adapter); + + kfree(adapter); + + return 0; +} + +static const struct of_device_id i2c_opal_of_match[] = { + { + .compatible = "ibm,power8-i2c-port", + }, + { } +}; +MODULE_DEVICE_TABLE(of, i2c_opal_of_match); + +static struct platform_driver i2c_opal_driver = { + .probe = i2c_opal_probe, + .remove = i2c_opal_remove, + .driver = { + .name = "i2c-opal", + .owner = THIS_MODULE,
Not needed.
quoted hunk ↗ jump to hunk
+ .of_match_table = i2c_opal_of_match, + }, +}; + +static int __init i2c_opal_init(void) +{ + if (!firmware_has_feature(FW_FEATURE_OPAL)) + return -ENODEV; + + return platform_driver_register(&i2c_opal_driver); +} + +static void __exit i2c_opal_exit(void) +{ + return platform_driver_unregister(&i2c_opal_driver); +} + +MODULE_AUTHOR("Neelesh Gupta <neelegup@linux.vnet.ibm.com>"); +MODULE_DESCRIPTION("IBM OPAL I2C driver"); +MODULE_LICENSE("GPL"); + +module_init(i2c_opal_init); +module_exit(i2c_opal_exit);
Please put thos right below the functions it references.
-- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachments
- signature.asc [application/pgp-signature] 819 bytes