Re: [PATCH v2 2/2] leds: Add LED1202 I2C driver

[Lee Jones <lee@kernel.org>]

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 [李琼斯]