Date: Mon, 13 Jul 2015 14:58:25 -0700
From: Bjorn Andersson <bjorn.andersson@sonymobile.com>
To: Lee Jones <lee.jones@linaro.org>
CC: <bjorn@kryo.se>, Andy Gross <agross@codeaurora.org>,
        Samuel Ortiz <sameo@linux.intel.com>, Mark Brown <broonie@kernel.org>,
        <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: <20150713215825.GB15178@usrtlx11787.corpusers.net>
References: <1435355419-23602-1-git-send-email-bjorn.andersson@sonymobile.com>
 <1435355419-23602-7-git-send-email-bjorn.andersson@sonymobile.com>
 <20150707123716.GA3182@x1>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20150707123716.GA3182@x1>
User-Agent: Mutt/1.5.22 (2013-10-16)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3950
Lines: 139

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
--
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/