NAK: [SRU][J/allwinner-5.17][PATCH 0/4] Fix more Allwinner D1 drivers

[Tim Gardner]

On 7/8/22 08:07, Emil Renner Berthing wrote:
> On Fri, 8 Jul 2022 at 15:59, Tim Gardner <tim.gardner at canonical.com> wrote:
>>
>> On 7/8/22 07:41, Emil Renner Berthing wrote:
>>> On Fri, 8 Jul 2022 at 14:40, Tim Gardner <tim.gardner at canonical.com> wrote:
>>>>
>>>> On 7/8/22 04:50, Emil Renner Berthing wrote:
>>>>> [Impact]
>>>>>
>>>>>     * The drivers for the display engine, crypto acceleration and USB
>>>>>       don't probe because of missing dependencies.
>>>>>
>>>>> [Test Plan]
>>>>>
>>>>>     * Boot the kernel in linux-image-5.17.0-1002-allwinner and see
>>>>>       /sys/kerne/debug/devices_deferred
>>>>>
>>>>> [Where problems could occur]
>>>>>
>>>>>     * Fixing this may introduce now bugs if the now probing drivers
>>>>>       turn out to be buggy.
>>>>>
>>>>> Andre Przywara (1):
>>>>>      phy: sun4i-usb: Rework HCI PHY (aka "pmu_unk1") handling
>>>>>
>>>>> Emil Renner Berthing (1):
>>>>>      UBUNTU: [Config] Enable additional Allwinner D1 options
>>>>>
>>>>> Samuel Holland (2):
>>>>>      UBUNTU: SAUCE: phy: sun4i-usb: Add D1 variant
>>>>>      UBUNTU: SAUCE: PM / devfreq: Add dummy R329/D1 MBUS driver
>>>>>
>>>>>     debian.allwinner/config/config.common.ubuntu |  5 ++-
>>>>>     drivers/devfreq/Kconfig                      |  6 +++
>>>>>     drivers/devfreq/Makefile                     |  1 +
>>>>>     drivers/devfreq/sun50i-r329-mbus.c           | 27 +++++++++++++
>>>>>     drivers/phy/allwinner/phy-sun4i-usb.c        | 40 +++++++++++---------
>>>>>     5 files changed, 60 insertions(+), 19 deletions(-)
>>>>>     create mode 100644 drivers/devfreq/sun50i-r329-mbus.c
>>>>>
>>>>
>>>> When backporting, don't forget the description of the steps you took. In
>>>> this case it looks like it was simple context adjustments in patch 1/1.
>>>
>>> I'm sorry I don't know what you mean by this. Do you mean describe the
>>> steps I took produce the error? Eg. install the allwinner kernel and
>>> look at /sys/kernel/debug/devices_deferred?
>>>
>>> Context adjustments? Patch 1/1 is a cherry-pick from mainline and it
>>> applied cleanly.
>>>
>>
>> Patch 1/1 says '(backported from commit
>> 1743dea7f06b939f67ba258bab993fa5ff6e43fb)'. This implies you had to futz
>> with the patch in order to get it to apply. If it was a clean cherry
>> pick, then it should say 'cherry picked from commit
>> 1743dea7f06b939f67ba258bab993fa5ff6e43fb'.
>>
>> The description of the steps you took to apply the backport typically
>> follow on the next line, e.g.,
>>
>> [emil - applied some grease to the muffler bearings]
> 
> Ah! Got it, thanks. Yeah, patch 1/1 should just have been
> "cherry-picked from" then.
> 
>>>> Where are the annotation updates for the config changes in patch 4/4 ?
>>>
>>> I ran cranky updateconfigs and it didn't complain, so I guess these
>>> options weren't annotated. Should I add annotations for all of them or
>>> just the option added in patch 3/4?
>>>
>>
>> Do you have 'FORMAT: 3' with "do_enforce_all=true" ?
> 
> Not in this tree unfortunately, but I'll be sure to add that when
> creating the allwinner and starfive trees based on 5.19.
> 
>> The annotations are there to remind us _why_ a config option was
>> changed. At the very least you'll want an annotation that enforces the
>> setting along with a note referencing the LP bug. Every config should
>> have an annotation, either in the master annotation file or in the
>> derivative annotation file.
> 
> Right, that's of course the goal. I'll add annotations for the options
> changed in this series, but do you want me to add annotations for
> every other un-annotated option too?
> 

For the purposes of this patch, adding annotations for the affected 
config options is sufficient. When you transition to 'FORMAT: 3' and 
do_enforce_all you'll have to have an annotation for every config option.

rtg

> /Emil
> 
>>> /Emil
>>>
>>>
>>>> rtg
>>>>
>>>> --
>>>> -----------
>>>> Tim Gardner
>>>> Canonical, Inc
>>
>>
>> --
>> -----------
>> Tim Gardner
>> Canonical, Inc


-- 
-----------
Tim Gardner
Canonical, Inc