On Tue, 25 Jun 2024, Vicentiu Galanopulo wrote: > 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 What does the comment above say? If you agree with a review comment, you don't need to reply to it. > > > 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 That seems wrong. I haven't looked at what you're doing in detail, but dev_name() is usually used to fetch the name of the device. See include/linux/device.h for more generic APIs. > "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? No one will stop you. :) -- Lee Jones [李琼斯]
