On Tue, Jun 25, 2024 at 08:16:10AM +0100, Lee Jones wrote: > No idea. I use NeoVim (with kickstart.nvim). > > https://dev.to/lico/set-up-neovim-with-kickstartnvim-on-mac-as-a-vimginner-53f5 Thanks for the pointer. > > Please strip out review comments that you agree with. Hopefully like I did for the rest of comments? > > Numbers should be easily identifiable/readable by humans. Ok, will do my best > > I reused some naming. Should it be led1202_ for all? > > st1202_? st1202 will be in v3 > > If this is not appropiate or custom practice I can redo it, but I need some pointers > > on where to look as "good" examples. > > Google: "Linux Error Codes" > > `git grep "return " -- drivers` My concern was mostly with how I'm extracting the channel(LED number). ll1202_get_channel is called inside functions where only struct device is available. So, I extract the device_node to have access to the device tree "label". I'm char compairing label value and dev->kobj.name, and if they're the same, I use the "reg" value property from the device tree to get the LED number. For most if not all of the functions I did see some similar setup in other drivers files, but I might be doing something the wrong way... > > A dump of all the registers with their values. I didn't add show/get functions for > > all the registers. > > Remove it? > > How often are people going to need that after initial authorship, really? > No idea. I'll just remove it. > > > > > > Space out the code properly - this is really tough to read. > > > > > Ok.. with or without the help of the IDE, it shall be done > > I mean new lines between functional groups. > Understood. > > > > +} > > > > + > > > > +static int ll1202_channel_activate(struct ll1202_led *led) > > > > +{ > > > > + struct ll1202_chip *chip; > > > > + uint8_t reg_chan_low, reg_chan_high; > > > > + int ret = 0; > > > > + > > > > + chip = led->chip; > > > > + if (led->is_active) { > > > > > > Reverse this logic and unindent this block. > > > > > Sorry, I need some more details on what I need to do here. > > if (!led->is_active) > return ret; > Thanks for explaining this. > > > > > > We already have global helpers for this type of thing. > > > > > Ok, could you please point me to the file/link? > > I suggest you pull as much of this out to another _normal_ function as > you can, then have the fewest lines possible inside the macro instead. > Ok. Will do. > -- > Lee Jones [李琼斯] Thanks Lee. May I now push a v3?
