On Tue, Oct 6, 2015 at 4:35 AM, Stanimir Varbanov <svarbanov@xxxxxxxxxx> wrote:
> Hi Rob,
>
> <snip>
>
>> +
>> +struct ocmem_config {
>> + uint8_t num_regions;
>
> u8
>
>> + uint32_t macro_size;
>
> u32
tbh, only place where stdint types are potentially an issue is in
uapi. I changed the qcom-scm patches to use uN types instead since
the rest of the file was already using those (and when in Rome..)
but otherwise I prefer stdint types, since they are, well, standard..
>
>> +static const struct ocmem_config ocmem_8974_config = {
>> + .num_regions = 3, .macro_size = SZ_128K,
>> +};
>> +
>> +static const struct of_device_id ocmem_dt_match[] = {
>> + { .compatible = "qcom,ocmem-msm8974", .data = &ocmem_8974_config },
>
> Just a suggestion. As we have REG_OCMEM_HW_PROFILE and
> REG_OCMEM_HW_VERSION register why not distinguish by version and hw
> profile instead of SoC?
I take what I can from HW_PROFILE reg, but afaict macro_size and
num_regions is missing..
>> + {}
>> +};
>> +
>> +static int ocmem_dev_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + const struct ocmem_config *config = NULL;
>> + const struct of_device_id *match;
>> + struct resource *res;
>> + uint32_t reg, num_banks, region_size;
>> + int i, j, ret;
>> +
>> + /* we need scm to be available: */
>> + if (!qcom_scm_is_available())
>> + return -EPROBE_DEFER;
>> +
>> + match = of_match_device(ocmem_dt_match, dev);
>> + if (match)
>> + config = match->data;
>> +
>> + if (!config) {
>> + dev_err(dev, "unknown config: %s\n", dev->of_node->name);
>> + return -ENXIO;
>> + }
>> +
>> + ocmem = devm_kzalloc(dev, sizeof(*ocmem), GFP_KERNEL);
>> + if (!ocmem)
>> + return -ENOMEM;
>> +
>> + ocmem->dev = dev;
>> + ocmem->config = config;
>> +
>> + ocmem->core_clk = devm_clk_get(dev, "core_clk");
>> + if (IS_ERR(ocmem->core_clk)) {
>> + dev_err(dev, "Unable to get the core clock\n");
>
> this dev_err will flood console in EPROBE_DEFER case, IMO should be removed.
It is kind of nice to have some hints when debugging probe-defer or
fail to load cases.. I guess I could change that to dev_info() for the
probe-defer case..
>> + return PTR_ERR(ocmem->core_clk);
>> + }
>> +
>> + ocmem->iface_clk = devm_clk_get(dev, "iface_clk");
>> + if (IS_ERR_OR_NULL(ocmem->iface_clk)) {
>> + ret = PTR_ERR(ocmem->iface_clk);
>> + ocmem->iface_clk = NULL;
>> + /* in probe-defer case, propagate error up and try again later: */
>> + if (ret == -EPROBE_DEFER)
>> + goto fail;
>
> Why error path of the iface clock is different comparing with core clk.
I guess there are some cases where there is no iface_clk? This is
just based on what downstream driver was doing, which is the only
documentation I have ;-)
If someone knows better, I'd be happy to change it..
> Also I failed to found a case when common clk framework will return NULL?
>
> if (IS_ERR(ocmem->iface_clk)
> return PTR_ERR(ocmem->iface_clk);
>
> this should be enough.
ok
>> + }
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> + "ocmem_ctrl_physical");
>> + if (!res) {
>> + dev_err(&pdev->dev, "failed to get memory resource\n");
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> +
>> + ocmem->mmio = devm_ioremap_nocache(&pdev->dev, res->start,
>> + resource_size(res));
>
> I think you could use following to simplify ioremap:
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "ocmem_ctrl_physical");
> ocmem->mmio = devm_ioremap_resource(dev, res);
> if (IS_ERR(ocmem->mmio))
> return PTR_ERR(ocmem->mmio);
sure
BR,
-R
>> + if (IS_ERR(ocmem->mmio)) {
>> + dev_err(&pdev->dev, "failed to ioremap memory resource\n");
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> +
>> + ocmem->ocmem_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> + "ocmem_physical");
>> + if (!ocmem->ocmem_mem) {
>> + dev_err(dev, "could not get OCMEM region\n");
>> + return -ENXIO;
>> + }
>> +
>
> <snip>
>
> --
> regards,
> Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
