lkml.org 
[lkml]   [2011]   [Sep]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] mfd: add Intel MSIC driver
Hi Mika,

On Wed, Sep 07, 2011 at 04:06:50PM +0300, Mika Westerberg wrote:
> Add support for Intel MSIC chip found on Intel Medfield platforms. This
> chip embeds several subdevices: audio, ADC, GPIO, power button, etc. The
> driver creates platform device for each subdevice.
>
> We also provide an MSIC register access API which should replace the more
> generic SCU IPC interface currently used. Existing drivers can choose
> whether they convert to this new API or stick with the SCU IPC interface.
Looks good overall, I just have a few comments/questions:


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


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


> +/**
> + * intel_msic_bulk_read - read an array of registers
> + * @reg: array of register addresses to read
> + * @buf: array where the read values are placed
> + * @count: number of registers to read
> + *
> + * Function reads @count registers from the MSIC using addresses passed in
> + * @reg. Read values are placed in @buf. Reads are performed atomically
> + * wrt. MSIC.
> + *
> + * Returns %0 in case of success and negative errno in case of failure.
> + *
> + * Function may sleep.
> + */
> +int intel_msic_bulk_read(unsigned short *reg, u8 *buf, size_t count)
> +{
> + if (WARN_ON(count > SCU_IPC_RWBUF_LIMIT))
> + return -EINVAL;
> +
> + return intel_scu_ipc_readv(reg, buf, count);
> +}
> +EXPORT_SYMBOL_GPL(intel_msic_bulk_read);
> +
> +/**
> + * intel_msic_bulk_write - write an array of values to the MSIC registers
> + * @reg: array of registers to write
> + * @buf: values to write to each register
> + * @count: number of registers to write
> + *
> + * Function writes @count registers in @buf to MSIC. Writes are performed
> + * atomically wrt MSIC. Returns %0 in case of success and negative errno in
> + * case of failure.
> + *
> + * Function may sleep.
> + */
> +int intel_msic_bulk_write(unsigned short *reg, u8 *buf, size_t count)
> +{
> + if (WARN_ON(count > SCU_IPC_RWBUF_LIMIT))
> + return -EINVAL;
> +
> + return intel_scu_ipc_writev(reg, buf, count);
> +}
> +EXPORT_SYMBOL_GPL(intel_msic_bulk_write);
The 2 routines above make more sense.



> +static int __devinit intel_msic_init_devices(struct intel_msic *msic)
> +{
> + struct platform_device *pdev = msic->pdev;
> + struct intel_msic_platform_data *pdata = pdev->dev.platform_data;
> + int ret, i;
> +
> + if (pdata->gpio) {
> + struct mfd_cell *cell = &msic_devs[INTEL_MSIC_BLOCK_GPIO];
> +
> + cell->platform_data = pdata->gpio;
> + cell->pdata_size = sizeof(*pdata->gpio);
> + }
> +
> + if (pdata->ocd) {
> + unsigned gpio = pdata->ocd->gpio;
> +
> + ret = gpio_request_one(gpio, GPIOF_IN, "ocd_gpio");
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register OCD GPIO\n");
> + return ret;
> + }
> +
> + ret = gpio_to_irq(gpio);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "no IRQ number for OCD GPIO\n");
> + gpio_free(gpio);
> + return ret;
> + }
> +
> + /* Update the IRQ number for the OCD */
> + pdata->irq[INTEL_MSIC_BLOCK_OCD] = ret;
> + }
> +
> + 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.

Cheers,
Samuel.

--
Intel Open Source Technology Centre
http://oss.intel.com/


\
 
 \ /
  Last update: 2011-09-20 19:17    [W:0.373 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site