lkml.org 
[lkml]   [2021]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [RFC PATCH 6/9] memory: apple: Add apple-mcc driver to manage MCC perf in Apple SoCs
Date
On 12/10/2021 18.19, Krzysztof Kozlowski wrote:
>> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
>> +/*
>> + * Apple SoC MCC memory controller performance control driver
>> + *
>> + * Copyright The Asahi Linux Contributors
>
> Copyright date?

We've gone over this one a few times already; most copyright dates
quickly become outdated and meaningless :)

See:
https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/

>> +static int apple_mcc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *node = dev->of_node;
>
> By convention mostly we call the variable "np".

Ack, I'll change it for v2.

>> + mcc->reg_base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(mcc->reg_base))
>> + return PTR_ERR(mcc->reg_base);
>> +
>> + if (of_property_read_u32(node, "apple,num-channels", &mcc->num_channels)) {
>
> Don't you have a limit of supported channels? It cannot be any uint32...

Today, it's max 8. But if come Monday we find out Apple's new chips have
16 channels and otherwise the same register layout, I'd much rather not
have to change the driver...

>> + dev_err(dev, "missing apple,num-channels property\n");
>
> Use almost everywhere dev_err_probe - less code and you get error msg
> printed.

Heh, I didn't know about that one. Thanks!

>> +
>> + dev_info(dev, "Apple MCC performance driver initialized\n");
>
> Please skip it, or at least make it a dev_dbg, you don't print any
> valuable information here.

Ack, I'll remove this.

>> +static struct platform_driver apple_mcc_driver = {
>> + .probe = apple_mcc_probe,
>> + .driver = {
>> + .name = "apple-mcc",
>> + .of_match_table = apple_mcc_of_match,
>> + },
>> +};
>
> module_platform_driver() goes here.

Ack, will fix for v2.

>
>> +
>> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
>> +MODULE_DESCRIPTION("MCC memory controller performance tuning driver for Apple SoCs");
>> +MODULE_LICENSE("GPL v2");
>
> I think this will be "Dual MIT/GPL", based on your SPDX.

Ah, I didn't realize that was a valid option for MODULE_LICENSE. I guess
anything containing "GPL" works with EXPORT_SYMBOL_GPL?

Thanks for the review!

--
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

\
 
 \ /
  Last update: 2021-10-14 09:00    [W:0.473 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site