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

[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

[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

[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

[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

[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

[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

[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

[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