Re: [PATCH 05/10] ARM: bcm2835: Add the mailbox power channel driver.

On 03/02/2015 01:54 PM, Eric Anholt wrote:

This just enables the power to the USB controller, so that DWC2 can
initialize.

The downstream tree has an interface to this channel for tracking
enables from multiple clients, except it doesn't have any clients as
far as I can see.  For now, just make the simplest thing that gets USB
working.

 drivers/mailbox/Makefile                |   1 +
 drivers/mailbox/bcm2835-mailbox-power.c | 127 ++++++++++++++++++++++++++++++++

Along the lines of my comments on the previous patch, I would expect
this driver to show up within the directory for the subsystem/API that
it implements (power domains, regulators?)

+#define BCM_POWER_USB (1 << 3)

I'd expect that to be encoded in DT, in the manner I mentioned in reply
to patch 4, so that this driver can work for arbitrary clients.

+#define BCM_MBOX_DATA_SHIFT 4

I'd expect that to be defined in some public header that's part of patch
2, so that clients don't have to duplicate the definition.

+/*
+ * Submits a set of concatenated tags to the VPU firmware through the
+ * mailbox power interface.
+ *
+ * The buffer header and the ending tag are added by this function and
+ * don't need to be supplied, just the actual tags for your operation.
+ * See struct bcm_mbox_power_tag_header for the per-tag structure.
+ */
+static int bcm_mbox_set_power(uint32_t power_enables)
+{
+	int ret;
+
+	reinit_completion(&mbox_power->c);
+	ret = mbox_send_message(mbox_power->chan,
+				(void *)(power_enables << BCM_MBOX_DATA_SHIFT));
+	if (ret >= 0)
+		wait_for_completion(&mbox_power->c);
+
+	return ret;
+}

The comment sounds more like the property mailbox interface/channel,
whereas the code appears to be using the non-property channel. In
particular, the code only appears to be sending one "tag"/message,
without the header or ending tag mentioned in the comment.

+static int bcm2835_mbox_power_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret = 0;
+
+	mbox_power = devm_kzalloc(dev, sizeof(*mbox_power),
+				       GFP_KERNEL);
+	if (!mbox_power) {
+		dev_err(dev, "Failed to allocate device memory\n");

Duplicate error message.

+	/* Enable power to the USB phy. */
+	if (IS_ENABLED(CONFIG_USB_DWC2)) {
+		bcm_mbox_set_power(BCM_POWER_USB);
+		bcm2835_mbox_power_initialized = true;
+	}

Hmm. Shouldn't the DWC2 driver make a call to do that?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html