Re: [PATCH v7] ath10k: add LED and GPIO controlling support for various chipsets

From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
To: Steve deRosier <derosier@gmail.com>
Cc: Sebastian Gottschall <s.gottschall@newmedia-net.de>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	ath10k@lists.infradead.org, Kalle Valo <kvalo@codeaurora.org>
Subject: Re: [PATCH v7] ath10k: add LED and GPIO controlling support for various chipsets
Date: Tue, 20 Feb 2018 20:49:53 +0100	[thread overview]
Message-ID: <248047fb-3805-5b49-bb98-6ace37bc31ca@dd-wrt.com> (raw)
In-Reply-To: <CALLGbR+GQHBcuoO=6BVJYLUhwQk=tv2juWBRDpkyrDtFedfn5Q@mail.gmail.com>


> Agreed, I am also not a fan of bloat code. I wasn't suggesting you
> move it to the .h, just saying that if it were not static, that's how
> I'd suggest doing it.
>
> In this case, keep it static, keep it in the .c.  And use the blocking
> I suggested. You will get the optimizer friendly result you're looking
> for, but make it more readable and more-inline with the dominant style
> for these things.
thats what i did already in my sourcetree
>
>> true, but consider that required structures like gpio_chip (ar->gpio) and
>> led_classdev arent defined in the kernel if these CONFIG options arent
>> configured. so i may move it into a separate function ,but the guards will
>> remain.
>> thats not my fault. thats a fault of the kernel api.
>>
> Hm. I assumed the names were still valid, even if the functionality
> that uses it wasn't.  Understood.
>
> In that case, I suggest moving the code to a separate static function
> that can be guarded. It keeps the mess out of the main startup
> function, makes it easier to read as it collapses to a single
> self-documented function name, and gets rid of the label and jump.
yes but we have 2 guards. GPIOLIB and LED_CLASS
so the mess remaines in the same way with no change. the same unreadable 
code just moved (personally i think its not that unreadable since its small)

>
>>> The rest of this looks OK to me.
>> okay. i already changed some code, based on your requests but will wait for
>> reply of you and for other comments until i send out a new version
> I appreciate the extra work you've put in. I hadn't expected my
> comments would've generated so much extra work; sorry about that. It's
> looking good.
i just want it upstream. i have alot of usefull patches in my backhand 
and its now getting hard to maintain the code with new ath10k patches.
and at the end its not that much work as it seems. just a question of 
some seconds todo that changes. i can live with it.

Sebastian


-- 
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k