Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Carlo Caione 
On 20/05/16 14:20, Neil Armstrong wrote:
> On 05/20/2016 12:04 PM, Carlo Caione wrote:
> > On 20/05/16 11:10, Neil Armstrong wrote:
> >> On 05/20/2016 11:04 AM, Carlo Caione wrote:
> >>> On 20/05/16 10:27, Neil Armstrong wrote:
>  This patch adds the platform driver for the Amlogic Meson GXBB Reset
>  Controller.
> 
>  Signed-off-by: Neil Armstrong 
>  ---
>   drivers/reset/Kconfig|   6 ++
>   drivers/reset/Makefile   |   1 +
>   drivers/reset/reset-meson-gxbb.c | 129 
>  +++
>   3 files changed, 136 insertions(+)
>   create mode 100644 drivers/reset/reset-meson-gxbb.c
> >>>
> >>> Do we really need to be that specific (-gxbb)? This driver looks generic
> >>> and simple enough to be used for several Amlogic families. You are
> >>> already differentiating between them with the include file defining the
> >>> reset indexes for the SoC.
> >>
> >> This is a good question, do the S805 have similar registers ? Same count 
> >> and width ?
> >> I no, it should need a rework to add a data structure per-SoC.
> > 
> > According to the datasheet on S805 we have 7 registers with 16 reset
> > bits per register.
> 
> It will fit, I made the change to be generic. Do you know if the meson8 and 
> previous meson6 has these registers ?

No idea. No datasheet for those.

Cheers,

-- 
Carlo Caione

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Carlo Caione 
On 20/05/16 14:20, Neil Armstrong wrote:
> On 05/20/2016 12:04 PM, Carlo Caione wrote:
> > On 20/05/16 11:10, Neil Armstrong wrote:
> >> On 05/20/2016 11:04 AM, Carlo Caione wrote:
> >>> On 20/05/16 10:27, Neil Armstrong wrote:
>  This patch adds the platform driver for the Amlogic Meson GXBB Reset
>  Controller.
> 
>  Signed-off-by: Neil Armstrong 
>  ---
>   drivers/reset/Kconfig|   6 ++
>   drivers/reset/Makefile   |   1 +
>   drivers/reset/reset-meson-gxbb.c | 129 
>  +++
>   3 files changed, 136 insertions(+)
>   create mode 100644 drivers/reset/reset-meson-gxbb.c
> >>>
> >>> Do we really need to be that specific (-gxbb)? This driver looks generic
> >>> and simple enough to be used for several Amlogic families. You are
> >>> already differentiating between them with the include file defining the
> >>> reset indexes for the SoC.
> >>
> >> This is a good question, do the S805 have similar registers ? Same count 
> >> and width ?
> >> I no, it should need a rework to add a data structure per-SoC.
> > 
> > According to the datasheet on S805 we have 7 registers with 16 reset
> > bits per register.
> 
> It will fit, I made the change to be generic. Do you know if the meson8 and 
> previous meson6 has these registers ?

No idea. No datasheet for those.

Cheers,

-- 
Carlo Caione

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Neil Armstrong 
On 05/20/2016 12:04 PM, Carlo Caione wrote:
> On 20/05/16 11:10, Neil Armstrong wrote:
>> On 05/20/2016 11:04 AM, Carlo Caione wrote:
>>> On 20/05/16 10:27, Neil Armstrong wrote:
 This patch adds the platform driver for the Amlogic Meson GXBB Reset
 Controller.

 Signed-off-by: Neil Armstrong 
 ---
  drivers/reset/Kconfig|   6 ++
  drivers/reset/Makefile   |   1 +
  drivers/reset/reset-meson-gxbb.c | 129 
 +++
  3 files changed, 136 insertions(+)
  create mode 100644 drivers/reset/reset-meson-gxbb.c
>>>
>>> Do we really need to be that specific (-gxbb)? This driver looks generic
>>> and simple enough to be used for several Amlogic families. You are
>>> already differentiating between them with the include file defining the
>>> reset indexes for the SoC.
>>
>> This is a good question, do the S805 have similar registers ? Same count and 
>> width ?
>> I no, it should need a rework to add a data structure per-SoC.
> 
> According to the datasheet on S805 we have 7 registers with 16 reset
> bits per register.

