| Message ID | 20240124-dwc3-of-simple-reset-control-array-fix-v1-1-808182cc3f0e@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3-of-simple: Stop using of_reset_control_array_get() directly | expand |
On Wed, Jan 24, 2024 at 12:26:20PM +0100, Philipp Zabel wrote: > Use of_reset_control_array_get_optional_exclusive() instead, it is > implemented as: > > static inline struct reset_control * > of_reset_control_array_get_optional_exclusive(struct device_node *node) > { > return of_reset_control_array_get(node, false, true, true); > } > > This makes the code easier to understand and removes the last remaining > direct use of of_reset_control_array_get(). No functional changes. > > Fixes: f4cc91ddd856 ("usb: dwc3: of-simple: remove Amlogic GXL and AXG compatibles") No functional change, but a Fixes: tag? That doesn't make sense to me, sorry. thanks, greg k-h
On Wed, Jan 24, 2024 at 12:26:20PM +0100, Philipp Zabel wrote: > Use of_reset_control_array_get_optional_exclusive() instead, it is > implemented as: > > static inline struct reset_control * > of_reset_control_array_get_optional_exclusive(struct device_node *node) > { > return of_reset_control_array_get(node, false, true, true); > } > > This makes the code easier to understand and removes the last remaining > direct use of of_reset_control_array_get(). No functional changes. Does this mean the function should be removed or made static now? thanks, greg k-h
On Mi, 2024-01-24 at 04:39 -0800, Greg Kroah-Hartman wrote: > On Wed, Jan 24, 2024 at 12:26:20PM +0100, Philipp Zabel wrote: > > Use of_reset_control_array_get_optional_exclusive() instead, it is > > implemented as: > > > > static inline struct reset_control * > > of_reset_control_array_get_optional_exclusive(struct device_node *node) > > { > > return of_reset_control_array_get(node, false, true, true); > > } > > > > This makes the code easier to understand and removes the last remaining > > direct use of of_reset_control_array_get(). No functional changes. > > > > Fixes: f4cc91ddd856 ("usb: dwc3: of-simple: remove Amlogic GXL and AXG compatibles") > > No functional change, but a Fixes: tag? That doesn't make sense to me, > sorry. The referenced patch made the boolean parameters const but missed that there is a static inline wrapper for this combination. I can drop the Fixes: tag and describe this in the text. regards Philipp
On Mi, 2024-01-24 at 04:39 -0800, Greg Kroah-Hartman wrote: > On Wed, Jan 24, 2024 at 12:26:20PM +0100, Philipp Zabel wrote: > > Use of_reset_control_array_get_optional_exclusive() instead, it is > > implemented as: > > > > static inline struct reset_control * > > of_reset_control_array_get_optional_exclusive(struct device_node *node) > > { > > return of_reset_control_array_get(node, false, true, true); > > } > > > > This makes the code easier to understand and removes the last remaining > > direct use of of_reset_control_array_get(). No functional changes. > > Does this mean the function should be removed or made static now? I consider "hiding" it from general use by renaming it to __of_reset_control_array_get(). There are other, indirect users of this function, but it is always called via a more self-explanatory static inline function: drivers/amba/bus.c: of_reset_control_array_get_optional_shared() drivers/net/dsa/lantiq_gswip.c: of_reset_control_array_get_exclusive() drivers/phy/cadence/phy-cadence-sierra.c: of_reset_control_array_get_exclusive() drivers/phy/cadence/phy-cadence-torrent.c: of_reset_control_array_get_exclusive() drivers/soc/tegra/pmc.c: of_reset_control_array_get_exclusive_released() drivers/usb/dwc3/dwc3-of-simple.c: of_reset_control_array_get() I would like to eventually replace the use of multiple boolean parameters for configuration. It is hard to read and errors have slipped through in the past (e.g. a57f68ddc886, "reset: Fix devm bulk optional exclusive control getter"). regards Philipp
On Wed, Jan 24, 2024 at 01:56:18PM +0100, Philipp Zabel wrote: > On Mi, 2024-01-24 at 04:39 -0800, Greg Kroah-Hartman wrote: > > On Wed, Jan 24, 2024 at 12:26:20PM +0100, Philipp Zabel wrote: > > > Use of_reset_control_array_get_optional_exclusive() instead, it is > > > implemented as: > > > > > > static inline struct reset_control * > > > of_reset_control_array_get_optional_exclusive(struct device_node *node) > > > { > > > return of_reset_control_array_get(node, false, true, true); > > > } > > > > > > This makes the code easier to understand and removes the last remaining > > > direct use of of_reset_control_array_get(). No functional changes. > > > > > > Fixes: f4cc91ddd856 ("usb: dwc3: of-simple: remove Amlogic GXL and AXG compatibles") > > > > No functional change, but a Fixes: tag? That doesn't make sense to me, > > sorry. > > The referenced patch made the boolean parameters const but missed that > there is a static inline wrapper for this combination. I can drop the > Fixes: tag and describe this in the text. That would be best, thanks. greg k-h
On Sat, Jan 27, 2024 at 04:34:14PM -0800, Greg Kroah-Hartman wrote: > On Wed, Jan 24, 2024 at 01:56:18PM +0100, Philipp Zabel wrote: > > On Mi, 2024-01-24 at 04:39 -0800, Greg Kroah-Hartman wrote: > > > On Wed, Jan 24, 2024 at 12:26:20PM +0100, Philipp Zabel wrote: > > > > Use of_reset_control_array_get_optional_exclusive() instead, it is > > > > implemented as: > > > > > > > > static inline struct reset_control * > > > > of_reset_control_array_get_optional_exclusive(struct device_node *node) > > > > { > > > > return of_reset_control_array_get(node, false, true, true); > > > > } > > > > > > > > This makes the code easier to understand and removes the last remaining > > > > direct use of of_reset_control_array_get(). No functional changes. > > > > > > > > Fixes: f4cc91ddd856 ("usb: dwc3: of-simple: remove Amlogic GXL and AXG compatibles") > > > > > > No functional change, but a Fixes: tag? That doesn't make sense to me, > > > sorry. > > > > The referenced patch made the boolean parameters const but missed that > > there is a static inline wrapper for this combination. I can drop the > > Fixes: tag and describe this in the text. > > That would be best, thanks. Ah you already did so, thanks!
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c index d1539fc9eabd..9cf9ee1b637b 100644 --- a/drivers/usb/dwc3/dwc3-of-simple.c +++ b/drivers/usb/dwc3/dwc3-of-simple.c @@ -52,8 +52,7 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) if (of_device_is_compatible(np, "rockchip,rk3399-dwc3")) simple->need_reset = true; - simple->resets = of_reset_control_array_get(np, false, true, - true); + simple->resets = of_reset_control_array_get_optional_exclusive(np); if (IS_ERR(simple->resets)) { ret = PTR_ERR(simple->resets); dev_err(dev, "failed to get device resets, err=%d\n", ret);
Use of_reset_control_array_get_optional_exclusive() instead, it is implemented as: static inline struct reset_control * of_reset_control_array_get_optional_exclusive(struct device_node *node) { return of_reset_control_array_get(node, false, true, true); } This makes the code easier to understand and removes the last remaining direct use of of_reset_control_array_get(). No functional changes. Fixes: f4cc91ddd856 ("usb: dwc3: of-simple: remove Amlogic GXL and AXG compatibles") Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/usb/dwc3/dwc3-of-simple.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d change-id: 20240124-dwc3-of-simple-reset-control-array-fix-e4e9822028dd Best regards,