Re: [PATCH V2 1/2] mfd: Add driver for STA2X11 MFD block

[Samuel Ortiz <sameo@linux.intel.com>]

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/