On 21/07/16 21:42, Mirza Krak wrote:
> 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.
Should just be the inverse of the probe (although there is no inverse
for the parsing DT bit). If you don't wish to add a remove, that is
fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so
it cannot be configured as a module.
>>
>>> +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? :).
The patches would have to go via Thierry and so ultimately, Thierry.
However, I can't imagine he would object to GMI ;-)
Jon
--
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
