On 4/7/20 9:28 AM, Pavel Machek wrote: > Hi! > >>> I'm sorry I failed to meet your high expectations... But I don't >>> believe I done anything completely outside of usual kernel procedures. >> >> I believe code review is quite usual kernel procedure. > > I don't disagree with that. > >>> Could you list the patches and objections you have? >> >> I already expressed my concerns regarding Turris Omnia patch. > > Ok. > >> My comments regarding remaining patches: >> >> - "Make label "white:power" to be consistent with" >> >> I disagree here. "system" was OK. > > It was too vague... I know the hardware and it is a LED above power > button used as a power indicator. > >> - "Warn about old defines that probably should not be used." >> >> Obsolete is only LED_FULL, so the comment is in wrong line > > No, all of them are bad. Maybe LED_OFF could be used going forward, > but... it is simply easier to write 0. The type is not really an en > enumeration, it is brightness, with variable maximum value. In this case brightness should be turned into an int and the change should be applied throughout the whole kernel. Otherwise it is questionable - why all enums are made obsolete if the type of struct led_classdev's brightness property is still enum led_brightness? Do we have some replacement? - one could ask. > >> - "Group LED functions according to functionality, and add some" >> >> You're adding here some random comments referencing obsolete >> naming. I think that it is enough to say what is current standard. > > Ok, I'll drop that part. But I really want to get that documented > _somewhere_, because obsolete naming is currently in use, and we won't > be able to change it :-(. You've pushed it out anyway... >> Also, I had a patch [0] describing standard LED functions in my LED >> naming patch set, but it was not merged. It could be worth getting >> back to it at this occasion. > > I'll take a look. > > Best regards, > Pavel > -- Best regards, Jacek Anaszewski
