2016-07-21 12:15 GMT+02:00 Jon Hunter <jonathanh@xxxxxxxxxx>:
>
> On 19/07/16 14:36, Mirza Krak wrote:
>> From: Mirza Krak <mirza.krak@xxxxxxxxx>
>> +config TEGRA_NOR
>> + tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR"
>> + depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC
>> + depends on OF
>> + help
>> + Driver for NOR flash bus found on Tegra30/20 SOC`s.
>
> It is actually present on all Tegra's and so I would drop the 30/20.
ACK.
>> +++ b/drivers/bus/tegra-nor.c
>> @@ -0,0 +1,118 @@
>> +/*
>> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI.
>> + *
>> + * Copyright (C) 2016 Host Mobility AB. All rights reserved.
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/syscon.h>
>
> Is this needed?
It is not. ACK.
>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>
> Or this?
It is not. ACK.
>
>> +
>> +#define TEGRA_NOR_TIMING_REG_COUNT 2
>> +
>> +#define TEGRA_NOR_CONFIG 0x00
>> +#define TEGRA_NOR_STATUS 0x04
>> +#define TEGRA_NOR_ADDR_PTR 0x08
>> +#define TEGRA_NOR_AHB_ADDR_PTR 0x0c
>> +#define TEGRA_NOR_TIMING0 0x10
>> +#define TEGRA_NOR_TIMING1 0x14
>> +#define TEGRA_NOR_MIO_CONFIG 0x18
>> +#define TEGRA_NOR_MIO_TIMING 0x1c
>> +#define TEGRA_NOR_DMA_CONFIG 0x20
>> +#define TEGRA_NOR_CS_MUX_CONFIG 0x24
>
> Not all of these are used. It is good to define them and I wonder if we
> should add support for MIO while are at it :-)
We can come back to this once I have all SNOR stuff in order.
>
>> +
>> +#define TEGRA_NOR_CONFIG_GO BIT(31)
>> +
>> +static const struct of_device_id nor_id_table[] = {
>> + /* Tegra30 */
>
> I don't think this comment is needed.
ACK.
>
>> + { .compatible = "nvidia,tegra30-nor", .data = NULL, },
>
> You don't need to set data to NULL.
ACK.
>
>> + /* Tegra20 */
>> + { .compatible = "nvidia,tegra20-nor", .data = NULL, },
>
> Same here.
ACK
>> +static int __init nor_probe(struct platform_device *pdev)
>> +{
>
> I would drop the __init.
ACK.
>> +static struct platform_driver nor_driver = {
>> + .driver = {
>> + .name = "tegra-nor",
>> + .of_match_table = nor_id_table,
>> + },
>> +};
>
> The driver should have a remove function given that we can build as a
> module.
At the moment I do not know what we would do in a remove function and
hence me not adding one.
>
>> +module_platform_driver_probe(nor_driver, nor_probe);
>
> I would use "tegra_nor" namespace for all the structs, functions, etc.
> However, we may prefer to go with GMI and in which case tegra_gmi_probe,
> etc.
ACK. Who gets the last call on what we should call the driver? It
seems that we both think GMI is a better name, do we need a third? :).
Thank you Jonathan for your feedback.
Best Regards,
Mirza
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
