On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
[off-list ref] wrote:

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into multiple slices and each
slice gets its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
slice they get and activate or deactivate the slice as needed. LLCC driver
provides API for the clients to perform these operations.

+static const struct of_device_id sdm845_qcom_llcc_of_match[] = {
+       { .compatible = "qcom,sdm845-llcc", },

+       { },

Slightly better w/o comma

+};

+static struct platform_driver sdm845_qcom_llcc_driver = {
+       .driver = {
+               .name = "sdm845-llcc",

+               .owner = THIS_MODULE,

No need. See below.

+               .of_match_table = sdm845_qcom_llcc_of_match,
+       },
+       .probe = sdm845_qcom_llcc_probe,
+};

+
+static int __init sdm845_init_qcom_llcc_init(void)
+{
+       return platform_driver_register(&sdm845_qcom_llcc_driver);
+}
+module_init(sdm845_init_qcom_llcc_init);
+
+static void __exit sdm845_exit_qcom_llcc_exit(void)
+{
+       platform_driver_unregister(&sdm845_qcom_llcc_driver);
+}
+module_exit(sdm845_exit_qcom_llcc_exit);

Why not to use module_platform_driver() macro?

+#define ACTIVATE                      0x1
+#define DEACTIVATE                    0x2
+#define ACT_CTRL_OPCODE_ACTIVATE      0x1
+#define ACT_CTRL_OPCODE_DEACTIVATE    0x2
+#define ACT_CTRL_ACT_TRIG             0x1

Are these bits? Perhaps BIT() ?

+#define ACT_CTRL_OPCODE_SHIFT         0x1
+#define ATTR1_PROBE_TARGET_WAYS_SHIFT 0x2
+#define ATTR1_FIXED_SIZE_SHIFT        0x3
+#define ATTR1_PRIORITY_SHIFT          0x4
+#define ATTR1_MAX_CAP_SHIFT           0x10

Better to use fixed size pattern, i.e. 0x01, 0x02, 0x03, 0x04, 0x10.

+#define ATTR0_RES_WAYS_MASK           0x00000fff
+#define ATTR0_BONUS_WAYS_MASK         0x0fff0000

GENMASK()

+#define LLCC_LB_CNT_MASK               0xf0000000

Ditto.

+#define MAX_CAP_TO_BYTES(n) (n * 1024)

(n * SZ_1K) ?

+#define LLCC_TRP_ACT_CTRLn(n) (n * 0x1000)

SZ_4K ?

+#define LLCC_TRP_STATUSn(n)   (4 + n * 0x1000)

Ditto.

+struct llcc_slice_desc *llcc_slice_getd(u32 uid)
+{
+       const struct llcc_slice_config *cfg;
+       struct llcc_slice_desc *desc;
+       u32 sz, count = 0;
+
+       cfg = drv_data->cfg;
+       sz = drv_data->cfg_size;
+

+       while (cfg && count < sz) {
+               if (cfg->usecase_id == uid)
+                       break;
+               cfg++;
+               count++;
+       }
+       if (cfg == NULL || count == sz)
+               return ERR_PTR(-ENODEV);

if (!cfg)
          return ERR_PTR(-ENODEV);

while (cfg->... != uid) {
  cfg++;
  count++;
}

if (count == sz)
 return ...

Though I would rather put it to for () loop.

+static int llcc_update_act_ctrl(u32 sid,
+                               u32 act_ctrl_reg_val, u32 status)
+{
+       u32 act_ctrl_reg;
+       u32 status_reg;
+       u32 slice_status;

+       int ret = 0;

Useless assignment. Check entire patch series for a such.

+       ret = regmap_read_poll_timeout(drv_data->regmap, status_reg,
+       slice_status, !(slice_status & status), 0, LLCC_STATUS_READ_DELAY);

Wrong indentation.

+       return ret;
+}

+       ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+                                 DEACTIVATE);

Perhaps one line (~83 characters here is OK) ?

+       ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+                                 ACTIVATE);

Ditto.

+               attr1_cfg = bcast_off +
+                               LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);
+               attr0_cfg = bcast_off +
+                               LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);

Ditto.

+               attr1_val |= llcc_table[i].probe_target_ways <<
+                               ATTR1_PROBE_TARGET_WAYS_SHIFT;
+               attr1_val |= llcc_table[i].fixed_size <<
+                               ATTR1_FIXED_SIZE_SHIFT;
+               attr1_val |= llcc_table[i].priority << ATTR1_PRIORITY_SHIFT;

foo |=
  bar << SHIFT;

would look slightly better.

+int qcom_llcc_probe(struct platform_device *pdev,
+                     const struct llcc_slice_config *llcc_cfg, u32 sz)
+{

+       drv_data->offsets = devm_kzalloc(dev, num_banks * sizeof(u32),
+                                                       GFP_KERNEL);
+       if (!drv_data->offsets)
+               return -ENOMEM;

devm_kcalloc() ?

+
+       for (i = 0; i < num_banks; i++)
+               drv_data->offsets[i] = (i * BANK_OFFSET_STRIDE);

Pointless parens.

+       drv_data->bitmap = devm_kcalloc(dev,
+       BITS_TO_LONGS(drv_data->max_slices), sizeof(unsigned long),
+                                               GFP_KERNEL);
+       if (!drv_data->bitmap)
+               return -ENOMEM;

Perhaps at some point someone will add
bitmap_alloc()
devm_bitmap_alloc()

+       bitmap_zero(drv_data->bitmap, drv_data->max_slices);

Pointless

-- 
With Best Regards,
Andy Shevchenko