Re: [PATCH net-next 1/1] net: phy: Refactor mediatek-ge-soc.c for clarity and correctness

[Simon Horman <horms@kernel.org>]

On Tue, Oct 15, 2024 at 05:18:49PM +0000, SkyLake Huang (黃啟澤) wrote:
> On Mon, 2024-10-14 at 09:18 +0100, Simon Horman wrote:
> >  	 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  On Mon, Oct 14, 2024 at 12:05:21PM +0800, Sky Huang wrote:
> > > From: "SkyLake.Huang" <skylake.huang@mediatek.com>
> > > 
> > > This patch does the following clean-up:
> > > 1. Fix spelling errors and rearrange variables with reverse
> > >    Xmas tree order.
> > > 2. Shrink mtk-ge-soc.c line wrapping to 80 characters.
> > > 3. Propagate error code correctly in cal_cycle().
> > > 4. Fix some functions with FIELD_PREP()/FIELD_GET().
> > > 5. Remove unnecessary outer parens of supported_triggers var.
> > > 
> > > Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
> > > ---
> > > This patch is derived from Message ID:
> > > 20241004102413.5838-9-SkyLake.Huang@mediatek.com
> > 
> > Hi Sky,
> > 
> > I think this patch is trying to do two many things (at least 5 are
> > listed
> > above). Please consider breaking this up into separate patches,
> > perhaps
> > one for each of the points above.
> > 
> > Also, I think it would be best to drop the following changes unless
> > you are
> > touching those lines for some other reason [1] or are preparing to do
> > so:
> > 
> > * xmas tree
> > * 80 character lines
> > * parentheses updates
> > 
> > [1] 
> > https://docs.kernel.org/process/maintainer-netdev.html#clean-up-patches
> 
> Hi Simon,
>   Reverse Xmas tree style & 80 char preferences come from your advise
> in  
> https://lore.kernel.org/all/20240530034844.11176-6-SkyLake.Huang@mediatek.com/
>   Parens removing comes from Daniel's advise in 
> https://lore.kernel.org/all/20240701105417.19941-14-SkyLake.Huang@mediatek.com/

Ok, sorry for the mixed messages.
In this case think these can stay after all.

> 
>   Because previous patchset(
> https://lore.kernel.org/all/20240701105417.19941-1-SkyLake.Huang@mediatek.com/
> ) is too large, I guess it's better to commit this change first so that
> I can handle the rest. And this should be "some other reason"?

I think it is sufficient to bring to our attention that there is "some
other reason". Sorry for not remembering it.

>   And since this patch is simple clean-ups, is it necessary to separate
> it?

I do think that would be best.
But if you strongly think otherwise I can try to review it as-is.