Message-ID: <daa732cb31d947c308513b535930c729.sboyd@kernel.org>
Date: Wed, 10 Apr 2024 20:22:48 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Linus Walleij <linus.walleij@...aro.org>, Michael Turquette <mturquette@...libre.com>, Philipp Zabel <p.zabel@...gutronix.de>, Rob Herring <robh@...nel.org>, Théo Lebrun <theo.lebrun@...tlin.com>
Cc: linux-mips@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, linux-gpio@...r.kernel.org, Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>, Gregory CLEMENT <gregory.clement@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>, Tawfik Bayouk <tawfik.bayouk@...ileye.com>, Théo Lebrun <theo.lebrun@...tlin.com>
Subject: Re: [PATCH 05/11] clk: eyeq: add driver

Quoting Théo Lebrun (2024-04-10 10:12:34)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 50af5fc7f570..1eb6e70977a3 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -218,6 +218,17 @@ config COMMON_CLK_EN7523
>           This driver provides the fixed clocks and gates present on Airoha
>           ARM silicon.
>  
> +config COMMON_CLK_EYEQ
> +       bool "Clock driver for the Mobileye EyeQ platform"
> +       depends on OF || COMPILE_TEST

The OF build dependency looks useless as we have the MACH_ dependency
below.

> +       depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST
> +       default MACH_EYEQ5 || MACH_EYEQ6H
> +       help
> +         This driver provides clocks found on Mobileye EyeQ5, EyeQ6L and Eye6H
> +         SoCs. Controllers live in shared register regions called OLB. Driver
> +         provides read-only PLLs, derived from the main crystal clock (which
> +         must be constant). It also exposes some divider clocks.
> +
>  config COMMON_CLK_FSL_FLEXSPI
>         tristate "Clock driver for FlexSPI on Layerscape SoCs"
>         depends on ARCH_LAYERSCAPE || COMPILE_TEST
> diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> new file mode 100644
> index 000000000000..bb2535010ae6
> --- /dev/null
> +++ b/drivers/clk/clk-eyeq.c
> @@ -0,0 +1,644 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PLL clock driver for the Mobileye EyeQ5, EyeQ6L and EyeQ6H platforms.
> + *
> + * This controller handles read-only PLLs, all derived from the same main
> + * crystal clock. It also exposes divider clocks, those are children to PLLs.
> + * Parent clock is expected to be constant. This driver's registers live in
> + * a shared region called OLB. Some PLLs are initialised early by of_clk_init().

Is OLB a different DT node? It sounds like maybe this is trying to jam a
driver into DT when the OLB node should be a #clock-cells node.

> + *
> + * We use eqc_ as prefix, as-in "EyeQ Clock", but way shorter.
> + *
> + * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
> + */
> +
> +#define pr_fmt(fmt) "clk-eyeq: " fmt
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/overflow.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include <dt-bindings/clock/mobileye,eyeq5-clk.h>
> +
> +#define EQC_MAX_DIV_COUNT              4
> +
> +/* In frac mode, it enables fractional noise canceling DAC. Else, no function. */
> +#define PCSR0_DAC_EN                   BIT(0)
> +/* Fractional or integer mode */
> +#define PCSR0_DSM_EN                   BIT(1)
> +#define PCSR0_PLL_EN                   BIT(2)
> +/* All clocks output held at 0 */
> +#define PCSR0_FOUTPOSTDIV_EN           BIT(3)
> +#define PCSR0_POST_DIV1                        GENMASK(6, 4)
> +#define PCSR0_POST_DIV2                        GENMASK(9, 7)
> +#define PCSR0_REF_DIV                  GENMASK(15, 10)
> +#define PCSR0_INTIN                    GENMASK(27, 16)
> +#define PCSR0_BYPASS                   BIT(28)
> +/* Bits 30..29 are reserved */
> +#define PCSR0_PLL_LOCKED               BIT(31)
> +
> +#define PCSR1_RESET                    BIT(0)
> +#define PCSR1_SSGC_DIV                 GENMASK(4, 1)
> +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
> +#define PCSR1_SPREAD                   GENMASK(9, 5)
> +#define PCSR1_DIS_SSCG                 BIT(10)
> +/* Down-spread or center-spread */
> +#define PCSR1_DOWN_SPREAD              BIT(11)
> +#define PCSR1_FRAC_IN                  GENMASK(31, 12)
> +
> +/*
> + * Driver might register clock provider from eqc_init() if PLLs are required
> + * early (before platform bus is ready). Store struct eqc_priv inside linked
> + * list to pass clock provider from eqc_init() to eqc_probe() and register
> + * remaining clocks from platform device probe.
> + *
> + * Clock provider is NOT created by eqc_init() if no early clock is required.
> + * Store as linked list because EyeQ6H has multiple clock controller instances.
> + * Matching is done based on devicetree node pointer.
> + */
> +static DEFINE_SPINLOCK(eqc_list_slock);
> +static LIST_HEAD(eqc_list);
> +
> +struct eqc_pll {
> +       unsigned int    index;
> +       const char      *name;
> +       u32             reg64;
> +};
> +
> +/*
> + * Divider clock. Divider is 2*(v+1), with v the register value.
> + * Min divider is 2, max is 2*(2^width).
> + */
> +struct eqc_div {
> +       unsigned int    index;
> +       const char      *name;
> +       unsigned int    parent;
> +       const char      *resource_name;
> +       u8              shift;
> +       u8              width;
> +};
> +
> +struct eqc_match_data {
> +       unsigned int            early_pll_count;
> +       const struct eqc_pll    *early_plls;
> +
> +       unsigned int            pll_count;
> +       const struct eqc_pll    *plls;
> +
> +       unsigned int            div_count;
> +       const struct eqc_div    *divs;
> +};
> +
> +struct eqc_priv {
> +       struct clk_hw_onecell_data      *cells;
> +       const struct eqc_match_data     *data;
> +       void __iomem                    *base_plls;
> +       struct device_node              *np;
> +       struct list_head                list;
> +};
> +
> +static int eqc_pll_parse_registers(u32 r0, u32 r1, unsigned long *mult,
> +                                  unsigned long *div, unsigned long *acc)
> +{
> +       if (r0 & PCSR0_BYPASS) {
> +               *mult = 1;
> +               *div = 1;
> +               *acc = 0;
> +               return 0;
> +       }
> +
> +       if (!(r0 & PCSR0_PLL_LOCKED))
> +               return -EINVAL;
> +
> +       *mult = FIELD_GET(PCSR0_INTIN, r0);
> +       *div = FIELD_GET(PCSR0_REF_DIV, r0);
> +       if (r0 & PCSR0_FOUTPOSTDIV_EN)
> +               *div *= FIELD_GET(PCSR0_POST_DIV1, r0) * FIELD_GET(PCSR0_POST_DIV2, r0);
> +
> +       /* Fractional mode, in 2^20 (0x100000) parts. */
> +       if (r0 & PCSR0_DSM_EN) {
> +               *div *= 0x100000;
> +               *mult = *mult * 0x100000 + FIELD_GET(PCSR1_FRAC_IN, r1);
> +       }
> +
> +       if (!*mult || !*div)
> +               return -EINVAL;
> +
> +       /* Spread spectrum. */
> +       if (!(r1 & (PCSR1_RESET | PCSR1_DIS_SSCG))) {
> +               /*
> +                * Spread is 1/1000 parts of frequency, accuracy is half of
> +                * that. To get accuracy, convert to ppb (parts per billion).
> +                */
> +               u32 spread = FIELD_GET(PCSR1_SPREAD, r1);
> +
> +               *acc = spread * 500000;
> +               if (r1 & PCSR1_DOWN_SPREAD) {
> +                       /*
> +                        * Downspreading: the central frequency is half a
> +                        * spread lower.
> +                        */
> +                       *mult *= 2000 - spread;
> +                       *div *= 2000;
> +               }
> +       } else {
> +               *acc = 0;
> +       }
> +
> +       return 0;
> +}
> +
> +static unsigned int eqc_compute_clock_count(const struct eqc_match_data *data)
> +{
> +       unsigned int i, nb_clks = 0;
> +
> +       for (i = 0; i < data->early_pll_count; i++)
> +               if (data->early_plls[i].index >= nb_clks)
> +                       nb_clks = data->early_plls[i].index + 1;
> +       for (i = 0; i < data->pll_count; i++)
> +               if (data->plls[i].index >= nb_clks)
> +                       nb_clks = data->plls[i].index + 1;
> +       for (i = 0; i < data->div_count; i++)
> +               if (data->divs[i].index >= nb_clks)
> +                       nb_clks = data->divs[i].index + 1;
> +
> +       /* We expect the biggest clock index to be 1 below the clock count. */
> +       WARN_ON(nb_clks != data->early_pll_count + data->pll_count + data->div_count);
> +
> +       return nb_clks;
> +}
> +
> +static int eqc_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       void __iomem *div_resources[EQC_MAX_DIV_COUNT];
> +       struct device_node *np = dev->of_node;
> +       const struct eqc_match_data *data;
> +       struct eqc_priv *priv = NULL;
> +       struct clk_hw *hw;
> +       unsigned int i;
> +
> +       data = device_get_match_data(dev);
> +       if (!data)
> +               return -ENODEV;
> +
> +       if (data->early_pll_count) {
> +               /* Device got inited early. Retrieve clock provider from list. */
> +               struct eqc_priv *entry;
> +
> +               spin_lock(&eqc_list_slock);
> +               list_for_each_entry(entry, &eqc_list, list) {
> +                       if (entry->np == np) {
> +                               priv = entry;
> +                               break;
> +                       }
> +               }
> +               spin_unlock(&eqc_list_slock);
> +
> +               if (!priv)
> +                       return -ENODEV;

This can be a sub-function.

> +       } else {
> +               /* Device did NOT get init early. Do it now. */
> +               unsigned int nb_clks;
> +
> +               priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +               if (!priv)
> +                       return -ENOMEM;
> +
> +               priv->np = np;
> +               priv->data = data;
> +
> +               nb_clks = eqc_compute_clock_count(data);
> +               priv->cells = devm_kzalloc(dev, struct_size(priv->cells, hws, nb_clks),
> +                                          GFP_KERNEL);
> +               if (!priv->cells)
> +                       return -ENOMEM;
> +
> +               priv->cells->num = nb_clks;
> +
> +               /*
> +                * We expect named resources if divider clocks are present.
> +                * Else, we only expect one resource.
> +                */
> +               if (data->div_count)
> +                       priv->base_plls = devm_platform_ioremap_resource_byname(pdev, "plls");
> +               else
> +                       priv->base_plls = devm_platform_ioremap_resource(pdev, 0);
> +               if (IS_ERR(priv->base_plls))
> +                       return PTR_ERR(priv->base_plls);
> +       }
> +
> +       for (i = 0; i < data->pll_count; i++) {
> +               const struct eqc_pll *pll = &data->plls[i];
> +               unsigned long mult, div, acc;
> +               u32 r0, r1;
> +               u64 val;
> +               int ret;

All variables should be declared at the start of the function. Once it
becomes "too heavy" you can split it up into smaller functions, that
again have all variables declared at the start of the function.

> +
> +               val = readq(priv->base_plls + pll->reg64);
> +               r0 = val;
> +               r1 = val >> 32;
> +
> +               ret = eqc_pll_parse_registers(r0, r1, &mult, &div, &acc);
> +               if (ret) {
> +                       dev_warn(dev, "failed parsing state of %s\n", pll->name);
> +                       priv->cells->hws[pll->index] = ERR_PTR(ret);
> +                       continue;
> +               }
> +
> +               hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev,
> +                               dev->of_node, pll->name, "ref", 0, mult, div, acc);
> +               priv->cells->hws[pll->index] = hw;
> +               if (IS_ERR(hw))
> +                       dev_warn(dev, "failed registering %s: %pe\n", pll->name, hw);
> +       }
> +
> +       BUG_ON(ARRAY_SIZE(div_resources) < data->div_count);

