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

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

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@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.

>> +       /* 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)

> 
>> +       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
> 

Will do.

> 
> [...]
>> +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.

> [...]
>> +       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.

> 
> Best regards,
> Martin
> 
> 
> [0] https://lore.kernel.org/linux-amlogic/bd68352f-7f8c-4cbc-9f4f-f83645cc1f70@amlogic.com/

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic