RE: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more generic for other i.MX platforms

From: Anson Huang
Date: Tue Aug 28 2018 - 04:03:03 EST

Next message: Jarkko Sakkinen: "Re: [PATCH v13 13/13] x86/sgx: Driver documentation"
Previous message: Paul Kocialkowski: "[PATCH 2/2] media: cedrus: Add HEVC/H.265 decoding support"
In reply to: Andrey Smirnov: "Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more generic for other i.MX platforms"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Anson Huang
Best Regards!

> -----Original Message-----
> From: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> Sent: Tuesday, August 28, 2018 4:01 PM
> To: Anson Huang <anson.huang@xxxxxxx>
> Cc: Shawn Guo <shawnguo@xxxxxxxxxx>; Sascha Hauer
> <s.hauer@xxxxxxxxxxxxxx>; Sascha Hauer <kernel@xxxxxxxxxxxxxx>; Fabio
> Estevam <fabio.estevam@xxxxxxx>; linux-arm-kernel
> <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; linux-kernel
> <linux-kernel@xxxxxxxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>
> Subject: Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more generic for
> other i.MX platforms
>
> On Tue, Aug 28, 2018 at 12:28 AM Anson Huang <anson.huang@xxxxxxx>
> wrote:
> >
> >
> >
> > Anson Huang
> > Best Regards!
> >
> >
> > > -----Original Message-----
> > > From: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > > Sent: Tuesday, August 28, 2018 6:51 AM
> > > To: Anson Huang <anson.huang@xxxxxxx>
> > > Cc: Shawn Guo <shawnguo@xxxxxxxxxx>; Sascha Hauer
> > > <s.hauer@xxxxxxxxxxxxxx>; Sascha Hauer <kernel@xxxxxxxxxxxxxx>;
> > > Fabio Estevam <fabio.estevam@xxxxxxx>; linux-arm-kernel
> > > <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; linux-kernel
> > > <linux-kernel@xxxxxxxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>
> > > Subject: Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more
> > > generic for other i.MX platforms
> > >
> > > On Sun, Aug 5, 2018 at 11:45 PM Anson Huang <Anson.Huang@xxxxxxx>
> > > wrote:
> > > >
> > > > i.MX8MQ and i.MX8MM share same gpc module with i.MX7D, they can
> > > > reuse
> > > > gpcv2 pgc driver for power domain control, this patch renames all
> > > > functions and structure definitions started with "imx7" to "imx",
> > > > and check machine type to pass platform specific power domain data
> > > > for power domain driver, thus make
> > > > gpcv2 pgc driver more generic for i.MX platforms.
> > > >
> > >
> > > Just for the sake of
> > >
> > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> > > > ---
> > > > drivers/soc/imx/gpcv2.c | 68
> > > > +++++++++++++++++++++++++++++--------------------
> > > > 1 file changed, 40 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > > index
> > > > 0e31465..0e33cb5 100644
> > > > --- a/drivers/soc/imx/gpcv2.c
> > > > +++ b/drivers/soc/imx/gpcv2.c
> > > > @@ -53,7 +53,7 @@
> > > >
> > > > #define GPC_PGC_CTRL_PCR BIT(0)
> > > >
> > > > -struct imx7_pgc_domain {
> > > > +struct imx_pgc_domain {
> > > > struct generic_pm_domain genpd;
> > > > struct regmap *regmap;
> > > > struct regulator *regulator; @@ -69,11 +69,11 @@ struct
> > > > imx7_pgc_domain {
> > > > struct device *dev;
> > > > };
> > > >
> > > > -static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain
> > > > *genpd,
> > > > +static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain
> > > > +*genpd,
> > > > bool on) {
> > > > - struct imx7_pgc_domain *domain = container_of(genpd,
> > > > - struct
> > > imx7_pgc_domain,
> > > > + struct imx_pgc_domain *domain = container_of(genpd,
> > > > + struct
> > > > + imx_pgc_domain,
> > > >
> genpd);
> > > > unsigned int offset = on ?
> > > > GPC_PU_PGC_SW_PUP_REQ :
> > > GPC_PU_PGC_SW_PDN_REQ; @@
> > > > -150,17 +150,17 @@ static int imx7_gpc_pu_pgc_sw_pxx_req(struct
> > > generic_pm_domain *genpd,
> > > > return ret;
> > > > }
> > > >
> > > > -static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain
> > > > *genpd)
> > > > +static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain
> > > > +*genpd)
> > > > {
> > > > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > > > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > > > }
> > > >
> > > > -static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain
> > > > *genpd)
> > > > +static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain
> > > > +*genpd)
> > > > {
> > > > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > > > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > > > }
> > > >
> > > > -static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> > > > +static const struct imx_pgc_domain imx7_pgc_domains[] = {
> > > > [IMX7_POWER_DOMAIN_MIPI_PHY] = {
> > > > .genpd = {
> > > > .name = "mipi-phy",
> > > > @@ -198,9 +198,9 @@ static const struct imx7_pgc_domain
> > > imx7_pgc_domains[] = {
> > > > },
> > > > };
> > > >
> > > > -static int imx7_pgc_domain_probe(struct platform_device *pdev)
> > > > +static int imx_pgc_domain_probe(struct platform_device *pdev)
> > > > {
> > > > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > > > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > > > int ret;
> > > >
> > > > domain->dev = &pdev->dev;
> > > > @@ -233,9 +233,9 @@ static int imx7_pgc_domain_probe(struct
> > > platform_device *pdev)
> > > > return ret;
> > > > }
> > > >
> > > > -static int imx7_pgc_domain_remove(struct platform_device *pdev)
> > > > +static int imx_pgc_domain_remove(struct platform_device *pdev)
> > > > {
> > > > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > > > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > > >
> > > > of_genpd_del_provider(domain->dev->of_node);
> > > > pm_genpd_remove(&domain->genpd); @@ -243,23 +243,24
> @@
> > > static
> > > > int imx7_pgc_domain_remove(struct platform_device *pdev)
> > > > return 0;
> > > > }
> > > >
> > > > -static const struct platform_device_id imx7_pgc_domain_id[] = {
> > > > - { "imx7-pgc-domain", },
> > > > +static const struct platform_device_id imx_pgc_domain_id[] = {
> > > > + { "imx-pgc-domain", },
> > > > { },
> > > > };
> > > >
> > > > -static struct platform_driver imx7_pgc_domain_driver = {
> > > > +static struct platform_driver imx_pgc_domain_driver = {
> > > > .driver = {
> > > > - .name = "imx7-pgc",
> > > > + .name = "imx-pgc",
> > > > },
> > > > - .probe = imx7_pgc_domain_probe,
> > > > - .remove = imx7_pgc_domain_remove,
> > > > - .id_table = imx7_pgc_domain_id,
> > > > + .probe = imx_pgc_domain_probe,
> > > > + .remove = imx_pgc_domain_remove,
> > > > + .id_table = imx_pgc_domain_id,
> > > > };
> > > > -builtin_platform_driver(imx7_pgc_domain_driver)
> > > > +builtin_platform_driver(imx_pgc_domain_driver)
> > > >
> > > > static int imx_gpcv2_probe(struct platform_device *pdev) {
> > > > + static const struct imx_pgc_domain *imx_pgc_domains;
> > > > static const struct regmap_range yes_ranges[] = {
> > > > regmap_reg_range(GPC_LPCR_A_CORE_BSC,
> > > > GPC_M4_PU_PDN_FLG), @@
> > > -287,6 +288,7
> > > > @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> > > > struct regmap *regmap;
> > > > struct resource *res;
> > > > void __iomem *base;
> > > > + int pgc_max_index;
> > > > int ret;
> > > >
> > > > pgc_np = of_get_child_by_name(dev->of_node, "pgc"); @@
> > > > -307,9
> > > > +309,19 @@ static int imx_gpcv2_probe(struct platform_device
> > > > +*pdev)
> > > > return ret;
> > > > }
> > > >
> > > > + if (of_machine_is_compatible("fsl,imx7d")) {
> > > > + pgc_max_index = ARRAY_SIZE(imx7_pgc_domains);
> > > > + imx_pgc_domains = imx7_pgc_domains;
> > > > + }
> > >
> > > Is there any reason to do this explicit call to
> > > of_machine_is_compatible() as opposed to passing necessary data via
> > > .data in imx_gpcv2_dt_ids[]? The latter seems like a more
> > > straightforward way of passing variant specific driver info
> >
> > The reason of NOT using .data is that currently gpcv2 driver uses a
> > structure array of imx7_pgc_domains[] which has no fixed length, the
> > .data can ONLY pass the pointer of the starting address of
> > imx7_pgc_domains[], and we need to know the array size of
> > imx7_pgc_domains, how can we know it if we ONLY got the starting address
> of this array passed from .data. Please advise, thanks.
> >
>
> Unless I am missing something, you should be able to do something like:
>
> struct imx_pgc_domain_data {
> struct imx_pgc_domain *domains;
> size_t domains_num;
> };
>
> const struct imx_pgc_domain_data imx7_pgc_domain_data = {
> .domains = imx7_pgc_domains,
> .domains_num = ARRAY_SIZE(imx7_pgc_domains), };
>
> and then just pass imx7_pgc_domain_data via .data. That's a pretty common
> idiom in kernel driver.
>
> Thanks,
> Andrey Smirnov

Thanks Andrey. I just thought we can avoid another warp for the structure, if
we can NOT, I will add a wrap in V2 patch set as you suggested, thanks.

Anson.

Next message: Jarkko Sakkinen: "Re: [PATCH v13 13/13] x86/sgx: Driver documentation"
Previous message: Paul Kocialkowski: "[PATCH 2/2] media: cedrus: Add HEVC/H.265 decoding support"
In reply to: Andrey Smirnov: "Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more generic for other i.MX platforms"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]