It will fit, I made the change to be generic. Do you know if the meson8 and 
previous meson6 has these registers ?

Neil

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Neil Armstrong 
On 05/20/2016 12:04 PM, Carlo Caione wrote:
> On 20/05/16 11:10, Neil Armstrong wrote:
>> On 05/20/2016 11:04 AM, Carlo Caione wrote:
>>> On 20/05/16 10:27, Neil Armstrong wrote:
 This patch adds the platform driver for the Amlogic Meson GXBB Reset
 Controller.

 Signed-off-by: Neil Armstrong 
 ---
  drivers/reset/Kconfig|   6 ++
  drivers/reset/Makefile   |   1 +
  drivers/reset/reset-meson-gxbb.c | 129 
 +++
  3 files changed, 136 insertions(+)
  create mode 100644 drivers/reset/reset-meson-gxbb.c
>>>
>>> Do we really need to be that specific (-gxbb)? This driver looks generic
>>> and simple enough to be used for several Amlogic families. You are
>>> already differentiating between them with the include file defining the
>>> reset indexes for the SoC.
>>
>> This is a good question, do the S805 have similar registers ? Same count and 
>> width ?
>> I no, it should need a rework to add a data structure per-SoC.
> 
> According to the datasheet on S805 we have 7 registers with 16 reset
> bits per register.

It will fit, I made the change to be generic. Do you know if the meson8 and 
previous meson6 has these registers ?

Neil

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Neil Armstrong 
Hi Philipp,

On 05/20/2016 11:27 AM, Philipp Zabel wrote:
> Hi Neil,
> 
> Am Freitag, den 20.05.2016, 10:27 +0200 schrieb Neil Armstrong:
>> +config MESON_GXBB_RESET
>> +tristate "Amlogic Meson GXBB Reset Driver"
>> +depends on (ARCH_MESON && RESET_CONTROLLER)
> 
> With the "reset: fix Kconfig menu to include reset drivers in sub-menu"
> patch [1] applied this is wrapped in "if RESET_CONTROLLER ... endif", so
> no need to depend on RESET_CONTROLLER.
> Is there a reason to have this configurable at all, though?
> 
> [1] https://patchwork.kernel.org/patch/9000591/
> 
> Or drop CONFIG_MESON_GXBB_RESET and use CONFIG_ARCH_MESON directly.

No strong reason, I will switch to CONFIG_ARCH_MESON.

> [...]
> 
> With the "reset: add devm_reset_controller_register API" patch [2]
> applied you can use devm_reset_controller_register() here and remove the
> meson_gxbb_reset_remove() function below.
> 
> [2] https://patchwork.kernel.org/patch/8988471/
> 
>> +}
>> +
>> +static int meson_gxbb_reset_remove(struct platform_device *pdev)
>> +{
>> +struct meson_gxbb_reset *data = platform_get_drvdata(pdev);
>> +
>> +reset_controller_unregister(>rcdev);
>> +
>> +return 0;
>> +}
> 
> Could be removed, then.
> 
> regards
> Philipp

It will be for sure !

Neil

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Neil Armstrong 
Hi Philipp,

On 05/20/2016 11:27 AM, Philipp Zabel wrote:
> Hi Neil,
> 
> Am Freitag, den 20.05.2016, 10:27 +0200 schrieb Neil Armstrong:
>> +config MESON_GXBB_RESET
>> +tristate "Amlogic Meson GXBB Reset Driver"
>> +depends on (ARCH_MESON && RESET_CONTROLLER)
> 
> With the "reset: fix Kconfig menu to include reset drivers in sub-menu"
> patch [1] applied this is wrapped in "if RESET_CONTROLLER ... endif", so
> no need to depend on RESET_CONTROLLER.
> Is there a reason to have this configurable at all, though?
> 
> [1] https://patchwork.kernel.org/patch/9000591/
> 
> Or drop CONFIG_MESON_GXBB_RESET and use CONFIG_ARCH_MESON directly.

