Hi Greg, thanks for your review! >Why do we need this noise in the kernel log? I guess it could be left there as a debug print? Knowing your hardware revision seems like a good, but yeah, not a necessary thing. >You can drop the GPL boilerplate text and add a proper SPDX line at the >top. Seems I only did that in my other local tree.. whoops! >Drivers should always use dev_err() and friends, as you have access to a >struct device * always. Please fix up the driver here to use that api >instead, no pr_* should be needed at all. Will do. >Horrible global symbol name. Who calls this? Welcome to development on qcom platforms :D >This is the last patch in >the series, so if there is no user for this, please don't export it. Other downstream drivers make use of it.. need to get this up first, sorry :V >Why do you need a .h file in the include directory if only a single .c >file needs it? Just put that info in the .c file itself. Again, other downstream drivers which some people and I intend to bring to upstream standards use that to access the PMIC model/hw revision. >But again, who uses this module? If it's only good for a single line in >the kernel log, that feels like a huge waste to me. downstream-kernel-dir$ rg -l qpnp-revid.h | wc -l 25 So yeah, quite a bunch of other qcom-specific drivers. I'll try to fix these and send a v2. Regards Konrad
