Re: [PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id

[Thomas Petazzoni]

Hello,

On Tue, 27 Feb 2018 22:24:30 +0100, MylÃne Josserand wrote:
> To prepare the support for the PCM1789, add a new compatible
> and use the i2c_id to handle, later, the differences between
> these two DACs even if they are pretty similar.
> 
> Signed-off-by: MylÃne Josserand <mylene.josserand@xxxxxxxxxxx>
> ---
>  Documentation/devicetree/bindings/sound/pcm179x.txt | 2 +-

The DT binding change should be in a separate patch.

>  sound/soc/codecs/pcm179x-i2c.c                      | 6 ++++--
>  sound/soc/codecs/pcm179x.c                          | 3 ++-
>  sound/soc/codecs/pcm179x.h                          | 8 +++++++-

And this should be together with the PCM1789 support patch. Otherwise
your series is not bisectable: if we apply only PATCH 1/4, the driver
supports the new compatible string, but it doesn't have the actual code
to handle PCM1789. Am I missing something here ?

> -	return pcm179x_common_init(&client->dev, regmap);
> +	return pcm179x_common_init(&client->dev, regmap, id->driver_data);

This can be done in a preparation patch.

>  }
>  
>  static const struct of_device_id pcm179x_of_match[] = {
>  	{ .compatible = "ti,pcm1792a", },
> +	{ .compatible = "ti,pcm1789", },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, pcm179x_of_match);
>  
>  static const struct i2c_device_id pcm179x_i2c_ids[] = {
> -	{ "pcm179x", 0 },
> +	{ "pcm179x", PCM179X },

And also this addition.

> +	{ "pcm1789", PCM1789 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, pcm179x_i2c_ids);
> diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
> index 4b311c06f97d..81cbf09319f6 100644
> --- a/sound/soc/codecs/pcm179x.c
> +++ b/sound/soc/codecs/pcm179x.c
> @@ -218,7 +218,8 @@ static const struct snd_soc_component_driver soc_component_dev_pcm179x = {
>  	.non_legacy_dai_naming	= 1,
>  };
>  
> -int pcm179x_common_init(struct device *dev, struct regmap *regmap)
> +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
> +			enum pcm17xx_type type)

And this done.

>  {
>  	struct pcm179x_private *pcm179x;
>  
> diff --git a/sound/soc/codecs/pcm179x.h b/sound/soc/codecs/pcm179x.h
> index cf8681c9a373..8c08689e3b8b 100644
> --- a/sound/soc/codecs/pcm179x.h
> +++ b/sound/soc/codecs/pcm179x.h
> @@ -17,11 +17,17 @@
>  #ifndef __PCM179X_H__
>  #define __PCM179X_H__
>  
> +enum pcm17xx_type {
> +	PCM179X,

And this one.

> +	PCM1789,
> +};
> +
>  #define PCM1792A_FORMATS (SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | \
>  			  SNDRV_PCM_FMTBIT_S16_LE)
>  
>  extern const struct regmap_config pcm179x_regmap_config;
>  
> -int pcm179x_common_init(struct device *dev, struct regmap *regmap);
> +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
> +			enum pcm17xx_type type);

And this one. Just as a "preparation patch" to support multiple codecs
in the existing pcm179x.c driver.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com