Hi,

On Fri, Jul 4, 2025 at 5:07 AM Xianwei Zhao via B4 Relay
[off-list ref] wrote:
[...]

+       div->table = tbl;
+
+       /* Register value should not be outside of the table */
+       regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV,
+                          FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1));

Are you doing this to prevent errors for value zero?
If so, is CLK_DIVIDER_MAX_AT_ZERO applicable instead (it has been
discussed for the t7 clock controller recently: [0])?

+       /* Register clk-divider */
+       parent_names[0] = __clk_get_name(spisg->pclk);

Instead of using __clk_get_name my suggestion is to use struct
clk_parent_data with .fw_name set.
If you want to simplify the code further you can use helper macros
like CLK_HW_INIT_FW_NAME

+       snprintf(name, sizeof(name), "%s_div", dev_name(dev));
+       init.name = name;
+       init.ops = &clk_divider_ops;
+       init.flags = CLK_SET_RATE_PARENT;
+       init.parent_names = parent_names;
+       init.num_parents = 1;
+       div->hw.init = &init;
+
+       spisg->sclk = devm_clk_register(dev, &div->hw);

My understanding is that devm_clk_register() is not recommended for new drivers.
The replacement is to use devm_clk_hw_register() first, then
devm_clk_hw_get_clk(). drivers/pwm/pwm-meson.c implements this in case
you're looking for an example


[...]

+static int aml_spisg_probe(struct platform_device *pdev)
+{
+       struct spi_controller *ctlr;
+       struct spisg_device *spisg;
+       struct device *dev = &pdev->dev;
+       void __iomem *base;
+       int ret, irq;
+
+       const struct regmap_config aml_regmap_config = {
+               .reg_bits = 32,
+               .val_bits = 32,
+               .reg_stride = 4,
+               .max_register = SPISG_MAX_REG,
+       };

Most regmap_configs in Amlogic drivers are static const.
If you make it a static const then I suggest renaming the variable to
aml_spisg_regmap_config for consistency.

[...]

+       device_reset_optional(dev);

I haven't checked the reset code but I think the return value still
needs to be checked (drivers/mmc/host/meson-gx-mmc.c does so).
Even though the reset is optional there's conditions where we must act
for example on -EPROBE_DEFER (which must be propagated).


Best regards,
Martin


[0] https://lore.kernel.org/linux-amlogic/bd68352f-7f8c-4cbc-9f4f-f83645cc1f70@amlogic.com/ (local)