* Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Andy Shevchenko @ 2024-04-02 16:39 UTC (permalink / raw)
To: Cristian Marussi
Cc: Peng Fan, Peng Fan (OSS), Sudeep Holla, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <ZgwrKnx3hb59OG77@pluto>
On Tue, Apr 2, 2024 at 6:58 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
> On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> > <cristian.marussi@arm.com> wrote:
> > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
...
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/scmi_protocol.h>
> > > > > > +#include <linux/slab.h>
> > > > >
> > > > > This is semi-random list of headers. Please, follow IWYU principle (include
> > > > > what you use). There are a lot of inclusions I see missing (just in the context of
> > > > > this page I see bits.h, types.h, and asm/byteorder.h).
> > > >
> > > > Is there any documentation about this requirement?
> > > > Some headers are already included by others.
> >
> > The documentation here is called "a common sense".
> > The C language is built like this and we expect that nobody will
> > invest into the dependency hell that we have already, that's why IWYU
> > principle, please follow it.
>
> Yes, but given that we have a growing number of SCMI protocols there is a
> common local protocols.h header to group all includes needed by any
> protocols: the idea behind this (and the devm_ saga down below) was to ease
> development of protocols, since there are lots of them and growing, given
> the SCMI spec is extensible.
Yes, and what you are effectively suggesting is: "Technical debt? Oh,
fine, we do not care!" This is not good. I'm in a long term of
cleaning up the dependency hell in the kernel (my main focus is
kernel.h for now) and I am talking from my experience. I do not like
what people are doing in 95% of the code, that's why I really want to
stop the bad practices as soon as possible.
Last to add, but not least is that your code may be used as an example
for others, hence we really have to do our best in order to avoid bad
design, practices, and cargo cults. If this requires more refactoring
of the existing code, then do it sooner than later.
...
> > > Andy made (mostly) the same remarks on this same patch ~1-year ago on
> > > this same patch while it was posted by Oleksii.
> > >
> > > And I told that time that most of the remarks around devm_ usage were
> > > wrong due to how the SCMI core handles protocol initialization (using a
> > > devres group transparently).
> > >
> > > This is what I answered that time.
> > >
> > > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
> > >
> > > I wont repeat myself, but, in a nutshell the memory allocation like it
> > > is now is fine: a bit happens via devm_ at protocol initialization, the
> > > other is doe via explicit kmalloc at runtime and freed via kfree at
> > > remove time (if needed...i.e. checking the present flag of some structs)
> >
> > This sounds like a mess. devm_ is expected to be used only for the
> > ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> > to have automatic free at the paths where memory is not needed.
>
> Indeed, this protocol_init code is called by the SCMI core once for all when
> an SCMI driver tries at first to use this specific protocol by 'getting' its
> protocol_ops, so it is indeed called inside the probe chain of the driver:
> at this point you *can* decide to use devres to allocate memory and be assured
> that if the init fails, or when the driver cease to use this protocol (calling
> its remove()) and no other driver is using it, all the stuff that have been
> allocated related to this protocol will be released by the core for you.
> (using an internal devres group)
>
> Without this you should handle manually all the deallocation manually on
> the init error-paths AND also provide all the cleanup explicitly when
> the protocol is no more used by any driver (multiple users of the same
> protocol instance are possible)...for all protocols.
Yes. Is it a problem?
> This is/was handy since, till now, all the SCMI querying and resources
> allocation happened anyway all at once at init time...
>
> ...the mess, as you kindly called it, derives from the fact that this specific
> protocol is the first and only one that does NOT allocate all that it needs
> during the initialization (to minimize needless allocs for a lot of possibly
> unused resources) and this lazy-initialization phase, done after init at runtime,
> must be handled manually since it cannot be managed by the devres group that is
> open/clsoed around init by the SCMI core.
>
> I dont like particularly this split allocation but it has a reason and any
> other solution seems more messy to me at the moment.
>
> And I dont feel like changing all the SCMI protocol initialziation core code
> (that address a lot more under the hood) is a desirable solution to address a
> non-existent problem really.
>
> > And the function naming doesn't suggest that you have a probe-remove
> > pair. Moreover, if the init-deinit part is called in the probe-remove,
> > the devm_ must not be mixed with non-devm ones, as it breaks the order
> > and leads to subtle mistakes.
>
> Initialization order is enforced by SCMI core like this:
>
> @driver_probe->get_protocol_ops()
> @core/get_protocol_ops
> -> devres_group_open()
> -> protocol_init->devm_*()
> -> devres_group_close()
> -> driver_probing
>
> @runtime optional explicit_lazy_kmallocs inside the protocol
>
> @driver_remove->put_protocol_ops()
> @core/put_protocol_ops()
> -> protocol_denit->optional_explicit_kfree_of_the_above
> -> devres_group_release()
> -> driver_removing
>
> ... dont think there's an ordering problem.
The mess with devm_ vs. non-devm is quite easy to achieve. You are
probably out of the control of what the protocol driver wants to do in
the init. Is the usage of devm (which APIs and in which order can be
used) WRT SCMI documented somewhere?
Misuse of devm is a common issue, I'm not surprised it will hit your
subsystem one day with such an approach.
> ...note that the ph->dev provided in the protocol_init and used by devm_
> is NOT the dev of the SCMI driver probe/remove that uses the get_protocol_ops,
> it is an internal SCMI device associated with the core SCMI stack probing and
> allocations, within which a devres group for the specific protocol is created
> when that specific protocol is initialized...protocols are not fully
> fledged drivers are just bits of the SCMI stack that are initialized when needed
> (and possibly also loaded when needed for vendor protocols) and
> de-initialzed when no more SCMI driver users exist for that protocol.
P.S. I guess from now on it's your call, but this code and in case
other drivers use similar, is badly written. I hope some documentation
exists to at least justify all this mess and explaining why there no
and (what's really important) will never be a problem.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 1/2] drm/bridge: lt8912b: add support for P/N pin swap
From: Francesco Dolcini @ 2024-04-02 16:53 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-kernel, dri-devel, devicetree, adrien.grassein,
andrzej.hajda, neil.armstrong, rfoss, Laurent.pinchart, jonas,
jernej.skrabec, airlied, daniel, maarten.lankhorst, mripard,
tzimmermann, robh, krzysztof.kozlowski+dt, conor+dt,
stefan.eichenberger, francesco.dolcini, marius.muresan,
irina.muresan
In-Reply-To: <20240402105925.905144-1-alex@shruggie.ro>
Hello Alexandru, thanks for your patch.
On Tue, Apr 02, 2024 at 01:59:24PM +0300, Alexandru Ardelean wrote:
> On some HW designs, it's easier for the layout if the P/N pins are swapped.
> In those cases, we need to adjust (for this) by configuring the MIPI analog
> registers differently. Specifically, register 0x3e needs to be 0xf6
> (instead of 0xd6).
>
> This change adds a 'lontium,pn-swap' device-tree property to configure the
> MIPI analog registers for P/N swap.
>
> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> ---
> drivers/gpu/drm/bridge/lontium-lt8912b.c | 25 +++++++++++++++++++++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c b/drivers/gpu/drm/bridge/lontium-lt8912b.c
> index 4b2ae27f0a57f..154126bb922b4 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt8912b.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c
> @@ -47,6 +47,7 @@ struct lt8912 {
>
> u8 data_lanes;
> bool is_power_on;
> + bool do_pn_swap;
> };
>
> static int lt8912_write_init_config(struct lt8912 *lt)
> @@ -78,15 +79,31 @@ static int lt8912_write_init_config(struct lt8912 *lt)
> {0x55, 0x44},
> {0x57, 0x01},
> {0x5a, 0x02},
> -
> - /*MIPI Analog*/
> + };
> + const struct reg_sequence mipi_analog_seq[] = {
> {0x3e, 0xd6},
> {0x3f, 0xd4},
> {0x41, 0x3c},
> {0xB2, 0x00},
> };
> + const struct reg_sequence mipi_analog_pn_swap_seq[] = {
> + {0x3e, 0xf6},
> + {0x3f, 0xd4},
> + {0x41, 0x3c},
> + {0xB2, 0x00},
> + };
> + int ret;
>
> - return regmap_multi_reg_write(lt->regmap[I2C_MAIN], seq, ARRAY_SIZE(seq));
> + ret = regmap_multi_reg_write(lt->regmap[I2C_MAIN], seq, ARRAY_SIZE(seq));
> + if (ret < 0)
> + return ret;
> +
> + if (!lt->do_pn_swap)
> + return regmap_multi_reg_write(lt->regmap[I2C_MAIN], mipi_analog_seq,
> + ARRAY_SIZE(mipi_analog_seq));
> +
> + return regmap_multi_reg_write(lt->regmap[I2C_MAIN], mipi_analog_pn_swap_seq,
> + ARRAY_SIZE(mipi_analog_pn_swap_seq));
Can you just remove {0x3e, 0xd6} from the register/value array and write
it afterward depending on `do_pn_swap` value? Or keep it with the
current value and only overwrite it when do_pn_swap is true?
If you do it this way is a 4 line change.
> static int lt8912_write_mipi_basic_config(struct lt8912 *lt)
> @@ -702,6 +719,8 @@ static int lt8912_parse_dt(struct lt8912 *lt)
> }
> lt->gp_reset = gp_reset;
>
> + lt->do_pn_swap = device_property_read_bool(dev, "lontium,pn-swap");
I would call this variable the same that is called in the lontium
documentation, mipirx_diff_swap
Francesco
^ permalink raw reply
* Re: [PATCH] arm64: dts: ti: k3-j722s: Disable ethernet ports by default
From: Francesco Dolcini @ 2024-04-02 16:58 UTC (permalink / raw)
To: Michael Walle
Cc: Nishanth Menon, Vignesh Raghavendra, Tero Kristo, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-kernel, devicetree,
linux-kernel
In-Reply-To: <20240402151802.3803708-1-mwalle@kernel.org>
Hello Michael,
On Tue, Apr 02, 2024 at 05:18:02PM +0200, Michael Walle wrote:
> Device tree best practice is to disable any external interface in the
> dtsi and just enable them if needed in the device tree. Thus, disable
> both ethernet ports by default and just enable the one used by the EVM
> in its device tree.
>
> There is no functional change.
>
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
> This should also be true for all the other SoCs. But I don't wanted to
> touch all the (older) device trees. j722s is pretty new, so there we
> should get it right.
> ---
> arch/arm64/boot/dts/ti/k3-j722s-evm.dts | 5 +----
> arch/arm64/boot/dts/ti/k3-j722s.dtsi | 8 ++++++++
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-j722s-evm.dts b/arch/arm64/boot/dts/ti/k3-j722s-evm.dts
> index d045dc7dde0c..afe7f68e6a4b 100644
> --- a/arch/arm64/boot/dts/ti/k3-j722s-evm.dts
> +++ b/arch/arm64/boot/dts/ti/k3-j722s-evm.dts
> @@ -224,14 +224,11 @@ cpsw3g_phy0: ethernet-phy@0 {
> };
>
> &cpsw_port1 {
> + status = "okay";
status should be the last property, according to the dts coding guidelines.
> phy-mode = "rgmii-rxid";
> phy-handle = <&cpsw3g_phy0>;
> };
Francesco
^ permalink raw reply
* Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver
From: Guenter Roeck @ 2024-04-02 17:11 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Lee Jones, Wim Van Sebroeck, devicetree,
linux-kernel, linux-watchdog
In-Reply-To: <f8e743a6c49607de0dd7a27778383477e051b130.1712058690.git.mazziesaccount@gmail.com>
On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote:
> Introduce driver for WDG block on ROHM BD96801 scalable PMIC.
>
> This driver only supports watchdog with I2C feeding and delayed
> response detection. Whether the watchdog toggles PRSTB pin or
> just causes an interrupt can be configured via device-tree.
>
> The BD96801 PMIC HW supports also window watchdog (too early
> feeding detection) and Q&A mode. These are not supported by
> this driver.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> drivers/watchdog/Kconfig | 13 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/bd96801_wdt.c | 375 +++++++++++++++++++++++++++++++++
> 3 files changed, 389 insertions(+)
> create mode 100644 drivers/watchdog/bd96801_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 6bee137cfbe0..d97e735e1faa 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -181,6 +181,19 @@ config BD957XMUF_WATCHDOG
> watchdog. Alternatively say M to compile the driver as a module,
> which will be called bd9576_wdt.
>
> +config BD96801_WATCHDOG
> + tristate "ROHM BD96801 PMIC Watchdog"
> + depends on MFD_ROHM_BD96801
> + select WATCHDOG_CORE
> + help
> + Support for the watchdog in the ROHM BD96801 PMIC. Watchdog can be
> + configured to only generate IRQ or to trigger system reset via reset
> + pin.
> +
> + Say Y here to include support for the ROHM BD96801 watchdog.
> + Alternatively say M to compile the driver as a module,
> + which will be called bd96801_wdt.
> +
> config CROS_EC_WATCHDOG
> tristate "ChromeOS EC-based watchdog"
> select WATCHDOG_CORE
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 3710c218f05e..31bc94436c81 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -217,6 +217,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>
> # Architecture Independent
> obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
> +obj-$(CONFIG_BD96801_WATCHDOG) += bd96801_wdt.o
> obj-$(CONFIG_CROS_EC_WATCHDOG) += cros_ec_wdt.o
> obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
> obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
> diff --git a/drivers/watchdog/bd96801_wdt.c b/drivers/watchdog/bd96801_wdt.c
> new file mode 100644
> index 000000000000..cb2b526ecc21
> --- /dev/null
> +++ b/drivers/watchdog/bd96801_wdt.c
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 ROHM Semiconductors
> + *
> + * ROHM BD96801 watchdog driver
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/mfd/rohm-bd96801.h>
> +#include <linux/mfd/rohm-generic.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/watchdog.h>
> +
> +static bool nowayout;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> + "Watchdog cannot be stopped once started (default=\"false\")");
> +
> +#define BD96801_WD_TMO_SHORT_MASK 0x70
> +#define BD96801_WD_RATIO_MASK 0x3
> +#define BD96801_WD_TYPE_MASK 0x4
> +#define BD96801_WD_TYPE_SLOW 0x4
> +#define BD96801_WD_TYPE_WIN 0x0
> +
> +#define BD96801_WD_EN_MASK 0x3
> +#define BD96801_WD_IF_EN 0x1
> +#define BD96801_WD_QA_EN 0x2
> +#define BD96801_WD_DISABLE 0x0
> +
> +#define BD96801_WD_ASSERT_MASK 0x8
> +#define BD96801_WD_ASSERT_RST 0x8
> +#define BD96801_WD_ASSERT_IRQ 0x0
> +
> +#define BD96801_WD_FEED_MASK 0x1
> +#define BD96801_WD_FEED 0x1
> +
> +/* units in uS */
> +#define FASTNG_MIN 3370
> +#define BD96801_WDT_DEFAULT_MARGIN 6905120
> +/* Unit is seconds */
> +#define DEFAULT_TIMEOUT 30
> +
> +/*
> + * BD96801 WDG supports window mode so the TMO consists of SHORT and LONG
> + * timeout values. SHORT time is meaningfull only in window mode where feeding
> + * period shorter than SHORT would be an error. LONG time is used to detect if
> + * feeding is not occurring within given time limit (SoC SW hangs). The LONG
> + * timeout time is a multiple of (2, 4, 8 0r 16 times) the SHORT timeout.
> + */
> +
> +struct wdtbd96801 {
> + struct device *dev;
> + struct regmap *regmap;
> + bool always_running;
> + struct watchdog_device wdt;
> +};
> +
> +static int bd96801_wdt_ping(struct watchdog_device *wdt)
> +{
> + struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
> +
> + return regmap_update_bits(w->regmap, BD96801_REG_WD_FEED,
> + BD96801_WD_FEED_MASK, BD96801_WD_FEED);
> +}
> +
> +static int bd96801_wdt_start(struct watchdog_device *wdt)
> +{
> + struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
> + int ret;
> +
> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_EN_MASK, BD96801_WD_IF_EN);
> +
> + return ret;
> +}
> +
> +static int bd96801_wdt_stop(struct watchdog_device *wdt)
> +{
> + struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
> +
> + if (!w->always_running)
> + return regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_EN_MASK, BD96801_WD_DISABLE);
> + set_bit(WDOG_HW_RUNNING, &wdt->status);
> +
> + return 0;
> +}
> +
> +static const struct watchdog_info bd96801_wdt_info = {
> + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> + WDIOF_SETTIMEOUT,
> + .identity = "BD96801 Watchdog",
> +};
> +
> +static const struct watchdog_ops bd96801_wdt_ops = {
> + .start = bd96801_wdt_start,
> + .stop = bd96801_wdt_stop,
> + .ping = bd96801_wdt_ping,
> +};
> +
> +static int find_closest_fast(int target, int *sel, int *val)
> +{
> + int i;
> + int window = FASTNG_MIN;
> +
> + for (i = 0; i < 8 && window < target; i++)
> + window <<= 1;
> +
> + *val = window;
> + *sel = i;
> +
> + if (i == 8)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int find_closest_slow_by_fast(int fast_val, int *target, int *slowsel)
> +{
> + int sel;
> + static const int multipliers[] = {2, 4, 8, 16};
> +
> + for (sel = 0; sel < ARRAY_SIZE(multipliers) &&
> + multipliers[sel] * fast_val < *target; sel++)
> + ;
> +
> + if (sel == ARRAY_SIZE(multipliers))
> + return -EINVAL;
> +
> + *slowsel = sel;
> + *target = multipliers[sel] * fast_val;
> +
> + return 0;
> +}
> +
> +static int find_closest_slow(int *target, int *slow_sel, int *fast_sel)
> +{
> + static const int multipliers[] = {2, 4, 8, 16};
> + int i, j;
> + int val = 0;
> + int window = FASTNG_MIN;
> +
> + for (i = 0; i < 8; i++) {
> + for (j = 0; j < ARRAY_SIZE(multipliers); j++) {
> + int slow;
> +
> + slow = window * multipliers[j];
> + if (slow >= *target && (!val || slow < val)) {
> + val = slow;
> + *fast_sel = i;
> + *slow_sel = j;
> + }
> + }
> + window <<= 1;
> + }
> + if (!val)
> + return -EINVAL;
> +
> + *target = val;
> +
> + return 0;
> +}
> +
> +static int bd96801_set_wdt_mode(struct wdtbd96801 *w, int hw_margin,
> + int hw_margin_min)
> +{
> + int ret, fastng, slowng, type, reg, mask;
> + struct device *dev = w->dev;
> +
> + /* convert to uS */
> + hw_margin *= 1000;
> + hw_margin_min *= 1000;
> + if (hw_margin_min) {
> + int min;
> +
> + type = BD96801_WD_TYPE_WIN;
> + dev_dbg(dev, "Setting type WINDOW 0x%x\n", type);
> + ret = find_closest_fast(hw_margin_min, &fastng, &min);
> + if (ret) {
> + dev_err(dev, "bad WDT window for fast timeout\n");
> + return ret;
> + }
> +
> + ret = find_closest_slow_by_fast(min, &hw_margin, &slowng);
> + if (ret) {
> + dev_err(dev, "bad WDT window\n");
> + return ret;
> + }
> + w->wdt.min_hw_heartbeat_ms = min / 1000;
> + } else {
> + type = BD96801_WD_TYPE_SLOW;
> + dev_dbg(dev, "Setting type SLOW 0x%x\n", type);
> + ret = find_closest_slow(&hw_margin, &slowng, &fastng);
> + if (ret) {
> + dev_err(dev, "bad WDT window\n");
> + return ret;
> + }
> + }
> +
> + w->wdt.max_hw_heartbeat_ms = hw_margin / 1000;
> +
> + fastng <<= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;
> +
> + reg = slowng | fastng;
> + mask = BD96801_WD_RATIO_MASK | BD96801_WD_TMO_SHORT_MASK;
> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_TMO,
> + mask, reg);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_TYPE_MASK, type);
> +
> + return ret;
> +}
> +
> +static int bd96801_set_heartbeat_from_hw(struct wdtbd96801 *w,
> + unsigned int conf_reg)
> +{
> + int ret;
> + unsigned int val, sel, fast;
> +
> + /*
> + * The BD96801 supports a somewhat peculiar QA-mode, which we do not
> + * support in this driver. If the QA-mode is enabled then we just
> + * warn and bail-out.
> + */
> + if ((conf_reg & BD96801_WD_EN_MASK) != BD96801_WD_IF_EN) {
> + dev_warn(w->dev, "watchdog set to Q&A mode - exiting\n");
> + return -EINVAL;
> + }
> +
> + ret = regmap_read(w->regmap, BD96801_REG_WD_TMO, &val);
> + if (ret)
> + return ret;
> +
> + sel = val & BD96801_WD_TMO_SHORT_MASK;
> + sel >>= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;
> + fast = FASTNG_MIN << sel;
> +
> + sel = (val & BD96801_WD_RATIO_MASK) + 1;
> + w->wdt.max_hw_heartbeat_ms = (fast << sel) / USEC_PER_MSEC;
> +
> + if ((conf_reg & BD96801_WD_TYPE_MASK) == BD96801_WD_TYPE_WIN)
> + w->wdt.min_hw_heartbeat_ms = fast / USEC_PER_MSEC;
> +
> + return 0;
> +}
> +
> +static int init_wdg_hw(struct wdtbd96801 *w)
> +{
> + u32 hw_margin[2];
> + int count, ret;
> + u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0;
> +
> + count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms");
> + if (count < 0 && count != -EINVAL)
> + return count;
> +
> + if (count > 0) {
> + if (count > ARRAY_SIZE(hw_margin))
> + return -EINVAL;
> +
> + ret = device_property_read_u32_array(w->dev->parent,
> + "rohm,hw-timeout-ms",
> + &hw_margin[0], count);
> + if (ret < 0)
> + return ret;
> +
> + if (count == 1)
> + hw_margin_max = hw_margin[0];
> +
> + if (count == 2) {
> + hw_margin_max = hw_margin[1];
> + hw_margin_min = hw_margin[0];
> + }
> + }
> +
> + ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min);
> + if (ret)
> + return ret;
> +
> + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> + "prstb");
> + if (ret >= 0) {
> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_ASSERT_MASK,
> + BD96801_WD_ASSERT_RST);
> + return ret;
> + }
> +
> + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> + "intb-only");
> + if (ret >= 0) {
> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_ASSERT_MASK,
> + BD96801_WD_ASSERT_IRQ);
> + return ret;
> + }
I don't see the devicetree bindings documented in the series.
I am also a bit surprised that the interrupt isn't handled in the driver.
Please explain.
> +
> + return 0;
> +}
> +
> +static int bd96801_wdt_probe(struct platform_device *pdev)
> +{
> + struct wdtbd96801 *w;
> + int ret;
> + unsigned int val;
> +
> + w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
> + if (!w)
> + return -ENOMEM;
> +
> + w->regmap = dev_get_regmap(pdev->dev.parent, NULL);
dev_get_regmap() can return NULL.
> + w->dev = &pdev->dev;
> +
> + w->wdt.info = &bd96801_wdt_info;
> + w->wdt.ops = &bd96801_wdt_ops;
> + w->wdt.parent = pdev->dev.parent;
> + w->wdt.timeout = DEFAULT_TIMEOUT;
> + watchdog_set_drvdata(&w->wdt, w);
> +
> + w->always_running = device_property_read_bool(pdev->dev.parent,
> + "always-running");
> +
Without documentation, it looks like the always-running (from
linux,wdt-gpio.yaml) may be abused. Its defined meaning is
"the watchdog is always running and can not be stopped". Its
use here seems to be "start watchdog when loading the module and
prevent it from being stopped".
Oh well, looks like the abuse was introduced with bd9576_wdt. That
doesn't make it better. At the very least it needs to be documented
that the property does not have the intended (documented) meaning.
> + ret = regmap_read(w->regmap, BD96801_REG_WD_CONF, &val);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "Failed to get the watchdog state\n");
> +
> + /*
> + * If the WDG is already enabled we assume it is configured by boot.
> + * In this case we just update the hw-timeout based on values set to
> + * the timeout / mode registers and leave the hardware configs
> + * untouched.
> + */
> + if ((val & BD96801_WD_EN_MASK) != BD96801_WD_DISABLE) {
> + dev_dbg(&pdev->dev, "watchdog was running during probe\n");
> + ret = bd96801_set_heartbeat_from_hw(w, val);
> + if (ret)
> + return ret;
> +
> + set_bit(WDOG_HW_RUNNING, &w->wdt.status);
> + } else {
> + /* If WDG is not running so we will initializate it */
> + ret = init_wdg_hw(w);
> + if (ret)
> + return ret;
> + }
> +
> + watchdog_init_timeout(&w->wdt, 0, pdev->dev.parent);
> + watchdog_set_nowayout(&w->wdt, nowayout);
> + watchdog_stop_on_reboot(&w->wdt);
> +
> + if (w->always_running)
> + bd96801_wdt_start(&w->wdt);
I think this needs to set WDOG_HW_RUNNING or the watchdog will trigger
a reboot if the watchdog device is not opened and the watchdog wasn't
already running when the module was loaded.
That makes me wonder what happens if the property is set and the
watchdog daemon isn't started in the bd9576_wdt driver.
> +
> + return devm_watchdog_register_device(&pdev->dev, &w->wdt);
> +}
> +
> +static struct platform_driver bd96801_wdt = {
> + .driver = {
> + .name = "bd96801-wdt"
> + },
> + .probe = bd96801_wdt_probe,
> +};
> +module_platform_driver(bd96801_wdt);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD96801 watchdog driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:bd96801-wdt");
> --
> 2.43.2
>
>
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
>
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =]
^ permalink raw reply
* Re: [PATCH] dt-bindings: pci: altera: covert to yaml
From: matthew.gerlach @ 2024-04-02 17:14 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: joyce.ooi, bhelgaas, lpieralisi, kw, robh, krzysztof.kozlowski+dt,
conor+dt, linux-pci, devicetree, linux-kernel
In-Reply-To: <20240401224447.GA1762763@bhelgaas>
On Mon, 1 Apr 2024, Bjorn Helgaas wrote:
> "git log --oneline Documentation/devicetree/bindings/pci/" says the
> typical style would be:
>
> dt-bindings: PCI: altera: Convert to YAML
Good suggestion about the 'git log --oneline ...' I will update title in
v2.
>
> On Fri, Mar 29, 2024 at 12:00:31PM -0500, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Covert the device tree bindings for the Altera Root
>> Port controller from text to yaml.
>
> s/covert/convert/ (both in subject and commit log).
>
> Rewrap to fill 80 columns.
>
Thanks for catching the spelling error. Wrapping and spelling fix will be
included in v2.
Matthew Gerlach
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: phy: phy-imx8-pcie: Add binding for i.MX8Q HSIO SerDes PHY
From: Rob Herring @ 2024-04-02 17:21 UTC (permalink / raw)
To: Richard Zhu
Cc: vkoul, kishon, krzysztof.kozlowski+dt, frank.li, conor+dt,
linux-phy, devicetree, linux-arm-kernel, linux-kernel, kernel,
imx
In-Reply-To: <1712036704-21064-2-git-send-email-hongxing.zhu@nxp.com>
On Tue, Apr 02, 2024 at 01:45:02PM +0800, Richard Zhu wrote:
> Add binding for controller ID and HSIO configuration setting of the
> i.MX8Q HSIO SerDes PHY.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> include/dt-bindings/phy/phy-imx8-pcie.h | 29 +++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/include/dt-bindings/phy/phy-imx8-pcie.h b/include/dt-bindings/phy/phy-imx8-pcie.h
> index 8bbe2d6538d8..3292c8be3354 100644
> --- a/include/dt-bindings/phy/phy-imx8-pcie.h
> +++ b/include/dt-bindings/phy/phy-imx8-pcie.h
> @@ -11,4 +11,33 @@
> #define IMX8_PCIE_REFCLK_PAD_INPUT 1
> #define IMX8_PCIE_REFCLK_PAD_OUTPUT 2
>
> +/*
> + * i.MX8QM HSIO subsystem has three lane PHYs and three controllers:
> + * PCIEA(2 lanes capapble PCIe controller), PCIEB (only support one
capable
> + * lane) and SATA.
> + *
> + * In the different use cases. PCIEA can be binded to PHY lane0, lane1
s/binded/bound/
And throughout your patches.
> + * or Lane0 and lane1. PCIEB can be binded to lane1 or lane2 PHY. SATA
> + * can only be binded to last lane2 PHY.
> + *
> + * Define i.MX8Q HSIO controller ID here to specify the controller
> + * binded to the PHY.
> + * Meanwhile, i.MX8QXP HSIO subsystem has one lane PHY and PCIEB(only
> + * support one lane) controller.
> + */
> +#define IMX8Q_HSIO_PCIEA_ID 0
> +#define IMX8Q_HSIO_PCIEB_ID 1
> +#define IMX8Q_HSIO_SATA_ID 2
Please use the standard phy mode defines.
> +
> +/*
> + * On i.MX8QM, PCIEA is mandatory required if the HSIO is enabled.
> + * Define configurations beside PCIEA is enabled.
> + *
> + * On i.MX8QXP, HSIO module only has PCIEB and one lane PHY.
> + * The "IMX8Q_HSIO_CFG_PCIEB" can be used on i.MX8QXP platforms.
> + */
> +#define IMX8Q_HSIO_CFG_SATA 1
> +#define IMX8Q_HSIO_CFG_PCIEB 2
> +#define IMX8Q_HSIO_CFG_PCIEBSATA 3
This seems somewhat redundant both as the 3rd define is just an OR of
the first 2 and all 3 overlap with the prior defines.
Seems like with standard PHY modes, the only additional information you
might need is PCIEB vs. PCIEA.
Rob
^ permalink raw reply
* Re: [PATCH v2 12/13] ASoC: dt-bindings: davinci-mcbsp: Add the 'ti,T1-framing-{rx/tx}' flags
From: Rob Herring @ 2024-04-02 17:32 UTC (permalink / raw)
To: Bastien Curutchet
Cc: Peter Ujfalusi, Thomas Petazzoni, Liam Girdwood, Takashi Iwai,
linux-kernel, christophercordahi, devicetree, alsa-devel,
Krzysztof Kozlowski, herve.codina, Conor Dooley, Jaroslav Kysela,
linux-sound, Mark Brown
In-Reply-To: <20240402071213.11671-13-bastien.curutchet@bootlin.com>
On Tue, 02 Apr 2024 09:12:12 +0200, Bastien Curutchet wrote:
> McBSP's data delay can be configured from 0 to 2 bit clock periods. 0 is
> used for DSP_B format, 1 for DSP_A format. A data delay of 2 bit clock
> periods can be used to interface to 'T1 framing' devices where data
> stream is preceded by a 'framing bit'. This 2 bit clock data delay is
> not described in the bindings.
>
> Add two flags 'ti,T1-framing-[rx/tx]' to enable a data delay of 2
> bit clock periods in reception or transmission.
>
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
> .../devicetree/bindings/sound/davinci-mcbsp.yaml | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v1 5/5] LoongArch: dts: Add PWM support to Loongson-2K2000
From: kernel test robot @ 2024-04-02 17:34 UTC (permalink / raw)
To: Binbin Zhou, Binbin Zhou, Huacai Chen, Uwe Kleine-König,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: oe-kbuild-all, loongson-kernel, linux-pwm, devicetree,
Xuerui Wang, loongarch
In-Reply-To: <7214b933ce85f9d030828e9efab7fbeb57eb712b.1711953223.git.zhoubinbin@loongson.cn>
Hi Binbin,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.9-rc2 next-20240402]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Binbin-Zhou/dt-bindings-pwm-Add-Loongson-PWM-controller/20240402-160109
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/7214b933ce85f9d030828e9efab7fbeb57eb712b.1711953223.git.zhoubinbin%40loongson.cn
patch subject: [PATCH v1 5/5] LoongArch: dts: Add PWM support to Loongson-2K2000
config: loongarch-allnoconfig (https://download.01.org/0day-ci/archive/20240403/202404030108.rzArK10u-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240403/202404030108.rzArK10u-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404030108.rzArK10u-lkp@intel.com/
All errors (new ones prefixed by >>):
>> Error: arch/loongarch/boot/dts/loongson-2k2000.dtsi:123.19-20 syntax error
FATAL ERROR: Unable to parse input tree
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 3/3] arm64: dts: qcom: msm8996: Add missing UFS host controller reset
From: Bjorn Andersson @ 2024-04-02 17:35 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Manivannan Sadhasivam, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andy Gross, Andy Gross, James E.J. Bottomley, Martin K. Petersen,
linux-arm-msm, linux-scsi, devicetree, linux-kernel
In-Reply-To: <CAA8EJpqZYp0C8rT8E=LoVo9fispLNhBn8CEgx1-iMqN_2MQXfg@mail.gmail.com>
On Fri, Feb 09, 2024 at 10:16:25PM +0200, Dmitry Baryshkov wrote:
> On Tue, 30 Jan 2024 at 08:55, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Mon, Jan 29, 2024 at 11:44:15AM +0200, Dmitry Baryshkov wrote:
> > > On Mon, 29 Jan 2024 at 09:55, Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > >
> > > > UFS host controller reset is required for the drivers to properly reset the
> > > > controller. Hence, add it.
> > > >
> > > > Fixes: 57fc67ef0d35 ("arm64: dts: qcom: msm8996: Add ufs related nodes")
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > >
> > > I think I had issues previously when I attempted to reset the
> > > controller, but it might be because of the incomplete clocks
> > > programming. Let met check whether it works first.
> > >
> >
> > Sure. Please let me know.
>
> With the clocking fixes in place (I'll send them in a few minutes) and
> with this patch the UFS breaks in the following way:
>
Was this further reviewed/investigated? What would you like me to do
with this patch?
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH v1 1/5] dt-bindings: pwm: Add Loongson PWM controller
From: Rob Herring @ 2024-04-02 17:40 UTC (permalink / raw)
To: Binbin Zhou
Cc: Binbin Zhou, Huacai Chen, Uwe Kleine-König,
Krzysztof Kozlowski, Conor Dooley, Huacai Chen, loongson-kernel,
linux-pwm, devicetree, Xuerui Wang, loongarch
In-Reply-To: <edad2bb5b0045c633734c1499fb163c3c6776121.1711953223.git.zhoubinbin@loongson.cn>
On Tue, Apr 02, 2024 at 03:58:38PM +0800, Binbin Zhou wrote:
> Add Loongson PWM controller binding with DT schema format using
> json-schema.
>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
> .../devicetree/bindings/pwm/pwm-loongson.yaml | 64 +++++++++++++++++++
Filename should match compatible.
> MAINTAINERS | 6 ++
> 2 files changed, 70 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-loongson.yaml
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-loongson.yaml b/Documentation/devicetree/bindings/pwm/pwm-loongson.yaml
> new file mode 100644
> index 000000000000..d25904468353
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-loongson.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/pwm-loongson.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson PWM Controller
> +
> +maintainers:
> + - Binbin Zhou <zhoubinbin@loongson.cn>
> +
> +description:
> + It is the generic PWM framework driver for Loongson family.
That's describing the driver. Not really relevant to the binding.
> + Each PWM has one pulse width output signal and one pulse input
> + signal to be measured.
> + It can be found on Loongson-2K series cpus and Loongson LS7A bridge chips.
> +
> +allOf:
> + - $ref: pwm.yaml#
> +
> +properties:
> + compatible:
> + oneOf:
> + - const: loongson,ls7a-pwm
> + - items:
> + - enum:
> + - loongson,ls2k0500-pwm
> + - loongson,ls2k1000-pwm
> + - loongson,ls2k2000-pwm
> + - const: loongson,ls7a-pwm
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> + '#pwm-cells':
> + const: 3
Please define what is in each cell. If there's only 2 signals, then the
first cell defines the output or input (what value for which one?).
Really, the PWM binding is only for outputs, so is a cell even needed? I
suppose we could use it for inputs too, but that's really "input
capture" type operation that timers often have. I'll defer to the PWM
maintainers...
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - '#pwm-cells'
pwm.yaml makes this required already.
Rob
^ permalink raw reply
* [PATCH v4 1/2] media: dt-bindings: ovti,ov2680: Fix the power supply names
From: Fabio Estevam @ 2024-04-02 17:40 UTC (permalink / raw)
To: sakari.ailus
Cc: rmfrfs, laurent.pinchart, hansg, robh, krzysztof.kozlowski+dt,
conor+dt, linux-media, devicetree, Fabio Estevam
From: Fabio Estevam <festevam@denx.de>
The original .txt bindings had the OV2680 power supply names correct,
but the transition from .txt to yaml spelled them incorrectly.
Fix the OV2680 power supply names as the original .txt bindings
as these are the names used by the OV2680 driver and in devicetree.
Fixes: 57226cd8c8bf ("media: dt-bindings: ov2680: convert bindings to yaml")
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v3:
- Newly introduced.
.../bindings/media/i2c/ovti,ov2680.yaml | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
index cf456f8d9ddc..c87677f5e2a2 100644
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
@@ -37,15 +37,15 @@ properties:
active low.
maxItems: 1
- dovdd-supply:
+ DOVDD-supply:
description:
Definition of the regulator used as interface power supply.
- avdd-supply:
+ AVDD-supply:
description:
Definition of the regulator used as analog power supply.
- dvdd-supply:
+ DVDD-supply:
description:
Definition of the regulator used as digital power supply.
@@ -59,9 +59,9 @@ required:
- reg
- clocks
- clock-names
- - dovdd-supply
- - avdd-supply
- - dvdd-supply
+ - DOVDD-supply
+ - AVDD-supply
+ - DVDD-supply
- reset-gpios
- port
@@ -82,9 +82,9 @@ examples:
clock-names = "xvclk";
reset-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;
- dovdd-supply = <&sw2_reg>;
- dvdd-supply = <&sw2_reg>;
- avdd-supply = <®_peri_3p15v>;
+ DOVDD-supply = <&sw2_reg>;
+ DVDD-supply = <&sw2_reg>;
+ AVDD-supply = <®_peri_3p15v>;
port {
ov2680_to_mipi: endpoint {
--
2.34.1
^ permalink raw reply related
* [PATCH v4 2/2] media: dt-bindings: ovti,ov2680: Document link-frequencies
From: Fabio Estevam @ 2024-04-02 17:40 UTC (permalink / raw)
To: sakari.ailus
Cc: rmfrfs, laurent.pinchart, hansg, robh, krzysztof.kozlowski+dt,
conor+dt, linux-media, devicetree, Fabio Estevam
In-Reply-To: <20240402174028.205434-1-festevam@gmail.com>
From: Fabio Estevam <festevam@denx.de>
Document the link-frequencies property as recommended by the following
document:
https://www.kernel.org/doc/html/v6.9-rc1/driver-api/media/camera-sensor.html#handling-clocks
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v3:
- Only document link-frequencies.
.../bindings/media/i2c/ovti,ov2680.yaml | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
index c87677f5e2a2..634d3b821b8c 100644
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
@@ -50,9 +50,23 @@ properties:
Definition of the regulator used as digital power supply.
port:
- $ref: /schemas/graph.yaml#/properties/port
description:
A node containing an output port node.
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ additionalProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ additionalProperties: false
+
+ properties:
+ link-frequencies: true
+
+ remote-endpoint: true
+
+ required:
+ - link-frequencies
required:
- compatible
@@ -89,6 +103,7 @@ examples:
port {
ov2680_to_mipi: endpoint {
remote-endpoint = <&mipi_from_sensor>;
+ link-frequencies = /bits/ 64 <330000000>;
};
};
};
--
2.34.1
^ permalink raw reply related
* Re: [PATCH RFT 01/10] arm64: dts: microchip: sparx5: fix mdio reg
From: Conor Dooley @ 2024-04-02 17:46 UTC (permalink / raw)
To: Steen Hegelund
Cc: Krzysztof Kozlowski, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Daniel Machon, UNGLinuxDriver,
David S. Miller, Bjarni Jonasson, linux-arm-kernel, devicetree,
linux-kernel
In-Reply-To: <b3d818df8819d2fb3e96fa61b277d49941d9b01b.camel@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 2259 bytes --]
Hey,
On Tue, Apr 02, 2024 at 04:00:32PM +0200, Steen Hegelund wrote:
> On Mon, 2024-04-01 at 17:37 +0200, Krzysztof Kozlowski wrote:
> > [Some people who received this message don't often get email from
> > krzk@kernel.org. Learn why this is important at
> > https://aka.ms/LearnAboutSenderIdentification ]
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> >
> > Correct the reg address of mdio node to match unit address. Assume
> > the
> > reg is not correct and unit address was correct, because there is
> > alerady node using the existing reg 0x110102d4.
> >
> > sparx5.dtsi:443.25-451.5: Warning (simple_bus_reg):
> > /axi@600000000/mdio@6110102f8: simple-bus unit address format error,
> > expected "6110102d4"
> >
> > Fixes: d0f482bb06f9 ("arm64: dts: sparx5: Add the Sparx5 switch
> > node")
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> >
> > ---
> >
> > Not tested on hardware
> > ---
> > arch/arm64/boot/dts/microchip/sparx5.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi
> > b/arch/arm64/boot/dts/microchip/sparx5.dtsi
> > index 24075cd91420..5d820da8c69d 100644
> > --- a/arch/arm64/boot/dts/microchip/sparx5.dtsi
> > +++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi
> > @@ -447,7 +447,7 @@ mdio2: mdio@6110102f8 {
> > pinctrl-names = "default";
> > #address-cells = <1>;
> > #size-cells = <0>;
> > - reg = <0x6 0x110102d4 0x24>;
> > + reg = <0x6 0x110102f8 0x24>;
> > };
> >
> > mdio3: mdio@61101031c {
> > --
> > 2.34.1
> >
>
> I did a check of our current Sparx5 EVBs and none of them uses
> controller 2 in any revision, so this is probably why it has not come
> up before, so as it stands we have no platform to test this change on
> currently.
>
> Besides that the change looks good to me.
>
> Best Regards
> Steen
>
> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
Are you okay with the rest of the series, or have you only looked at
this one patch?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] dt-bindings: timer: renesas,tmu: Add R-Car V4M support
From: Conor Dooley @ 2024-04-02 17:49 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Daniel Lezcano, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Laurent Pinchart, devicetree, linux-renesas-soc,
linux-kernel
In-Reply-To: <8a39386b1a33db6e83e852b3b365bc1adeb25242.1712068574.git.geert+renesas@glider.be>
[-- Attachment #1: Type: text/plain, Size: 280 bytes --]
On Tue, Apr 02, 2024 at 04:37:02PM +0200, Geert Uytterhoeven wrote:
> Document support for the Timer Unit (TMU) in the Renesas R-Car V4M
> (R8A779H0) SoC.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] dt-bindings: timer: renesas,cmt: Add R-Car V4M support
From: Conor Dooley @ 2024-04-02 17:49 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Daniel Lezcano, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Laurent Pinchart, devicetree, linux-renesas-soc,
linux-kernel
In-Reply-To: <3e8a7a93261d8ad264dec2fa2784fe1bbfc4a23c.1712068536.git.geert+renesas@glider.be>
[-- Attachment #1: Type: text/plain, Size: 313 bytes --]
On Tue, Apr 02, 2024 at 04:36:05PM +0200, Geert Uytterhoeven wrote:
> Document support for the Compare Match Timer Type0 (CMT0) and Type1
> (CMT1) in the Renesas R-Car V4M (R8A779H0) SoC.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] dt-bindings: leds: leds-qcom-lpg: Add support for PMI8950 PWM
From: Conor Dooley @ 2024-04-02 17:54 UTC (permalink / raw)
To: Gianluca Boiano
Cc: Pavel Machek, Lee Jones, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-leds,
linux-kernel, linux-arm-msm, devicetree
In-Reply-To: <20240402-pmi8950-pwm-support-v1-3-1a66899eeeb3@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 236 bytes --]
On Tue, Apr 02, 2024 at 02:35:44PM +0200, Gianluca Boiano wrote:
> Update leds-qcom-lpg binding to support PMI8950 PWM.
>
> Signed-off-by: Gianluca Boiano <morf3089@gmail.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] dt-bindings: mfd: syscon: Add ti,am62p-cpsw-mac-efuse compatible
From: Krzysztof Kozlowski @ 2024-04-02 18:06 UTC (permalink / raw)
To: Siddharth Vadapalli
Cc: lee, robh, krzk+dt, conor+dt, devicetree, linux-kernel,
linux-arm-kernel, srk
In-Reply-To: <30065bdc-ccef-4610-b1c1-7661f801b8e9@ti.com>
On 02/04/2024 14:30, Siddharth Vadapalli wrote:
> On Tue, Apr 02, 2024 at 02:08:32PM +0200, Krzysztof Kozlowski wrote:
>> On 02/04/2024 12:57, Siddharth Vadapalli wrote:
>>> The CTRLMMR_MAC_IDx registers within the CTRL_MMR space of TI's AM62p SoC
>>> contain the MAC Address programmed in the eFuse. Add compatible for
>>> allowing the CPSW driver to obtain a regmap for the CTRLMMR_MAC_IDx
>>> registers within the System Controller device-tree node. The default MAC
>>> Address for the interface corresponding to the first MAC port will be set
>>> to the value programmed in the eFuse.
>>>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> ---
>>>
>>> This patch is based on linux-next tagged next-20240402.
>>
>> Where is the DTS using it?
>
> The current implementation in the device-tree for older TI K3 SoCs is as
> follows:
>
> cpsw_port1: port@1 {
> reg = <1>;
> ti,mac-only;
> label = "port1";
> phys = <&phy_gmii_sel 1>;
> mac-address = [00 00 00 00 00 00];
> ti,syscon-efuse = <&wkup_conf 0x200>;
> };
>
> The "ti,syscon-efuse" property passes the reference to the System
> Controller node as well as the offset to the CTRLMMR_MAC_IDx registers
> within the CTRL_MMR space.
Please reference upstream DTS or lore link to patch under review.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 2/2] dt-bindings: display: bridge: lt8912b: document 'lontium,pn-swap' property
From: Conor Dooley @ 2024-04-02 18:06 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-kernel, dri-devel, devicetree, adrien.grassein,
andrzej.hajda, neil.armstrong, rfoss, Laurent.pinchart, jonas,
jernej.skrabec, airlied, daniel, maarten.lankhorst, mripard,
tzimmermann, robh, krzysztof.kozlowski+dt, conor+dt,
stefan.eichenberger, francesco.dolcini, marius.muresan,
irina.muresan
In-Reply-To: <20240402105925.905144-2-alex@shruggie.ro>
[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]
On Tue, Apr 02, 2024 at 01:59:25PM +0300, Alexandru Ardelean wrote:
> On some HW designs, it's easier for the layout if the P/N pins are swapped.
> The driver currently has a DT property to do that.
"currently", because 1/2 adds it. bindings patches should precede the
driver patches in the series, so please swap the patches and remove this
portion of the description.
>
> This change documents the 'lontium,pn-swap' property.
>
> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> ---
> .../devicetree/bindings/display/bridge/lontium,lt8912b.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> index 2cef252157985..3a804926b288a 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> @@ -24,6 +24,12 @@ properties:
> maxItems: 1
> description: GPIO connected to active high RESET pin.
>
> + lontium,pn-swap:
> + description: Swap the polarities of the P/N pins in software.
> + On some HW designs, the layout is simplified if the P/N pins
> + are inverted.
Please explain what configuration of a board would cause these to be
swapped, rather than why someone might want to configure the board this
way. I've got no idea what this hardware is actually doing, so this is
being pulled out of a hat, but I'd expect something like "Some boards
swap the polarity of the P/N pins, use this property to indicate this to
software".
> + type: boolean
The type here should be flag.
Cheers,
Conor.
> +
> ports:
> $ref: /schemas/graph.yaml#/properties/ports
>
> --
> 2.44.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] dt-bindings: watchdog: Convert Aspeed binding to DT schema
From: Rob Herring @ 2024-04-02 18:07 UTC (permalink / raw)
To: Andrew Jeffery
Cc: wim, linux, krzysztof.kozlowski+dt, conor+dt, joel, zev,
linux-watchdog, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel
In-Reply-To: <20240402120118.282035-1-andrew@codeconstruct.com.au>
On Tue, Apr 02, 2024 at 10:31:18PM +1030, Andrew Jeffery wrote:
> Squash warnings such as:
>
> ```
> arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-galaxy100.dtb: /ahb/apb@1e600000/watchdog@1e785000: failed to match any schema with compatible: ['aspeed,ast2400-wdt']
> ```
>
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
> .../bindings/watchdog/aspeed,ast2400-wdt.yaml | 130 ++++++++++++++++++
> .../bindings/watchdog/aspeed-wdt.txt | 73 ----------
> 2 files changed, 130 insertions(+), 73 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> delete mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>
> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> new file mode 100644
> index 000000000000..10fcb50c4051
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml
> @@ -0,0 +1,130 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/aspeed,ast2400-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed watchdog timer controllers
> +
> +maintainers:
> + - Andrew Jeffery <andrew@codeconstruct.com.au>
> +
> +properties:
> + compatible:
> + enum:
> + - aspeed,ast2400-wdt
> + - aspeed,ast2500-wdt
> + - aspeed,ast2600-wdt
> +
> + reg:
> + maxItems: 1
> +
> + clocks: true
# and order/function if more than 1 must be defined.
Please note it was missing from the original binding in the commit
message.
> +
> + aspeed,reset-type:
> + enum:
> + - cpu
> + - soc
> + - system
> + - none
> + description: |
> + Reset behaviour - The watchdog can be programmed to generate one of three
> + different types of reset when a timeout occcurs.
> +
> + Specifying 'cpu' will only reset the processor on a timeout event.
> +
> + Specifying 'soc' will reset a configurable subset of the SoC's controllers
> + on a timeout event. Controllers critical to the SoC's operation may remain untouched.
> +
> + Specifying 'system' will reset all controllers on a timeout event, as if EXTRST had been asserted.
> + Specifying "none" will cause the timeout event to have no reset effect.
> + Another watchdog engine on the chip must be used for chip reset operations.
> +
> + The default reset type is "system"
Express as schema:
default: system
> +
> + aspeed,alt-boot:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: |
Don't need '|' if no formatting to preserve.
> + Direct the watchdog to configure the SoC to boot from the alternative boot
> + region if a timeout occurs.
> +
> + aspeed,external-signal:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: |
> + Assert the timeout event on an external signal pin associated with the
> + watchdog controller instance. The pin must be muxed appropriately.
> +
> + aspeed,ext-pulse-duration:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + The duration, in microseconds, of the pulse emitted on the external signal pin
Wrap at <80. Period at end needed.
> +
> + aspeed,ext-push-pull:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: |
> + If aspeed,external-signal is specified in the node, set the external
> + signal pin's drive type to push-pull. If aspeed,ext-push-pull is not
> + specified then the pin is configured as open-drain.
> +
> + aspeed,ext-active-high:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: |
> + If both aspeed,external-signal and aspeed,ext-push-pull are specified in
> + the node, set the pulse polarity to active-high. If aspeed,ext-active-high
> + is not specified then the pin is configured as active-low.
> +
> + aspeed,reset-mask:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 1
> + maxItems: 2
> + description: |
> + A bitmaks indicating which peripherals will be reset if the watchdog
> + timer expires. On AST2500 SoCs this should be a single word defined using
> + the AST2500_WDT_RESET_* macros; on AST2600 SoCs this should be a two-word
> + array with the first word defined using the AST2600_WDT_RESET1_* macros,
> + and the second word defined using the AST2600_WDT_RESET2_* macros.
> +
> +required:
> + - compatible
> + - reg
> +
> +allOf:
> + - if:
> + anyOf:
> + - required:
> + - aspeed,ext-push-pull
> + - required:
> + - aspeed,ext-active-high
> + - required:
> + - aspeed,reset-mask
> + then:
> + properties:
> + compatible:
> + enum:
> + - aspeed,ast2500-wdt
> + - aspeed,ast2600-wdt
> + - if:
> + required:
> + - aspeed,ext-active-high
> + then:
> + required:
> + - aspeed,ext-push-pull
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + wdt1: watchdog@1e785000 {
Drop unused labels.
> + compatible = "aspeed,ast2400-wdt";
> + reg = <0x1e785000 0x1c>;
> + aspeed,reset-type = "system";
> + aspeed,external-signal;
> + };
> + - |
> + #include <dt-bindings/watchdog/aspeed-wdt.h>
> + wdt2: watchdog@1e785040 {
> + compatible = "aspeed,ast2600-wdt";
> + reg = <0x1e785040 0x40>;
> + aspeed,reset-mask = <AST2600_WDT_RESET1_DEFAULT
> + (AST2600_WDT_RESET2_DEFAULT & ~AST2600_WDT_RESET2_LPC)>;
> + };
> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> deleted file mode 100644
> index 3208adb3e52e..000000000000
> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> +++ /dev/null
> @@ -1,73 +0,0 @@
> -Aspeed Watchdog Timer
> -
> -Required properties:
> - - compatible: must be one of:
> - - "aspeed,ast2400-wdt"
> - - "aspeed,ast2500-wdt"
> - - "aspeed,ast2600-wdt"
> -
> - - reg: physical base address of the controller and length of memory mapped
> - region
> -
> -Optional properties:
> -
> - - aspeed,reset-type = "cpu|soc|system|none"
> -
> - Reset behavior - Whenever a timeout occurs the watchdog can be programmed
> - to generate one of three different, mutually exclusive, types of resets.
> -
> - Type "none" can be specified to indicate that no resets are to be done.
> - This is useful in situations where another watchdog engine on chip is
> - to perform the reset.
> -
> - If 'aspeed,reset-type=' is not specified the default is to enable system
> - reset.
> -
> - Reset types:
> -
> - - cpu: Reset CPU on watchdog timeout
> -
> - - soc: Reset 'System on Chip' on watchdog timeout
> -
> - - system: Reset system on watchdog timeout
> -
> - - none: No reset is performed on timeout. Assumes another watchdog
> - engine is responsible for this.
> -
> - - aspeed,alt-boot: If property is present then boot from alternate block.
> - - aspeed,external-signal: If property is present then signal is sent to
> - external reset counter (only WDT1 and WDT2). If not
> - specified no external signal is sent.
> - - aspeed,ext-pulse-duration: External signal pulse duration in microseconds
> -
> -Optional properties for AST2500-compatible watchdogs:
> - - aspeed,ext-push-pull: If aspeed,external-signal is present, set the pin's
> - drive type to push-pull. The default is open-drain.
> - - aspeed,ext-active-high: If aspeed,external-signal is present and and the pin
> - is configured as push-pull, then set the pulse
> - polarity to active-high. The default is active-low.
> -
> -Optional properties for AST2500- and AST2600-compatible watchdogs:
> - - aspeed,reset-mask: A bitmask indicating which peripherals will be reset if
> - the watchdog timer expires. On AST2500 this should be a
> - single word defined using the AST2500_WDT_RESET_* macros;
> - on AST2600 this should be a two-word array with the first
> - word defined using the AST2600_WDT_RESET1_* macros and the
> - second word defined using the AST2600_WDT_RESET2_* macros.
> -
> -Examples:
> -
> - wdt1: watchdog@1e785000 {
> - compatible = "aspeed,ast2400-wdt";
> - reg = <0x1e785000 0x1c>;
> - aspeed,reset-type = "system";
> - aspeed,external-signal;
> - };
> -
> - #include <dt-bindings/watchdog/aspeed-wdt.h>
> - wdt2: watchdog@1e785040 {
> - compatible = "aspeed,ast2600-wdt";
> - reg = <0x1e785040 0x40>;
> - aspeed,reset-mask = <AST2600_WDT_RESET1_DEFAULT
> - (AST2600_WDT_RESET2_DEFAULT & ~AST2600_WDT_RESET2_LPC)>;
> - };
> --
> 2.39.2
>
^ permalink raw reply
* Re: [PATCH 1/3] dt-bindings: leds: add LED_FUNCTION_FNLOCK
From: Krzysztof Kozlowski @ 2024-04-02 18:08 UTC (permalink / raw)
To: Gergo Koteles, Ike Panhc, Hans de Goede, Ilpo Järvinen,
Pavel Machek, Lee Jones, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: platform-driver-x86, linux-kernel, linux-leds, devicetree
In-Reply-To: <5864594aa47ecfeb23d5d05a3afc02393f84b44e.camel@irl.hu>
On 02/04/2024 16:36, Gergo Koteles wrote:
> Hi Krzysztof,
>
> On Tue, 2024-04-02 at 15:55 +0200, Krzysztof Kozlowski wrote:
>>
>> Do we really need to define all these possible LED functions? Please
>> link to DTS user for this.
>>
>
> I think for userspace it's easier to support an LED with a specified
> name than to use various sysfs attributes. LED devices are easy to find
> because they available are in the /sys/class/leds/ directory.
> So I think it's a good thing to define LED names somewhere.
You did not add anything for user-space, but DT bindings. We do not keep
here anything for user-space.
>
> J Luke missed this LED from /sys/class/leds/, that's where the idea
> came from. The scrollock, numlock, capslock and kbd_backlight LEDs are
> already exported.
>
> https://github.com/tomsom/yoga-linux/issues/58#issuecomment-2029926094
I see there sysfs, so what does it have to do with bindings?
Again, please link to in-tree or in-review DTS.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: input: Add Himax HX83102J touchscreen
From: Conor Dooley @ 2024-04-02 18:09 UTC (permalink / raw)
To: Allen_Lin
Cc: dmitry.torokhov, robh, krzysztof.kozlowski+dt, jikos,
benjamin.tissoires, linux-input, devicetree, linux-kernel
In-Reply-To: <TY0PR06MB5611C37640AA40B2B7716ABE9E3E2@TY0PR06MB5611.apcprd06.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 4523 bytes --]
On Tue, Apr 02, 2024 at 06:49:27PM +0800, Allen_Lin wrote:
> Add the HX83102j touchscreen device tree bindings documents.
> HX83102j is a Himax TDDI touchscreen controller.
> It's power sequence should be bound with a lcm driver, thus it
> needs to be a panel follower. Others are the same as normal SPI
> touchscreen controller.
>
> Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
> ---
> .../input/touchscreen/himax,hx83102j.yaml | 100 ++++++++++++++++++
> MAINTAINERS | 6 ++
> 2 files changed, 106 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/himax,hx83102j.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83102j.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83102j.yaml
> new file mode 100644
> index 000000000000..fe79129f704a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83102j.yaml
> @@ -0,0 +1,100 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/himax,hx83102j.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Himax hx83102j touchscreen
> +
> +maintainers:
> + - Allen Lin <allencl_lin@hotmail.com>
> +
> +description:
> + This Himax hx83102j touchscreen uses the spi protocol.
> +
> +allOf:
> + - $ref: /schemas/input/touchscreen/touchscreen.yaml#
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> + const: himax,hx83102j
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + reset-gpios:
> + maxItems: 1
> +
> + vccd-supply:
> + description: A phandle for the regualtor supplying IO power.
nit: regulator
> +
> + vsn-supply:
> + description: Negative supply regulator.
> +
> + vsp-supply:
> + description: Positive supply regulator.
Cool, thanks for adding these.
> +
> + ddreset-gpios:
> + description: A phandle of gpio for display reset controlled by the LCD driver.
> + This is the master reset, if this reset is triggered, the TP reset will
> + also be triggered.
> +
> + spi-cpha: true
> +
> + spi-cpol: true
> +
> + spi-max-frequency: true
> +
> + panel: true
> +
> + himax,firmware-name:
firmware-name is a standard property, you don't need to vendor prefix it.
> + $ref: /schemas/types.yaml#/definitions/string
> + description:
> + Specify the file name for firmware loading.
> +
> + himax,pid:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + PID for HID device, used to validate firmware.
Why do you need this _and_ firmware-name? You should be able to trust
the firmware that the dt has told you to use, no?
Cheers,
Conor.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - reset-gpios
> + - panel
> + - vccd-supply
> + - vsn-supply
> + - vsp-supply
> + - ddreset-gpios
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + ap_ts: touchscreen@0 {
> + compatible = "himax,hx83102j";
> + reg = <0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&touch_int0 &touch_reset>;
> + reset-gpios = <&gpio1 8 GPIO_ACTIVE_LOW>;
> + spi-cpha;
> + spi-cpol;
> + interrupt-parent = <&gpio1>;
> + interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> + panel = <&panel>;
> + vccd-supply = <®ulator>;
> + vsn-supply = <®ulator>;
> + vsp-supply = <®ulator>;
> + ddreset-gpios = <&gpio1>;
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 43b39956694a..aa51c60fd66d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9669,6 +9669,12 @@ L: linux-kernel@vger.kernel.org
> S: Maintained
> F: drivers/misc/hisi_hikey_usb.c
>
> +HIMAX HID HX83102J TOUCHSCREEN
> +M: Allen Lin <allencl_lin@hotmail.com>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83102j.yaml
> +
> HIMAX HX83112B TOUCHSCREEN SUPPORT
> M: Job Noorman <job@noorman.info>
> L: linux-input@vger.kernel.org
> --
> 2.34.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 17/18] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property
From: Krzysztof Kozlowski @ 2024-04-02 18:10 UTC (permalink / raw)
To: Damien Le Moal, Manivannan Sadhasivam, Lorenzo Pieralisi,
Kishon Vijay Abraham I, Shawn Lin, Krzysztof Wilczyński,
Bjorn Helgaas, Heiko Stuebner, linux-pci, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree
Cc: linux-rockchip, linux-arm-kernel, Rick Wertenbroek,
Wilfred Mallawa, Niklas Cassel
In-Reply-To: <be2a0fa0-9d5d-45c3-810a-56d6924c8891@kernel.org>
On 02/04/2024 09:55, Damien Le Moal wrote:
> On 4/2/24 16:38, Damien Le Moal wrote:
>> On 4/2/24 16:33, Krzysztof Kozlowski wrote:
>>> On 02/04/2024 01:36, Damien Le Moal wrote:
>>>> On 4/1/24 18:57, Krzysztof Kozlowski wrote:
>>>>> On 01/04/2024 01:06, Damien Le Moal wrote:
>>>>>> On 3/30/24 18:16, Krzysztof Kozlowski wrote:
>>>>>>> On 30/03/2024 05:19, Damien Le Moal wrote:
>>>>>>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
>>>>>>>>
>>>>>>>> Describe the `ep-gpios` property which is used to map the PERST# input
>>>>>>>> signal for endpoint mode.
>>>>>>>>
>>>>>>>> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
>>>>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>>>>>>> ---
>>>>>>>> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 3 +++
>>>>>>>> 1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
>>>>>>>> index 6b62f6f58efe..9331d44d6963 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
>>>>>>>> @@ -30,6 +30,9 @@ properties:
>>>>>>>> maximum: 32
>>>>>>>> default: 32
>>>>>>>>
>>>>>>>> + ep-gpios:
>>>>>>>> + description: Input GPIO configured for the PERST# signal.
>>>>>>>
>>>>>>> Missing maxItems. But more important: why existing property perst-gpios,
>>>>>>> which you already have there in common schema, is not correct for this case?
>>>>>>
>>>>>> I am confused... Where do you find perst-gpios defined for the rk3399 ?
>>>>>> Under Documentation/devicetree/bindings/pci/, the only schema I see using
>>>>>> perst-gpios property are for the qcom (Qualcomm) controllers.
>>>>>
>>>>> You are right, it's so far only in Qualcomm.
>>>>>
>>>>>> The RC bindings for the rockchip rk3399 PCIe controller
>>>>>> (pci/rockchip,rk3399-pcie.yaml) already define the ep-gpios property. So if
>>>>>
>>>>> Any reason why this cannot be named like GPIO? Is there already a user
>>>>> of this in Linux kernel? Commit msg says nothing about this, so that's
>>>>> why I would expect name matching the signal.
>>>>
>>>> The RC-mode PCIe controller node of the rk3399 DTS already defines the ep-gpios
>>>> property for RC side PERST# signal handling. So we simply reused the exact same
>>>> name to be consistent between RC and EP. I personnally have no preferences. If
>>>> there is an effort to rename such signal with some preferred pattern, I will
>>>> follow. For the EP node, there was no PERST signal handling in the driver and
>>>> no property defined for it, so any name is fine. "perst-gpios" would indeed be
>>>> a better name, but again, given that the RC controller node has ep-gpios, we
>>>> reused that. What is your recommendation here ?
>>>
>>> Actually I don't know, perst and ep would work for me. If you do not
>>> have code for this in the driver yet (nothing is shared between ep and
>>> host), then maybe let's go with perst to match the actual name.
>>
>> That works for me. The other simple solution would be to move the RC node
>> ep-gpios description to the common schema pci/rockchip,rk3399-pcie-common.yaml,
>> maybe ? Otherwise, perst-gpios like the Qualcomm schemas would be nice too.
>
> Thinking more about this, I think moving the ep-gpios description to the common
> schema is the right thing to do given that the driver uses common code between
> RC and EP to get that property. But if that is not acceptable, I can rename it
> and get that property in the controller EP mode initialization code. That will
> be add a little more code in the driver.
I forgot that it is actually the same hardware, so if host has
"ep-gpios" already then EP mode should have the same property. Common
schema is good idea.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: iio: imu: add icm42688 inside inv_icm42600
From: Conor Dooley @ 2024-04-02 18:13 UTC (permalink / raw)
To: inv.git-commit
Cc: jic23, robh, krzysztof.kozlowski+dt, conor+dt, lars, linux-iio,
devicetree, Jean-Baptiste Maneyrol
In-Reply-To: <20240402090046.764572-2-inv.git-commit@tdk.com>
[-- Attachment #1: Type: text/plain, Size: 898 bytes --]
On Tue, Apr 02, 2024 at 09:00:45AM +0000, inv.git-commit@tdk.com wrote:
Acked-by: Conor Dooley <conor.dooley@microchip.com>
> TDK-Micronas GmbH
> Company Headquarters / Sitz der Gesellschaft: Freiburg i. Br. - Municipal Court of / Amtsgericht: Freiburg i. Br. HRB 6108. VAT ID / USt-IdNr.: DE 812878184
> Management / Gesch?ftsf?hrung: Sam Maddalena
>
> This e-mail and any files transmitted with it are confidential information of TDK-Micronas and intended solely for the use of the individual or entity to whom they are addressed. If you have received this e-mail in error please notify the sender by return e-mail and delete all copies of this e-mail message along with all attachments. If you are not the named addressee you should not disseminate, distribute or copy this e-mail.
Well, I didn't get it in error since I am an explicit CC, but you sent
this to a mailing list...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/3] dt-bindings: phy: Add i.MX8Q HSIO SerDes PHY binding
From: Conor Dooley @ 2024-04-02 18:16 UTC (permalink / raw)
To: Frank Li
Cc: Richard Zhu, vkoul, kishon, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-phy, devicetree, linux-arm-kernel, linux-kernel,
kernel, imx
In-Reply-To: <ZgwU8edE3VFYngWR@lizhi-Precision-Tower-5810>
[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]
On Tue, Apr 02, 2024 at 10:23:45AM -0400, Frank Li wrote:
> On Tue, Apr 02, 2024 at 01:45:03PM +0800, Richard Zhu wrote:
> > Add i.MX8QM and i.MX8QXP HSIO SerDes PHY binding.
> > - Use the controller ID to specify which controller is binded to the
> > PHY.
> > - Introduce one HSIO configuration, mandatory required to set
> > "PCIE_AB_SELECT" and "PHY_X1_EPCS_SEL" during the initialization.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
>
> You missed all conor's comments.
> Please double check v1's comments.
> > + fsl,refclk-pad-mode:
> > + description: |
> > + Specifies the mode of the refclk pad used. It can be UNUSED(PHY
> > + refclock is derived from SoC internal source), INPUT(PHY refclock
> > + is provided externally via the refclk pad) or OUTPUT(PHY refclock
> > + is derived from SoC internal source and provided on the refclk pad).
> > + Refer include/dt-bindings/phy/phy-imx8-pcie.h for the constants
> > + to be used.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [ 0, 1, 2 ]
>
> I remember needn't enum because there are header file.
Yah, specifically my comments about this property were missed and were
probably the most meaningful comments I left.
Thanks for the reminder Frank.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] dt-bindings: net: snps,dwmac: Align 'snps,priority' type definition
From: Conor Dooley @ 2024-04-02 18:20 UTC (permalink / raw)
To: Rob Herring
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Krzysztof Kozlowski, Conor Dooley, Alexandre Torgue,
Giuseppe Cavallaro, Jose Abreu, netdev, devicetree, linux-kernel
In-Reply-To: <20240401204422.1692359-2-robh@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 372 bytes --]
On Mon, Apr 01, 2024 at 03:44:22PM -0500, Rob Herring wrote:
> 'snps,priority' is also defined in dma/snps,dw-axi-dmac.yaml as a
> uint32-array. It's preferred to have a single type for a given property
> name, so update the type in snps,dwmac schema to match.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox