Re: [PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-04 Thread Daniel Baluta 
On Sun, Feb 4, 2018 at 4:31 PM, Andy Shevchenko
 wrote:
> On Sat, Feb 3, 2018 at 1:11 AM, Mark Brown  wrote:
>> On Fri, Feb 02, 2018 at 09:33:18PM +0200, Andy Shevchenko wrote:
>>> On Fri, Feb 2, 2018 at 6:20 PM, Daniel Baluta  wrote:
>>
>>> > +static int ak5558_set_dai_mute(struct snd_soc_dai *dai, int mute)
>>> > +{
>>> > +   struct snd_soc_codec *codec = dai->codec;
>>> > +   struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec);
>>>
>>> > +   int ndt = 0;
>>>
>>> It might be even
>>>
>>>   int ndt = max(ak5558->fs ? 583000 / ak5558->fs : 5, 5);
>>
>> Please don't encourage people to use the ternery operator like that, it
>> does nothing for legibility not to write out the conditionals.
>
> Noted.
>
>>> > +static const struct i2c_device_id ak5558_i2c_id[] = {
>>> > +   { "ak5558", 0 },
>>> > +   { }
>>> > +};
>>> > +MODULE_DEVICE_TABLE(i2c, ak5558_i2c_id);
>>
>>> I dunno if it's really helpful to have. Though it's up to Mark and you.
>>
>> I don't care either way.
>
> Since Mark is okay with either, I would rather switch to ->probe_new()
> and remove above table.

Sure, will do. I forgot to put it in v1.

Re: [PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-04 Thread Daniel Baluta 
On Sun, Feb 4, 2018 at 4:31 PM, Andy Shevchenko
 wrote:
> On Sat, Feb 3, 2018 at 1:11 AM, Mark Brown  wrote:
>> On Fri, Feb 02, 2018 at 09:33:18PM +0200, Andy Shevchenko wrote:
>>> On Fri, Feb 2, 2018 at 6:20 PM, Daniel Baluta  wrote:
>>
>>> > +static int ak5558_set_dai_mute(struct snd_soc_dai *dai, int mute)
>>> > +{
>>> > +   struct snd_soc_codec *codec = dai->codec;
>>> > +   struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec);
>>>
>>> > +   int ndt = 0;
>>>
>>> It might be even
>>>
>>>   int ndt = max(ak5558->fs ? 583000 / ak5558->fs : 5, 5);
>>
>> Please don't encourage people to use the ternery operator like that, it
>> does nothing for legibility not to write out the conditionals.
>
> Noted.
>
>>> > +static const struct i2c_device_id ak5558_i2c_id[] = {
>>> > +   { "ak5558", 0 },
>>> > +   { }
>>> > +};
>>> > +MODULE_DEVICE_TABLE(i2c, ak5558_i2c_id);
>>
>>> I dunno if it's really helpful to have. Though it's up to Mark and you.
>>
>> I don't care either way.
>
> Since Mark is okay with either, I would rather switch to ->probe_new()
> and remove above table.

Sure, will do. I forgot to put it in v1.

Re: [PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-04 Thread Andy Shevchenko 
On Sat, Feb 3, 2018 at 1:11 AM, Mark Brown  wrote:
> On Fri, Feb 02, 2018 at 09:33:18PM +0200, Andy Shevchenko wrote:
>> On Fri, Feb 2, 2018 at 6:20 PM, Daniel Baluta  wrote:
>
>> > +static int ak5558_set_dai_mute(struct snd_soc_dai *dai, int mute)
>> > +{
>> > +   struct snd_soc_codec *codec = dai->codec;
>> > +   struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec);
>>
>> > +   int ndt = 0;
>>
>> It might be even
>>
>>   int ndt = max(ak5558->fs ? 583000 / ak5558->fs : 5, 5);
>
> Please don't encourage people to use the ternery operator like that, it
> does nothing for legibility not to write out the conditionals.

Noted.

>> > +static const struct i2c_device_id ak5558_i2c_id[] = {
>> > +   { "ak5558", 0 },
>> > +   { }
>> > +};
>> > +MODULE_DEVICE_TABLE(i2c, ak5558_i2c_id);
>
>> I dunno if it's really helpful to have. Though it's up to Mark and you.
>
> I don't care either way.

Since Mark is okay with either, I would rather switch to ->probe_new()
and remove above table.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-04 Thread Andy Shevchenko 
On Sat, Feb 3, 2018 at 1:11 AM, Mark Brown  wrote:
> On Fri, Feb 02, 2018 at 09:33:18PM +0200, Andy Shevchenko wrote:
>> On Fri, Feb 2, 2018 at 6:20 PM, Daniel Baluta  wrote:
>
>> > +static int ak5558_set_dai_mute(struct snd_soc_dai *dai, int mute)
>> > +{
>> > +   struct snd_soc_codec *codec = dai->codec;
>> > +   struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec);
>>
>> > +   int ndt = 0;
>>
>> It might be even
>>
>>   int ndt = max(ak5558->fs ? 583000 / ak5558->fs : 5, 5);
>
> Please don't encourage people to use the ternery operator like that, it
> does nothing for legibility not to write out the conditionals.

Noted.

>> > +static const struct i2c_device_id ak5558_i2c_id[] = {
>> > +   { "ak5558", 0 },
>> > +   { }
>> > +};
>> > +MODULE_DEVICE_TABLE(i2c, ak5558_i2c_id);
>
>> I dunno if it's really helpful to have. Though it's up to Mark and you.
>
> I don't care either way.

Since Mark is okay with either, I would rather switch to ->probe_new()
and remove above table.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-02 Thread Mark Brown 
On Fri, Feb 02, 2018 at 09:33:18PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 2, 2018 at 6:20 PM, Daniel Baluta  wrote:

> > +static int ak5558_set_dai_mute(struct snd_soc_dai *dai, int mute)
> > +{
> > +   struct snd_soc_codec *codec = dai->codec;
> > +   struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec);
> 
> > +   int ndt = 0;
> 
> It might be even
> 
>   int ndt = max(ak5558->fs ? 583000 / ak5558->fs : 5, 5);

Please don't encourage people to use the ternery operator like that, it
does nothing for legibility not to write out the conditionals.

> > +static const struct i2c_device_id ak5558_i2c_id[] = {
> > +   { "ak5558", 0 },
> > +   { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ak5558_i2c_id);

> I dunno if it's really helpful to have. Though it's up to Mark and you.

I don't care either way.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-02 Thread Mark Brown 
On Fri, Feb 02, 2018 at 09:33:18PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 2, 2018 at 6:20 PM, Daniel Baluta  wrote:

> > +static int ak5558_set_dai_mute(struct snd_soc_dai *dai, int mute)
> > +{
> > +   struct snd_soc_codec *codec = dai->codec;
> > +   struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec);
> 
> > +   int ndt = 0;
> 
> It might be even
> 
>   int ndt = max(ak5558->fs ? 583000 / ak5558->fs : 5, 5);

Please don't encourage people to use the ternery operator like that, it
does nothing for legibility not to write out the conditionals.

> > +static const struct i2c_device_id ak5558_i2c_id[] = {
> > +   { "ak5558", 0 },
> > +   { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ak5558_i2c_id);

> I dunno if it's really helpful to have. Though it's up to Mark and you.

I don't care either way.


signature.asc
Description: PGP signature

[PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-02 Thread Daniel Baluta 
AK5558 is a 32-bit, 768 kHZ sampling, differential input ADC
for digital audio systems.

Datasheet is available at:

https://www.akm.com/akm/en/file/datasheet/AK5558VN.pdf

Initial patch includes support for normal and TDM modes.

Signed-off-by: Junichi Wakasugi 
[initial coding for 3.18 kernel]
Signed-off-by: Mihai Serban 
[cleanups and porting to 4.9 kernel]
Signed-off-by: Shengjiu Wang 
[tdm support]
Signed-off-by: Daniel Baluta 
[pm support, cleanups and porting to latest kernel]
---
 sound/soc/codecs/Kconfig  |   6 +
 sound/soc/codecs/Makefile |   2 +
 sound/soc/codecs/ak5558.c | 626 ++
 sound/soc/codecs/ak5558.h |  52 
 4 files changed, 686 insertions(+)
 create mode 100644 sound/soc/codecs/ak5558.c
 create mode 100644 sound/soc/codecs/ak5558.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 2b331f7..c29728c 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -42,6 +42,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_AK4642 if I2C
select SND_SOC_AK4671 if I2C
select SND_SOC_AK5386
+   select SND_SOC_AK5558 if I2C
select SND_SOC_ALC5623 if I2C
select SND_SOC_ALC5632 if I2C
select SND_SOC_BT_SCO
@@ -398,6 +399,11 @@ config SND_SOC_AK4671
 config SND_SOC_AK5386
tristate "AKM AK5638 CODEC"
 
+config SND_SOC_AK5558
+   tristate "AKM AK5558 CODEC"
+   depends on I2C
+   select REGMAP_I2C
+
 config SND_SOC_ALC5623
tristate "Realtek ALC5623 CODEC"
depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index da15713..3e71d40 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -34,6 +34,7 @@ snd-soc-ak4641-objs := ak4641.o
 snd-soc-ak4642-objs := ak4642.o
 snd-soc-ak4671-objs := ak4671.o
 snd-soc-ak5386-objs := ak5386.o
+snd-soc-ak5558-objs := ak5558.o
 snd-soc-arizona-objs := arizona.o
 snd-soc-bt-sco-objs := bt-sco.o
 snd-soc-cq93vc-objs := cq93vc.o
@@ -277,6 +278,7 @@ obj-$(CONFIG_SND_SOC_AK4641)+= snd-soc-ak4641.o
 obj-$(CONFIG_SND_SOC_AK4642)   += snd-soc-ak4642.o
 obj-$(CONFIG_SND_SOC_AK4671)   += snd-soc-ak4671.o
 obj-$(CONFIG_SND_SOC_AK5386)   += snd-soc-ak5386.o
+obj-$(CONFIG_SND_SOC_AK5558)   += snd-soc-ak5558.o
 obj-$(CONFIG_SND_SOC_ALC5623)+= snd-soc-alc5623.o
 obj-$(CONFIG_SND_SOC_ALC5632)  += snd-soc-alc5632.o
 obj-$(CONFIG_SND_SOC_ARIZONA)  += snd-soc-arizona.o
diff --git a/sound/soc/codecs/ak5558.c b/sound/soc/codecs/ak5558.c
new file mode 100644
index 000..74993cf
--- /dev/null
+++ b/sound/soc/codecs/ak5558.c
@@ -0,0 +1,626 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ak5558.c  --  audio driver for AK5558 ADC
+ *
+ * Copyright (C) 2015 Asahi Kasei Microdevices Corporation
+ * Copyright 2018 NXP
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "ak5558.h"
+
+/* AK5558 Codec Private Data */
+struct ak5558_priv {
+   struct snd_soc_codec codec;
+   struct regmap *regmap;
+   struct i2c_client *i2c;
+   int fs; /* Sampling Frequency */
+   int rclk;   /* Master Clock */
+   struct gpio_desc *reset_gpiod; /* Reset & Power down GPIO */
+   int slots;
+   int slot_width;
+};
+
+/* ak5558 register cache & default register settings */
+static const struct reg_default ak5558_reg[] = {
+   { 0x0, 0xFF },  /*  0x00AK5558_00_POWER_MANAGEMENT1 */
+   { 0x1, 0x01 },  /*  0x01AK5558_01_POWER_MANAGEMENT2 */
+   { 0x2, 0x01 },  /*  0x02AK5558_02_CONTROL1  */
+   { 0x3, 0x00 },  /*  0x03AK5558_03_CONTROL2  */
+   { 0x4, 0x00 },  /*  0x04AK5558_04_CONTROL3  */
+   { 0x5, 0x00 }   /*  0x05AK5558_05_DSD   */
+};
+
+static const char * const mono_texts[] = {
+   "8 Slot", "2 Slot", "4 Slot", "1 Slot",
+};
+
+static const struct soc_enum ak5558_mono_enum[] = {
+   SOC_ENUM_SINGLE(AK5558_01_POWER_MANAGEMENT2, 1,
+   ARRAY_SIZE(mono_texts), mono_texts)
+};
+
+static const char * const tdm_texts[] = {
+   "Off", "TDM128",  "TDM256", "TDM512",
+};
+
+static const char * const digfil_texts[] = {
+   "Sharp Roll-Off", "Show Roll-Off",
+   "Short Delay Sharp Roll-Off", "Short Delay Show Roll-Off",
+};
+
+static const struct soc_enum ak5558_adcset_enum[] = {
+   SOC_ENUM_SINGLE(AK5558_03_CONTROL2, 5,
+   ARRAY_SIZE(tdm_texts), tdm_texts),
+   SOC_ENUM_SINGLE(AK5558_04_CONTROL3, 0,
+   ARRAY_SIZE(digfil_texts), digfil_texts),
+};
+
+static const char * const dsdon_texts[] = {
+   "PCM", "DSD",
+};
+
+static const char * const dsdsel_texts[] = {
+   "64fs", "128fs", "256fs"
+};
+
+static const

[PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-02 Thread Daniel Baluta 
AK5558 is a 32-bit, 768 kHZ sampling, differential input ADC
for digital audio systems.

Datasheet is available at:

https://www.akm.com/akm/en/file/datasheet/AK5558VN.pdf

Initial patch includes support for normal and TDM modes.

Signed-off-by: Junichi Wakasugi 
[initial coding for 3.18 kernel]
Signed-off-by: Mihai Serban 
[cleanups and porting to 4.9 kernel]
Signed-off-by: Shengjiu Wang 
[tdm support]
Signed-off-by: Daniel Baluta 
[pm support, cleanups and porting to latest kernel]
---
 sound/soc/codecs/Kconfig  |   6 +
 sound/soc/codecs/Makefile |   2 +
 sound/soc/codecs/ak5558.c | 626 ++
 sound/soc/codecs/ak5558.h |  52 
 4 files changed, 686 insertions(+)
 create mode 100644 sound/soc/codecs/ak5558.c
 create mode 100644 sound/soc/codecs/ak5558.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 2b331f7..c29728c 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -42,6 +42,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_AK4642 if I2C
select SND_SOC_AK4671 if I2C
select SND_SOC_AK5386
+   select SND_SOC_AK5558 if I2C
select SND_SOC_ALC5623 if I2C
select SND_SOC_ALC5632 if I2C
select SND_SOC_BT_SCO
@@ -398,6 +399,11 @@ config SND_SOC_AK4671
 config SND_SOC_AK5386
tristate "AKM AK5638 CODEC"
 
+config SND_SOC_AK5558
+   tristate "AKM AK5558 CODEC"
+   depends on I2C
+   select REGMAP_I2C
+
 config SND_SOC_ALC5623
tristate "Realtek ALC5623 CODEC"
depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index da15713..3e71d40 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -34,6 +34,7 @@ snd-soc-ak4641-objs := ak4641.o
 snd-soc-ak4642-objs := ak4642.o
 snd-soc-ak4671-objs := ak4671.o
 snd-soc-ak5386-objs := ak5386.o
+snd-soc-ak5558-objs := ak5558.o
 snd-soc-arizona-objs := arizona.o
 snd-soc-bt-sco-objs := bt-sco.o
 snd-soc-cq93vc-objs := cq93vc.o
@@ -277,6 +278,7 @@ obj-$(CONFIG_SND_SOC_AK4641)+= snd-soc-ak4641.o
 obj-$(CONFIG_SND_SOC_AK4642)   += snd-soc-ak4642.o
 obj-$(CONFIG_SND_SOC_AK4671)   += snd-soc-ak4671.o
 obj-$(CONFIG_SND_SOC_AK5386)   += snd-soc-ak5386.o
+obj-$(CONFIG_SND_SOC_AK5558)   += snd-soc-ak5558.o
 obj-$(CONFIG_SND_SOC_ALC5623)+= snd-soc-alc5623.o
 obj-$(CONFIG_SND_SOC_ALC5632)  += snd-soc-alc5632.o
 obj-$(CONFIG_SND_SOC_ARIZONA)  += snd-soc-arizona.o
diff --git a/sound/soc/codecs/ak5558.c b/sound/soc/codecs/ak5558.c
new file mode 100644
index 000..74993cf
--- /dev/null
+++ b/sound/soc/codecs/ak5558.c
@@ -0,0 +1,626 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ak5558.c  --  audio driver for AK5558 ADC
+ *
+ * Copyright (C) 2015 Asahi Kasei Microdevices Corporation
+ * Copyright 2018 NXP
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "ak5558.h"
+
+/* AK5558 Codec Private Data */
+struct ak5558_priv {
+   struct snd_soc_codec codec;
+   struct regmap *regmap;
+   struct i2c_client *i2c;
+   int fs; /* Sampling Frequency */
+   int rclk;   /* Master Clock */
+   struct gpio_desc *reset_gpiod; /* Reset & Power down GPIO */
+   int slots;
+   int slot_width;
+};
+
+/* ak5558 register cache & default register settings */
+static const struct reg_default ak5558_reg[] = {
+   { 0x0, 0xFF },  /*  0x00AK5558_00_POWER_MANAGEMENT1 */
+   { 0x1, 0x01 },  /*  0x01AK5558_01_POWER_MANAGEMENT2 */
+   { 0x2, 0x01 },  /*  0x02AK5558_02_CONTROL1  */
+   { 0x3, 0x00 },  /*  0x03AK5558_03_CONTROL2  */
+   { 0x4, 0x00 },  /*  0x04AK5558_04_CONTROL3  */
+   { 0x5, 0x00 }   /*  0x05AK5558_05_DSD   */
+};
+
+static const char * const mono_texts[] = {
+   "8 Slot", "2 Slot", "4 Slot", "1 Slot",
+};
+
+static const struct soc_enum ak5558_mono_enum[] = {
+   SOC_ENUM_SINGLE(AK5558_01_POWER_MANAGEMENT2, 1,
+   ARRAY_SIZE(mono_texts), mono_texts)
+};
+
+static const char * const tdm_texts[] = {
+   "Off", "TDM128",  "TDM256", "TDM512",
+};
+
+static const char * const digfil_texts[] = {
+   "Sharp Roll-Off", "Show Roll-Off",
+   "Short Delay Sharp Roll-Off", "Short Delay Show Roll-Off",
+};
+
+static const struct soc_enum ak5558_adcset_enum[] = {
+   SOC_ENUM_SINGLE(AK5558_03_CONTROL2, 5,
+   ARRAY_SIZE(tdm_texts), tdm_texts),
+   SOC_ENUM_SINGLE(AK5558_04_CONTROL3, 0,
+   ARRAY_SIZE(digfil_texts), digfil_texts),
+};
+
+static const char * const dsdon_texts[] = {
+   "PCM", "DSD",
+};
+
+static const char * const dsdsel_texts[] = {
+   "64fs", "128fs", "256fs"
+};
+
+static const char * const dckb_texts[] = {
+   "Falling", "Rising",
+};
+
+static const char * const

Re: [PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-02 Thread Andy Shevchenko 
On Fri, Feb 2, 2018 at 6:20 PM, Daniel Baluta  wrote:
> AK5558 is a 32-bit, 768 kHZ sampling, differential input ADC
> for digital audio systems.
>
> Datasheet is available at:
>
> https://www.akm.com/akm/en/file/datasheet/AK5558VN.pdf
>
> Initial patch includes support for normal and TDM modes.
>
> Signed-off-by: Junichi Wakasugi 
> [initial coding for 3.18 kernel]
> Signed-off-by: Mihai Serban 
> [cleanups and porting to 4.9 kernel]
> Signed-off-by: Shengjiu Wang 
> [tdm support]
> Signed-off-by: Daniel Baluta 
> [pm support, cleanups and porting to latest kernel]

Thanks for an update.

Couple of nitpicks below, otherwise FWIW,
Reviewed-by: Andy Shevchenko 


> @@ -0,0 +1,626 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*

> + * ak5558.c  --  audio driver for AK5558 ADC

I would remove filename. If file is ever be renamed this will make an
additional noise.

> + *
> + * Copyright (C) 2015 Asahi Kasei Microdevices Corporation
> + * Copyright 2018 NXP
> + */

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

I would rather keep it sorted

 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
  // yeah keep an empty line here to split groups
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 

+#include 

This one is redundant.

> +static int ak5558_set_dai_mute(struct snd_soc_dai *dai, int mute)
> +{
> +   struct snd_soc_codec *codec = dai->codec;
> +   struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec);

> +   int ndt = 0;

It might be even

  int ndt = max(ak5558->fs ? 583000 / ak5558->fs : 5, 5);

> +   if (!mute)
> +   return 0;
> +

> +   if (ak5558->fs != 0)
> +   ndt = 583000 / ak5558->fs;
> +
> +   msleep(max(ndt, 5));

...and here just

   msleep(ndt);

But I don't know if Mark is okay with that.

> +
> +   return 0;
> +}

> +   pm_runtime_enable(>dev);
> +   pm_runtime_disable(>dev);

I'm also not sure about these calls, but I leave it to you.

> +static const struct i2c_device_id ak5558_i2c_id[] = {
> +   { "ak5558", 0 },
> +   { }
> +};
> +MODULE_DEVICE_TABLE(i2c, ak5558_i2c_id);

I dunno if it's really helpful to have. Though it's up to Mark and you.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] ASoC: codecs: Add support for AK5558 ADC driver

2018-02-02 Thread Andy Shevchenko 
On Fri, Feb 2, 2018 at 6:20 PM, Daniel Baluta  wrote:
> AK5558 is a 32-bit, 768 kHZ sampling, differential input ADC
> for digital audio systems.
>
> Datasheet is available at:
>
> https://www.akm.com/akm/en/file/datasheet/AK5558VN.pdf
>
> Initial patch includes support for normal and TDM modes.
>
> Signed-off-by: Junichi Wakasugi 
> [initial coding for 3.18 kernel]
> Signed-off-by: Mihai Serban 
> [cleanups and porting to 4.9 kernel]
> Signed-off-by: Shengjiu Wang 
> [tdm support]
> Signed-off-by: Daniel Baluta 
> [pm support, cleanups and porting to latest kernel]

Thanks for an update.

Couple of nitpicks below, otherwise FWIW,
Reviewed-by: Andy Shevchenko 


> @@ -0,0 +1,626 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*

> + * ak5558.c  --  audio driver for AK5558 ADC

I would remove filename. If file is ever be renamed this will make an
additional noise.

> + *
> + * Copyright (C) 2015 Asahi Kasei Microdevices Corporation
> + * Copyright 2018 NXP
> + */

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

I would rather keep it sorted

 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
  // yeah keep an empty line here to split groups
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 

+#include 

This one is redundant.

> +static int ak5558_set_dai_mute(struct snd_soc_dai *dai, int mute)
> +{
> +   struct snd_soc_codec *codec = dai->codec;
> +   struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec);

> +   int ndt = 0;

It might be even

  int ndt = max(ak5558->fs ? 583000 / ak5558->fs : 5, 5);

> +   if (!mute)
> +   return 0;
> +

> +   if (ak5558->fs != 0)
> +   ndt = 583000 / ak5558->fs;
> +
> +   msleep(max(ndt, 5));

...and here just

   msleep(ndt);

But I don't know if Mark is okay with that.

> +
> +   return 0;
> +}

> +   pm_runtime_enable(>dev);
> +   pm_runtime_disable(>dev);

I'm also not sure about these calls, but I leave it to you.

> +static const struct i2c_device_id ak5558_i2c_id[] = {
> +   { "ak5558", 0 },
> +   { }
> +};
> +MODULE_DEVICE_TABLE(i2c, ak5558_i2c_id);

I dunno if it's really helpful to have. Though it's up to Mark and you.

-- 
With Best Regards,
Andy Shevchenko