No strong reason, I will switch to CONFIG_ARCH_MESON.

> [...]
> 
> With the "reset: add devm_reset_controller_register API" patch [2]
> applied you can use devm_reset_controller_register() here and remove the
> meson_gxbb_reset_remove() function below.
> 
> [2] https://patchwork.kernel.org/patch/8988471/
> 
>> +}
>> +
>> +static int meson_gxbb_reset_remove(struct platform_device *pdev)
>> +{
>> +struct meson_gxbb_reset *data = platform_get_drvdata(pdev);
>> +
>> +reset_controller_unregister(>rcdev);
>> +
>> +return 0;
>> +}
> 
> Could be removed, then.
> 
> regards
> Philipp

It will be for sure !

Neil

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Carlo Caione 
On 20/05/16 11:10, Neil Armstrong wrote:
> On 05/20/2016 11:04 AM, Carlo Caione wrote:
> > On 20/05/16 10:27, Neil Armstrong wrote:
> >> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> >> Controller.
> >>
> >> Signed-off-by: Neil Armstrong 
> >> ---
> >>  drivers/reset/Kconfig|   6 ++
> >>  drivers/reset/Makefile   |   1 +
> >>  drivers/reset/reset-meson-gxbb.c | 129 
> >> +++
> >>  3 files changed, 136 insertions(+)
> >>  create mode 100644 drivers/reset/reset-meson-gxbb.c
> > 
> > Do we really need to be that specific (-gxbb)? This driver looks generic
> > and simple enough to be used for several Amlogic families. You are
> > already differentiating between them with the include file defining the
> > reset indexes for the SoC.
> 
> This is a good question, do the S805 have similar registers ? Same count and 
> width ?
> I no, it should need a rework to add a data structure per-SoC.

According to the datasheet on S805 we have 7 registers with 16 reset
bits per register.

[...]
> > 
> > [...]
> >> +static struct platform_driver meson_gxbb_reset_driver = {
> >> +  .probe  = meson_gxbb_reset_probe,
> >> +  .remove = meson_gxbb_reset_remove,
> >> +  .driver = {
> >> +  .name   = "meson_gxbb_reset",
> >> +  .of_match_table = meson_gxbb_reset_dt_ids,
> >> +  },
> >> +};
> >> +
> >> +module_platform_driver(meson_gxbb_reset_driver);
> > 
> > No MODULE_AUTHOR, MODULE_LICENSE, etc... ?
> 
> What is the MODULE_LICENCE format for dual licensing ?

Dual BSD/GPL ?

Cheers,

-- 
Carlo Caione

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Carlo Caione 
On 20/05/16 11:10, Neil Armstrong wrote:
> On 05/20/2016 11:04 AM, Carlo Caione wrote:
> > On 20/05/16 10:27, Neil Armstrong wrote:
> >> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> >> Controller.
> >>
> >> Signed-off-by: Neil Armstrong 
> >> ---
> >>  drivers/reset/Kconfig|   6 ++
> >>  drivers/reset/Makefile   |   1 +
> >>  drivers/reset/reset-meson-gxbb.c | 129 
> >> +++
> >>  3 files changed, 136 insertions(+)
> >>  create mode 100644 drivers/reset/reset-meson-gxbb.c
> > 
> > Do we really need to be that specific (-gxbb)? This driver looks generic
> > and simple enough to be used for several Amlogic families. You are
> > already differentiating between them with the include file defining the
> > reset indexes for the SoC.
> 
> This is a good question, do the S805 have similar registers ? Same count and 
> width ?
> I no, it should need a rework to add a data structure per-SoC.

According to the datasheet on S805 we have 7 registers with 16 reset
bits per register.

[...]
> > 
> > [...]
> >> +static struct platform_driver meson_gxbb_reset_driver = {
> >> +  .probe  = meson_gxbb_reset_probe,
> >> +  .remove = meson_gxbb_reset_remove,
> >> +  .driver = {
> >> +  .name   = "meson_gxbb_reset",
> >> +  .of_match_table = meson_gxbb_reset_dt_ids,
> >> +  },
> >> +};
> >> +
> >> +module_platform_driver(meson_gxbb_reset_driver);
> > 
> > No MODULE_AUTHOR, MODULE_LICENSE, etc... ?
> 
> What is the MODULE_LICENCE format for dual licensing ?

