Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <743f08ed-1a26-8cc0-4f7a-32e41f5040f1@redhat.com>
Date:   Mon, 7 Mar 2022 18:34:19 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH 2/2] mfd: arizona-spi: Add Android board ACPI table
 handling
Content-Language: en-US
To:     Lee Jones <lee.jones@linaro.org>
Cc:     patches@opensource.cirrus.com, linux-kernel@vger.kernel.org
References: <20220223134222.730886-1-hdegoede@redhat.com>
 <20220223134222.730886-2-hdegoede@redhat.com> <YiYb+Ea7aKO99gXL@google.com>
 <ac21f173-2cba-0ddc-67db-27e06950c2d4@redhat.com>
 <YiYicoaSc7KYTbvj@google.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <YiYicoaSc7KYTbvj@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 3/7/22 16:19, Lee Jones wrote:
> On Mon, 07 Mar 2022, Hans de Goede wrote:
> 
>> Hi,
>>
>> On 3/7/22 15:51, Lee Jones wrote:
>>> On Wed, 23 Feb 2022, Hans de Goede wrote:
>>>
>>>> x86/ACPI boards with an arizona WM5102 codec ship with either Windows or
>>>> Android as factory installed OS.
>>>>
>>>> The ACPI fwnode for the codec on Android boards misses 2 things compared
>>>> to the Windows boards (this is hardcoded in the Android board kernels):
>>>>
>>>> 1. There is no CLKE ACPI method to enabe the 32 KHz clock the codec needs
>>>>    for jack-detection.
>>>>
>>>> 2. The GPIOs used by the codec are not listed in the fwnode for the codec.
>>>>
>>>> The ACPI tables on x86/ACPI boards shipped with Android being incomplete
>>>> happens a lot. The special drivers/platform/x86/x86-android-tablets.c
>>>> module contains DMI based per model handling to compensate for this.
>>>>
>>>> This module will enable the 32KHz clock through the pinctrl framework
>>>> to fix 1. and it will also register a gpio-lookup table for all GPIOs
>>>> needed by the codec + machine driver, including the GPIOs coming from
>>>> the codec itself.
>>>>
>>>> Add an arizona_spi_acpi_android_probe() function which waits for the
>>>> x86-android-tablets to have set things up before continue with probing
>>>> the arizona WM5102 codec.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/mfd/arizona-spi.c | 34 +++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 33 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c
>>>> index 238355542ab1..2c686e71db21 100644
>>>> --- a/drivers/mfd/arizona-spi.c
>>>> +++ b/drivers/mfd/arizona-spi.c
>>>> @@ -81,6 +81,29 @@ static int arizona_spi_acpi_windows_probe(struct arizona *arizona)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +/* For ACPI tables from boards which ship with Android as factory OS */
>>>> +static int arizona_spi_acpi_android_probe(struct arizona *arizona)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	/*
>>>> +	 * Get the reset GPIO, treating -ENOENT as -EPROBE_DEFER to wait for
>>>> +	 * the x86-android-tablets module to register the board specific GPIO
>>>> +	 * lookup table.
>>>> +	 */
>>>> +	arizona->pdata.reset = devm_gpiod_get(arizona->dev, "reset", GPIOD_OUT_LOW);
>>>> +	if (IS_ERR(arizona->pdata.reset)) {
>>>> +		ret = PTR_ERR(arizona->pdata.reset);
>>>> +		if (ret == -ENOENT) {
>>>> +			dev_info_once(arizona->dev, "Deferring probe till GPIO lookup is registered\n");
>>>
>>> Nit: How many chars is this?
>>
>> 105.
>>
>>> I thought we were drawing the line at 100 these days?
>>
>> We have an exception for log lines, since we don't want to break them
>> up because that makes grepping for them impossible.
>>
>>> Does this patch pass checkpatch.pl?
>>
>> Yes because of the exception for log lines:
>>
>> [hans@x1 linux]$ scripts/checkpatch.pl 0001-mfd-arizona-spi-Add-Android-board-ACPI-table-handlin.patch 
>> total: 0 errors, 0 warnings, 54 lines checked
>>
>> 0001-mfd-arizona-spi-Add-Android-board-ACPI-table-handlin.patch has no obvious style problems and is ready for submission.
> 
> What do you mean by long lines?

Not long lines, log lines, as in lines logging a message,
sorry if that was unclear.

> I'm aware of the exception for long strings.
> 
> Not sure why anyone would grep for "dev_info_once(arizona->dev, ".
> 
> I would break this after the first comma.

Ok, I'll send a v2 with a break after the first comma.

Regards,

Hans