Re: [PATCH v2 06/11] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM over SMD

[Lee Jones <lee.jones@linaro.org>]

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