Dual BSD/GPL ?

Cheers,

-- 
Carlo Caione

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Philipp Zabel 
Hi Neil,

Am Freitag, den 20.05.2016, 10:27 +0200 schrieb Neil Armstrong:
> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> Controller.
> 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/reset/Kconfig|   6 ++
>  drivers/reset/Makefile   |   1 +
>  drivers/reset/reset-meson-gxbb.c | 129 
> +++
>  3 files changed, 136 insertions(+)
>  create mode 100644 drivers/reset/reset-meson-gxbb.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index df37212..4ac5c4d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -12,5 +12,11 @@ menuconfig RESET_CONTROLLER
>  
> If unsure, say no.
>  
> +config MESON_GXBB_RESET
> + tristate "Amlogic Meson GXBB Reset Driver"
> + depends on (ARCH_MESON && RESET_CONTROLLER)

With the "reset: fix Kconfig menu to include reset drivers in sub-menu"
patch [1] applied this is wrapped in "if RESET_CONTROLLER ... endif", so
no need to depend on RESET_CONTROLLER.
Is there a reason to have this configurable at all, though?

[1] https://patchwork.kernel.org/patch/9000591/

> + help
> +   Build the Amlogic Meson GxBB reset driver.
> +
>  source "drivers/reset/sti/Kconfig"
>  source "drivers/reset/hisilicon/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index a1fc8ed..5ff83a1 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
> +obj-$(CONFIG_MESON_GXBB_RESET) += reset-meson-gxbb.o

Or drop CONFIG_MESON_GXBB_RESET and use CONFIG_ARCH_MESON directly.

[...]
> +static int meson_gxbb_reset_probe(struct platform_device *pdev)
> +{
> + struct meson_gxbb_reset *data;
> + struct resource *res;
> +
> + data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->reg_base = devm_ioremap_resource(>dev, res);
> + if (IS_ERR(data->reg_base))
> + return PTR_ERR(data->reg_base);
> +
> + platform_set_drvdata(pdev, data);
> +
> + data->rcdev.owner = THIS_MODULE;
> + data->rcdev.nr_resets = REG_COUNT * BITS_PER_REG;
> + data->rcdev.ops = _gxbb_reset_ops;
> + data->rcdev.of_node = pdev->dev.of_node;
> +
> + return reset_controller_register(>rcdev);

With the "reset: add devm_reset_controller_register API" patch [2]
applied you can use devm_reset_controller_register() here and remove the
meson_gxbb_reset_remove() function below.

[2] https://patchwork.kernel.org/patch/8988471/

> +}
> +
> +static int meson_gxbb_reset_remove(struct platform_device *pdev)
> +{
> + struct meson_gxbb_reset *data = platform_get_drvdata(pdev);
> +
> + reset_controller_unregister(>rcdev);
> +
> + return 0;
> +}

Could be removed, then.

regards
Philipp

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Philipp Zabel 
Am Freitag, den 20.05.2016, 11:10 +0200 schrieb Neil Armstrong:
[...]
> >> +module_platform_driver(meson_gxbb_reset_driver);
> > 
> > No MODULE_AUTHOR, MODULE_LICENSE, etc... ?
> 
> What is the MODULE_LICENCE format for dual licensing ?

"Dual BSD/GPL"

regards
Philipp

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Philipp Zabel 
Am Freitag, den 20.05.2016, 11:10 +0200 schrieb Neil Armstrong:
[...]
> >> +module_platform_driver(meson_gxbb_reset_driver);
> > 
> > No MODULE_AUTHOR, MODULE_LICENSE, etc... ?
> 
> What is the MODULE_LICENCE format for dual licensing ?

"Dual BSD/GPL"

regards
Philipp

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Philipp Zabel 
Hi Neil,

