Re: [PATCH 1/2] usb: dwc2: Extract USB DWC2 register definitions

[Marek Vasut <marex@denx.de>]

On 6/27/24 1:33 PM, Kongyang Liu wrote:
> Marek Vasut <marex@denx.de> 于2024年6月23日周日 07:49写道：
>>
>> On 5/22/24 4:22 PM, Kongyang Liu wrote:
>>
>> Hi,
>>
>> sorry for the late reply.
>>
>>> diff --git a/drivers/usb/common/dwc2_core.c b/drivers/usb/common/dwc2_core.c
>>> new file mode 100644
>>> index 0000000000..2fa11fd59d
>>> --- /dev/null
>>> +++ b/drivers/usb/common/dwc2_core.c
>>> @@ -0,0 +1,53 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (c) 2024, Kongyang Liu <seashell11234455@gmail.com>
>>> + */
>>> +
>>> +#include <linux/bitfield.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/io.h>
>>> +#include <wait_bit.h>
>>> +
>>> +#include "dwc2_core.h"
>>> +
>>> +void dwc2_flush_tx_fifo(struct dwc2_core_regs *regs, const int num)
>>> +{
>>> +     int ret;
>>> +
>>> +     log_debug("Flush Tx FIFO %d\n", num);
>>> +
>>> +     /* Wait for AHB master IDLE state */
>>> +     ret = wait_for_bit_le32(&regs->global_regs.grstctl, GRSTCTL_AHBIDLE, true, 1000, false);
>>
>> Just a quick design point, would it be possible to split this patch into
>> two, one which adds this .global_regs and changes the code accordingly,
>> and another which does the code refactoring/move ? That would make it
>> easier to review.
> 
> This patch only extracts the common parts of the host mode and gadget
> mode, without any code refactoring. Could you please describe more clearly
> how the patch should be split?

Sure, sorry for being unclear. There seems to be multiple types of 
changes that are combined in this patch:

1) Structure reorganization:
-	writel(DIEPMSK_INIT, &reg->diepmsk);
+	writel(DIEPMSK_INIT, &reg->device_regs.diepmsk);

2) Structure rename:
-	reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
+	reg = (struct dwc2_core_regs *)pdata->regs_otg;

3) Conversion from (1 << n) to BIT(n)
4) Macro rename:
5) Moving code around between files:
#define DWC2_HWCFG4_IDDIG_FILT_EN			(1 << 20)
#define GHWCFG4_IDDIG_FILT_EN			BIT(20)

It would be good to have those as separate patches (one type of clean up 
change per patch), otherwise it is really hard to review such a combined 
patch.

Thanks !