Re: [PATCH 8/9] mfd: wm97xx-core: core support for wm97xx Codec

[Robert Jarzmik]

Lee Jones <lee.jones@xxxxxxxxxx> writes:

> Mark, please see below:
You'll get better chances to have an answer if you put Mark in the To: list.

Mark, Lee has question, especially in the part where he wrote "I'd like to get
Mark Brown's opinion on this.". I added the code extract in [1] to spare you
going through the all patch.

I copy-pasted his reply below in [2], which makes it top-posting ... let's say
for this time it's acceptable.

Cheers.

--
Robert

[1] Probe function and your opinion
	+static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
	+{
	+	struct wm97xx_priv *wm97xx;
	+	int ret;
	+	void *pdata = snd_ac97_codec_get_platdata(adev);
	+
	+	wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
	+			      sizeof(*wm97xx), GFP_KERNEL);
	+	if (!wm97xx)
	+		return -ENOMEM;
	+
	+	wm97xx->dev = ac97_codec_dev2dev(adev);
	+	wm97xx->ac97 = snd_ac97_compat_alloc(adev);
	+	if (IS_ERR(wm97xx->ac97))
	+		return PTR_ERR(wm97xx->ac97);
	+
	+
	+	ac97_set_drvdata(adev, wm97xx);
	+	dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
	+		 adev->vendor_id);

