[Deepin-Kernel-SIG] [linux 6.6-y] [CIX] Apply usb, irpchip and i2c suppors for CIX #646
Conversation
Add usb controller driver for Cix. Add platform driver for sky1 platform. Signed-off-by: Guomin.Chen <[email protected]>
Add realtek typec pd driver for sky1. Signed-off-by: Chao.Zeng <[email protected]> Signed-off-by: Guomin.Chen <[email protected]>
add irqchip driver for sky1. Signed-off-by: Richie.Ren <[email protected]> Signed-off-by: Wenxue.Ding <[email protected]> Signed-off-by: Guomin.Chen <[email protected]>
add cadence i2c adapter driver for Cix. Signed-off-by: Jun.Guo <[email protected]> Signed-off-by: Lihua Liu <[email protected]> Signed-off-by: Guomin.Chen <[email protected]>
Reviewer's Guide by SourceryThis pull request introduces new drivers and configurations to support CIX hardware, specifically adding USB host/device/OTG, I2C, and interrupt controller functionality. It includes clock and reset management, device-specific configurations, and integration with the broader kernel framework. Class diagram for cdnsp_sky1 structclassDiagram
class cdnsp_sky1 {
*dev : struct device
*axi_base : void __iomem
*ctst_base : void __iomem
*reset : struct reset_control
*preset : struct reset_control
id : int
*usb_syscon : struct regmap
u3_disable : bool
sof_clk_freq : u32
lpm_clk_freq : u32
axi_bmax_value : u32
*xhci_base : void
*device_base : void
*cix_usb_clks : struct clk**
}
cdnsp_sky1 : device
cdnsp_sky1 : reset_control
cdnsp_sky1 : regmap
cdnsp_sky1 : clk
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[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.
Hey @Avenger-285714 - I've reviewed your changes - here's some feedback:
Overall Comments:
- This PR introduces several new drivers; consider splitting it into smaller, more manageable chunks.
- It would be helpful to include a cover letter that provides a high-level overview of the changes and their purpose.
Here's what I looked at during the review
- 🟡 General issues: 4 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.
| { | ||
| struct cdnsp_sky1 *data = dev_get_drvdata(dev); | ||
|
|
||
| return regmap_update_bits(data->usb_syscon, |
There was a problem hiding this comment.
suggestion (bug_risk): Consider validating the index into sky1_usb_signals.
The function uses data->id to index sky1_usb_signals without any bounds check. A guard to ensure that data->id is within the expected range would protect against potential invalid memory accesses.
Suggested implementation:
struct cdnsp_sky1 *data = dev_get_drvdata(dev);
if (data->id < 0 || data->id >= ARRAY_SIZE(sky1_usb_signals))
return -EINVAL; return regmap_update_bits(data->usb_syscon,
sky1_usb_signals[data->id].offset,
GENMASK(sky1_usb_signals[data->id].bit+1,
sky1_usb_signals[data->id].bit),
mode << sky1_usb_signals[data->id].bit);If not already included, ensure that ARRAY_SIZE() is available by including the appropriate header such as <linux/kernel.h>.
| char data_device[] = "usb device"; | ||
|
|
||
|
|
||
| static int rts5453h_typec_port_update(struct rts5453h *typec) |
There was a problem hiding this comment.
issue (bug_risk): Avoid storing pointers to local stack data in persistent state.
In this function a local variable (dp_data) is assigned and its address is stored in typec->state.data. While the mux configuration call may use the structure synchronously, storing a pointer to a local variable risks later use after the function returns. Consider either using a persistent buffer or copying the displayport data into the typec state.
| struct fwnode_handle *fwnode; | ||
| struct device *rdev; | ||
|
|
||
| fwnode = fwnode_find_reference(dev_fwnode(dev), string, 0); |
There was a problem hiding this comment.
suggestion: Consider releasing the fwnode reference.
In devm_phy_optional_ref_get, the retrieved fwnode may require an explicit fwnode_handle_put when it is no longer needed. This ensures that its reference count is managed properly.
Suggested implementation:
fwnode = fwnode_find_reference(dev_fwnode(dev), string, 0);
if (IS_ERR_OR_NULL(fwnode))
return NULL;
{
const char *fwnode_name;
struct fwnode_handle *fwnode_parent;
fwnode_name = fwnode_get_name(fwnode);
fwnode_parent = fwnode_get_parent(fwnode);
fwnode_handle_put(fwnode); rdev = get_dev_from_fwnode(fwnode_parent); phy = phy_get(rdev, fwnode_name);Since we introduced a new block that scoping the new variables (fwnode_name and fwnode_parent), make sure that the remainder of the function that needs these values is within the same scope (or adjust the variable declarations accordingly) if they are required later.
| raw_spin_unlock_irqrestore(&cd->rlock, flags); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
suggestion (bug_risk): Consider evaluating the SMC call result.
The function sky1_pdc_irq_set_wake issues an SMCCC call but unconditionally returns 0. Adding error checking based on the returned values from arm_smccc_smc could improve robustness, especially for failure scenarios.
| raw_spin_unlock_irqrestore(&cd->rlock, flags); | |
| return 0; | |
| raw_spin_unlock_irqrestore(&cd->rlock, flags); | |
| /* Evaluate the result of the SMCCC call and return an error if needed */ | |
| if (res.a0 != 0) | |
| return -EINVAL; | |
| return 0; |
| * | ||
| */ | ||
|
|
||
| int sky1_usb_clk_enable_all(struct device *dev) |
There was a problem hiding this comment.
issue (complexity): Consider creating helper functions for clock enabling/disabling and register writing to reduce code duplication and improve readability throughout the driver.
Consider extracting common helper functions to eliminate duplicated looping and register‐write logic. For example, you could create a generic clock enable/disable helper:
static int enable_clks(struct device *dev, struct clk **clks,
const char * const clk_names[], int count)
{
int i, ret;
for (i = 0; i < count; i++) {
clks[i] = devm_clk_get_optional(dev, clk_names[i]);
if (IS_ERR(clks[i]))
return dev_err_probe(dev, PTR_ERR(clks[i]),
"could not get %s clock\n", clk_names[i]);
if (!clks[i])
continue;
ret = clk_prepare_enable(clks[i]);
if (ret)
goto err;
}
return 0;
err:
while (--i >= 0)
clk_disable_unprepare(clks[i]);
return ret;
}
static void disable_clks(struct clk **clks, int count)
{
int i;
for (i = 0; i < count; i++)
clk_disable_unprepare(clks[i]);
}Then replace duplicated clock loops with calls to these helpers. Similarly, the sequences writing pre-register values (for SOF and LPM) can be consolidated. For instance:
static void write_pre_regs(void __iomem *base, unsigned int clk,
const unsigned int pre_offsets[8],
bool return_zero_when_disabled)
{
int v0 = (int)(25 * clk / 100000000);
int v1 = (int)(clk / 10000);
int v2 = (int)(clk / 10);
/* Adjust the default values based on context (enable vs resume) */
unsigned int v0_val = (v0 > 1 ? v0 - 1 : (return_zero_when_disabled ? 0 : 1));
unsigned int v1_100_val = (v1 / 100 > 1 ? v1 / 100 - 1 : (return_zero_when_disabled ? 0 : 1));
unsigned int v1_10_val = (v1 / 10 > 1 ? v1 / 10 - 1 : (return_zero_when_disabled ? 0 : 1));
unsigned int v1_val = (v1 > 1 ? v1 - 1 : (return_zero_when_disabled ? 0 : 1));
unsigned int reg125_val = (125 * clk / 1000000 > 1 ? 125 * clk / 1000000 : (return_zero_when_disabled ? 0 : 1));
unsigned int v2_100_val = (v2 / 100 > 1 ? v2 / 100 - 1 : (return_zero_when_disabled ? 0 : 1));
unsigned int v2_10_val = (v2 / 10 > 1 ? v2 / 10 - 1 : (return_zero_when_disabled ? 0 : 1));
unsigned int v2_val = (v2 > 1 ? v2 - 1 : (return_zero_when_disabled ? 0 : 1));
writel(v0_val, base + pre_offsets[0]);
writel(v1_100_val, base + pre_offsets[1]);
writel(v1_10_val, base + pre_offsets[2]);
writel(v1_val, base + pre_offsets[3]);
writel(reg125_val, base + pre_offsets[4]);
writel(v2_100_val, base + pre_offsets[5]);
writel(v2_10_val, base + pre_offsets[6]);
writel(v2_val, base + pre_offsets[7]);
}Then call this helper with proper offsets for each register block. This refactoring reduces duplication and nesting while keeping all functionality intact.
Summary by Sourcery
This pull request adds USB, IRP chip, and I2C support for CIX. It includes driver implementations and necessary configurations for these interfaces.
New Features: