Re: [PATCH 1/3] mfd: add Intel MSIC driver

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Wed, Sep 21, 2011 at 12:44:56PM +0200, Samuel Ortiz wrote:
> Hi Mika,
> 
> On Wed, Sep 21, 2011 at 11:07:17AM +0300, Mika Westerberg wrote:
> > > > +/*
> > > > + * Other MSIC related devices which are not directly available via SFI DEVS
> > > > + * table. 
> > > Would that mean a broken firmware, or something else I'm missing ?
> > > It looks odd.
> > 
> > Not all devices in the MSIC are listed in SFI DEVS table. For example
> > regulators are missing. We currently don't have regulator support for MSIC
> > but once it is added the devices will be created here.
> > 
> > Audio codec is similar - it does not exist in the SFI DEVS table.
> Ok. So I can expect further patches removing this one then.

For the regulators and audio codec, I'm not sure whether we ever get those
added to the SFI table. But we know that they will be embedded in the MSIC
chip.

It might be the case that we need to add there more stuff - regulators come
into mind when we get driver for them.

> 
> > > > +/**
> > > > + * intel_msic_reg_read - read a single MSIC register
> > > > + * @reg: register to read
> > > > + * @val: register value is placed here
> > > > + *
> > > > + * Read a single register from MSIC. Returns %0 on success and negative
> > > > + * errno in case of failure.
> > > > + *
> > > > + * Function may sleep.
> > > > + */
> > > > +int intel_msic_reg_read(unsigned short reg, u8 *val)
> > > > +{
> > > > +	return intel_scu_ipc_ioread8(reg, val);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_msic_reg_read);
> > > > +
> > > > +/**
> > > > + * intel_msic_reg_write - write a single MSIC register
> > > > + * @reg: register to write
> > > > + * @val: value to write to that register
> > > > + *
> > > > + * Write a single MSIC register. Returns 0 on success and negative
> > > > + * errno in case of failure.
> > > > + *
> > > > + * Function may sleep.
> > > > + */
> > > > +int intel_msic_reg_write(unsigned short reg, u8 val)
> > > > +{
> > > > +	return intel_scu_ipc_iowrite8(reg, val);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_msic_reg_write);
> > > What is the benefit of those wrappers since you're probably not going to get
> > > rid of the SCU IPC APIs, right ? What's wrong with the subdevices using the
> > > SCU IPC API directly ?
> > 
> > Right, we are not going to get rid of SCU IPC APIs. The idea behind having
> > these register access wrapper functions is that we are now able to separate
> > the MSIC subdevices from other SCU IPC usage. In other words we get a bit
> > cleaner "architecture".
> I'm not entirely convinced that wrapping around the IPC API gives you a
> cleaner architecture.

Ok.

> 
>  
> > It also allows us to add caching and intercepting register accesses if a
> > need rises.
> That would define a real need for this API, yes.

Ok.

> 
> 
> > > > +	for (i = 0; i < ARRAY_SIZE(msic_devs); i++) {
> > > > +		if (!pdata->irq[i])
> > > > +			continue;
> > > I would expect some sort of warning here since it would mean your platform
> > > code defined a sub device platform data without giving you the right IRQ. And
> > > afaiu, all of your sub devices must have one.
> > 
> > The interface for platform data says that if irq is zero, it means that no
> > platform device is created.
> > 
> > For example in patch 3/3 we add the platform data for battery, gpio, audio,
> > power_button and ocd but the SFI table actually contains more devices. We
> > don't yet have (upstream) driver for those so there is a little point of
> > creating platform device for them at this point.
> 
> Ok, I see.
> 
> So, I'm fine with this patch, at least the MFD part. I'll go ahead and apply
> it to my for-next branch. I'd appreciate to get ACKs or NACKs for the x86
> parts from the relevant people though.

Thanks.

I would also like to hear what x86 maintainers think about those parts.
Hmm, is there some mailing list which I missed or did forgot to Cc someone?