2023-01-23 11:48:57

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH 06/15] soc: add Ambarella driver

Hey Li Chen,

Since you've already got a bunch of other comments, I have
two minor ones for you.

On 23/01/2023 07:32, Li Chen wrote:
>
> This driver add soc_id support for Ambarella,
> which is stored inside "cpuid" AXI address mapping.
>
> Also provide sys_config(POC, aka power on configuration)
> for other drivers.
>
> Signed-off-by: Li Chen <[email protected]>
> Change-Id: I4869a3497366ac7779e792835f8e0309239036a8
> ---

> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index fff513bd522d..e3e270aa32a4 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -8,6 +8,7 @@ obj-y += apple/
> obj-y += aspeed/
> obj-$(CONFIG_ARCH_AT91) += atmel/
> obj-y += bcm/
> +obj-$(CONFIG_ARCH_AMBARELLA) += ambarella/
> obj-$(CONFIG_SOC_CANAAN) += canaan/

If you could follow the existing (by directory) alphabetical
order that'd be great.

> obj-$(CONFIG_ARCH_DOVE) += dove/
> obj-$(CONFIG_MACH_DOVE) += dove/
> diff --git a/drivers/soc/ambarella/Makefile b/drivers/soc/ambarella/Makefile
> new file mode 100644
> index 000000000000..384276c046ca
> --- /dev/null
> +++ b/drivers/soc/ambarella/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_ARCH_AMBARELLA) += soc.o

The subdirectory is already gated by this symbol, so is there much point
gating it on the same one again?



2023-01-24 08:29:05

by Li Chen

[permalink] [raw]
Subject: Re: [PATCH 06/15] soc: add Ambarella driver

On Mon, 23 Jan 2023 19:48:42 +0800,

Hi Conor.Dooley

<[email protected]> wrote:
>
> Hey Li Chen,
>
> Since you've already got a bunch of other comments, I have
> two minor ones for you.
>
> On 23/01/2023 07:32, Li Chen wrote:
> >
> > This driver add soc_id support for Ambarella,
> > which is stored inside "cpuid" AXI address mapping.
> >
> > Also provide sys_config(POC, aka power on configuration)
> > for other drivers.
> >
> > Signed-off-by: Li Chen <[email protected]>
> > Change-Id: I4869a3497366ac7779e792835f8e0309239036a8
> > ---
>
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index fff513bd522d..e3e270aa32a4 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -8,6 +8,7 @@ obj-y += apple/
> > obj-y += aspeed/
> > obj-$(CONFIG_ARCH_AT91) += atmel/
> > obj-y += bcm/
> > +obj-$(CONFIG_ARCH_AMBARELLA) += ambarella/
> > obj-$(CONFIG_SOC_CANAAN) += canaan/
>
> If you could follow the existing (by directory) alphabetical
> order that'd be great.

Well noted. I will fix it in v2.

> > obj-$(CONFIG_ARCH_DOVE) += dove/
> > obj-$(CONFIG_MACH_DOVE) += dove/
> > diff --git a/drivers/soc/ambarella/Makefile b/drivers/soc/ambarella/Makefile
> > new file mode 100644
> > index 000000000000..384276c046ca
> > --- /dev/null
> > +++ b/drivers/soc/ambarella/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +obj-$(CONFIG_ARCH_AMBARELLA) += soc.o
>
> The subdirectory is already gated by this symbol, so is there much point
> gating it on the same one again?

Yeah, it lookgs kind of redundant now, but I will upstream other drivers after
this series get merged, which are not gated by this symbol.

Regards,
Li

2023-01-24 08:46:54

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH 06/15] soc: add Ambarella driver

Hey,

>>> +++ b/drivers/soc/ambarella/Makefile
>>> @@ -0,0 +1,3 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +obj-$(CONFIG_ARCH_AMBARELLA) += soc.o
>>
>> The subdirectory is already gated by this symbol, so is there much point
>> gating it on the same one again?
>
> Yeah, it lookgs kind of redundant now, but I will upstream other drivers after
> this series get merged, which are not gated by this symbol.

You could make the directory by obj-y, and therefore always included,
and have various drivers controlled by their own Kconfig symbols.

Or else, you could leave the directory controlled by ARCH_AMBARELLA
and make the above `obj-y += soc.o` instead, since it's always
going to be built if the directory is included.

Thanks,
Conor.


2023-01-24 14:26:50

by Li Chen

[permalink] [raw]
Subject: Re: [PATCH 06/15] soc: add Ambarella driver


Hi Conor.Dooley,

On Tue, 24 Jan 2023 16:46:41 +0800,
<[email protected]> wrote:
>
> Hey,
>
> >>> +++ b/drivers/soc/ambarella/Makefile
> >>> @@ -0,0 +1,3 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only
> >>> +
> >>> +obj-$(CONFIG_ARCH_AMBARELLA) += soc.o
> >>
> >> The subdirectory is already gated by this symbol, so is there much point
> >> gating it on the same one again?
> >
> > Yeah, it lookgs kind of redundant now, but I will upstream other drivers after
> > this series get merged, which are not gated by this symbol.
>
> You could make the directory by obj-y, and therefore always included,
> and have various drivers controlled by their own Kconfig symbols.
>
> Or else, you could leave the directory controlled by ARCH_AMBARELLA
> and make the above `obj-y += soc.o` instead, since it's always
> going to be built if the directory is included.

Gotcha, I will fix it in v2. Thanks for your kindness!

Regards,
Li