On Mon, Apr 10, 2023, Stanley Chang[昌育德] wrote:
>
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > > 476b63618511..771b35449376 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -1785,6 +1785,23 @@ static int dwc3_probe(struct platform_device
> > *pdev)
> > > dwc_res = *res;
> > > dwc_res.start += DWC3_GLOBALS_REGS_START;
> > >
> > > + /* For some dwc3 controller, the dwc3 global register start address is
> > > + * not at DWC3_GLOBALS_REGS_START (0xc100).
> > > + */
> >
> > Please use this comment style block:
> > /*
> > * a b c
> > * d e f
> > */
> >
>
> I will follow this rule.
>
> > > + if (dev) {
> >
> > Why do we need this if (dev) check? When would this not be the case?
>
> I want the variable "fixed_dwc3_globals_regs_start" to be a local variable.
> So I added an if statement.
> I can modify it to "if (dev->of_node)" which looks more make sense.
Why? If it's within this function, it's a local variable. Don't create
arbitrary condition just to limit the scope of the variable.
Thanks,
Thinh
>
> >
> > > + int fixed_dwc3_globals_regs_start;
> >
> > Need to initialize this in case you get bogus value when the property is not
> > defined.
>
> Thanks. I will add the initial value.
>
> >
> > > +
> > > + device_property_read_u32(dev,
> > > + "snps,fixed_dwc3_globals_regs_start",
> >
> > Property name should be using "-" instead of "_". Also can we rename it to
> > "snps,global-regs-starting-offset"
> >
> > Thanks,
> > Thinh
> >
>
> I will rename as "snps,global-regs-starting-offset" in next version.
>
> Thanks a lot of.
> Stanley
