Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 8 Mar 2022 08:15:58 +0000
From:   Lee Jones <lee.jones@linaro.org>
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     patches@opensource.cirrus.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] mfd: arizona-spi: Add Android board ACPI table
 handling
Message-ID: <YicQvl2VKe4pxLuD@google.com>
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>
 <743f08ed-1a26-8cc0-4f7a-32e41f5040f1@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <743f08ed-1a26-8cc0-4f7a-32e41f5040f1@redhat.com>
Precedence: bulk

On Mon, 07 Mar 2022, Hans de Goede wrote:

> 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.

My fault, I misread.

> > 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.

Thank you.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog