From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lee Jones Subject: Re: [PATCH v2 06/11] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD Date: Thu, 23 Jul 2015 14:22:51 +0100 Message-ID: <20150723132251.GA3436@x1> References: <1435355419-23602-1-git-send-email-bjorn.andersson@sonymobile.com> <1435355419-23602-7-git-send-email-bjorn.andersson@sonymobile.com> <20150707123716.GA3182@x1> <20150713215825.GB15178@usrtlx11787.corpusers.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-wi0-f169.google.com ([209.85.212.169]:36968 "EHLO mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751768AbbGWNW5 (ORCPT ); Thu, 23 Jul 2015 09:22:57 -0400 Received: by wibud3 with SMTP id ud3so218880612wib.0 for ; Thu, 23 Jul 2015 06:22:56 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20150713215825.GB15178@usrtlx11787.corpusers.net> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Bjorn Andersson Cc: bjorn@kryo.se, Andy Gross , Samuel Ortiz , Mark Brown , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org On Mon, 13 Jul 2015, Bjorn Andersson wrote: > On Tue 07 Jul 05:37 PDT 2015, Lee Jones wrote: >=20 > > On Fri, 26 Jun 2015, bjorn@kryo.se wrote: > >=20 > > > From: Bjorn Andersson > [..] >=20 > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >=20 > [..] >=20 > > > +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 t= he > > > + Resource Power Manager system found in the Qualcomm 8974 base= d > > > + 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 RP= M as a > > > + module. This will build a module called "qcom-smd-rpm". > >=20 > > I'm not exactly sure what makes this an MFD device. > >=20 >=20 > It represents a piece of hardware (a micro-controller) that exposes > control of a multitude of regulators and clocks in the Qualcomm > platforms. >=20 > It's basically just a successor of the qcom_rpm driver - same > functionality but a new communication method is used. My point still stands. Please investigate moving this (and the qcom_rpm driver if it's the same) into either drivers/soc or drivers/platform. The support in these two directories _seem_ to be pretty similar. > > > diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rp= m.c >=20 > [..] >=20 > > > + > > > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist" > >=20 > > I don't like this at all. > >=20 >=20 > Which part of it? >=20 > It should probably be a static const char *, inlined in the function > below. Would that be to your liking? It would be better, but I never really see the point in initialising variables with these types of messages. I'd get rid of the superfluous chuff and just do: memcmp(msg->message, "resource does not exist", 23); > > > +static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev, > > > + const void *data, > > > + size_t count) > > > +{ > > > + const struct qcom_rpm_header *hdr =3D data; > > > + const struct qcom_rpm_message *msg; > > > + const size_t inv_res_len =3D sizeof(RPM_ERR_INVALID_RESOURCE) -= 1; > > > + struct qcom_smd_rpm *rpm =3D dev_get_drvdata(&qsdev->dev); > > > + const u8 *buf =3D data + sizeof(struct qcom_rpm_header); > > > + const u8 *end =3D buf + hdr->length; > > > + int status =3D 0; > > > + > > > + if (hdr->service_type !=3D RPM_SERVICE_TYPE_REQUEST || > > > + hdr->length < sizeof(struct qcom_rpm_message)) { > > > + dev_err(&qsdev->dev, "invalid request\n"); > > > + return 0; > > > + } > > > + > > > + while (buf < end) { > > > + msg =3D (struct qcom_rpm_message *)buf; > > > + switch (msg->msg_type) { > > > + case RPM_MSG_TYPE_MSG_ID: > > > + break; > > > + case RPM_MSG_TYPE_ERR: > > > + if (msg->length =3D=3D inv_res_len && > > > + !memcmp(msg->message, > > > + RPM_ERR_INVALID_RESOURCE, > > > + inv_res_len)) > >=20 > > strncpy(msg->message, "resource does not exist", 23); > >=20 >=20 > No, I want to compare the content of msg->message with the string Yes, I just noticed that. > "resource does not exist" - as that's the only way to know what type = of > error we got. >=20 > This is unfortunately how the protocol looks :/ What about either my memcmp suggestion above or this then: strncmp(msg->message, "resource does not exist", 23); > > > + status =3D -ENXIO; > > > + else > > > + status =3D -EIO; > > > + break; > > > + } > > > + > > > + buf =3D PTR_ALIGN(buf + 2 * sizeof(u32) + msg->length, 4); > > > + } > > > + > > > + rpm->ack_status =3D status; > > > + complete(&rpm->ack); > > > + return 0; > > > +} > > > + >=20 > [..] >=20 > > > + > > > +static struct qcom_smd_driver qcom_smd_rpm_driver =3D { > > > + .probe =3D qcom_smd_rpm_probe, > > > + .remove =3D qcom_smd_rpm_remove, > > > + .callback =3D qcom_smd_rpm_callback, > > > + .driver =3D { > > > + .name =3D "qcom_smd_rpm", > > > + .owner =3D THIS_MODULE, > >=20 > > Remove this line. Still not 100% sure why you need your own 'special' driver struct. If it's for the .callback, there are other ways to do this without having to invent your own bus. > The module_qcom_smd_driver does not initialize the .owner, but to fol= low > the general direction of the kernel I can add that to the macro... >=20 > > > + .of_match_table =3D qcom_smd_rpm_of_match, > > > + }, > > > +}; > > > + > > > +module_qcom_smd_driver(qcom_smd_rpm_driver); > > > + > > > +MODULE_AUTHOR("Bjorn Andersson "= ); > > > +MODULE_DESCRIPTION("Qualcomm SMD backed RPM driver"); > > > +MODULE_LICENSE("GPL v2"); >=20 > Thanks, > Bjorn --=20 Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org =E2=94=82 Open source software for ARM SoCs =46ollow Linaro: Facebook | Twitter | Blog