Can this be a static assert instead on the arrays these are based on?
Put some static_assert() near the match data macros.

> +
> +       for (i = 0; i < data->div_count; i++) {
> +               const struct eqc_div *div = &data->divs[i];
> +               void __iomem *base = NULL;
> +               struct clk_hw *parent;
> +               unsigned int j;
> +
> +               /*
> +                * Multiple divider clocks can request the same resource. Store
> +                * resource pointers during probe(). For each divider clock,
> +                * check if previous clocks referenced the same resource name.
> +                *
> +                * See EQ6HC_SOUTH_DIV_OSPI_REF and EQ6HC_SOUTH_DIV_OSPI_SYS.
> +                */
> +               for (j = 0; j < i; j++) {
> +                       if (strcmp(data->divs[j].resource_name, div->resource_name) == 0) {
> +                               base = div_resources[j];
> +                               break;
> +                       }
> +               }
> +
> +               /* Resource is first encountered. */
> +               if (!base) {
> +                       base = devm_platform_ioremap_resource_byname(pdev, div->resource_name);
> +                       if (IS_ERR(base)) {
> +                               dev_warn(dev, "failed to iomap resource for %s\n", div->name);
> +                               priv->cells->hws[div->index] = base;
> +                               continue;
> +                       }
> +               }

I don't get this code at all. The driver should simply map the
resources because it knows that there's an io resource. I'll look at the
binding which is probably wrong and causing the driver to be written
this way.

> +
> +               div_resources[i] = base;
> +
> +               parent = priv->cells->hws[div->parent];
> +               hw = clk_hw_register_divider_table_parent_hw(dev, div->name,
> +                               parent, 0, base, div->shift, div->width,
> +                               CLK_DIVIDER_EVEN_INTEGERS, NULL, NULL);
> +               priv->cells->hws[div->index] = hw;
> +               if (IS_ERR(hw))
> +                       dev_warn(dev, "failed registering %s: %pe\n",
> +                                div->name, hw);
> +       }
> +
> +       /* Clock provider has not been registered by eqc_init(). Do it now. */
> +       if (data->early_pll_count == 0) {
> +               /* When providing a single clock, require no cell. */
> +               if (priv->cells->num == 1)
> +                       return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> +                                                          priv->cells->hws);
> +               else
> +                       return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +                                                          priv->cells);
> +       }
> +
> +       return 0;
> +}
> +
> +/* Required early for GIC timer (pll-cpu) and UARTs (pll-per). */
> +static const struct eqc_pll eqc_eyeq5_early_plls[] = {
> +       { .index = EQ5C_PLL_CPU, .name = "pll-cpu",  .reg64 = 0x00, },
> +       { .index = EQ5C_PLL_PER, .name = "pll-per",  .reg64 = 0x30, },
> +};
> +
> +static const struct eqc_pll eqc_eyeq5_plls[] = {
> +       { .index = EQ5C_PLL_VMP,  .name = "pll-vmp",  .reg64 = 0x08, },
> +       { .index = EQ5C_PLL_PMA,  .name = "pll-pma",  .reg64 = 0x10, },
> +       { .index = EQ5C_PLL_VDI,  .name = "pll-vdi",  .reg64 = 0x18, },
> +       { .index = EQ5C_PLL_DDR0, .name = "pll-ddr0", .reg64 = 0x20, },
> +       { .index = EQ5C_PLL_PCI,  .name = "pll-pci",  .reg64 = 0x28, },
> +       { .index = EQ5C_PLL_PMAC, .name = "pll-pmac", .reg64 = 0x38, },
> +       { .index = EQ5C_PLL_MPC,  .name = "pll-mpc",  .reg64 = 0x40, },
> +       { .index = EQ5C_PLL_DDR1, .name = "pll-ddr1", .reg64 = 0x48, },
> +};
> +
> +static const struct eqc_div eqc_eyeq5_divs[] = {
> +       {
> +               .index = EQ5C_DIV_OSPI,
> +               .name = "div-ospi",
> +               .parent = EQ5C_PLL_PER,
> +               .resource_name = "ospi",
> +               .shift = 0,
> +               .width = 4,
> +       },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq5_match_data = {
> +       .early_pll_count        = ARRAY_SIZE(eqc_eyeq5_early_plls),
> +       .early_plls             = eqc_eyeq5_early_plls,
> +
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq5_plls),
> +       .plls           = eqc_eyeq5_plls,
> +
> +       .div_count      = ARRAY_SIZE(eqc_eyeq5_divs),
> +       .divs           = eqc_eyeq5_divs,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6l_plls[] = {
> +       { .index = EQ6LC_PLL_DDR, .name = "pll-ddr", .reg64 = 0x2C },
> +       { .index = EQ6LC_PLL_CPU, .name = "pll-cpu", .reg64 = 0x34 }, /* also acc */
> +       { .index = EQ6LC_PLL_PER, .name = "pll-per", .reg64 = 0x3C },
> +       { .index = EQ6LC_PLL_VDI, .name = "pll-vdi", .reg64 = 0x44 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6l_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6l_plls),
> +       .plls           = eqc_eyeq6l_plls,
> +};
> +
> +/* Required early for GIC timer. */
> +static const struct eqc_pll eqc_eyeq6h_central_early_plls[] = {
> +       { .index = 0, .name = "pll-cpu", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_central_match_data = {
> +       .early_pll_count        = ARRAY_SIZE(eqc_eyeq6h_central_early_plls),
> +       .early_plls             = eqc_eyeq6h_central_early_plls,
> +};
> +
> +/* Required early for UART. */
> +static const struct eqc_pll eqc_eyeq6h_west_early_plls[] = {
> +       { .index = 0, .name = "pll-west", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_west_match_data = {
> +       .early_pll_count        = ARRAY_SIZE(eqc_eyeq6h_west_early_plls),
> +       .early_plls             = eqc_eyeq6h_west_early_plls,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_east_plls[] = {
> +       { .index = 0, .name = "pll-east", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_east_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_east_plls),
> +       .plls           = eqc_eyeq6h_east_plls,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_south_plls[] = {
> +       { .index = EQ6HC_SOUTH_PLL_VDI,  .name = "pll-vdi",  .reg64 = 0x00 },
> +       { .index = EQ6HC_SOUTH_PLL_PCIE, .name = "pll-pcie", .reg64 = 0x08 },
> +       { .index = EQ6HC_SOUTH_PLL_PER,  .name = "pll-per",  .reg64 = 0x10 },
> +       { .index = EQ6HC_SOUTH_PLL_ISP,  .name = "pll-isp",  .reg64 = 0x18 },
> +};
> +
> +static const struct eqc_div eqc_eyeq6h_south_divs[] = {
> +       {
> +               .index = EQ6HC_SOUTH_DIV_EMMC,
> +               .name = "div-emmc",
> +               .parent = EQ6HC_SOUTH_PLL_PER,
> +               .resource_name = "emmc",
> +               .shift = 4,
> +               .width = 4,
> +       },
> +       {
> +               .index = EQ6HC_SOUTH_DIV_OSPI_REF,
> +               .name = "div-ospi-ref",
> +               .parent = EQ6HC_SOUTH_PLL_PER,
> +               .resource_name = "ospi",
> +               .shift = 4,
> +               .width = 4,
> +       },
> +       {
> +               .index = EQ6HC_SOUTH_DIV_OSPI_SYS,
> +               .name = "div-ospi-sys",
> +               .parent = EQ6HC_SOUTH_PLL_PER,
> +               .resource_name = "ospi",
> +               .shift = 8,
> +               .width = 1,
> +       },
> +       {
> +               .index = EQ6HC_SOUTH_DIV_TSU,
> +               .name = "div-tsu",
> +               .parent = EQ6HC_SOUTH_PLL_PCIE,
> +               .resource_name = "tsu",
> +               .shift = 4,
> +               .width = 8,
> +       },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_south_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_south_plls),
> +       .plls           = eqc_eyeq6h_south_plls,
> +
> +       .div_count      = ARRAY_SIZE(eqc_eyeq6h_south_divs),
> +       .divs           = eqc_eyeq6h_south_divs,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_ddr0_plls[] = {
> +       { .index = 0, .name = "pll-ddr0", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_ddr0_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_ddr0_plls),
> +       .plls           = eqc_eyeq6h_ddr0_plls,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_ddr1_plls[] = {
> +       { .index = 0, .name = "pll-ddr1", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_ddr1_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_ddr1_plls),
> +       .plls           = eqc_eyeq6h_ddr1_plls,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_acc_plls[] = {
> +       { .index = EQ6HC_ACC_PLL_XNN, .name = "pll-xnn", .reg64 = 0x00 },
> +       { .index = EQ6HC_ACC_PLL_VMP, .name = "pll-vmp", .reg64 = 0x10 },
> +       { .index = EQ6HC_ACC_PLL_PMA, .name = "pll-pma", .reg64 = 0x1C },
> +       { .index = EQ6HC_ACC_PLL_MPC, .name = "pll-mpc", .reg64 = 0x28 },
> +       { .index = EQ6HC_ACC_PLL_NOC, .name = "pll-noc", .reg64 = 0x30 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_acc_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_acc_plls),
> +       .plls           = eqc_eyeq6h_acc_plls,
> +};
> +
> +static const struct of_device_id eqc_match_table[] = {
> +       { .compatible = "mobileye,eyeq5-clk", .data = &eqc_eyeq5_match_data },
> +       { .compatible = "mobileye,eyeq6l-clk", .data = &eqc_eyeq6l_match_data },
> +       { .compatible = "mobileye,eyeq6h-central-clk", .data = &eqc_eyeq6h_central_match_data },
> +       { .compatible = "mobileye,eyeq6h-west-clk", .data = &eqc_eyeq6h_west_match_data },
> +       { .compatible = "mobileye,eyeq6h-east-clk", .data = &eqc_eyeq6h_east_match_data },
> +       { .compatible = "mobileye,eyeq6h-south-clk", .data = &eqc_eyeq6h_south_match_data },
> +       { .compatible = "mobileye,eyeq6h-ddr0-clk", .data = &eqc_eyeq6h_ddr0_match_data },
> +       { .compatible = "mobileye,eyeq6h-ddr1-clk", .data = &eqc_eyeq6h_ddr1_match_data },
> +       { .compatible = "mobileye,eyeq6h-acc-clk", .data = &eqc_eyeq6h_acc_match_data },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, eqc_match_table);
> +
> +static struct platform_driver eqc_driver = {
> +       .probe = eqc_probe,
> +       .driver = {
> +               .name = "clk-eyeq",
> +               .of_match_table = eqc_match_table,
> +       },
> +};
> +builtin_platform_driver(eqc_driver);
> +
> +static void __init eqc_init(struct device_node *np)
> +{
> +       const struct eqc_match_data *data;
> +       unsigned int nb_clks = 0;
> +       struct eqc_priv *priv;
> +       unsigned int i;
> +       int ret;
> +
> +       data = of_match_node(eqc_match_table, np)->data;
> +
> +       /* No reason to early init this clock provider. Do it at probe. */
> +       if (data->early_pll_count == 0)

You can have a different match table for this function then.

> +               return;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       priv->np = np;
> +       priv->data = data;
> +
> +       nb_clks = eqc_compute_clock_count(data);
> +       priv->cells = kzalloc(struct_size(priv->cells, hws, nb_clks), GFP_KERNEL);
> +       if (!priv->cells) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       priv->cells->num = nb_clks;
> +
> +       /*
> +        * Mark non-early clocks as deferred; they'll be registered at platform
> +        * device probe.
> +        */
> +       for (i = 0; i < data->pll_count; i++)
> +               priv->cells->hws[data->plls[i].index] = ERR_PTR(-EPROBE_DEFER);
> +       for (i = 0; i < data->div_count; i++)
> +               priv->cells->hws[data->divs[i].index] = ERR_PTR(-EPROBE_DEFER);
> +
> +       /*
> +        * We expect named resources if divider clocks are present.
> +        * Else, we only expect one resource.
> +        */

Please avoid named resources. They give the false sense of hope that the
binding can re-order the reg property when that can't be done. Instead,
just index and know which index to use in the driver.

Powered by blists - more mailing lists