On Thu, May 29, 2014 at 9:19 AM, Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:
>> +++ b/drivers/mfd/qcom_rpm.c
[...]
>> +struct qcom_rpm {
>> + struct device *dev;
>> + struct completion ack;
>> + struct mutex lock;
>> +
>> + void __iomem *status_regs;
>> + void __iomem *ctrl_regs;
>> + void __iomem *req_regs;
>> +
>> + void __iomem *ipc_rpm_reg;
>> +
>> + u32 ack_status;
>> +
>> + u32 version;
>> +
>> + const struct qcom_rpm_resource *resource_table;
>> + unsigned n_resources;
>
>
> the line spacing looks bit odd.
>
I'll fix
>> +};
>> +
>> +#define RPM_STATUS_REG(rpm, i) ((rpm)->status_regs + (i) * 4)
>> +#define RPM_CTRL_REG(rpm, i) ((rpm)->ctrl_regs + (i) * 4)
>> +#define RPM_REQ_REG(rpm, i) ((rpm)->req_regs + (i) * 4)
>
>
> Probably you could make these macros bit more generic by removing the rpm
> and let the calling code dereference it.
>
>
I first open coded them, I then had separate writel/readl wrappers for them and
then I settled for this, as I figured it help clarifying the code. I can have
another look at it, but I don't think that below will make things clearer.
#define RPM_IDX_2_OFFSET(i) ((i) * 4)
[...]
>> +
>> +static irqreturn_t qcom_rpm_ack_interrupt(int irq, void *dev)
>> +{
>> + struct qcom_rpm *rpm = dev;
>> + u32 ack;
>> + int i;
>> +
>> + ack = readl_relaxed(RPM_CTRL_REG(rpm, RPM_ACK_CONTEXT));
>
> /n
>
Will try it out.
>> + for (i = 0; i < RPM_SELECT_SIZE; i++)
>> + writel_relaxed(0, RPM_CTRL_REG(rpm, RPM_ACK_SELECTOR +
>> i));
>
>
> /n
>
Will try it out, although to me this grouping says "write all selectors and the
context".
>> + writel(0, RPM_CTRL_REG(rpm, RPM_ACK_CONTEXT));
>> +
>> + if (ack & RPM_NOTIFICATION) {
>> + dev_warn(rpm->dev, "ignoring notification!\n");
>> + } else {
>> + rpm->ack_status = ack;
>> + complete(&rpm->ack);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
[...]
>> +static int qcom_rpm_probe(struct platform_device *pdev)
>> +{
>> + const struct of_device_id *match;
>> + const struct qcom_rpm *template;
>> + struct resource *res;
>> + struct qcom_rpm *rpm;
>> + u32 fw_version[3];
>> + int irq_wakeup;
>> + int irq_ack;
>> + int irq_err;
>> + int ret;
>> +
>> + rpm = devm_kzalloc(&pdev->dev, sizeof(*rpm), GFP_KERNEL);
>
>
> Sorry If I missed somthing obvious, But why not just use the structure from
> of_data. Is the global structure going to be used for something else?
>
> Or make a seperate structure for of_data and not use struct qcom_rpm?
>
>
>
Although we will not have more than one rpm in a system and therefore not
instatiate this driver multiple times I do not want to run it off the global
state.
>> + if (!rpm) {
>> + dev_err(&pdev->dev, "Can't allocate qcom_rpm\n");
>
> message not necessary as kernel will print the alocation failures.
>
Thanks!
>> + return -ENOMEM;
>> + }
[...]
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> Should't you use the platform_get_resource_byname here?
>
> missed error case checks too.
>
This is a fairly commonly used construct, to have the error from
platform_get_resource being propagated through devm_ioremap_resource and catch
it there. It gives an extra error print in the log, but I find it very clean.
>> + rpm->status_regs = devm_ioremap_resource(&pdev->dev, res);
>> + rpm->ctrl_regs = rpm->status_regs + 0x400;
>> + rpm->req_regs = rpm->status_regs + 0x600;
>> + if (IS_ERR(rpm->status_regs))
>> + return PTR_ERR(rpm->status_regs);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>
> Dito.
>
[...]
>
>
> [ ..
>
>> + ret = irq_set_irq_wake(irq_ack, 1);
>> + if (ret)
>> + dev_warn(&pdev->dev, "failed to mark ack irq as
>> wakeup\n");
>> +
>
> ..]
>
> Shouln't these be set as part of the pm suspend call, if the device is
> wakeup capable?
>
>
Is there any reason to toggle this?
I'm not sure when this interrupt will actually be fired, but I don't see any
harm in keeping it wakup enabled at all times.
[...]
>> +++ b/include/linux/mfd/qcom_rpm.h
>> @@ -0,0 +1,12 @@
>> +#ifndef __QCOM_RPM_H__
>> +#define __QCOM_RPM_H__
>> +
>> +#include <linux/types.h>
>> +
>> +struct device;
>> +struct qcom_rpm;
>> +
>> +struct qcom_rpm *dev_get_qcom_rpm(struct device *dev);
>> +int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t
>> count);
>
>
> IMHO, dummy functions for these are required, otherwise you would get a
> compilation errors on client drivers.
>
I didn't expect us to compile the children into a kernel that doesn't have the
rpm, as I see them as one entity. An exception would be if we want to add
COMPILE_TEST to the children, but that would require an extra change anyways.
Thanks for the review!
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
