mbox series

[v2,0/2] Add support for AK5558 ADC

Message ID 1517588406-3295-1-git-send-email-daniel.baluta@nxp.com
Headers show
Series Add support for AK5558 ADC | expand

Message

Daniel Baluta Feb. 2, 2018, 4:20 p.m. UTC 
We support normal mode, TDM mode and pm.

Changes since v1: [addressed comments from Andy and Fabio]
 * fix GPIO polarity from active high to active low for correct documentation
 * fix license header by using SPDX identifier
 * remove debug prints at the beginning of functions.
 * only support auto clock switching (manual switching was dead code anyway) (in the
   future we could add a DT property to choose between manual and auto)
 * Use gpiod API
 * use GENMASK
 * introduce power_off/power_on

One open question is the resume sequence which appears to need power_off/power_on.
Just power_on alone isn't enough. With just power_on after resume aplay plays a
song for 1 seconds and then the sound stops.

Datasheet says, page 55"

(1) The PDN pin should be held to “L” for more than 150 ns after AVDD and TVDD are powered up.

Daniel Baluta (2):
  ASoC: codecs: Add support for AK5558 ADC driver
  ASoC: ak5558: Add bindings for AK5558 ADC

 Documentation/devicetree/bindings/sound/ak5558.txt |  23 +
 sound/soc/codecs/Kconfig                           |   6 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/ak5558.c                          | 626 +++++++++++++++++++++
 sound/soc/codecs/ak5558.h                          |  52 ++
 5 files changed, 709 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/ak5558.txt
 create mode 100644 sound/soc/codecs/ak5558.c
 create mode 100644 sound/soc/codecs/ak5558.h

Comments

Mark Brown Feb. 2, 2018, 11:11 p.m. UTC | #1 
On Fri, Feb 02, 2018 at 09:33:18PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 2, 2018 at 6:20 PM, Daniel Baluta <daniel.baluta@nxp.com> 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.
Andy Shevchenko Feb. 4, 2018, 2:31 p.m. UTC | #2 
On Sat, Feb 3, 2018 at 1:11 AM, Mark Brown <broonie@kernel.org> 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 <daniel.baluta@nxp.com> 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.
Daniel Baluta Feb. 4, 2018, 2:42 p.m. UTC | #3 
On Sun, Feb 4, 2018 at 4:31 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Feb 3, 2018 at 1:11 AM, Mark Brown <broonie@kernel.org> 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 <daniel.baluta@nxp.com> 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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html