Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <4374f40f-b1ab-a009-8d7f-1f4b6f6355ee@intel.com>
Date:   Tue, 10 Oct 2023 13:43:58 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v6 08/12] ASoC: Intel: avs: Move snd_hdac_i915_init to
 before probe_work.
Content-Language: en-US
To:     Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        <alsa-devel@alsa-project.org>
CC:     Maarten Lankhorst <dev@lankhorst.se>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Liam Girdwood <liam.r.girdwood@linux.intel.com>,
        Peter Ujfalusi <peter.ujfalusi@linux.intel.com>,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        Kai Vehmanen <kai.vehmanen@linux.intel.com>,
        Mark Brown <broonie@kernel.org>,
        "Daniel Baluta" <daniel.baluta@nxp.com>,
        <linux-kernel@vger.kernel.org>,
        <sound-open-firmware@alsa-project.org>,
        =?UTF-8?Q?Amadeusz_S=c5=82awi=c5=84ski?= 
        <amadeuszx.slawinski@linux.intel.com>
References: <20231004145540.32321-1-maarten.lankhorst@linux.intel.com>
 <20231004145540.32321-9-maarten.lankhorst@linux.intel.com>
From:   Cezary Rojewski <cezary.rojewski@intel.com>
In-Reply-To: <20231004145540.32321-9-maarten.lankhorst@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RHlOd0NzU3M3ZGIwZDF6ZjhxY0ZPVjRPb3ZSWUFLUVc0RHczVmc0NzhvU3Vq?=
 =?utf-8?B?V2hwOVNwRVY1T2p0K2w5SGU3bXFVbEJRN2dBUm9qYTA3dEFFSEs4S29zUWMr?=
 =?utf-8?B?QlkwVEIxQ2VOc3oyYnBCdElwNzVkaUcySEluUEF1U2xPVWVOTzVwaHJzV1A0?=
 =?utf-8?B?c2M3R1NzUGplYUViN1NHYklha29hZVpOOVh6NzFvNjVPVVhNRUR1ZWprTjdw?=
 =?utf-8?B?cVY1bXJLUXFRVU11K3VqL3ljNjZncFFxWWJwM1JqWVk3c2pGMDZIclQwT2hp?=
 =?utf-8?B?eTJneGxjcXBKWm03S2NOZWhaY1Vha3R2U29rei82Yk9wY0hlU29IN0pEQnQy?=
 =?utf-8?B?OU5PbWdtcTJqeEpPWHI1dTRwcDVyYjgvZGRIYjlqa1l2NEJBcVdHd2NsN3Y5?=
 =?utf-8?B?UGpOWGVSZ0EzSnhtdkpuYndBRU96Nm5LVGJ1LzdIVnNCdVgwcWFVVFNjR094?=
 =?utf-8?B?ZGpTRklmdGpBQzNsR1gzL25KaDZyblB6bzBTd1lBbzZodDVka1RrdVJQZ2hK?=
 =?utf-8?B?M0x6eko2UW9kVWZPVFJBeDVNV1JqaDlnSEZSRW43aForQUY0TStyeWhYbUU4?=
 =?utf-8?B?bFFFNkYwSTJuZTVoYklvM3grL3lUdDVUWnZ4bCtWMjFkVHgwVGwyNUNldU9L?=
 =?utf-8?B?VU82d1IzSFlIYm4wdEN6MGdXa29OcXdIUXRtdmh6V1R6S0xPS243Zlo2RzBI?=
 =?utf-8?B?ZWE5ckc2QlhrQmh0TjNJVVdPUE01cXVobnY0d1M4bFgwVEIzS3JUcnk1c3JE?=
 =?utf-8?B?T2dYK1lUV0ZXeDRSV01wNlRhTENwY3c3d2UxNnJLaTU4bWtXRmorcTdrQ3Rx?=
 =?utf-8?B?ZER5WFV1aFY1S3E2cFdLU2hKa0VZQytTeXB6WnYvek1iamM0SUhxQmRPZDNa?=
 =?utf-8?B?NERvSG1rK1EzNkI5ZlZvTFE3RURaaituU25DRGJMeDVNUHFkTjVBZUVxRlhH?=
 =?utf-8?B?N29Ib3kvWmlTY3NKanFrOTY4Vk1pZ3BBNkh5Nkh0Um9LZnpyRmV0S3NyTytT?=
 =?utf-8?B?NFU4dTlHUGZSaHBKVElsUkdsaVFuZm9nazlYSHlLeFpQa0ZWa2IySVZrNVpV?=
 =?utf-8?B?d3dTOWNEbDZ5RHRWaDdlWUFxbDlnY0ZEdlJXZ3ZxOUliZmhjbzkzU3o5SEJq?=
 =?utf-8?B?RHdydXM0NHZ0NklJWlJzSGhtYzRJZG5wRTAxaG9VK0RoWVlUT0FRczUxakpt?=
 =?utf-8?B?WnlraFBORnByRHBIYThrZlRKcTUrbDFNckxaYWN4citYdi84L3lnNi9OaFI2?=
 =?utf-8?B?NGt2VGk0KzV6bkg3TDBwNUtHSGQvMFNkWVBHRzNtaHZmekwyM1liZk1oT0J0?=
 =?utf-8?B?T0daNitHekZldEw0Z1o3eVpzNHByaUVQajNaSmhCbWgvaVUvU2lhYUs5TWli?=
 =?utf-8?B?LzIrOVN3bFpnSi9VNS9NZ0lJUXh2TWV4bFZJVHFOcEU5WHZOTzFLcHM3WWNo?=
 =?utf-8?B?WVlpUkR4b3IrT3U1WkFoKzVwTlRTMGNqb2N3Rm9OUklMcW8zWUt5UFNWT3dy?=
 =?utf-8?B?QjF2ajBjYjFLclA3WnUwQmovQllZTU1WbU8yVHJ1d0R3Tk04dW8xWXU1dXZ4?=
 =?utf-8?B?SkFwVmpQeHFEekdBeTR4aXkvZkxEK0JzUlJGSkNSN2JSR1FBUnBaN2tQV0w3?=
 =?utf-8?B?U1JzMUtwRkNSVE9LK3lSalVZMEYzZXErbEY5L01NWnpKVHFJcUV6dVJBa01Q?=
 =?utf-8?B?NFBNdE93MkZHd29XR3pDMDh1QjRDeC8rdWxCQ0xPaGxpVUhCL2d0UU4zQnJq?=
 =?utf-8?B?UkIvREtiNE5MU0hvQ2JLUHVZcHg0WkZ4eHQ4TXVVYnVyUzRlUmtKYjh1dm5k?=
 =?utf-8?B?OE9vTlI3NGJQcnBLeWtsdUU5MEZEdDdTRlQyZEpOWUpsZ0dHSjYvdHlvVDE3?=
 =?utf-8?B?QjVWRUF3UHI5d0xQU2dtNUhEUzdyYmdIUkRhbWViV3Y0Ulk2TEZYWWtocmIw?=
 =?utf-8?B?R2FUbDhISHdaQnNzNEZpalFMLzQ3ZXZpWmE4NnhiYzJhV1Eva0dvSDdlZVlu?=
 =?utf-8?B?WHBIbHI4aE80VnJ0a09STWppdnN0OExYamtBSWphT1hGVVJGVVUrc2ZYR3JM?=
 =?utf-8?B?YWRKbjIwQjFTWUF1WTBaSWFVdDZZRnlzZVdRR3ZXL2NHbjJ4dWNtY0hFbXhz?=
 =?utf-8?B?RzhoRHFRdi9HYVMxblhmVUJEbDkzZG1udFBISzBUNVFFdjBmTnNFU1BEOWxu?=
 =?utf-8?B?aGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 36e9edb2-98a5-4f17-d96d-08dbc9863558
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB6375.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Oct 2023 11:44:06.2971
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: MBSqurcSLTLKsqa82YADYc/NOrCjMZyHpFaEpm/bbM8dlkDfVKCGRqArj/m+r2wSFwoYnM5aqZeIpv8KhP/Mo6pD8eeDaSPTCbNL3agsrDA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7675
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.6 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Tue, 10 Oct 2023 04:44:33 -0700 (PDT)