Am Freitag, den 20.05.2016, 10:27 +0200 schrieb Neil Armstrong:
> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> Controller.
> 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/reset/Kconfig|   6 ++
>  drivers/reset/Makefile   |   1 +
>  drivers/reset/reset-meson-gxbb.c | 129 
> +++
>  3 files changed, 136 insertions(+)
>  create mode 100644 drivers/reset/reset-meson-gxbb.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index df37212..4ac5c4d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -12,5 +12,11 @@ menuconfig RESET_CONTROLLER
>  
> If unsure, say no.
>  
> +config MESON_GXBB_RESET
> + tristate "Amlogic Meson GXBB Reset Driver"
> + depends on (ARCH_MESON && RESET_CONTROLLER)

With the "reset: fix Kconfig menu to include reset drivers in sub-menu"
patch [1] applied this is wrapped in "if RESET_CONTROLLER ... endif", so
no need to depend on RESET_CONTROLLER.
Is there a reason to have this configurable at all, though?

[1] https://patchwork.kernel.org/patch/9000591/

> + help
> +   Build the Amlogic Meson GxBB reset driver.
> +
>  source "drivers/reset/sti/Kconfig"
>  source "drivers/reset/hisilicon/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index a1fc8ed..5ff83a1 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
> +obj-$(CONFIG_MESON_GXBB_RESET) += reset-meson-gxbb.o

Or drop CONFIG_MESON_GXBB_RESET and use CONFIG_ARCH_MESON directly.

[...]
> +static int meson_gxbb_reset_probe(struct platform_device *pdev)
> +{
> + struct meson_gxbb_reset *data;
> + struct resource *res;
> +
> + data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->reg_base = devm_ioremap_resource(>dev, res);
> + if (IS_ERR(data->reg_base))
> + return PTR_ERR(data->reg_base);
> +
> + platform_set_drvdata(pdev, data);
> +
> + data->rcdev.owner = THIS_MODULE;
> + data->rcdev.nr_resets = REG_COUNT * BITS_PER_REG;
> + data->rcdev.ops = _gxbb_reset_ops;
> + data->rcdev.of_node = pdev->dev.of_node;
> +
> + return reset_controller_register(>rcdev);

With the "reset: add devm_reset_controller_register API" patch [2]
applied you can use devm_reset_controller_register() here and remove the
meson_gxbb_reset_remove() function below.

[2] https://patchwork.kernel.org/patch/8988471/

> +}
> +
> +static int meson_gxbb_reset_remove(struct platform_device *pdev)
> +{
> + struct meson_gxbb_reset *data = platform_get_drvdata(pdev);
> +
> + reset_controller_unregister(>rcdev);
> +
> + return 0;
> +}

Could be removed, then.

regards
Philipp

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Neil Armstrong 
On 05/20/2016 11:04 AM, Carlo Caione wrote:
> On 20/05/16 10:27, Neil Armstrong wrote:
>> This patch adds the platform driver for the Amlogic Meson GXBB Reset
>> Controller.
>>
>> Signed-off-by: Neil Armstrong 
>> ---
>>  drivers/reset/Kconfig|   6 ++
>>  drivers/reset/Makefile   |   1 +
>>  drivers/reset/reset-meson-gxbb.c | 129 
>> +++
>>  3 files changed, 136 insertions(+)
>>  create mode 100644 drivers/reset/reset-meson-gxbb.c
> 
> Do we really need to be that specific (-gxbb)? This driver looks generic
> and simple enough to be used for several Amlogic families. You are
> already differentiating between them with the include file defining the
> reset indexes for the SoC.

This is a good question, do the S805 have similar registers ? Same count and 
width ?
I no, it should need a rework to add a data structure per-SoC.

