Re: [PATCH v2 00/15] Introduce block device LED trigger

From: Ian Pilcher <arequipeno@gmail.com>
To: "Marek Behún" <kabel@kernel.org>
Cc: axboe@kernel.dk, pavel@ucw.cz, linux-leds@vger.kernel.org,
	linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	gregkh@linuxfoundation.org
Subject: Re: [PATCH v2 00/15] Introduce block device LED trigger
Date: Fri, 10 Sep 2021 09:04:19 -0500	[thread overview]
Message-ID: <06581fe9-f9b2-5bee-07d6-e5b276a5e7f8@gmail.com> (raw)
In-Reply-To: <20210910040959.5ae4a6a1@thinkpad>

On 9/9/21 9:09 PM, Marek Behún wrote:
> I have tried to look into this and replied to some of your patches.
> 
> There are still many things to do, and I think the reviewing would be
> much easier to review if you sent all the code changes as one patch
> (since the changes are doing an atomic change: adding support for blkdev
> LED trigger). Keep only the sysfs doc change in a separate patch.

Marek -

I'll try to get a simplified version out as soon as I can.  It will
probably be 3 patches, because I do think that the block subsystem
changes should be in a separate patch.

(I agree that it will be simpler to review - not to mention easier for
me to create.  Past experience does tell me that there are likely some
folks who will object to that format, however.)

> You are unnecessary using the const keyword in places where it is not
> needed and not customary for Linux kernel codebase. See in another of
> my replies.

I did see that.  I'm a believer in declaring anything that should not
change as const (to the extent that C allows).  It documents the
fact that the value is expected to remain unchanged throughout the
function call, and it enlists the compiler to enforce it.

So while it's true that they aren't necessary, they do result in code
that is at least slightly less likely to be broken by future changes.

> You are using a weird comment style, i.e.
>    /*
>     *
>     *	Disassociate an LED from the trigger
>     *
>     */
> 
>    static void blkdev_deactivate(struct led_classdev *const led_dev)
> 
> Please look at how functions are documented in led-class.c, for example.

Well ... that comment isn't documenting that function.  It's intended to
identify a section of the file whose contents are related.  If there's a
different comment style that I should be using for that purpose, please
let me know.

I'll respond to your other feedback separately.

Thanks for taking your time on this.  I really do appreciate it!

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================