Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface
On Tue, Nov 24, 2020 at 09:05:52PM +0900, Bongsu Jeon wrote:
> On Mon, Nov 23, 2020 at 5:55 PM k...@kernel.org wrote:
> > > +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy = phy_id;
> > > + enum s3fwrn5_mode mode;
> > > +
> > > + mutex_lock(>mutex);
> > > + mode = phy->mode;
> > > + mutex_unlock(>mutex);
> > > + return mode;
> > > +}
> >
> > All this duplicates I2C version. You need to start either reusing common
> > blocks.
> >
>
> Okay. I will do refactoring on i2c.c and uart.c to make common blocks.
> is it okay to separate a patch for it?
Yes, that would be the best - refactor the driver to split some common
methods and then in next patch add new s3fwrn82 UART driver.
> > > +
> > > +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy = phy_id;
> > > + int err;
> > > +
> > > + err = serdev_device_write(phy->ser_dev,
> > > + out->data, out->len,
> > > + MAX_SCHEDULE_TIMEOUT);
> > > + if (err < 0)
> > > + return err;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct s3fwrn5_phy_ops uart_phy_ops = {
> > > + .set_wake = s3fwrn82_uart_set_wake,
> > > + .set_mode = s3fwrn82_uart_set_mode,
> > > + .get_mode = s3fwrn82_uart_get_mode,
> > > + .write = s3fwrn82_uart_write,
> > > +};
> > > +
> > > +static int s3fwrn82_uart_read(struct serdev_device *serdev,
> > > + const unsigned char *data,
> > > + size_t count)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> > > + size_t i;
> > > +
> > > + for (i = 0; i < count; i++) {
> > > + skb_put_u8(phy->recv_skb, *data++);
> > > +
> > > + if (phy->recv_skb->len < S3FWRN82_NCI_HEADER)
> > > + continue;
> > > +
> > > + if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER)
> > > + < phy->recv_skb->data[S3FWRN82_NCI_IDX])
> > > + continue;
> > > +
> > > + s3fwrn5_recv_frame(phy->ndev, phy->recv_skb, phy->mode);
> > > + phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
> > > + if (!phy->recv_skb)
> > > + return 0;
> > > + }
> > > +
> > > + return i;
> > > +}
> > > +
> > > +static struct serdev_device_ops s3fwrn82_serdev_ops = {
> >
> > const
> >
> > > + .receive_buf = s3fwrn82_uart_read,
> > > + .write_wakeup = serdev_device_write_wakeup,
> > > +};
> > > +
> > > +static const struct of_device_id s3fwrn82_uart_of_match[] = {
> > > + { .compatible = "samsung,s3fwrn82-uart", },
> > > + {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, s3fwrn82_uart_of_match);
> > > +
> > > +static int s3fwrn82_uart_parse_dt(struct serdev_device *serdev)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> > > + struct device_node *np = serdev->dev.of_node;
> > > +
> > > + if (!np)
> > > + return -ENODEV;
> > > +
> > > + phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> > > + if (!gpio_is_valid(phy->gpio_en))
> > > + return -ENODEV;
> > > +
> > > + phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> >
> > You should not cast it it unsigned int. I'll fix the s3fwrn5 from which
> > you copied this apparently.
> >
>
> Okay. I will fix it.
>
> > > + if (!gpio_is_valid(phy->gpio_fw_wake))
> > > + return -ENODEV;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int s3fwrn82_uart_probe(struct serdev_device *serdev)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy;
> > > + int ret = -ENOMEM;
> > > +
> > > + phy = devm_kzalloc(>dev, sizeof(*phy), GFP_KERNEL);
> > > + if (!phy)
&g
Re: [PATCH net-next 1/2] dt-bindings: net: nfc: s3fwrn5: Support a UART interface
On Tue, Nov 24, 2020 at 08:39:40PM +0900, Bongsu Jeon wrote:
> On Mon, Nov 23, 2020 at 5:02 PM k...@kernel.org wrote:
> >
> > On Mon, Nov 23, 2020 at 04:55:26PM +0900, Bongsu Jeon wrote:
> > examples:
> > >- |
> > > #include
> > > @@ -71,3 +81,17 @@ examples:
> > > wake-gpios = < 2 GPIO_ACTIVE_HIGH>;
> > > };
> > > };
> > > + # UART example on Raspberry Pi
> > > + - |
> > > + {
> > > +status = "okay";
> > > +
> > > +s3fwrn82_uart {
> >
> > Just "bluetooth" to follow Devicetree specification.
> Sorry. I don't understand this comment.
> Could you explain it?
> Does it mean i need to refer to the net/broadcom-bluetooth.txt?
The node name should be "bluetooth", not "s3fwrn82_uart", because of
Devicetree naming convention - node names should represent generic class
of a device.
Best regards,
Krzysztof
Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface
On Mon, Nov 23, 2020 at 04:56:58PM +0900, Bongsu Jeon wrote:
> Since S3FWRN82 NFC Chip, The UART interface can be used.
> S3FWRN82 uses NCI protocol and supports I2C and UART interface.
>
> Signed-off-by: Bongsu Jeon
Please start sending emails properly, e.g. with git send-email, so all
your patches in the patchset are referencing the first patch.
> ---
> drivers/nfc/s3fwrn5/Kconfig | 12 ++
> drivers/nfc/s3fwrn5/Makefile | 2 +
> drivers/nfc/s3fwrn5/uart.c | 250 +++
> 3 files changed, 264 insertions(+)
> create mode 100644 drivers/nfc/s3fwrn5/uart.c
>
> diff --git a/drivers/nfc/s3fwrn5/Kconfig b/drivers/nfc/s3fwrn5/Kconfig
> index 3f8b6da58280..6f88737769e1 100644
> --- a/drivers/nfc/s3fwrn5/Kconfig
> +++ b/drivers/nfc/s3fwrn5/Kconfig
> @@ -20,3 +20,15 @@ config NFC_S3FWRN5_I2C
> To compile this driver as a module, choose m here. The module will
> be called s3fwrn5_i2c.ko.
> Say N if unsure.
> +
> +config NFC_S3FWRN82_UART
> + tristate "Samsung S3FWRN82 UART support"
> + depends on NFC_NCI && SERIAL_DEV_BUS
What about SERIAL_DEV_BUS as module? Shouldn't this be
SERIAL_DEV_BUS || !SERIAL_DEV_BUS?
> + select NFC_S3FWRN5
> + help
> + This module adds support for a UART interface to the S3FWRN82 chip.
> + Select this if your platform is using the UART bus.
> +
> + To compile this driver as a module, choose m here. The module will
> + be called s3fwrn82_uart.ko.
> + Say N if unsure.
> diff --git a/drivers/nfc/s3fwrn5/Makefile b/drivers/nfc/s3fwrn5/Makefile
> index d0ffa35f50e8..d1902102060b 100644
> --- a/drivers/nfc/s3fwrn5/Makefile
> +++ b/drivers/nfc/s3fwrn5/Makefile
> @@ -5,6 +5,8 @@
>
> s3fwrn5-objs = core.o firmware.o nci.o
> s3fwrn5_i2c-objs = i2c.o
> +s3fwrn82_uart-objs = uart.o
>
> obj-$(CONFIG_NFC_S3FWRN5) += s3fwrn5.o
> obj-$(CONFIG_NFC_S3FWRN5_I2C) += s3fwrn5_i2c.o
> +obj-$(CONFIG_NFC_S3FWRN82_UART) += s3fwrn82_uart.o
> diff --git a/drivers/nfc/s3fwrn5/uart.c b/drivers/nfc/s3fwrn5/uart.c
> new file mode 100644
> index ..b3c36a5b28d3
> --- /dev/null
> +++ b/drivers/nfc/s3fwrn5/uart.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * UART Link Layer for S3FWRN82 NCI based Driver
> + *
> + * Copyright (C) 2020 Samsung Electronics
> + * Author: Bongsu Jeon
You copied a lot from existing i2c.c. Please keep also the original
copyrights.
> + * All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "s3fwrn5.h"
> +
> +#define S3FWRN82_UART_DRIVER_NAME "s3fwrn82_uart"
Remove the define, it is used only once.
> +#define S3FWRN82_NCI_HEADER 3
> +#define S3FWRN82_NCI_IDX 2
> +#define S3FWRN82_EN_WAIT_TIME 20
> +#define NCI_SKB_BUFF_LEN 258
> +
> +struct s3fwrn82_uart_phy {
> + struct serdev_device *ser_dev;
> + struct nci_dev *ndev;
> + struct sk_buff *recv_skb;
> +
> + unsigned int gpio_en;
> + unsigned int gpio_fw_wake;
> +
> + /* mutex is used to synchronize */
Please do not write obvious comments. Mutex is always used to
synchronize, what else is it for? Instead you must describe what exactly
is protected with mutex.
> + struct mutex mutex;
> + enum s3fwrn5_mode mode;
> +};
> +
> +static void s3fwrn82_uart_set_wake(void *phy_id, bool wake)
> +{
> + struct s3fwrn82_uart_phy *phy = phy_id;
> +
> + mutex_lock(>mutex);
> + gpio_set_value(phy->gpio_fw_wake, wake);
> + msleep(S3FWRN82_EN_WAIT_TIME);
> + mutex_unlock(>mutex);
> +}
> +
> +static void s3fwrn82_uart_set_mode(void *phy_id, enum s3fwrn5_mode mode)
> +{
> + struct s3fwrn82_uart_phy *phy = phy_id;
> +
> + mutex_lock(>mutex);
> + if (phy->mode == mode)
> + goto out;
> + phy->mode = mode;
> + gpio_set_value(phy->gpio_en, 1);
> + gpio_set_value(phy->gpio_fw_wake, 0);
> + if (mode == S3FWRN5_MODE_FW)
> + gpio_set_value(phy->gpio_fw_wake, 1);
> + if (mode != S3FWRN5_MODE_COLD) {
> + msleep(S3FWRN82_EN_WAIT_TIME);
> + gpio_set_value(phy->gpio_en, 0);
> + msleep(S3FWRN82_EN_WAIT_TIME);
> + }
> +out:
> + mutex_unlock(>mutex);
> +}
> +
> +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
> +{
> + struct s3fwrn82_uart_phy *phy = phy_id;
> + enum s3fwrn5_mode mode;
> +
> + mutex_lock(>mutex);
> + mode = phy->mode;
> + mutex_unlock(>mutex);
> + return mode;
> +}
All this duplicates I2C version. You need to start either reusing common
blocks.
> +
> +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
> +{
> + struct s3fwrn82_uart_phy *phy = phy_id;
> + int err;
> +
> + err = serdev_device_write(phy->ser_dev,
> + out->data, out->len,
> + MAX_SCHEDULE_TIMEOUT);
> + if (err < 0)
> + return err;
> +
> +
Re: [PATCH net-next 1/2] dt-bindings: net: nfc: s3fwrn5: Support a UART interface
On Mon, Nov 23, 2020 at 04:55:26PM +0900, Bongsu Jeon wrote:
> Since S3FWRN82 NFC Chip, The UART interface can be used.
> S3FWRN82 supports I2C and UART interface.
>
> Signed-off-by: Bongsu Jeon
> ---
> .../bindings/net/nfc/samsung,s3fwrn5.yaml | 28 +--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
> b/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
> index cb0b8a560282..37b3e5ae5681 100644
> --- a/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
> +++ b/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
> @@ -13,6 +13,7 @@ maintainers:
> properties:
>compatible:
> const: samsung,s3fwrn5-i2c
> +const: samsung,s3fwrn82-uart
This does not work, you need to use enum. Did you run at least
dt_bindings_check?
The compatible should be just "samsung,s3fwrn82". I think it was a
mistake in the first s3fwrn5 submission to add a interface to
compatible.
>
>en-gpios:
> maxItems: 1
> @@ -47,10 +48,19 @@ additionalProperties: false
> required:
>- compatible
>- en-gpios
> - - interrupts
> - - reg
>- wake-gpios
>
> +allOf:
> + - if:
> + properties:
> +compatible:
> + contains:
> +const: samsung,s3fwrn5-i2c
> +then:
> + required:
> +- interrupts
> +- reg
> +
> examples:
>- |
> #include
> @@ -71,3 +81,17 @@ examples:
> wake-gpios = < 2 GPIO_ACTIVE_HIGH>;
> };
> };
> + # UART example on Raspberry Pi
> + - |
> + {
> +status = "okay";
> +
> +s3fwrn82_uart {
Just "bluetooth" to follow Devicetree specification.
Best regards,
Krzysztof
Re: [PATCH net-next v2 1/3] nfc: s3fwrn5: Remove the max_payload
On Tue, Nov 17, 2020 at 10:16:11AM +0900, Bongsu Jeon wrote: > max_payload is unused. Why did you resend the patch ignoring my review? I already provided you with a tag, so you should include it. https://www.kernel.org/doc/html/latest/process/submitting-patches.html Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next v2 2/3] nfc: s3fwrn5: Fix the misspelling in a comment
On Tue, Nov 17, 2020 at 10:17:42AM +0900, Bongsu Jeon wrote: > stucture should be replaced by structure. > > Signed-off-by: Bongsu Jeon I already reviewed it. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next v2 3/3] nfc: s3fwrn5: Change the error code
On Tue, Nov 17, 2020 at 10:18:50AM +0900, Bongsu Jeon wrote: > ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP. > > Signed-off-by: Bongsu Jeon > --- > drivers/nfc/s3fwrn5/s3fwrn5.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) I already reviewed it. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next 2/3] nfc: s3fwrn5: Fix the misspelling in a comment
On Mon, Nov 16, 2020 at 10:17:55AM +0900, Bongsu Jeon wrote: > stucture should be replaced by structure. > > Signed-off-by: Bongsu Jeon > --- > drivers/nfc/s3fwrn5/firmware.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next 3/3] nfc: s3fwrn5: Change the error code
On Mon, Nov 16, 2020 at 10:19:50AM +0900, Bongsu Jeon wrote: > ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP. > > Signed-off-by: Bongsu Jeon > --- > drivers/nfc/s3fwrn5/s3fwrn5.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next 1/3] nfc: s3fwrn5: Remove the max_payload
On Mon, Nov 16, 2020 at 10:12:05AM +0900, Bongsu Jeon wrote: > max_payload is unused. > > Signed-off-by: Bongsu Jeon Please version your patches (this should be a v2) and describe changes between versions in changelog, either in cover letter or after --- separator. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
On Wed, Sep 09, 2020 at 08:57:36AM +, Vaittinen, Matti wrote:
> Hello Krzysztof,
>
> On Wed, 2020-09-09 at 10:17 +0200, k...@kernel.org wrote:
> > On Wed, Sep 09, 2020 at 06:30:44AM +, Vaittinen, Matti wrote:
> > > On Tue, 2020-09-08 at 16:59 +0200, Krzysztof Kozlowski wrote:
> > > > GPIO_ACTIVE_x flags are not correct in the context of interrupt
> > > > flags.
> > > > These are simple defines so they could be used in DTS but they
> > > > will
> > > > not
> > > > have the same meaning:
> > > > 1. GPIO_ACTIVE_HIGH = 0 = IRQ_TYPE_NONE
> > > > 2. GPIO_ACTIVE_LOW = 1 = IRQ_TYPE_EDGE_RISING
> > > >
> > > > Correct the interrupt flags, assuming the author of the code
> > > > wanted
> > > > some
> > > > logical behavior behind the name "ACTIVE_xxx", this is:
> > > > ACTIVE_LOW => IRQ_TYPE_LEVEL_LOW
> > > >
> > > > Signed-off-by: Krzysztof Kozlowski
> > >
> > > For BD70528:
> > > Acked-By: Matti Vaittinen
> > >
> > > > ---
> > > > Documentation/devicetree/bindings/mfd/act8945a.txt | 2
> > > > +-
> > > > Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml| 3
> > > > ++-
> > > > Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt | 2
> > > > +-
> > > > 3 files changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd70528-
> > > > pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd70528-
> > > > pmic.txt
> > > > index c3c02ce73cde..386eec06cf08 100644
> > > > --- a/Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
> > > > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
> > > > @@ -39,7 +39,7 @@ pmic: pmic@4b {
> > > > compatible = "rohm,bd70528";
> > > > reg = <0x4b>;
> > > > interrupt-parent = <>;
> > > > - interrupts = <29 GPIO_ACTIVE_LOW>;
> > > > + interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
> > >
> > > This is how it should have been from the beginning :) Thanks!
> >
> > I start to wonder now. It seems some boards do not configure a pull
> > up
> > there, so IRQ_TYPE_LEVEL_LOW is wrong - causes the line to stay in
> > low
> > state. But actually this maybe is a problem of missing pull up, not
> > the
> > IRQ flag?
>
> The BD70528 is designed so that it will use level active interrupts -
> and line is pulled down when IRQ is active. Thus the example should
> have IRQ_TYPE_LEVEL_LOW - and your fix is correct.
>
> After that being said - I can't comment on actual board using BD70528
> (or other ROHM ICs) - even less I can comment boards using other ICs.
>
> After that being said - it's not a rare mistake to configure level
> active IRQs to be triggered at edge - it actually works most of the
> time - untill they deadlock at the race of generating new IRQ between
> reading the status and acking the line... I've debugged way too many
> such cases...
>
> Anyways, for BD70528 DTS example your fix looks correct. Thanks.
Thanks. I found this error in multiple DTS files - most probably a copy
paste from example or from evalkit (e.g. imx8mm-evk.dts). The trouble is
that I don't have the schematics for them and at least in one hardware
(Variscite VAR-SOM-MX8M which I am using) it looks like logic got
reversed...
Best regards,
Krzysztof
Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
On Wed, Sep 09, 2020 at 06:30:44AM +, Vaittinen, Matti wrote:
>
> On Tue, 2020-09-08 at 16:59 +0200, Krzysztof Kozlowski wrote:
> > GPIO_ACTIVE_x flags are not correct in the context of interrupt
> > flags.
> > These are simple defines so they could be used in DTS but they will
> > not
> > have the same meaning:
> > 1. GPIO_ACTIVE_HIGH = 0 = IRQ_TYPE_NONE
> > 2. GPIO_ACTIVE_LOW = 1 = IRQ_TYPE_EDGE_RISING
> >
> > Correct the interrupt flags, assuming the author of the code wanted
> > some
> > logical behavior behind the name "ACTIVE_xxx", this is:
> > ACTIVE_LOW => IRQ_TYPE_LEVEL_LOW
> >
> > Signed-off-by: Krzysztof Kozlowski
>
> For BD70528:
> Acked-By: Matti Vaittinen
>
> > ---
> > Documentation/devicetree/bindings/mfd/act8945a.txt | 2 +-
> > Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml| 3 ++-
> > Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt | 2 +-
> > 3 files changed, 4 insertions(+), 3 deletions(-)
> >
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd70528-
> > pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd70528-
> > pmic.txt
> > index c3c02ce73cde..386eec06cf08 100644
> > --- a/Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
> > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
> > @@ -39,7 +39,7 @@ pmic: pmic@4b {
> > compatible = "rohm,bd70528";
> > reg = <0x4b>;
> > interrupt-parent = <>;
> > - interrupts = <29 GPIO_ACTIVE_LOW>;
> > + interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
>
> This is how it should have been from the beginning :) Thanks!
I start to wonder now. It seems some boards do not configure a pull up
there, so IRQ_TYPE_LEVEL_LOW is wrong - causes the line to stay in low
state. But actually this maybe is a problem of missing pull up, not the
IRQ flag?
Best regards,
Krzysztof
Re: [PATCH 03/16] arm64: dts: imx8mm-beacon-som.dtsi: Align regulator names with schema
On Tue, Aug 25, 2020 at 08:22:18AM +, Vaittinen, Matti wrote:
> Hello Krzysztof,
>
> On Tue, 2020-08-25 at 09:50 +0200, k...@kernel.org wrote:
> > On Tue, Aug 25, 2020 at 09:45:00AM +0200, k...@kernel.org wrote:
> > > On Tue, Aug 25, 2020 at 09:25:37AM +0200, k...@kernel.org wrote:
> > > > On Tue, Aug 25, 2020 at 06:51:33AM +, Vaittinen, Matti wrote:
> > > > > Hello Krzysztof,
> > > > >
> > > > > Just some questions - please ignore if I misunderstood the
> > > > > impact of
> > > > > the change.
> > > > >
> > > > > On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski wrote:
> > > > > > Device tree schema expects regulator names to be
> > > > > > lowercase. This
> > > > > > fixes
> > > > > > dtbs_check warnings like:
> > > > > >
> > > > > > arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml:
> > > > > > pmic@4b:
> > > > > > regulators:LDO1:regulator-name:0: 'LDO1' does not match
> > > > > > '^ldo[1-6]$'
> > > > > >
> > > > > > Signed-off-by: Krzysztof Kozlowski
> > > > > > ---
> > > > > > .../boot/dts/freescale/imx8mn-ddr4-evk.dts| 22
> > > > > > +--
> > > > > >
> > > > > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-
> > > > > > evk.dts
> > > > > > b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > > > index a1e5483dbbbe..299caed5d46e 100644
> > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > > > @@ -60,7 +60,7 @@
> > > > > >
> > > > > > regulators {
> > > > > > buck1_reg: BUCK1 {
> > > > > > - regulator-name = "BUCK1";
> > > > > > + regulator-name = "buck1";
> > > > >
> > > > > I am not against this change but I would expect seeing some
> > > > > other
> > > > > patches too? I guess this will change the regulator name in
> > > > > regulator
> > > > > core, right? So maybe I am mistaken but it looks to me this
> > > > > change is
> > > > > visible in suppliers, sysfs and debugfs too? Thus changing this
> > > > > sounds
> > > > > a bit like asking for a nose bleed :) Am I right that the
> > > > > impact of
> > > > > this change has been thoroughly tested? Are there any other
> > > > > patches
> > > > > (that I have not seen) related to this change?
> > > >
> > > > Oh, crap, the names of regulators in the driver are lowercase,
> > > > but they
> > > > use of_match_ptr for upper case. Seriously, why making a binding
> > > > which
> > > > is contradictory to the driver implementation on the first day?
> > > >
> > > > The driver goes with binding, right? One expects uppercase, other
> > > > lowercase...
> > > >
> > > > And tell me, what is now the ABI? The binding or the incorrect
> > > > implementation?
> > >
> > > Wait, my mistake. I got confused by my own change. The node name
> > > stays
> > > the same, so of_match will be correct.
>
> Yes. I think so too. Match will still work as earler.
>
> > >
> > > The driver internally already uses lowercase names.
>
> Yep. I was simply thinking that if anyone has been specifying the
> regulators as suppliers by name - then this change will change things
> (as is seen for LDO5). Additionally, if any user-space SW has been
> reading the regulator states from sysfs - then these names will also
> change the sysfs. Debugfs change is hopefully not such a big deal.
About user-space, I think the embedded DT is not part of kernel ABI, so
there is no such requirement about keeping it stable. I agree though it
might be annoying surprise.
>
> Whether this really breaks anything is beyond my knowledge as I don't
> even have this board. Anyways, I think that by minimum the commit
> message should
Re: [PATCH 03/16] arm64: dts: imx8mm-beacon-som.dtsi: Align regulator names with schema
On Tue, Aug 25, 2020 at 09:45:00AM +0200, k...@kernel.org wrote:
> On Tue, Aug 25, 2020 at 09:25:37AM +0200, k...@kernel.org wrote:
> > On Tue, Aug 25, 2020 at 06:51:33AM +, Vaittinen, Matti wrote:
> > > Hello Krzysztof,
> > >
> > > Just some questions - please ignore if I misunderstood the impact of
> > > the change.
> > >
> > > On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski wrote:
> > > > Device tree schema expects regulator names to be lowercase. This
> > > > fixes
> > > > dtbs_check warnings like:
> > > >
> > > > arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
> > > > regulators:LDO1:regulator-name:0: 'LDO1' does not match '^ldo[1-6]$'
> > > >
> > > > Signed-off-by: Krzysztof Kozlowski
> > > > ---
> > > > .../boot/dts/freescale/imx8mn-ddr4-evk.dts| 22 +--
> > > >
> > > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > index a1e5483dbbbe..299caed5d46e 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > @@ -60,7 +60,7 @@
> > > >
> > > > regulators {
> > > > buck1_reg: BUCK1 {
> > > > - regulator-name = "BUCK1";
> > > > + regulator-name = "buck1";
> > >
> > > I am not against this change but I would expect seeing some other
> > > patches too? I guess this will change the regulator name in regulator
> > > core, right? So maybe I am mistaken but it looks to me this change is
> > > visible in suppliers, sysfs and debugfs too? Thus changing this sounds
> > > a bit like asking for a nose bleed :) Am I right that the impact of
> > > this change has been thoroughly tested? Are there any other patches
> > > (that I have not seen) related to this change?
> >
> > Oh, crap, the names of regulators in the driver are lowercase, but they
> > use of_match_ptr for upper case. Seriously, why making a binding which
> > is contradictory to the driver implementation on the first day?
> >
> > The driver goes with binding, right? One expects uppercase, other
> > lowercase...
> >
> > And tell me, what is now the ABI? The binding or the incorrect
> > implementation?
>
> Wait, my mistake. I got confused by my own change. The node name stays
> the same, so of_match will be correct.
>
> The driver internally already uses lowercase names.
>
> Everything looks good. I will just double check whether the constraints
> did not change on the board after boot.
Constraints look good.
>
> >
> > >
> > > > regulator-min-microvolt = <70>;
> > > > regulator-max-microvolt = <130>;
> > > > regulator-boot-on;
> > > > @@ -69,7 +69,7 @@
> > > > };
> > > >
> > > > buck2_reg: BUCK2 {
> > > > - regulator-name = "BUCK2";
> > > > + regulator-name = "buck2";
> > > > regulator-min-microvolt = <70>;
> > > > regulator-max-microvolt = <130>;
> > > > regulator-boot-on;
> > > > @@ -79,14 +79,14 @@
> > > >
> > > > buck3_reg: BUCK3 {
> > > > // BUCK5 in datasheet
> > > > - regulator-name = "BUCK3";
> > > > + regulator-name = "buck3";
> > > > regulator-min-microvolt = <70>;
> > > > regulator-max-microvolt = <135>;
> > > > };
> > > >
> > > > buck4_reg: BUCK4 {
> > > > // BUCK6 in datasheet
> > > > - regulator-name = "BUCK4";
> &
Re: [PATCH 03/16] arm64: dts: imx8mm-beacon-som.dtsi: Align regulator names with schema
On Tue, Aug 25, 2020 at 09:25:37AM +0200, k...@kernel.org wrote:
> On Tue, Aug 25, 2020 at 06:51:33AM +, Vaittinen, Matti wrote:
> > Hello Krzysztof,
> >
> > Just some questions - please ignore if I misunderstood the impact of
> > the change.
> >
> > On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski wrote:
> > > Device tree schema expects regulator names to be lowercase. This
> > > fixes
> > > dtbs_check warnings like:
> > >
> > > arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
> > > regulators:LDO1:regulator-name:0: 'LDO1' does not match '^ldo[1-6]$'
> > >
> > > Signed-off-by: Krzysztof Kozlowski
> > > ---
> > > .../boot/dts/freescale/imx8mn-ddr4-evk.dts| 22 +--
> > >
> > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > index a1e5483dbbbe..299caed5d46e 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > @@ -60,7 +60,7 @@
> > >
> > > regulators {
> > > buck1_reg: BUCK1 {
> > > - regulator-name = "BUCK1";
> > > + regulator-name = "buck1";
> >
> > I am not against this change but I would expect seeing some other
> > patches too? I guess this will change the regulator name in regulator
> > core, right? So maybe I am mistaken but it looks to me this change is
> > visible in suppliers, sysfs and debugfs too? Thus changing this sounds
> > a bit like asking for a nose bleed :) Am I right that the impact of
> > this change has been thoroughly tested? Are there any other patches
> > (that I have not seen) related to this change?
>
> Oh, crap, the names of regulators in the driver are lowercase, but they
> use of_match_ptr for upper case. Seriously, why making a binding which
> is contradictory to the driver implementation on the first day?
>
> The driver goes with binding, right? One expects uppercase, other
> lowercase...
>
> And tell me, what is now the ABI? The binding or the incorrect
> implementation?
Wait, my mistake. I got confused by my own change. The node name stays
the same, so of_match will be correct.
The driver internally already uses lowercase names.
Everything looks good. I will just double check whether the constraints
did not change on the board after boot.
>
> >
> > > regulator-min-microvolt = <70>;
> > > regulator-max-microvolt = <130>;
> > > regulator-boot-on;
> > > @@ -69,7 +69,7 @@
> > > };
> > >
> > > buck2_reg: BUCK2 {
> > > - regulator-name = "BUCK2";
> > > + regulator-name = "buck2";
> > > regulator-min-microvolt = <70>;
> > > regulator-max-microvolt = <130>;
> > > regulator-boot-on;
> > > @@ -79,14 +79,14 @@
> > >
> > > buck3_reg: BUCK3 {
> > > // BUCK5 in datasheet
> > > - regulator-name = "BUCK3";
> > > + regulator-name = "buck3";
> > > regulator-min-microvolt = <70>;
> > > regulator-max-microvolt = <135>;
> > > };
> > >
> > > buck4_reg: BUCK4 {
> > > // BUCK6 in datasheet
> > > - regulator-name = "BUCK4";
> > > + regulator-name = "buck4";
> > > regulator-min-microvolt = <300>;
> > > regulator-max-microvolt = <330>;
> > > regulator-boot-on;
> > > @@ -95,7 +95,7 @@
> > >
> > > buck5_reg: BUCK5 {
> > > // BUCK7 in datasheet
> > > - regulator-name = "BUCK5";
> > > + regulator-name = "buck5";
> >
> > What I see in bd718x7-regulator.c for LDO6 desc is:
> >
> > /* LDO6 is supplied by buck5 */
> > .supply_name = "buck5",
> >
> > So, is this change going to change the supply-chain for the board? Is
> > this intended? (Or am I mistaken on what is the impact of regulator-
> > name property?)
Good point, let me check the supplies.
>
> The names will take regulator names from the driver. The problem is with
> matching the of_node.
>
>
> Dear Rob,
>
> Maybe you have an idea how to fix this driver-binding ABI
> incompatibility? Or better just leave it?
Not valid anymore, I just got confused...
Best regards,
Krzysztof
Re: [PATCH 03/16] arm64: dts: imx8mm-beacon-som.dtsi: Align regulator names with schema
On Tue, Aug 25, 2020 at 06:51:33AM +, Vaittinen, Matti wrote:
> Hello Krzysztof,
>
> Just some questions - please ignore if I misunderstood the impact of
> the change.
>
> On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski wrote:
> > Device tree schema expects regulator names to be lowercase. This
> > fixes
> > dtbs_check warnings like:
> >
> > arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
> > regulators:LDO1:regulator-name:0: 'LDO1' does not match '^ldo[1-6]$'
> >
> > Signed-off-by: Krzysztof Kozlowski
> > ---
> > .../boot/dts/freescale/imx8mn-ddr4-evk.dts| 22 +--
> >
> > 1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > index a1e5483dbbbe..299caed5d46e 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > @@ -60,7 +60,7 @@
> >
> > regulators {
> > buck1_reg: BUCK1 {
> > - regulator-name = "BUCK1";
> > + regulator-name = "buck1";
>
> I am not against this change but I would expect seeing some other
> patches too? I guess this will change the regulator name in regulator
> core, right? So maybe I am mistaken but it looks to me this change is
> visible in suppliers, sysfs and debugfs too? Thus changing this sounds
> a bit like asking for a nose bleed :) Am I right that the impact of
> this change has been thoroughly tested? Are there any other patches
> (that I have not seen) related to this change?
Oh, crap, the names of regulators in the driver are lowercase, but they
use of_match_ptr for upper case. Seriously, why making a binding which
is contradictory to the driver implementation on the first day?
The driver goes with binding, right? One expects uppercase, other
lowercase...
And tell me, what is now the ABI? The binding or the incorrect
implementation?
>
> > regulator-min-microvolt = <70>;
> > regulator-max-microvolt = <130>;
> > regulator-boot-on;
> > @@ -69,7 +69,7 @@
> > };
> >
> > buck2_reg: BUCK2 {
> > - regulator-name = "BUCK2";
> > + regulator-name = "buck2";
> > regulator-min-microvolt = <70>;
> > regulator-max-microvolt = <130>;
> > regulator-boot-on;
> > @@ -79,14 +79,14 @@
> >
> > buck3_reg: BUCK3 {
> > // BUCK5 in datasheet
> > - regulator-name = "BUCK3";
> > + regulator-name = "buck3";
> > regulator-min-microvolt = <70>;
> > regulator-max-microvolt = <135>;
> > };
> >
> > buck4_reg: BUCK4 {
> > // BUCK6 in datasheet
> > - regulator-name = "BUCK4";
> > + regulator-name = "buck4";
> > regulator-min-microvolt = <300>;
> > regulator-max-microvolt = <330>;
> > regulator-boot-on;
> > @@ -95,7 +95,7 @@
> >
> > buck5_reg: BUCK5 {
> > // BUCK7 in datasheet
> > - regulator-name = "BUCK5";
> > + regulator-name = "buck5";
>
> What I see in bd718x7-regulator.c for LDO6 desc is:
>
> /* LDO6 is supplied by buck5 */
> .supply_name = "buck5",
>
> So, is this change going to change the supply-chain for the board? Is
> this intended? (Or am I mistaken on what is the impact of regulator-
> name property?)
The names will take regulator names from the driver. The problem is with
matching the of_node.
Dear Rob,
Maybe you have an idea how to fix this driver-binding ABI
incompatibility? Or better just leave it?
Best regards,
Krzysztof
Re: [PATCH 01/16] dt-bindings: mfd: rohm,bd71847-pmic: Correct clock properties requirements
On Tue, Aug 25, 2020 at 06:23:36AM +, Vaittinen, Matti wrote: > > Hello Krzysztof, > > On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski wrote: > > The input clock and number of clock provider cells are not required > > for > > the PMIC to operate. They are needed only for the optional bd718x7 > > clock driver. > > I have always found the DT bindings hard to do. I quite often end up > having a different view with Rob so I probably could just shut-up and > watch how this evolves :) > > But as keeping my mouth is so difficult... > > ...All of the drivers are optional. The PMIC can power-on without any > drivers. Drivers are mostly used just for disabling the voltage from > graphics accelerator block when it is not needed (optional). Or some > DVS (optional). But yes, maybe the clk driver is "more optional" than > the rest. XD So, I am not against this. Each regulator node is optional, it can be skipped. And device will work and regulator driver will bind. The difference here is that without clocks the clock driver won't even bind... but if we keep clocks as required, then multiple DTSes do not pass the bindings check. I don't have strong feelings about dropping requirement for clocks, just this looks easier to implement and logical to me (this is a PMIC so clock is a secondary feature). > > > Add also clock-output-names as driver takes use of it. > > > > This fixes dtbs_check warnings like: > > > > arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b: > > 'clocks' is a required property > > arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b: > > '#clock-cells' is a required property > > > > Signed-off-by: Krzysztof Kozlowski > > --- > > .../devicetree/bindings/mfd/rohm,bd71847-pmic.yaml | 9 > > +++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71847- > > pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71847- > > pmic.yaml > > index 77bcca2d414f..5d531051a153 100644 > > --- a/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml > > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml > > @@ -38,6 +38,9 @@ properties: > >"#clock-cells": > > const: 0 > > > > + clock-output-names: > > +maxItems: 1 > > I had this in original binding (text) document patch series. For some > reason it was later dropped. Unfortunately I didn't easily find a > reason as to why. Adding it back now is absolutely fine for me though. > > > + > > # The BD71847 abd BD71850 support two different HW states as reset > > target > > # states. States are called as SNVS and READY. At READY state all > > the PMIC > > # power outputs go down and OTP is reload. At the SNVS state all > > other logic > > @@ -116,12 +119,14 @@ required: > >- compatible > >- reg > >- interrupts > > - - clocks > > - - "#clock-cells" > >- regulators > > > > additionalProperties: false > > > > +dependencies: > > + '#clock-cells': [clocks] > > + clocks: ['#clock-cells'] > > This is new to me. Please educate me - does this simply mean that if > '#clock-cells' is given, then also the 'clocks' must be given - and the > other way around? Yes, because the clocks do not have sense without clock-cells and vice versa. > > If so, then: > Acked-By: Matti Vaittinen > Thanks. Best regards, Krzysztof
Re: [PATCH 05/10] ARM: dts: imx6ul-kontron-n6x1x: Add 'chosen' node with 'stdout-path'
On Wed, Oct 16, 2019 at 03:07:28PM +, Schrempf Frieder wrote: > From: Frieder Schrempf > > The Kontron N6x1x SoMs all use uart4 as a debug serial interface. > Therefore we set in the 'chosen' node. > > Signed-off-by: Frieder Schrempf > --- > arch/arm/boot/dts/imx6ul-kontron-n6x1x-som-common.dtsi | 6 ++ > 1 file changed, 6 insertions(+) Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH 09/10] dt-bindings: arm: fsl: Add more Kontron i.MX6UL/ULL compatibles
On Wed, Oct 16, 2019 at 03:07:36PM +, Schrempf Frieder wrote: > From: Frieder Schrempf > > Add the compatibles for Kontron i.MX6UL N6311 SoM and boards and > the compatibles for Kontron i.MX6ULL N6411 SoM and boards. > > Signed-off-by: Frieder Schrempf > --- > Documentation/devicetree/bindings/arm/fsl.yaml | 14 ++ > 1 file changed, 14 insertions(+) Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH 04/10] ARM: dts: Add support for two more Kontron evalkit boards 'N6311 S' and 'N6411 S'
On Wed, Oct 16, 2019 at 03:07:28PM +, Schrempf Frieder wrote: > From: Frieder Schrempf > > The 'N6311 S' and the 'N6411 S' are similar to the Kontron 'N6310 S' > evaluation kit boards. Instead of the N6310 SoM, they feature a N6311 > or N6411 SoM. > > Signed-off-by: Frieder Schrempf > --- > arch/arm/boot/dts/imx6ul-kontron-n6311-s.dts | 16 > arch/arm/boot/dts/imx6ull-kontron-n6411-s.dts | 16 > 2 files changed, 32 insertions(+) > create mode 100644 arch/arm/boot/dts/imx6ul-kontron-n6311-s.dts > create mode 100644 arch/arm/boot/dts/imx6ull-kontron-n6411-s.dts Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH 03/10] ARM: dts: imx6ul-kontron-n6310-s: Move common nodes to a separate file
On Wed, Oct 16, 2019 at 03:07:25PM +, Schrempf Frieder wrote:
> From: Frieder Schrempf
>
> The baseboard for the Kontron N6310 SoM is also used for other SoMs
> such as N6311 and N6411. In order to share the code, we move the
> definitions of the baseboard to a separate dtsi file.
>
> Signed-off-by: Frieder Schrempf
> ---
> arch/arm/boot/dts/imx6ul-kontron-n6310-s.dts | 405 +
> arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi | 412 ++
> 2 files changed, 413 insertions(+), 404 deletions(-)
> create mode 100644 arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>
> diff --git a/arch/arm/boot/dts/imx6ul-kontron-n6310-s.dts
> b/arch/arm/boot/dts/imx6ul-kontron-n6310-s.dts
> index 0205fd56d975..5a3e06d6219b 100644
> --- a/arch/arm/boot/dts/imx6ul-kontron-n6310-s.dts
> +++ b/arch/arm/boot/dts/imx6ul-kontron-n6310-s.dts
> @@ -8,413 +8,10 @@
> /dts-v1/;
>
> #include "imx6ul-kontron-n6310-som.dtsi"
> +#include "imx6ul-kontron-n6x1x-s.dtsi"
>
> / {
> model = "Kontron N6310 S";
> compatible = "kontron,imx6ul-n6310-s", "kontron,imx6ul-n6310-som",
>"fsl,imx6ul";
> -
> - gpio-leds {
> - compatible = "gpio-leds";
> - pinctrl-names = "default";
> - pinctrl-0 = <_gpio_leds>;
> -
> - led1 {
> - label = "debug-led1";
> - gpios = < 30 GPIO_ACTIVE_LOW>;
> - default-state = "off";
> - linux,default-trigger = "heartbeat";
> - };
> -
> - led2 {
> - label = "debug-led2";
> - gpios = < 3 GPIO_ACTIVE_LOW>;
> - default-state = "off";
> - };
> -
> - led3 {
> - label = "debug-led3";
> - gpios = < 2 GPIO_ACTIVE_LOW>;
> - default-state = "off";
> - };
> - };
> -
> - pwm-beeper {
> - compatible = "pwm-beeper";
> - pwms = < 0 5000>;
> - };
> -
> - reg_3v3: regulator-3v3 {
> - compatible = "regulator-fixed";
> - regulator-name = "3v3";
> - regulator-min-microvolt = <330>;
> - regulator-max-microvolt = <330>;
> - };
> -
> - reg_usb_otg1_vbus: regulator-usb-otg1-vbus {
> - compatible = "regulator-fixed";
> - regulator-name = "usb_otg1_vbus";
> - regulator-min-microvolt = <500>;
> - regulator-max-microvolt = <500>;
> - gpio = < 4 GPIO_ACTIVE_HIGH>;
> - enable-active-high;
> - };
> -
> - reg_vref_adc: regulator-vref-adc {
> - compatible = "regulator-fixed";
> - regulator-name = "vref-adc";
> - regulator-min-microvolt = <330>;
> - regulator-max-microvolt = <330>;
> - };
> -};
> -
> - {
> - pinctrl-names = "default";
> - pinctrl-0 = <_adc1>;
> - num-channels = <3>;
> - vref-supply = <_vref_adc>;
> - status = "okay";
> -};
> -
> - {
> - pinctrl-names = "default";
> - pinctrl-0 = <_flexcan2>;
> - status = "okay";
> -};
> -
> - {
> - cs-gpios = < 26 GPIO_ACTIVE_HIGH>;
> - pinctrl-names = "default";
> - pinctrl-0 = <_ecspi1>;
> - status = "okay";
> -
> - eeprom@0 {
> - compatible = "anvo,anv32e61w", "atmel,at25";
> - reg = <0>;
> - spi-max-frequency = <2000>;
> - spi-cpha;
> - spi-cpol;
> - pagesize = <1>;
> - size = <8192>;
> - address-width = <16>;
> - };
> -};
> -
> - {
> - pinctrl-0 = <_enet1>;
> - /delete-node/ mdio;
> -};
> -
> - {
> - pinctrl-names = "default";
> - pinctrl-0 = <_enet2 _enet2_mdio>;
> - phy-mode = "rmii";
> - phy-handle = <>;
> - status = "okay";
> -
> - mdio {
> - #address-cells = <1>;
> - #size-cells = <0>;
> -
> - ethphy1: ethernet-phy@1 {
> - reg = <1>;
> - micrel,led-mode = <0>;
> - clocks = < IMX6UL_CLK_ENET_REF>;
> - clock-names = "rmii-ref";
> - };
> -
> - ethphy2: ethernet-phy@2 {
> - reg = <2>;
> - micrel,led-mode = <0>;
> - clocks = < IMX6UL_CLK_ENET2_REF>;
> - clock-names = "rmii-ref";
> - };
> - };
> -};
> -
> - {
> - clock-frequency = <10>;
> - pinctrl-names = "default";
> - pinctrl-0 = <_i2c1>;
> - status = "okay";
> -};
> -
> - {
> - clock-frequency = <10>;
> - pinctrl-names = "default";
> - pinctrl-0 = <_i2c4>;
> - status = "okay";
> -
> - rtc@32 {
> - compatible = "epson,rx8900";
> - reg = <0x32>;
> - };
> -};
> -
> - {
> - pinctrl-names = "default";
> - pinctrl-0 = <_pwm8>;
> -
Re: [PATCH 02/10] ARM: dts: Add support for two more Kontron SoMs N6311 and N6411
On Wed, Oct 16, 2019 at 03:07:21PM +, Schrempf Frieder wrote: > From: Frieder Schrempf > > The N6311 and the N6411 SoM are similar to the Kontron N6310 SoM. > They are pin-compatible, but feature a larger RAM and NAND flash > (512MiB instead of 256MiB). Further, the N6411 has an i.MX6ULL SoC, > instead of an i.MX6UL. > > Signed-off-by: Frieder Schrempf > --- > .../boot/dts/imx6ul-kontron-n6311-som.dtsi| 40 +++ > .../boot/dts/imx6ull-kontron-n6411-som.dtsi | 40 +++ > 2 files changed, 80 insertions(+) > create mode 100644 arch/arm/boot/dts/imx6ul-kontron-n6311-som.dtsi > create mode 100644 arch/arm/boot/dts/imx6ull-kontron-n6411-som.dtsi > Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH 01/10] ARM: dts: imx6ul-kontron-n6310: Move common SoM nodes to a separate file
On Wed, Oct 16, 2019 at 03:07:19PM +, Schrempf Frieder wrote:
> From: Frieder Schrempf
>
> The Kontron N6311 and N6411 SoMs are very similar to N6310. In
> preparation to add support for them, we move the common nodes to a
> separate file imx6ul-kontron-n6x1x-som-common.dtsi.
>
> Signed-off-by: Frieder Schrempf
> ---
> .../boot/dts/imx6ul-kontron-n6310-som.dtsi| 95 +-
> .../dts/imx6ul-kontron-n6x1x-som-common.dtsi | 123 ++
> 2 files changed, 124 insertions(+), 94 deletions(-)
> create mode 100644 arch/arm/boot/dts/imx6ul-kontron-n6x1x-som-common.dtsi
>
> diff --git a/arch/arm/boot/dts/imx6ul-kontron-n6310-som.dtsi
> b/arch/arm/boot/dts/imx6ul-kontron-n6310-som.dtsi
> index a896b2348dd2..47d3ce5d255f 100644
> --- a/arch/arm/boot/dts/imx6ul-kontron-n6310-som.dtsi
> +++ b/arch/arm/boot/dts/imx6ul-kontron-n6310-som.dtsi
> @@ -6,7 +6,7 @@
> */
>
> #include "imx6ul.dtsi"
> -#include
> +#include "imx6ul-kontron-n6x1x-som-common.dtsi"
>
> / {
> model = "Kontron N6310 SOM";
> @@ -18,49 +18,7 @@
> };
> };
>
> - {
> - cs-gpios = < 22 GPIO_ACTIVE_HIGH>;
> - pinctrl-names = "default";
> - pinctrl-0 = <_ecspi2>;
> - status = "okay";
> -
> - spi-flash@0 {
> - compatible = "mxicy,mx25v8035f", "jedec,spi-nor";
> - spi-max-frequency = <5000>;
> - reg = <0>;
> - };
> -};
> -
> - {
> - pinctrl-names = "default";
> - pinctrl-0 = <_enet1 _enet1_mdio>;
> - phy-mode = "rmii";
> - phy-handle = <>;
> - status = "okay";
> -
> - mdio {
> - #address-cells = <1>;
> - #size-cells = <0>;
> -
> - ethphy1: ethernet-phy@1 {
> - reg = <1>;
> - micrel,led-mode = <0>;
> - clocks = < IMX6UL_CLK_ENET_REF>;
> - clock-names = "rmii-ref";
> - };
> - };
> -};
> -
> - {
> - phy-mode = "rmii";
> - status = "disabled";
> -};
> -
> {
> - pinctrl-names = "default";
> - pinctrl-0 = <_qspi>;
> - status = "okay";
> -
> spi-flash@0 {
You left qspi and flash partitions here, while adding it later. It is
not pure move then and some duplicated stuff remains.
Best regards,
Krzysztof
