On Thu, Feb 27, 2025 at 11:19:37AM +0100, Mattijs Korpershoek wrote: > Hi Siddharth, > > Thank you for the the review. > > On jeu., févr. 27, 2025 at 13:44, Siddharth Vadapalli <s-vadapa...@ti.com> > wrote: > > > On Wed, Feb 26, 2025 at 03:47:11PM +0100, Mattijs Korpershoek wrote: > > > > Hello Mattijs, > > > >> The dwc3 driver allows SoC-specific configuration via the dwc3_glue_ops > >> structure. > >> These glue drivers usually just implement the .glue_configure function. > > > > nitpick: Is there a reason behind an abrupt newline above? The sentence: > > "These glue drivers..." could continue on the same line as the previous > > sentence. Also, "These glue drivers..." is referring to glue drivers > > which haven't been mentioned yet and it is the first time that they are > > being referred to. It is not clear to me if there was more content in > > between the first sentence and the second which accidentally got > > removed. > > I'll rephrase as: > > """ > The dwc3 driver allows SoC-specific configuration via the dwc3_glue_ops > structure. Glue drivers, which implement dwc3_glue_ops, usually just > define the .glue_configure() function. > """ > > Does that looks good ?
Yes. > > > > >> > >> For such glue drivers, we don't need to be a SIMPLE_BUS, since we > >> don't interact with any child devices. > > > > This sounds confusing. What does "we" refer to? The dwc3-am62.c driver > > which is being modified in this patch is *the* glue driver itself. > > "For such glue drivers" and "we don't need to be a SIMPLE_BUS" seem to > > be referring to two different entities when they are probably the same. > > > > "For such glue drivers" => dwc3-am62.c > > "we don't need to be a SIMPLE_BUS" => Again dwc3-am62.c ? > > I'll rephrase as: > > """ > dwc3-am62 is such glue driver which does not need to be a SIMPLE_BUS > since it does not interact with any child device. > """ > > Is that okay ? Yes. > > > > > Did you mean something like the following? > > "Glue drivers don't need to be a SIMPLE_BUS since they don't interact > > with any child devices" > > I can't write that down, because that's not true. Some glue drivers (like > dwc3-meson-g12a) need to be SIMPLE_BUS because they rely on > .child_pre_probe() and .child_post_remove(). Understood. Thank you for clarifying and for rephrasing the commit message above. Please feel free to add my Reviewed-by tag when posting the v2 patch. Regards, Siddharth.
