* Re: [PATCH v2 1/3] dt-bindings: display: mediatek: Add OF graph support for board path
From: Dmitry Baryshkov @ 2024-04-09 15:20 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: chunkuang.hu, robh, krzysztof.kozlowski+dt, conor+dt, p.zabel,
airlied, daniel, maarten.lankhorst, mripard, tzimmermann,
matthias.bgg, shawn.sung, yu-chang.lee, ck.hu, jitao.shi,
devicetree, linux-kernel, dri-devel, linux-mediatek,
linux-arm-kernel, wenst, kernel
In-Reply-To: <20240409120211.321153-2-angelogioacchino.delregno@collabora.com>
On Tue, Apr 09, 2024 at 02:02:09PM +0200, AngeloGioacchino Del Regno wrote:
> The display IPs in MediaTek SoCs support being interconnected with
> different instances of DDP IPs (for example, merge0 or merge1) and/or
> with different DDP IPs (for example, rdma can be connected with either
> color, dpi, dsi, merge, etc), forming a full Display Data Path that
> ends with an actual display.
>
> The final display pipeline is effectively board specific, as it does
> depend on the display that is attached to it, and eventually on the
> sensors supported by the board (for example, Adaptive Ambient Light
> would need an Ambient Light Sensor, otherwise it's pointless!), other
> than the output type.
With the color and gamma being in play, should the configuration be
board-driver or rather use-case driven with the driver being able to
reroute some of the blocks at runtime?
>
> Add support for OF graphs to most of the MediaTek DDP (display) bindings
> to add flexibility to build custom hardware paths, hence enabling board
> specific configuration of the display pipeline and allowing to finally
> migrate away from using hardcoded paths.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 2/6] soc: qcom: smem: Add pcode/fcode getters
From: Bjorn Andersson @ 2024-04-09 15:20 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-kernel,
dri-devel, freedreno, devicetree, Neil Armstrong
In-Reply-To: <20240405-topic-smem_speedbin-v1-2-ce2b864251b1@linaro.org>
On Fri, Apr 05, 2024 at 10:41:30AM +0200, Konrad Dybcio wrote:
> Introduce getters for SoC product and feature codes and export them.
>
Thought I commented on this already, but I don't see my reply...
Can you please elaborate on what this stuff is, such that we have a
track record in the history of this driver as well, for those of us that
don't know what "feature/product codes" contain or are good for (or have
forgotten next week).
Regards,
Bjorn
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> drivers/soc/qcom/smem.c | 66 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/soc/qcom/smem.h | 2 ++
> 2 files changed, 68 insertions(+)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 7191fa0c087f..e89b4d26877a 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -795,6 +795,72 @@ int qcom_smem_get_soc_id(u32 *id)
> }
> EXPORT_SYMBOL_GPL(qcom_smem_get_soc_id);
>
> +/**
> + * qcom_smem_get_feature_code() - return the feature code
> + * @id: On success, we return the feature code here.
> + *
> + * Look up the feature code identifier from SMEM and return it.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int qcom_smem_get_feature_code(u32 *code)
> +{
> + struct socinfo *info;
> + u32 raw_code;
> +
> + info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + /* This only makes sense for socinfo >= 16 */
> + if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
> + return -EINVAL;
> +
> + raw_code = __le32_to_cpu(info->feature_code);
> +
> + /* Ensure the value makes sense */
> + if (raw_code >= SOCINFO_FC_INT_RESERVE)
> + raw_code = SOCINFO_FC_UNKNOWN;
> +
> + *code = raw_code;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_get_feature_code);
> +
> +/**
> + * qcom_smem_get_product_code() - return the product code
> + * @id: On success, we return the product code here.
> + *
> + * Look up feature code identifier from SMEM and return it.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int qcom_smem_get_product_code(u32 *code)
> +{
> + struct socinfo *info;
> + u32 raw_code;
> +
> + info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + /* This only makes sense for socinfo >= 16 */
> + if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
> + return -EINVAL;
> +
> + raw_code = __le32_to_cpu(info->pcode);
> +
> + /* Ensure the value makes sense */
> + if (raw_code >= SOCINFO_FC_INT_RESERVE)
> + raw_code = SOCINFO_FC_UNKNOWN;
> +
> + *code = raw_code;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_get_product_code);
> +
> static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
> {
> struct smem_header *header;
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index a36a3b9d4929..aef8c9fc6c08 100644
> --- a/include/linux/soc/qcom/smem.h
> +++ b/include/linux/soc/qcom/smem.h
> @@ -13,5 +13,7 @@ int qcom_smem_get_free_space(unsigned host);
> phys_addr_t qcom_smem_virt_to_phys(void *p);
>
> int qcom_smem_get_soc_id(u32 *id);
> +int qcom_smem_get_feature_code(u32 *code);
> +int qcom_smem_get_product_code(u32 *code);
>
> #endif
>
> --
> 2.40.1
>
^ permalink raw reply
* Re: [PATCH 3/6] drm/msm/adreno: Allow specifying default speedbin value
From: Dmitry Baryshkov @ 2024-04-09 15:23 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Rob Clark, Abhinav Kumar, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-kernel,
dri-devel, freedreno, devicetree, Neil Armstrong
In-Reply-To: <d8a2ef87-f29e-4bdb-a9b8-591b8bd5d2b2@linaro.org>
On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote:
>
>
> On 4/6/24 04:56, Dmitry Baryshkov wrote:
> > On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote:
> > > From: Neil Armstrong <neil.armstrong@linaro.org>
> > >
> > > Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock
> > > the highest. Falling back to it when things go wrong is largely
> > > suboptimal, as more often than not, the top frequencies are not
> > > supposed to work on other bins.
> >
> > Isn't it better to just return an error here instead of trying to guess
> > which speedbin to use?
>
> Not sure. I'd rather better compatibility for e.g. booting up a new
> laptop with just dt.
New speedbin can have lower max speed, so by attempting to run it at
higher freq you might be breaking it.
>
> >
> > If that's not the case, I think the commit should be expanded with
> > actually setting default_speedbin for the existing GPUs.
>
> I think that should be addressed, although separately.
I'd prefer to have it as a part of this patch, but I'd not NAK it just
for this reason.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 5/6] drm/msm/adreno: Add speedbin data for SM8550 / A740
From: Dmitry Baryshkov @ 2024-04-09 15:24 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Rob Clark, Abhinav Kumar, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-kernel,
dri-devel, freedreno, devicetree, Neil Armstrong
In-Reply-To: <0955cabc-fc4e-4790-a82c-7f6f807fe36b@linaro.org>
On Tue, Apr 09, 2024 at 05:13:15PM +0200, Konrad Dybcio wrote:
>
>
> On 4/6/24 05:25, Dmitry Baryshkov wrote:
> > On Fri, Apr 05, 2024 at 10:41:33AM +0200, Konrad Dybcio wrote:
> > > Add speebin data for A740, as found on SM8550 and derivative SoCs.
> > >
> > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > > ---
> > > drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++++++++++++++
> > > 1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > index 901ef767e491..c976a485aef2 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > @@ -570,6 +570,20 @@ static const struct adreno_info gpulist[] = {
> > > .zapfw = "a740_zap.mdt",
> > > .hwcg = a740_hwcg,
> > > .address_space_size = SZ_16G,
> > > + .speedbins = ADRENO_SPEEDBINS(
> >
> > I think this deserves either a comment or some info in the commit
> > message.
>
> "this" = ?
I see two types of speedbins here, it would be nice to understand at
least some reason or some defailts for that (if you know them).
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 2/4] arm64: dts: qcom: msm8976: Add MDSS nodes
From: Bjorn Andersson @ 2024-04-09 15:24 UTC (permalink / raw)
To: Adam Skladowski
Cc: phone-devel, ~postmarketos/upstreaming, Andy Gross, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm,
devicetree, linux-kernel
In-Reply-To: <20240401172153.9231-3-a39.skl@gmail.com>
On Mon, Apr 01, 2024 at 07:21:51PM +0200, Adam Skladowski wrote:
> diff --git a/arch/arm64/boot/dts/qcom/msm8976.dtsi b/arch/arm64/boot/dts/qcom/msm8976.dtsi
[..]
> + mdss: display-subsystem@1a00000 {
[..]
> + mdss_dsi0: dsi@1a94000 {
> + compatible = "qcom,msm8976-dsi-ctrl", "qcom,mdss-dsi-ctrl";
> + reg = <0x01a94000 0x25c>;
> + reg-names = "dsi_ctrl";
> +
> + interrupt-parent = <&mdss>;
> + interrupts = <4>;
> +
> + clocks = <&gcc GCC_MDSS_MDP_CLK>,
> + <&gcc GCC_MDSS_AHB_CLK>,
> + <&gcc GCC_MDSS_AXI_CLK>,
> + <&gcc GCC_MDSS_BYTE0_CLK>,
> + <&gcc GCC_MDSS_PCLK0_CLK>,
> + <&gcc GCC_MDSS_ESC0_CLK>;
> + clock-names = "mdp_core",
> + "iface",
> + "bus",
> + "byte",
> + "pixel",
> + "core";
> +
> + assigned-clocks = <&gcc GCC_MDSS_BYTE0_CLK_SRC>,
> + <&gcc GCC_MDSS_PCLK0_CLK_SRC>;
> + assigned-clock-parents = <&mdss_dsi0_phy 0>,
> + <&mdss_dsi0_phy 1>;
> +
> + phys = <&mdss_dsi0_phy>;
> +
> + operating-points-v2 = <&dsi0_opp_table>;
> + power-domains = <&gcc MDSS_GDSC>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
Seems reasonable to keep this disabled as well. Further more &mdss_dsi0
depends on &mdss_dsi0_phy which is disabled.
> + ports {
[..]
> + mdss_dsi0_phy: phy@1a94a00 {
> + compatible = "qcom,dsi-phy-28nm-hpm-fam-b";
> + reg = <0x01a94a00 0xd4>,
> + <0x01a94400 0x280>,
> + <0x01a94b80 0x30>;
> + reg-names = "dsi_pll",
> + "dsi_phy",
> + "dsi_phy_regulator";
> +
> + #clock-cells = <1>;
> + #phy-cells = <0>;
> +
> + clocks = <&gcc GCC_MDSS_AHB_CLK>,
> + <&rpmcc RPM_SMD_XO_CLK_SRC>;
> + clock-names = "iface", "ref";
> +
> + status = "disabled";
> + };
PS. Leave &mdss_mdp enabled...
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH v2 3/4] arm64: dts: qcom: msm8976: Add Adreno GPU
From: Bjorn Andersson @ 2024-04-09 15:25 UTC (permalink / raw)
To: Adam Skladowski
Cc: phone-devel, ~postmarketos/upstreaming, Andy Gross, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm,
devicetree, linux-kernel
In-Reply-To: <20240401172153.9231-4-a39.skl@gmail.com>
On Mon, Apr 01, 2024 at 07:21:52PM +0200, Adam Skladowski wrote:
> Add Adreno GPU node.
>
> Signed-off-by: Adam Skladowski <a39.skl@gmail.com>
> ---
> arch/arm64/boot/dts/qcom/msm8976.dtsi | 65 +++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8976.dtsi b/arch/arm64/boot/dts/qcom/msm8976.dtsi
> index 6be310079f5b..77670fce9b8f 100644
> --- a/arch/arm64/boot/dts/qcom/msm8976.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8976.dtsi
> @@ -1074,6 +1074,71 @@ mdss_dsi1_phy: phy@1a96a00 {
> };
> };
>
> + adreno_gpu: gpu@1c00000 {
> + compatible = "qcom,adreno-510.0", "qcom,adreno";
> +
> + reg = <0x01c00000 0x40000>;
> + reg-names = "kgsl_3d0_reg_memory";
> +
> + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "kgsl_3d0_irq";
> +
> + clocks = <&gcc GCC_GFX3D_OXILI_CLK>,
> + <&gcc GCC_GFX3D_OXILI_AHB_CLK>,
> + <&gcc GCC_GFX3D_OXILI_GMEM_CLK>,
> + <&gcc GCC_GFX3D_BIMC_CLK>,
> + <&gcc GCC_GFX3D_OXILI_TIMER_CLK>,
> + <&gcc GCC_GFX3D_OXILI_AON_CLK>;
> + clock-names = "core",
> + "iface",
> + "mem",
> + "mem_iface",
> + "rbbmtimer",
> + "alwayson";
> +
> + power-domains = <&gcc OXILI_GX_GDSC>;
> +
> + iommus = <&gpu_iommu 0>;
> +
> + status = "disabled";
Make status the last property of the node, please.
Regards,
Bjorn
> +
> + operating-points-v2 = <&gpu_opp_table>;
> +
> + gpu_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
^ permalink raw reply
* Re: [PATCH v8 3/8] perf: imx_perf: let the driver manage the counter usage rather the user
From: Will Deacon @ 2024-04-09 15:26 UTC (permalink / raw)
To: Xu Yang
Cc: frank.li, mark.rutland, robh+dt, krzysztof.kozlowski+dt, conor+dt,
shawnguo, s.hauer, kernel, festevam, john.g.garry, jolsa,
namhyung, irogers, mike.leach, peterz, mingo, acme,
alexander.shishkin, adrian.hunter, linux-arm-kernel, devicetree,
linux-kernel, linux-perf-users, imx
In-Reply-To: <20240322063930.749126-3-xu.yang_2@nxp.com>
On Fri, Mar 22, 2024 at 02:39:25PM +0800, Xu Yang wrote:
> In current design, the user of perf app needs to input counter ID to count
> events. However, this is not user-friendly since the user needs to lookup
> the map table to find the counter. Instead of letting the user to input
> the counter, let this driver to manage the counters in this patch.
I think we still have to support the old interface so that we don't break
those existing users (even if the driver just ignores whatever counter ID
is provided in a backwards-compatible way).
> This will be implemented by:
> 1. allocate counter 0 for cycle event.
> 2. find unused counter from 1-10 for reference events.
> 3. allocate specific counter for counter-specific events.
>
> In this patch, counter attribute is removed too. To mark counter-specific
> events, counter ID will be encoded into perf_pmu_events_attr.id.
>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>
> ---
> Changes in v6:
> - new patch
> Changes in v7:
> - no changes
> Changes in v8:
> - add Rb tag
> ---
> drivers/perf/fsl_imx9_ddr_perf.c | 168 ++++++++++++++++++-------------
> 1 file changed, 99 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> index 0017f2c9ef91..b728719b494c 100644
> --- a/drivers/perf/fsl_imx9_ddr_perf.c
> +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> @@ -245,14 +249,12 @@ static const struct attribute_group ddr_perf_events_attr_group = {
> .attrs = ddr_perf_events_attrs,
> };
>
> -PMU_FORMAT_ATTR(event, "config:0-7");
> -PMU_FORMAT_ATTR(counter, "config:8-15");
> +PMU_FORMAT_ATTR(event, "config:0-15");
Sadly, this is a user-visible change so I think it will break old tools,
won't it?
Will
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: usb: mtk-xhci: add compatible for MT7988
From: Rob Herring @ 2024-04-09 15:26 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Matthias Brugger, AngeloGioacchino Del Regno, Krzysztof Kozlowski,
Conor Dooley, Chunfeng Yun, Greg Kroah-Hartman, Daniel Golle,
linux-usb, linux-arm-kernel, linux-mediatek, devicetree,
linux-kernel, Rafał Miłecki
In-Reply-To: <20240213130044.1976-1-zajec5@gmail.com>
On Tue, Feb 13, 2024 at 02:00:43PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> MT7988 SoC contains two on-SoC XHCI controllers. Add proper binding.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml | 1 +
> 1 file changed, 1 insertion(+)
Seems like this got missed. Applied now.
Rob
^ permalink raw reply
* Re: [PATCH net-next v2 4/5] net: stmmac: add support for RZ/N1 GMAC
From: Geert Uytterhoeven @ 2024-04-09 15:27 UTC (permalink / raw)
To: Romain Gantois
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Alexandre Torgue, Jose Abreu,
Maxime Coquelin, Russell King, Clément Léger,
Thomas Petazzoni, netdev, devicetree, linux-kernel,
linux-renesas-soc, linux-stm32, linux-arm-kernel
In-Reply-To: <20240409-rzn1-gmac1-v2-4-79ca45f2fc79@bootlin.com>
Hi Romain,
On Tue, Apr 9, 2024 at 11:21 AM Romain Gantois
<romain.gantois@bootlin.com> wrote:
> From: Clément Léger <clement.leger@bootlin.com>
>
> Add support for the Renesas RZ/N1 GMAC. This support can make use of a
> custom RZ/N1 PCS which is fetched by parsing the pcs-handle device tree
> property.
>
> Signed-off-by: "Clément Léger" <clement.leger@bootlin.com>
> Co-developed-by: Romain Gantois <romain.gantois@bootlin.com>
> Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
Thanks for your patch!
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -142,6 +142,18 @@ config DWMAC_ROCKCHIP
> This selects the Rockchip RK3288 SoC glue layer support for
> the stmmac device driver.
>
> +config DWMAC_RZN1
> + tristate "Renesas RZ/N1 dwmac support"
> + default ARCH_RZN1
Why default to enabled?
> + depends on OF && (ARCH_RZN1 || COMPILE_TEST)
> + select PCS_RZN1_MIIC
> + help
> + Support for Ethernet controller on Renesas RZ/N1 SoC family.
> +
> + This selects the Renesas RZ/N1 SoC glue layer support for
> + the stmmac device driver. This support can make use of a custom MII
> + converter PCS device.
> +
> config DWMAC_SOCFPGA
> tristate "SOCFPGA dwmac support"
> default ARCH_INTEL_SOCFPGA
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000
From: Marcelo Schmitt @ 2024-04-09 15:30 UTC (permalink / raw)
To: David Lechner
Cc: Marcelo Schmitt, lars, Michael.Hennerich, jic23, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-iio, devicetree,
linux-kernel
In-Reply-To: <CAMknhBGKNZhGbD7pQ0Z7SMCWqxqGux0LcO_wW0XGP4hLTOwNBg@mail.gmail.com>
On 04/08, David Lechner wrote:
> On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
> >
> > Add device tree documentation for AD4000 family of ADC devices.
> >
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> >
>
> Suggested-by: David Lechner <dlechner@baylibre.com>
>
> (if you still use mostly my suggestions in the end)
Yes, it's been of great help. Will include the tag in future ad4000 DT patches.
>
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> > .../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++++++++++++++
> > MAINTAINERS | 7 +
> > 2 files changed, 208 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > new file mode 100644
> > index 000000000000..ca06afb5149e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > @@ -0,0 +1,201 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices AD4000 and similar Analog to Digital Converters
> > +
> > +maintainers:
> > + - Marcelo Schmitt <marcelo.schmitt@analog.com>
> > +
> > +description: |
> > + Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
> > + Specifications can be found at:
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> > +
> > +$ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - adi,ad4000
> > + - adi,ad4001
> > + - adi,ad4002
> > + - adi,ad4003
> > + - adi,ad4004
> > + - adi,ad4005
> > + - adi,ad4006
> > + - adi,ad4007
> > + - adi,ad4008
> > + - adi,ad4010
> > + - adi,ad4011
> > + - adi,ad4020
> > + - adi,ad4021
> > + - adi,ad4022
> > + - adi,adaq4001
> > + - adi,adaq4003
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + spi-max-frequency:
> > + maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> > +
> > + spi-cpha: true
> > +
> > + adi,spi-mode:
> > + $ref: /schemas/types.yaml#/definitions/string
> > + enum: [ single, chain ]
>
> It sounds like there are more possible wiring configurations for these
> chips that I thought when suggesting reusing this binding from AD7944
> so we probably need more options here. (see my reply to the cover
> letter for the complete context of these remarks)
>
> We identified A) an additional wiring configuration where SDI of the
> ADC chip is wired to SDO of the SPI controller and B) a potential need
> to pin mux between wiring modes to work around SPI controller
> limitations perhaps we could omit the adi,spi-mode property and just
> use the standard pinctrl properties.
>
> pinctrl-names:
> description: |
> Names for possible ways the SDI line of the controller is wired.
>
> * default: The SDI line of the ADC is connected to the SDO line of the
> SPI controller. CNV line of the ADC is connected to CS of the SPI
> controller.
Not sure if should be DT, but maybe also point out that in default mode the
SPI controller must be capable of keeping ADC SDI (controller SDO) line high
during ADC conversions.
> * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> definition of 3-wire mode is NOT at all related to the standard
> spi-3wire property!) In this mode, SDI is tied to VIO, and the CNV line
> can be connected to the CS line of the SPI controller (typical) or to a
> GPIO, in which case the CS line of the controller is unused. The SDO
> line of the SPI controller is not connected.
> * multi: The datasheet calls this "4-wire mode" and is used when multiple
> chips are connected in parallel. In this mode, the ADC SDI line is tied
> to the CS line on the SPI controller and the CNV line is connected to
> a GPIO. The SDO line of the SPI controller is not connected.
> * chain: The datasheet calls this "chain mode". This mode is used to save
> on wiring when multiple ADCs are used. In this mode, the SDI line of
> one chip is tied to the SDO of the next chip in the chain and the SDI of
> the last chip in the chain is tied to GND. Only the first chip in the
> chain is connected to the SPI bus. The CNV line of all chips are tied
> together. The CS line of the SPI controller can be used as the CNV line
> only if it is active high.
>
> If one name is specified, it is assumed the chip is hard-wired in this
> configuration.
>
> If two names are specified, it is assumed that a pinmux can switch between
> the two wiring configurations. The first is the default mode for reading
> and writing registers on the chip and the second is the mode for reading
> the conversion data from the chip.
> oneOf:
> - items:
> - enum:
> - default
> - single
> - multi
> - chain
> - items:
> - const: default
> - enum:
> - single
> - multi
> - chain
>
> pinctrl-0:
> maxItems: 1
>
> pinctrl-1:
> maxItems: 1
>
>
> > + description: |
> > + This property indicates the SPI wiring configuration.
> > +
> > + When this property is omitted, it is assumed that the device is using what
> > + the datasheet calls "4-wire mode". This is the conventional SPI mode used
> > + when there are multiple devices on the same bus. In this mode, the CNV
> > + line is used to initiate the conversion and the SDI line is connected to
> > + CS on the SPI controller.
> > +
> > + When this property is present, it indicates that the device is using one
> > + of the following alternative wiring configurations:
> > +
> > + * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> > + definition of 3-wire mode is NOT at all related to the standard
> > + spi-3wire property!) This mode is often used when the ADC is the only
> > + device on the bus. In this mode, SDI is tied to VIO, and the CNV line
> > + can be connected to the CS line of the SPI controller or to a GPIO, in
> > + which case the CS line of the controller is unused.
> > + * chain: The datasheet calls this "chain mode". This mode is used to save
> > + on wiring when multiple ADCs are used. In this mode, the SDI line of
> > + one chip is tied to the SDO of the next chip in the chain and the SDI of
> > + the last chip in the chain is tied to GND. Only the first chip in the
> > + chain is connected to the SPI bus. The CNV line of all chips are tied
> > + together. The CS line of the SPI controller can be used as the CNV line
> > + only if it is active high.
> > +
> > + '#daisy-chained-devices': true
> > +
> > + vdd-supply:
> > + description: A 1.8V supply that powers the chip (VDD).
> > +
> > + vio-supply:
> > + description:
> > + A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
> > +
> > + ref-supply:
> > + description:
> > + A 2.5 to 5V supply for the external reference voltage (REF).
> > +
> > + cnv-gpios:
> > + description:
> > + The Convert Input (CNV). This input has multiple functions. It initiates
> > + the conversions and selects the SPI mode of the device (chain or CS). In
> > + 'single' mode, this property is omitted if the CNV pin is connected to the
> > + CS line of the SPI controller. If 'single' mode is selected and this GPIO
> > + is provided, it must be active low.
>
> Since the conversion is triggered on the low to high transition of
> CNV, I think it only makes sense to have it active high and not active
> low.
The idea was to use the GPIO as a replacement for the controller CS when
in "3-wire"/single mode so we could have simpler handling of SPI transfers.
But if changing transfer to avoid latency then this might not simplify anything
anymore. Will probably drop this last line.
>
> > + maxItems: 1
> > +
> > + adi,high-z-input:
> > + type: boolean
> > + description:
> > + High-Z mode allows the amplifier and RC filter in front of the ADC to be
> > + chosen based on the signal bandwidth of interest, rather than the settling
> > + requirements of the switched capacitor SAR ADC inputs.
> > +
> > + adi,gain-milli:
> > + description: |
> > + The hardware gain applied to the ADC input (in milli units).
> > + The gain provided by the ADC input scaler is defined by the hardware
> > + connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
> > + If not present, default to 1000 (no actual gain applied).
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [454, 909, 1000, 1900]
> > + default: 1000
>
> Same suggestion as in V1 - we should make it clear that this property
> only applies to ADAQ chips (in the description and also a -if: for the
> bindings validator). Also, looking at the datasheet, it looks like
> there are a lot more pins on the ADAQ chips, so I think there are more
> properties missing here.
>
> Some trivial ones:
>
> vs-pos-supply (VS+ pin, 0 to 11V supply) and vs-neg-supply (VS- pin,
> -11 to 0V supply)
>
> pd-amp-gpios (active low) and pd-ref-gpios (active low) for optional
> runtime power management.
Ok, will have closer look to these and other pins described in the datasheet and
include them here too.
>
> Also the datasheet says the ADAQ chips supports "Single-ended to
> differential conversion". So it seems like we might need some extra
> properties to describe that case (a flag for indicating single-ended
> wiring and an optional voltage supply to describe what is connected to
> the negative input if it isn't tied to GND)
Yes, the differential ADCs also support "Single-ended to differential conversion".
Will provide support those too.
^ permalink raw reply
* Re: [PATCH v6 00/16] power: sequencing: implement the subsystem and add first users
From: Bartosz Golaszewski @ 2024-04-09 15:35 UTC (permalink / raw)
To: Xilin Wu
Cc: Marcel Holtmann, Luiz Augusto von Dentz, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Kalle Valo, Bjorn Andersson,
Konrad Dybcio, Liam Girdwood, Mark Brown, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Saravana Kannan, Geert Uytterhoeven,
Arnd Bergmann, Neil Armstrong, Marek Szyprowski, Alex Elder,
Srini Kandagatla, Greg Kroah-Hartman, Abel Vesa,
Manivannan Sadhasivam, Lukas Wunner, Dmitry Baryshkov,
linux-bluetooth, netdev, devicetree, linux-kernel, linux-wireless,
linux-arm-msm, linux-arm-kernel, linux-pci, linux-pm,
Bartosz Golaszewski
In-Reply-To: <6b63d5d2-5f30-4fbd-a872-91f32dc32c87@gmail.com>
On Sat, Apr 6, 2024 at 5:03 AM Xilin Wu <wuxilin123@gmail.com> wrote:
>
> I tested the patchset on SM8550 and it does give me working WiFi. However I
> seethe following warnings during boot.
>
> [ 5.973011] mhi mhi0: Requested to power ON
> [ 6.597591] mhi mhi0: Power on setup success
> [ 6.597631] sysfs: cannot create duplicate filename '/devices/platform/soc@0/1c00000.pcie/pci0000:00/0000:00:00.0/resource0'
> [ 6.597634] CPU: 7 PID: 154 Comm: kworker/u32:5 Tainted: G S 6.9.0-rc1-next-20240328-g955237c9980c #1
> [ 6.597635] Hardware name: AYN Odin 2 (DT)
> [ 6.597637] Workqueue: async async_run_entry_fn
> [ 6.597645] Call trace:
> [ 6.597646] dump_backtrace+0xa0/0x128
> [ 6.597649] show_stack+0x20/0x38
> [ 6.597650] dump_stack_lvl+0x74/0x90
> [ 6.597653] dump_stack+0x18/0x28
> [ 6.597654] sysfs_warn_dup+0x6c/0x90
> [ 6.597658] sysfs_add_bin_file_mode_ns+0xdc/0x100
> [ 6.597660] sysfs_create_bin_file+0x7c/0xb8
> [ 6.597662] pci_create_attr+0xb4/0x1a8
> [ 6.597665] pci_create_resource_files+0x64/0xd0
> [ 6.597667] pci_create_sysfs_dev_files+0x24/0x40
> [ 6.597669] pci_bus_add_device+0x54/0x138
> [ 6.597670] pci_bus_add_devices+0x40/0x98
> [ 6.597672] pci_host_probe+0x70/0xf0
> [ 6.597673] dw_pcie_host_init+0x248/0x658
> [ 6.597676] qcom_pcie_probe+0x234/0x330
> [ 6.597677] platform_probe+0x70/0xd8
> [ 6.597680] really_probe+0xc8/0x3a0
> [ 6.597681] __driver_probe_device+0x84/0x170
> [ 6.597682] driver_probe_device+0x44/0x120
> [ 6.597683] __device_attach_driver+0xc4/0x168
> [ 6.597684] bus_for_each_drv+0x8c/0xf0
> [ 6.597686] __device_attach_async_helper+0xb4/0x118
> [ 6.597687] async_run_entry_fn+0x40/0x178
> [ 6.597689] process_one_work+0x16c/0x410
> [ 6.597691] worker_thread+0x284/0x3a0
> [ 6.597693] kthread+0x118/0x128
> [ 6.597693] ret_from_fork+0x10/0x20
> [ 6.597698] ------------[ cut here ]------------
> [ 6.597698] proc_dir_entry '0000:00/00.0' already registered
> [ 6.597710] WARNING: CPU: 7 PID: 154 at fs/proc/generic.c:375 proc_register+0x138/0x1d0
> [ 6.597713] Modules linked in:
> [ 6.597714] CPU: 7 PID: 154 Comm: kworker/u32:5 Tainted: G S 6.9.0-rc1-next-20240328-g955237c9980c #1
> [ 6.597715] Hardware name: AYN Odin 2 (DT)
> [ 6.597716] Workqueue: async async_run_entry_fn
> [ 6.597718] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> [ 6.597719] pc : proc_register+0x138/0x1d0
> [ 6.597721] lr : proc_register+0x138/0x1d0
> [ 6.597723] sp : ffff800081e3b9a0
> [ 6.597723] x29: ffff800081e3b9a0 x28: 0000000000000000 x27: ffffddb2a28eabe0
> [ 6.597725] x26: ffff3425c9ada5c0 x25: ffffddb2a2d4eef0 x24: ffff3425c9ada540
> [ 6.597726] x23: 0000000000000004 x22: ffff3425c7b1822c x21: 0000000000000004
> [ 6.597727] x20: ffff3425c7b18180 x19: ffff3425c9adaec8 x18: ffffffffffffffff
> [ 6.597729] x17: 3040636f732f6d72 x16: 6f6674616c702f73 x15: ffff800081e3b910
> [ 6.597730] x14: 0000000000000000 x13: 0a64657265747369 x12: 6765722079646165
> [ 6.597731] x11: fffffffffff00000 x10: ffffddb2a27c4fb0 x9 : ffffddb29f5d7528
> [ 6.597733] x8 : 00000000ffff7fff x7 : ffffddb2a27c4fb0 x6 : 80000000ffff8000
> [ 6.597734] x5 : 0000000000000358 x4 : 0000000000000000 x3 : 00000000ffffffff
> [ 6.597736] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff3425c5ce0000
> [ 6.597737] Call trace:
> [ 6.597737] proc_register+0x138/0x1d0
> [ 6.597739] proc_create_data+0x48/0x78
> [ 6.597741] pci_proc_attach_device+0x84/0x118
> [ 6.597743] pci_bus_add_device+0x5c/0x138
> [ 6.597744] pci_bus_add_devices+0x40/0x98
> [ 6.597745] pci_host_probe+0x70/0xf0
> [ 6.597746] dw_pcie_host_init+0x248/0x658
> [ 6.597748] qcom_pcie_probe+0x234/0x330
> [ 6.597749] platform_probe+0x70/0xd8
> [ 6.597750] really_probe+0xc8/0x3a0
> [ 6.597751] __driver_probe_device+0x84/0x170
> [ 6.597752] driver_probe_device+0x44/0x120
> [ 6.597753] __device_attach_driver+0xc4/0x168
> [ 6.597754] bus_for_each_drv+0x8c/0xf0
> [ 6.597756] __device_attach_async_helper+0xb4/0x118
> [ 6.597757] async_run_entry_fn+0x40/0x178
> [ 6.597759] process_one_work+0x16c/0x410
> [ 6.597760] worker_thread+0x284/0x3a0
> [ 6.597761] kthread+0x118/0x128
> [ 6.597762] ret_from_fork+0x10/0x20
> [ 6.597763] ---[ end trace 0000000000000000 ]---
>
> This probably only occurs when the relevant drivers on compiled as built-in.
> Similar behavior has been noticed before as well:
>
> https://lore.kernel.org/lkml/20240201155532.49707-1-brgl@bgdev.pl/T/#mdeeca9bc8e19458787d53738298abcfff443068a
>
> Thanks,
> Xilin
>
Thanks for the report. The reason for this was populating the platform
devices before the bridge device was fully added. In case of loadable
modules this meant the pwrctl probe would be deferred long enough for
that to complete so I didn't see it but with pwrctl built-in this
would trigger the problem. I fixed it locally and will resend with
that addressed.
Bart
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: display: mediatek: Add OF graph support for board path
From: AngeloGioacchino Del Regno @ 2024-04-09 15:41 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: chunkuang.hu, robh, krzysztof.kozlowski+dt, conor+dt, p.zabel,
airlied, daniel, maarten.lankhorst, mripard, tzimmermann,
matthias.bgg, shawn.sung, yu-chang.lee, ck.hu, jitao.shi,
devicetree, linux-kernel, dri-devel, linux-mediatek,
linux-arm-kernel, wenst, kernel
In-Reply-To: <oe75tx35rd27r2a24ofdxfaqwr53tylfp5fwz3nrwc2uz6nmrs@vwc2krbpy3fh>
Il 09/04/24 17:20, Dmitry Baryshkov ha scritto:
> On Tue, Apr 09, 2024 at 02:02:09PM +0200, AngeloGioacchino Del Regno wrote:
>> The display IPs in MediaTek SoCs support being interconnected with
>> different instances of DDP IPs (for example, merge0 or merge1) and/or
>> with different DDP IPs (for example, rdma can be connected with either
>> color, dpi, dsi, merge, etc), forming a full Display Data Path that
>> ends with an actual display.
>>
>> The final display pipeline is effectively board specific, as it does
>> depend on the display that is attached to it, and eventually on the
>> sensors supported by the board (for example, Adaptive Ambient Light
>> would need an Ambient Light Sensor, otherwise it's pointless!), other
>> than the output type.
>
> With the color and gamma being in play, should the configuration be
> board-driver or rather use-case driven with the driver being able to
> reroute some of the blocks at runtime?
>
The driver can already set some blocks to "BYPASS MODE" at runtime, meaning
that those will work as simple pass-through, performing *no* processing at
all, so that's addressed from the very beginning.
This doesn't mean that a specific pipeline must always support the "DISP_GAMMA"
or the "DISP_CCOLOR" block(s) alone, or together, or in combination with another
specific block.
For any other question, clarification, etc, I'm here :-)
Cheers!
>>
>> Add support for OF graphs to most of the MediaTek DDP (display) bindings
>> to add flexibility to build custom hardware paths, hence enabling board
>> specific configuration of the display pipeline and allowing to finally
>> migrate away from using hardcoded paths.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: display: mediatek: Add OF graph support for board path
From: Dmitry Baryshkov @ 2024-04-09 15:45 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: chunkuang.hu, robh, krzysztof.kozlowski+dt, conor+dt, p.zabel,
airlied, daniel, maarten.lankhorst, mripard, tzimmermann,
matthias.bgg, shawn.sung, yu-chang.lee, ck.hu, jitao.shi,
devicetree, linux-kernel, dri-devel, linux-mediatek,
linux-arm-kernel, wenst, kernel
In-Reply-To: <8600acf8-7b51-456b-8a81-4233cfd6f121@collabora.com>
On Tue, 9 Apr 2024 at 18:41, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 09/04/24 17:20, Dmitry Baryshkov ha scritto:
> > On Tue, Apr 09, 2024 at 02:02:09PM +0200, AngeloGioacchino Del Regno wrote:
> >> The display IPs in MediaTek SoCs support being interconnected with
> >> different instances of DDP IPs (for example, merge0 or merge1) and/or
> >> with different DDP IPs (for example, rdma can be connected with either
> >> color, dpi, dsi, merge, etc), forming a full Display Data Path that
> >> ends with an actual display.
> >>
> >> The final display pipeline is effectively board specific, as it does
> >> depend on the display that is attached to it, and eventually on the
> >> sensors supported by the board (for example, Adaptive Ambient Light
> >> would need an Ambient Light Sensor, otherwise it's pointless!), other
> >> than the output type.
> >
> > With the color and gamma being in play, should the configuration be
> > board-driver or rather use-case driven with the driver being able to
> > reroute some of the blocks at runtime?
> >
>
> The driver can already set some blocks to "BYPASS MODE" at runtime, meaning
> that those will work as simple pass-through, performing *no* processing at
> all, so that's addressed from the very beginning.
>
> This doesn't mean that a specific pipeline must always support the "DISP_GAMMA"
> or the "DISP_CCOLOR" block(s) alone, or together, or in combination with another
> specific block.
I was thinking about slightly different case: do you have enough
colour blocks to drive all outputs or do you have to select them for
the particular output only?
(excuse me, I didn't check the platform details).
> For any other question, clarification, etc, I'm here :-)
>
> Cheers!
>
> >>
> >> Add support for OF graphs to most of the MediaTek DDP (display) bindings
> >> to add flexibility to build custom hardware paths, hence enabling board
> >> specific configuration of the display pipeline and allowing to finally
> >> migrate away from using hardcoded paths.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v3 01/18] ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document
From: Krzysztof Kozlowski @ 2024-04-09 15:46 UTC (permalink / raw)
To: Alexandre Mergnat, Liam Girdwood, Mark Brown, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones, Flora Fu, Jaroslav Kysela,
Takashi Iwai, Sumit Semwal, Christian König, Catalin Marinas,
Will Deacon, Rob Herring
Cc: linux-sound, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-media, dri-devel, linaro-mm-sig
In-Reply-To: <20240226-audio-i350-v3-1-16bb2c974c55@baylibre.com>
On 09/04/2024 15:41, Alexandre Mergnat wrote:
> Add MT8365 audio front-end bindings
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
> +properties:
> + compatible:
> + const: mediatek,mt8365-afe-pcm
> +
> + reg:
> + maxItems: 1
> +
> + "#sound-dai-cells":
> + const: 0
> +
> + clocks:
> + items:
> + - description: 26M clock
> + - description: mux for audio clock
> + - description: audio i2s0 mck
> + - description: audio i2s1 mck
> + - description: audio i2s2 mck
> + - description: audio i2s3 mck
> + - description: engen 1 clock
> + - description: engen 2 clock
> + - description: audio 1 clock
> + - description: audio 2 clock
> + - description: mux for i2s0
> + - description: mux for i2s1
> + - description: mux for i2s2
> + - description: mux for i2s3
> +
> + clock-names:
> + items:
> + - const: top_clk26m_clk
> + - const: top_audio_sel
> + - const: audio_i2s0_m
> + - const: audio_i2s1_m
> + - const: audio_i2s2_m
> + - const: audio_i2s3_m
> + - const: engen1
> + - const: engen2
> + - const: aud1
> + - const: aud2
> + - const: i2s0_m_sel
> + - const: i2s1_m_sel
> + - const: i2s2_m_sel
> + - const: i2s3_m_sel
> +
> + interrupts:
> + maxItems: 1
> +
> + power-domains:
> + maxItems: 1
> +
> + mediatek,dmic-mode:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Indicates how many data pins are used to transmit two channels of PDM
> + signal. 1 means two wires, 0 means one wire. Default value is 0.
> + enum:
> + - 0 # one wire
> + - 1 # two wires
> +
> + mediatek,topckgen:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: The phandle of the mediatek topckgen controller
Nothing improved, so again, so something which is not obvious. What is
it used for? Why AFE needs topckgen for example?
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - interrupts
> + - power-domains
> + - mediatek,topckgen
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/mediatek,mt8365-clk.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/power/mediatek,mt8365-power.h>
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + afe@11220000 {
Did you implement the comment or decided to keep afe?
BTW, whatever "consistency" you have in mind, it does not really matter
that much for that example. And for sure do not add incorrect code
intentionally just to fix it in next patch.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH RFC 02/11] dt-bindings: riscv: Add Sdtrig optional CSRs existence on DT
From: Conor Dooley @ 2024-04-09 15:49 UTC (permalink / raw)
To: Andrew Jones
Cc: Conor Dooley, Max Hsu, Rob Herring, Krzysztof Kozlowski,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Rafael J. Wysocki,
Pavel Machek, Anup Patel, Atish Patra, Paolo Bonzini, Shuah Khan,
Palmer Dabbelt, linux-riscv, devicetree, linux-kernel, linux-pm,
kvm, kvm-riscv, linux-kselftest
In-Reply-To: <20240405-ebdb2943657ab08d2d563c03@orel>
[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]
On Fri, Apr 05, 2024 at 05:59:41PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2024 at 10:31:10AM +0000, Conor Dooley wrote:
> > On Fri, Mar 29, 2024 at 05:26:18PM +0800, Max Hsu wrote:
> > > The mcontext/hcontext/scontext CSRs are optional in the Sdtrig extension,
> > > to prevent RW operations to the missing CSRs, which will cause
> > > illegal instructions.
> > >
> > > As a solution, we have proposed the dt format for these CSRs.
> >
> > As I mentioned in your other patch, I amn't sure what the actual value
> > is in being told about "sdtrig" itself if so many of the CSRs are
> > optional. I think we should define pseudo extensions that represent
> > usable subsets that are allowed by riscv,isa-extensions, such as
> > those you describe here: sdtrig + mcontext, sdtrig + scontext and
> > sdtrig + hcontext. Probably also for strig + mscontext. What
> > additional value does having a debug child node give us that makes
> > it worth having over something like the above?
>
> Yeah, Sdtrig, which doesn't tell you what you get, isn't nice at all.
> I wonder if we can start with requiring Sdtrig to be accompanied by
> Ssstrict in order to enable the context CSRs, i.e.
>
> Sdtrig - support without optional CSRs
> Sdtrig+Ssstrict - probe for optional CSRs, support what's found
>
> If there are platforms with Sdtrig and optional CSRs, but not Ssstrict,
> then maybe the optional CSRs can be detected in some vendor-specific way,
> where the decision as to whether or not that vendor-specific way is
> acceptable is handled case-by-case.
I think it's pretty reasonable to make sstrict a requirement for the
kernel's use of sdtrig. If we have some non-sstrict systems that do
implement these particular CSRs, then I guess we can add some psuedo
instructions then (and nothing would stop the sstrict systems also
specifying directly). If they're using some non-standard CSRs then
case-by-case I guess.
I'm just specifically not keen on adding extra dt properties that do
things we can already do with the ones we have!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3 02/18] ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document
From: Krzysztof Kozlowski @ 2024-04-09 15:51 UTC (permalink / raw)
To: Alexandre Mergnat, Liam Girdwood, Mark Brown, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones, Flora Fu, Jaroslav Kysela,
Takashi Iwai, Sumit Semwal, Christian König, Catalin Marinas,
Will Deacon, Rob Herring
Cc: linux-sound, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-media, dri-devel, linaro-mm-sig
In-Reply-To: <20240226-audio-i350-v3-2-16bb2c974c55@baylibre.com>
On 09/04/2024 15:42, Alexandre Mergnat wrote:
> Add soundcard bindings for the MT8365 SoC with the MT6357 audio codec.
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> +patternProperties:
> + "^dai-link-[0-9]+$":
> + type: object
> + description:
> + Container for dai-link level properties and CODEC sub-nodes.
> +
> + properties:
> + codec:
> + type: object
> + description: Holds subnode which indicates codec dai.
> +
> + properties:
> + sound-dai:
> + maxItems: 1
> + description: phandle of the codec DAI
> +
> + additionalProperties: false
> +
> + link-name:
> + description:
> + This property corresponds to the name of the BE dai-link to which
> + we are going to update parameters in this node.
> + items:
> + const: 2ND_I2S_BE
What is the type of link-name? Why is it fixed? How can you have here
multiple dai links if all of them must have the same name?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 05/11] spi: cadence-qspi: add FIFO depth detection quirk
From: Mark Brown @ 2024-04-09 15:51 UTC (permalink / raw)
To: Théo Lebrun
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Vaishnav Achath,
Thomas Bogendoerfer, Rob Herring, linux-spi, devicetree,
linux-kernel, linux-mips, Vladimir Kondratiev, Gregory CLEMENT,
Thomas Petazzoni, Tawfik Bayouk
In-Reply-To: <D0FIC34Z35BV.1RT6NNGWA85SL@bootlin.com>
[-- Attachment #1: Type: text/plain, Size: 274 bytes --]
On Tue, Apr 09, 2024 at 12:07:56PM +0200, Théo Lebrun wrote:
> - (3) Make DT property optional for all compatibles.
> - (3a) If provided, warn if runtime detect value is different.
> - (3b) If provided, do not detect+warn.
I think either of these is fine.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v3 03/18] ASoC: dt-bindings: mt6357: Add audio codec document
From: Krzysztof Kozlowski @ 2024-04-09 15:55 UTC (permalink / raw)
To: Alexandre Mergnat, Liam Girdwood, Mark Brown, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones, Flora Fu, Jaroslav Kysela,
Takashi Iwai, Sumit Semwal, Christian König, Catalin Marinas,
Will Deacon, Rob Herring
Cc: linux-sound, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-media, dri-devel, linaro-mm-sig
In-Reply-To: <20240226-audio-i350-v3-3-16bb2c974c55@baylibre.com>
On 09/04/2024 15:42, Alexandre Mergnat wrote:
> Add MT8365 audio codec bindings to set required
> and optional voltage properties between the codec and the board.
> The properties are:
> - phandle of the requiered power supply.
typo
> - Setup of microphone bias voltage.
> - Setup of the speaker pin pull-down.
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
> .../devicetree/bindings/sound/mt6357.yaml | 54 ++++++++++++++++++++++
Filename using compatible syntax, so missing vendor prefix.
> 1 file changed, 54 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/mt6357.yaml b/Documentation/devicetree/bindings/sound/mt6357.yaml
> new file mode 100644
> index 000000000000..381cb71b959f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/mt6357.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/mt6357.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek MT6357 Codec
> +
> +maintainers:
> + - Alexandre Mergnat <amergnat@baylibre.com>
> +
> +description: |
Do not need '|' unless you need to preserve formatting.
> + This is the required and optional voltage properties for this subdevice.
> + The communication between MT6357 and SoC is through Mediatek PMIC wrapper.
> + For more detail, please visit Mediatek PMIC wrapper documentation.
> + Must be a child node of PMIC wrapper.
Why?
> +
> +properties:
> +
Drop blank line
> + mediatek,hp-pull-down:
> + description:
> + Earphone driver positive output stage short to
> + the audio reference ground.
> + type: boolean
> +
> + mediatek,micbias0-microvolt:
> + description: Selects MIC Bias 0 output voltage.
> + enum: [1700000, 1800000, 1900000, 2000000,
> + 2100000, 2500000, 2600000, 2700000]
> + default: 1700000
> +
> + mediatek,micbias1-microvolt:
> + description: Selects MIC Bias 1 output voltage.
> + enum: [1700000, 1800000, 1900000, 2000000,
> + 2100000, 2500000, 2600000, 2700000]
> + default: 1700000
> +
> + mediatek,vaud28-supply:
> + description: 2.8 volt supply phandle for the audio codec
Supplies go without vendor prefixes.
> +
> +required:
> + - mediatek,vaud28-supply
That's basically no-op schema. I do not understand what you are trying
to achieve here.
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + codec {
> + mediatek,micbias0-microvolt = <1900000>;
> + mediatek,micbias1-microvolt = <1700000>;
> + mediatek,vaud28-supply = <&mt6357_vaud28_reg>;
Sorry, this does not work. Change voltage to 1111111 and check the results.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 04/18] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC
From: Krzysztof Kozlowski @ 2024-04-09 15:56 UTC (permalink / raw)
To: Alexandre Mergnat, Liam Girdwood, Mark Brown, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones, Flora Fu, Jaroslav Kysela,
Takashi Iwai, Sumit Semwal, Christian König, Catalin Marinas,
Will Deacon, Rob Herring
Cc: linux-sound, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-media, dri-devel, linaro-mm-sig
In-Reply-To: <20240226-audio-i350-v3-4-16bb2c974c55@baylibre.com>
On 09/04/2024 15:42, Alexandre Mergnat wrote:
> Add the audio codec sub-device. This sub-device is used to set required
> and optional voltage properties between the codec and the board.
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
> Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> index 37423c2e0fdf..7c6a4a587b5f 100644
> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> @@ -37,6 +37,11 @@ properties:
> "#interrupt-cells":
> const: 2
>
> + codec:
> + type: object
> + $ref: /schemas/sound/mt6357.yaml
> + unevaluatedProperties: false
Just put the properties here, without the codec node.
Also, your example is now incomplete.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 16/18] arm64: dts: mediatek: add mt6357 audio codec support
From: Krzysztof Kozlowski @ 2024-04-09 15:58 UTC (permalink / raw)
To: Alexandre Mergnat, Liam Girdwood, Mark Brown, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones, Flora Fu, Jaroslav Kysela,
Takashi Iwai, Sumit Semwal, Christian König, Catalin Marinas,
Will Deacon, Rob Herring
Cc: linux-sound, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-media, dri-devel, linaro-mm-sig
In-Reply-To: <20240226-audio-i350-v3-16-16bb2c974c55@baylibre.com>
On 09/04/2024 15:42, Alexandre Mergnat wrote:
> Add audio codec support of MT6357 PMIC.
> Update the file header.
Why?
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
> arch/arm64/boot/dts/mediatek/mt6357.dtsi | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt6357.dtsi b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
> index 3330a03c2f74..ade410851524 100644
> --- a/arch/arm64/boot/dts/mediatek/mt6357.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: (GPL-2.0 OR MIT)
> /*
> * Copyright (c) 2020 MediaTek Inc.
> - * Copyright (c) 2023 BayLibre Inc.
> + * Copyright (c) 2024 BayLibre Inc.
That's not a reasonable change. The file was published on 2023, wasn't
it? If this is not correct, please explain why/how and make it separate
patch.
> */
>
> #include <dt-bindings/input/input.h>
> @@ -10,6 +10,9 @@ &pwrap {
> mt6357_pmic: pmic {
> compatible = "mediatek,mt6357";
>
> + mt6357_codec: codec {
> + };
There is no single point of having empty nodes.
NAK.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 18/18] arm64: dts: mediatek: add audio support for mt8365-evk
From: Krzysztof Kozlowski @ 2024-04-09 16:00 UTC (permalink / raw)
To: Alexandre Mergnat, Liam Girdwood, Mark Brown, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones, Flora Fu, Jaroslav Kysela,
Takashi Iwai, Sumit Semwal, Christian König, Catalin Marinas,
Will Deacon, Rob Herring
Cc: linux-sound, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-media, dri-devel, linaro-mm-sig
In-Reply-To: <20240226-audio-i350-v3-18-16bb2c974c55@baylibre.com>
On 09/04/2024 15:42, Alexandre Mergnat wrote:
> Add the sound node which is linked to the MT8365 SoC AFE and
> the MT6357 audio codec.
>
> Update the file header.
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
> arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 98 +++++++++++++++++++++++++++--
> 1 file changed, 94 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> index 50cbaefa1a99..eb0c5f076dd4 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> @@ -1,9 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * Copyright (c) 2021-2022 BayLibre, SAS.
> - * Authors:
> - * Fabien Parent <fparent@baylibre.com>
> - * Bernhard Rosenkränzer <bero@baylibre.com>
> + * Copyright (c) 2024 BayLibre, SAS.
What is happening with your copyrights? Why do you change existing ones?
> + * Authors: Fabien Parent <fparent@baylibre.com>
> + * Bernhard Rosenkränzer <bero@baylibre.com>
> + * Alexandre Mergnat <amergnat@baylibre.com>
> */
>
> /dts-v1/;
> @@ -86,6 +86,29 @@ optee_reserved: optee@43200000 {
> reg = <0 0x43200000 0 0x00c00000>;
> };
> };
> +
> + sound: sound {
> + compatible = "mediatek,mt8365-mt6357";
> + pinctrl-names = "default",
> + "dmic",
> + "miso_off",
> + "miso_on",
> + "mosi_off",
> + "mosi_on";
> + pinctrl-0 = <&aud_default_pins>;
> + pinctrl-1 = <&aud_dmic_pins>;
> + pinctrl-2 = <&aud_miso_off_pins>;
> + pinctrl-3 = <&aud_miso_on_pins>;
> + pinctrl-4 = <&aud_mosi_off_pins>;
> + pinctrl-5 = <&aud_mosi_on_pins>;
> + mediatek,platform = <&afe>;
> + status = "okay";
Where did you disable the node?
> + };
> +};
> +
> +&afe {
> + mediatek,dmic-mode = <1>;
> + status = "okay";
> };
>
> &cpu0 {
> @@ -174,6 +197,12 @@ &mmc1 {
> status = "okay";
> };
>
> +&mt6357_codec {
> + mediatek,micbias0-microvolt = <1900000>;
> + mediatek,micbias1-microvolt = <1700000>;
> + mediatek,vaud28-supply = <&mt6357_vaud28_reg>;
> +};
> +
> &mt6357_pmic {
> interrupts-extended = <&pio 145 IRQ_TYPE_LEVEL_HIGH>;
> interrupt-controller;
> @@ -181,6 +210,67 @@ &mt6357_pmic {
> };
>
> &pio {
> + aud_default_pins: audiodefault-pins {
> + pins {
> + pinmux = <MT8365_PIN_72_CMDAT4__FUNC_I2S3_BCK>,
> + <MT8365_PIN_73_CMDAT5__FUNC_I2S3_LRCK>,
> + <MT8365_PIN_74_CMDAT6__FUNC_I2S3_MCK>,
> + <MT8365_PIN_75_CMDAT7__FUNC_I2S3_DO>;
You have broken indentation everywhere.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/1] arm64: dts: imx8qxp-mek: add cm40_i2c, wm8960/wm8962 and sai[0,1,4,5]
From: Frank Li @ 2024-04-09 16:02 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list
In-Reply-To: <76714850-0e02-4333-acce-02c7657666b0@linaro.org>
On Tue, Apr 09, 2024 at 08:33:22AM +0200, Krzysztof Kozlowski wrote:
> On 08/04/2024 17:36, Frank Li wrote:
> > On Fri, Apr 05, 2024 at 08:21:18PM +0200, Krzysztof Kozlowski wrote:
> >> On 05/04/2024 16:46, Frank Li wrote:
> >>> On Fri, Apr 05, 2024 at 08:41:59AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 04/04/2024 18:19, Frank Li wrote:
> >>>>> imx8qxp-mek use two kind audio codec, wm8960 and wm8962. Using dummy gpio
> >>>>> i2c bus mux to connect both i2c devices. One will probe failure and other
> >>>>> will probe success when devices driver check whoami. So one dtb can cover
> >>>>> both board configuration.
> >>>>
> >>>> I don't understand it. Either you add real device or not. If one board
> >>>> has two devices, then why do you need to check for failures?
> >>>>
> >>>> Anyway, don't add fake stuff to DTS.
> >>>
> >>> NAK can't resolve the problem. It should be common problem for long time
> >>> cycle boards. Some chipes will be out life cycle. such as some sensor. So
> >>> chips on boards have been replace by some pin to pin compatible sensor. For
> >>> example:
> >>> old boards: use sensor A with address 0x1a
> >>> new bench: use sensor B with address 0x1b.
> >>>
> >>> You can treat it as two kind boards, RevA or RevB. But most user want to
> >>> use one dtb to handle such small differences. For this case, it should be
> >>> simple. Just add a super set.
> >>> i2c
> >>> {
> >>> sensorA@1a
> >>> {
> >>> }
> >>> sensorB@1b
> >>> {
> >>> }
> >>> }
> >>>
> >>> It also depend on whoami check by i2c devices. Only A or B will probe.
> >>>
> >>> wm8960 and wm8962 are more complex example. wm8960 is out of life. But
> >>> wm8962 and wm8960 have the same i2c address. The current i2c frame can't
> >>> allow the same i2c address in one i2c bus.
> >>>
> >>> You are feel to NAK my method, but I hope you also provide constructive
> >>> solution to help resolve the problem.
> >>
> >> Yes, we resolved it long time ago. Your bootloader can (usually easily)
> >> detect revision of the board and load appropriate DTS or DTS+DTSO.
> >
> > I knewn it. But the problem is one development boards A have many options,
> > so create many child dts for files, A1, A2, ... An which base on A
>
> So use DTSO, what's the problem? Other vendors, liek Rpi does not have
> problem with it and it works well. No confusion.
>
> >
> > If there are difference happen at A, create new B. then create all child
> > dtb, B1, B2, ... Bn. DTB number will increase exponent.
> >
> > If change is quite bit, we have to do that. But if change is quite small,
> > One dtb can cover it by driver auto detect, which will work like some
> > adaptor card have not plug into boards, or some sensor or NOR-flash have
> > not installed because reduce cost.
>
> You have two boards, not 20 here!
Actually, it is around ~20 derived boards. It is not upstream just because
we have not time to do that yet. After some clean up, I estimate about 7 -
- 10.
>
> >
> > Although boot loader can update dts or choose difference dts, It also cause
> > many confusition, such as layerscape, uboot update many kernel dtb's
> > information, which actually increase dependence between uboot and kernel.
> > Also it confuse people, for example, when try to debug kernel dtb, why
> > change have not token affect when change dts because not realized uboot
> > over write it.
> >
> > What's I dide is that trying to reduce unnecessary dts.
>
> There is no confusion. That's normal process, so if someone is confused
> by having variants of board, this someone will be even more confused by
> seeing non-existing hardware in his DTS.
How about existed dummy_clk and dummy regulator in dts?
>
> This problem was solved long time ago and you do not bring any
> reasonable new arguments. All vendors solved it (just look at ongoing
> discussions on board id) but only you have problem with their solution.
I never said solution was not work. Just not friend for user enough for
this case. It likes USB TypeC vs USB A port. Both works, just TypeC can't
care direction.
>
> NAK
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH v2 2/2] iio: adc: Add support for AD4000
From: Marcelo Schmitt @ 2024-04-09 16:09 UTC (permalink / raw)
To: David Lechner
Cc: Marcelo Schmitt, lars, Michael.Hennerich, jic23, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-iio, devicetree,
linux-kernel
In-Reply-To: <CAMknhBEMDg3YF5pvoKJ-6y0Y5OJpmBthWfogCjy90B=F84SvzA@mail.gmail.com>
On 04/08, David Lechner wrote:
> On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
> >
> > Add support for AD4000 family of low noise, low power, high speed,
> > successive aproximation register (SAR) ADCs.
> >
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ad4000.c | 649 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 663 insertions(+)
> > create mode 100644 drivers/iio/adc/ad4000.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5dfe118a5dd3..86aa96115f5a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1165,6 +1165,7 @@ L: linux-iio@vger.kernel.org
> > S: Supported
> > W: https://ez.analog.com/linux-software-drivers
> > F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > +F: drivers/iio/adc/ad4000.c
> >
> > ANALOG DEVICES INC AD4130 DRIVER
> > M: Cosmin Tanislav <cosmin.tanislav@analog.com>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 8db68b80b391..9c9d13d4b74f 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -21,6 +21,18 @@ config AD_SIGMA_DELTA
> > select IIO_BUFFER
> > select IIO_TRIGGERED_BUFFER
> >
> > +config AD4000
> > + tristate "Analog Devices AD4000 ADC Driver"
> > + depends on SPI
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + help
> > + Say yes here to build support for Analog Devices AD4000 high speed
> > + SPI analog to digital converters (ADC).
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called ad4000.
> > +
> > config AD4130
> > tristate "Analog Device AD4130 ADC Driver"
> > depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index edb32ce2af02..aa52068d864b 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -6,6 +6,7 @@
> > # When adding new entries keep the list in alphabetical order
> > obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
> > obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> > +obj-$(CONFIG_AD4000) += ad4000.o
> > obj-$(CONFIG_AD4130) += ad4130.o
> > obj-$(CONFIG_AD7091R) += ad7091r-base.o
> > obj-$(CONFIG_AD7091R5) += ad7091r5.o
> > diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> > new file mode 100644
> > index 000000000000..7997d9d98743
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4000.c
> > @@ -0,0 +1,649 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * AD4000 SPI ADC driver
> > + *
> > + * Copyright 2024 Analog Devices Inc.
> > + */
> > +#include <asm/unaligned.h>
> > +#include <linux/bits.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/math.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/units.h>
> > +#include <linux/util_macros.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +
> > +#define AD400X_READ_COMMAND 0x54
> > +#define AD400X_WRITE_COMMAND 0x14
> > +
> > +/* AD4000 Configuration Register programmable bits */
> > +#define AD4000_STATUS BIT(4) /* Status bits output */
> > +#define AD4000_SPAN_COMP BIT(3) /* Input span compression */
> > +#define AD4000_HIGHZ BIT(2) /* High impedance mode */
> > +#define AD4000_TURBO BIT(1) /* Turbo mode */
>
> Usually bits of the same register share a similar prefix, e.g.
> AD4000_CFG_TURBO, AD4000_CFG_HIGHZ, etc.
This only has one register, but if this makes things look better will do it.
>
> > +
> > +#define AD4000_TQUIET2_NS 60
> > +
> > +#define AD4000_18BIT_MSK GENMASK(31, 14)
> > +#define AD4000_20BIT_MSK GENMASK(31, 12)
> > +
> > +#define AD4000_DIFF_CHANNEL(_sign, _real_bits) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .differential = 1, \
> > + .channel = 0, \
> > + .channel2 = 1, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE), \
> > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
> > + .scan_type = { \
> > + .sign = _sign, \
> > + .realbits = _real_bits, \
> > + .storagebits = _real_bits > 16 ? 32 : 16, \
> > + .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
> > + .endianness = IIO_BE, \
> > + }, \
> > + } \
> > +
> > +#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = 0, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE) | \
> > + BIT(IIO_CHAN_INFO_OFFSET), \
> > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
> > + .scan_type = { \
> > + .sign = _sign, \
> > + .realbits = _real_bits, \
> > + .storagebits = _real_bits > 16 ? 32 : 16, \
> > + .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
> > + .endianness = IIO_BE, \
> > + }, \
> > + } \
>
> It looks like all differential chips are signed and all
> pseduo-differential chips are unsigned, so I don't think we need the
> _sign parameter in these macros.
That's correct, the _sign param can be removed after the split of channel macros.
Will do it for v3.
>
> I also still have doubts about using IIO_BE and 8-bit xfers when it
> comes to adding support later to achieve max sample rate with a SPI
> offload. For example to get 2MSPS with an 18-bit chip, it will require
> an approx 33% faster SPI clock than the actual slowest clock possible
> because it will have to read 6 extra bits per sample. I didn't check
> the specs, but this may not even be physically possible without
> exceeding the datasheet max SPI clock rate. Also errors could be
> reduced if we could actually use the slowest allowable SPI clock rate.
> Furthermore, the offload hardware would have to be capable of adding
> an extra byte per sample for 18 and 20-bit chips when piping the data
> to DMA in order to get the 32-bit alignment in the buffer required by
> IIO scan_type and the natural alignment requirements of IIO buffers in
> general.
Maybe I should already implement support for reading with SPI offload
rather than doing it after this set is merged?
So we can test what happens at faster sample rates before we commit to a solution.
>
> > +
> > +enum ad4000_ids {
> > + ID_AD4000,
> > + ID_AD4001,
> > + ID_AD4002,
> > + ID_AD4003,
> > + ID_AD4004,
> > + ID_AD4005,
> > + ID_AD4006,
> > + ID_AD4007,
> > + ID_AD4008,
> > + ID_AD4010,
> > + ID_AD4011,
> > + ID_AD4020,
> > + ID_AD4021,
> > + ID_AD4022,
> > + ID_ADAQ4001,
> > + ID_ADAQ4003,
> > +};
> > +
> > +struct ad4000_chip_info {
> > + const char *dev_name;
> > + struct iio_chan_spec chan_spec;
> > +};
> > +
> > +static const struct ad4000_chip_info ad4000_chips[] = {
> > + [ID_AD4000] = {
> > + .dev_name = "ad4000",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> > + },
> > + [ID_AD4001] = {
> > + .dev_name = "ad4001",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> > + },
> > + [ID_AD4002] = {
> > + .dev_name = "ad4002",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> > + },
> > + [ID_AD4003] = {
> > + .dev_name = "ad4003",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> > + },
> > + [ID_AD4004] = {
> > + .dev_name = "ad4004",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> > + },
> > + [ID_AD4005] = {
> > + .dev_name = "ad4005",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> > + },
> > + [ID_AD4006] = {
> > + .dev_name = "ad4006",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> > + },
> > + [ID_AD4007] = {
> > + .dev_name = "ad4007",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> > + },
> > + [ID_AD4008] = {
> > + .dev_name = "ad4008",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> > + },
> > + [ID_AD4010] = {
> > + .dev_name = "ad4010",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> > + },
> > + [ID_AD4011] = {
> > + .dev_name = "ad4011",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> > + },
> > + [ID_AD4020] = {
> > + .dev_name = "ad4020",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> > + },
> > + [ID_AD4021] = {
> > + .dev_name = "ad4021",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> > + },
> > + [ID_AD4022] = {
> > + .dev_name = "ad4022",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> > + },
> > + [ID_ADAQ4001] = {
> > + .dev_name = "adaq4001",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> > + },
> > + [ID_ADAQ4003] = {
> > + .dev_name = "adaq4003",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> > + },
> > +};
> > +
> > +enum ad4000_gains {
> > + AD4000_0454_GAIN = 0,
> > + AD4000_0909_GAIN = 1,
> > + AD4000_1_GAIN = 2,
>
> AD4000_1000_GAIN would be more consistent with the others.
Ack
>
> > + AD4000_1900_GAIN = 3,
> > + AD4000_GAIN_LEN
> > +};
> > +
> > +/*
> > + * Gains stored and computed as fractions to avoid introducing rounding errors.
> > + */
> > +static const int ad4000_gains_frac[AD4000_GAIN_LEN][2] = {
> > + [AD4000_0454_GAIN] = { 227, 500 },
> > + [AD4000_0909_GAIN] = { 909, 1000 },
> > + [AD4000_1_GAIN] = { 1, 1 },
> > + [AD4000_1900_GAIN] = { 19, 10 },
> > +};
>
> Why not just store the numerator in milli units and always use 1000
> for the denominator? It seems like it would simplify the code and make
> it easier to read and understand. Also, these values are coming from
> the adi,gain-milli property already, so we could avoid the enum and
> the lookup table entirely and simplify things even more.
Makes sense. Will do it.
>
> > +
> > +struct ad4000_state {
> > + struct spi_device *spi;
> > + struct gpio_desc *cnv_gpio;
> > + int vref;
> > + bool status_bits;
> > + bool span_comp;
> > + bool turbo_mode;
> > + bool high_z_mode;
> > +
> > + enum ad4000_gains pin_gain;
> > + int scale_tbl[AD4000_GAIN_LEN][2][2];
> > +
> > + /*
> > + * DMA (thus cache coherency maintenance) requires the
> > + * transfer buffers to live in their own cache lines.
> > + */
> > + struct {
> > + union {
> > + u16 sample_buf16;
> > + u32 sample_buf32;
>
> Technically, these are holding big-endian data, so __be16 and __be32
> would be more correct.
Ack
>
> > + } data;
> > + s64 timestamp __aligned(8);
> > + } scan;
> > + __be16 tx_buf __aligned(IIO_DMA_MINALIGN);
> > + __be16 rx_buf;
> > +};
>
> scan.data is used as SPI rx_buf so __aligned(IIO_DMA_MINALIGN); needs
> to be moved to the scan field.
I have already tried it. Maybe I did something wrong besides buffer alignment
at that time. Will give it another try.
>
> > +
> > +static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
> > + const struct ad4000_chip_info *chip)
> > +{
> > + int diff = chip->chan_spec.differential;
> > + int val, val2, tmp0, tmp1, i;
> > + u64 tmp2;
> > +
> > + val2 = scale_bits;
> > + for (i = 0; i < AD4000_GAIN_LEN; i++) {
>
> Only one gain is selected by the devicetree, so why do we need to do
> this for all 4 gains?
>
Good point. Will think better how to simplify this.
> > + val = st->vref / 1000;
> > + /* Multiply by MILLI here to avoid losing precision */
> > + val = mult_frac(val, ad4000_gains_frac[i][1] * MILLI,
> > + ad4000_gains_frac[i][0]);
> > + /* Would multiply by NANO here but we already multiplied by MILLI */
> > + tmp2 = shift_right((u64)val * MICRO, val2);
> > + tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);
> > + /* Store scale for when span compression is disabled */
> > + st->scale_tbl[i][0][0] = tmp0; /* Integer part */
> > + st->scale_tbl[i][0][1] = abs(tmp1); /* Fractional part */
> > + /* Store scale for when span compression is enabled */
> > + st->scale_tbl[i][1][0] = tmp0;
> > + if (diff)
> > + st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
> > + else
> > + st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
> > + }
> > +}
> > +
> > +static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
> > +{
> > + put_unaligned_be16(AD400X_WRITE_COMMAND << BITS_PER_BYTE | val,
> > + &st->tx_buf);
> > + return spi_write(st->spi, &st->tx_buf, 2);
> > +}
> > +
> > +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> > +{
> > + struct spi_transfer t[] = {
> > + {
> > + .tx_buf = &st->tx_buf,
> > + .rx_buf = &st->rx_buf,
> > + .len = 2,
> > + },
> > + };
> > + int ret;
> > +
> > + put_unaligned_be16(AD400X_READ_COMMAND << BITS_PER_BYTE, &st->tx_buf);
> > + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = get_unaligned_be16(&st->rx_buf);
> > +
> > + return ret;
> > +}
> > +
>
> It would be very helpful to have comments here explaining the exact
> expected wiring configuration and signal timing here since there are
> so many possibilities for this chip.
>
Ok, will be spliting this into different handling for the wiring modes so
will add comments to make clear what supports each configuration.
> > +static int ad4000_read_sample(struct ad4000_state *st,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct spi_transfer t[] = {
>
> Don't really need [] here since there is only one xfer.
>
Ack
> > + {
> > + .rx_buf = &st->scan.data,
> > + .len = BITS_TO_BYTES(chan->scan_type.storagebits),
> > + .delay = {
> > + .value = AD4000_TQUIET2_NS,
> > + .unit = SPI_DELAY_UNIT_NSECS,
> > + },
> > + },
> > + };
> > + int ret;
> > +
> > + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int ad4000_single_conversion(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan, int *val)
> > +{
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > + u32 sample;
> > + int ret;
> > +
> > + if (st->cnv_gpio)
> > + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_HIGH);
>
> It would make more sense and be less redundant to move the gpio code
> into ad4000_read_sample().
>
> Also, gpiod_set_value_cansleep() checks for NULL, so the if () is redundant.
>
Good point. I think the execution flow migth change a bit here but will try
to avoid things like that.
> > +
> > + ret = ad4000_read_sample(st, chan);
> > + if (ret)
> > + return ret;
> > +
> > + if (st->cnv_gpio)
> > + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_LOW);
> > +
> > + if (chan->scan_type.storagebits > 16)
> > + sample = get_unaligned_be32(&st->scan.data);
> > + else
> > + sample = get_unaligned_be16(&st->scan.data);
>
> data is aligned, so be32/16_to_cpu() should be fine. Also, Jonathan
> will suggest to use &st->scan.data.sample_b32/16 here too. :-)
Ack
>
> > +
> > + switch (chan->scan_type.realbits) {
> > + case 16:
> > + break;
> > + case 18:
> > + sample = FIELD_GET(AD4000_18BIT_MSK, sample);
> > + break;
> > + case 20:
> > + sample = FIELD_GET(AD4000_20BIT_MSK, sample);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (chan->scan_type.sign == 's')
> > + *val = sign_extend32(sample, chan->scan_type.realbits - 1);
> > +
> > + return IIO_VAL_INT;
> > +}
> > +
> > +static int ad4000_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int *val,
> > + int *val2, long info)
> > +{
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_RAW:
> > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> > + return ad4000_single_conversion(indio_dev, chan, val);
> > + unreachable();
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = st->scale_tbl[st->pin_gain][st->span_comp][0];
> > + *val2 = st->scale_tbl[st->pin_gain][st->span_comp][1];
> > + return IIO_VAL_INT_PLUS_NANO;
> > + case IIO_CHAN_INFO_OFFSET:
> > + *val = 0;
> > + if (st->span_comp)
> > + *val = mult_frac(st->vref / 1000, 1, 10);
> > +
> > + return IIO_VAL_INT;
> > + default:
> > + break;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int ad4000_read_avail(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + const int **vals, int *type, int *length,
> > + long info)
> > +{
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_SCALE:
> > + *vals = (int *)st->scale_tbl[st->pin_gain];
> > + *length = 2 * 2;
> > + *type = IIO_VAL_INT_PLUS_NANO;
> > + return IIO_AVAIL_LIST;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, long mask)
> > +{
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + return IIO_VAL_INT_PLUS_NANO;
> > + default:
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + }
> > + return -EINVAL;
>
> not reachable because of default, so can be left out
>
Ack
> > +}
> > +
> > +static int ad4000_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int val, int val2,
> > + long mask)
> > +{
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > + unsigned int reg_val;
> > + bool span_comp_en;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > + ret = ad4000_read_reg(st, ®_val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + span_comp_en = (val2 == st->scale_tbl[st->pin_gain][1][1]);
> > + reg_val &= ~AD4000_SPAN_COMP;
> > + reg_val |= FIELD_PREP(AD4000_SPAN_COMP, span_comp_en);
> > +
> > + ret = ad4000_write_reg(st, reg_val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + st->span_comp = span_comp_en;
> > + return 0;
> > + }
> > + unreachable();
>
> Can bring out the return 0 to avoid unreachable.
Ack
>
> > + default:
> > + break;
>
> Can return -EINVAL to avoid break;
>
Ack
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static irqreturn_t ad4000_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + if (st->cnv_gpio)
> > + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_HIGH);
> > +
> > + ret = ad4000_read_sample(st, &indio_dev->channels[0]);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + if (st->cnv_gpio)
> > + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_LOW);
> > +
> > + iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
> > + iio_get_time_ns(indio_dev));
> > +
> > +err_out:
> > + iio_trigger_notify_done(indio_dev->trig);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static const struct iio_info ad4000_info = {
> > + .read_raw = &ad4000_read_raw,
> > + .read_avail = &ad4000_read_avail,
> > + .write_raw = &ad4000_write_raw,
> > + .write_raw_get_fmt = &ad4000_write_raw_get_fmt,
> > +};
> > +
> > +static void ad4000_config(struct ad4000_state *st)
> > +{
> > + unsigned int reg_val;
> > + int ret;
> > +
> > + reg_val = FIELD_PREP(AD4000_TURBO, 1);
>
> Since the driver in it's current state can get anywhere near the max
> sample rate of ~1MSPS, I don't think it makes sense to enable turbo at
> this point.
>
This is just enabling turbo at start up. If not enabling turbo during probe,
we would want(need?) to provide some interface for that, which might not be
much desired.
> > +
> > + if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> > + reg_val |= FIELD_PREP(AD4000_HIGHZ, 1);
> > +
> > + /*
> > + * The ADC SDI pin might be connected to controller CS line in which
> > + * case the write might fail. This, however, does not prevent the device
> > + * from functioning even though in a configuration other than the
> > + * requested one.
> > + */
> > + ret = ad4000_write_reg(st, reg_val);
> > + if (ret < 0)
> > + dev_dbg(&st->spi->dev, "Failed to config device\n");
>
> If writing fails because there is no CS line wired up, we won't get an
> error returned here. The SPI controller has no way of knowing this
> happened, so it can only assume the write was successful and return 0.
> So this should return ret.
>
Ok, ack.
> Ideally, the devicetree should tell us if CS is wired up or not.
>
> > +}
> > +
> > +static void ad4000_regulator_disable(void *reg)
> > +{
> > + regulator_disable(reg);
> > +}
> > +
> > +static int ad4000_probe(struct spi_device *spi)
> > +{
> > + const struct ad4000_chip_info *chip;
> > + struct regulator *vref_reg;
> > + struct iio_dev *indio_dev;
> > + struct ad4000_state *st;
> > + int ret;
>
> We need a check somewhere in here to make sure that adi,spi-mode is in
> a supported configuration. E.g. chain mode is not currently
> implemented.
ok, will add that.
>
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + chip = spi_get_device_match_data(spi);
> > + if (!chip)
> > + return -EINVAL;
> > +
> > + st = iio_priv(indio_dev);
> > + st->spi = spi;
> > +
> > + ret = devm_regulator_get_enable(&spi->dev, "vdd");
> > + if (ret)
> > + return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
> > +
> > + ret = devm_regulator_get_enable(&spi->dev, "vio");
> > + if (ret)
> > + return dev_err_probe(&spi->dev, ret, "Failed to enable VIO supply\n");
> > +
> > + vref_reg = devm_regulator_get(&spi->dev, "ref");
> > + if (IS_ERR(vref_reg))
> > + return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
> > + "Failed to get vref regulator\n");
> > +
> > + ret = regulator_enable(vref_reg);
> > + if (ret < 0)
> > + return dev_err_probe(&spi->dev, ret,
> > + "Failed to enable voltage regulator\n");
> > +
> > + ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable, vref_reg);
> > + if (ret)
> > + return dev_err_probe(&spi->dev, ret,
> > + "Failed to add regulator disable action\n");
> > +
> > + st->vref = regulator_get_voltage(vref_reg);
> > + if (st->vref < 0)
> > + return dev_err_probe(&spi->dev, st->vref, "Failed to get vref\n");
> > +
> > + st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv", GPIOD_OUT_HIGH);
> > + if (IS_ERR(st->cnv_gpio)) {
> > + if (PTR_ERR(st->cnv_gpio) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
>
> EPROBE_DEFER check is not needed with dev_err_probe();, it already does that.
Ack
>
>
> > +
> > + return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
> > + "Failed to get CNV GPIO");
> > + }
> > +
> > + ad4000_config(st);
> > +
> > + indio_dev->name = chip->dev_name;
> > + indio_dev->info = &ad4000_info;
> > + indio_dev->channels = &chip->chan_spec;
> > + indio_dev->num_channels = 1;
> > +
> > + st->pin_gain = AD4000_1_GAIN;
> > + if (device_property_present(&spi->dev, "adi,gain-milli")) {
> > + u32 val;
>
> Should it be an error if adi,gain-milli is set on non-adaq chips?
Maybe. We should not change the scale if it's a chip that don't have the
amplifier in front of the ADC. I think the best handling would be to just
ignore adi,gain-milli if it's not an ADAQ device. Maybe better add a DT
constraint,
- if:
properties:
compatible:
contains:
enum:
- adi,adaq4001
- adi,adaq4003
then:
properties:
adi,gain-milli: false
?
>
> > +
> > + ret = device_property_read_u32(&spi->dev, "adi,gain-milli", &val);
> > + if (ret)
> > + return ret;
> > +
> > + switch (val) {
> > + case 454:
> > + st->pin_gain = AD4000_0454_GAIN;
> > + break;
> > + case 909:
> > + st->pin_gain = AD4000_0909_GAIN;
> > + break;
> > + case 1000:
> > + st->pin_gain = AD4000_1_GAIN;
> > + break;
> > + case 1900:
> > + st->pin_gain = AD4000_1900_GAIN;
> > + break;
> > + default:
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "Invalid firmware provided gain\n");
>
> Could help debugging if val is included in the error message.
Ack
>
>
> > + }
> > + }
> > +
> > + /*
> > + * ADCs that output twos complement code have one less bit to express
> > + * voltage magnitude.
> > + */
> > + if (chip->chan_spec.scan_type.sign == 's')
> > + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits - 1,
> > + chip);
> > + else
> > + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits,
> > + chip);
> > +
> > + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> > + &iio_pollfunc_store_time,
> > + &ad4000_trigger_handler, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + return devm_iio_device_register(&spi->dev, indio_dev);
> > +}
> > +
> > +static const struct spi_device_id ad4000_id[] = {
> > + { "ad4000", (kernel_ulong_t)&ad4000_chips[ID_AD4000] },
> > + { "ad4001", (kernel_ulong_t)&ad4000_chips[ID_AD4001] },
> > + { "ad4002", (kernel_ulong_t)&ad4000_chips[ID_AD4002] },
> > + { "ad4003", (kernel_ulong_t)&ad4000_chips[ID_AD4003] },
> > + { "ad4004", (kernel_ulong_t)&ad4000_chips[ID_AD4004] },
> > + { "ad4005", (kernel_ulong_t)&ad4000_chips[ID_AD4005] },
> > + { "ad4006", (kernel_ulong_t)&ad4000_chips[ID_AD4006] },
> > + { "ad4007", (kernel_ulong_t)&ad4000_chips[ID_AD4007] },
> > + { "ad4008", (kernel_ulong_t)&ad4000_chips[ID_AD4008] },
> > + { "ad4010", (kernel_ulong_t)&ad4000_chips[ID_AD4010] },
> > + { "ad4011", (kernel_ulong_t)&ad4000_chips[ID_AD4011] },
> > + { "ad4020", (kernel_ulong_t)&ad4000_chips[ID_AD4020] },
> > + { "ad4021", (kernel_ulong_t)&ad4000_chips[ID_AD4021] },
> > + { "ad4022", (kernel_ulong_t)&ad4000_chips[ID_AD4022] },
> > + { "adaq4001", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4001] },
> > + { "adaq4003", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4003] },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(spi, ad4000_id);
> > +
> > +static const struct of_device_id ad4000_of_match[] = {
> > + { .compatible = "adi,ad4000", .data = &ad4000_chips[ID_AD4000] },
> > + { .compatible = "adi,ad4001", .data = &ad4000_chips[ID_AD4001] },
> > + { .compatible = "adi,ad4002", .data = &ad4000_chips[ID_AD4002] },
> > + { .compatible = "adi,ad4003", .data = &ad4000_chips[ID_AD4003] },
> > + { .compatible = "adi,ad4004", .data = &ad4000_chips[ID_AD4004] },
> > + { .compatible = "adi,ad4005", .data = &ad4000_chips[ID_AD4005] },
> > + { .compatible = "adi,ad4006", .data = &ad4000_chips[ID_AD4006] },
> > + { .compatible = "adi,ad4007", .data = &ad4000_chips[ID_AD4007] },
> > + { .compatible = "adi,ad4008", .data = &ad4000_chips[ID_AD4008] },
> > + { .compatible = "adi,ad4010", .data = &ad4000_chips[ID_AD4010] },
> > + { .compatible = "adi,ad4011", .data = &ad4000_chips[ID_AD4011] },
> > + { .compatible = "adi,ad4020", .data = &ad4000_chips[ID_AD4020] },
> > + { .compatible = "adi,ad4021", .data = &ad4000_chips[ID_AD4021] },
> > + { .compatible = "adi,ad4022", .data = &ad4000_chips[ID_AD4022] },
> > + { .compatible = "adi,adaq4001", .data = &ad4000_chips[ID_ADAQ4001] },
> > + { .compatible = "adi,adaq4003", .data = &ad4000_chips[ID_ADAQ4003] },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ad4000_of_match);
> > +
> > +static struct spi_driver ad4000_driver = {
> > + .driver = {
> > + .name = "ad4000",
> > + .of_match_table = ad4000_of_match,
> > + },
> > + .probe = ad4000_probe,
> > + .id_table = ad4000_id,
> > +};
> > +module_spi_driver(ad4000_driver);
> > +
> > +MODULE_AUTHOR("Mircea Caprioru <mircea.caprioru@analog.com>");
> > +MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@analog.com>");
> > +MODULE_DESCRIPTION("Analog Devices AD4000 ADC driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.43.0
> >
> >
^ permalink raw reply
* Re: [PATCH v7 2/3] dt-bindings: pinctrl: Document nuvoton ma35d1 pin control
From: Rob Herring @ 2024-04-09 16:29 UTC (permalink / raw)
To: Jacky Huang
Cc: linus.walleij, krzysztof.kozlowski+dt, conor+dt, p.zabel,
j.neuschaefer, linux-arm-kernel, linux-gpio, devicetree,
linux-kernel, ychuang3, schung, Krzysztof Kozlowski
In-Reply-To: <20240409095637.2135-3-ychuang570808@gmail.com>
On Tue, Apr 09, 2024 at 09:56:36AM +0000, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
>
> Add documentation to describe nuvoton ma35d1 pin control and GPIO.
>
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> .../pinctrl/nuvoton,ma35d1-pinctrl.yaml | 163 ++++++++++++++++++
> 1 file changed, 163 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml
> new file mode 100644
> index 000000000000..8b9ec263213f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml
> @@ -0,0 +1,163 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nuvoton,ma35d1-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton MA35D1 pin control and GPIO
> +
> +maintainers:
> + - Shan-Chun Hung <schung@nuvoton.com>
> + - Jacky Huang <ychuang3@nuvoton.com>
> +
> +allOf:
> + - $ref: pinctrl.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - nuvoton,ma35d1-pinctrl
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 1
> +
> + nuvoton,sys:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description:
> + phandle of the system-management node.
If these are the *only* registers to access the pinctrl functions, then
this binding should be a child node of the system-management node and
then you don't need this property.
And if the registers for pinctrl are a defined range, you should add a
'reg' property (even though Linux and regmap don't use it).
> +
> + ranges: true
This property makes no sense with the binding as-is. You don't have
any address to translate. Maybe with the above changes it will.
> +
> +patternProperties:
> + "^gpio@[0-9a-f]+$":
> + type: object
> + additionalProperties: false
> + properties:
> + gpio-controller: true
> +
> + '#gpio-cells':
> + const: 2
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> + interrupt-controller: true
> +
> + '#interrupt-cells':
> + const: 2
> +
> + interrupts:
> + description:
> + The interrupt outputs to sysirq.
> + maxItems: 1
> +
> + required:
> + - gpio-controller
> + - '#gpio-cells'
> + - reg
> + - clocks
> + - interrupt-controller
> + - '#interrupt-cells'
> + - interrupts
> +
> + "^pin-[a-z0-9]+$":
> + type: object
> + description:
> + A pinctrl node should contain at least one subnodes representing the
> + pinctrl groups available on the machine. Each subnode will list the
> + pins it needs, and how they should be configured, with regard to muxer
> + configuration, pullups, drive strength, input enable/disable and input
> + schmitt.
> +
> + $ref: pincfg-node.yaml#
> +
> + properties:
> + power-source:
> + description: |
> + Valid arguments are described as below:
> + 0: power supply of 1.8V
> + 1: power supply of 3.3V
> + enum: [0, 1]
> +
> + drive-strength-microamp:
> + oneOf:
> + - enum: [ 2900, 4400, 5800, 7300, 8600, 10100, 11500, 13000 ]
> + description: 1.8V I/O driving strength
> + - enum: [ 17100, 25600, 34100, 42800, 48000, 56000, 77000, 82000 ]
> + description: 3.3V I/O driving strength
> +
> + unevaluatedProperties: false
In the indented cases, it's preferred to put this before 'properties'.
> +
> + "-grp$":
> + type: object
> + description:
> + Pinctrl node's client devices use subnodes for desired pin configuration.
> + Client device subnodes use below standard properties.
Missing $ref to common properties and 'unevaluatedProperties'.
> + properties:
> + nuvoton,pins:
> + description:
> + Each entry consists of 4 parameters and represents the mux and config
> + setting for one pin.
> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> + minItems: 1
> + items:
> + items:
> + - minimum: 0
> + maximum: 13
> + description:
> + Pin bank.
> + - minimum: 0
> + maximum: 15
> + description:
> + Pin bank index.
> + - minimum: 0
> + maximum: 15
> + description:
> + Mux 0 means GPIO and mux 1 to 15 means the specific device function.
> +
> +required:
> + - compatible
> + - nuvoton,sys
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
> +
> + pinctrl@40040000 {
> + compatible = "nuvoton,ma35d1-pinctrl";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + nuvoton,sys = <&sys>;
> + ranges = <0 0x40040000 0xc00>;
> +
> + gpio@0 {
> + reg = <0x0 0x40>;
> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clk GPA_GATE>;
> + gpio-controller;
> + #gpio-cells = <2>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> +
> + uart-grp {
> + uart11-pins {
This is not what the schema says.
> + nuvoton,pins = <11 0 2>,
> + <11 1 2>,
> + <11 2 2>,
> + <11 3 2>;
> + bias-disable;
> + power-source = <1>;
> + };
> + };
Include a pin-* node in the example.
> + };
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH v6 4/4] drivers: watchdog: ast2500 and ast2600 support bootstatus
From: PeterYin @ 2024-04-09 16:28 UTC (permalink / raw)
To: patrick, Wim Van Sebroeck, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Joel Stanley, Andrew Jeffery,
linux-watchdog, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel
In-Reply-To: <20240328022231.3649741-5-peteryin.openbmc@gmail.com>
Peter Yin 於 3/28/24 10:22 寫道:
> Add WDIOF_EXTERN1 and WDIOF_CARDRESET bootstatus in ast2600
>
> Regarding the AST2600 specification, the WDTn Timeout Status Register
> (WDT10) has bit 1 reserved. Bit 1 of the status register indicates
> on ast2500 if the boot was from the second boot source.
> It does not indicate that the most recent reset was triggered by
> the watchdog. The code should just be changed to set WDIOF_CARDRESET
> if bit 0 of the status register is set.
>
> Include SCU register to veriy WDIOF_EXTERN1 in ast2600 SCU74 or
> ast2500 SCU3C when bit1 is set.
>
> Signed-off-by: Peter Yin <peteryin.openbmc@gmail.com>
> ---
> drivers/watchdog/aspeed_wdt.c | 35 +++++++++++++++++++++++++++++++----
> 1 file changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index b4773a6aaf8c..0e7ef860cbdc 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -11,10 +11,12 @@
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/kstrtox.h>
> +#include <linux/mfd/syscon.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_irq.h>
> #include <linux/platform_device.h>
> +#include <linux/regmap.h>
> #include <linux/watchdog.h>
>
> static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -77,11 +79,19 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
> #define WDT_TIMEOUT_STATUS 0x10
> #define WDT_TIMEOUT_STATUS_IRQ BIT(2)
> #define WDT_TIMEOUT_STATUS_BOOT_SECONDARY BIT(1)
> +#define WDT_TIMEOUT_STATUS_EVENT BIT(0)
> #define WDT_CLEAR_TIMEOUT_STATUS 0x14
> #define WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION BIT(0)
> #define WDT_RESET_MASK1 0x1c
> #define WDT_RESET_MASK2 0x20
>
> +/*
> + * Ast2600 SCU74 bit1 is External reset flag
> + * Ast2500 SCU3C bit1 is External reset flag
> + */
> +#define AST2500_SYSTEM_RESET_EVENT 0x3C
> +#define AST2600_SYSTEM_RESET_EVENT 0x74
> +#define EXTERN_RESET_FLAG BIT(1)
> /*
> * WDT_RESET_WIDTH controls the characteristics of the external pulse (if
> * enabled), specifically:
> @@ -330,6 +340,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> if (IS_ERR(wdt->base))
> return PTR_ERR(wdt->base);
>
> + struct regmap *scu_base = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "aspeed,scu");
> + if (IS_ERR(scu_base))
> + return PTR_ERR(scu_base);
> +
> wdt->wdd.info = &aspeed_wdt_info;
>
> if (wdt->cfg->irq_mask) {
> @@ -459,14 +474,26 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> }
>
> status = readl(wdt->base + WDT_TIMEOUT_STATUS);
> - if (status & WDT_TIMEOUT_STATUS_BOOT_SECONDARY) {
> + if (status & WDT_TIMEOUT_STATUS_EVENT)
> wdt->wdd.bootstatus = WDIOF_CARDRESET;
>
> - if (of_device_is_compatible(np, "aspeed,ast2400-wdt") ||
> - of_device_is_compatible(np, "aspeed,ast2500-wdt"))
> - wdt->wdd.groups = bswitch_groups;
> + if (of_device_is_compatible(np, "aspeed,ast2600-wdt")) {
> + ret = regmap_read(scu_base,
> + AST2600_SYSTEM_RESET_EVENT,
> + &status);
> + } else {
> + ret = regmap_read(scu_base,
> + AST2500_SYSTEM_RESET_EVENT,
> + &status);
> + wdt->wdd.groups = bswitch_groups;
> }
>
> + /*
> + * Reset cause by Extern Reset
> + */
> + if (status & EXTERN_RESET_FLAG && !ret)
> + wdt->wdd.bootstatus |= WDIOF_EXTERN1;
> +
> dev_set_drvdata(dev, wdt);
>
> return devm_watchdog_register_device(dev, &wdt->wdd);
Hi Guenter,
Could you help me understand the definition of WDIOF_CARDRESET in
the kernel? If it resets the CPU, should all values be reset to default?
Should we check the POR (RstPwr Power on reset SRST# flag) flag in SCU
0x74 register bit 0 in ast2600?
^ 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