Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753601AbbGXKHJ (ORCPT ); Fri, 24 Jul 2015 06:07:09 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:35481 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753474AbbGXKHC (ORCPT ); Fri, 24 Jul 2015 06:07:02 -0400 Date: Fri, 24 Jul 2015 11:06:57 +0100 From: Lee Jones To: Bjorn Andersson Cc: "bjorn@kryo.se" , Andy Gross , Samuel Ortiz , Mark Brown , "linux-kernel@vger.kernel.org" , "linux-arm-msm@vger.kernel.org" Subject: Re: [PATCH v2 06/11] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD Message-ID: <20150724100657.GD3436@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> <20150723132251.GA3436@x1> <20150723165556.GE4753@usrtlx11787.corpusers.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150723165556.GE4753@usrtlx11787.corpusers.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6205 Lines: 177 On Thu, 23 Jul 2015, Bjorn Andersson wrote: > On Thu 23 Jul 06:22 PDT 2015, Lee Jones wrote: > > > On Mon, 13 Jul 2015, Bjorn Andersson wrote: > > > > > On Tue 07 Jul 05:37 PDT 2015, Lee Jones wrote: > > > > > > > On Fri, 26 Jun 2015, bjorn@kryo.se wrote: > > > > > > > > > From: Bjorn Andersson > > > [..] > > > > > > > > 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. > > > > 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. > > > > We had this exact discussion last year and I argued that a piece of > hardware that exposes regulators and clocks - like most PMICs - is a > mfd and you agreed and picked the driver. I've become stricter since then. An IC which only does power management should either live in drivers/power, or more recently they have been described as platform specific drivers which have subsequently been moved to drivers/platform. Particularly if they have their own special, platform specific communication method/bus. > I will have a word with Andy about moving this and the qcom_rpm driver > out of mfd. Thanks. > > > > > 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? > > > > 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); > > > > The point was simply to not have to write: > > if (msg->length == 23 && memcmp(msg->message, ..., 23); > > Simply because I don't like the first part of the expression. I'll > rewrite it... All this cruft just to avoid that? Just define '23', then code looks good and problem vaporises. > > > > > +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 > > > > Yes, I just noticed that. > > > > > "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 :/ > > > > What about either my memcmp suggestion above or this then: > > > > strncmp(msg->message, "resource does not exist", 23); > > > > That would require the string to be 0-terminated. No it doesn't. strNcmp, only compares the first N characters. > > > > > +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. > > > > 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. > > Because the life cycle of these components are much like, say, USB - > they can come and go. As such e.g. a platform_driver is not a good fit. You mean they are hot-swappable? Don't we have any platform devices which support that already? -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/