On 2023-10-04 4:55 PM, Maarten Lankhorst wrote:
> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
> the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue
> can be destroyed, but I don't have the means to test this.
> 
> Removing the workqueue would simplify init even further, but is left
> as exercise for the reviewer.

Nitpick: Half of the commit message is redundant. From the 
reader/maintainer point of view, presented opinions and suggestions are 
fine as a part of cover-letter but not in the long run from git log level.

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Nitpick: commit title is not a sentence, should not end with full stop.

> ---
>   sound/soc/intel/avs/core.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
> index bbb40339c75f..8a2063958248 100644
> --- a/sound/soc/intel/avs/core.c
> +++ b/sound/soc/intel/avs/core.c
> @@ -191,10 +191,6 @@ static void avs_hda_probe_work(struct work_struct *work)
>   
>   	pm_runtime_set_active(bus->dev); /* clear runtime_error flag */
>   
> -	ret = snd_hdac_i915_init(bus, true);
> -	if (ret < 0)
> -		dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
> -
>   	snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
>   	avs_hdac_bus_init_chip(bus, true);
>   	avs_hdac_bus_probe_codecs(bus);
> @@ -465,10 +461,19 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>   	pci_set_drvdata(pci, bus);
>   	device_disable_async_suspend(dev);
>   
> +	ret = snd_hdac_i915_init(bus, false);
> +	if (ret == -EPROBE_DEFER)
> +		goto err_i915_init;
> +	else if (ret < 0)

The 'else' part seems redundant. s/else if/else/.

As I'm aware that this is v6 already, my team can also clean this up 
later with separate series to not delay the merge any longer.

> +		dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
> +
>   	schedule_work(&adev->probe_work);
>   
>   	return 0;
>   
> +err_i915_init:
> +	pci_clear_master(pci);
> +	pci_set_drvdata(pci, NULL);
>   err_acquire_irq:
>   	snd_hdac_bus_free_stream_pages(bus);
>   	snd_hdac_ext_stream_free_all(bus);