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

From: Vicentiu Galanopulo
Date: Tue Jun 25 2024 - 17:51:03 EST


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?