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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFBinCB4Lw04StL-kPpzKHPyETKfi5FYFipHRBDDnPdtRVDrmA@mail.gmail.com>
Date: Tue, 8 Jul 2025 18:01:45 +0200
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: xianwei.zhao@...ogic.com
Cc: Sunny Luo <sunny.luo@...ogic.com>, Mark Brown <broonie@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	linux-amlogic@...ts.infradead.org, linux-spi@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

Hi,

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