linux-kernel - Re: [PATCH v1 2/3] clk: starfive: Add StarFive JH7110 PLL clock 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: <5e4007b7-6522-4c81-ca15-15a98c586aad@starfivetech.com>
Date:   Thu, 23 Feb 2023 17:32:58 +0800
From:   Xingyu Wu <xingyu.wu@...rfivetech.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        <linux-riscv@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        "Michael Turquette" <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Emil Renner Berthing <kernel@...il.dk>
CC:     Rob Herring <robh+dt@...nel.org>, Conor Dooley <conor@...nel.org>,
        "Paul Walmsley" <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Hal Feng <hal.feng@...rfivetech.com>,
        <linux-kernel@...r.kernel.org>, <linux-clk@...r.kernel.org>
Subject: Re: [PATCH v1 2/3] clk: starfive: Add StarFive JH7110 PLL clock
 driver

On 2023/2/23 16:56, Krzysztof Kozlowski wrote:
> On 21/02/2023 15:11, Xingyu Wu wrote:
>> Add driver for the StarFive JH7110 PLL clock controller and
>> modify the JH7110 system clock driver to rely on this PLL clocks.
>> 
>> Signed-off-by: Xingyu Wu <xingyu.wu@...rfivetech.com>
>> ---
> 
> 
>> +
>> +static int jh7110_pll_clk_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct of_phandle_args args;
>> +	struct regmap *pll_syscon_regmap;
>> +	unsigned int idx;
>> +	struct jh7110_clk_pll_priv *priv;
>> +	struct jh7110_clk_pll_data *data;
>> +	char *pll_name[JH7110_PLLCLK_END] = {
>> +		"pll0_out",
>> +		"pll1_out",
>> +		"pll2_out"
>> +	};
>> +
>> +	priv = devm_kzalloc(&pdev->dev,
>> +			    struct_size(priv, data, JH7110_PLLCLK_END),
>> +			    GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->dev = &pdev->dev;
>> +	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node, "starfive,sysreg", 0, 0, &args);
> 
> 1. Wrong wrapping. Wrap code at 80 as coding style asks.
> 
> 2. Why you are using syscon for normal, device MMIO operation? Your DTS
> also points that this is incorrect, hacky representation of hardware.
> Don't add devices to DT to fake places and then overuse syscon to fix
> that fake placement. The clock is in system registers, thus it must be
> there.
> 
> 3. Even if this stays, why so complicated code instead of
> syscon_regmap_lookup_by_phandle()?
> 

Thanks for your advice. Will use syscon_regmap_lookup_by_phandle instead it
and remove useless part.

> 
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to parse starfive,sys-syscon : %d\n", ret);
> 
> dev_err_probe and in other places as well, if applicable.

Will drop.

Best regards,
Xingyu Wu