[2] Lee's previous mail
> On Sat, 19 Nov 2016, Robert Jarzmik wrote:
>> Lee Jones <lee.jones@xxxxxxxxxx> writes:
>> 
>> >> +#define WM9705_VENDOR_ID 0x574d4c05
>> >> +#define WM9712_VENDOR_ID 0x574d4c12
>> >> +#define WM9713_VENDOR_ID 0x574d4c13
>> >> +#define WM97xx_VENDOR_ID_MASK 0xffffffff
>> >
>> > These are probably better represented as enums.
>> These are ids, just as devicetree ids, or PCI ids, I don't think an enum will
>> fit.
>
> That's fine.  We can use enums to simply group items of a similar
> type.  Representing these as enums would only serve to benefit code
> cleanliness.  What makes you think they won't fit?
>
>> >> +struct wm97xx_priv {
>> >> +	struct regmap *regmap;
>> >> +	struct snd_ac97 *ac97;
>> >> +	struct device *dev;
>> >> +};
>> >
>> > Replace _priv with _ddata.
>> Ok, will do, would you care to explain if this is something specific to your mfd
>> tree, or is it a kernel overall recommendation ?
>
> It's personal preference.  But these aren't necessarily private, so
> priv doesn't really make a great deal of sense.  These types of saved
> pointers are better described as device data (ddata).
>
>
> [...]
>
>> >> +static const struct reg_default wm97xx_reg_defaults[] = {
>> >> +};
>> >
>> > What's the point in this?
>> Ah, that's a reminder I have still more work on this patch ... Either I remove
>> support for wm9705 and wm9712 in the first version, or I complete it and add the
>> tables :
>>  - wm9705_reg_defaults
>>  - wm9712_reg_defaults
>
> Please don't add redundant code.  I suggest you remove it for this
> submission.
>
>> >> +static int wm9705_register(struct wm97xx_priv *wm97xx)
>> >> +{
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static int wm9712_register(struct wm97xx_priv *wm97xx)
>> >> +{
>> >> +	return 0;
>> >> +}
>> >
>> > I don't get it?
>> >
>> > Either populate or don't provide.
>> Same story as above, either I complete wm9705 and wm9712 support or I remove it.
>
> Remove it please.
>
>> >> +static int wm9713_register(struct wm97xx_priv *wm97xx,
>> >> +			   struct wm97xx_pdata *pdata)
>> >> +{
>> >
>> > What are you lining this up with?
>> Emacs ... I don't see your point though, seeing it not aligned in the diff chunk
>> doesn't mean it's not properly aligned.
>
> That is true.  So the two "scruct"s are line up in the source file,
> right?
>
> [...]
>
>> >> +	codec_pdata.ac97 = wm97xx->ac97;
>> >> +	codec_pdata.regmap = devm_regmap_init_ac97(wm97xx->ac97,
>> >> +						   &wm9713_regmap_config);
>> >> +	codec_pdata.batt_pdata = pdata->batt_pdata;
>> >> +	if (IS_ERR(codec_pdata.regmap))
>> >> +		return PTR_ERR(codec_pdata.regmap);
>> >
>> > This doesn't look like pdata.  You can get rid of all of this hoop
>> > jumping if you add it to ddata and set it as such.
>> You mean I should pass ddata to mfd sub-cells, right ?
>
> Correct.
>
> [...]
>
>> >> +static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
>> >
>> > This looks sound specific.
>> >
>> > Why isn't this a plane platform driver?
>> That's the whole purpose of the patch serie.
>> 
>> This serie transforms the wm97xx drivers from a platform driver model to an ac97
>> model, where the ac97 devices are automatically discovered. The long explanation
>> is in patch 0/9, on how it started and what is the global purpose.
>> 
>> The short story is : switch to the new AC97 bus driver model.
>> 
>> As for the "sound specific part", it's because AC97 bus is mainly used in sound
>> oriented drivers, but still the codec IPs provide more than just sound, as the
>> Wolfson codecs for instance.
>
> I'd like to get Mark Brown's opinion on this.
>
>> >> +{
>> >> +	struct wm97xx_priv *wm97xx;
>> >> +	int ret;
>> >> +	void *pdata = snd_ac97_codec_get_platdata(adev);
>> >> +
>> >> +	wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
>> >> +			      sizeof(*wm97xx), GFP_KERNEL);
>> >> +	if (!wm97xx)
>> >> +		return -ENOMEM;
>> >> +
>> >> +	wm97xx->dev = ac97_codec_dev2dev(adev);
>> >> +	wm97xx->ac97 = snd_ac97_compat_alloc(adev);
>> >> +	if (IS_ERR(wm97xx->ac97))
>> >> +		return PTR_ERR(wm97xx->ac97);
>> >> +
>> >> +
>> >> +	ac97_set_drvdata(adev, wm97xx);
>> >> +	dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
>> >> +		 adev->vendor_id);
>> >
>> > All of this ac97/sound stuff should be done in the ac97/sound driver.
>> 
>> Nope, it's not sound adherence you're seeing here, it's ac97 bus and driver
>> model adherence you're seeing. Would the bus be in drivers/ac97 instead of
>> sound/ac97, the code would remain the same, would be bus be i2c you would see
>> the same kind of calls but with i2c_xxx and not ac97_xxx.
>> 
>> The wm97xx needs an ac97 bus to interact with the hardware, to provide sound,
>> gpio, adc, etc ... functions. That's what is expressed here, and the fact that
>> this ac97 access has to shared amongst the mfd sub-cells, and that these cells
>> require :
>>  - wm97xx->ac97 : this one is for drivers/input/touchscreen/wm97xx-core.c
>> 
>> So the requirement in this case is not for ac97/sound but input/touchscreen.
>> 
>> >> diff --git a/include/linux/mfd/wm97xx.h b/include/linux/mfd/wm97xx.h
>> >> new file mode 100644
>> >> index 000000000000..627322f14d48
>> >> --- /dev/null
>> >> +++ b/include/linux/mfd/wm97xx.h
>> >> @@ -0,0 +1,31 @@
>> >> +/*
>> >> + * wm97xx client interface
>> >> + *
>> >> + * Copyright (C) 2016 Robert Jarzmik
>> >> + *
>> >> + * 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.
>> >> + */
>> >> +
>> >> +#ifndef __LINUX_MFD_WM97XX_H
>> >> +#define __LINUX_MFD_WM97XX_H
>> >> +
>> >> +struct regmap;
>> >> +struct wm97xx_batt_pdata;
>> >> +struct snd_ac97;
>> >
>> > Why can't you just add the include files?
>> I could, but I don't need to, do I ?
>> Moreover, if a mfd sub-cell doesn't use regmap for example, I won't include a
>> useless define.
>> 
>> Thanks for the review, Lee. This will iterate, I'll split out mfd patch(es) to
>> follow up the review with you and Mark to lessen the burden on your mailbox.
>> 
>> Cheers.
>>