Re: [PATCH v3 1/3] ASoC: pcm179x: Split into core and SPI parts

[Michael Trimarchi]

Hi

On Thu, Jan 21, 2016 at 4:36 PM, Johan Hovold <johan@xxxxxxxxxx> wrote:
> On Thu, Jan 21, 2016 at 04:26:56PM +0100, Jacob Siverskog wrote:
>> The pcm179x family supports both SPI and I2C for configuration. This
>> patch splits the driver into core and SPI parts, in preparation for
>> I2C support.
>>
>> Reviewed-by: Johan Hovold <johan@xxxxxxxxxx>
>> Signed-off-by: Jacob Siverskog <jacob@xxxxxxxxxxxxxxxxxxx>
>> ---
>
>> diff --git a/sound/soc/codecs/pcm179x-spi.c b/sound/soc/codecs/pcm179x-spi.c
>> new file mode 100644
>> index 0000000..5842add9
>> --- /dev/null
>> +++ b/sound/soc/codecs/pcm179x-spi.c
>> @@ -0,0 +1,63 @@
>> +/*
>> + * PCM179X ASoC SPI driver
>> + *
>> + * Copyright (c) Amarula Solutions B.V. 2013
>> + *
>> + *     Michael Trimarchi <michael@xxxxxxxxxxxxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "pcm179x.h"
>> +
>> +static int pcm179x_spi_probe(struct spi_device *spi)
>> +{
>> +     return pcm179x_common_init(&spi->dev,
>> +                     devm_regmap_init_spi(spi, &pcm179x_regmap_config));
>> +}
>> +
>
>> -static int pcm179x_spi_probe(struct spi_device *spi)
>> +int pcm179x_common_init(struct device *dev, struct regmap *regmap)
>>  {
>>       struct pcm179x_private *pcm179x;
>>       int ret;
>>
>> -     pcm179x = devm_kzalloc(&spi->dev, sizeof(struct pcm179x_private),
>> +     if (IS_ERR(regmap)) {
>> +             ret = PTR_ERR(regmap);
>> +             dev_err(dev, "Failed to register regmap: %d\n", ret);
>> +             return ret;
>> +     }
>
> This looks weird. I think you should check for error where you do the
> allocation even if this means a four more lines of code in total.
>

agree on that

>> +
>> +     pcm179x = devm_kzalloc(dev, sizeof(struct pcm179x_private),
>>                               GFP_KERNEL);
>>       if (!pcm179x)
>>               return -ENOMEM;
>>
>> -     spi_set_drvdata(spi, pcm179x);
>> -
>> -     pcm179x->regmap = devm_regmap_init_spi(spi, &pcm179x_regmap);
>> -     if (IS_ERR(pcm179x->regmap)) {
>> -             ret = PTR_ERR(pcm179x->regmap);
>> -             dev_err(&spi->dev, "Failed to register regmap: %d\n", ret);
>> -             return ret;
>> -     }
>> +     pcm179x->regmap = regmap;
>> +     dev_set_drvdata(dev, pcm179x);
>>

snd_soc_codec_set_drvdata

Is this more "codec" like?

Michael

>> -     return snd_soc_register_codec(&spi->dev,
>> +     return snd_soc_register_codec(dev,
>>                       &soc_codec_dev_pcm179x, &pcm179x_dai, 1);
>>  }
>
> Thanks,
> Johan



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |