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

[Mark Brown <broonie@kernel.org>]

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

On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
> On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:

> >> +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?

That certainly looks like what it's doing.  Probably also make the name
that gets passed in const while you're at it.

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

The messiness is orthogonal to my comment here - either it's legal to
request a mailbox without a device or it isn't, it shouldn't depend on a
random kernel configuration option for a particular mailbox driver which
it is.

> "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)

Arguably that's what things like cpufreq end up doing, though people
tend to just shove a device into DT.  Are you sure there isn't any
device at all in ACPI that you could hang this off, looking at my
desktop I see rather a lot of apparently synthetic ACPI devices with
names starting LNX including for example LNXSYSTM:00?

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

If PCC is described by ACPI tables how would non-ACPI users be able to
use it?

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