| 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: <20131030180536.GI21983@codeaurora.org>
Date: Wed, 30 Oct 2013 11:05:36 -0700
From: Stephen Boyd <sboyd@...eaurora.org>
To: Josh Cartwright <joshc@...eaurora.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-arm-msm@...r.kernel.org,
Sagar Dharia <sdharia@...eaurora.org>,
Gilad Avidov <gavidov@...eaurora.org>,
Michael Bohan <mbohan@...eaurora.org>
Subject: Re: [PATCH v3 04/10] spmi: Add MSM PMIC Arbiter SPMI controller
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,
> + .of_match_table = spmi_pmic_arb_match_table,
> + },
> +};
> +module_platform_driver(spmi_pmic_arb_driver);
MODULE_LICENSE()
MODULE_ALIAS()
?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists