lkml.org 
[lkml]   [2012]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V2 1/2] mfd: Add driver for STA2X11 MFD block
Hi Alessandro,

A few minor comments, since it seems you're going to re-spin this one:

On Thu, Feb 16, 2012 at 02:00:40PM +0100, Alessandro Rubini wrote:
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
> Passage) chip. This chip embeds audio, battery, GPIO, etc.
> devices used in Intel Medfield platforms.
>
> +config MFD_STA2X11
> + bool "STA2X11 multi function device support"
> + depends on STA2X11
> + select MFD_CORE
> + select GPIO_STA2X11
I think it's better to have your GPIO driver depend on MFD_STA2X11. With the
above code, you could end up trying to build your GPIO driver with GPIOLIB not
being selected.


> +/*
> + * What follows is the PCI device that hosts the above two pdevs.
> + * Each logic block is 4kB and they are all consecutive: we use this info.
> + */
> +
> +/* Bar 0 */
> +enum bar0_cells {
> + GPIO_0 = 0,
> + GPIO_1,
> + GPIO_2,
> + GPIO_3,
> + SCTL,
> + SCR,
> + TIME,
As Linus said, I'd protect that with a namespace.


> +static int __devinit sta2x11_mfd_probe(struct pci_dev *pdev,
> + const struct pci_device_id *pci_id)
> +{
> + int err, i;
> + struct sta2x11_gpio_pdata *gpio_data;
> +
> + dev_info(&pdev->dev, "%s\n", __func__);
> +
> + err = pci_enable_device(pdev);
> + if (err) {
> + dev_err(&pdev->dev, "Can't enable device.\n");
> + return err;
> + }
> +
> + err = pci_enable_msi(pdev);
> + if (err)
> + dev_info(&pdev->dev, "Enable msi failed\n");
> +
> + /* Read gpio config data as pci device's platform data */
> + gpio_data = dev_get_platdata(&pdev->dev);
> + if (!gpio_data)
> + dev_warn(&pdev->dev, "no gpio configuration\n");
> +
> +#if 1
> + dev_dbg(&pdev->dev, "%s, gpio_data = %p (%p)\n", __func__,
> + gpio_data, &gpio_data);
> +#endif
You can get rid of the #if #endif here.


> + dev_dbg(&pdev->dev, "%s, pdev = %p (%p)\n", __func__,
> + pdev, &pdev);
> +
Extra line here


> + /* platform data is the pci device for all of them */
> + for (i = 0; i < ARRAY_SIZE(sta2x11_mfd_bar0); i++) {
> + sta2x11_mfd_bar0[i].pdata_size = sizeof(pdev);
> + sta2x11_mfd_bar0[i].platform_data = &pdev;
> + }
> + sta2x11_mfd_bar1[0].pdata_size = sizeof(pdev);
> + sta2x11_mfd_bar1[0].platform_data = &pdev;
> +
> + /* Record this pdev before mfd_add_devices: their probe looks for it */
> + sta2x11_mfd_add(pdev, GFP_ATOMIC);
> +
> +
> + err = mfd_add_devices(&pdev->dev, -1,
> + sta2x11_mfd_bar0,
> + ARRAY_SIZE(sta2x11_mfd_bar0),
> + &pdev->resource[0],
> + 0);
> + if (err) {
> + dev_err(&pdev->dev, "mfd_add_devices[0] failed: %d\n", err);
> + goto err_disable;
> + }
> +
> + err = mfd_add_devices(&pdev->dev, -1,
> + sta2x11_mfd_bar1,
> + ARRAY_SIZE(sta2x11_mfd_bar1),
> + &pdev->resource[1],
> + 0);
> + if (err) {
> + dev_err(&pdev->dev, "mfd_add_devices[1] failed: %d\n", err);
> + goto err_disable;
> + }
> +
> + return 0;
> +
> +err_disable:
> + pci_disable_device(pdev);
> + pci_disable_msi(pdev);
> + /* mfd_remove(pdev); -- it is in an exit section, I can't call it */
Why can't you call mfd_remove_devices() here ?

Cheers,
Samuel.

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


\
 
 \ /
  Last update: 2012-02-23 17:21    [W:0.157 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site