Hi Mark,
On 27 August 2014 06:27, Mark Brown <broonie@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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?
Call this mbox_find_controller_by_name() instead?
>
>> /* 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.
So this is where it got really messy. We're trying to create a
"device" out of something that isn't. The PCCT, which is used as a
mailbox controller here, is a table and not a peripheral device. To
treat this as a device (without faking it by manually putting together
a struct device), would require adding a DSDT entry which is really a
wrong place for it. Are there examples today where drivers manually
create a struct driver and struct device and match them internally?
(i.e. w/o using the generic driver subsystem)
The main reason why I thought this Mailbox framework looked useful
(after you pointed me to it) for PCC was due to its async notification
features. But thats easy and small enough to add to the PCC driver
itself. We can also add a generic controller lookup mechanism in the
PCC driver for anyone who doesn't want to use ACPI. I think thats a
much cleaner way to handle PCC support. Adding PCC as a generic
mailbox controller is turning out to be more messier that we'd
originally imagined.
Cheers,
Ashwin
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
