On 10/28, Josh Cartwright wrote:
+
+/**
+ * pa_write_data: write 1..4 bytes from buf to pmic-arb's register
+ * @bc byte-count -1. range: 0..3
+ * @reg register's address
+ * @buf buffer to write. length must be bc+1
Missing colon between variable and description.
+ */
+static void
+pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
+{
+ u32 data = 0;
+ memcpy(data, buf, (bc 3) + 1);
+ pmic_arb_base_write(dev, reg, data);
+}
+
[...]
+static int pmic_arb_read_cmd(struct spmi_controller *ctrl,
+ u8 opc, u8 sid, u16 addr, u8 bc, u8 *buf)
+{
+ struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
+ unsigned long flags;
+ u32 cmd;
+ int rc;
+
+ if (bc = PMIC_ARB_MAX_TRANS_BYTES) {
+ dev_err(ctrl-dev,
+ pmic-arb supports 1..%d bytes per trans, but:%d
requested,
Nitpick: Please replace the colon between but and %d with a space.
+ PMIC_ARB_MAX_TRANS_BYTES, bc+1);
Space around that '+' please.
+ return -EINVAL;
+ }
+ dev_dbg(ctrl-dev,
+ op:0x%x sid:%d bc:%d addr:0x%x\n, opc, sid, bc, addr);
+
+ /* Check the opcode */
+ if (opc = 0x60 opc = 0x7F)
+ opc = PMIC_ARB_OP_READ;
+ else if (opc = 0x20 opc = 0x2F)
+ opc = PMIC_ARB_OP_EXT_READ;
+ else if (opc = 0x38 opc = 0x3F)
+ opc = PMIC_ARB_OP_EXT_READL;
+ else
+ return -EINVAL;
+
+ cmd = (opc 27) | ((sid 0xf) 20) | (addr 4) | (bc 0x7);
+
+ spin_lock_irqsave(pmic_arb-lock, flags);
+ pmic_arb_base_write(pmic_arb, PMIC_ARB_CMD(pmic_arb-channel), cmd);
+ rc = pmic_arb_wait_for_done(ctrl);
+ if (rc)
+ goto done;
+
+ /* Read from FIFO, note 'bc' is actually number of bytes minus 1 */
+ pa_read_data(pmic_arb, buf, PMIC_ARB_RDATA0(pmic_arb-channel)
+ , min_t(u8, bc, 3));
Nitpick: Weird comma starting a line here.
+
+ if (bc 3)
+ pa_read_data(pmic_arb, buf + 4,
+ PMIC_ARB_RDATA1(pmic_arb-channel), bc - 4);
+
+done:
+ spin_unlock_irqrestore(pmic_arb-lock, flags);
+ return rc;
+}
+
[...]
+static int spmi_pmic_arb_probe(struct platform_device *pdev)
+{
+ struct spmi_pmic_arb_dev *pa;
+ struct spmi_controller *ctrl;
+ struct resource *res;
+ int err, i;
+
+ ctrl = spmi_controller_alloc(pdev-dev, sizeof(*pa));
+ if (!ctrl)
+ return -ENOMEM;
+
+ pa = spmi_controller_get_drvdata(ctrl);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, core);
+ pa-base = devm_ioremap_resource(ctrl-dev, res);
+ if (IS_ERR(pa-base)) {
+ err = PTR_ERR(pa-base);
+ goto err_put_ctrl;
+ }
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, intr);
+ pa-intr = devm_ioremap_resource(ctrl-dev, res);
+ if (IS_ERR(pa-intr)) {
+ err = PTR_ERR(pa-intr);
+ goto err_put_ctrl;
+ }
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, cnfg);
+ pa-cnfg = devm_ioremap_resource(ctrl-dev, res);
+ if (IS_ERR(pa-cnfg)) {
+ err = PTR_ERR(pa-cnfg);
+ goto err_put_ctrl;
+ }
+
+ for (i = 0; i ARRAY_SIZE(pa-mapping_table); ++i)
+ pa-mapping_table[i] = readl_relaxed(
+ pa-cnfg + SPMI_MAPPING_TABLE_REG(i));
+
+ platform_set_drvdata(pdev, ctrl);
+ spin_lock_init(pa-lock);
+
+ pa-channel = 0;
+ pa-max_apid = 0;
+ pa-min_apid = PMIC_ARB_MAX_PERIPHS - 1;
That looks backwards. Is this right?
+
+ ctrl-cmd = pmic_arb_cmd;
+ ctrl-read_cmd = pmic_arb_read_cmd;
+ ctrl-write_cmd = pmic_arb_write_cmd;
+
+ err = spmi_controller_add(ctrl);
+ if (err)
+ goto err_put_ctrl;
+
+ dev_dbg(ctrl-dev, PMIC Arb Version 0x%x\n,
+ pmic_arb_base_read(pa, PMIC_ARB_VERSION));
+
+ return 0;
+
+err_put_ctrl:
+ spmi_controller_put(ctrl);
+ return err;
+}
+
+static int __exit spmi_pmic_arb_remove(struct platform_device *pdev)
__exit shouldn't be here. We want this function in modules.
+{
+ struct spmi_controller *ctrl = platform_get_drvdata(pdev);
+ spmi_controller_remove(ctrl);
+ return 0;
+}
+
+static struct of_device_id spmi_pmic_arb_match_table[] = {
+ { .compatible = qcom,spmi-pmic-arb, },
+ {},
+};
+MODULE_DEVICE_TABLE(of, spmi_pmic_arb_match_table);
+
+static struct platform_driver spmi_pmic_arb_driver = {
+ .probe = spmi_pmic_arb_probe,
+ .remove = __exit_p(spmi_pmic_arb_remove),
Please drop this __exit_p() usage as well.
+ .driver = {
+ .name = spmi_pmic_arb,
+ .owner = THIS_MODULE,
+
