[PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Martin Blumenstingl]

Hi,

On Fri, Jul 4, 2025 at 5:07 AM Xianwei Zhao via B4 Relay
<devnull+xianwei.zhao.amlogic.com at kernel.org> 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/