[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240626092744.GY1318296@google.com>
Date: Wed, 26 Jun 2024 10:27:44 +0100
From: Lee Jones <lee@...nel.org>
To: Vicentiu Galanopulo <vicentiu.galanopulo@...ote-tech.co.uk>
Cc: pavel@....cz, linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH v2 2/2] leds: Add LED1202 I2C driver
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 [李琼斯]
Powered by blists - more mailing lists