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

[Vicentiu Galanopulo <vicentiu.galanopulo@remote-tech.co.uk>]

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?