Re: [PATCH V3 08/11] soc: mediatek: PMIC wrap: remove pwrap_is_mt8135() and pwrap_is_mt8173()

[Yingjoe Chen <yingjoe.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>]

On Mon, 2016-02-01 at 12:27 +0100, Matthias Brugger wrote:
> 
> On 01/02/16 12:15, John Crispin wrote:
> >
> >
> > On 01/02/2016 12:11, Matthias Brugger wrote:
> >>
> >>
> >> On 01/02/16 12:00, John Crispin wrote:
> >>>
> >>>
> >>> On 01/02/2016 11:55, Matthias Brugger wrote:
> >>>>
> >>>>
> >>>> On 25/01/16 10:53, John Crispin wrote:
> >>>>> With ore SoCs being added the list of helper functions like these would
> >>>>
> >>>> The commit message is something strange:
> >>>> "With every new SoC being added..." maybe?
> >>>>
> >>>>> grow. While at it also add a new flag "bridge" and use that insted of
> >>>>
> >>>> s/insted/instead
> >>>>
> >>>>> pwrap_is_mt8173() where appropriate.
> >>>
> >>> you are lookign at V3 of the series, V4 has this fix done already
> >>>
> >>> [...]
> >>>
> >>>
> >>>>>         }
> >>>>> @@ -830,6 +824,7 @@ static struct pmic_wrapper_type pwrap_mt8135 = {
> >>>>>         .int_en_all = BIT(31) | BIT(1),
> >>>>>         .spi_w = PWRAP_MAN_CMD_SPI_WRITE,
> >>>>>         .wdt_src = PWRAP_WDT_SRC_MASK_ALL,
> >>>>> +    .has_bridge = 1,
> >>>>>         .init_reg_clock = pwrap_mt8135_init_reg_clock,
> >>>>>         .init_special = pwrap_mt8135_init_special,
> >>>>>     };
> >>>>
> >>>> Please set has_bridge explicitly for mt8173.
> >>>
> >>> I dont get it. the original code never did that.
> >>>
> >>
> >> has_bridge was introduced by this patch, but you don't set it explicitly
> >> to 0 in pwrap_mt8173.
> >>
> >> Just as I see it, please try to write a summary to every new version of
> >> a patch set which explains what you changed between one version and
> >> another. This will help a lot making the review easier.
> >>
> >> Thanks,
> >> Matthias
> >>
> >
> >
> > You missed the "to zero" part before. now the comment makes sense. I can
> > set it to 0 if it is more obvious for you in that case.
> >
> > general consent is to not declare statics to 0. check_patch.pl will
> > actually complain about those declarations. that is why i was confused.
> 
> If that's the case, then we accept the authority of check_patch.pl ;)
> I didn't know that, so just leave has_bridge as it was.

I believe checkpatch only complain for global/static variables
initializers, not inside a struct initializers even when they are
global.

In MTK i2c drivers drivers/i2c/busses/i2c-mt65xx.c, we always explicitly
init flags in compatible struct, IMHO it make support feature for each
IC more clearly. This does not trigger any warning from checkpatch.

Joe.C