Re: [PATCH 1/2] swap led_brightness from enum to typedef

[Randy Dunlap]

Amy,

Please see comments below.

On 7/16/21 2:07 PM, Amy Parker wrote:
> On Thu, Jul 15, 2021 at 8:11 PM Amy Parker
> <apark0006@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> Ah - I see there was an issue with header files not being properly updated.
>>
>> Check back for another patch resolving this.
>>
>>
>> On Thu, Jul 15, 2021 at 7:15 PM kernel test robot <lkp@xxxxxxxxx> wrote:
>>>
>>> Hi Amy,
>>>
>>> Thank you for the patch! Perhaps something to improve:
>>>
>>> [auto build test WARNING on linus/master]
>>> [also build test WARNING on v5.14-rc1 next-20210715]
>>> [cannot apply to pavel-linux-leds/for-next wireless-drivers-next/master wireless-drivers/master]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented in
>>> https://git-scm.com/docs/git-format-patch]
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Amy-Parker/leds-change-led_brightness-definition-from-enum-to-typedef/20210716-052140
>>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git dd9c7df94c1b23feacd54112f33ad95d93f64533
>>> config: m68k-buildonly-randconfig-r006-20210715 (attached as .config)
>>> compiler: m68k-linux-gcc (GCC) 10.3.0
>>> reproduce (this is a W=1 build):
>>>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>>         chmod +x ~/bin/make.cross
>>>         # https://github.com/0day-ci/linux/commit/b14a971f1045205d49d9d001f33d33afdd8208f9
>>>         git remote add linux-review https://github.com/0day-ci/linux
>>>         git fetch --no-tags linux-review Amy-Parker/leds-change-led_brightness-definition-from-enum-to-typedef/20210716-052140
>>>         git checkout b14a971f1045205d49d9d001f33d33afdd8208f9
>>>         # save the attached .config to linux build tree
>>>         mkdir build_dir
>>>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash drivers/md/bcache/ drivers/media/v4l2-core/
>>>
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>>>
>>> All warnings (new ones prefixed by >>):
>>>
>>>    In file included from drivers/media/v4l2-core/v4l2-flash-led-class.c:15:
>>>>> include/media/v4l2-flash-led-class.h:18:1: warning: useless type name in empty declaration
>>>       18 | led_brightness;
>>>          | ^~~~~~~~~~~~~~
>>>
>>>
>>> vim +18 include/media/v4l2-flash-led-class.h
>>>
>>>     14
>>>     15  struct led_classdev_flash;
>>>     16  struct led_classdev;
>>>     17  struct v4l2_flash;
>>>   > 18  led_brightness;
>>>     19
>>>
>>> ---

> 
> Another patch was sent into the list to correct this error.

Hopefully Pavel (LED subsystem maintainer) will comment soon-ish.

My comments:

a. This patch would be the right thing to do if your large patch had already been
applied (merged) somewhere, but AFAIK it hasn't been. So:

b. IMO you should resend your entire patch set with this fix included.
Send it as "v2" (version 2) and explain the changes in it since your
original patch (which was v1). This v2 explanation should be below the
"---" line in the patch. (See Documentation/process/submitting-patches.rst
for more info -- or ask for more info/help.)

c. For your follow-up patch to include/media/v4l2-flash-led-class.h, which was:

-led_brightness;
+typedef u8 led_brightness;

I would just add this to include/media/v4l2-flash-led-class.h:

#include <linux/leds.h>

That way, in a few years, when the type of led_brightness changes again,
someone won't have to remember to search for other typedefs of it and
update them also. Or maybe they will do that after a bug happens and
someone notices it.

(Note that I am just trying to help. Pavel has more of a final
say-so about this.)


HTH.
-- 
~Randy