Re: [alsa-devel] [PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id
Hello,
On Thu, 1 Mar 2018 10:04:11 +0100
Michael Nazzareno Trimarchi wrote:
> Hi
>
> On Thu, Mar 1, 2018 at 8:43 AM, Mylène Josserand
> wrote:
> > Hello,
> >
> > Thank you for the review.
> >
> > On Tue, 27 Feb 2018 22:51:40 +0100
> > Thomas Petazzoni wrote:
> >
> >> 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
> >> > ---
> >> > 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 ?
> >
> > No, you are right, I will merge it with patch 02.
> >
>
> Can you please include me in next series?
Sure, I will do it.
>
> I have several hardware running on pcm179x family
Currently, some colleagues recommend me to create a new file for this
driver instead of adding it in PCM179x driver. I think that I will not
touch pcm179x driver anymore in my next series.
Best regards,
Mylène
>
> Michael
>
> >>
> >> > - 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
> >
> > Thanks,
> >
> > Mylène
> >
> > --
> > Mylène Josserand, Bootlin (formerly Free Electrons)
> > Embedded Linux and Kernel engineering
> > http://bootlin.com
> > ___
> > Alsa-devel mailing list
> > alsa-de...@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
>
>
--
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
Re: [alsa-devel] [PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id
Hi
On Thu, Mar 1, 2018 at 8:43 AM, Mylène Josserand
wrote:
> Hello,
>
> Thank you for the review.
>
> On Tue, 27 Feb 2018 22:51:40 +0100
> Thomas Petazzoni wrote:
>
>> 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
>> > ---
>> > 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 ?
>
> No, you are right, I will merge it with patch 02.
>
Can you please include me in next series?
I have several hardware running on pcm179x family
Michael
>>
>> > - 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
>
> Thanks,
>
> Mylène
>
> --
> Mylène Josserand, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> http://bootlin.com
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
--
| Michael Nazzareno Trimarchi Amarula Solutions BV |
| COO - Founder Cruquiuskade 47 |
| +31(0)851119172 Amsterdam 1018 AM NL |
| [`as] http://www.amarulasolutions.com |
Re: [PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id
Hello,
Thank you for the review.
On Tue, 27 Feb 2018 22:51:40 +0100
Thomas Petazzoni wrote:
> 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
> > ---
> > 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 ?
No, you are right, I will merge it with patch 02.
>
> > - 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
Thanks,
Mylène
--
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
Re: [PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id
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
> ---
> 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
[PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id
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
---
Documentation/devicetree/bindings/sound/pcm179x.txt | 2 +-
sound/soc/codecs/pcm179x-i2c.c | 6 --
sound/soc/codecs/pcm179x.c | 3 ++-
sound/soc/codecs/pcm179x.h | 8 +++-
4 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/pcm179x.txt
b/Documentation/devicetree/bindings/sound/pcm179x.txt
index 436c2b247693..bf725d302958 100644
--- a/Documentation/devicetree/bindings/sound/pcm179x.txt
+++ b/Documentation/devicetree/bindings/sound/pcm179x.txt
@@ -4,7 +4,7 @@ This driver supports both the I2C and SPI bus.
Required properties:
- - compatible: "ti,pcm1792a"
+ - compatible: "ti,pcm1792a" or "ti,pcm1789"
For required properties on SPI, please consult
Documentation/devicetree/bindings/spi/spi-bus.txt
diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
index 03747966c6bc..795a0657c097 100644
--- a/sound/soc/codecs/pcm179x-i2c.c
+++ b/sound/soc/codecs/pcm179x-i2c.c
@@ -36,17 +36,19 @@ static int pcm179x_i2c_probe(struct i2c_client *client,
return ret;
}
- return pcm179x_common_init(&client->dev, regmap);
+ return pcm179x_common_init(&client->dev, regmap, id->driver_data);
}
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 },
+ { "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)
{
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,
+ 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);
#endif
--
2.11.0
