| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20110920171742.GU32263@sortiz-mobl>
Date: Tue, 20 Sep 2011 19:17:42 +0200
From: Samuel Ortiz <sameo@...ux.intel.com>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc: linux-kernel@...r.kernel.org, alan@...ux.intel.com,
tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
feng.tang@...el.com, jacob.jun.pan@...ux.intel.com
Subject: Re: [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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists