[Deepin-Kernel-SIG] [linux 6.6-y] [CIX] Apply syscon, gpio and phy suppors for CIX #642
Conversation
Add ACPI support for syscon subsystem. Signed-off-by: Lihua Liu <[email protected]> Signed-off-by: Guomin.Chen <[email protected]>
Add acpi support for cadence gpio driver. Signed-off-by: Zichar.Zhang <[email protected]>
Add usb2,usb3 and combo phy driver for cix. Signed-off-by: Guomin.Chen <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
We failed to fetch the diff for pull request #642
You can try again by commenting this pull request with @sourcery-ai review, or contact us for help.
|
@sourcery-ai review |
There was a problem hiding this comment.
We failed to fetch the diff for pull request #642
You can try again by commenting this pull request with @sourcery-ai review, or contact us for help.
|
@sourcery-ai review |
|
I suggest to splict the pull request to many small pull request to review, instead of many commits in one request in the situation :(. |
Avenger-285714
changed the title
Avenger-285714
changed the title
Avenger-285714
force-pushed
the
linux-6.6.y
branch
from
93ef82d to
efc56a4
Compare
Avenger-285714
changed the title
|
@sourcery-ai review |
Reviewer's Guide by SourceryThis pull request adds support for CIX hardware, including syscon, GPIO, and PHY drivers. It introduces ACPI support for existing drivers and adds new drivers for USB, USBDP, and USB2 PHYs. Sequence diagram for udphy_orien_sw_setsequenceDiagram
participant User
participant typec_switch_dev
participant cix_udphy
User->>+typec_switch_dev: udphy_orien_sw_set(sw, orien)
typec_switch_dev->>+cix_udphy: mutex_lock(&udphy->mutex)
cix_udphy->>cix_udphy: udphy->flip = (orien == TYPEC_ORIENTATION_REVERSE)
cix_udphy->>-typec_switch_dev: mutex_unlock(&udphy->mutex)
typec_switch_dev-->>User: return 0
Sequence diagram for usbdp_typec_mux_setsequenceDiagram
participant User
participant typec_mux_dev
participant cix_udphy
User->>+typec_mux_dev: usbdp_typec_mux_set(mux, state)
typec_mux_dev->>+cix_udphy: mutex_lock(&udphy->mutex)
cix_udphy->>cix_udphy: switch (state->mode)
cix_udphy->>cix_udphy: udphy->next_mode = ...
cix_udphy->>cix_udphy: usbdp_set_dpphy_mode(udphy)
cix_udphy->>-typec_mux_dev: mutex_unlock(&udphy->mutex)
typec_mux_dev-->>User: return 0
Class diagram for cix_u3phyclassDiagram
class cix_u3phy {
struct device *dev
void __iomem *base
struct regmap *phy_regmap
struct regmap *usbphy_syscon
struct reset_control *preset
struct reset_control *reset
struct clk *apb_clk
struct clk *ref_clk
struct mutex mutex
bool init
int init_count
int id
const struct cix_u3phy_cfg *cfg
}
class cix_u3phy_cfg {
int (*u3phy_init)(struct cix_u3phy *u3phy)
int (*u3phy_exit)(struct cix_u3phy *u3phy)
}
cix_u3phy -- cix_u3phy_cfg : has a
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @Avenger-285714 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a Kconfig option for the CIX syscon driver to allow it to be disabled if necessary.
- The phy driver adds a lot of code - can it be split into multiple commits?
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert_phy: | ||
| reset_control_assert(udphy->reset); | ||
|
|
||
| assert_apb: |
There was a problem hiding this comment.
suggestion (bug_risk): Return error code instead of 0 on failure in sky1_udphy_init.
In the error-handling paths (after "assert_phy:" and "assert_apb:"), the function unconditionally returns 0 even when an error has occurred (e.g. if regmap_multi_reg_write fails or udphy_status_check returns an error). This behavior might hide failures and lead the caller to incorrectly assume success. Consider propagating the appropriate error code instead.
Suggested implementation:
assert_phy:
reset_control_assert(udphy->reset);
return ret; assert_apb:
reset_control_assert(udphy->preset);
clk_disable_unprepare(udphy->apb_clk);
return ret;Ensure that 'ret' is correctly set to a valid error code before branching to these error-handling labels. If additional clean-up is needed before returning the error code, consider that in the function design.
| preset = devm_reset_control_get(dev, "preset"); | ||
|
|
||
| if (IS_ERR(preset)) | ||
| dev_err(dev, "%s: failed to get preset\n", __func__); | ||
| else | ||
| reset_control_deassert(preset); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
suggestion (bug_risk): Revisit error handling for missing reset control.
Currently, if obtaining the reset control fails, an error is logged but the probe continues and returns success. If the reset control is critical to proper operation, consider returning the error code (e.g., PTR_ERR(preset)) to prevent initializing in an invalid state.
| preset = devm_reset_control_get(dev, "preset"); | |
| if (IS_ERR(preset)) | |
| dev_err(dev, "%s: failed to get preset\n", __func__); | |
| else | |
| reset_control_deassert(preset); | |
| return 0; | |
| preset = devm_reset_control_get(dev, "preset"); | |
| if (IS_ERR(preset)) { | |
| dev_err(dev, "%s: failed to get preset\n", __func__); | |
| return PTR_ERR(preset); | |
| } | |
| reset_control_deassert(preset); | |
| return 0; |
| } | ||
| return 0; | ||
| } | ||
| static int sky1_u3phy_init(struct cix_u3phy *u3phy) |
There was a problem hiding this comment.
issue (complexity): Consider using helper functions to consolidate error cleanup and initialization steps, reducing nested gotos and improving readability in the init function
Consider consolidating the error‐cleanup and initialization steps into helper functions to reduce the nested goto/if structure. For example, you can extract the cleanup work in the init function:
```c
static void cleanup_clocks_and_reset(struct cix_u3phy *u3phy)
{
clk_disable_unprepare(u3phy->ref_clk);
clk_disable_unprepare(u3phy->apb_clk);
reset_control_deassert(u3phy->preset);
reset_control_deassert(u3phy->reset);
u3phy->init = false;
}Then simplify your init function as follows:
static int sky1_u3phy_init(struct cix_u3phy *u3phy)
{
int ret;
if (!u3phy->init) {
reset_control_assert(u3phy->reset);
reset_control_assert(u3phy->preset);
ret = clk_prepare_enable(u3phy->apb_clk);
if (ret) {
dev_err(u3phy->dev, "Failed to enable apb clock\n");
goto err;
}
ret = clk_prepare_enable(u3phy->ref_clk);
if (ret) {
dev_err(u3phy->dev, "Failed to enable ref clock\n");
goto disable_apb;
}
reset_control_deassert(u3phy->preset);
ret = regmap_multi_reg_write(u3phy->phy_regmap, sky1_u3phy_conf,
ARRAY_SIZE(sky1_u3phy_conf));
if (ret) {
dev_err(u3phy->dev, "Failed to write the reg sequence\n");
goto disable_ref;
}
u3phy->init = true;
reset_control_deassert(u3phy->reset);
}
u3phy->init_count++;
return 0;
disable_ref:
clk_disable_unprepare(u3phy->ref_clk);
disable_apb:
clk_disable_unprepare(u3phy->apb_clk);
err:
cleanup_clocks_and_reset(u3phy);
return ret;
}This refactoring removes multiple nested goto labels and duplicate cleanup calls while keeping all functionality intact.
没用。 |
Summary by Sourcery
Adds support for CIXtech devices, including syscon, GPIO, and PHY drivers. This includes adding ACPI IDs for syscon and GPIO, and introducing new PHY drivers for USBDP, USB3, and USB2.
New Features: