Re: [PATCH v9 08/10] ASoC: Add phycore-ac97-dt driver

2013-07-08 Thread Mark Brown 
On Sun, Jul 07, 2013 at 09:19:49AM +0200, Markus Pargmann wrote:
 On Wed, Jul 03, 2013 at 05:17:27PM +0100, Mark Brown wrote:

   +extern void pca100_ac97_cold_reset(struct snd_ac97 *ac97);
   +extern void pca100_ac97_warm_reset(struct snd_ac97 *ac97);
   +#else
   +static void pca100_ac97_cold_reset(struct snd_ac97 *ac97) { }
   +static void pca100_ac97_warm_reset(struct snd_ac97 *ac97) { }

  These functions have no reason to be anywhere except in the driver and
  really you should just be specifying which pins to use there - ideally
  via pinctrl but I don't think i.MX has adopted that yet.

 There are no pinctrl driver for imx27(pca100) or imx35(pcm043). The
 iomux/pad configure functions are defined inside mach-imx/ including
 their headers. Both reset functions have to configure iomux/pad before
 and after using gpio. So I think it's not possible to make the reset
 based on the gpio pins without the necessary iomux/pad configuration.

That all sounds fixable - either make pinctrl available or make the
headers usefully visible.  You can see the arch headers in the build
of the entire kernel...


signature.asc
Description: Digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v9 08/10] ASoC: Add phycore-ac97-dt driver

2013-07-07 Thread Markus Pargmann 
On Wed, Jul 03, 2013 at 05:17:27PM +0100, Mark Brown wrote:
 On Thu, Jun 20, 2013 at 03:20:27PM +0200, Markus Pargmann wrote:
 
  Notes:
  Changes in v9:
   - Fix blank line at end of file.
  
 
 Please don't include enormous changelogs like this, they're just noise.
 
  +config SND_SOC_PHYCORE_AC97_DT
  +   bool SoC Audio support for Phytec phyCORE (and phyCARD) boards 
  (devicetree only)
  +   depends on MACH_PCA100 || MACH_PCM043
 
 Is there an actual dependency on the machine type?  This seems wrong for
 a DT driver.

No, there is no dependency. Fixed.

 
  +static struct snd_soc_ops imx_phycore_hifi_ops = {
  +};
  +
 
 You shouldn't have this if there's nothing in it.

Removed.

 
  +static struct platform_device *imx_phycore_snd_device;
 
 This shouldn't be a global, it should be in driver data.

Moved to driver data.

 
  +/*
  + * Pointer to AC97 reset functions for specific boards
  + */
  +#if IS_ENABLED(CONFIG_MACH_PCA100)
  +extern void pca100_ac97_cold_reset(struct snd_ac97 *ac97);
  +extern void pca100_ac97_warm_reset(struct snd_ac97 *ac97);
  +#else
  +static void pca100_ac97_cold_reset(struct snd_ac97 *ac97) { }
  +static void pca100_ac97_warm_reset(struct snd_ac97 *ac97) { }
  +#endif
 
  +   if (of_machine_is_compatible(phytec,imx27-pca100)) {
  +   phycore_ac97_reset = pca100_ac97_cold_reset;
  +   phycore_ac97_warm_reset = pca100_ac97_warm_reset;
  +   } else if (of_machine_is_compatible(phytec,imx35-pcm043)) {
  +   phycore_ac97_reset = pcm043_ac97_cold_reset;
  +   phycore_ac97_warm_reset = pcm043_ac97_warm_reset;
  +   } else {
  +   dev_err(pdev-dev, Failed to set AC97 reset functions, 
  unknown board.\n);
  +   return -EINVAL;
  +   }
 
 These functions have no reason to be anywhere except in the driver and
 really you should just be specifying which pins to use there - ideally
 via pinctrl but I don't think i.MX has adopted that yet.

There are no pinctrl driver for imx27(pca100) or imx35(pcm043). The
iomux/pad configure functions are defined inside mach-imx/ including
their headers. Both reset functions have to configure iomux/pad before
and after using gpio. So I think it's not possible to make the reset
based on the gpio pins without the necessary iomux/pad configuration.

Thanks,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v9 08/10] ASoC: Add phycore-ac97-dt driver

2013-07-03 Thread Mark Brown 
On Thu, Jun 20, 2013 at 03:20:27PM +0200, Markus Pargmann wrote:

 Notes:
 Changes in v9:
  - Fix blank line at end of file.
 

Please don't include enormous changelogs like this, they're just noise.

 +config SND_SOC_PHYCORE_AC97_DT
 + bool SoC Audio support for Phytec phyCORE (and phyCARD) boards 
 (devicetree only)
 + depends on MACH_PCA100 || MACH_PCM043

Is there an actual dependency on the machine type?  This seems wrong for
a DT driver.

 +static struct snd_soc_ops imx_phycore_hifi_ops = {
 +};
 +

You shouldn't have this if there's nothing in it.

 +static struct platform_device *imx_phycore_snd_device;

This shouldn't be a global, it should be in driver data.

 +/*
 + * Pointer to AC97 reset functions for specific boards
 + */
 +#if IS_ENABLED(CONFIG_MACH_PCA100)
 +extern void pca100_ac97_cold_reset(struct snd_ac97 *ac97);
 +extern void pca100_ac97_warm_reset(struct snd_ac97 *ac97);
 +#else
 +static void pca100_ac97_cold_reset(struct snd_ac97 *ac97) { }
 +static void pca100_ac97_warm_reset(struct snd_ac97 *ac97) { }
 +#endif

 + if (of_machine_is_compatible(phytec,imx27-pca100)) {
 + phycore_ac97_reset = pca100_ac97_cold_reset;
 + phycore_ac97_warm_reset = pca100_ac97_warm_reset;
 + } else if (of_machine_is_compatible(phytec,imx35-pcm043)) {
 + phycore_ac97_reset = pcm043_ac97_cold_reset;
 + phycore_ac97_warm_reset = pcm043_ac97_warm_reset;
 + } else {
 + dev_err(pdev-dev, Failed to set AC97 reset functions, 
 unknown board.\n);
 + return -EINVAL;
 + }

These functions have no reason to be anywhere except in the driver and
really you should just be specifying which pins to use there - ideally
via pinctrl but I don't think i.MX has adopted that yet.


signature.asc
Description: Digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss