diff mbox

[V6] ASoC: pcm179x: Add support for pcm1795 and pcm1796

Message ID 20160313085346.GA8896@panicking (mailing list archive)
State New, archived
Headers show

Commit Message

Michael Nazzareno Trimarchi March 13, 2016, 8:53 a.m. UTC
On Sat, Mar 12, 2016 at 12:53:17PM +0700, Mark Brown wrote:
> On Sat, Mar 12, 2016 at 12:29:00AM +0100, Michael Trimarchi wrote:
> 
> > diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
> > index 4118106..609f07f 100644
> > --- a/sound/soc/codecs/pcm179x-i2c.c
> > +++ b/sound/soc/codecs/pcm179x-i2c.c
> > @@ -44,12 +44,6 @@ static int pcm179x_i2c_remove(struct i2c_client *client)
> >  	return pcm179x_common_exit(&client->dev);
> >  }
> >  
> > -static const struct of_device_id pcm179x_of_match[] = {
> > -	{ .compatible = "ti,pcm1792a", },
> > -	{ }
> > -};
> > -MODULE_DEVICE_TABLE(of, pcm179x_of_match);
> > -
> 
> One effect of moving this out of the I2C driver is to break module
> autoloading: currently modutils does not pay attention to the reference
> from the driver structure to the ID table but instead just looks at the
> MODULE_DEVICE_TABLE.  This is a problem with some existing devices but
> we shouldn't introduce new instances.
> 
> I don't actually know if MODULE_DEVICE_TABLE can reference an object in
> a different file off the top of my head, though a brief test suggests it
> has issues.
> 
Does this approch can work?

> > +static int pcm179x_startup(struct snd_pcm_substream *substream,
> > +			    struct snd_soc_dai *dai)
> > +{
> > +	struct snd_soc_codec *codec = dai->codec;
> > +	struct pcm179x_private *priv = snd_soc_codec_get_drvdata(codec);
> > +	u64 formats = PCM1792A_FORMATS;
> > +
> > +	switch (priv->codec_model) {
> > +	case PCM1795:
> > +		formats = PCM1795_FORMATS;
> > +		break;
> 
> It would be more data driven to just register a different DAI structure
> for the 1795.  Both approaches work, it's just a little nicer to avoid
> code where possible.

Yes I know but my idea was exactly to not create special DAI because
technically they are the same code with diffferent constraints. This even
give an other example of the usage of the API.

Michael

Comments

Michael Nazzareno Trimarchi March 13, 2016, 9:10 a.m. UTC | #1
On Sun, Mar 13, 2016 at 09:53:46AM +0100, Michael Trimarchi wrote:
> On Sat, Mar 12, 2016 at 12:53:17PM +0700, Mark Brown wrote:
> > On Sat, Mar 12, 2016 at 12:29:00AM +0100, Michael Trimarchi wrote:
> > 
> > > diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
> > > index 4118106..609f07f 100644
> > > --- a/sound/soc/codecs/pcm179x-i2c.c
> > > +++ b/sound/soc/codecs/pcm179x-i2c.c
> > > @@ -44,12 +44,6 @@ static int pcm179x_i2c_remove(struct i2c_client *client)
> > >  	return pcm179x_common_exit(&client->dev);
> > >  }
> > >  
> > > -static const struct of_device_id pcm179x_of_match[] = {
> > > -	{ .compatible = "ti,pcm1792a", },
> > > -	{ }
> > > -};
> > > -MODULE_DEVICE_TABLE(of, pcm179x_of_match);
> > > -
> > 
> > One effect of moving this out of the I2C driver is to break module
> > autoloading: currently modutils does not pay attention to the reference
> > from the driver structure to the ID table but instead just looks at the
> > MODULE_DEVICE_TABLE.  This is a problem with some existing devices but
> > we shouldn't introduce new instances.
> > 
> > I don't actually know if MODULE_DEVICE_TABLE can reference an object in
> > a different file off the top of my head, though a brief test suggests it
> > has issues.
> > 
> Does this approch can work?
> 

I need to test, as you said I don't have feeling. I found only this commit

commit 5e4cb7b60833b0124a9f71dbc5118144ca79c3c4
Author: Axel Lin <axel.lin@ingics.com>
Date:   Thu Jun 25 21:44:13 2015 +0800

    ASoC: cs42xx8: Setup of_match_table
    
    Setup of_match_table and since cs42xx8_of_match is exported and used in
    cs42xx8-i2c.c, it cannot be static.
    
    Signed-off-by: Axel Lin <axel.lin@ingics.com>
    Acked-by: Brian Austin <brian.austin@cirrus.com>
    Signed-off-by: Mark Brown <broonie@kernel.org>

Seems a similar approch

Michael

> diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
> index 609f07f..6bed54f 100644
> --- a/sound/soc/codecs/pcm179x-i2c.c
> +++ b/sound/soc/codecs/pcm179x-i2c.c
> @@ -49,6 +49,7 @@ static const struct i2c_device_id pcm179x_i2c_ids[] = {
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, pcm179x_i2c_ids);
> +MODULE_DEVICE_TABLE(of, pcm179x_of_match);
>  
>  static struct i2c_driver pcm179x_i2c_driver = {
>  	.driver = {
> diff --git a/sound/soc/codecs/pcm179x-spi.c b/sound/soc/codecs/pcm179x-spi.c
> index 6ae0e4d..556c875 100644
> --- a/sound/soc/codecs/pcm179x-spi.c
> +++ b/sound/soc/codecs/pcm179x-spi.c
> @@ -48,6 +48,7 @@ static const struct spi_device_id pcm179x_spi_ids[] = {
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(spi, pcm179x_spi_ids);
> +MODULE_DEVICE_TABLE(of, pcm179x_of_match);
>  
>  static struct spi_driver pcm179x_spi_driver = {
>  	.driver = {
> diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
> index fb01822..a34af5b 100644
> --- a/sound/soc/codecs/pcm179x.c
> +++ b/sound/soc/codecs/pcm179x.c
> @@ -255,7 +255,6 @@ const struct of_device_id pcm179x_of_match[] = {
>  	{ .compatible = "ti,pcm1796", },
>  	{ }
>  };
> -MODULE_DEVICE_TABLE(of, pcm179x_of_match);
>  EXPORT_SYMBOL_GPL(pcm179x_of_match);
>  
>  int pcm179x_common_init(struct device *dev, struct regmap *regmap)
> > > +static int pcm179x_startup(struct snd_pcm_substream *substream,
> > > +			    struct snd_soc_dai *dai)
> > > +{
> > > +	struct snd_soc_codec *codec = dai->codec;
> > > +	struct pcm179x_private *priv = snd_soc_codec_get_drvdata(codec);
> > > +	u64 formats = PCM1792A_FORMATS;
> > > +
> > > +	switch (priv->codec_model) {
> > > +	case PCM1795:
> > > +		formats = PCM1795_FORMATS;
> > > +		break;
> > 
> > It would be more data driven to just register a different DAI structure
> > for the 1795.  Both approaches work, it's just a little nicer to avoid
> > code where possible.
> 
> Yes I know but my idea was exactly to not create special DAI because
> technically they are the same code with diffferent constraints. This even
> give an other example of the usage of the API.
> 
> Michael
> 
> 
> -- 
> | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> | COO  -  Founder                                      Cruquiuskade 47 |
> | +31(0)851119172                                 Amsterdam 1018 AM NL |
> |                  [`as] http://www.amarulasolutions.com               |
Mark Brown March 14, 2016, 4:24 p.m. UTC | #2
On Sun, Mar 13, 2016 at 09:53:50AM +0100, Michael Trimarchi wrote:
> On Sat, Mar 12, 2016 at 12:53:17PM +0700, Mark Brown wrote:
> > On Sat, Mar 12, 2016 at 12:29:00AM +0100, Michael Trimarchi wrote:

> > I don't actually know if MODULE_DEVICE_TABLE can reference an object in
> > a different file off the top of my head, though a brief test suggests it
> > has issues.

> Does this approch can work?

I'd suggest testing...

> > It would be more data driven to just register a different DAI structure
> > for the 1795.  Both approaches work, it's just a little nicer to avoid
> > code where possible.

> Yes I know but my idea was exactly to not create special DAI because
> technically they are the same code with diffferent constraints. This even
> give an other example of the usage of the API.

They would still share all the same code, it's just moving the decision
to probe time given that this is a trivial constraint change and the
constraints are listed in the data structure.
diff mbox

Patch

diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
index 609f07f..6bed54f 100644
--- a/sound/soc/codecs/pcm179x-i2c.c
+++ b/sound/soc/codecs/pcm179x-i2c.c
@@ -49,6 +49,7 @@  static const struct i2c_device_id pcm179x_i2c_ids[] = {
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pcm179x_i2c_ids);
+MODULE_DEVICE_TABLE(of, pcm179x_of_match);
 
 static struct i2c_driver pcm179x_i2c_driver = {
 	.driver = {
diff --git a/sound/soc/codecs/pcm179x-spi.c b/sound/soc/codecs/pcm179x-spi.c
index 6ae0e4d..556c875 100644
--- a/sound/soc/codecs/pcm179x-spi.c
+++ b/sound/soc/codecs/pcm179x-spi.c
@@ -48,6 +48,7 @@  static const struct spi_device_id pcm179x_spi_ids[] = {
 	{ },
 };
 MODULE_DEVICE_TABLE(spi, pcm179x_spi_ids);
+MODULE_DEVICE_TABLE(of, pcm179x_of_match);
 
 static struct spi_driver pcm179x_spi_driver = {
 	.driver = {
diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
index fb01822..a34af5b 100644
--- a/sound/soc/codecs/pcm179x.c
+++ b/sound/soc/codecs/pcm179x.c
@@ -255,7 +255,6 @@  const struct of_device_id pcm179x_of_match[] = {
 	{ .compatible = "ti,pcm1796", },
 	{ }
 };
-MODULE_DEVICE_TABLE(of, pcm179x_of_match);
 EXPORT_SYMBOL_GPL(pcm179x_of_match);
 
 int pcm179x_common_init(struct device *dev, struct regmap *regmap)