Re: [PATCH 6/7] mfd: wm831x: Correct kerneldoc
On Mon, Apr 19, 2021 at 10:17:25AM +0200, Krzysztof Kozlowski wrote: > Correct kerneldoc function name to fix W=1 warning: > > drivers/mfd/wm831x-core.c:121: warning: > expecting prototype for wm831x_reg_unlock(). Prototype was for > wm831x_reg_lock() instead > > Signed-off-by: Krzysztof Kozlowski > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH] mfd: arizona: Fix rumtime PM imbalance on error
On Wed, Apr 07, 2021 at 01:11:49PM +0800, Dinghao Liu wrote: > pm_runtime_get_sync() will increase the rumtime PM counter > even it returns an error. Thus a pairing decrement is needed > to prevent refcount leak. Fix this by replacing this API with > pm_runtime_resume_and_get(), which will not change the runtime > PM counter on error. > > Signed-off-by: Dinghao Liu > --- Acked-by: Charles Keepax Wow, did not realise that was how that worked. Thanks for fixing up. Thanks, Charles
Re: [PATCH 32/32] pinctrl: update pin-control.rst references
On Thu, Apr 01, 2021 at 02:17:52PM +0200, Mauro Carvalho Chehab wrote:
> Changeset 5513b411ea5b ("Documentation: rename pinctl to pin-control")
> renamed: Documentation/driver-api/pinctl.rst
> to: Documentation/driver-api/pin-control.rst.
>
> Update the cross-references accordingly.
>
> Fixes: 5513b411ea5b ("Documentation: rename pinctl to pin-control")
> Signed-off-by: Mauro Carvalho Chehab
> ---
Acked-by: Charles Keepax
Thanks,
Charles
Re: [PATCH] ASoC: wm8960: Fix wrong bclk and lrclk with pll enabled for some chips
On Fri, Mar 19, 2021 at 06:48:46PM +0800, Shengjiu Wang wrote:
> The input MCLK is 12.288MHz, the desired output sysclk is 11.2896MHz
> and sample rate is 44100Hz, with the configuration pllprescale=2,
> postscale=sysclkdiv=1, some chip may have wrong bclk
> and lrclk output with pll enabled in master mode, but with the
> configuration pllprescale=1, postscale=2, the output clock is correct.
>
> >From Datasheet, the PLL performs best when f2 is between
> 90MHz and 100MHz when the desired sysclk output is 11.2896MHz
> or 12.288MHz, so sysclkdiv = 2 (f2/8) is the best choice.
>
> So search available sysclk_divs from 2 to 1 other than from 1 to 2.
>
> Fixes: 84fdc00d519f ("ASoC: codec: wm9860: Refactor PLL out freq search")
> Signed-off-by: Shengjiu Wang
> ---
Acked-by: Charles Keepax
Thanks,
Charles
Re: [PATCH v2] ASoC: wm8962: Relax bit clock divider searching
On Mon, Mar 08, 2021 at 10:34:37AM +0800, Shengjiu Wang wrote: > With S20_3LE format case, the sysclk = rate * 384, > the bclk = rate * 20 * 2, there is no proper bclk divider > for 384 / 40, because current condition needs exact match. > So driver fails to configure the clocking: > > wm8962 3-001a: Unsupported BCLK ratio 9 > > Fix this by relaxing bitclk divider searching, so that when > no exact value can be derived from sysclk pick the closest > value greater than expected bitclk. > > Signed-off-by: Shengjiu Wang > Reviewed-by: Daniel Baluta > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH] sound: soc: codecs: Fix a spello in the file wm8955.c
On Sat, Mar 06, 2021 at 05:21:51PM +0530, Bhaskar Chowdhury wrote:
>
> s/sortd/sorted/
>
> Signed-off-by: Bhaskar Chowdhury
> ---
> sound/soc/codecs/wm8955.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/codecs/wm8955.c b/sound/soc/codecs/wm8955.c
> index 513df47bd87d..538bb8b0db39 100644
> --- a/sound/soc/codecs/wm8955.c
> +++ b/sound/soc/codecs/wm8955.c
> @@ -151,7 +151,7 @@ static int wm8955_pll_factors(struct device *dev,
> /* The oscilator should run at should be 90-100MHz, and
>* there's a divide by 4 plus an optional divide by 2 in the
>* output path to generate the system clock. The clock table
> - * is sortd so we should always generate a suitable target. */
> + * is sorted so we should always generate a suitable target. */
> target = Fout * 4;
> if (target < 9000) {
> pll->outdiv = 1;
> --
> 2.26.2
>
Content of the patch looks good, the commit message might need a
little work. The subject line needs to be appropriate for the
subsystem. Would be better to start with "ASoC: wm8955: " and
typo is a more standard term than spello :-)
Thanks,
Charles
Re: [PATCH] ASoC: wm8962: Relax bit clock divider searching
On Wed, Mar 03, 2021 at 07:21:28PM +0800, Shengjiu Wang wrote:
> With S20_3LE format case, the sysclk = rate * 384,
> the bclk = rate * 20 * 2, there is no proper bclk divider
> for 384 / 40, because current condition needs exact match.
> So driver fails to configure the clocking:
>
> wm8962 3-001a: Unsupported BCLK ratio 9
>
> Fix this by relaxing bitclk divider searching, so that when
> no exact value can be derived from sysclk pick the closest
> value greater than expected bitclk.
>
> Signed-off-by: Shengjiu Wang
> ---
> sound/soc/codecs/wm8962.c | 21 +++--
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
> index ce4666a74793..f5cd22450190 100644
> --- a/sound/soc/codecs/wm8962.c
> +++ b/sound/soc/codecs/wm8962.c
> @@ -2403,6 +2403,7 @@ static const int sysclk_rates[] = {
> static void wm8962_configure_bclk(struct snd_soc_component *component)
> {
> struct wm8962_priv *wm8962 = snd_soc_component_get_drvdata(component);
> + int best, min_diff, diff;
> int dspclk, i;
> int clocking2 = 0;
> int clocking4 = 0;
> @@ -2473,23 +2474,23 @@ static void wm8962_configure_bclk(struct
> snd_soc_component *component)
>
> dev_dbg(component->dev, "DSPCLK is %dHz, BCLK %d\n", dspclk,
> wm8962->bclk);
>
Very minor nit but it would probably be nice to have some
equivalent debug statement that prints out the actual BCLK we end
up with. There are a couple of statements printing the requested
speed, but nothing that will output what the driver actually
applies after this change.
Otherwise I think the change looks good.
Thanks,
Charles
Re: [PATCH v2] ASoC: wm8960: Remove bitclk relax condition in wm8960_configure_sysclk
On Wed, Mar 03, 2021 at 11:07:42AM +0800, Shengjiu Wang wrote:
> The call sequence in wm8960_configure_clocking is
>
>ret = wm8960_configure_sysclk();
>if (ret >= 0)
> goto configure_clock;
>
>
>
>ret = wm8960_configure_pll();
>
> configure_clock:
>...
>
> wm8960_configure_sysclk is called before wm8960_configure_pll, as
> there is bitclk relax on both functions, so wm8960_configure_sysclk
> always return success, then wm8960_configure_pll() never be called.
>
> With this case:
> aplay -Dhw:0,0 -d 5 -r 48000 -f S24_LE -c 2 audio48k24b2c.wav
> the required bitclk is 48000 * 24 * 2 = 2304000, bitclk got from
> wm8960_configure_sysclk is 3072000, but if go to wm8960_configure_pll.
> it can get correct bitclk 2304000.
>
> So bitclk relax condition should be removed in wm8960_configure_sysclk,
> then wm8960_configure_pll can be called, and there is also bitclk relax
> function in wm8960_configure_pll.
>
> Fixes: 3c01b9ee2ab9 ("ASoC: codec: wm8960: Relax bit clock computation")
> Signed-off-by: Shengjiu Wang
> Signed-off-by: Daniel Baluta
> ---
Acked-by: Charles Keepax
Thanks,
Charles
Re: [PATCH] ASoC: wm8960: Remove bitclk relax condition
On Tue, Mar 02, 2021 at 07:18:11PM +0800, Shengjiu Wang wrote:
> From: Daniel Baluta
>
> Using a higher bitclk then expected doesn't always work.
> Here is an example:
>
> aplay -Dhw:0,0 -d 5 -r 48000 -f S24_LE -c 2 audio48k24b2c.wav
>
> In this case, the required bitclk is 48000 * 24 * 2 = 2304000
> but the closest bitclk that can be derived is 3072000. Since
> the clock is faster than expected, it will start to send bytes
> from the next channel so the sound will be corrupted.
>
> Fixes: 82bab88910ee ("ASoC: codec: wm8960: Relax bit clock computation when
> using PLL")
> Fixes: 3c01b9ee2ab9 ("ASoC: codec: wm8960: Relax bit clock computation")
> Signed-off-by: Daniel Baluta
> Signed-off-by: Shengjiu Wang
> ---
I think this is probably going to need a much more involved fix.
The problem is that there are systems that depend on this
behaviour, so you can't just flat out revert it. And to be fair
the I2S specification says that bit clock can run at a higher
rate than required for the data, so the behaviour is correct as
well.
Probably the best solution here is to add additional contraints
from the machine driver on which rates/bit depths/channels are
supported.
Thanks,
Charles
[PATCH] ASoC: wm_adsp: Remove unused control callback structure
This callback structure has never been used and it is not clear why it
was added in the first place. Remove it to clear up the code a little.
Signed-off-by: Charles Keepax
---
sound/soc/codecs/wm_adsp.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 262c2db4f81ce..070ca7d8c6610 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -595,13 +595,6 @@ static const struct {
[WM_ADSP_FW_MISC] = { .file = "misc" },
};
-struct wm_coeff_ctl_ops {
- int (*xget)(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol);
- int (*xput)(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol);
-};
-
struct wm_coeff_ctl {
const char *name;
const char *fw_name;
@@ -609,7 +602,6 @@ struct wm_coeff_ctl {
const char *subname;
unsigned int subname_len;
struct wm_adsp_alg_region alg_region;
- struct wm_coeff_ctl_ops ops;
struct wm_adsp *dsp;
unsigned int enabled:1;
struct list_head list;
@@ -1497,8 +1489,6 @@ static int wm_adsp_create_control(struct wm_adsp *dsp,
}
ctl->enabled = 1;
ctl->set = 0;
- ctl->ops.xget = wm_coeff_get;
- ctl->ops.xput = wm_coeff_put;
ctl->dsp = dsp;
ctl->flags = flags;
--
2.11.0
Re: [PATCH -next] mfd: arizona: Make some symbols static
On Wed, Feb 10, 2021 at 07:56:26AM +, Wei Yongjun wrote:
> The sparse tool complains as follows:
>
> drivers/mfd/arizona-spi.c:28:31: warning:
> symbol 'reset_gpios' was not declared. Should it be static?
> drivers/mfd/arizona-spi.c:29:31: warning:
> symbol 'ldoena_gpios' was not declared. Should it be static?
>
> Those symbols are not used outside of arizona-spi.c, so this
> commit marks them static.
>
> Fixes: e933836744a2 ("mfd: arizona: Add support for ACPI enumeration of
> WM5102 connected over SPI")
> Reported-by: Hulk Robot
> Signed-off-by: Wei Yongjun
> ---
Acked-by: Charles Keepax
Thanks,
Charles
Re: [PATCH v4 11/13] ASoC: arizona-jack: Cleanup logging
On Sat, Jan 23, 2021 at 01:17:18PM +0100, Hans de Goede wrote: > Cleanup the use of dev_foo functions used for logging: > > 1. Many of these are unnecessarily split over multiple lines > 2. Use dev_err_probe() in cases where we might get a -EPROBE_DEFER >return value > > Suggested-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 01/13] mfd: arizona: Drop arizona-extcon cells
On Sat, Jan 23, 2021 at 01:13:01PM +0100, Hans de Goede wrote: > The arizona jack-dection handling is being reworked so that the > codec-child-device drivers directly handle jack-detect themselves, > so it is no longer necessary to instantiate "arizona-extcon" > child-devices. > > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 09/13] ASoC: arizona-jack: convert into a helper library for codec drivers
On Sat, Jan 23, 2021 at 01:13:09PM +0100, Hans de Goede wrote: > Convert the arizona extcon driver into a helper library for direct use > from the arizona codec-drivers, rather then being bound to a separate > MFD cell. > > Note the probe (and remove) sequence is split into 2 parts: > > 1. The arizona_jack_codec_dev_probe() function inits a bunch of > jack-detect specific variables in struct arizona_priv and tries to get > a number of resources where getting them may fail with -EPROBE_DEFER. > > 2. Then once the machine driver has create a snd_sock_jack through > snd_soc_card_jack_new() it calls snd_soc_component_set_jack() on > the codec component, which will call the new arizona_jack_set_jack(), > which sets up jack-detection and requests the IRQs. > > This split is necessary, because the IRQ handlers need access to the > arizona->dapm pointer and the snd_sock_jack which are not available > when the codec-driver's probe function runs. > > Note this requires that machine-drivers for codecs which are converted > to use the new helper functions from arizona-jack.c are modified to > create a snd_soc_jack through snd_soc_card_jack_new() and register > this jack with the codec through snd_soc_component_set_jack(). > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 02/13] extcon: arizona: Fix some issues when HPDET IRQ fires after the jack has been unplugged
On Sat, Jan 23, 2021 at 01:13:02PM +0100, Hans de Goede wrote: > When the jack is partially inserted and then removed again it may be > removed while the hpdet code is running. In this case the following > may happen: > > 1. The "JACKDET rise" or ""JACKDET fall" IRQ triggers > 2. arizona_jackdet runs and takes info->lock > 3. The "HPDET" IRQ triggers > 4. arizona_hpdet_irq runs, blocks on info->lock > 5. arizona_jackdet calls arizona_stop_mic() and clears info->hpdet_done > 6. arizona_jackdet releases info->lock > 7. arizona_hpdet_irq now can continue running and: > 7.1 Calls arizona_start_mic() (if a mic was detected) > 7.2 sets info->hpdet_done > > Step 7 is undesirable / a bug: > 7.1 causes the device to stay in a high power-state (with MICVDD enabled) > 7.2 causes hpdet to not run on the next jack insertion, which in turn > causes the EXTCON_JACK_HEADPHONE state to never get set > > This fixes both issues by skipping these 2 steps when arizona_hpdet_irq > runs after the jack has been unplugged. > > Reviewed-by: Andy Shevchenko > Acked-by: Charles Keepax > Signed-off-by: Hans de Goede > --- Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 08/13] ASoC: arizona-jack: Use arizona->dev for runtime-pm
On Sat, Jan 23, 2021 at 01:13:08PM +0100, Hans de Goede wrote: > Drivers for MFD child-devices such as the arizona codec drivers > and the arizona-extcon driver can choose to either make > runtime_pm_get/_put calls on their own child-device, which will > then be propagated to their parent; or they can make them directly > on their MFD parent-device. > > The arizona-extcon code was using runtime_pm_get/_put calls on > its own child-device where as the codec drivers are using > runtime_pm_get/_put calls on their parent. > > The arizona-extcon MFD cell/child-device has been removed and this > commit is part of refactoring the arizona-extcon code into a library > to be used directly from the codec drivers. > > Specifically this commit moves the code over to make > runtime_pm_get/_put calls on the parent device (on arizona->dev) > bringing the code inline with how the codec drivers do this. > > Note this also removes the pm_runtime_enable/_disable calls > as pm_runtime support has already been enabled on the parent-device > by the arizona MFD driver. > > This is part of a patch series converting the arizona extcon driver into > a helper library for letting the arizona codec-drivers directly report > jack state through the standard sound/soc/soc-jack.c functions. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 07/13] ASoC: arizona-jack: Move jack-detect variables to struct arizona_priv
On Sat, Jan 23, 2021 at 01:13:07PM +0100, Hans de Goede wrote: > Move all the jack-detect variables from struct arizona_extcon_info to > struct arizona_priv. > > This is part of a patch series converting the arizona extcon driver into > a helper library for letting the arizona codec-drivers directly report jack > state through the standard sound/soc/soc-jack.c functions. > > Reviewed-by: Andy Shevchenko > Acked-by: Charles Keepax > Signed-off-by: Hans de Goede > --- Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 12/13] ASoC: arizona: Make the wm5102, wm5110, wm8997 and wm8998 drivers use the new jack library
On Sat, Jan 23, 2021 at 01:17:19PM +0100, Hans de Goede wrote: > Make all arizona codec drivers for which drivers/mfd/arizona-core.c used > to instantiate a "arizona-extcon" child-device use the new arizona-jack.c > library for jack-detection. > > This has been tested on a Lenovo Yoga Tablet 2 1051L with a WM5102 codec. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 10/13] ASoC: arizona-jack: Use snd_soc_jack to report jack events
On Sat, Jan 23, 2021 at 01:17:17PM +0100, Hans de Goede wrote: > Use the snd_soc_jack code to report jack events, instead of using extcon > for reporting the cable-type + an input_dev for reporting the button > presses. > > The snd_soc_jack code will report the cable-type through both input_dev > events and through ALSA controls and the button-presses through input_dev > events. > > Note that this means that when the codec drivers are moved over to use > the new arizona-jack.c library code instead of having a separate MFD > extcon cell with the extcon-arizona.c driver, we will no longer report > extcon events to userspace for cable-type changes. This should not be > a problem since "standard" Linux distro userspace does not (and has > never) used the extcon class interface for this. Android does have > support for the extcon class interface, but that was introduced in > the same release as support for input_dev cable-type events, so this > should not be a problem for Android either. > > Note this also reduces ARIZONA_MAX_MICD_RANGE from 8 to 6, this is > ok to do since this info is always provided through pdata (or defaults) > and cannot be overridden from devicetree. All in-kernel users of the > pdata (and the fallback defaults) define 6 or less buttons/ranges. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 13/13] ASoC: Intel: bytcr_wm5102: Add jack detect support
On Sat, Jan 23, 2021 at 01:17:20PM +0100, Hans de Goede wrote:
> Add jack detect support by creating a jack and calling
> snd_soc_component_set_jack to register the created jack
> with the codec.
>
> Reviewed-by: Andy Shevchenko
> Signed-off-by: Hans de Goede
> ---
> +static struct snd_soc_jack_pin byt_wm5102_pins[] = {
> + {
> + .pin= "Headphone",
> + .mask = SND_JACK_HEADPHONE,
> + },
> + {
> + .pin= "Headset Mic",
> + .mask = SND_JACK_MICROPHONE,
> + },
> +};
> +
This patch looks fine to me, but I did have one small question.
What is the thinking behind punting this to the machine driver?
I guess you can not register it if there is no jack present
on the board, or if you have multiple jacks name them
meaningfully. Although I sort of feel like those applied to
the old extcon approach that just internally registered all
the interfaces.
But to be clear not asking for any changes just more about trying
to refine my understanding of things.
Thanks,
Charles
Re: [PATCH v4 03/13] extcon: arizona: Fix various races on driver unbind
On Sat, Jan 23, 2021 at 01:13:03PM +0100, Hans de Goede wrote: > We must free/disable all interrupts and cancel all pending works > before doing further cleanup. > > Before this commit arizona_extcon_remove() was doing several > register writes to shut things down before disabling the IRQs > and it was cancelling only 1 of the 3 different works used. > > Move all the register-writes shutting things down to after > the disabling of the IRQs and add the 2 missing > cancel_delayed_work_sync() calls. > > This fixes various possible races on driver unbind. One of which > would always trigger on devices using the mic-clamp feature for > jack detection. The ARIZONA_MICD_CLAMP_MODE_MASK update was > done before disabling the IRQs, causing: > 1. arizona_jackdet() to run > 2. detect a jack being inserted (clamp disabled means jack inserted) > 3. call arizona_start_mic() which: > 3.1 Enables the MICVDD regulator > 3.2 takes a pm_runtime_reference > > And this was all happening after the ARIZONA_MICD_ENA bit clearing, > which would undo 3.1 and 3.2 because the ARIZONA_MICD_CLAMP_MODE_MASK > update was being done after the ARIZONA_MICD_ENA bit clearing. > > So this means that arizona_extcon_remove() would exit with > 1. MICVDD enabled and 2. The pm_runtime_reference being unbalanced. > > MICVDD still being enabled caused the following oops when the > regulator is released by the devm framework: > > [ 2850.745757] [ cut here ] > [ 2850.745827] WARNING: CPU: 2 PID: 2098 at drivers/regulator/core.c:2123 > _regulator_put.part.0+0x19f/0x1b0 > [ 2850.745835] Modules linked in: extcon_arizona ... > ... > [ 2850.746909] Call Trace: > [ 2850.746932] regulator_put+0x2d/0x40 > [ 2850.746946] release_nodes+0x22a/0x260 > [ 2850.746984] __device_release_driver+0x190/0x240 > [ 2850.747002] driver_detach+0xd4/0x120 > ... > [ 2850.747337] ---[ end trace f455dfd7abd9781f ]--- > > Note this oops is just one of various theoretically possible races caused > by the wrong ordering inside arizona_extcon_remove(), this fixes the > ordering fixing all possible races, including the reported oops. > > Reviewed-by: Andy Shevchenko > Acked-by: Charles Keepax > Signed-off-by: Hans de Goede > --- Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 05/13] extcon: arizona: Always use pm_runtime_get_sync() when we need the device to be awake
On Sat, Jan 23, 2021 at 01:13:05PM +0100, Hans de Goede wrote: > Before this commit the extcon-arizona code was mixing pm_runtime_get() > and pm_runtime_get_sync() in different places. > > In all places where pm_runtime_get[_sync]() is called, the code > makes use of the device immediately after the call. > This means that we should always use pm_runtime_get_sync(). > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 06/13] ASoC/extcon: arizona: Move arizona jack code to sound/soc/codecs/arizona-jack.c
On Sat, Jan 23, 2021 at 01:13:06PM +0100, Hans de Goede wrote: > The jack handling for arizona codecs is being refactored so that it is > done directly by the codec drivers, instead of having an extcon-driver > bind to a separate "arizona-extcon" child-device for this. > > drivers/mfd/arizona-core.c has already been updated to no longer > instantiate an "arizona-extcon" child-device for the arizona codecs. > > This means that the "arizona-extcon" driver is no longer useful > (there are no longer any devices for it to bind to). > > This commit drops the extcon Kconfig / Makefile bits and moves > drivers/extcon/extcon-arizona.c to sound/soc/codecs/arizona-jack.c . > > This is a preparation patch for converting the arizona extcon-driver into > a helper library for letting the arizona codec-drivers directly report jack > state through the standard sound/soc/soc-jack.c functions. > > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Tested-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 05/13] extcon: arizona: Always use pm_runtime_get_sync() when we need the device to be awake
On Sat, Jan 23, 2021 at 01:13:05PM +0100, Hans de Goede wrote: > Before this commit the extcon-arizona code was mixing pm_runtime_get() > and pm_runtime_get_sync() in different places. > > In all places where pm_runtime_get[_sync]() is called, the code > makes use of the device immediately after the call. > This means that we should always use pm_runtime_get_sync(). > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH] regulator: core: Avoid debugfs: Directory ... already present! error
On Fri, Jan 22, 2021 at 07:32:50PM +0100, Hans de Goede wrote:
> Sometimes regulator_get() gets called twice for the same supply on the
> same device. This may happen e.g. when a framework / library is used
> which uses the regulator; and the driver itself also needs to enable
> the regulator in some cases where the framework will not enable it.
>
> Commit ff268b56ce8c ("regulator: core: Don't spew backtraces on
> duplicate sysfs") already takes care of the backtrace which would
> trigger when creating a duplicate consumer symlink under
> /sys/class/regulator/regulator.%d in this scenario.
>
> Commit c33d442328f5 ("debugfs: make error message a bit more verbose")
> causes a new error to get logged in this scenario:
>
> [ 26.938425] debugfs: Directory 'wm5102-codec-MICVDD' with parent
> 'spi-WM510204:00-MICVDD' already present!
>
> There is no _nowarn variant of debugfs_create_dir(), but we can detect
> and avoid this problem by checking the return value of the earlier
> sysfs_create_link_nowarn() call.
>
> Add a check for the earlier sysfs_create_link_nowarn() failing with
> -EEXIST and skip the debugfs_create_dir() call in that case, avoiding
> this error getting logged.
>
> Fixes: c33d442328f5 ("debugfs: make error message a bit more verbose")
> Cc: Charles Keepax
> Signed-off-by: Hans de Goede
> ---
Reviewed-by: Charles Keepax
Thanks,
Charles
> - int err;
> + int err = 0;
>
> @@ -1663,8 +1663,8 @@ static struct regulator *create_regulator(struct
> regulator_dev *rdev,
>
> - regulator->debugfs = debugfs_create_dir(supply_name,
> - rdev->debugfs);
> + if (err != -EEXIST)
> + regulator->debugfs = debugfs_create_dir(supply_name,
> rdev->debugfs);
There is a slight oddity here in that if this regulator has
no struct device we will still get the warning. However, I
am totally not clear on when/why a regulator might not have a
dev, and am fairly sure it isn't common. So my vote would be
to cross that bridge if we ever come to it.
Thanks,
Charles
Re: [PATCH v2 08/12] ASoC: arizona-jack: convert into a helper library for codec drivers
On Thu, Jan 21, 2021 at 05:55:00PM +0100, Hans de Goede wrote:
> On 1/19/21 10:51 AM, Richard Fitzgerald wrote:
> > On 18/01/2021 17:24, Andy Shevchenko wrote:
> >> On Sun, Jan 17, 2021 at 6:06 PM Hans de Goede wrote:
> 1. Keep the code as is, live with the debugfs error. This might be
> best for now, as I don't want to grow the scope of this series too much.
> I will go with this for the next version of this series (unless
> I receive feedback otherwise before I get around to posting the next
> version).
Thinking about this more, I seem to remember this is something
that has been discussed before, having the need to have
situations where a driver and the framework are both managing the
regulator at once on the same device.
I wonder if this commit was related to that:
commit ff268b56ce8c ("regulator: core: Don't spew backtraces on duplicate
sysfs")
Apologies I don't have as much time as I normally would to look
into such issues at the moment, due to various internal company
things going on.
I do suspect that this option is the way to go though and if
there are issues of duplicates being created by the regulator
core those probably need to be resolved in there. But that can
probably be done separate from this series.
Thanks,
Charles
Re: [PATCH v2 08/12] ASoC: arizona-jack: convert into a helper library for codec drivers
On Fri, Jan 22, 2021 at 01:23:44PM +0100, Hans de Goede wrote:
> On 1/22/21 12:26 PM, Charles Keepax wrote:
> > On Thu, Jan 21, 2021 at 05:55:00PM +0100, Hans de Goede wrote:
> >> On 1/19/21 10:51 AM, Richard Fitzgerald wrote:
> >>> On 18/01/2021 17:24, Andy Shevchenko wrote:
> >>>> On Sun, Jan 17, 2021 at 6:06 PM Hans de Goede
> >>>> wrote:
> >> Note there is a pretty big issue with the original code here, if
> >> the MICVDD DAPM pin is on for an internal-mic and then we run through the
> >> jack-detect mic-detect sequence, we end up setting
> >> bypass=true causing the micbias for the internal-mic to no longer
> >> be what was configured. IOW poking the bypass setting underneath the
> >> DAPM code is racy.
> >>
> >
> > The regulator bypass code keeps an internal reference count. All
> > the users of the regulator need to allow bypass for it to be
> > placed into bypass mode, so I believe this can't happen.
>
> Ah I did not know that, since the regulator_allow_bypass function
> takes a bool rather then having enable/disable variants I thought
> it would directly set the bypass, but you are right. So this is not
> a problem, good.
>
> So this has made me look at the problem again and I believe that
> a much better solution is to simply re-use the MICVDD regulator-reference
> which has been regulator_get-ed by the dapm code when instantiating the:
>
> SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
>
> widget. So I plan to have a new patch in v3 of the series which replaces
> the devm_regulator_get with something like this:
>
> /*
>* There is a DAPM widget for the MICVDD regulator, since
>* the button-press detection has special requirements wrt
>* the regulator bypass settings we cannot directly
>* use snd_soc_component_force_enable_pin("MICVDD") /
>* snd_soc_component_disable_pin("MICVDD").
>*
>* Instead we lookup the widget's regulator reference here
>* and use that to directly control the regulator.
>* Both the regulator's enable and bypass settings are
>* ref-counted so this will not interfere with the DAPM use
>* of the regulator.
>*/
> for_each_card_widgets(dapm->card, w) {
> if (!strcmp(w->name, "MICVDD"))
> info->micvdd_regulator = w->regulator;
> break;
> }
> }
>
> (note I've not tested this yet, but I expect this to work fine).
>
Alas this won't work either. When I say reference count that
isn't quite a totally accurate reflection of the usage of the
function. When you call allow_bypass you are saying as this
consumer of the regulator I don't mind if it goes into bypass.
Then if all consumers agree the regulator will be put into
bypass. So it is comparing the reference count to the number of
consumers the regulator has to make a decision.
If you call allow_bypass independently from the jack detection
code and the ASoC framework on the same consumer, as you
describe here you will get bad effects. For example the
regulator has two consumers, our CODEC driver and some other
device. If our codec driver calls allow_bypass twice, then
the regulator would go into bypass without the other consumer
having approved it would could be fatal to that device.
Thanks,
Charles
Re: [PATCH v2 08/12] ASoC: arizona-jack: convert into a helper library for codec drivers
On Thu, Jan 21, 2021 at 05:55:00PM +0100, Hans de Goede wrote:
> Hi,
>
> On 1/19/21 10:51 AM, Richard Fitzgerald wrote:
> > On 18/01/2021 17:24, Andy Shevchenko wrote:
> >> On Sun, Jan 17, 2021 at 6:06 PM Hans de Goede wrote:
> >>>
> >>> Convert the arizona extcon driver into a helper library for direct use
> >>> from the arizona codec-drivers, rather then being bound to a separate
> >>> MFD cell.
> >>>
> >>> Note the probe (and remove) sequence is split into 2 parts:
> >>>
> >>> 1. The arizona_jack_codec_dev_probe() function inits a bunch of
> >>> jack-detect specific variables in struct arizona_priv and tries to get
> >>> a number of resources where getting them may fail with -EPROBE_DEFER.
> >>>
> >>> 2. Then once the machine driver has create a snd_sock_jack through
> >>> snd_soc_card_jack_new() it calls snd_soc_component_set_jack() on
> >>> the codec component, which will call the new arizona_jack_set_jack(),
> >>> which sets up jack-detection and requests the IRQs.
> >>>
> >>> This split is necessary, because the IRQ handlers need access to the
> >>> arizona->dapm pointer and the snd_sock_jack which are not available
> >>> when the codec-driver's probe function runs.
> >>>
> >>> Note this requires that machine-drivers for codecs which are converted
> >>> to use the new helper functions from arizona-jack.c are modified to
> >>> create a snd_soc_jack through snd_soc_card_jack_new() and register
> >>> this jack with the codec through snd_soc_component_set_jack().
> >>
> >> ...
> >>
> >>> +int arizona_jack_codec_dev_probe(struct arizona_priv *info, struct
> >>> device *dev)
> >>> {
> >>> - struct arizona *arizona = dev_get_drvdata(pdev->dev.parent);
> >>> + struct arizona *arizona = info->arizona;
> >>> struct arizona_pdata *pdata = >pdata;
> >>
> >>> + int ret, mode;
> >>>
> >>> if (!dev_get_platdata(arizona->dev))
> >>> - arizona_extcon_device_get_pdata(>dev, arizona);
> >>> + arizona_extcon_device_get_pdata(dev, arizona);
> >>>
> >>> - info->micvdd = devm_regulator_get(>dev, "MICVDD");
> >>> + info->micvdd = devm_regulator_get(arizona->dev, "MICVDD");
> >>
> >> I'm wondering if arizona->dev == dev here. if no, can this function
> >> get a comment / kernel-doc explaining what dev is?
> >>
> >
> > pdev->dev would be *this* driver.
> > arizona->dev should be the MFD parent driver.
> >
> > I think these gets should be against the dev passed in as argument
> > (I assume that is the caller's pdev->dev). So they are owned by this
> > driver, not its parent.
>
> Right, this is all correct.
>
> The reason why I used arizona->dev instead of dev for the devm_regulator_get()
> is because the codec code already does a regulator_get for MICVDD through:
>
> SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
>
> And doing it again leads to an error being logged about trying to
> create a file in debugs with a name which already exists, because now
> we do a regulator_get("MICVDD") with the same consumer twice.
>
> But I now see that I overlooked the devm part, turning my "fix" from
> a cute hack to just being outright wrong.
>
Aye we should definitely drop the devm here.
> So there are a number of solutions here:
>
>
> 1. Keep the code as is, live with the debugfs error. This might be
> best for now, as I don't want to grow the scope of this series too much.
> I will go with this for the next version of this series (unless
> I receive feedback otherwise before I get around to posting the next
> version).
>
Not ideal but as you say might be the best thing for now.
>
> 2. Switch the arizona-jack code from directly poking the regulator
> to using snd_soc_component_force_enable_pin("MICVDD") and
> snd_soc_component_disable_pin("MICVDD"). I like this, but there is
> one downside, the dapm code assumes that when the regulator is
> enabled the bypass must be disabled:
>
...
>
> When enabling MIC-current / button-press IRQs.
>
> If we switch to using snd_soc_component_force_enable_pin("MICVDD") and
> snd_soc_component_disable_pin("MICVDD") we loose the power-saving
> of using the bypass when we only need MICVDD for button-press
> detection.
>
Yeah we really don't want to force the micbias's to be regulated
during button detect, so I think this option has to go.
> Note there is a pretty big issue with the original code here, if
> the MICVDD DAPM pin is on for an internal-mic and then we run through the
> jack-detect mic-detect sequence, we end up setting
> bypass=true causing the micbias for the internal-mic to no longer
> be what was configured. IOW poking the bypass setting underneath the
> DAPM code is racy.
>
The regulator bypass code keeps an internal reference count. All
the users of the regulator need to allow bypass for it to be
placed into bypass mode, so I believe this can't happen.
> Keeping in mind that switching to force_enable fixes the current racy code,
> as well as the KISS-ness
Re: [PATCH v2 06/12] ASoC: arizona-jack: Move jack-detect variables to struct arizona_priv
On Sun, Jan 17, 2021 at 05:05:49PM +0100, Hans de Goede wrote: > Move all the jack-detect variables from struct arizona_extcon_info to > struct arizona_priv. > > This is part of a patch series converting the arizona extcon driver into > a helper library for letting the arizona codec-drivers directly report jack > state through the standard sound/soc/soc-jack.c functions. > > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH v2 03/12] ASoC: arizona-jack: Fix some issues when HPDET IRQ fires after the jack has been unplugged
On Sun, Jan 17, 2021 at 05:05:46PM +0100, Hans de Goede wrote: > When the jack is partially inserted and then removed again it may be > removed while the hpdet code is running. In this case the following > may happen: > > 1. The "JACKDET rise" or ""JACKDET fall" IRQ triggers > 2. arizona_jackdet runs and takes info->lock > 3. The "HPDET" IRQ triggers > 4. arizona_hpdet_irq runs, blocks on info->lock > 5. arizona_jackdet calls arizona_stop_mic() and clears info->hpdet_done > 6. arizona_jackdet releases info->lock > 7. arizona_hpdet_irq now can continue running and: > 7.1 Calls arizona_start_mic() (if a mic was detected) > 7.2 sets info->hpdet_done > > Step 7 is undesirable / a bug: > 7.1 causes the device to stay in a high power-state (with MICVDD enabled) > 7.2 causes hpdet to not run on the next jack insertion, which in turn > causes the EXTCON_JACK_HEADPHONE state to never get set > > This fixes both issues by skipping these 2 steps when arizona_hpdet_irq > runs after the jack has been unplugged. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 5/5] ASoC: Intel: bytcr_wm5102: Add machine driver for BYT/WM5102
On Wed, Jan 20, 2021 at 10:49:57PM +0100, Hans de Goede wrote: > From: Pierre-Louis Bossart > > Add a new ASoc Machine driver for Intel Baytrail platforms with a > Wolfson Microelectronics WM5102 codec. > > This is based on a past contributions [1] from Paulo Sergio Travaglia > based on the Levono kernel [2] combined with > insights in things like the speaker GPIO from the android-x86 android > port for the Lenovo Yoga Tablet 2 1051F/L [3]. > > [1] > https://patchwork.kernel.org/project/alsa-devel/patch/593313f5.3636c80a.50e05.4...@mx.google.com/ > [2] > https://github.com/lenovo-yt2-dev/android_kernel_lenovo_baytrail/blob/cm-12.1/sound/soc/intel/board/byt_bl_wm5102.c > [3] https://github.com/Kitsune/Android_Yoga_Tablet_2-1051F_Kernel > > The original machine driver from the Android ports was a crude modified > copy of bytcr_rt5640.c adjusted to work with the WM5102 codec. > This version has been extensively reworked to: > > 1. Remove all rt5640 related quirk handling. to the best of my knowledge > this setup is only used on the Lenovo Yoga Tablet 2 series (8, 10 and 13 > inch models) which all use the same setup. So there is no need to deal > with all the variations with which we need to deal on rt5640 boards. > > 2. Rework clock handling, properly turn off the FLL and the platform-clock > when they are no longer necessary and don't reconfigure the FLL > unnecessarily when it is already running. This fixes a number of: > "Timed out waiting for lock" warnings being logged. > > 3. Add the GPIO controlled Speaker-VDD regulator as a DAPM_SUPPLY > > This only adds the machine driver and ACPI hooks, the BYT-CR detection > quirk which these devices need will be added in a separate patch. > > BugLink: https://github.com/thesofproject/linux/issues/2485 > Co-authored-by: Pierre-Louis Bossart > Signed-off-by: Pierre-Louis Bossart > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Reviewed-by: Charles Keepax Thanks, Charles
Re: [PATCH v4 3/5] mfd: arizona: Add support for ACPI enumeration of WM5102 connected over SPI
On Wed, Jan 20, 2021 at 10:49:55PM +0100, Hans de Goede wrote: > The Intel Bay Trail (x86/ACPI) based Lenovo Yoga Tablet 2 series use > a WM5102 codec connected over SPI. > > Add support for ACPI enumeration to arizona-spi so that arizona-spi can > bind to the codec on these tablets. > > This is loosely based on an earlier attempt (for Android-x86) at this by > Christian Hartmann, combined with insights in things like the speaker GPIO > from the android-x86 android port for the Lenovo Yoga Tablet 2 1051F/L [1]. > > [1] https://github.com/Kitsune/Android_Yoga_Tablet_2-1051F_Kernel > > Cc: Christian Hartmann > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH v3 2/5] mfd: arizona: Replace arizona_of_get_type() with device_get_match_data()
On Sun, Jan 17, 2021 at 10:22:49PM +0100, Hans de Goede wrote: > Replace the custom arizona_of_get_type() function with the generic > device_get_match_data() helper. Besides being a nice cleanup this > also makes it easier to add support for binding to ACPI enumerated > devices. > > While at it also fix a possible NULL pointer deref of the id > argument to the probe functions (this could happen on e.g. manual > driver binding through sysfs). > > Suggested-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH v2] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag
On Wed, Dec 30, 2020 at 05:44:41PM +0100, Andrew Lunn wrote:
> On Wed, Dec 30, 2020 at 10:33:09AM +0000, Charles Keepax wrote:
> > A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of
> > macb_set_tx_clk were gated on the presence of this flag.
> >
> > - if (!clk)
> > + if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
> >
> > However the flag was not added to anything other than the new
> > sama7g5_gem, turning that function call into a no op for all other
> > systems. This breaks the networking on Zynq.
> >
> > The commit message adding this states: a new capability so that
> > macb_set_tx_clock() to not be called for IPs having this
> > capability
> >
> > This strongly implies that present of the flag was intended to skip
> > the function not absence of the flag. Update the if statement to
> > this effect, which repairs the existing users.
> >
> > Fixes: daafa1d33cc9 ("net: macb: add capability to not set the clock rate")
> > Suggested-by: Andrew Lunn
> > Signed-off-by: Charles Keepax
>
> Reviewed-by: Andrew Lunn
>
> Hi Charles
>
> Since this is a fix, this should be based on the net tree. And you
> indicate this in the subject line with [PATCH net]. If this applies
> cleanly to net, Dave will probably just accept it, but please keep
> this in mind for any more submissions you make to netdev.
Apologies I totally forgot to add your Reviewed-by on the new
version. Too much turkey on the brain after the fattening season.
Thanks,
Charles
Re: [PATCH] ASoC: wm8962: Add optional mclk device tree binding
On Thu, Dec 17, 2020 at 10:27:40AM -0600, Adam Ford wrote: > The driver can request an optional clock for mclk. > Update the txt file to reflect this. > > Suggested-by: Geert Uytterhoeven > Signed-off-by: Adam Ford Acked-by: Charles Keepax Thanks, Charles
[PATCH net v3] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag
A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of
macb_set_tx_clk were gated on the presence of this flag.
- if (!clk)
+ if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
However the flag was not added to anything other than the new
sama7g5_gem, turning that function call into a no op for all other
systems. This breaks the networking on Zynq.
The commit message adding this states: a new capability so that
macb_set_tx_clock() to not be called for IPs having this
capability
This strongly implies that present of the flag was intended to skip
the function not absence of the flag. Update the if statement to
this effect, which repairs the existing users.
Fixes: daafa1d33cc9 ("net: macb: add capability to not set the clock rate")
Suggested-by: Andrew Lunn
Signed-off-by: Charles Keepax
---
Changes since v1:
- Updated flag semantics to skip function, as appears to have been
intended by the initial commit.
Changes since v2:
- Adding "net" to the subject line
Thanks,
Charles
drivers/net/ethernet/cadence/macb_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c
b/drivers/net/ethernet/cadence/macb_main.c
index d5d910916c2e8..814a5b10141d1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -467,7 +467,7 @@ static void macb_set_tx_clk(struct macb *bp, int speed)
{
long ferr, rate, rate_rounded;
- if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
+ if (!bp->tx_clk || (bp->caps & MACB_CAPS_CLK_HW_CHG))
return;
switch (speed) {
--
2.11.0
Re: [PATCH v2] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag
On Wed, Dec 30, 2020 at 05:44:41PM +0100, Andrew Lunn wrote:
> On Wed, Dec 30, 2020 at 10:33:09AM +0000, Charles Keepax wrote:
> > A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of
> > macb_set_tx_clk were gated on the presence of this flag.
> >
> > - if (!clk)
> > + if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
> >
> > However the flag was not added to anything other than the new
> > sama7g5_gem, turning that function call into a no op for all other
> > systems. This breaks the networking on Zynq.
> >
> > The commit message adding this states: a new capability so that
> > macb_set_tx_clock() to not be called for IPs having this
> > capability
> >
> > This strongly implies that present of the flag was intended to skip
> > the function not absence of the flag. Update the if statement to
> > this effect, which repairs the existing users.
> >
> > Fixes: daafa1d33cc9 ("net: macb: add capability to not set the clock rate")
> > Suggested-by: Andrew Lunn
> > Signed-off-by: Charles Keepax
>
> Reviewed-by: Andrew Lunn
>
> Hi Charles
>
> Since this is a fix, this should be based on the net tree. And you
> indicate this in the subject line with [PATCH net]. If this applies
> cleanly to net, Dave will probably just accept it, but please keep
> this in mind for any more submissions you make to netdev.
>
Is already based on the net tree, sorry wasn't aware I needed to add
that into the subject line. Will do a respin today.
Thanks,
Charles
[PATCH v2] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag
A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of
macb_set_tx_clk were gated on the presence of this flag.
- if (!clk)
+ if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
However the flag was not added to anything other than the new
sama7g5_gem, turning that function call into a no op for all other
systems. This breaks the networking on Zynq.
The commit message adding this states: a new capability so that
macb_set_tx_clock() to not be called for IPs having this
capability
This strongly implies that present of the flag was intended to skip
the function not absence of the flag. Update the if statement to
this effect, which repairs the existing users.
Fixes: daafa1d33cc9 ("net: macb: add capability to not set the clock rate")
Suggested-by: Andrew Lunn
Signed-off-by: Charles Keepax
---
Changes since v1:
- Updated flag semantics to skip function, as appears to have been
intended by the initial commit.
Thanks,
Charles
drivers/net/ethernet/cadence/macb_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c
b/drivers/net/ethernet/cadence/macb_main.c
index d5d910916c2e8..814a5b10141d1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -467,7 +467,7 @@ static void macb_set_tx_clk(struct macb *bp, int speed)
{
long ferr, rate, rate_rounded;
- if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
+ if (!bp->tx_clk || (bp->caps & MACB_CAPS_CLK_HW_CHG))
return;
switch (speed) {
--
2.11.0
Re: [PATCH 01/14] mfd: arizona: Add jack pointer to struct arizona
On Tue, Dec 29, 2020 at 02:57:38PM +0100, Hans de Goede wrote: > On 12/29/20 2:06 PM, Charles Keepax wrote: > > On Mon, Dec 28, 2020 at 04:28:07PM +, Mark Brown wrote: > >> On Mon, Dec 28, 2020 at 02:16:04PM +0100, Hans de Goede wrote: > >> > >>> And more in general AFAIK extcon is sort of deprecated and it is > >>> not advised to use it for new code. I would esp. not expect it to > >>> be used for new jack-detection code since we already have standard > >>> uAPI support for that through sound/core/jack.c . > >> > >> Has Android been fixed to use the ALSA/input layer interfaces? That's > >> why that code is there, long term the goal was to have ALSA generate > >> extcon events too so userspace could fall over to using that. The basic > >> thing at the time was that nobody liked any of the existing interfaces > >> (the input layer thing is a total bodge stemming from it having been > >> easy to hack in a key for GPIO detection and using ALSA controls means > >> having to link against alsa-lib which is an awful faff for system level > >> UI stuff) and there were three separate userspace interfaces used by > >> different software stacks which needed to be joined together, extcon was > >> felt to be a bit more designed and is a superset so that was the > >> direction we were heading in. > > > > Android has been updated to have the option to catch input events > > for jack detection now. > > > > I have always been slightly confused between extcon and the ALSA > > jack reporting and have been unsure as to what is the longer term > > plan here. I vaguely thought there was a gentle plan to move to > > extcon, it is interesting to see Hans basically saying the > > opposite that extcon is intended to be paritially deprecated. I > > assume you just mean with respect to audio jacks, not other > > connector types? > > No I mean that afaik extcon is being deprecated in general. Extcon > is mostly meant for kernel internal use, to allow things like > charger-type-detection done by e.g. a fsa micro-usb mux or a > Type-C PD controller to be hooked up to the actual charger chip > and set the input-current-limit based on this. > Fascinating thanks for taking the time to write such detailed answers. I thought it was mostly intended for user-space usage, but I guess I never really thought through that most of this stuff you don't really need to know from user-space. > > I would agree with Mark though that if extcon exists for external > > connectors it seems odd that audio jacks would have their own > > special way rather than just using the connector stuff. > > Well as I said above in me experience the extcon code is (was) mostly > meant for kernel internal use. The sysfs API is more of a debugging > tool then anything else (IMHO). > > Also the kernel has support for a lot of sound devices, including > many with jack-detection support. Yet a grep for EXTCON_JACK_HEADPHONE > over the entire mainline kernel tree shows that only extcon-arizona.c > is using it. So given that we have dozens of drivers providing jack > functionality through the sound/core/jack.c core and only 1 driver > using the extcon interface I believe that the ship on how to export > this to userspace has long sailed, since most userspace code will > clearly expect the sound/core/jack.c way of doing things to be used. > > Arguably we should/could maybe even drop the extcon part of extcon-arizona.c > but I did not do that as I did not want to regress existing userspace > code which may depend on this (on specific embedded/android devices). > All reasonable arguments, with Android now supporting input events for jacks I guess there would be no need for us to use extcon for future devices. There is maybe more argument for porting the Arizona code across anyways, since for a long time Android didn't properly support extcon either. It supported the earlier out of tree switch stuff, extcon had a switch compatibility mode, but that didn't actually work I think due to android hard coding some sysfs naming or something (memory is a little fuzzy on the details was a while ago now). I think extcon support was fixed in Android at about the same time the support for input events was added. So it might be harmless but someone probably needs to go and check the timeline before we go changing stuff. Thanks, Charles
Re: [PATCH 13/14] ASoC: Intel: bytcr_wm5102: Add machine driver for BYT/WM5102
On Sun, Dec 27, 2020 at 10:12:31PM +0100, Hans de Goede wrote:
> From: Pierre-Louis Bossart
>
> Add a new ASoc Machine driver for Intel Baytrail platforms with a
> Wolfson Microelectronics WM5102 codec.
>
> This is based on a past contributions [1] from Paulo Sergio Travaglia
> based on the Levono kernel [2] combined with
> insights in things like the speaker GPIO from the android-x86 android
> port for the Lenovo Yoga Tablet 2 1051F/L [3].
>
> [1]
> https://patchwork.kernel.org/project/alsa-devel/patch/593313f5.3636c80a.50e05.4...@mx.google.com/
> [2]
> https://github.com/lenovo-yt2-dev/android_kernel_lenovo_baytrail/blob/cm-12.1/sound/soc/intel/board/byt_bl_wm5102.c
> [3] https://github.com/Kitsune/Android_Yoga_Tablet_2-1051F_Kernel
>
> The original machine driver from the Android ports was a crude modified
> copy of bytcr_rt5640.c adjusted to work with the WM5102 codec.
> This version has been extensively reworked to:
>
> 1. Remove all rt5640 related quirk handling. to the best of my knowledge
> this setup is only used on the Lenovo Yoga Tablet 2 series (8, 10 and 13
> inch models) which all use the same setup. So there is no need to deal
> with all the variations with which we need to deal on rt5640 boards.
>
> 2. Rework clock handling, properly turn off the FLL and the platform-clock
> when they are no longer necessary and don't reconfigure the FLL
> unnecessarily when it is already running. This fixes a number of:
> "Timed out waiting for lock" warnings being logged.
>
> 3. Add the GPIO controlled Speaker-VDD regulator as a DAPM_SUPPLY
>
> This only adds the machine driver and ACPI hooks, the BYT-CR detection
> quirk which these devices need will be added in a separate patch.
>
> Co-authored-by: Pierre-Louis Bossart
> Signed-off-by: Pierre-Louis Bossart
> Signed-off-by: Hans de Goede
> ---
Just a couple really minor comments.
> +static int byt_wm5102_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai,
> int rate)
> +{
> + struct snd_soc_component *codec_component = codec_dai->component;
> + int sr_mult = ((rate % 4000) == 0) ?
> + (WM5102_MAX_SYSCLK_4K / rate) :
> + (WM5102_MAX_SYSCLK_11025 / rate);
> + int ret;
> +
> + /* Reset FLL1 */
> + snd_soc_dai_set_pll(codec_dai, WM5102_FLL1_REFCLK,
> ARIZONA_FLL_SRC_NONE, 0, 0);
> + snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_FLL_SRC_NONE, 0, 0);
> +
> + /* Configure the FLL1 PLL before selecting it */
> + ret = snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_CLK_SRC_MCLK1,
> + MCLK_FREQ, rate * sr_mult);
> + if (ret) {
> + dev_err(codec_component->dev, "Error setting PLL: %d\n", ret);
> + return ret;
> + }
> +
> + ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_SYSCLK,
> +ARIZONA_CLK_SRC_FLL1, rate * sr_mult,
> +SND_SOC_CLOCK_IN);
> + if (ret) {
> + dev_err(codec_component->dev, "Error setting ASYNCCLK: %d\n",
> ret);
Error message should say SYSCLK not ASYNCCLK.
> + return ret;
> + }
> +
> + ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_OPCLK,
> 0,
> +rate * sr_mult, SND_SOC_CLOCK_OUT);
> + if (ret) {
> + dev_err(codec_component->dev, "Error setting OPCLK: %d\n", ret);
> + return ret;
> + }
OPCLK is a clock that can be outputted on the CODECs GPIOs. Is
that being used to clock some external component? If so it should
be added to the DAPM graph, if not you might as well remove this
call.
> +
> + ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_SYSCLK,
> + rate * 512, SND_SOC_CLOCK_IN);
> + if (ret) {
> + dev_err(codec_component->dev, "Error setting clock: %d\n", ret);
> + return ret;
> + }
> +
The rate you set here doesn't actually matter, on wm5102 this
just links the DAI to a specific clock domain and as they all
default to SYSCLK you can omit this call if you want. Although no
harm is caused by leaving it in.
Thanks,
Charles
Re: [PATCH 01/14] mfd: arizona: Add jack pointer to struct arizona
On Mon, Dec 28, 2020 at 04:28:07PM +, Mark Brown wrote: > On Mon, Dec 28, 2020 at 02:16:04PM +0100, Hans de Goede wrote: > > > And more in general AFAIK extcon is sort of deprecated and it is > > not advised to use it for new code. I would esp. not expect it to > > be used for new jack-detection code since we already have standard > > uAPI support for that through sound/core/jack.c . > > Has Android been fixed to use the ALSA/input layer interfaces? That's > why that code is there, long term the goal was to have ALSA generate > extcon events too so userspace could fall over to using that. The basic > thing at the time was that nobody liked any of the existing interfaces > (the input layer thing is a total bodge stemming from it having been > easy to hack in a key for GPIO detection and using ALSA controls means > having to link against alsa-lib which is an awful faff for system level > UI stuff) and there were three separate userspace interfaces used by > different software stacks which needed to be joined together, extcon was > felt to be a bit more designed and is a superset so that was the > direction we were heading in. Android has been updated to have the option to catch input events for jack detection now. I have always been slightly confused between extcon and the ALSA jack reporting and have been unsure as to what is the longer term plan here. I vaguely thought there was a gentle plan to move to extcon, it is interesting to see Hans basically saying the opposite that extcon is intended to be paritially deprecated. I assume you just mean with respect to audio jacks, not other connector types? I would agree with Mark though that if extcon exists for external connectors it seems odd that audio jacks would have their own special way rather than just using the connector stuff. Thanks, Charles
Re: [PATCH 09/14] extcon: arizona: Add arizona_set_extcon_state() helper
On Sun, Dec 27, 2020 at 10:12:27PM +0100, Hans de Goede wrote:
> All the callers of extcon_set_state_sync() log an error on failure,
> without doing any further error-handling (as there is nothing they
> can do about the error).
>
> Factor this out into a helper to remove some duplicate code.
>
> Signed-off-by: Hans de Goede
> ---
> drivers/extcon/extcon-arizona.c | 47 -
> 1 file changed, 17 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index 145ca5cd40d5..d5b3231744f9 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -595,6 +595,16 @@ static int arizona_hpdet_do_id(struct
> arizona_extcon_info *info, int *reading,
> return 0;
> }
>
> +static void arizona_set_extcon_state(struct arizona_extcon_info *info,
> + unsigned int id, bool state)
> +{
> + int ret;
> +
> + ret = extcon_set_state_sync(info->edev, id, state);
> + if (ret)
> + dev_err(info->arizona->dev, "Failed to set extcon state: %d\n",
> ret);
> +}
Would be nice to also print which ID it is that is failing,
would help to narrow things down since we lose the customer error
messages for each case.
Thanks,
Charles
Re: [PATCH 08/14] extcon: arizona: Fix flags parameter to the gpiod_get("wlf,micd-pol") call
On Sun, Dec 27, 2020 at 10:12:26PM +0100, Hans de Goede wrote: > The initial value of the GPIO should match the info->micd_modes[0].gpio > value. arizona_extcon_probe() already stores the necessary flag in a > mode variable, but instead of passing mode as flags to the gpiod_get() > it was using a hardcoded GPIOD_OUT_LOW. > > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 06/14] extcon: arizona: Fix various races on driver unbind
On Sun, Dec 27, 2020 at 10:12:24PM +0100, Hans de Goede wrote: > We must free/disable all interrupts and cancel all pending works > before doing further cleanup. > > Before this commit arizona_extcon_remove() was doing several > register writes to shut things down before disabling the IRQs > and it was cancelling only 1 of the 3 different works used. > > Move all the register-writes shutting things down to after > the disabling of the IRQs and add the 2 missing > cancel_delayed_work_sync() calls. > > This fixes various possible races on driver unbind. One of which > would always trigger on devices using the mic-clamp feature for > jack detection. The ARIZONA_MICD_CLAMP_MODE_MASK update was > done before disabling the IRQs, causing: > 1. arizona_jackdet() to run > 2. detect a jack being inserted (clamp disabled means jack inserted) > 3. call arizona_start_mic() which: > 3.1 Enables the MICVDD regulator > 3.2 takes a pm_runtime_reference > > And this was all happening after the ARIZONA_MICD_ENA bit clearing, > which would undo 3.1 and 3.2 because the ARIZONA_MICD_CLAMP_MODE_MASK > update was being done after the ARIZONA_MICD_ENA bit clearing. > > So this means that arizona_extcon_remove() would exit with > 1. MICVDD enabled and 2. The pm_runtime_reference being unbalanced. > > MICVDD still being enabled caused the following oops when the > regulator is released by the devm framework: > > [ 2850.745757] [ cut here ] > [ 2850.745827] WARNING: CPU: 2 PID: 2098 at drivers/regulator/core.c:2123 > _regulator_put.part.0+0x19f/0x1b0 > [ 2850.745835] Modules linked in: extcon_arizona ... > ... > [ 2850.746909] Call Trace: > [ 2850.746932] regulator_put+0x2d/0x40 > [ 2850.746946] release_nodes+0x22a/0x260 > [ 2850.746984] __device_release_driver+0x190/0x240 > [ 2850.747002] driver_detach+0xd4/0x120 > ... > [ 2850.747337] ---[ end trace f455dfd7abd9781f ]--- > > Note this oops is just one of various theoretically possible races caused > by the wrong ordering inside arizona_extcon_remove(), this fixes the > ordering fixing all possible races, including the reported oops. > > Signed-off-by: Hans de Goede > --- Sorry yes there are a few rough corners on the extcon stuff, I have clean up series I have been working on as part of upstreaming the Madera extcon hopefully I will get that sent out one day. Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 07/14] extcon: arizona: Fix modalias
On Sun, Dec 27, 2020 at 10:12:25PM +0100, Hans de Goede wrote: > Fix the modalias so that the driver will be loaded automatically. The > module's name is "extcon-arizona", following other extcon module-names. > > But the driver's and platform-device's name is "arizona-extcon" and the > modalias must match that. > > Signed-off-by: Hans de Goede > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 04/14] mfd: arizona: Allow building arizona MFD-core as module
On Sun, Dec 27, 2020 at 10:12:22PM +0100, Hans de Goede wrote: > There is no reason why the arizona core,irq and codec model specific > regmap bits cannot be build as a module. All they do is export symbols > which are used by the arizona-spi and/or arizona-i2c modules, which > themselves can be built as module. > > Change the Kconfig and Makefile arizona bits so that the arizona MFD-core > can be built as a module. > > This is especially useful on x86 platforms with a WM5102 codec, this > allows the arizona MFD driver necessary for the WM5102 codec to be > enabled in generic distro-kernels without growing the base kernel-image > size. > > Signed-off-by: Hans de Goede > --- I think this patch might still cause some issues. ASoC has an idiom where the machine driver does a select on the necessary CODEC drivers. Select doesn't take care of dependencies etc. So I believe if you build the machine driver as built in, it then selects the CODEC as built in. If you have the MFD as a module the build then fails due to the the CODEC calling some Arizona functions. arizona_request_irq, arizona_free_irq, arizona_set_irq_wake On Madera we made the equivalents inline functions to avoid the issue, the same should work here. include/linux/irqchip/irq-madera.h Thanks, Charles
Re: [PATCH 02/14] mfd: arizona: Add MODULE_SOFTDEP("pre: arizona_ldo1")
On Sun, Dec 27, 2020 at 10:12:20PM +0100, Hans de Goede wrote:
> The (shared) probing code of the arizona-i2c and arizona-spi modules
> takes the following steps during init:
>
> 1. Call mfd_add_devices() for a set of early child-devices, this
> includes the arizona_ldo1 device which provides one of the
> core-regulators.
>
> 2. Bulk enable the core-regulators.
>
> 3. Read the device id.
>
> 4. Call mfd_add_devices() for the other child-devices.
>
> This sequence depends on 1. leading to not only the child-device
> being created, but also the driver for the child-device binding
> to it and registering its regulator.
>
> This requires the arizona_ldo1 driver to be loaded before the
> shared probing code runs. Add a doftdep for this to both modules to
> ensure that this requirement is met.
>
> Note this mirrors the existing MODULE_SOFTDEP("pre: wm8994_regulator")
> in the wm8994 code, which has a similar init sequence.
>
> Signed-off-by: Hans de Goede
> ---
Acked-by: Charles Keepax
Thanks,
Charles
Re: [PATCH] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag on Zynq
On Wed, Dec 23, 2020 at 08:24:41PM +0100, Andrew Lunn wrote: > On Wed, Dec 23, 2020 at 06:41:44PM +0000, Charles Keepax wrote: > > A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of > > macb_set_tx_clk were gated on the presence of this flag. > > > > if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG)) > > > > However the flag was not added to anything other than the new > > sama7g5_gem, turning that function call into a no op for all other > > systems. This breaks the networking on Zynq. > > I'm not sure this is the correct fix. I think the original patch might > be broken. Look at the commit message wording: > > The patch adds a new > capability so that macb_set_tx_clock() to not be called for IPs having > this capability > > So MACB_CAPS_CLK_HW_CHG disables something, not enables it. So i > suspect this if statement is wrong and needs fixing. Hmm... good spot, hopefully the original author can comment. The flag name reads to me as clock rate can change, the commit message definitely implies the opposite. So it really depends if this function was intended to be skipped for the sama7g5 gem or emac. Thanks, Charles
[PATCH] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag on Zynq
A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of
macb_set_tx_clk were gated on the presence of this flag.
if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
However the flag was not added to anything other than the new
sama7g5_gem, turning that function call into a no op for all other
systems. This breaks the networking on Zynq.
This patch adds that flag to Zynq config, it is probably needed on other
systems as well but it is hard to know that without having access to
those systems, so I guess we just have to wait to see who else spots
breakage here.
Fixes: daafa1d33cc9 ("net: macb: add capability to not set the clock rate")
Signed-off-by: Charles Keepax
---
drivers/net/ethernet/cadence/macb_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c
b/drivers/net/ethernet/cadence/macb_main.c
index d5d910916c2e8..590116b236ef7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4536,7 +4536,7 @@ static const struct macb_config zynqmp_config = {
static const struct macb_config zynq_config = {
.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF |
- MACB_CAPS_NEEDS_RSTONUBR,
+ MACB_CAPS_NEEDS_RSTONUBR | MACB_CAPS_CLK_HW_CHG,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = macb_init,
--
2.11.0
Re: [PATCH] ASoC: meson: axg-tdm-interface: fix loopback
On Thu, Dec 17, 2020 at 04:08:12PM +0100, Jerome Brunet wrote:
> When the axg-tdm-interface was introduced, the backend DAI was marked as an
> endpoint when DPCM was walking the DAPM graph to find a its BE.
>
> It is no longer the case since this
> commit 8dd26dff00c0 ("ASoC: dapm: Fix handling of custom_stop_condition on
> DAPM graph walks")
> Because of this, when DPCM finds a BE it does everything it needs on the
> DAIs but it won't power up the widgets between the FE and the BE if there
> is no actual endpoint after the BE.
>
> On meson-axg HWs, the loopback is a special DAI of the tdm-interface BE.
> It is only linked to the dummy codec since there no actual HW after it.
> >From the DAPM perspective, the DAI has no endpoint. Because of this, the TDM
> decoder, which is a widget between the FE and BE is not powered up.
>
> >From the user perspective, everything seems fine but no data is produced.
>
> Connecting the Loopback DAI to a dummy DAPM endpoint solves the problem.
>
> Fixes: 8dd26dff00c0 ("ASoC: dapm: Fix handling of custom_stop_condition on
> DAPM graph walks")
> Cc: Charles Keepax
> Signed-off-by: Jerome Brunet
> ---
Reviewed-by: Charles Keepax
Thanks,
Charles
Re: [PATCH] ASoC: dapm: remove widget from dirty list on free
On Sat, Dec 12, 2020 at 05:20:12PM -0800, Thomas Hebb wrote:
> A widget's "dirty" list_head, much like its "list" list_head, eventually
> chains back to a list_head on the snd_soc_card itself. This means that
> the list can stick around even after the widget (or all widgets) have
> been freed. Currently, however, widgets that are in the dirty list when
> freed remain there, corrupting the entire list and leading to memory
> errors and undefined behavior when the list is next accessed or
> modified.
>
> I encountered this issue when a component failed to probe relatively
> late in snd_soc_bind_card(), causing it to bail out and call
> soc_cleanup_card_resources(), which eventually called
> snd_soc_dapm_free() with widgets that were still dirty from when they'd
> been added.
>
> Fixes: db432b414e20 ("ASoC: Do DAPM power checks only for widgets changed
> since last run")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Thomas Hebb
> ---
>
> sound/soc/soc-dapm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index 7f87b449f950..148c095df27b 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -2486,6 +2486,7 @@ void snd_soc_dapm_free_widget(struct
> snd_soc_dapm_widget *w)
> enum snd_soc_dapm_direction dir;
>
> list_del(>list);
> + list_del(>dirty);
I can't help but wonder if we should be taking the DAPM lock for
snd_soc_dapm_free_widgets. However your patch doesn't look like it
is making that any more scary and looks like we should be making
sure we remove the widget from the dirty list.
Reviewed-by: Charles Keepax
Thanks,
Charles
Re: [PATCH] mfd: wm831x-auxadc: Prevent use after free in wm831x_auxadc_read_irq()
On Mon, Dec 14, 2020 at 02:59:10PM +0300, Dan Carpenter wrote:
> The "req" struct is always added to the "wm831x->auxadc_pending" list,
> but it's only removed from the list on the success path. If a failure
> occurs then the "req" struct is freed but it's still on the list,
> leading to a use after free.
>
> Fixes: 78bb3688ea18 ("mfd: Support multiple active WM831x AUXADC conversions")
> Signed-off-by: Dan Carpenter
> ---
Acked-by: Charles Keepax
Thanks,
Charles
Re: [PATCH -next] regulator: wm831x-isink: convert comma to semicolon
On Fri, Dec 11, 2020 at 04:44:40PM +0800, Zheng Yongjun wrote: > Replace a comma between expression statements by a semicolon. > > Signed-off-by: Zheng Yongjun > --- Acked-by: Charles Keepax Thanks, Charles
Re: wm8960: add DAC Slope switch
On Tue, Nov 24, 2020 at 06:23:13PM +0100, Lukas F. Hartmann wrote:
> The WM8960 DAC has a "DAC Slope" switch that can toggle between two
> different output filter curves. This patch adds support for it.
>
> Signed-off-by: Lukas F. Hartmann
> ---
> +static int wm8960_put_dacslope(struct snd_kcontrol *kcontrol,
> +struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component =
> snd_soc_kcontrol_component(kcontrol);
> + struct wm8960_priv *wm8960 = snd_soc_component_get_drvdata(component);
> + unsigned int val = ucontrol->value.integer.value[0];
> +
> + if (val > 1)
> + return -EINVAL;
> +
> + wm8960->dacslope = val;
> +
> + return snd_soc_component_update_bits(component, WM8960_DACCTL1,
> +0x2, val<<1);
Is the DAC Slope switch not in DACCTL2?
Thanks,
Charles
Re: [PATCH 14/16] mfd: wm8994: drop of_match_ptr from of_device_id table
On Fri, Nov 20, 2020 at 05:21:31PM +0100, Krzysztof Kozlowski wrote: > The driver can match only via the DT table so the table should be always > used and the of_match_ptr does not have any sense (this also allows ACPI > matching via PRP0001, even though it is not relevant here). This fixes > compile warning (!CONFIG_OF on x86_64): > > drivers/mfd/wm8994-core.c:618:34: warning: ‘wm8994_of_match’ defined but > not used [-Wunused-const-variable=] > > Signed-off-by: Krzysztof Kozlowski > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH] regmap: Fix order of regmap write log
On Thu, Nov 12, 2020 at 03:02:17PM +, Lucas Tanure wrote: > _regmap_write can trigger a _regmap_select_page, which will call > another _regmap_write that will be executed first, but the log shows > the inverse order > > Also, keep consistency with _regmap_read which only logs in case of > success > > Signed-off-by: Lucas Tanure > --- Reviewed-by: Charles Keepax Thanks, Charles
Re: [PATCH v1] spi: fix client driver breakages when using GPIO descriptors
On Wed, Nov 11, 2020 at 11:24:14AM -0500, Sven Van Asbroeck wrote:
> On Wed, Nov 11, 2020 at 10:48 AM Mark Brown wrote:
> >
> > Applied to
> >
> >https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
>
> Thank you !
>
> Now that our minds are still focused on this subject, should
> commit 138c9c32f090 ("spi: spidev: Fix CS polarity if GPIO descriptors
> are used")
> be reverted?
>
> This fixed spidev to deal with SPI_CS_HIGH on gpiod.
> But after our fix, its behaviour will probably be broken again.
>
> Another candidate for revert is
> commit ada9e3fcc175 ("spi: dw: Correct handling of native chipselect")
> although I don't understand that code well enough to be sure.
>
> Adding Charles Keepax.
Looks like the code has changed a fair amount since my patch. The
important detail from it was trying to clarify the semantics of the
controller->set_cs callback. That function is called with a boolean
argument and that argument could have two possible meanings:
1) True means apply a high logic level to the chip select line.
2) True mean apply chip select.
Under interpretation 2) the chip select line would be set to a
different logic level depending on if the device is active high or
active low.
If I remember correctly at the point of my patch the core had just
changed between the two a couple of times but now consistently did 1)
(and looks like it still does), my patch intended to updated the
spi-dw driver to match that, as my SPI had stopped working. I think
it then turned out, my patch broke some other use-cases and that
the bit in the IP basically had 2) semantics in hardware. Which is
what this patch fixed:
commit 9aea644ca17b ("spi: dw: Fix native CS being unset")
After that patch my patch is mostly replaced so I don't think it
would make any sense to revert my patch at this point, and I
don't think your patch will break the spi-dw driver. I don't
have easy access to the hardware right now to test, but I will
give it is quick run when that option becomes available to me
again.
Your fix looks good to me, but I suspect you do need to fix the
spidev stuff although I have haven't looked at that in detail.
Thanks,
Charles
Re: [PATCH 24/25] ASoC: wm8994: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 03:43:00PM +0800, Coiby Xu wrote:
> SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
>
> Signed-off-by: Coiby Xu
> ---
> sound/soc/codecs/wm8994.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
> index fc9ea198ac79..9294ad06f76d 100644
> --- a/sound/soc/codecs/wm8994.c
> +++ b/sound/soc/codecs/wm8994.c
> @@ -4656,7 +4656,6 @@ static int wm8994_remove(struct platform_device *pdev)
> return 0;
> }
>
> -#ifdef CONFIG_PM_SLEEP
> static int wm8994_suspend(struct device *dev)
> {
> struct wm8994_priv *wm8994 = dev_get_drvdata(dev);
> @@ -4681,7 +4680,6 @@ static int wm8994_resume(struct device *dev)
>
> return 0;
> }
> -#endif
>
> static const struct dev_pm_ops wm8994_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(wm8994_suspend, wm8994_resume)
Not sure this really makes sense, what is going to stop the
unused function warning if PM isn't configured?
Thanks,
Charles
[PATCH RESEND 1/2] mfd: madera: Add reset as part of resume
The DCVDD supply does not always power down when the CODEC enters
suspend, for example shared regulators or always-on regulators. In
these cases if a register is written back to the default value whilst
the CODEC is in suspend that register will retain the previous value.
As DCVDD never powered down, the register retains its old value and
as the cache sync only synchronises registers that differ from the
default the new value is never written out.
Ensure the registers are in the expected state after suspend by always
resetting the CODEC on resume. This also has the benefit of being
recommended by the datasheet for DCVDD supplies that take longer than
2mS to rise.
Signed-off-by: Charles Keepax
---
drivers/mfd/madera-core.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index 4ed6ad8ce0020..a9c6f0833f327 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -291,6 +291,8 @@ static int __maybe_unused madera_runtime_resume(struct
device *dev)
dev_dbg(dev, "Leaving sleep mode\n");
+ madera_enable_hard_reset(madera);
+
ret = regulator_enable(madera->dcvdd);
if (ret) {
dev_err(dev, "Failed to enable DCVDD: %d\n", ret);
@@ -300,7 +302,19 @@ static int __maybe_unused madera_runtime_resume(struct
device *dev)
regcache_cache_only(madera->regmap, false);
regcache_cache_only(madera->regmap_32bit, false);
- usleep_range(MADERA_RESET_MIN_US, MADERA_RESET_MAX_US);
+ madera_disable_hard_reset(madera);
+
+ if (!madera->pdata.reset) {
+ ret = madera_wait_for_boot(madera);
+ if (ret)
+ goto err;
+
+ ret = madera_soft_reset(madera);
+ if (ret) {
+ dev_err(dev, "Failed to reset: %d\n", ret);
+ goto err;
+ }
+ }
ret = madera_wait_for_boot(madera);
if (ret)
--
2.11.0
[PATCH RESEND 2/2] mfd: madera: Add special errata reset handling for cs47l15
An errata exists for cs47l15 where the reset must be handled
differently and removed before DCVDD is applied. A soft reset is used
for situations where a reset is required to reset state. This does
however, make this part unsuitable for DCVDD supplies with a rise time
greater than 2mS.
Signed-off-by: Charles Keepax
---
drivers/mfd/madera-core.c | 25 -
include/linux/mfd/madera/core.h | 1 +
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index a9c6f0833f327..a2abc0094def7 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -38,6 +38,9 @@
#define MADERA_RESET_MIN_US2000
#define MADERA_RESET_MAX_US3000
+#define ERRATA_DCVDD_MIN_US1
+#define ERRATA_DCVDD_MAX_US15000
+
static const char * const madera_core_supplies[] = {
"AVDD",
"DBVDD1",
@@ -291,7 +294,8 @@ static int __maybe_unused madera_runtime_resume(struct
device *dev)
dev_dbg(dev, "Leaving sleep mode\n");
- madera_enable_hard_reset(madera);
+ if (!madera->reset_errata)
+ madera_enable_hard_reset(madera);
ret = regulator_enable(madera->dcvdd);
if (ret) {
@@ -302,9 +306,12 @@ static int __maybe_unused madera_runtime_resume(struct
device *dev)
regcache_cache_only(madera->regmap, false);
regcache_cache_only(madera->regmap_32bit, false);
- madera_disable_hard_reset(madera);
+ if (madera->reset_errata)
+ usleep_range(ERRATA_DCVDD_MIN_US, ERRATA_DCVDD_MAX_US);
+ else
+ madera_disable_hard_reset(madera);
- if (!madera->pdata.reset) {
+ if (!madera->pdata.reset || madera->reset_errata) {
ret = madera_wait_for_boot(madera);
if (ret)
goto err;
@@ -503,6 +510,8 @@ int madera_dev_init(struct madera *madera)
*/
switch (madera->type) {
case CS47L15:
+ madera->reset_errata = true;
+ break;
case CS47L35:
case CS47L90:
case CS47L91:
@@ -553,13 +562,19 @@ int madera_dev_init(struct madera *madera)
goto err_dcvdd;
}
+ if (madera->reset_errata)
+ madera_disable_hard_reset(madera);
+
ret = regulator_enable(madera->dcvdd);
if (ret) {
dev_err(dev, "Failed to enable DCVDD: %d\n", ret);
goto err_enable;
}
- madera_disable_hard_reset(madera);
+ if (madera->reset_errata)
+ usleep_range(ERRATA_DCVDD_MIN_US, ERRATA_DCVDD_MAX_US);
+ else
+ madera_disable_hard_reset(madera);
regcache_cache_only(madera->regmap, false);
regcache_cache_only(madera->regmap_32bit, false);
@@ -667,7 +682,7 @@ int madera_dev_init(struct madera *madera)
* It looks like a device we support. If we don't have a hard reset
* we can now attempt a soft reset.
*/
- if (!madera->pdata.reset) {
+ if (!madera->pdata.reset || madera->reset_errata) {
ret = madera_soft_reset(madera);
if (ret)
goto err_reset;
diff --git a/include/linux/mfd/madera/core.h b/include/linux/mfd/madera/core.h
index ad2c138105d4b..03a8a788424a2 100644
--- a/include/linux/mfd/madera/core.h
+++ b/include/linux/mfd/madera/core.h
@@ -186,6 +186,7 @@ struct madera {
struct regulator_bulk_data core_supplies[MADERA_MAX_CORE_SUPPLIES];
struct regulator *dcvdd;
bool internal_dcvdd;
+ bool reset_errata;
struct madera_pdata pdata;
--
2.11.0
Re: [PATCH] power: remove unneeded break
On Mon, Oct 19, 2020 at 11:59:37AM -0700, t...@redhat.com wrote: > From: Tom Rix > > A break is not needed if it is preceded by a goto > > Signed-off-by: Tom Rix > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 1/8] ASoC: wm8350: use semicolons rather than commas to separate statements
On Sun, Oct 11, 2020 at 11:19:32AM +0200, Julia Lawall wrote: > Replace commas with semicolons. What is done is essentially described by > the following Coccinelle semantic patch (http://coccinelle.lip6.fr/): > > // > @@ expression e1,e2; @@ > e1 > -, > +; > e2 > ... when any > // > > Signed-off-by: Julia Lawall > > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 7/8] ASoC: madera: use semicolons rather than commas to separate statements
On Sun, Oct 11, 2020 at 11:19:38AM +0200, Julia Lawall wrote: > Replace commas with semicolons. What is done is essentially described by > the following Coccinelle semantic patch (http://coccinelle.lip6.fr/): > > // > @@ expression e1,e2; @@ > e1 > -, > +; > e2 > ... when any > // > > Signed-off-by: Julia Lawall > > --- Acked-by: Charles Keepax Thanks, Charles
[PATCH 1/2] mfd: madera: Add reset as part of resume
The DCVDD supply does not always power down when the CODEC enters
suspend, for example shared regulators or always-on regulators. In
these cases if a register is written back to the default value whilst
the CODEC is in suspend that register will retain the previous value.
As DCVDD never powered down, the register retains its old value and
as the cache sync only synchronises registers that differ from the
default the new value is never written out.
Ensure the registers are in the expected state after suspend by always
resetting the CODEC on resume. This also has the benefit of being
recommended by the datasheet for DCVDD supplies that take longer than
2mS to rise.
Signed-off-by: Charles Keepax
---
drivers/mfd/madera-core.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index 4ed6ad8ce0020..a9c6f0833f327 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -291,6 +291,8 @@ static int __maybe_unused madera_runtime_resume(struct
device *dev)
dev_dbg(dev, "Leaving sleep mode\n");
+ madera_enable_hard_reset(madera);
+
ret = regulator_enable(madera->dcvdd);
if (ret) {
dev_err(dev, "Failed to enable DCVDD: %d\n", ret);
@@ -300,7 +302,19 @@ static int __maybe_unused madera_runtime_resume(struct
device *dev)
regcache_cache_only(madera->regmap, false);
regcache_cache_only(madera->regmap_32bit, false);
- usleep_range(MADERA_RESET_MIN_US, MADERA_RESET_MAX_US);
+ madera_disable_hard_reset(madera);
+
+ if (!madera->pdata.reset) {
+ ret = madera_wait_for_boot(madera);
+ if (ret)
+ goto err;
+
+ ret = madera_soft_reset(madera);
+ if (ret) {
+ dev_err(dev, "Failed to reset: %d\n", ret);
+ goto err;
+ }
+ }
ret = madera_wait_for_boot(madera);
if (ret)
--
2.11.0
[PATCH 2/2] mfd: madera: Add special errata reset handling for cs47l15
An errata exists for cs47l15 where the reset must be handled
differently and removed before DCVDD is applied. A soft reset is used
for situations where a reset is required to reset state. This does
however, make this part unsuitable for DCVDD supplies with a rise time
greater than 2mS.
Signed-off-by: Charles Keepax
---
drivers/mfd/madera-core.c | 25 -
include/linux/mfd/madera/core.h | 1 +
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index a9c6f0833f327..a2abc0094def7 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -38,6 +38,9 @@
#define MADERA_RESET_MIN_US2000
#define MADERA_RESET_MAX_US3000
+#define ERRATA_DCVDD_MIN_US1
+#define ERRATA_DCVDD_MAX_US15000
+
static const char * const madera_core_supplies[] = {
"AVDD",
"DBVDD1",
@@ -291,7 +294,8 @@ static int __maybe_unused madera_runtime_resume(struct
device *dev)
dev_dbg(dev, "Leaving sleep mode\n");
- madera_enable_hard_reset(madera);
+ if (!madera->reset_errata)
+ madera_enable_hard_reset(madera);
ret = regulator_enable(madera->dcvdd);
if (ret) {
@@ -302,9 +306,12 @@ static int __maybe_unused madera_runtime_resume(struct
device *dev)
regcache_cache_only(madera->regmap, false);
regcache_cache_only(madera->regmap_32bit, false);
- madera_disable_hard_reset(madera);
+ if (madera->reset_errata)
+ usleep_range(ERRATA_DCVDD_MIN_US, ERRATA_DCVDD_MAX_US);
+ else
+ madera_disable_hard_reset(madera);
- if (!madera->pdata.reset) {
+ if (!madera->pdata.reset || madera->reset_errata) {
ret = madera_wait_for_boot(madera);
if (ret)
goto err;
@@ -503,6 +510,8 @@ int madera_dev_init(struct madera *madera)
*/
switch (madera->type) {
case CS47L15:
+ madera->reset_errata = true;
+ break;
case CS47L35:
case CS47L90:
case CS47L91:
@@ -553,13 +562,19 @@ int madera_dev_init(struct madera *madera)
goto err_dcvdd;
}
+ if (madera->reset_errata)
+ madera_disable_hard_reset(madera);
+
ret = regulator_enable(madera->dcvdd);
if (ret) {
dev_err(dev, "Failed to enable DCVDD: %d\n", ret);
goto err_enable;
}
- madera_disable_hard_reset(madera);
+ if (madera->reset_errata)
+ usleep_range(ERRATA_DCVDD_MIN_US, ERRATA_DCVDD_MAX_US);
+ else
+ madera_disable_hard_reset(madera);
regcache_cache_only(madera->regmap, false);
regcache_cache_only(madera->regmap_32bit, false);
@@ -667,7 +682,7 @@ int madera_dev_init(struct madera *madera)
* It looks like a device we support. If we don't have a hard reset
* we can now attempt a soft reset.
*/
- if (!madera->pdata.reset) {
+ if (!madera->pdata.reset || madera->reset_errata) {
ret = madera_soft_reset(madera);
if (ret)
goto err_reset;
diff --git a/include/linux/mfd/madera/core.h b/include/linux/mfd/madera/core.h
index ad2c138105d4b..03a8a788424a2 100644
--- a/include/linux/mfd/madera/core.h
+++ b/include/linux/mfd/madera/core.h
@@ -186,6 +186,7 @@ struct madera {
struct regulator_bulk_data core_supplies[MADERA_MAX_CORE_SUPPLIES];
struct regulator *dcvdd;
bool internal_dcvdd;
+ bool reset_errata;
struct madera_pdata pdata;
--
2.11.0
Re: [PATCH AUTOSEL 5.8 14/29] regmap: debugfs: Fix handling of name string for debugfs init delays
On Mon, Sep 28, 2020 at 09:30:11PM -0400, Sasha Levin wrote: > From: Charles Keepax > > [ Upstream commit 94cc89eb8fa5039fcb6e3e3d50f929ddcccee095 ] > > In regmap_debugfs_init the initialisation of the debugfs is delayed > if the root node isn't ready yet. Most callers of regmap_debugfs_init > pass the name from the regmap_config, which is considered temporary > ie. may be unallocated after the regmap_init call returns. This leads > to a potential use after free, where config->name has been freed by > the time it is used in regmap_debugfs_initcall. > Afraid this patch had some issues if you are back porting it you definitely need to take these two patches as well: commit 1d512ee861b80da63cbc501b973c53131aa22f29 regmap: debugfs: Fix more error path regressions commit d36cb0205f034e943aa29e35b59c6a441f0056b5 regmap: debugfs: Add back in erroneously removed initialisation of ret Thanks, Charles
Re: [PATCH 4/8] mfd: wm: Constify static struct resource
On Tue, Sep 22, 2020 at 09:26:55PM +0200, Rikard Falkeborn wrote: > Constify a number of static struct resource. The only usage of the > structs are to assign their address to the resources field in the > mfd_cell struct. This allows the compiler to put them in read-only > memory. Done with the help of Coccinelle. > > Signed-off-by: Rikard Falkeborn > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 41/42] mfd: wm8400: use PLATFORM_DEVID_NONE
On Mon, Sep 21, 2020 at 10:50:15PM +0200, Krzysztof Kozlowski wrote: > Use PLATFORM_DEVID_NONE define instead of "-1" value because: > - it brings some meaning, > - it might point attention why auto device ID was not used. > > Signed-off-by: Krzysztof Kozlowski > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 42/42] mfd: wm8994: use PLATFORM_DEVID_NONE
On Mon, Sep 21, 2020 at 10:50:16PM +0200, Krzysztof Kozlowski wrote: > Use PLATFORM_DEVID_NONE define instead of "-1" value because: > - it brings some meaning, > - it might point attention why auto device ID was not used. > > Signed-off-by: Krzysztof Kozlowski > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 01/42] mfd: arizona: use PLATFORM_DEVID_NONE
On Mon, Sep 21, 2020 at 10:49:35PM +0200, Krzysztof Kozlowski wrote: > Use PLATFORM_DEVID_NONE define instead of "-1" value because: > - it brings some meaning, > - it might point attention why auto device ID was not used. > > Signed-off-by: Krzysztof Kozlowski > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH v2 09/13] dt-bindings: pinctrl: include common schema in GPIO controllers
On Thu, Sep 17, 2020 at 06:52:57PM +0200, Krzysztof Kozlowski wrote: > Include the common GPIO schema in GPIO controllers to be sure all common > properties are properly validated. > > Signed-off-by: Krzysztof Kozlowski > > --- For the Cirrus bits: Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH v2 08/13] dt-bindings: mfd: include common schema in GPIO controllers
On Thu, Sep 17, 2020 at 06:52:56PM +0200, Krzysztof Kozlowski wrote: > Include the common GPIO schema in GPIO controllers to be sure all common > properties are properly validated. > > Signed-off-by: Krzysztof Kozlowski > > --- For the Cirrus/Wolfson bits: Acked-by: Charles Keepax Thanks, Charles
[PATCH] regmap: debugfs: Fix more error path regressions
Many error paths in __regmap_init rely on ret being pre-initialised to
-EINVAL, add an extra initialisation in after the new call to
regmap_set_name.
Fixes: 94cc89eb8fa5 ("regmap: debugfs: Fix handling of name string for debugfs
init delays")
Reported-by: Dan Carpenter
Signed-off-by: Charles Keepax
---
drivers/base/regmap/regmap.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index b24f14ea96d8a..4c2a114584236 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -723,6 +723,8 @@ struct regmap *__regmap_init(struct device *dev,
if (ret)
goto err_map;
+ ret = -EINVAL; /* Later error paths rely on this */
+
if (config->disable_locking) {
map->lock = map->unlock = regmap_lock_unlock_none;
map->can_sleep = config->can_sleep;
--
2.11.0
Re: [PATCH] regmap: debugfs: Add back in erroneously removed initialisation of ret
On Fri, Sep 18, 2020 at 03:38:43PM +0300, Dan Carpenter wrote:
> On Fri, Sep 18, 2020 at 12:20:02PM +0100, Charles Keepax wrote:
> > Fixes: 94cc89eb8fa5 ("regmap: debugfs: Fix handling of name string for
> > debugfs init delays")
> > Reported-by: kernel test robot
> > Reported-by: Dan Carpenter
> > Signed-off-by: Charles Keepax
> > ---
> > drivers/base/regmap/regmap.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> > index d0f7cc574ff34..b24f14ea96d8a 100644
> > --- a/drivers/base/regmap/regmap.c
> > +++ b/drivers/base/regmap/regmap.c
> > @@ -706,9 +706,9 @@ struct regmap *__regmap_init(struct device *dev,
> > const char *lock_name)
> > {
> > struct regmap *map;
> > + int ret = -EINVAL;
> > enum regmap_endian reg_endian, val_endian;
> > int i, j;
> > - int ret;
> >
> > if (!config)
> > goto err;
>
> Hi Charles, this isn't enough. There are several goto err_map; paths
> were "ret" is zero. That will result in an Oops in the caller. It has
> to be an error code.
>
Aye, sorry thought it was just cause I removed the initialisation
but yeah there is more fall out. Apologies for mucking this up,
third time lucky hopefully.
Thanks,
Charles
[PATCH] regmap: debugfs: Add back in erroneously removed initialisation of ret
Fixes: 94cc89eb8fa5 ("regmap: debugfs: Fix handling of name string for debugfs
init delays")
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Signed-off-by: Charles Keepax
---
drivers/base/regmap/regmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d0f7cc574ff34..b24f14ea96d8a 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -706,9 +706,9 @@ struct regmap *__regmap_init(struct device *dev,
const char *lock_name)
{
struct regmap *map;
+ int ret = -EINVAL;
enum regmap_endian reg_endian, val_endian;
int i, j;
- int ret;
if (!config)
goto err;
--
2.11.0
[PATCH v2] regmap: debugfs: Fix handling of name string for debugfs init delays
In regmap_debugfs_init the initialisation of the debugfs is delayed
if the root node isn't ready yet. Most callers of regmap_debugfs_init
pass the name from the regmap_config, which is considered temporary
ie. may be unallocated after the regmap_init call returns. This leads
to a potential use after free, where config->name has been freed by
the time it is used in regmap_debugfs_initcall.
This situation can be seen on Zynq, where the architecture init_irq
callback registers a syscon device, using a local variable for the
regmap_config. As init_irq is very early in the platform bring up the
regmap debugfs root isn't ready yet. Although this doesn't crash it
does result in the debugfs entry not having the correct name.
Regmap already sets map->name from config->name on the regmap_init
path and the fact that a separate field is used to pass the name
to regmap_debugfs_init appears to be an artifact of the debugfs
name being added before the map name. As such this patch updates
regmap_debugfs_init to use map->name, which is already duplicated from
the config avoiding the issue.
This does however leave two lose ends, both regmap_attach_dev and
regmap_reinit_cache can be called after a regmap is registered and
would have had the effect of applying a new name to the debugfs
entries. In both of these cases it was chosen to update the map
name. In the case of regmap_attach_dev there are 3 users that
currently use this function to update the name, thus doing so avoids
changes for those users and it seems reasonable that attaching
a device would want to set the name of the map. In the case of
regmap_reinit_cache the primary use-case appears to be devices that
need some register access to identify the device (for example devices
in the same family) and then update the cache to match the exact
hardware. Whilst no users do currently update the name here, given the
use-case it seemed reasonable the name might want to be updated once
the device is better identified.
Signed-off-by: Charles Keepax
---
drivers/base/regmap/internal.h | 4 ++--
drivers/base/regmap/regmap-debugfs.c | 7 +++---
drivers/base/regmap/regmap.c | 44 +++-
3 files changed, 38 insertions(+), 17 deletions(-)
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 8a59359e145f6..e7ab94cc327a9 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -220,7 +220,7 @@ struct regmap_field {
#ifdef CONFIG_DEBUG_FS
extern void regmap_debugfs_initcall(void);
-extern void regmap_debugfs_init(struct regmap *map, const char *name);
+extern void regmap_debugfs_init(struct regmap *map);
extern void regmap_debugfs_exit(struct regmap *map);
static inline void regmap_debugfs_disable(struct regmap *map)
@@ -230,7 +230,7 @@ static inline void regmap_debugfs_disable(struct regmap
*map)
#else
static inline void regmap_debugfs_initcall(void) { }
-static inline void regmap_debugfs_init(struct regmap *map, const char *name) {
}
+static inline void regmap_debugfs_init(struct regmap *map) { }
static inline void regmap_debugfs_exit(struct regmap *map) { }
static inline void regmap_debugfs_disable(struct regmap *map) { }
#endif
diff --git a/drivers/base/regmap/regmap-debugfs.c
b/drivers/base/regmap/regmap-debugfs.c
index f58baff2be0af..b6d63ef16b442 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -17,7 +17,6 @@
struct regmap_debugfs_node {
struct regmap *map;
- const char *name;
struct list_head link;
};
@@ -544,11 +543,12 @@ static const struct file_operations
regmap_cache_bypass_fops = {
.write = regmap_cache_bypass_write_file,
};
-void regmap_debugfs_init(struct regmap *map, const char *name)
+void regmap_debugfs_init(struct regmap *map)
{
struct rb_node *next;
struct regmap_range_node *range_node;
const char *devname = "dummy";
+ const char *name = map->name;
/*
* Userspace can initiate reads from the hardware over debugfs.
@@ -569,7 +569,6 @@ void regmap_debugfs_init(struct regmap *map, const char
*name)
if (!node)
return;
node->map = map;
- node->name = name;
mutex_lock(_debugfs_early_lock);
list_add(>link, _debugfs_early_list);
mutex_unlock(_debugfs_early_lock);
@@ -679,7 +678,7 @@ void regmap_debugfs_initcall(void)
mutex_lock(_debugfs_early_lock);
list_for_each_entry_safe(node, tmp, _debugfs_early_list, link) {
- regmap_debugfs_init(node->map, node->name);
+ regmap_debugfs_init(node->map);
list_del(>link);
kfree(node);
}
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 2807e544658e0..4876e5f543c70 100644
--- a/drivers/base/regm
[PATCH] regmap: debugfs: Duplicate name string if delaying debugfs init
In regmap_debugfs_init the initialisation of the debugfs is delayed
if the root node isn't ready yet. Most callers of regmap_debugfs_init
pass the name from the regmap_config, which is considered temporary
ie. may be unallocated after the regmap_init call returns. This leads
to a potential use after free, where config->name has been freed by
the time it is used in regmap_debugfs_initcall.
This situation can be seen on Zynq, where the architecture init_irq
callback registers a syscon device, using a local variable for the
regmap_config. As init_irq is very early in the platform bring up the
regmap debugfs root isn't ready yet. Although this doesn't crash it
does result in the debugfs entry not having the correct name.
Signed-off-by: Charles Keepax
---
drivers/base/regmap/regmap-debugfs.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/base/regmap/regmap-debugfs.c
b/drivers/base/regmap/regmap-debugfs.c
index f58baff2be0af..184fc327192bf 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -569,7 +569,12 @@ void regmap_debugfs_init(struct regmap *map, const char
*name)
if (!node)
return;
node->map = map;
- node->name = name;
+ node->name = kstrdup(name, GFP_KERNEL);
+ if (!node->name) {
+ kfree(node);
+ return;
+ }
+
mutex_lock(_debugfs_early_lock);
list_add(>link, _debugfs_early_list);
mutex_unlock(_debugfs_early_lock);
@@ -681,6 +686,7 @@ void regmap_debugfs_initcall(void)
list_for_each_entry_safe(node, tmp, _debugfs_early_list, link) {
regmap_debugfs_init(node->map, node->name);
list_del(>link);
+ kfree(node->name);
kfree(node);
}
mutex_unlock(_debugfs_early_lock);
--
2.11.0
Re: [PATCH] regmap: debugfs: Duplicate name string if delaying debugfs init
On Wed, Sep 16, 2020 at 05:14:18PM +0100, Mark Brown wrote:
> On Wed, Sep 16, 2020 at 04:44:33PM +0100, Charles Keepax wrote:
>
> > - node->name = name;
> > + node->name = kstrdup(name, GFP_KERNEL);
> > + if (!node->name) {
>
> Two things here - one is that this should be kstrdup_const(), the other
> is that we already took a copy of the name in __regmap_init() so the
> thing to do here is to change the regmap_debugfs_init() call there to
> use the copy we just made rather than the copy in the config.
Thanks Mark, I will have a look and update.
Thanks,
Charles
Re: [linux-sunxi] [PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit support
On Thu, Sep 03, 2020 at 09:40:23AM +0200, Maxime Ripard wrote: > On Wed, Sep 02, 2020 at 09:22:33PM -0500, Samuel Holland wrote: > > On 9/2/20 1:10 PM, Jernej Škrabec wrote: > > > Hi Samuel! > > > > > > Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a): > > >> On 7/4/20 6:38 AM, Clément Péron wrote: > > >>> From: Marcus Cooper > > >>> > > >>> Extend the functionality of the driver to include support of 20 and > > >>> 24 bits per sample. > > >>> > > >>> Signed-off-by: Marcus Cooper > > >>> Signed-off-by: Clément Péron > > >>> --- > > >>> > > >>> sound/soc/sunxi/sun4i-i2s.c | 11 +-- > > >>> 1 file changed, 9 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > >>> index f78167e152ce..bc7f9343bc7a 100644 > > >>> --- a/sound/soc/sunxi/sun4i-i2s.c > > >>> +++ b/sound/soc/sunxi/sun4i-i2s.c > > >>> @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct > > >>> snd_pcm_substream *substream,> > > >>> case 16: > > >>> width = DMA_SLAVE_BUSWIDTH_2_BYTES; > > >>> break; > > >>> > > >>> + case 32: > > >>> + width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > >>> + break; > > >> > > >> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for > > >> a 32 > > >> bit width, but it needs to return 3. > > > > > > I'm not sure what has WSS with physical width and DMA? > > > > This is the change where creating a S24_LE stream no longer fails with > > -EINVAL. > > So this is the change where userspace stops downsampling 24-bit audio > > sources. > > So this is the change where playback of 24-bit audio sources breaks, > > because WSS > > is programmed wrong. > > > > >> As a side note, I wonder why we use the physical width (the spacing > > >> between > > >> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per > > >> sample > > >> in RAM, which we need for DMA. But I don't see why we would want to > > >> transmit the padding over the wire. I would expect it to be transmitted > > >> the > > >> same as S24_3LE (which has no padding). It did not matter before, because > > >> the only supported format had no padding. > > > > > > Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bus > > > width, so if sample is 24 bits in size, we have no other way but to > > > transmit > > > padding too. > > > > I understand why we do 4 byte DMA from RAM <=> I2S FIFO; that was not my > > question. I'm referring to the actual wire format (FIFO <=> PCM_DIN/DOUT). > > The > > sample is already truncated from 32 bits to 24 bits in the FIFO -- that's > > what > > TXIM and RXOM in FIFO_CTRL control. > > > > If a sample is 24 bits wide, why would we send 32 BCLKs for every LRCK? I > > would > > expect the slot width to match the sample resolution by default. But yet we > > have > > this code in the driver: > > > > unsigned int word_size = params_width(params); > > unsigned int slot_width = params_physical_width(params); > > > > I think slot_width should be the same as word_size, and I suggest changing > > it > > before adding 20/24-bit support. > > Generally speaking, the slot width doesn't necessarily match the > physical width. With TDM for example you may very well have slots > larger than their samples. > > That being said, S24 is explicitly a format where you send a sample of > 24 bits in a 32-bit word (in the lowest three bytes, little endian) > > See: > https://elixir.bootlin.com/linux/v5.9-rc3/source/sound/core/pcm_misc.c#L75 > https://mailman.alsa-project.org/pipermail/alsa-devel/2013-April/061073.html > > 24 bits of data over three bytes like you suggest is S24_3LE > My understanding is physical_width refers to the in memory representation, but shouldn't be used to control the slot width on the bus. If not specified otherwise (say through the set_tdm callback), and if the appropriate BCLK is supported, then the slot should be just large enough to hold the data. Thanks, Charles
[PATCH] regulator: lochnagar: Add additional VDDCORE range
In the case of an unrecognised mini-card the Lochnagar will not
initialise the VDDCORE voltage register leading to a value outside of the
current range. Add an additional range to cover these values, initially
this wasn't done since they are duplicates of the existing minimum
value.
Signed-off-by: Charles Keepax
---
drivers/regulator/lochnagar-regulator.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/regulator/lochnagar-regulator.c
b/drivers/regulator/lochnagar-regulator.c
index 5ea3e41416849..cb71fa5f43c3e 100644
--- a/drivers/regulator/lochnagar-regulator.c
+++ b/drivers/regulator/lochnagar-regulator.c
@@ -98,6 +98,7 @@ static const struct regulator_ops lochnagar_vddcore_ops = {
};
static const struct linear_range lochnagar_vddcore_ranges[] = {
+ REGULATOR_LINEAR_RANGE(60, 0,0x7, 0),
REGULATOR_LINEAR_RANGE(60, 0x8, 0x41, 12500),
};
--
2.11.0
Re: [PATCH 2/2] ASoC: wm8994: Ensure the device is resumed in wm89xx_mic_detect functions
On Thu, Aug 27, 2020 at 07:33:57PM +0200, Sylwester Nawrocki wrote: > When the wm8958_mic_detect, wm8994_mic_detect functions get called from > the machine driver, e.g. from the card's late_probe() callback, the CODEC > device may be PM runtime suspended and any regmap writes have no effect. > Add PM runtime calls to these functions to ensure the device registers > are updated as expected. > This suppresses an error during boot > "wm8994-codec: ASoC: error at snd_soc_component_update_bits on wm8994-codec" > caused by the regmap access error due to the cache_only flag being set. > > Signed-off-by: Sylwester Nawrocki > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 1/2] ASoC: wm8994: Skip setting of the WM8994_MICBIAS register for WM1811
On Thu, Aug 27, 2020 at 07:33:56PM +0200, Sylwester Nawrocki wrote: > The WM8994_MICBIAS register is not available in the WM1811 CODEC so skip > initialization of that register for that device. > This suppresses an error during boot: > "wm8994-codec: ASoC: error at snd_soc_component_update_bits on wm8994-codec" > > Signed-off-by: Sylwester Nawrocki > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 1/3] mfd: madera: Simplify with dev_err_probe()
On Wed, Aug 26, 2020 at 04:49:33PM +0200, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH] ASoC: wm8962: Export DAC/ADC monomix switches
On Sun, Aug 16, 2020 at 03:23:34AM +0200, Sebastian Krzyszkowiak wrote: > This allows solutions like ALSA UCM to utilize hardware mono downmix > for cases where mono output to a single speaker is desired only in > specific situations (like on a mobile phone). > > Signed-off-by: Sebastian Krzyszkowiak > --- Acked-by: Charles Keepax Thanks, Charles
[RESEND PATCH v3 2/2] mfd: madera: Improve handling of regulator unbinding
The current unbinding process for Madera has some issues. The trouble
is runtime PM is disabled as the first step of the process, but
some of the drivers release IRQs causing regmap IRQ to issue a
runtime get which fails. To allow runtime PM to remain enabled during
mfd_remove_devices, the DCVDD regulator must remain available. In
the case of external DCVDD's this is simple, the regulator can simply
be disabled/put after the call to mfd_remove_devices. However, in
the case of an internally supplied DCVDD the regulator needs to be
released after the other MFD children depending on it.
Use the new MFD mfd_remove_devices_late functionality to split
the DCVDD regulator off from the other drivers.
Signed-off-by: Charles Keepax
---
drivers/mfd/madera-core.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index 4724c1a01a39f..8a8d733fdce5b 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -44,7 +44,10 @@ static const char * const madera_core_supplies[] = {
};
static const struct mfd_cell madera_ldo1_devs[] = {
- { .name = "madera-ldo1", },
+ {
+ .name = "madera-ldo1",
+ .level = MFD_DEP_LEVEL_HIGH,
+ },
};
static const char * const cs47l15_supplies[] = {
@@ -743,18 +746,22 @@ int madera_dev_exit(struct madera *madera)
/* Prevent any IRQs being serviced while we clean up */
disable_irq(madera->irq);
- /*
-* DCVDD could be supplied by a child node, we must disable it before
-* removing the children, and prevent PM runtime from turning it back on
-*/
- pm_runtime_disable(madera->dev);
+ pm_runtime_get_sync(madera->dev);
- clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
+ mfd_remove_devices(madera->dev);
+
+ pm_runtime_disable(madera->dev);
regulator_disable(madera->dcvdd);
regulator_put(madera->dcvdd);
- mfd_remove_devices(madera->dev);
+ mfd_remove_devices_late(madera->dev);
+
+ pm_runtime_set_suspended(madera->dev);
+ pm_runtime_put_noidle(madera->dev);
+
+ clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
+
madera_enable_hard_reset(madera);
regulator_bulk_disable(madera->num_core_supplies,
--
2.11.0
[RESEND PATCH v3 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children
Currently, the only way to remove MFD children is with a call to
mfd_remove_devices, which will remove all the children. Under
some circumstances it is useful to remove only a subset of the
child devices. For example if some additional clean up is required
between removal of certain child devices.
To accomplish this a level field is added to mfd_cell, the normal
mfd_remove_devices is modified to not remove devices that are set
to a higher level and a corresponding mfd_remove_devices_late
function is added to remove those children.
See further discussion at:
https://lore.kernel.org/lkml/20200616075834.GF2608702@dell/
Suggested-by: Lee Jones
Signed-off-by: Charles Keepax
---
drivers/mfd/mfd-core.c | 16 +++-
include/linux/mfd/core.h | 5 +
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index c51183209f82c..c3ef58a802bee 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -357,6 +357,7 @@ static int mfd_remove_devices_fn(struct device *dev, void
*data)
struct platform_device *pdev;
const struct mfd_cell *cell;
struct mfd_of_node_entry *of_entry, *tmp;
+ int *level = data;
if (dev->type != _dev_type)
return 0;
@@ -364,6 +365,9 @@ static int mfd_remove_devices_fn(struct device *dev, void
*data)
pdev = to_platform_device(dev);
cell = mfd_get_cell(pdev);
+ if (level && cell->level > *level)
+ return 0;
+
regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
cell->num_parent_supplies);
@@ -380,9 +384,19 @@ static int mfd_remove_devices_fn(struct device *dev, void
*data)
return 0;
}
+void mfd_remove_devices_late(struct device *parent)
+{
+ int level = MFD_DEP_LEVEL_HIGH;
+
+ device_for_each_child_reverse(parent, , mfd_remove_devices_fn);
+}
+EXPORT_SYMBOL(mfd_remove_devices_late);
+
void mfd_remove_devices(struct device *parent)
{
- device_for_each_child_reverse(parent, NULL, mfd_remove_devices_fn);
+ int level = MFD_DEP_LEVEL_NORMAL;
+
+ device_for_each_child_reverse(parent, , mfd_remove_devices_fn);
}
EXPORT_SYMBOL(mfd_remove_devices);
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 6d68f44a26a1c..4b35baa14d308 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -46,6 +46,9 @@
#define MFD_CELL_NAME(_name) \
MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, false, NULL)
+#define MFD_DEP_LEVEL_NORMAL 0
+#define MFD_DEP_LEVEL_HIGH 1
+
struct irq_domain;
struct property_entry;
@@ -63,6 +66,7 @@ struct mfd_cell_acpi_match {
struct mfd_cell {
const char *name;
int id;
+ int level;
int (*enable)(struct platform_device *dev);
int (*disable)(struct platform_device *dev);
@@ -150,6 +154,7 @@ static inline int mfd_add_hotplug_devices(struct device
*parent,
}
extern void mfd_remove_devices(struct device *parent);
+extern void mfd_remove_devices_late(struct device *parent);
extern int devm_mfd_add_devices(struct device *dev, int id,
const struct mfd_cell *cells, int n_devs,
--
2.11.0
Re: [PATCH v5 3/3] ALSA: compress: fix partial_drain completion state
On Mon, Jun 29, 2020 at 07:17:37PM +0530, Vinod Koul wrote:
> On partial_drain completion we should be in SNDRV_PCM_STATE_RUNNING
> state, so set that for partially draining streams in
> snd_compr_drain_notify() and use a flag for partially draining streams
>
> While at it, add locks for stream state change in
> snd_compr_drain_notify() as well.
>
> Fixes: f44f2a5417b2 ("ALSA: compress: fix drain calls blocking other compress
> functions (v6)")
> Reviewed-by: Srinivas Kandagatla
> Tested-by: Srinivas Kandagatla
> Signed-off-by: Vinod Koul
> ---
Worth noting I haven't actually tested the gapless, but keeps all
the compressed capture stuff happy.
Tested-by: Charles Keepax
Reviewed-by: Charles Keepax
Thanks,
Charles
Re: [PATCH v3 1/3] ALSA: compress: document the compress audio state machine
On Thu, Jun 25, 2020 at 09:16:49PM +0530, Vinod Koul wrote: > So we had some discussions of the stream states, so I thought it is a > good idea to document the state transitions, so add it documentation > > Signed-off-by: Vinod Koul > --- Reviewed-by: Charles Keepax Thanks, Charles
Re: [RESEND 06/10] regulator: wm8350-regulator: Repair odd formatting in documentation
On Thu, Jun 25, 2020 at 08:17:04PM +0100, Lee Jones wrote: > Kerneldoc expects function arguments to be in the format '@.*:'. If > this format is not followed the kerneldoc tooling/parsers/validators > get confused. > > Fixes the following W=1 warning(s): > > drivers/regulator/wm8350-regulator.c:1234: warning: Function parameter or > member 'wm8350' not described in 'wm8350_register_led' > drivers/regulator/wm8350-regulator.c:1234: warning: Function parameter or > member 'lednum' not described in 'wm8350_register_led' > drivers/regulator/wm8350-regulator.c:1234: warning: Function parameter or > member 'dcdc' not described in 'wm8350_register_led' > drivers/regulator/wm8350-regulator.c:1234: warning: Function parameter or > member 'isink' not described in 'wm8350_register_led' > drivers/regulator/wm8350-regulator.c:1234: warning: Function parameter or > member 'pdata' not described in 'wm8350_register_led' > > Cc: patc...@opensource.cirrus.com > Signed-off-by: Lee Jones > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH v3 3/3] ALSA: compress: fix partial_drain completion state
On Thu, Jun 25, 2020 at 09:16:51PM +0530, Vinod Koul wrote:
> On partial_drain completion we should be in SNDRV_PCM_STATE_RUNNING
> state, so set that for partially draining streams in
> snd_compr_drain_notify() and use a flag for partially draining streams
>
> While at it, add locks for stream state change in
> snd_compr_drain_notify() as well.
>
> Fixes: f44f2a5417b2 ("ALSA: compress: fix drain calls blocking other compress
> functions (v6)")
> Reviewed-by: Srinivas Kandagatla
> Tested-by: Srinivas Kandagatla
> Signed-off-by: Vinod Koul
> ---
> @@ -187,7 +189,15 @@ static inline void snd_compr_drain_notify(struct
> snd_compr_stream *stream)
> if (snd_BUG_ON(!stream))
> return;
>
> - stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> + mutex_lock(>device->lock);
> + /* for partial_drain case we are back to running state on success */
> + if (stream->partial_drain) {
> + stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> + stream->partial_drain = false; /* clear this flag as well */
> + } else {
> + stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> + }
> + mutex_unlock(>device->lock);
You have added locking here in snd_compr_drain_notify but
>
> wake_up(>runtime->sleep);
> }
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index e618580feac4..1c4b2cf450a0 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -803,6 +803,9 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
>
> retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP);
> if (!retval) {
> + /* clear flags and stop any drain wait */
> + stream->partial_drain = false;
> + stream->metadata_set = false;
> snd_compr_drain_notify(stream);
that can be called from snd_compr_stop here which is already
holding the lock resulting in deadlock.
Thanks,
Charles
Re: [PATCH 8/9] regulator: wm8400-regulator: Repair dodgy kerneldoc header formatting
On Fri, Jun 26, 2020 at 07:57:37AM +0100, Lee Jones wrote: > W=1 kernel builds report a lack of descriptions for various > function arguments. In reality they are documented, but the > formatting was not as expected '@.*:'. Instead, some weird > arg identifiers were used. > > This change fixes the following warnings: > > drivers/regulator/wm8400-regulator.c:243: warning: Function parameter or > member 'dev' not described in 'wm8400_register_regulator' > drivers/regulator/wm8400-regulator.c:243: warning: Function parameter or > member 'reg' not described in 'wm8400_register_regulator' > drivers/regulator/wm8400-regulator.c:243: warning: Function parameter or > member 'initdata' not described in 'wm8400_register_regulator' > > Cc: patc...@opensource.cirrus.com > Signed-off-by: Lee Jones > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 10/10] mfd: wm831x-core: Supply description wm831x_reg_{un}lock args
On Wed, Jun 24, 2020 at 04:07:04PM +0100, Lee Jones wrote: > Kerneldoc syntax is used, but not complete. Descriptions required. > > Prevents warnings like: > > drivers/mfd/wm831x-core.c:119: warning: Function parameter or member > 'wm831x' not described in 'wm831x_reg_lock' > drivers/mfd/wm831x-core.c:145: warning: Function parameter or member > 'wm831x' not described in 'wm831x_reg_unlock' > > Cc: > Cc: Mark Brown > Cc: patc...@opensource.cirrus.com > Signed-off-by: Lee Jones > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH 09/10] mfd: wm8400-core: Supply description for wm8400_reset_codec_reg_cache's arg
On Wed, Jun 24, 2020 at 04:07:03PM +0100, Lee Jones wrote: > Kerneldoc syntax is used, but not complete. Descriptions required. > > Prevents warnings like: > > drivers/mfd/wm8400-core.c:113: warning: Function parameter or member > 'wm8400' not described in 'wm8400_reset_codec_reg_cache' > > Cc: > Cc: Mark Brown > Cc: patc...@opensource.cirrus.com > Signed-off-by: Lee Jones > --- Acked-by: Charles Keepax Thanks, Charles
Re: [PATCH v2 1/3] ALSA: compress: document the compress audio state machine
On Mon, Jun 22, 2020 at 08:28:48AM -0500, Pierre-Louis Bossart wrote: > On 6/22/20 1:58 AM, Vinod Koul wrote: > >++--+ > >+| | > >+| OPEN | > >+| | > >++--+ > >+ | > >+ | > >+ | compr_set_params() > >+ | > >+ v > >+ compr_free() +--+ > >+ +-| | > >+ | | SETUP | > >+ | +>| > >|<-+ > >+ | | compr_drain_notify()+--+ > > | > >+ | | | > > | > >+ | | | > > | > >+ | | | compr_write() > > | > >+ | | | > > | > >+ | | v > > | > >+ | | +--+ > > | > >+ | | | | > > | > >+ | | | PREPARE | > > | > >+ | | | | > > | > >+ | | +--+ > > | > >+ | | | > > | > >+ | | | > > | > >+ | | | compr_start() > > | > >+ | | | > > | > >+ | | v > > | > >+ | +--++--+ compr_pause() > >+--+ | > >+ | | |compr_drain() | |>| > >| | > >+ | | DRAIN |<---| RUNNING | | > >PAUSE | | > >+ | | || |<| > >| | > >+ | +--++--+ compr_resume() > >+--+ | > >+ | | | | > > | > >+ | | | | > > | > >+ | | | | > > | > >+ | | | | compr_stop() > > | > >+ | | | > >++ > >+ | | +--+| > >+ | | | || > >+ +---+-->| |<---+ > >+ compr_free() | FREE | compr_free() > >+ | | > >+ +--+ > a) can you clarify if we can go from running to free directly? is > this really a legit transition? There's already the option of doing > a stop and a a drain. > This is allowed in the current code, the kernel sends the stop internally in this case, so it kinda does go through the setup state just not from the users view point. I am not sure I have a good handle on if that makes sense or not. > c) no way to stop a paused stream? Currently the code does allow this and it certainly makes sense so should probably be added. Thanks, Charles
[PATCH] regmap: Fix memory leak from regmap_register_patch
When a register patch is registered the reg_sequence is copied but the
memory allocated is never freed. Add a kfree in regmap_exit to clean it
up.
Fixes: 22f0d90a3482 ("regmap: Support register patch sets")
Signed-off-by: Charles Keepax
---
drivers/base/regmap/regmap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 94e2e2d1824bb..414422e1a2e6e 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1349,6 +1349,7 @@ void regmap_exit(struct regmap *map)
if (map->hwlock)
hwspin_lock_free(map->hwlock);
kfree_const(map->name);
+ kfree(map->patch);
kfree(map);
}
EXPORT_SYMBOL_GPL(regmap_exit);
--
2.11.0
[PATCH v3 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children
Currently, the only way to remove MFD children is with a call to
mfd_remove_devices, which will remove all the children. Under
some circumstances it is useful to remove only a subset of the
child devices. For example if some additional clean up is required
between removal of certain child devices.
To accomplish this a level field is added to mfd_cell, the normal
mfd_remove_devices is modified to not remove devices that are set
to a higher level and a corresponding mfd_remove_devices_late
function is added to remove those children.
See further discussion at:
https://lore.kernel.org/lkml/20200616075834.GF2608702@dell/
Suggested-by: Lee Jones
Signed-off-by: Charles Keepax
---
Changes since v2:
- Removed old tag system and now just have 2 levels, one removed
by new mfd_remove_devices_late
Thanks,
Charles
drivers/mfd/mfd-core.c | 16 +++-
include/linux/mfd/core.h | 5 +
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index f5a73af60dd40..d1602a87db29b 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -287,6 +287,7 @@ static int mfd_remove_devices_fn(struct device *dev, void
*data)
{
struct platform_device *pdev;
const struct mfd_cell *cell;
+ int *level = data;
if (dev->type != _dev_type)
return 0;
@@ -294,6 +295,9 @@ static int mfd_remove_devices_fn(struct device *dev, void
*data)
pdev = to_platform_device(dev);
cell = mfd_get_cell(pdev);
+ if (level && cell->level > *level)
+ return 0;
+
regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
cell->num_parent_supplies);
@@ -301,9 +305,19 @@ static int mfd_remove_devices_fn(struct device *dev, void
*data)
return 0;
}
+void mfd_remove_devices_late(struct device *parent)
+{
+ int level = MFD_DEP_LEVEL_HIGH;
+
+ device_for_each_child_reverse(parent, , mfd_remove_devices_fn);
+}
+EXPORT_SYMBOL(mfd_remove_devices_late);
+
void mfd_remove_devices(struct device *parent)
{
- device_for_each_child_reverse(parent, NULL, mfd_remove_devices_fn);
+ int level = MFD_DEP_LEVEL_NORMAL;
+
+ device_for_each_child_reverse(parent, , mfd_remove_devices_fn);
}
EXPORT_SYMBOL(mfd_remove_devices);
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 7e5ac3c00891a..1ca2048eba22b 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -41,6 +41,9 @@
#define MFD_CELL_NAME(_name) \
MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL) \
+#define MFD_DEP_LEVEL_NORMAL 0
+#define MFD_DEP_LEVEL_HIGH 1
+
struct irq_domain;
struct property_entry;
@@ -58,6 +61,7 @@ struct mfd_cell_acpi_match {
struct mfd_cell {
const char *name;
int id;
+ int level;
int (*enable)(struct platform_device *dev);
int (*disable)(struct platform_device *dev);
@@ -135,6 +139,7 @@ static inline int mfd_add_hotplug_devices(struct device
*parent,
}
extern void mfd_remove_devices(struct device *parent);
+extern void mfd_remove_devices_late(struct device *parent);
extern int devm_mfd_add_devices(struct device *dev, int id,
const struct mfd_cell *cells, int n_devs,
--
2.11.0
[PATCH v3 2/2] mfd: madera: Improve handling of regulator unbinding
The current unbinding process for Madera has some issues. The trouble
is runtime PM is disabled as the first step of the process, but
some of the drivers release IRQs causing regmap IRQ to issue a
runtime get which fails. To allow runtime PM to remain enabled during
mfd_remove_devices, the DCVDD regulator must remain available. In
the case of external DCVDD's this is simple, the regulator can simply
be disabled/put after the call to mfd_remove_devices. However, in
the case of an internally supplied DCVDD the regulator needs to be
released after the other MFD children depending on it.
Use the new MFD mfd_remove_devices_late functionality to split
the DCVDD regulator off from the other drivers.
Signed-off-by: Charles Keepax
---
Changes since v2:
- Moved to new mfd_remove_devices_late
Thanks,
Charles
drivers/mfd/madera-core.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index 4724c1a01a39f..8a8d733fdce5b 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -44,7 +44,10 @@ static const char * const madera_core_supplies[] = {
};
static const struct mfd_cell madera_ldo1_devs[] = {
- { .name = "madera-ldo1", },
+ {
+ .name = "madera-ldo1",
+ .level = MFD_DEP_LEVEL_HIGH,
+ },
};
static const char * const cs47l15_supplies[] = {
@@ -743,18 +746,22 @@ int madera_dev_exit(struct madera *madera)
/* Prevent any IRQs being serviced while we clean up */
disable_irq(madera->irq);
- /*
-* DCVDD could be supplied by a child node, we must disable it before
-* removing the children, and prevent PM runtime from turning it back on
-*/
- pm_runtime_disable(madera->dev);
+ pm_runtime_get_sync(madera->dev);
- clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
+ mfd_remove_devices(madera->dev);
+
+ pm_runtime_disable(madera->dev);
regulator_disable(madera->dcvdd);
regulator_put(madera->dcvdd);
- mfd_remove_devices(madera->dev);
+ mfd_remove_devices_late(madera->dev);
+
+ pm_runtime_set_suspended(madera->dev);
+ pm_runtime_put_noidle(madera->dev);
+
+ clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
+
madera_enable_hard_reset(madera);
regulator_bulk_disable(madera->num_core_supplies,
--
2.11.0
Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children
On Tue, Jun 16, 2020 at 02:22:59PM +0100, Lee Jones wrote: > On Tue, 16 Jun 2020, Charles Keepax wrote: > > On Tue, Jun 16, 2020 at 10:15:45AM +0100, Lee Jones wrote: > > > On Tue, 16 Jun 2020, Charles Keepax wrote: > > > > On Tue, Jun 16, 2020 at 08:58:34AM +0100, Lee Jones wrote: > > > > > On Mon, 15 Jun 2020, Charles Keepax wrote: > > > > Does this match how you would expect this to be used? > > > > > > No, not at all. > > > > > > > I do have some concerns. The code can't use mfd_get_cell since it > > > > returns a const pointer, although the pointer in platform_device > > > > isn't const so we access that directly, could update mfd_get_cell? We > > > > also don't have access to mfd_dev_type outside of the mfd core so > > > > its hard to check we are actually setting the mfd_cell of actual > > > > MFD children, I guess just checking for mfd_cell being not NULL is > > > > good enough? > > > > > > Hmmm... looks like I missed the fact that you needed additional > > > processing between the removal of each batch of devices. My > > > implementation simply handles the order in which devices are removed > > > (a bit like initcall()s). > > > > > > How about the inclusion of mfd_remove_devices_late(), whereby > > > mfd_remove_devices() will refuse to remove devices tagged with > > > MFD_DEP_LEVEL_HIGH. > > > > > > > Yeah this should work fine for my use-case. > > > > > Not sure why, but I really dislike the idea of device removal by > > > arbitrary value/tag, as I see it spawning all sorts of weird and > > > wonderful implications/hacks/abuse. > > > > > > > Its definitely a spectrum with flexibility covering more > > use-cases but also definitely opening things up to more abuse. If > > you are more comfortable with this approach that is fine with me. > > > > Would you like me to have a crack at coding it up this way? Or > > did you want to do a patch? > > Either/or. I don't want to steal your thunder, but I'm happy to draft > if you are. > Been having a poke this afternoon as I had some spare time, so will wing that up and you can take over if I am too far off the mark :-) Thanks, Charles
Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children
On Tue, Jun 16, 2020 at 10:15:45AM +0100, Lee Jones wrote: > On Tue, 16 Jun 2020, Charles Keepax wrote: > > On Tue, Jun 16, 2020 at 08:58:34AM +0100, Lee Jones wrote: > > > On Mon, 15 Jun 2020, Charles Keepax wrote: > > Does this match how you would expect this to be used? > > No, not at all. > > > I do have some concerns. The code can't use mfd_get_cell since it > > returns a const pointer, although the pointer in platform_device > > isn't const so we access that directly, could update mfd_get_cell? We > > also don't have access to mfd_dev_type outside of the mfd core so > > its hard to check we are actually setting the mfd_cell of actual > > MFD children, I guess just checking for mfd_cell being not NULL is > > good enough? > > Hmmm... looks like I missed the fact that you needed additional > processing between the removal of each batch of devices. My > implementation simply handles the order in which devices are removed > (a bit like initcall()s). > > How about the inclusion of mfd_remove_devices_late(), whereby > mfd_remove_devices() will refuse to remove devices tagged with > MFD_DEP_LEVEL_HIGH. > Yeah this should work fine for my use-case. > Not sure why, but I really dislike the idea of device removal by > arbitrary value/tag, as I see it spawning all sorts of weird and > wonderful implications/hacks/abuse. > Its definitely a spectrum with flexibility covering more use-cases but also definitely opening things up to more abuse. If you are more comfortable with this approach that is fine with me. Would you like me to have a crack at coding it up this way? Or did you want to do a patch? Thanks, Charles
Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children
On Tue, Jun 16, 2020 at 08:58:34AM +0100, Lee Jones wrote:
> On Mon, 15 Jun 2020, Charles Keepax wrote:
> > Happy to discuss other approaches as well, but this one was quite
> >
> >
> >│··
> > appealing as it was very simple but affords quite a lot of flexibility.
> >
> >
> >│··
>
> What about this?
>
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index f5a73af60dd40..a06e0332e1e31 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -283,7 +283,7 @@ int mfd_add_devices(struct device *parent, int id,
> }
> EXPORT_SYMBOL(mfd_add_devices);
>
> -static int mfd_remove_devices_fn(struct device *dev, void *data)
> +static int mfd_remove_devices_fn(struct device *dev, void *level)
> {
> struct platform_device *pdev;
> const struct mfd_cell *cell;
> @@ -294,6 +294,9 @@ static int mfd_remove_devices_fn(struct device *dev, void
> *data)
> pdev = to_platform_device(dev);
> cell = mfd_get_cell(pdev);
>
> + if (cell->level && (!level || cell->level != *level))
> + return 0;
> +
> regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
>cell->num_parent_supplies);
>
> @@ -303,7 +306,11 @@ static int mfd_remove_devices_fn(struct device *dev,
> void *data)
>
> void mfd_remove_devices(struct device *parent)
> {
> + int level = MFD_DEP_LEVEL_HIGH;
> +
> device_for_each_child_reverse(parent, NULL, mfd_remove_devices_fn);
> + device_for_each_child_reverse(parent, , mfd_remove_devices_fn);
> }
> EXPORT_SYMBOL(mfd_remove_devices);
>
> No need for special calls from the parent driver in this case.
>
> Just a requirement to set the cell's dependency level.
>
Apologies if I am missing something here, but this looks like a
pretty challenging interface from the drivers side. Rather than
just statically setting tag in the mfd_cells and separate calls
to mfd_remove_devices_by_tag, such as:
mfd_remove_devices_by_tag(madera->dev, MADERA_OPTIONAL_DRIVER);
pm_runtime_disable(madera->dev);
regulator_disable(madera->dcvdd);
regulator_put(madera->dcvdd);
mfd_remove_devices(madera->dev);
You need to statically set the level but then also iterate through
the children and update the cell level on each subsequent remove,
in my case:
static int arizona_set_mfd_level(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
if (pdev->mfd_cell)
pdev->mfd_cell->level = MFD_DEP_LEVEL_HIGH;
}
...
mfd_remove_devices(madera->dev);
device_for_each_child(madera->dev, NULL, arizona_set_mfd_level);
pm_runtime_disable(madera->dev);
regulator_disable(madera->dcvdd);
regulator_put(madera->dcvdd);
mfd_remove_devices(madera->dev);
Does this match how you would expect this to be used?
I do have some concerns. The code can't use mfd_get_cell since it
returns a const pointer, although the pointer in platform_device
isn't const so we access that directly, could update mfd_get_cell? We
also don't have access to mfd_dev_type outside of the mfd core so
its hard to check we are actually setting the mfd_cell of actual
MFD children, I guess just checking for mfd_cell being not NULL is
good enough?
Thanks,
Charles
