| Message ID | 1535987232-5588-1-git-send-email-georgii.staroselskii@emlid.com |
|---|---|
| Headers | show |
| Series | Add a pinctrl driver for Merrifield to pinmux I2C#6 | expand |
On Mon, Sep 3, 2018 at 7:41 PM Georgii Staroselskii <georgii.staroselskii@emlid.com> wrote: > > Now that we have the pinctrl driver for Merrifield in place we can make > use of it and set I2C#6 pins appropriately. > > Initial configuration came from the firmware. Which quite likely has > been used in the phones, where that is not part of Atom peripheral, is > in use. Thus we need to override the leftover. > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com> > --- > arch/x86/dts/edison.dts | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/x86/dts/edison.dts b/arch/x86/dts/edison.dts > index 1da7f54..0dd7240 100644 > --- a/arch/x86/dts/edison.dts > +++ b/arch/x86/dts/edison.dts > @@ -85,4 +85,26 @@ > compatible = "intel,reset-tangier"; > u-boot,dm-pre-reloc; > }; > + > + pinctrl { > + compatible = "intel,pinctrl-tangier"; > + reg = <0xff0c0000 0x8000>; > + > + /* > + * Initial configuration came from the firmware. > + * Which quite likely has been used in the phones, where I2C #8, > + * that is not part of Atom peripheral, is in use. > + * Thus we need to override the leftover. > + */ > + i2c6_scl@0 { > + pad-offset = <111>; > + mode-func = <1>; > + protected; > + }; > + i2c6_sda@0 { > + pad-offset = <112>; > + mode-func = <1>; > + protected; > + }; > + }; > }; > -- > 2.7.4 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On Mon, Sep 3, 2018 at 7:38 PM Georgii Staroselskii <georgii.staroselskii@emlid.com> wrote: > > Now that we have I2C#6 working, it's time to add a corresponsing > ACPI binding. > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com> > --- > arch/x86/include/asm/arch-tangier/acpi/southcluster.asl | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl > index b200e9f..7cdc4b2 100644 > --- a/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl > +++ b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl > @@ -231,6 +231,16 @@ Device (PCI0) > } > } > > + Device (I2C6) > + { > + Name (_ADR, 0x00090001) > + > + Method (_STA, 0, NotSerialized) > + { > + Return (STA_VISIBLE) > + } > + } > + > Device (GPIO) > { > Name (_ADR, 0x000c0000) > -- > 2.7.4 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On Mon, Sep 3, 2018 at 7:39 PM Georgii Staroselskii <georgii.staroselskii@emlid.com> wrote: > > This API is going to be used to configure some pins that are protected > for simple modification. > > It's not a comprehensive pinctrl driver but can be turned into one > when we need this in the future. Now it is planned to be used only > in one place. So that's why I decided not to polute the codebase with a polute -> pollute > full-blown pinctrl-merrifield nobody will use. > > This driver reads corresponding fields in DT and configures pins > accordingly. > > The "protected" flag is used to distinguish configuration of SCU-owned > pins from the ordinary ones. > > The code has been adapted from Linux work done by Andy Shevchenko > in pinctrl-merrfifield.c > +static struct mrfld_family mrfld_families[] = { > + MRFLD_FAMILY(7, 101, 114), > +}; This perhaps needs a comment that for now we are only serve I2C Family of pins. > +static const struct mrfld_family * > +mrfld_get_family(struct mrfld_pinctrl *mp, unsigned int pin) > +{ > + const struct mrfld_family *family; > + unsigned int i; > + > + for (i = 0; i < mp->nfamilies; i++) { > + family = &mp->families[i]; > + if (pin >= family->pin_base && > + pin < family->pin_base + family->npins) Can it be one line? > + return family; > + } > + printf("failed to find family for pin %u\n", pin); I think it should be an error message (I don't remember which function is good for that, pr_err() maybe?). > + return NULL; > +} > + return ret; > +} Missed blank line here. > +static int mrfld_pinctrl_cfg_pin(int pin_node) > +{ > + is_protected = fdtdec_get_bool(gd->fdt_blob, pin_node, "protected"); > + if (!is_protected) > + return -ENOTSUPP; Similar comment perhaps needed, that it's for now we support just protected Family of pins, i.e. I2C one. > + if (mode != 1) > + return -ENOTSUPP; I don't think we need to limit to mode 1. Rather to check for mask (if mode is in a range 0..7). > + u32 mask = MRFLD_PINMODE_MASK; I'm not sure mixing definitions with code is a good idea. > + ret = mrfld_pinconfig_protected(pad_offset, mask, mode); > + > + return ret; > +} > + > +static int tangier_pinctrl_probe(struct udevice *dev) > +{ > + void *base_addr = syscon_get_first_range(X86_SYSCON_PINCONF); > + struct mrfld_pinctrl *pinctrl = dev_get_priv(dev); > + int pin_node; > + int ret; > + > + mrfld_setup_families(base_addr, mrfld_families, > + ARRAY_SIZE(mrfld_families)); > + > + pinctrl->families = mrfld_families; > + pinctrl->nfamilies = ARRAY_SIZE(mrfld_families); > + > + for (pin_node = fdt_first_subnode(gd->fdt_blob, dev_of_offset(dev)); > + pin_node > 0; > + pin_node = fdt_next_subnode(gd->fdt_blob, pin_node)) { > + ret = mrfld_pinctrl_cfg_pin(pin_node); > + if (ret != 0) { Simple if (ret) should work. > + pr_err("%s: invalid configuration for the pin %d\n", > + __func__, pin_node); > + } > + } > + > + return 0; > +} > + > +static const struct udevice_id tangier_pinctrl_match[] = { > + { .compatible = "intel,pinctrl-tangier", .data = X86_SYSCON_PINCONF }, > + { /* sentinel */ } > +}; Side note: I don't know for sure naming standards for compatible strings, I guess this one is correct. > +U_BOOT_DRIVER(tangier_pinctrl) = { > + .name = "tangier_pinctrl", > + .id = UCLASS_SYSCON, > + .of_match = tangier_pinctrl_match, > + .probe = tangier_pinctrl_probe, > + .priv_auto_alloc_size = sizeof(struct mrfld_pinctrl), > +};
On Mon, Sep 3, 2018 at 9:26 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > Thanks for doing this! > > I'll review patches individually. > By some reason Gmail doesn't show me Simon's and Bin's emails in the > message. Are they delivered? I reviewed, looks pretty much good, except few small comments in patch 2. So, your next steps would be: - wait for Bin and / or Simon reacting on this to see if we have everything good with mail transport (might be just some Gmail craziness) - address my comments - wait a bit (one day or so) to give time for others to look at - address (meanwhile) comments from me and others if any - send a new (v2) version of the series (don't forget to incorporate tags I gave into your commit message) -- With Best Regards, Andy Shevchenko
On Tue, Sep 4, 2018 at 2:23 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Sep 3, 2018 at 7:41 PM Georgii Staroselskii > <georgii.staroselskii@emlid.com> wrote: > > > > Now that we have the pinctrl driver for Merrifield in place we can make > > use of it and set I2C#6 pins appropriately. > > > > Initial configuration came from the firmware. Which quite likely has > > been used in the phones, where that is not part of Atom peripheral, is > > in use. Thus we need to override the leftover. > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com> > > --- > > arch/x86/dts/edison.dts | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>