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

[Martin Blumenstingl]

Hi,

On Wed, Jul 9, 2025 at 8:29 AM Xianwei Zhao <xianwei.zhao at amlogic.com> wrote:
>
> Hi Martin,
>     Thanks for your reply.
>
> On 2025/7/9 00:01, Martin Blumenstingl wrote:
> > [ EXTERNAL EMAIL ]
> >
> > 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])?
> >
>
> No, if add add flag CLK_DIVIDER_MAX_AT_ZERO, reg value will equals
> divider, see
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/clk-divider.c?h=next-20250708
> As follow in function _get_div.
>          "if (flags & CLK_DIVIDER_MAX_AT_ZERO)
>                  return val ? val : clk_div_mask(width) + 1;"
>
> But here reg value adding one equals divider.
I see, thanks.

> >> +       /* 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
>
> here I don't know parent fw_name, The system does not have a
> corresponding interface to obtain parent fw_name, only name (clk-core->name)
The .fw_name is simply "pclk" in this case.
That clock is then internally looked up (by the common clock
framework) using the "clock-names" and "clocks" properties from your
device-tree.

[...]
> >> +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.
> >
>
> regmap_config as a local variable, it can save space.
Thanks, I was not aware of that.
For documentation purposes, with the original approach:
$ aarch64-linux-gnu-size -d drivers/spi/spi-amlogic-spisg.ko
  text    data     bss     dec     hex filename
  7504    1377       0    8881    22b1 drivers/spi/spi-amlogic-spisg.ko

and making the regmap_config static const:
$ aarch64-linux-gnu-size -d drivers/spi/spi-amlogic-spisg.ko
  text    data     bss     dec     hex filename
  7716    1377       0    9093    2385 drivers/spi/spi-amlogic-spisg.ko

[...]
> >> +       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).
> >
>
> The reset might not exist for this node, see relevant yaml file.
This part I understand. Optional however doesn't mean that we can
simply ignore all errors.
Let's consider the following three scenarios:
1) reset not provided in .dtb -> we don't expect any error here
2) reset is provided in .dtb but the reset-controller has not been
registered -> spisg driver must propagate -EPROBE_DEFER
3) reset is provided in .dtb but reset_control_reset (which is used
internally in device_reset_optional) returns an error -> spisg must
propagate this error

Your code works for the first case but it doesn't consider the other two.
That said, I'm not sure if the third case is realistic given the T7
reset controller uses MMIO access. This may change in future though
(if SPISG IP is the same but the reset controller changes).
It still leaves the second case where the spisg driver should be
probed only after the reset controller is available.


Best regards,
Martin