Messages in this thread | | | Date | Mon, 13 Jul 2015 14:58:25 -0700 | From | Bjorn Andersson <> | Subject | Re: [PATCH v2 06/11] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD |
| |
On Tue 07 Jul 05:37 PDT 2015, Lee Jones wrote:
> On Fri, 26 Jun 2015, bjorn@kryo.se wrote: > > > From: Bjorn Andersson <bjorn.andersson@sonymobile.com> [..]
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
[..]
> > +config MFD_QCOM_SMD_RPM > > + tristate "Qualcomm Resource Power Manager (RPM) over SMD" > > + depends on QCOM_SMD && OF > > + help > > + If you say yes to this option, support will be included for the > > + Resource Power Manager system found in the Qualcomm 8974 based > > + devices. > > + > > + This is required to access many regulators, clocks and bus > > + frequencies controlled by the RPM on these devices. > > + > > + Say M here if you want to include support for the Qualcomm RPM as a > > + module. This will build a module called "qcom-smd-rpm". > > I'm not exactly sure what makes this an MFD device. >
It represents a piece of hardware (a micro-controller) that exposes control of a multitude of regulators and clocks in the Qualcomm platforms.
It's basically just a successor of the qcom_rpm driver - same functionality but a new communication method is used.
[..]
> > diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rpm.c
[..]
> > + > > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist" > > I don't like this at all. >
Which part of it?
It should probably be a static const char *, inlined in the function below. Would that be to your liking?
> > +static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev, > > + const void *data, > > + size_t count) > > +{ > > + const struct qcom_rpm_header *hdr = data; > > + const struct qcom_rpm_message *msg; > > + const size_t inv_res_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1; > > + struct qcom_smd_rpm *rpm = dev_get_drvdata(&qsdev->dev); > > + const u8 *buf = data + sizeof(struct qcom_rpm_header); > > + const u8 *end = buf + hdr->length; > > + int status = 0; > > + > > + if (hdr->service_type != RPM_SERVICE_TYPE_REQUEST || > > + hdr->length < sizeof(struct qcom_rpm_message)) { > > + dev_err(&qsdev->dev, "invalid request\n"); > > + return 0; > > + } > > + > > + while (buf < end) { > > + msg = (struct qcom_rpm_message *)buf; > > + switch (msg->msg_type) { > > + case RPM_MSG_TYPE_MSG_ID: > > + break; > > + case RPM_MSG_TYPE_ERR: > > + if (msg->length == inv_res_len && > > + !memcmp(msg->message, > > + RPM_ERR_INVALID_RESOURCE, > > + inv_res_len)) > > strncpy(msg->message, "resource does not exist", 23); >
No, I want to compare the content of msg->message with the string "resource does not exist" - as that's the only way to know what type of error we got.
This is unfortunately how the protocol looks :/
> > + status = -ENXIO; > > + else > > + status = -EIO; > > + break; > > + } > > + > > + buf = PTR_ALIGN(buf + 2 * sizeof(u32) + msg->length, 4); > > + } > > + > > + rpm->ack_status = status; > > + complete(&rpm->ack); > > + return 0; > > +} > > +
[..]
> > + > > +static struct qcom_smd_driver qcom_smd_rpm_driver = { > > + .probe = qcom_smd_rpm_probe, > > + .remove = qcom_smd_rpm_remove, > > + .callback = qcom_smd_rpm_callback, > > + .driver = { > > + .name = "qcom_smd_rpm", > > + .owner = THIS_MODULE, > > Remove this line. >
The module_qcom_smd_driver does not initialize the .owner, but to follow the general direction of the kernel I can add that to the macro...
> > + .of_match_table = qcom_smd_rpm_of_match, > > + }, > > +}; > > + > > +module_qcom_smd_driver(qcom_smd_rpm_driver); > > + > > +MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>"); > > +MODULE_DESCRIPTION("Qualcomm SMD backed RPM driver"); > > +MODULE_LICENSE("GPL v2");
Thanks, Bjorn
| |