> 
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index df37212..4ac5c4d 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -12,5 +12,11 @@ menuconfig RESET_CONTROLLER
>>  
>>If unsure, say no.
>>  
>> +config MESON_GXBB_RESET
>> +tristate "Amlogic Meson GXBB Reset Driver"
>> +depends on (ARCH_MESON && RESET_CONTROLLER)
>> +help
>> +  Build the Amlogic Meson GxBB reset driver.
>> +
>>  source "drivers/reset/sti/Kconfig"
>>  source "drivers/reset/hisilicon/Kconfig"
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index a1fc8ed..5ff83a1 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
>>  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>>  obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
>> +obj-$(CONFIG_MESON_GXBB_RESET) += reset-meson-gxbb.o
> 
> obj-$(CONFIG_ARCH_MESON) ?
> 
> [...]
>> +static const struct reset_control_ops meson_gxbb_reset_ops = {
>> +.reset  = meson_gxbb_reset_reset,
>> +};
> 
> nit: unfortunate name :)
> 
> Any reason to not have also .assert / .deassert?

The registers seems write-only and does no retain the bit written, assert and 
deassert makes no sense in this case.

> 
> [...]
>> +static struct platform_driver meson_gxbb_reset_driver = {
>> +.probe  = meson_gxbb_reset_probe,
>> +.remove = meson_gxbb_reset_remove,
>> +.driver = {
>> +.name   = "meson_gxbb_reset",
>> +.of_match_table = meson_gxbb_reset_dt_ids,
>> +},
>> +};
>> +
>> +module_platform_driver(meson_gxbb_reset_driver);
> 
> No MODULE_AUTHOR, MODULE_LICENSE, etc... ?

What is the MODULE_LICENCE format for dual licensing ?

Thanks,
Neil

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Neil Armstrong 
On 05/20/2016 11:04 AM, Carlo Caione wrote:
> On 20/05/16 10:27, Neil Armstrong wrote:
>> This patch adds the platform driver for the Amlogic Meson GXBB Reset
>> Controller.
>>
>> Signed-off-by: Neil Armstrong 
>> ---
>>  drivers/reset/Kconfig|   6 ++
>>  drivers/reset/Makefile   |   1 +
>>  drivers/reset/reset-meson-gxbb.c | 129 
>> +++
>>  3 files changed, 136 insertions(+)
>>  create mode 100644 drivers/reset/reset-meson-gxbb.c
> 
> Do we really need to be that specific (-gxbb)? This driver looks generic
> and simple enough to be used for several Amlogic families. You are
> already differentiating between them with the include file defining the
> reset indexes for the SoC.

This is a good question, do the S805 have similar registers ? Same count and 
width ?
I no, it should need a rework to add a data structure per-SoC.

> 
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index df37212..4ac5c4d 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -12,5 +12,11 @@ menuconfig RESET_CONTROLLER
>>  
>>If unsure, say no.
>>  
>> +config MESON_GXBB_RESET
>> +tristate "Amlogic Meson GXBB Reset Driver"
>> +depends on (ARCH_MESON && RESET_CONTROLLER)
>> +help
>> +  Build the Amlogic Meson GxBB reset driver.
>> +
>>  source "drivers/reset/sti/Kconfig"
>>  source "drivers/reset/hisilicon/Kconfig"
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index a1fc8ed..5ff83a1 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
>>  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>>  obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
>> +obj-$(CONFIG_MESON_GXBB_RESET) += reset-meson-gxbb.o
> 
> obj-$(CONFIG_ARCH_MESON) ?
> 
> [...]
>> +static const struct reset_control_ops meson_gxbb_reset_ops = {
>> +.reset  = meson_gxbb_reset_reset,
>> +};
> 
> nit: unfortunate name :)
> 
> Any reason to not have also .assert / .deassert?

The registers seems write-only and does no retain the bit written, assert and 
deassert makes no sense in this case.

> 
> [...]
>> +static struct platform_driver meson_gxbb_reset_driver = {
>> +.probe  = meson_gxbb_reset_probe,
>> +.remove = meson_gxbb_reset_remove,
>> +.driver = {
>> +.name   = "meson_gxbb_reset",
>> +.of_match_table = meson_gxbb_reset_dt_ids,
>> +},
>> +};
>> +
>> +module_platform_driver(meson_gxbb_reset_driver);
> 
> No MODULE_AUTHOR, MODULE_LICENSE, etc... ?

What is the MODULE_LICENCE format for dual licensing ?

Thanks,
Neil

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Carlo Caione 
On 20/05/16 10:27, Neil Armstrong wrote:
> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> Controller.
> 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/reset/Kconfig|   6 ++
>  drivers/reset/Makefile   |   1 +
>  drivers/reset/reset-meson-gxbb.c | 129 
> +++
>  3 files changed, 136 insertions(+)
>  create mode 100644 drivers/reset/reset-meson-gxbb.c

Do we really need to be that specific (-gxbb)? This driver looks generic
and simple enough to be used for several Amlogic families. You are
already differentiating between them with the include file defining the
reset indexes for the SoC.

> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index df37212..4ac5c4d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -12,5 +12,11 @@ menuconfig RESET_CONTROLLER
>  
> If unsure, say no.
>  
> +config MESON_GXBB_RESET
> + tristate "Amlogic Meson GXBB Reset Driver"
> + depends on (ARCH_MESON && RESET_CONTROLLER)
> + help
> +   Build the Amlogic Meson GxBB reset driver.
> +
>  source "drivers/reset/sti/Kconfig"
>  source "drivers/reset/hisilicon/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index a1fc8ed..5ff83a1 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
> +obj-$(CONFIG_MESON_GXBB_RESET) += reset-meson-gxbb.o

obj-$(CONFIG_ARCH_MESON) ?

[...]
> +static const struct reset_control_ops meson_gxbb_reset_ops = {
> + .reset  = meson_gxbb_reset_reset,
> +};

nit: unfortunate name :)

Any reason to not have also .assert / .deassert?

[...]
> +static struct platform_driver meson_gxbb_reset_driver = {
> + .probe  = meson_gxbb_reset_probe,
> + .remove = meson_gxbb_reset_remove,
> + .driver = {
> + .name   = "meson_gxbb_reset",
> + .of_match_table = meson_gxbb_reset_dt_ids,
> + },
> +};
> +
> +module_platform_driver(meson_gxbb_reset_driver);

No MODULE_AUTHOR, MODULE_LICENSE, etc... ?

-- 
Carlo Caione

Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

2016-05-20 Thread Carlo Caione 
On 20/05/16 10:27, Neil Armstrong wrote:
> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> Controller.
> 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/reset/Kconfig|   6 ++
>  drivers/reset/Makefile   |   1 +
>  drivers/reset/reset-meson-gxbb.c | 129 
> +++
>  3 files changed, 136 insertions(+)
>  create mode 100644 drivers/reset/reset-meson-gxbb.c

Do we really need to be that specific (-gxbb)? This driver looks generic
and simple enough to be used for several Amlogic families. You are
already differentiating between them with the include file defining the
reset indexes for the SoC.

> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index df37212..4ac5c4d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -12,5 +12,11 @@ menuconfig RESET_CONTROLLER
>  
> If unsure, say no.
>  
> +config MESON_GXBB_RESET
> + tristate "Amlogic Meson GXBB Reset Driver"
> + depends on (ARCH_MESON && RESET_CONTROLLER)
> + help
> +   Build the Amlogic Meson GxBB reset driver.
> +
>  source "drivers/reset/sti/Kconfig"
>  source "drivers/reset/hisilicon/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index a1fc8ed..5ff83a1 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
> +obj-$(CONFIG_MESON_GXBB_RESET) += reset-meson-gxbb.o

obj-$(CONFIG_ARCH_MESON) ?

[...]
> +static const struct reset_control_ops meson_gxbb_reset_ops = {
> + .reset  = meson_gxbb_reset_reset,
> +};

nit: unfortunate name :)

Any reason to not have also .assert / .deassert?

[...]
> +static struct platform_driver meson_gxbb_reset_driver = {
> + .probe  = meson_gxbb_reset_probe,
> + .remove = meson_gxbb_reset_remove,
> + .driver = {
> + .name   = "meson_gxbb_reset",
> + .of_match_table = meson_gxbb_reset_dt_ids,
> + },
> +};
> +
> +module_platform_driver(meson_gxbb_reset_driver);

No MODULE_AUTHOR, MODULE_LICENSE, etc... ?

-- 
Carlo Caione