On Thu, Jul 20, 2023 at 02:29:07PM +0300, Nikita Shubin via B4 Relay wrote:
From: Nikita Shubin nikita.shubin@maquefel.me
This adds an SoC driver for the ep93xx. Currently there is only one thing not fitting into any other framework, and that is the swlock setting.
It's used for clock settings and restart.
+#include <linux/clk-provider.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/of.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <linux/sys_soc.h> +#include <linux/soc/cirrus/ep93xx.h>
...
+#define EP93XX_SYSCON_SYSCFG_REV_MASK 0xf0000000
GENMASK() (you will need bits.h, and looking below you seem missed it anyway)
...
- spin_lock_irqsave(&ep93xx_swlock, flags);
- regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
- val &= ~clear_bits;
- val |= set_bits;
- regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
- regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
Is this sequence a must? I.o.w. can you first supply magic and then update devcfg?
- spin_unlock_irqrestore(&ep93xx_swlock, flags);
...
+void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int reg,
unsigned int mask, unsigned int val)+{
Same Q as above.
+}
...
- int rev = ep93xx_chip_revision(map);
- switch (rev) {
switch(ep93xx_chip_revision(map))
?
- case EP93XX_CHIP_REV_D0:
return "D0";- case EP93XX_CHIP_REV_D1:
return "D1";- case EP93XX_CHIP_REV_E0:
return "E0";- case EP93XX_CHIP_REV_E1:
return "E1";- case EP93XX_CHIP_REV_E2:
return "E2";- default:
return "unknown";- }
...
+static unsigned long __init calc_pll_rate(u64 rate, u32 config_word) +{
- rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1; /* X1FBD */
- rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1; /* X2FBD */
- do_div(rate, (config_word & GENMASK(4, 0)) + 1); /* X2IPD */
- rate >>= ((config_word >> 16) & 3); /* PS */
GENMASK()
- return rate;
+}
...
- /* Multiplatform guard, only proceed on ep93xx */
- if (!of_machine_is_compatible("cirrus,ep9301"))
return 0;
Why?
- map = syscon_regmap_lookup_by_compatible("cirrus,ep9301-syscon");
- if (IS_ERR(map))
return PTR_ERR(map);
Is this not enough?
...
- if (!(value & EP93XX_SYSCON_CLKSET1_NBYP1))
clk_pll1_rate = EP93XX_EXT_CLK_RATE;- else
clk_pll1_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
Positive conditionals in if-else are easier to be read.
...
- clk_f_div = fclk_divisors[(value >> 25) & 0x7];
- clk_h_div = hclk_divisors[(value >> 20) & 0x7];
- clk_p_div = pclk_divisors[(value >> 18) & 0x3];
GENMASK() in all three?
...
- np = of_find_node_by_path("/");
- of_property_read_string(np, "model", &machine);
- if (machine)
attrs->machine = kstrdup(machine, GFP_KERNEL);
No error check?
- of_node_put(np);
Looks like NIH of_flat_dt_get_machine_name(). Can you rather make use of it as it's done, e.g., here arch/riscv/kernel/setup.c:251?