| Message ID | 1555038945-10245-1-git-send-email-light.hsieh@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] pinctrl: Add alternative way for specifying register bases | expand |
Hi, Light On Thu, Apr 11, 2019 at 8:15 PM Light Hsieh <light.hsieh@mediatek.com> wrote: > > The orginal PINCTRL_MTK_PARIS/PINCTRL_MTK_MOORE need more effort for > specifying register bases when porting platform driver: > 1. Write mtXXXX_pinctrl_register_base_name[] array in pinctrl-mtXXXX.c > to specify names of register bases, for exmaple: > > static const char * const mt6765_pinctrl_register_base_names[] = { > "iocfg0", "iocfg1", "iocfg2", "iocfg3", "iocfg4", "iocfg5", > "iocfg6", "iocfg7", > }; > 2. Write reg = <...>, ..., <...>; in mtXXXX.dts to specify register > bases. Each member of reg contain address range cloned from > pre-generated devicetree node. > 3. Write reg-names = "...", ..., "..."; in mtXXXX.dts to specify > names of register bases. The sequence of names in reg-names shall match > sequence of names that specified in pinctrl-mtXXXX.c. > Besides, the seqeunce of names in reg-names shall also match sequence of > address range in reg, for exmaple: > > pio: pinctrl { > compatible = "mediatek,mt6765-pinctrl"; > reg = <0 0x10005000 0 0x1000>, > <0 0x10002C00 0 0x200>, > <0 0x10002800 0 0x200>, > <0 0x10002A00 0 0x200>, > <0 0x10002000 0 0x200>, > <0 0x10002200 0 0x200>, > <0 0x10002400 0 0x200>, > <0 0x10002600 0 0x200>, > <0 0x1000b000 0 0x1000>; > reg-names = "iocfg0", "iocfg1", "iocfg2", "iocfg3", > "iocfg4", "iocfg5", "iocfg6", "iocfg7", > "eint"; > > To reduce porting effort, this patch add an alternative way for specifying > register bases: > 1. Write reg_bases = <...>, ..., <...>; and reg_base_eint = <&eint>; > in mtXXXX.dtsi where members in reg_bases and &eint are labels for > pre-generated devicetree nodes, for example: > pio: pinctrl { > compatible = "mediatek,mt6765-pinctrl"; > reg_bases = <&gpio0>, > <&iocfg0>, > <&iocfg1>, > <&iocfg2>, > <&iocfg3>, > <&iocfg4>, > <&iocfg5>, > <&iocfg6>, > <&iocfg7>; > reg_base_eint = <&eint>; reg and reg-names both are generic properties used in all of DT-based device and driver, described in Documentation/devicetree/bindings/resource-names.txt, but reg_based and reg_base_eint aren't. If these properties are not hardware specific related, I personally will encourage reusing those generic properties and relevant helpers because those generic properties handling in the base driver are almost bug-free, well maintained and even keep be extended in the future. This way can help driver people put more concentration on hardware specific stuff in the driver. > > Since this pre-generated nodes had already specify address range, > it is not necessary to specify address range again in pinctrl node. > > Using this way, porting effort is reduced and therefore typo can occur with > less chance. > > Change-Id: I55f5e328919f4f736ca4b9f8d1593e069f179637 > --- > drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 19 +++++--- > drivers/pinctrl/mediatek/pinctrl-paris.c | 62 ++++++++++++++++-------- > 2 files changed, 54 insertions(+), 27 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > index b1c3684..16b4863 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > @@ -12,6 +12,7 @@ > #include <linux/platform_device.h> > #include <linux/io.h> > #include <linux/of_irq.h> > +#include <linux/of_address.h> > > #include "mtk-eint.h" > #include "pinctrl-mtk-common-v2.h" > @@ -310,7 +311,7 @@ static int mtk_xt_set_gpio_as_eint(void *data, unsigned long eint_n) > > int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) > { > - struct device_node *np = pdev->dev.of_node; > + struct device_node *np = pdev->dev.of_node, *node; > struct resource *res; > > if (!IS_ENABLED(CONFIG_EINT_MTK)) > @@ -323,13 +324,19 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) > if (!hw->eint) > return -ENOMEM; > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "eint"); > - if (!res) { > - dev_err(&pdev->dev, "Unable to get eint resource\n"); > - return -ENODEV; > + node = of_parse_phandle(np, "reg_base_eint", 0); > + if (node) { > + hw->eint->base = of_iomap(node, 0); > + } else { > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "eint"); > + if (!res) { > + dev_err(&pdev->dev, "Unable to get eint resource\n"); > + return -ENODEV; > + } > + hw->eint->base = devm_ioremap_resource(&pdev->dev, res); > } > > - hw->eint->base = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(hw->eint->base)) > return PTR_ERR(hw->eint->base); > > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c > index b59e108..8ddb995 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c > @@ -9,6 +9,7 @@ > * Hongzhou.Yang <hongzhou.yang@mediatek.com> > */ > > +#include <linux/of_address.h> > #include <linux/gpio/driver.h> > #include <dt-bindings/pinctrl/mt65xx.h> > #include "pinctrl-paris.h" > @@ -815,10 +816,11 @@ static int mtk_pctrl_build_state(struct platform_device *pdev) > int mtk_paris_pinctrl_probe(struct platform_device *pdev, > const struct mtk_pin_soc *soc) > { > + struct device_node *np = pdev->dev.of_node, *node; > struct pinctrl_pin_desc *pins; > struct mtk_pinctrl *hw; > struct resource *res; > - int err, i; > + int err, i, get_base_pass; > > hw = devm_kzalloc(&pdev->dev, sizeof(*hw), GFP_KERNEL); > if (!hw) > @@ -828,31 +830,49 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev, > hw->soc = soc; > hw->dev = &pdev->dev; > > - if (!hw->soc->nbase_names) { > - dev_err(&pdev->dev, > - "SoC should be assigned at least one register base\n"); > - return -EINVAL; > - } > - > - hw->base = devm_kmalloc_array(&pdev->dev, hw->soc->nbase_names, > + if (hw->soc->nbase_names) { > + hw->base = devm_kmalloc_array(&pdev->dev, hw->soc->nbase_names, > sizeof(*hw->base), GFP_KERNEL); > - if (!hw->base) > - return -ENOMEM; > + if (!hw->base) > + return -ENOMEM; > + > + for (i = 0; i < hw->soc->nbase_names; i++) { > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + hw->soc->base_names[i]); > + if (!res) { > + dev_err(&pdev->dev, "missing IO resource\n"); > + return -ENXIO; > + } > > - for (i = 0; i < hw->soc->nbase_names; i++) { > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > - hw->soc->base_names[i]); > - if (!res) { > - dev_err(&pdev->dev, "missing IO resource\n"); > - return -ENXIO; > + hw->base[i] = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(hw->base[i])) > + return PTR_ERR(hw->base[i]); > } > > - hw->base[i] = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(hw->base[i])) > - return PTR_ERR(hw->base[i]); > - } > + hw->nbase = hw->soc->nbase_names; > + } else { > + for (get_base_pass = 0; get_base_pass < 2; get_base_pass++) { > + for (i = 0;; i++) { > + node = of_parse_phandle(np, "reg_bases", i); > + if (!node) > + break; > + if (get_base_pass == 1) > + hw->base[i] = of_iomap(node, 0); > + of_node_put(node); > + } > > - hw->nbase = hw->soc->nbase_names; > + if (i == 0) > + return -EINVAL; > + if (get_base_pass == 0) { > + hw->nbase = i; > + hw->base = devm_kmalloc_array(&pdev->dev, i, > + sizeof(*hw->base), > + GFP_KERNEL | __GFP_ZERO); > + if (IS_ERR(hw->base)) > + return PTR_ERR(hw->base); > + } > + } > + } > > err = mtk_pctrl_build_state(pdev); > if (err) { > -- > 1.8.1.1.dirty >
Hi, Sean: On Sun, 2019-04-14 at 16:01 -0700, Sean Wang wrote: > Hi, Light > > On Thu, Apr 11, 2019 at 8:15 PM Light Hsieh <light.hsieh@mediatek.com> wrote: > > > > The orginal PINCTRL_MTK_PARIS/PINCTRL_MTK_MOORE need more effort for > > specifying register bases when porting platform driver: > > 1. Write mtXXXX_pinctrl_register_base_name[] array in pinctrl-mtXXXX.c > > to specify names of register bases, for exmaple: > > > > static const char * const mt6765_pinctrl_register_base_names[] = { > > "iocfg0", "iocfg1", "iocfg2", "iocfg3", "iocfg4", "iocfg5", > > "iocfg6", "iocfg7", > > }; > > 2. Write reg = <...>, ..., <...>; in mtXXXX.dts to specify register > > bases. Each member of reg contain address range cloned from > > pre-generated devicetree node. > > 3. Write reg-names = "...", ..., "..."; in mtXXXX.dts to specify > > names of register bases. The sequence of names in reg-names shall match > > sequence of names that specified in pinctrl-mtXXXX.c. > > Besides, the seqeunce of names in reg-names shall also match sequence of > > address range in reg, for exmaple: > > > > pio: pinctrl { > > compatible = "mediatek,mt6765-pinctrl"; > > reg = <0 0x10005000 0 0x1000>, > > <0 0x10002C00 0 0x200>, > > <0 0x10002800 0 0x200>, > > <0 0x10002A00 0 0x200>, > > <0 0x10002000 0 0x200>, > > <0 0x10002200 0 0x200>, > > <0 0x10002400 0 0x200>, > > <0 0x10002600 0 0x200>, > > <0 0x1000b000 0 0x1000>; > > reg-names = "iocfg0", "iocfg1", "iocfg2", "iocfg3", > > "iocfg4", "iocfg5", "iocfg6", "iocfg7", > > "eint"; > > > > To reduce porting effort, this patch add an alternative way for specifying > > register bases: > > 1. Write reg_bases = <...>, ..., <...>; and reg_base_eint = <&eint>; > > in mtXXXX.dtsi where members in reg_bases and &eint are labels for > > pre-generated devicetree nodes, for example: > > pio: pinctrl { > > compatible = "mediatek,mt6765-pinctrl"; > > reg_bases = <&gpio0>, > > <&iocfg0>, > > <&iocfg1>, > > <&iocfg2>, > > <&iocfg3>, > > <&iocfg4>, > > <&iocfg5>, > > <&iocfg6>, > > <&iocfg7>; > > reg_base_eint = <&eint>; > > reg and reg-names both are generic properties used in all of DT-based > device and driver, described in > Documentation/devicetree/bindings/resource-names.txt, but reg_based > and reg_base_eint aren't. > > If these properties are not hardware specific related, I personally > will encourage reusing those generic properties and relevant helpers > because those generic properties handling in the base driver are > almost bug-free, well maintained and even keep be extended in the > future. This way can help driver people put more concentration on > hardware specific stuff in the driver. > This modification is somewhat like mtk pinctrl driver V1. In V1, you may have the following devicetree (refer to Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt): : syscfg_pctl_a: syscfg-pctl-a@10005000 { compatible = "mediatek,mt8135-pctl-a-syscfg", "syscon"; reg = <0 0x10005000 0 0x1000>; }; syscfg_pctl_b: syscfg-pctl-b@1020c020 { compatible = "mediatek,mt8135-pctl-b-syscfg", "syscon"; reg = <0 0x1020C020 0 0x1000>; }; pinctrl@1c20800 { compatible = "mediatek,mt8135-pinctrl"; mediatek,pctl-regmap = <&syscfg_pctl_a &syscfg_pctl_b>; and have following code in pinctrl-mtk-common.c: node = of_parse_phandle(np, "mediatek,pctl-regmap", 0); node = of_parse_phandle(np, "mediatek,pctl-regmap", 1); "mediatek, pctl-regmap" is self-defined devicetree property only used by mtk pinctrl drver V1. Now, I use similar logic with another property name.So I don't think the can add risk. Besides, since this way reduce porting effort and avoid human-error when writing devicetree and c code about device tree porting, it also help driver people put more concentration on hardware specific stuff in the driver. > > > > Since this pre-generated nodes had already specify address range, > > it is not necessary to specify address range again in pinctrl node. > > > > Using this way, porting effort is reduced and therefore typo can occur with > > less chance. > > > > Change-Id: I55f5e328919f4f736ca4b9f8d1593e069f179637 > > --- > > drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 19 +++++--- > > drivers/pinctrl/mediatek/pinctrl-paris.c | 62 ++++++++++++++++-------- > > 2 files changed, 54 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > > index b1c3684..16b4863 100644 > > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > > @@ -12,6 +12,7 @@ > > #include <linux/platform_device.h> > > #include <linux/io.h> > > #include <linux/of_irq.h> > > +#include <linux/of_address.h> > > > > #include "mtk-eint.h" > > #include "pinctrl-mtk-common-v2.h" > > @@ -310,7 +311,7 @@ static int mtk_xt_set_gpio_as_eint(void *data, unsigned long eint_n) > > > > int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) > > { > > - struct device_node *np = pdev->dev.of_node; > > + struct device_node *np = pdev->dev.of_node, *node; > > struct resource *res; > > > > if (!IS_ENABLED(CONFIG_EINT_MTK)) > > @@ -323,13 +324,19 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) > > if (!hw->eint) > > return -ENOMEM; > > > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "eint"); > > - if (!res) { > > - dev_err(&pdev->dev, "Unable to get eint resource\n"); > > - return -ENODEV; > > + node = of_parse_phandle(np, "reg_base_eint", 0); > > + if (node) { > > + hw->eint->base = of_iomap(node, 0); > > + } else { > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > + "eint"); > > + if (!res) { > > + dev_err(&pdev->dev, "Unable to get eint resource\n"); > > + return -ENODEV; > > + } > > + hw->eint->base = devm_ioremap_resource(&pdev->dev, res); > > } > > > > - hw->eint->base = devm_ioremap_resource(&pdev->dev, res); > > if (IS_ERR(hw->eint->base)) > > return PTR_ERR(hw->eint->base); > > > > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c > > index b59e108..8ddb995 100644 > > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c > > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c > > @@ -9,6 +9,7 @@ > > * Hongzhou.Yang <hongzhou.yang@mediatek.com> > > */ > > > > +#include <linux/of_address.h> > > #include <linux/gpio/driver.h> > > #include <dt-bindings/pinctrl/mt65xx.h> > > #include "pinctrl-paris.h" > > @@ -815,10 +816,11 @@ static int mtk_pctrl_build_state(struct platform_device *pdev) > > int mtk_paris_pinctrl_probe(struct platform_device *pdev, > > const struct mtk_pin_soc *soc) > > { > > + struct device_node *np = pdev->dev.of_node, *node; > > struct pinctrl_pin_desc *pins; > > struct mtk_pinctrl *hw; > > struct resource *res; > > - int err, i; > > + int err, i, get_base_pass; > > > > hw = devm_kzalloc(&pdev->dev, sizeof(*hw), GFP_KERNEL); > > if (!hw) > > @@ -828,31 +830,49 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev, > > hw->soc = soc; > > hw->dev = &pdev->dev; > > > > - if (!hw->soc->nbase_names) { > > - dev_err(&pdev->dev, > > - "SoC should be assigned at least one register base\n"); > > - return -EINVAL; > > - } > > - > > - hw->base = devm_kmalloc_array(&pdev->dev, hw->soc->nbase_names, > > + if (hw->soc->nbase_names) { > > + hw->base = devm_kmalloc_array(&pdev->dev, hw->soc->nbase_names, > > sizeof(*hw->base), GFP_KERNEL); > > - if (!hw->base) > > - return -ENOMEM; > > + if (!hw->base) > > + return -ENOMEM; > > + > > + for (i = 0; i < hw->soc->nbase_names; i++) { > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > + hw->soc->base_names[i]); > > + if (!res) { > > + dev_err(&pdev->dev, "missing IO resource\n"); > > + return -ENXIO; > > + } > > > > - for (i = 0; i < hw->soc->nbase_names; i++) { > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > - hw->soc->base_names[i]); > > - if (!res) { > > - dev_err(&pdev->dev, "missing IO resource\n"); > > - return -ENXIO; > > + hw->base[i] = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(hw->base[i])) > > + return PTR_ERR(hw->base[i]); > > } > > > > - hw->base[i] = devm_ioremap_resource(&pdev->dev, res); > > - if (IS_ERR(hw->base[i])) > > - return PTR_ERR(hw->base[i]); > > - } > > + hw->nbase = hw->soc->nbase_names; > > + } else { > > + for (get_base_pass = 0; get_base_pass < 2; get_base_pass++) { > > + for (i = 0;; i++) { > > + node = of_parse_phandle(np, "reg_bases", i); > > + if (!node) > > + break; > > + if (get_base_pass == 1) > > + hw->base[i] = of_iomap(node, 0); > > + of_node_put(node); > > + } > > > > - hw->nbase = hw->soc->nbase_names; > > + if (i == 0) > > + return -EINVAL; > > + if (get_base_pass == 0) { > > + hw->nbase = i; > > + hw->base = devm_kmalloc_array(&pdev->dev, i, > > + sizeof(*hw->base), > > + GFP_KERNEL | __GFP_ZERO); > > + if (IS_ERR(hw->base)) > > + return PTR_ERR(hw->base); > > + } > > + } > > + } > > > > err = mtk_pctrl_build_state(pdev); > > if (err) { > > -- > > 1.8.1.1.dirty > >
Hi, Light
I knew your idea, but the patch really depends on a separate patch
about the update of the corresponding dt-binding document for your
newly added properties. You can find out dt-binding document
pinctrl-mt8183.txt in linux-pinctrl.git branch for-next that is just
being merged two weeks ago and can be reused for all drivers using the
same base.
So, to keep the work moving on, we need a v2 including the change on
dt-binding document and the patch and then see some inputs from Rob
and Matthias.
Sean
On Mon, Apr 15, 2019 at 11:40 PM Light Hsieh <light.hsieh@mediatek.com> wrote:
>
> Hi, Sean:
>
> On Sun, 2019-04-14 at 16:01 -0700, Sean Wang wrote:
> > Hi, Light
> >
> > On Thu, Apr 11, 2019 at 8:15 PM Light Hsieh <light.hsieh@mediatek.com> wrote:
> > >
> > > The orginal PINCTRL_MTK_PARIS/PINCTRL_MTK_MOORE need more effort for
> > > specifying register bases when porting platform driver:
> > > 1. Write mtXXXX_pinctrl_register_base_name[] array in pinctrl-mtXXXX.c
> > > to specify names of register bases, for exmaple:
> > >
> > > static const char * const mt6765_pinctrl_register_base_names[] = {
> > > "iocfg0", "iocfg1", "iocfg2", "iocfg3", "iocfg4", "iocfg5",
> > > "iocfg6", "iocfg7",
> > > };
> > > 2. Write reg = <...>, ..., <...>; in mtXXXX.dts to specify register
> > > bases. Each member of reg contain address range cloned from
> > > pre-generated devicetree node.
> > > 3. Write reg-names = "...", ..., "..."; in mtXXXX.dts to specify
> > > names of register bases. The sequence of names in reg-names shall match
> > > sequence of names that specified in pinctrl-mtXXXX.c.
> > > Besides, the seqeunce of names in reg-names shall also match sequence of
> > > address range in reg, for exmaple:
> > >
> > > pio: pinctrl {
> > > compatible = "mediatek,mt6765-pinctrl";
> > > reg = <0 0x10005000 0 0x1000>,
> > > <0 0x10002C00 0 0x200>,
> > > <0 0x10002800 0 0x200>,
> > > <0 0x10002A00 0 0x200>,
> > > <0 0x10002000 0 0x200>,
> > > <0 0x10002200 0 0x200>,
> > > <0 0x10002400 0 0x200>,
> > > <0 0x10002600 0 0x200>,
> > > <0 0x1000b000 0 0x1000>;
> > > reg-names = "iocfg0", "iocfg1", "iocfg2", "iocfg3",
> > > "iocfg4", "iocfg5", "iocfg6", "iocfg7",
> > > "eint";
> > >
> > > To reduce porting effort, this patch add an alternative way for specifying
> > > register bases:
> > > 1. Write reg_bases = <...>, ..., <...>; and reg_base_eint = <&eint>;
> > > in mtXXXX.dtsi where members in reg_bases and &eint are labels for
> > > pre-generated devicetree nodes, for example:
> > > pio: pinctrl {
> > > compatible = "mediatek,mt6765-pinctrl";
> > > reg_bases = <&gpio0>,
> > > <&iocfg0>,
> > > <&iocfg1>,
> > > <&iocfg2>,
> > > <&iocfg3>,
> > > <&iocfg4>,
> > > <&iocfg5>,
> > > <&iocfg6>,
> > > <&iocfg7>;
> > > reg_base_eint = <&eint>;
> >
> > reg and reg-names both are generic properties used in all of DT-based
> > device and driver, described in
> > Documentation/devicetree/bindings/resource-names.txt, but reg_based
> > and reg_base_eint aren't.
> >
> > If these properties are not hardware specific related, I personally
> > will encourage reusing those generic properties and relevant helpers
> > because those generic properties handling in the base driver are
> > almost bug-free, well maintained and even keep be extended in the
> > future. This way can help driver people put more concentration on
> > hardware specific stuff in the driver.
> >
>
> This modification is somewhat like mtk pinctrl driver V1. In V1,
> you may have the following devicetree (refer to
> Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt):
> :
> syscfg_pctl_a: syscfg-pctl-a@10005000 {
> compatible = "mediatek,mt8135-pctl-a-syscfg", "syscon";
> reg = <0 0x10005000 0 0x1000>;
> };
>
> syscfg_pctl_b: syscfg-pctl-b@1020c020 {
> compatible = "mediatek,mt8135-pctl-b-syscfg", "syscon";
> reg = <0 0x1020C020 0 0x1000>;
> };
>
> pinctrl@1c20800 {
> compatible = "mediatek,mt8135-pinctrl";
> mediatek,pctl-regmap = <&syscfg_pctl_a &syscfg_pctl_b>;
>
>
> and have following code in pinctrl-mtk-common.c:
> node = of_parse_phandle(np, "mediatek,pctl-regmap", 0);
> node = of_parse_phandle(np, "mediatek,pctl-regmap", 1);
>
> "mediatek, pctl-regmap" is self-defined devicetree property only used
> by mtk pinctrl drver V1.
>
> Now, I use similar logic with another property name.So I don't think
> the can add risk.
> Besides, since this way reduce porting effort and avoid human-error
> when writing devicetree and c code about device tree porting, it also
> help driver people put more concentration on hardware specific stuff in
> the driver.
>
> > >
> > > Since this pre-generated nodes had already specify address range,
> > > it is not necessary to specify address range again in pinctrl node.
> > >
> > > Using this way, porting effort is reduced and therefore typo can occur with
> > > less chance.
> > >
> > > Change-Id: I55f5e328919f4f736ca4b9f8d1593e069f179637
> > > ---
> > > drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 19 +++++---
> > > drivers/pinctrl/mediatek/pinctrl-paris.c | 62 ++++++++++++++++--------
> > > 2 files changed, 54 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> > > index b1c3684..16b4863 100644
> > > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> > > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> > > @@ -12,6 +12,7 @@
> > > #include <linux/platform_device.h>
> > > #include <linux/io.h>
> > > #include <linux/of_irq.h>
> > > +#include <linux/of_address.h>
> > >
> > > #include "mtk-eint.h"
> > > #include "pinctrl-mtk-common-v2.h"
> > > @@ -310,7 +311,7 @@ static int mtk_xt_set_gpio_as_eint(void *data, unsigned long eint_n)
> > >
> > > int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev)
> > > {
> > > - struct device_node *np = pdev->dev.of_node;
> > > + struct device_node *np = pdev->dev.of_node, *node;
> > > struct resource *res;
> > >
> > > if (!IS_ENABLED(CONFIG_EINT_MTK))
> > > @@ -323,13 +324,19 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev)
> > > if (!hw->eint)
> > > return -ENOMEM;
> > >
> > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "eint");
> > > - if (!res) {
> > > - dev_err(&pdev->dev, "Unable to get eint resource\n");
> > > - return -ENODEV;
> > > + node = of_parse_phandle(np, "reg_base_eint", 0);
> > > + if (node) {
> > > + hw->eint->base = of_iomap(node, 0);
> > > + } else {
> > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > + "eint");
> > > + if (!res) {
> > > + dev_err(&pdev->dev, "Unable to get eint resource\n");
> > > + return -ENODEV;
> > > + }
> > > + hw->eint->base = devm_ioremap_resource(&pdev->dev, res);
> > > }
> > >
> > > - hw->eint->base = devm_ioremap_resource(&pdev->dev, res);
> > > if (IS_ERR(hw->eint->base))
> > > return PTR_ERR(hw->eint->base);
> > >
> > > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> > > index b59e108..8ddb995 100644
> > > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> > > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> > > @@ -9,6 +9,7 @@
> > > * Hongzhou.Yang <hongzhou.yang@mediatek.com>
> > > */
> > >
> > > +#include <linux/of_address.h>
> > > #include <linux/gpio/driver.h>
> > > #include <dt-bindings/pinctrl/mt65xx.h>
> > > #include "pinctrl-paris.h"
> > > @@ -815,10 +816,11 @@ static int mtk_pctrl_build_state(struct platform_device *pdev)
> > > int mtk_paris_pinctrl_probe(struct platform_device *pdev,
> > > const struct mtk_pin_soc *soc)
> > > {
> > > + struct device_node *np = pdev->dev.of_node, *node;
> > > struct pinctrl_pin_desc *pins;
> > > struct mtk_pinctrl *hw;
> > > struct resource *res;
> > > - int err, i;
> > > + int err, i, get_base_pass;
> > >
> > > hw = devm_kzalloc(&pdev->dev, sizeof(*hw), GFP_KERNEL);
> > > if (!hw)
> > > @@ -828,31 +830,49 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev,
> > > hw->soc = soc;
> > > hw->dev = &pdev->dev;
> > >
> > > - if (!hw->soc->nbase_names) {
> > > - dev_err(&pdev->dev,
> > > - "SoC should be assigned at least one register base\n");
> > > - return -EINVAL;
> > > - }
> > > -
> > > - hw->base = devm_kmalloc_array(&pdev->dev, hw->soc->nbase_names,
> > > + if (hw->soc->nbase_names) {
> > > + hw->base = devm_kmalloc_array(&pdev->dev, hw->soc->nbase_names,
> > > sizeof(*hw->base), GFP_KERNEL);
> > > - if (!hw->base)
> > > - return -ENOMEM;
> > > + if (!hw->base)
> > > + return -ENOMEM;
> > > +
> > > + for (i = 0; i < hw->soc->nbase_names; i++) {
> > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > + hw->soc->base_names[i]);
> > > + if (!res) {
> > > + dev_err(&pdev->dev, "missing IO resource\n");
> > > + return -ENXIO;
> > > + }
> > >
> > > - for (i = 0; i < hw->soc->nbase_names; i++) {
> > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > - hw->soc->base_names[i]);
> > > - if (!res) {
> > > - dev_err(&pdev->dev, "missing IO resource\n");
> > > - return -ENXIO;
> > > + hw->base[i] = devm_ioremap_resource(&pdev->dev, res);
> > > + if (IS_ERR(hw->base[i]))
> > > + return PTR_ERR(hw->base[i]);
> > > }
> > >
> > > - hw->base[i] = devm_ioremap_resource(&pdev->dev, res);
> > > - if (IS_ERR(hw->base[i]))
> > > - return PTR_ERR(hw->base[i]);
> > > - }
> > > + hw->nbase = hw->soc->nbase_names;
> > > + } else {
> > > + for (get_base_pass = 0; get_base_pass < 2; get_base_pass++) {
> > > + for (i = 0;; i++) {
> > > + node = of_parse_phandle(np, "reg_bases", i);
> > > + if (!node)
> > > + break;
> > > + if (get_base_pass == 1)
> > > + hw->base[i] = of_iomap(node, 0);
> > > + of_node_put(node);
> > > + }
> > >
> > > - hw->nbase = hw->soc->nbase_names;
> > > + if (i == 0)
> > > + return -EINVAL;
> > > + if (get_base_pass == 0) {
> > > + hw->nbase = i;
> > > + hw->base = devm_kmalloc_array(&pdev->dev, i,
> > > + sizeof(*hw->base),
> > > + GFP_KERNEL | __GFP_ZERO);
> > > + if (IS_ERR(hw->base))
> > > + return PTR_ERR(hw->base);
> > > + }
> > > + }
> > > + }
> > >
> > > err = mtk_pctrl_build_state(pdev);
> > > if (err) {
> > > --
> > > 1.8.1.1.dirty
> > >
>
>
diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c index b1c3684..16b4863 100644 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c @@ -12,6 +12,7 @@ #include <linux/platform_device.h> #include <linux/io.h> #include <linux/of_irq.h> +#include <linux/of_address.h> #include "mtk-eint.h" #include "pinctrl-mtk-common-v2.h" @@ -310,7 +311,7 @@ static int mtk_xt_set_gpio_as_eint(void *data, unsigned long eint_n) int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) { - struct device_node *np = pdev->dev.of_node; + struct device_node *np = pdev->dev.of_node, *node; struct resource *res; if (!IS_ENABLED(CONFIG_EINT_MTK)) @@ -323,13 +324,19 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) if (!hw->eint) return -ENOMEM; - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "eint"); - if (!res) { - dev_err(&pdev->dev, "Unable to get eint resource\n"); - return -ENODEV; + node = of_parse_phandle(np, "reg_base_eint", 0); + if (node) { + hw->eint->base = of_iomap(node, 0); + } else { + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "eint"); + if (!res) { + dev_err(&pdev->dev, "Unable to get eint resource\n"); + return -ENODEV; + } + hw->eint->base = devm_ioremap_resource(&pdev->dev, res); } - hw->eint->base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(hw->eint->base)) return PTR_ERR(hw->eint->base); diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c index b59e108..8ddb995 100644 --- a/drivers/pinctrl/mediatek/pinctrl-paris.c +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c @@ -9,6 +9,7 @@ * Hongzhou.Yang <hongzhou.yang@mediatek.com> */ +#include <linux/of_address.h> #include <linux/gpio/driver.h> #include <dt-bindings/pinctrl/mt65xx.h> #include "pinctrl-paris.h" @@ -815,10 +816,11 @@ static int mtk_pctrl_build_state(struct platform_device *pdev) int mtk_paris_pinctrl_probe(struct platform_device *pdev, const struct mtk_pin_soc *soc) { + struct device_node *np = pdev->dev.of_node, *node; struct pinctrl_pin_desc *pins; struct mtk_pinctrl *hw; struct resource *res; - int err, i; + int err, i, get_base_pass; hw = devm_kzalloc(&pdev->dev, sizeof(*hw), GFP_KERNEL); if (!hw) @@ -828,31 +830,49 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev, hw->soc = soc; hw->dev = &pdev->dev; - if (!hw->soc->nbase_names) { - dev_err(&pdev->dev, - "SoC should be assigned at least one register base\n"); - return -EINVAL; - } - - hw->base = devm_kmalloc_array(&pdev->dev, hw->soc->nbase_names, + if (hw->soc->nbase_names) { + hw->base = devm_kmalloc_array(&pdev->dev, hw->soc->nbase_names, sizeof(*hw->base), GFP_KERNEL); - if (!hw->base) - return -ENOMEM; + if (!hw->base) + return -ENOMEM; + + for (i = 0; i < hw->soc->nbase_names; i++) { + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, + hw->soc->base_names[i]); + if (!res) { + dev_err(&pdev->dev, "missing IO resource\n"); + return -ENXIO; + } - for (i = 0; i < hw->soc->nbase_names; i++) { - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, - hw->soc->base_names[i]); - if (!res) { - dev_err(&pdev->dev, "missing IO resource\n"); - return -ENXIO; + hw->base[i] = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(hw->base[i])) + return PTR_ERR(hw->base[i]); } - hw->base[i] = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(hw->base[i])) - return PTR_ERR(hw->base[i]); - } + hw->nbase = hw->soc->nbase_names; + } else { + for (get_base_pass = 0; get_base_pass < 2; get_base_pass++) { + for (i = 0;; i++) { + node = of_parse_phandle(np, "reg_bases", i); + if (!node) + break; + if (get_base_pass == 1) + hw->base[i] = of_iomap(node, 0); + of_node_put(node); + } - hw->nbase = hw->soc->nbase_names; + if (i == 0) + return -EINVAL; + if (get_base_pass == 0) { + hw->nbase = i; + hw->base = devm_kmalloc_array(&pdev->dev, i, + sizeof(*hw->base), + GFP_KERNEL | __GFP_ZERO); + if (IS_ERR(hw->base)) + return PTR_ERR(hw->base); + } + } + } err = mtk_pctrl_build_state(pdev); if (err) {
The orginal PINCTRL_MTK_PARIS/PINCTRL_MTK_MOORE need more effort for specifying register bases when porting platform driver: 1. Write mtXXXX_pinctrl_register_base_name[] array in pinctrl-mtXXXX.c to specify names of register bases, for exmaple: static const char * const mt6765_pinctrl_register_base_names[] = { "iocfg0", "iocfg1", "iocfg2", "iocfg3", "iocfg4", "iocfg5", "iocfg6", "iocfg7", }; 2. Write reg = <...>, ..., <...>; in mtXXXX.dts to specify register bases. Each member of reg contain address range cloned from pre-generated devicetree node. 3. Write reg-names = "...", ..., "..."; in mtXXXX.dts to specify names of register bases. The sequence of names in reg-names shall match sequence of names that specified in pinctrl-mtXXXX.c. Besides, the seqeunce of names in reg-names shall also match sequence of address range in reg, for exmaple: pio: pinctrl { compatible = "mediatek,mt6765-pinctrl"; reg = <0 0x10005000 0 0x1000>, <0 0x10002C00 0 0x200>, <0 0x10002800 0 0x200>, <0 0x10002A00 0 0x200>, <0 0x10002000 0 0x200>, <0 0x10002200 0 0x200>, <0 0x10002400 0 0x200>, <0 0x10002600 0 0x200>, <0 0x1000b000 0 0x1000>; reg-names = "iocfg0", "iocfg1", "iocfg2", "iocfg3", "iocfg4", "iocfg5", "iocfg6", "iocfg7", "eint"; To reduce porting effort, this patch add an alternative way for specifying register bases: 1. Write reg_bases = <...>, ..., <...>; and reg_base_eint = <&eint>; in mtXXXX.dtsi where members in reg_bases and &eint are labels for pre-generated devicetree nodes, for example: pio: pinctrl { compatible = "mediatek,mt6765-pinctrl"; reg_bases = <&gpio0>, <&iocfg0>, <&iocfg1>, <&iocfg2>, <&iocfg3>, <&iocfg4>, <&iocfg5>, <&iocfg6>, <&iocfg7>; reg_base_eint = <&eint>; Since this pre-generated nodes had already specify address range, it is not necessary to specify address range again in pinctrl node. Using this way, porting effort is reduced and therefore typo can occur with less chance. Change-Id: I55f5e328919f4f736ca4b9f8d1593e069f179637 --- drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 19 +++++--- drivers/pinctrl/mediatek/pinctrl-paris.c | 62 ++++++++++++++++-------- 2 files changed, 54 insertions(+), 27 deletions(-)