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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEnXRPt=8bZ1VxasNqdzY_vL=kwsxz9n=c_GsACMbMqaSARa5Q@mail.gmail.com>
Date:	Fri, 22 Jan 2016 13:17:58 +0100
From:	Jacob Siverskog <jacob@...nage.engineering>
To:	Johan Hovold <johan@...nel.org>
Cc:	Michael Trimarchi <michael@...rulasolutions.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>,
	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] ASoC: pcm179x: Split into core and SPI parts

On Thu, Jan 21, 2016 at 6:56 PM, Johan Hovold <johan@...nel.org> wrote:
> On Thu, Jan 21, 2016 at 05:27:58PM +0100, Michael Trimarchi wrote:
>> Hi
>>
>> On Thu, Jan 21, 2016 at 4:36 PM, Johan Hovold <johan@...nel.org> 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@...nel.org>
>> >> Signed-off-by: Jacob Siverskog <jacob@...nage.engineering>
>> >> ---
>> >
>> >> 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
>
>> >> -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

Ok.

>>
>> >> +
>> >> +     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?
>
> I'd say, only if you also add a codec probe callback and use it from
> there. The other codec drivers appear to be consistent on that.

I agree. Using snd_soc_codec_set_drvdata would mean larger logical
changes that I don't really see the purpose of doing. All other codec
drivers that are separated in a similar fashion as this patch use
dev_set_drvdata.