Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1744 bytes --]

On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:

> The PCC (Platform Communication Channel) is a generic
> mailbox to be used by PCC clients such as CPPC, RAS
> and MPST as defined in the ACPI 5.0+ spec. This patch
> modifies the Mailbox framework to lookup PCC mailbox
> controllers and channels such that PCC drivers can work
> with or without ACPI support in the kernel.
> 
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>  drivers/mailbox/mailbox.c          | 118 ++++++++++++++++++++++++++++++++++---
>  include/linux/mailbox_client.h     |   6 ++
>  include/linux/mailbox_controller.h |   1 +
>  3 files changed, 118 insertions(+), 7 deletions(-)

Based on the patch description (adding support for a particular kind of
mailbox) I'd expect to see a new driver or helper library being added to
drivers/mailbox rather than changes in the mailbox core.  If changes in
the core are needed to support this I'd expect to see them broken out as
separate patches.

> +static struct mbox_controller *
> +mbox_find_pcc_controller(char *name)
> +{
> +	struct mbox_controller *mbox;
> +	list_for_each_entry(mbox, &mbox_cons, node) {
> +		if (mbox->name)
> +			if (!strcmp(mbox->name, name))
> +				return mbox;
> +	}
> +
> +	return NULL;
> +}

This doesn't look particularly PCC specific?

>  	/* Sanity check */
> -	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
> +
> +	/*
> +	 * PCC clients and controllers are currently not backed by
> +	 * platform device structures.
> +	 */
> +#ifndef CONFIG_PCC
> +	if (!mbox->dev)
> +		return -EINVAL;
> +#endif

It seems better to make this consistent - either enforce it all the time
or don't enforce it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]