* Re: [PATCH v5 1/1] dt-bindings: net: dwmac: Document STM32 property st,ext-phyclk
From: Christophe ROULLIER @ 2024-03-28 15:07 UTC (permalink / raw)
To: Marek Vasut, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maxime Coquelin, Alexandre Torgue, Richard Cochran, Jose Abreu,
Liam Girdwood, Mark Brown
Cc: netdev, devicetree, linux-stm32, linux-arm-kernel, linux-kernel
In-Reply-To: <480d4064-b553-4005-ad98-499a862703ff@denx.de>
On 3/28/24 15:19, Marek Vasut wrote:
> On 3/28/24 3:08 PM, Christophe Roullier wrote:
>
> [...]
>
>> | RMII | - | eth-ck | eth-ck | n/a |
>> | | | st,ext-phyclk | st,eth-ref-clk-sel
>> | |
>> | | | | or st,ext-phyclk
>> | |
>>
>> ---------------------------------------------------------------------------
>>
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com>
>> ---
>> Documentation/devicetree/bindings/net/stm32-dwmac.yaml | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> index fc8c96b08d7dc..b35eae80ed6ac 100644
>> --- a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> @@ -82,6 +82,13 @@ properties:
>> Should be phandle/offset pair. The phandle to the syscon node
>> which
>> encompases the glue register, and the offset of the control
>> register
>> +st,ext-phyclk:
>
> Don't you need two spaces in front of the 'st,' here ?
Sorry, that's right.
>
>> + description:
>> + set this property in RMII mode when you have PHY without
>> crystal 50MHz and want to
>> + select RCC clock instead of ETH_REF_CLK. OR in RGMII mode when
>> you want to select
>> + RCC clock instead of ETH_CLK125.
>> + type: boolean
>> +
>
> With that fixed:
>
> Reviewed-by: Marek Vasut <marex@denx.de>
^ permalink raw reply
* Re: [PATCH 3/3] arm64: dts: qcom: sm6350: Add DisplayPort controller
From: Dmitry Baryshkov @ 2024-03-28 15:11 UTC (permalink / raw)
To: Luca Weiss
Cc: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Daniel Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Kuogee Hsieh, Krishna Manikandan, Bjorn Andersson, Konrad Dybcio,
~postmarketos/upstreaming, phone-devel, linux-arm-msm, dri-devel,
freedreno, devicetree, linux-kernel
In-Reply-To: <20240328-sm6350-dp-v1-3-215ca2b81c35@fairphone.com>
On Thu, 28 Mar 2024 at 11:43, Luca Weiss <luca.weiss@fairphone.com> wrote:
>
> Add the node for the DisplayPort controller found on the SM6350 SoC.
>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
> arch/arm64/boot/dts/qcom/sm6350.dtsi | 88 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 88 insertions(+)
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH net-next v6 12/17] net: pse-pd: Add support for setup_pi_matrix callback
From: Andrew Lunn @ 2024-03-28 15:14 UTC (permalink / raw)
To: Kory Maincent
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Luis Chamberlain, Russ Weight,
Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Oleksij Rempel, Mark Brown,
Frank Rowand, Heiner Kallweit, Russell King, Thomas Petazzoni,
netdev, linux-kernel, linux-doc, devicetree, Dent Project
In-Reply-To: <20240326-feature_poe-v6-12-c1011b6ea1cb@bootlin.com>
On Tue, Mar 26, 2024 at 03:04:49PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Implement setup_pi_matrix callback to configure the PSE PI matrix. This
> functionality is invoked before registering the PSE and following the core
> parsing of the pse_pis devicetree subnode.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 08/10] iio: backend: add new functionality
From: Jonathan Cameron @ 2024-03-28 15:16 UTC (permalink / raw)
To: Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240328-iio-backend-axi-dac-v1-8-afc808b3fde3@analog.com>
On Thu, 28 Mar 2024 14:22:32 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> From: Nuno Sa <nuno.sa@analog.com>
>
> This adds the needed backend ops for supporting a backend inerfacing
> with an high speed dac. The new ops are:
>
> * data_source_set();
> * set_sampling_freq();
> * extend_chan_spec();
> * ext_info_set();
> * ext_info_get().
>
> Also to note the new helpers that are meant to be used by the backends
> when extending an IIO channel (adding extended info):
>
> * iio_backend_ext_info_set();
> * iio_backend_ext_info_get().
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
I'm pretty flexible on this so far as I think we are still learning how front
ends and backends should interact. Maybe we'll figure that out in the medium
term and rework this stuff. For now it looks fine. A few minor things inline.
>
> +/**
> + * iio_backend_ext_info_get - IIO ext_info read callback
> + * @indio_dev: IIO device
> + * @private: Data private to the driver
> + * @chan: IIO channel
> + * @buf: Buffer where to place the attribute data
> + *
> + * This helper is intended to be used by backends that extend an IIO channel
> + * (trough iio_backend_extend_chan_spec()) with extended info. In that case,
> + * backends are not supposed to give their own callbacks (as they would not
> + * a way to get te backend from indio_dev). This is the getter.
te->the?
> +/**
> + * iio_backend_extend_chan_spec - Extend an IIO channel
> + * @indio_dev: IIO device
> + * @back: Backend device
> + * @chan: IIO channel
> + *
> + * Some backends may have their own functionalities and hence capable of
> + * extending a frontend's channel.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
> + struct iio_backend *back,
> + struct iio_chan_spec *chan)
> +{
> + const struct iio_chan_spec_ext_info *ext_info = chan->ext_info;
This is getting confusing. So this one is the front end value of ext_info?
Name it as such frontend_ext_info
> + int ret;
> +
> + ret = iio_backend_op_call(back, extend_chan_spec, chan);
> + if (ret)
> + return ret;
> + /*
> + * Let's keep things simple for now. Don't allow to overwrite the
> + * frontend's extended info. If ever needed, we can support appending
> + * it.
> + */
> + if (ext_info && chan->ext_info != ext_info)
> + return -EOPNOTSUPP;
> + if (!chan->ext_info)
This is checking if the backend added anything? Perhaps a comment on that
as we don't need a backend_ext_info local variable...
> + return 0;
> + /*
> + * !\NOTE: this will break as soon as we have multiple backends on one
> + * frontend and all of them extend channels. In that case, the core
> + * backend code has no way to get the correct backend given the
> + * iio device.
> + *
> + * One solution for this could be introducing a new backend
> + * dedicated callback in struct iio_info so we can callback into the
> + * frontend so it can give us the right backend given a chan_spec.
> + */
Hmm. This is indeed messy. Could we associate it with the buffer as presuably
a front end with multiple backends is using multiple IIO buffers?
As you say a dance via the front end would work fine.
> + iio_device_set_drvdata(indio_dev, back);
> +
> + /* Don't allow backends to get creative and force their own handlers */
> + for (ext_info = chan->ext_info; ext_info->name; ext_info++) {
> + if (ext_info->read != iio_backend_ext_info_get)
> + return -EINVAL;
> + if (ext_info->write != iio_backend_ext_info_set)
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(iio_backend_extend_chan_spec, IIO_BACKEND);
> diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> index a6d79381866e..09ff2f8f9fd8 100644
> --- a/include/linux/iio/backend.h
> +++ b/include/linux/iio/backend.h
> @@ -4,6 +4,7 @@
>
> #include <linux/types.h>
>
> +struct iio_chan_spec;
> struct fwnode_handle;
> struct iio_backend;
> struct device;
> @@ -15,6 +16,26 @@ enum iio_backend_data_type {
> IIO_BACKEND_DATA_TYPE_MAX
> };
>
> +enum iio_backend_data_source {
> + IIO_BACKEND_INTERNAL_CW,
CW? Either expand out what ever that is in definition of add a comment
at least.
> + IIO_BACKEND_EXTERNAL,
What does external mean in this case?
> + IIO_BACKEND_DATA_SOURCE_MAX
> +};
> +
> +/**
> + * IIO_BACKEND_EX_INFO - Helper for an IIO extended channel attribute
> + * @_name: Attribute name
> + * @_shared: Whether the attribute is shared between all channels
> + * @_what: Data private to the driver
> + */
> +#define IIO_BACKEND_EX_INFO(_name, _shared, _what) { \
> + .name = (_name), \
> + .shared = (_shared), \
> + .read = iio_backend_ext_info_get, \
> + .write = iio_backend_ext_info_set, \
> + .private = (_what), \
> +}
> +
> /**
> * struct iio_backend_data_fmt - Backend data format
> * @type: Data type.
> @@ -35,8 +56,13 @@ struct iio_backend_data_fmt {
> * @chan_enable: Enable one channel.
> * @chan_disable: Disable one channel.
> * @data_format_set: Configure the data format for a specific channel.
> + * @data_source_set: Configure the data source for a specific channel.
> + * @set_sample_rate: Configure the sampling rate for a specific channel.
> * @request_buffer: Request an IIO buffer.
> * @free_buffer: Free an IIO buffer.
> + * @extend_chan_spec: Extend an IIO channel.
> + * @ext_info_set: Extended info setter.
> + * @ext_info_get: Extended info getter.
> **/
> struct iio_backend_ops {
> int (*enable)(struct iio_backend *back);
> @@ -45,10 +71,21 @@ struct iio_backend_ops {
> int (*chan_disable)(struct iio_backend *back, unsigned int chan);
> int (*data_format_set)(struct iio_backend *back, unsigned int chan,
> const struct iio_backend_data_fmt *data);
> + int (*data_source_set)(struct iio_backend *back, unsigned int chan,
> + enum iio_backend_data_source data);
> + int (*set_sample_rate)(struct iio_backend *back, unsigned int chan,
> + u64 sample_rate);
Name the parameter that so we know the units. _hz?
^ permalink raw reply
* Re: [PATCH 01/10] iio: buffer: add helper for setting direction
From: Nuno Sá @ 2024-03-28 15:18 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240328143652.068492d0@jic23-huawei>
On Thu, 2024-03-28 at 14:36 +0000, Jonathan Cameron wrote:
> On Thu, 28 Mar 2024 14:22:25 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> > From: Nuno Sa <nuno.sa@analog.com>
> >
> > Simple helper for setting the buffer direction when it's allocated using
> > iio_dmaengine_buffer_alloc().
> >
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> I wonder if we should align with the approach for triggered-buffers with and _ext
> form of the registration function that takes a direction. It seems odd to allocate
> one then change the direction.
>
I agree it feels odd but I did not wanted to include buffer_impl.h in places that
should not have it :)
This patchseries adds the direction to devm_iio_dmaengine_buffer_setup(). Maybe what
we need is to have a non devm variant iio_dmaengine_buffer_setup() and turn
iio_dmaengine_buffer_alloc() static again. Maybe that would make things a bit more
consistent. In fact looking closer into that file, I would get rid of:
devm_iio_dmaengine_buffer_alloc() and __devm_iio_dmaengine_buffer_free()
and have:
devm_iio_dmaengine_buffer_setup() and iio_dmaengine_buffer_setup() that make use of
iio_dmaengine_buffer_free() and iio_dmaengine_buffer_alloc().
I think it would make more sense like the above. Thoughts?
- Nuno Sá
^ permalink raw reply
* [PATCH] ARM: dts: imx7s-warp: Pass OV2680 link-frequencies
From: Fabio Estevam @ 2024-03-28 15:19 UTC (permalink / raw)
To: shawnguo
Cc: sakari.ailus, hdegoede, robh, krzysztof.kozlowski+dt, conor+dt,
linux-arm-kernel, devicetree, Fabio Estevam, stable
From: Fabio Estevam <festevam@denx.de>
Since commit 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint
property verification") the ov2680 no longer probes on a imx7s-warp7:
ov2680 1-0036: error -EINVAL: supported link freq 330000000 not found
ov2680 1-0036: probe with driver ov2680 failed with error -22
Fix it by passing the required 'link-frequencies' property as
recommended by:
https://www.kernel.org/doc/html/v6.9-rc1/driver-api/media/camera-sensor.html#handling-clocks
Cc: stable@vger.kernel.org
Fixes: 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint property verification")
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
arch/arm/boot/dts/nxp/imx/imx7s-warp.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/nxp/imx/imx7s-warp.dts b/arch/arm/boot/dts/nxp/imx/imx7s-warp.dts
index ba7231b364bb..7bab113ca6da 100644
--- a/arch/arm/boot/dts/nxp/imx/imx7s-warp.dts
+++ b/arch/arm/boot/dts/nxp/imx/imx7s-warp.dts
@@ -210,6 +210,7 @@ ov2680_to_mipi: endpoint {
remote-endpoint = <&mipi_from_sensor>;
clock-lanes = <0>;
data-lanes = <1>;
+ link-frequencies = /bits/ 64 <330000000>;
};
};
};
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v5 2/2] ASoC: nau8325: new driver
From: Mark Brown @ 2024-03-28 15:22 UTC (permalink / raw)
To: Seven Lee
Cc: lgirdwood, alsa-devel, devicetree, linux-sound,
krzysztof.kozlowski+dt, linux-kernel, robh+dt, conor+dt, perex,
tiwai, YHCHuang, KCHSU0, CTLIN0, SJLIN0, scott6986,
supercraig0719, dardar923
In-Reply-To: <20240327075755.3410381-3-wtli@nuvoton.com>
[-- Attachment #1: Type: text/plain, Size: 542 bytes --]
On Wed, Mar 27, 2024 at 03:57:55PM +0800, Seven Lee wrote:
> +static const char * const nau8325_dac_oversampl[] = {
> + "64", "256", "128", "", "32" };
> +
> +static const struct soc_enum nau8325_dac_oversampl_enum =
> + SOC_ENUM_SINGLE(NAU8325_R29_DAC_CTRL1, NAU8325_DAC_OVERSAMPLE_SFT,
> + ARRAY_SIZE(nau8325_dac_oversampl),
> + nau8325_dac_oversampl);
This should really be a SOC_VALUE_ENUM so you can just hide the fourth
value rather than having the empty (presumably invalid) option. Please
send an incremental patch doing this.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v6 13/17] net: pse-pd: Use regulator framework within PSE framework
From: Andrew Lunn @ 2024-03-28 15:22 UTC (permalink / raw)
To: Kory Maincent
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Luis Chamberlain, Russ Weight,
Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Oleksij Rempel, Mark Brown,
Frank Rowand, Heiner Kallweit, Russell King, Thomas Petazzoni,
netdev, linux-kernel, linux-doc, devicetree, Dent Project
In-Reply-To: <20240326-feature_poe-v6-13-c1011b6ea1cb@bootlin.com>
On Tue, Mar 26, 2024 at 03:04:50PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Integrate the regulator framework to the PSE framework for enhanced
> access to features such as voltage, power measurement, and limits, which
> are akin to regulators. Additionally, PSE features like port priorities
> could potentially enhance the regulator framework. Note that this
> integration introduces some implementation complexity, including wrapper
> callbacks, but the potential benefits make it worthwhile.
>
> Regulator are using enable counter with specific behavior.
> Two calls to regulator_disable will trigger kernel warnings.
> If the counter exceeds one, regulator_disable call won't disable the
> PSE PI. These behavior isn't suitable for PSE control.
> Added a boolean 'enabled' state to prevent multiple calls to
> regulator_enable/disable. These calls will only be called from PSE
> framework as it won't have any regulator children, therefore no mutex are
> needed to safeguards this boolean.
>
> regulator_get needs the consumer device pointer. Use PSE as regulator
> provider and consumer device until we have RJ45 ports represented in
> the Kernel.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v5 1/1] dt-bindings: net: dwmac: Document STM32 property st,ext-phyclk
From: Rob Herring @ 2024-03-28 15:25 UTC (permalink / raw)
To: Christophe Roullier
Cc: linux-arm-kernel, Marek Vasut, Mark Brown, Jose Abreu,
Eric Dumazet, linux-kernel, Richard Cochran, Jakub Kicinski,
Rob Herring, linux-stm32, devicetree, Liam Girdwood,
David S . Miller, Maxime Coquelin, Conor Dooley,
Krzysztof Kozlowski, Alexandre Torgue, netdev, Paolo Abeni
In-Reply-To: <20240328140803.324141-2-christophe.roullier@foss.st.com>
On Thu, 28 Mar 2024 15:08:03 +0100, Christophe Roullier wrote:
> The Linux kernel dwmac-stm32 driver currently supports three DT
> properties used to configure whether PHY clock are generated by
> the MAC or supplied to the MAC from the PHY.
>
> Originally there were two properties, st,eth-clk-sel and
> st,eth-ref-clk-sel, each used to configure MAC clocking in
> different bus mode and for different MAC clock frequency.
> Since it is possible to determine the MAC 'eth-ck' clock
> frequency from the clock subsystem and PHY bus mode from
> the 'phy-mode' property, two disparate DT properties are
> no longer required to configure MAC clocking.
>
> Linux kernel commit 1bb694e20839 ("net: ethernet: stmmac: simplify phy modes management for stm32")
> introduced a third, unified, property st,ext-phyclk. This property
> covers both use cases of st,eth-clk-sel and st,eth-ref-clk-sel DT
> properties, as well as a new use case for 25 MHz clock generated
> by the MAC.
>
> The third property st,ext-phyclk is so far undocumented,
> document it.
>
> Below table summarizes the clock requirement and clock sources for
> supported PHY interface modes.
> __________________________________________________________________________
> |PHY_MODE | Normal | PHY wo crystal| PHY wo crystal |No 125Mhz from PHY|
> | | | 25MHz | 50MHz | |
>
> ---------------------------------------------------------------------------
> | MII | - | eth-ck | n/a | n/a |
> | | | st,ext-phyclk | | |
>
> ---------------------------------------------------------------------------
> | GMII | - | eth-ck | n/a | n/a |
> | | | st,ext-phyclk | | |
>
> ---------------------------------------------------------------------------
> | RGMII | - | eth-ck | n/a | eth-ck |
> | | | st,ext-phyclk | | st,eth-clk-sel or|
> | | | | | st,ext-phyclk |
>
> ---------------------------------------------------------------------------
> | RMII | - | eth-ck | eth-ck | n/a |
> | | | st,ext-phyclk | st,eth-ref-clk-sel | |
> | | | | or st,ext-phyclk | |
>
> ---------------------------------------------------------------------------
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com>
> ---
> Documentation/devicetree/bindings/net/stm32-dwmac.yaml | 7 +++++++
> 1 file changed, 7 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/net/stm32-dwmac.yaml:86:5: [warning] wrong indentation: expected 2 but found 4 (indentation)
./Documentation/devicetree/bindings/net/stm32-dwmac.yaml:92:3: [error] syntax error: expected <block end>, but found '<block mapping start>' (syntax)
dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/net/stm32-dwmac.example.dts'
Documentation/devicetree/bindings/net/stm32-dwmac.yaml:92:3: did not find expected key
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/stm32-dwmac.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/net/stm32-dwmac.yaml:92:3: did not find expected key
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/stm32-dwmac.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240328140803.324141-2-christophe.roullier@foss.st.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply
* Re: [PATCH net-next v6 14/17] dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller
From: Andrew Lunn @ 2024-03-28 15:27 UTC (permalink / raw)
To: Kory Maincent
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Luis Chamberlain, Russ Weight,
Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Oleksij Rempel, Mark Brown,
Frank Rowand, Heiner Kallweit, Russell King, Thomas Petazzoni,
netdev, linux-kernel, linux-doc, devicetree, Dent Project
In-Reply-To: <20240326-feature_poe-v6-14-c1011b6ea1cb@bootlin.com>
On Tue, Mar 26, 2024 at 03:04:51PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Add the PD692x0 I2C Power Sourcing Equipment controller device tree
> bindings documentation.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [net-next,v4 0/2] ravb: Support describing the MDIO bus
From: Jakub Kicinski @ 2024-03-28 15:28 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sergey Shtylyov, David S. Miller, Eric Dumazet, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Claudiu Beznea, Yoshihiro Shimoda, Biju Das,
netdev, devicetree, linux-renesas-soc
In-Reply-To: <20240328094546.GI1108818@ragnatech.se>
On Thu, 28 Mar 2024 10:45:46 +0100 Niklas Söderlund wrote:
> This series was marked as Deferred in patchwork. I just wonder why that
> is? Patch 1/2 touches bindings so it could go thru the Renesas tree but
> patch 2/2 touches the driver and depends on 1/2. Should not this whole
> series go thru net-next?
I don't see why either. Looks ready to apply, TBH.
pw-bot: under-review
I'll get to applying later today, hopefully.
^ permalink raw reply
* Re: [PATCH] dt-bindings: arm: qcom: Add Samsung Galaxy Z Fold5
From: Konrad Dybcio @ 2024-03-28 15:34 UTC (permalink / raw)
To: Alexandru Serdeliuc, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20620ab0-5024-439e-943b-ab12d35a60d8@yahoo.com>
On 28.03.2024 4:10 PM, Alexandru Serdeliuc wrote:
> Hi Konrad,
>
> Thanks, I unfortunately sent the patch 2 prior seeing your reply.
>
> The warning was this one which says that i need to send the mods separately in two patches:
>
>>>>
>
>>>>WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
Hm, if they were two separate patches, this is a false-positive. Could you
push the branch somewhere, so that we can report it to checkpatch maintainers?
>
>>>>
>
>
> I suppose that me sending two separate patches was not good, how i can fix this?
Please pick them both onto a single branch and send together as a series,
with a revision bump (v2) and mention that you made no changes other than
combining the two in the cover letter.
Konrad
^ permalink raw reply
* Re: [PATCH 09/10] iio: dac: add support for AXI DAC IP core
From: Jonathan Cameron @ 2024-03-28 15:35 UTC (permalink / raw)
To: Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240328-iio-backend-axi-dac-v1-9-afc808b3fde3@analog.com>
On Thu, 28 Mar 2024 14:22:33 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> From: Nuno Sa <nuno.sa@analog.com>
>
> Support the Analog Devices Generic AXI DAC IP core. The IP core is used
> for interfacing with digital-to-analog (DAC) converters that require either
> a high-speed serial interface (JESD204B/C) or a source synchronous parallel
> interface (LVDS/CMOS). Typically (for such devices) SPI will be used for
> configuration only, while this IP core handles the streaming of data into
> memory via DMA.
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
A few minor things inline, but mostly seems fine to me.
Jonathan
...
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> new file mode 100644
> index 000000000000..0022ecb4e4bb
> --- /dev/null
> +++ b/drivers/iio/dac/adi-axi-dac.c
> +
> +enum {
> + AXI_DAC_FREQ_TONE_1,
> + AXI_DAC_FREQ_TONE_2,
> + AXI_DAC_SCALE_TONE_1,
> + AXI_DAC_SCALE_TONE_2,
> + AXI_DAC_PHASE_TONE_1,
> + AXI_DAC_PHASE_TONE_2,
> +};
> +
> +static int __axi_dac_frequency_get(struct axi_dac_state *st, unsigned int chan,
> + unsigned int tone, unsigned int *freq)
> +{
> + u32 reg, raw;
> + int ret;
> +
> + if (!st->dac_clk) {
> + dev_err(st->dev, "Sampling rate is 0...\n");
> + return -EINVAL;
> + }
> +
> + if (tone == AXI_DAC_FREQ_TONE_1)
Given this is matching 2 out of enum with other values, it would be more
locally readable as a switch statement with an error returning default.
Then we wouldn't need to look at the caller.
Or at the caller convert from the enum to 0,1 for all these functions.
> + reg = AXI_DAC_REG_CHAN_CNTRL_2(chan);
> + else
> + reg = AXI_DAC_REG_CHAN_CNTRL_4(chan);
> +
> + ret = regmap_read(st->regmap, reg, &raw);
> + if (ret)
> + return ret;
> +
> + raw = FIELD_GET(AXI_DAC_FREQUENCY, raw);
> + *freq = DIV_ROUND_CLOSEST_ULL(raw * st->dac_clk, BIT(16));
> +
> + return 0;
> +}
...
> +static int axi_dac_scale_set(struct axi_dac_state *st,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len, unsigned int tone)
> +{
> + int integer, frac, scale;
> + u32 raw = 0, reg;
> + int ret;
> +
> + ret = iio_str_to_fixpoint(buf, 100000, &integer, &frac);
> + if (ret)
> + return ret;
> +
> + scale = integer * MEGA + frac;
> + if (scale <= -2 * (int)MEGA || scale >= 2 * (int)MEGA)
> + return -EINVAL;
> +
> + /* format is 1.1.14 (sign, integer and fractional bits) */
> + if (scale < 0) {
> + raw = FIELD_PREP(AXI_DAC_SCALE_SIGN, 1);
> + scale *= -1;
> + }
> +
> + raw |= div_u64((u64)scale * AXI_DAC_SCALE_INT, MEGA);
> +
> + if (tone == AXI_DAC_SCALE_TONE_1)
> + reg = AXI_DAC_REG_CHAN_CNTRL_1(chan->channel);
> + else
> + reg = AXI_DAC_REG_CHAN_CNTRL_3(chan->channel);
> +
> + guard(mutex)(&st->lock);
> + ret = regmap_write(st->regmap, reg, raw);
> + if (ret)
> + return ret;
> +
> + /* synchronize channels */
> + ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_1, AXI_DAC_SYNC);
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +
> +static int axi_dac_phase_set(struct axi_dac_state *st,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len, unsigned int tone)
> +{
> + int integer, frac, phase;
> + u32 raw, reg;
> + int ret;
> +
> + ret = iio_str_to_fixpoint(buf, 100000, &integer, &frac);
> + if (ret)
> + return ret;
> +
> + phase = integer * MEGA + frac;
> + if (phase < 0 || phase > AXI_DAC_2_PI_MEGA)
> + return -EINVAL;
> +
> + raw = DIV_ROUND_CLOSEST_ULL((u64)phase * U16_MAX, AXI_DAC_2_PI_MEGA);
> +
> + if (tone == AXI_DAC_PHASE_TONE_1)
Preference for a switch so it's clear there are only 2 choices.
> + reg = AXI_DAC_REG_CHAN_CNTRL_2(chan->channel);
> + else
> + reg = AXI_DAC_REG_CHAN_CNTRL_4(chan->channel);
> +
> + guard(mutex)(&st->lock);
> + ret = regmap_update_bits(st->regmap, reg, AXI_DAC_PHASE,
> + FIELD_PREP(AXI_DAC_PHASE, raw));
> + if (ret)
> + return ret;
> +
> + /* synchronize channels */
> + ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_1, AXI_DAC_SYNC);
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +
> +static int axi_dac_ext_info_set(struct iio_backend *back, uintptr_t private,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (private) {
> + case AXI_DAC_FREQ_TONE_1:
> + case AXI_DAC_FREQ_TONE_2:
Same as the get path - convert to which tone here so that the enum becomes
a tone index for the functions called and the mapping to that single enum
is kept clear of the lower level code.
> + return axi_dac_frequency_set(st, chan, buf, len, private);
> + case AXI_DAC_SCALE_TONE_1:
> + case AXI_DAC_SCALE_TONE_2:
> + return axi_dac_scale_set(st, chan, buf, len, private);
> + case AXI_DAC_PHASE_TONE_1:
> + case AXI_DAC_PHASE_TONE_2:
> + return axi_dac_phase_set(st, chan, buf, len, private);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int axi_dac_ext_info_get(struct iio_backend *back, uintptr_t private,
> + const struct iio_chan_spec *chan, char *buf)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (private) {
> + case AXI_DAC_FREQ_TONE_1:
> + case AXI_DAC_FREQ_TONE_2:
> + return axi_dac_frequency_get(st, chan, buf, private);
I'd break out private as an unsigned int here and then - AXI_DAC_FREQ_TONE_1
so that it is just which tone for all the calls made from here.
Similar for the following ones.
> + case AXI_DAC_SCALE_TONE_1:
> + case AXI_DAC_SCALE_TONE_2:
> + return axi_dac_scale_get(st, chan, buf, private);
> + case AXI_DAC_PHASE_TONE_1:
> + case AXI_DAC_PHASE_TONE_2:
> + return axi_dac_phase_get(st, chan, buf, private);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static const struct iio_chan_spec_ext_info axi_dac_ext_info[] = {
> + IIO_BACKEND_EX_INFO("frequency0", IIO_SEPARATE, AXI_DAC_FREQ_TONE_1),
> + IIO_BACKEND_EX_INFO("frequency1", IIO_SEPARATE, AXI_DAC_FREQ_TONE_2),
> + IIO_BACKEND_EX_INFO("scale0", IIO_SEPARATE, AXI_DAC_SCALE_TONE_1),
> + IIO_BACKEND_EX_INFO("scale1", IIO_SEPARATE, AXI_DAC_SCALE_TONE_2),
> + IIO_BACKEND_EX_INFO("phase0", IIO_SEPARATE, AXI_DAC_PHASE_TONE_1),
> + IIO_BACKEND_EX_INFO("phase1", IIO_SEPARATE, AXI_DAC_PHASE_TONE_2),
> + {}
> +};
> +
> +static int axi_dac_extend_chan(struct iio_backend *back,
> + struct iio_chan_spec *chan)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + if (chan->type != IIO_ALTVOLTAGE)
> + return -EINVAL;
> + if (st->reg_config & AXI_DDS_DISABLE)
> + /* nothing to extend */
> + return 0;
> +
> + chan->ext_info = axi_dac_ext_info;
> +
> + return 0;
> +}
> +static int axi_dac_set_sample_rate(struct iio_backend *back, unsigned int chan,
> + u64 sample_rate)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> + unsigned int freq;
> + int ret, tone;
> +
> + if (!sample_rate)
> + return -EINVAL;
> + if (st->reg_config & AXI_DDS_DISABLE)
> + /* nothing to care if DDS is disabled */
Rephrase this. Is the point that the sample rate has no meaning without DDS or
that it has meaning but nothing to do here?
> + return 0;
> +
> + guard(mutex)(&st->lock);
> + /*
> + * If dac_clk is 0 then this must be the first time we're being notified
> + * about the interface sample rate. Hence, just update our internal
> + * variable and bail... If it's not 0, then we get the current DDS
> + * frequency (for the old rate) and update the registers for the new
> + * sample rate.
> + */
> + if (!st->dac_clk) {
> + st->dac_clk = sample_rate;
> + return 0;
> + }
> +
> + for (tone = 0; tone <= AXI_DAC_FREQ_TONE_2; tone++) {
> + ret = __axi_dac_frequency_get(st, chan, tone, &freq);
> + if (ret)
> + return ret;
> +
> + ret = __axi_dac_frequency_set(st, chan, sample_rate, tone, freq);
> + if (ret)
> + return ret;
> + }
> +
> + st->dac_clk = sample_rate;
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH v8 19/20] virt: geniezone: Add tracing support for hyp call and vcpu exit_reason
From: Steven Rostedt @ 2024-03-28 15:40 UTC (permalink / raw)
To: Yi-De Wu
Cc: Yingshiuan Pan, Ze-Yu Wang, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, Catalin Marinas, Wihl Deacon,
Masami Hiramatsu, Mathieu Desnoyers, Richard Cochran,
Matthias Brugger, AngeloGioacchino Del Regno, devicetree,
linux-kernel, linux-doc, linux-arm-kernel, linux-trace-kernel,
netdev, linux-mediatek, David Bradil, Trilok Soni, Jade Shih,
Ivan Tseng, My Chuang, Shawn Hsiao, PeiLun Suei, Liju Chen,
Willix Yeh, Kevenny Hsieh
In-Reply-To: <20231228105147.13752-20-yi-de.wu@mediatek.com>
On Thu, 28 Dec 2023 18:51:46 +0800
Yi-De Wu <yi-de.wu@mediatek.com> wrote:
> Add tracepoints for hypervisor calls and VCPU exit reasons in GenieZone
> driver. It aids performance debugging by providing more information
> about hypervisor operations and VCPU behavior.
>
> Command Usage:
> echo geniezone:* >> /sys/kernel/tracing/set_event
> echo 1 > /sys/kernel/tracing/tracing_on
> echo 0 > /sys/kernel/tracing/tracing_on
> cat /sys/kernel/tracing/trace
>
> For example:
> crosvm_vcpu0-4838 [004] ..... 76053.536034: mtk_hypcall_enter: id=0xbb001005
> crosvm_vcpu0-4838 [004] ..... 76053.540039: mtk_hypcall_leave: id=0xbb001005 invalid=0
> crosvm_vcpu0-4838 [004] ..... 76053.540040: mtk_vcpu_exit: vcpu exit_reason=0x92920003
Cleaning out patchwork, I noticed this patch.
You can make the above more informative by having it output:
crosvm_vcpu0-4838 [004] ..... 76053.540040: mtk_vcpu_exit: vcpu exit_reason=IRQ
>
> This example tracks a hypervisor function call by an ID (`0xbb001005`)
> from initiation to termination, which is supported (invalid=0). A vCPU
> exit is triggered by an Interrupt Request (IRQ) (exit reason: 0x92920003).
>
> /* VM exit reason */
> enum {
> GZVM_EXIT_UNKNOWN = 0x92920000,
> GZVM_EXIT_MMIO = 0x92920001,
> GZVM_EXIT_HYPERCALL = 0x92920002,
> GZVM_EXIT_IRQ = 0x92920003,
> GZVM_EXIT_EXCEPTION = 0x92920004,
> GZVM_EXIT_DEBUG = 0x92920005,
> GZVM_EXIT_FAIL_ENTRY = 0x92920006,
> GZVM_EXIT_INTERNAL_ERROR = 0x92920007,
> GZVM_EXIT_SYSTEM_EVENT = 0x92920008,
> GZVM_EXIT_SHUTDOWN = 0x92920009,
> GZVM_EXIT_GZ = 0x9292000a,
> };
>
> Signed-off-by: Liju-clr Chen <liju-clr.chen@mediatek.com>
> Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
> ---
> arch/arm64/geniezone/vm.c | 5 +++
> drivers/virt/geniezone/gzvm_vcpu.c | 3 ++
> include/trace/events/geniezone.h | 54 ++++++++++++++++++++++++++++++
> 3 files changed, 62 insertions(+)
> create mode 100644 include/trace/events/geniezone.h
>
> diff --git a/arch/arm64/geniezone/vm.c b/arch/arm64/geniezone/vm.c
> index a9d264bbb3b1..5667643251b5 100644
> --- a/arch/arm64/geniezone/vm.c
> +++ b/arch/arm64/geniezone/vm.c
> @@ -7,6 +7,8 @@
> #include <linux/err.h>
> #include <linux/uaccess.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/geniezone.h>
> #include <linux/gzvm.h>
> #include <linux/gzvm_drv.h>
> #include "gzvm_arch_common.h"
> @@ -33,7 +35,10 @@ int gzvm_hypcall_wrapper(unsigned long a0, unsigned long a1,
> unsigned long a6, unsigned long a7,
> struct arm_smccc_res *res)
> {
> + trace_mtk_hypcall_enter(a0);
> arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res);
> + trace_mtk_hypcall_leave(a0, (res->a0 != ERR_NOT_SUPPORTED) ? 0 : 1);
> +
> return gzvm_err_to_errno(res->a0);
> }
>
> diff --git a/drivers/virt/geniezone/gzvm_vcpu.c b/drivers/virt/geniezone/gzvm_vcpu.c
> index 86c690749277..138ec064596b 100644
> --- a/drivers/virt/geniezone/gzvm_vcpu.c
> +++ b/drivers/virt/geniezone/gzvm_vcpu.c
> @@ -10,6 +10,8 @@
> #include <linux/mm.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> +
> +#include <trace/events/geniezone.h>
> #include <linux/gzvm_drv.h>
>
> /* maximum size needed for holding an integer */
> @@ -103,6 +105,7 @@ static long gzvm_vcpu_run(struct gzvm_vcpu *vcpu, void __user *argp)
>
> while (!need_userspace && !signal_pending(current)) {
> gzvm_arch_vcpu_run(vcpu, &exit_reason);
> + trace_mtk_vcpu_exit(exit_reason);
>
> switch (exit_reason) {
> case GZVM_EXIT_MMIO:
> diff --git a/include/trace/events/geniezone.h b/include/trace/events/geniezone.h
> new file mode 100644
> index 000000000000..1fa44f9c4b3c
> --- /dev/null
> +++ b/include/trace/events/geniezone.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2023 MediaTek Inc.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM geniezone
> +
> +#define _TRACE_GENIEZONE_H
> +
> +#include <linux/tracepoint.h>
#define GZVM_EXIT_REASONS \
EM(UNKNOWN) \
EM(MMIO) \
EM(HYPERCALL) \
EM(IRQ) \
EM(EXCEPTION) \
EM(DEBUG) \
EM(FAIL_ENTRY) \
EM(INTERNAL_ERROR) \
EM(SYSTEM_EVENT) \
EM(SHUTDOWN) \
EMe(GZ)
#undef EM
#undef EMe
#define EM(a) TRACE_DEFINE_ENUM(GZVM_EXIT_##a);
#define EMe(a) TRACE_DEFINE_ENUM(GZVM_EXIT_##a);
GZVM_EXIT_REASONS
#undef EM
#undef EMe
#define EM(a) { GZVM_EXIT_##a, #a },
#define EMe(a) { GZVM_EXIT_##a, #a }
> +
> +TRACE_EVENT(mtk_hypcall_enter,
> + TP_PROTO(unsigned long id),
> +
> + TP_ARGS(id),
> +
> + TP_STRUCT__entry(__field(unsigned long, id)),
> +
> + TP_fast_assign(__entry->id = id;),
> +
> + TP_printk("id=0x%lx", __entry->id)
> +);
> +
> +TRACE_EVENT(mtk_hypcall_leave,
> + TP_PROTO(unsigned long id, unsigned long invalid),
> +
> + TP_ARGS(id, invalid),
> +
> + TP_STRUCT__entry(__field(unsigned long, id)
> + __field(unsigned long, invalid)
> + ),
> +
> + TP_fast_assign(__entry->id = id;
> + __entry->invalid = invalid;
> + ),
> +
> + TP_printk("id=0x%lx invalid=%lu", __entry->id, __entry->invalid)
> +);
> +
> +TRACE_EVENT(mtk_vcpu_exit,
> + TP_PROTO(unsigned long exit_reason),
> +
> + TP_ARGS(exit_reason),
> +
> + TP_STRUCT__entry(__field(unsigned long, exit_reason)),
> +
> + TP_fast_assign(__entry->exit_reason = exit_reason;),
> +
> + TP_printk("vcpu exit_reason=0x%lx", __entry->exit_reason)
TP_printk("vcpu exit_reason=0x%lx",
__print_symbolic(__entry->exit_reason, GZVM_EXIT_REASONS))
And instead of having the cryptic enum values printed, you will have human
readable reasons.
-- Steve
> +);
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
^ permalink raw reply
* Re: [PATCH] dt-bindings: arm: qcom: Add Samsung Galaxy Z Fold5
From: Alexandru Serdeliuc @ 2024-03-28 15:10 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <8e09b779-a18f-46b4-926c-40e2a5782d85@linaro.org>
Hi Konrad,
Thanks, I unfortunately sent the patch 2 prior seeing your reply.
The warning was this one which says that i need to send the mods
separately in two patches:
>>>
>>>WARNING: DT binding docs and includes should be a separate patch.
See: Documentation/devicetree/bindings/submitting-patches.rst
>>>
I suppose that me sending two separate patches was not good, how i can
fix this?
Best regards,
Marc
On 28/3/24 15:47, Konrad Dybcio wrote:
> On 28.03.2024 3:42 PM, Alexandru Serdeliuc wrote:
>> Hi Konrad,
>>
>>
>> Thanks, yes, I am new to b4 and sending patches, in a few minutes I will add the second patch.
>>
>> That actually add the device tree, but without the previous patch it showed me a warning, and with both patches provided another warning that i need to split them in two.
> Oh no, you should send them together! Could you please paste the warning so that
> we can work out the issue?
>
> Konrad
^ permalink raw reply
* Re: [PATCH] dt-bindings: arm: qcom: Add Samsung Galaxy Z Fold5
From: Alexandru Serdeliuc @ 2024-03-28 15:42 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <6b77633e-c501-4488-9b10-1881cfbf6f2c@linaro.org>
Is there anything I need to do to sent them as a series? Or only modify
all 3 files on a new branch and send them as a new patch?
The situation can be replicated easy, modify the following two files
Documentation/devicetree/bindings/arm/qcom.yaml
arch/arm64/boot/dts/qcom/Makefile
Then generate the pach and check, will show this
$ ./scripts/checkpatch.pl
/tmp/tosend2/0001-arm64-dts-qcom-add-support-for-samsung-galaxy-z-fold5.eml
WARNING: Missing commit description - Add an appropriate one
WARNING: DT binding docs and includes should be a separate patch. See:
Documentation/devicetree/bindings/submitting-patches.rst
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#59:
new file mode 100644
total: 0 errors, 3 warnings, 630 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.
/tmp/tosend2/0001-arm64-dts-qcom-add-support-for-samsung-galaxy-z-fold5.eml
has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
On 28/3/24 16:34, Konrad Dybcio wrote:
> On 28.03.2024 4:10 PM, Alexandru Serdeliuc wrote:
>> Hi Konrad,
>>
>> Thanks, I unfortunately sent the patch 2 prior seeing your reply.
>>
>> The warning was this one which says that i need to send the mods separately in two patches:
>>
>>>>> WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
> Hm, if they were two separate patches, this is a false-positive. Could you
> push the branch somewhere, so that we can report it to checkpatch maintainers?
>
>>
>> I suppose that me sending two separate patches was not good, how i can fix this?
> Please pick them both onto a single branch and send together as a series,
> with a revision bump (v2) and mention that you made no changes other than
> combining the two in the cover letter.
>
> Konrad
^ permalink raw reply
* Re: [PATCH 08/10] iio: backend: add new functionality
From: Nuno Sá @ 2024-03-28 15:42 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240328151632.298bd95f@jic23-huawei>
On Thu, 2024-03-28 at 15:16 +0000, Jonathan Cameron wrote:
> On Thu, 28 Mar 2024 14:22:32 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> > From: Nuno Sa <nuno.sa@analog.com>
> >
> > This adds the needed backend ops for supporting a backend inerfacing
> > with an high speed dac. The new ops are:
> >
> > * data_source_set();
> > * set_sampling_freq();
> > * extend_chan_spec();
> > * ext_info_set();
> > * ext_info_get().
> >
> > Also to note the new helpers that are meant to be used by the backends
> > when extending an IIO channel (adding extended info):
> >
> > * iio_backend_ext_info_set();
> > * iio_backend_ext_info_get().
> >
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> I'm pretty flexible on this so far as I think we are still learning how front
> ends and backends should interact. Maybe we'll figure that out in the medium
> term and rework this stuff. For now it looks fine. A few minor things inline.
> >
> > +/**
> > + * iio_backend_ext_info_get - IIO ext_info read callback
> > + * @indio_dev: IIO device
> > + * @private: Data private to the driver
> > + * @chan: IIO channel
> > + * @buf: Buffer where to place the attribute data
> > + *
> > + * This helper is intended to be used by backends that extend an IIO channel
> > + * (trough iio_backend_extend_chan_spec()) with extended info. In that case,
> > + * backends are not supposed to give their own callbacks (as they would not
> > + * a way to get te backend from indio_dev). This is the getter.
>
> te->the?
Yes and some more typos :).
>
>
> > +/**
> > + * iio_backend_extend_chan_spec - Extend an IIO channel
> > + * @indio_dev: IIO device
> > + * @back: Backend device
> > + * @chan: IIO channel
> > + *
> > + * Some backends may have their own functionalities and hence capable of
> > + * extending a frontend's channel.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
> > + struct iio_backend *back,
> > + struct iio_chan_spec *chan)
> > +{
> > + const struct iio_chan_spec_ext_info *ext_info = chan->ext_info;
> This is getting confusing. So this one is the front end value of ext_info?
> Name it as such frontend_ext_info
Yes, it's the frontend pointer. Just to enforce the below constrain. Will rename as
suggested.
>
> > + int ret;
> > +
> > + ret = iio_backend_op_call(back, extend_chan_spec, chan);
> > + if (ret)
> > + return ret;
> > + /*
> > + * Let's keep things simple for now. Don't allow to overwrite the
> > + * frontend's extended info. If ever needed, we can support appending
> > + * it.
> > + */
> > + if (ext_info && chan->ext_info != ext_info)
> > + return -EOPNOTSUPP;
> > + if (!chan->ext_info)
>
> This is checking if the backend added anything? Perhaps a comment on that
> as we don't need a backend_ext_info local variable...
Yes, but just regarding ext_info as that is the only thing we're handling below
(doing some sanity checks). Note that (as you said above) I'm keeping things a bit
"open" in that the backend is open to extend whatever it wants. With time we may
learn better what do we want constrain or not.
>
> > + return 0;
> > + /*
> > + * !\NOTE: this will break as soon as we have multiple backends on one
> > + * frontend and all of them extend channels. In that case, the core
> > + * backend code has no way to get the correct backend given the
> > + * iio device.
> > + *
> > + * One solution for this could be introducing a new backend
> > + * dedicated callback in struct iio_info so we can callback into the
> > + * frontend so it can give us the right backend given a chan_spec.
> > + */
>
> Hmm. This is indeed messy. Could we associate it with the buffer as presuably
> a front end with multiple backends is using multiple IIO buffers?
>
Hmm, the assumption of having multiple buffers seems plausible to me but considering
the example we have in hands it would be cumbersome to get the backend. Considering
iio_backend_ext_info_get(), how could we get the backend if it was associated to one
of the IIO buffers? I think we would need more "intrusive" changes to make that work
or do you have something in mind=
> As you say a dance via the front end would work fine.
I'm happy you're also open for a proper solution already. I mention this in the
cover. My idea was something like (consider the iio_backend_ext_info_get()):
if (!indio_dev->info->get_iio_backend())
return -EOPNOTSUPP;
back = indio_dev->info->get_iio_backend(indio_dev, chan_spec);
It would be nice to have some "default/generic" implementation for cases where we
only have one backend per frontend so that the frontend would not need to define the
callback.
>
>
> > + iio_device_set_drvdata(indio_dev, back);
> > +
> > + /* Don't allow backends to get creative and force their own handlers */
> > + for (ext_info = chan->ext_info; ext_info->name; ext_info++) {
> > + if (ext_info->read != iio_backend_ext_info_get)
> > + return -EINVAL;
> > + if (ext_info->write != iio_backend_ext_info_set)
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_backend_extend_chan_spec, IIO_BACKEND);
>
> > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> > index a6d79381866e..09ff2f8f9fd8 100644
> > --- a/include/linux/iio/backend.h
> > +++ b/include/linux/iio/backend.h
> > @@ -4,6 +4,7 @@
> >
> > #include <linux/types.h>
> >
> > +struct iio_chan_spec;
> > struct fwnode_handle;
> > struct iio_backend;
> > struct device;
> > @@ -15,6 +16,26 @@ enum iio_backend_data_type {
> > IIO_BACKEND_DATA_TYPE_MAX
> > };
> >
> > +enum iio_backend_data_source {
> > + IIO_BACKEND_INTERNAL_CW,
>
> CW? Either expand out what ever that is in definition of add a comment
> at least.
Continuous wave :)
>
> > + IIO_BACKEND_EXTERNAL,
> What does external mean in this case?
In this particular case comes from a DMA source (IP). I thought external to be more
generic but if you prefer, I can do something like IIO_BACKEND_DMA?
> > + IIO_BACKEND_DATA_SOURCE_MAX
> > +};
> > +
> > +/**
> > + * IIO_BACKEND_EX_INFO - Helper for an IIO extended channel attribute
> > + * @_name: Attribute name
> > + * @_shared: Whether the attribute is shared between all channels
> > + * @_what: Data private to the driver
> > + */
> > +#define IIO_BACKEND_EX_INFO(_name, _shared, _what) { \
> > + .name = (_name), \
> > + .shared = (_shared), \
> > + .read = iio_backend_ext_info_get, \
> > + .write = iio_backend_ext_info_set, \
> > + .private = (_what), \
> > +}
> > +
> > /**
> > * struct iio_backend_data_fmt - Backend data format
> > * @type: Data type.
> > @@ -35,8 +56,13 @@ struct iio_backend_data_fmt {
> > * @chan_enable: Enable one channel.
> > * @chan_disable: Disable one channel.
> > * @data_format_set: Configure the data format for a specific channel.
> > + * @data_source_set: Configure the data source for a specific channel.
> > + * @set_sample_rate: Configure the sampling rate for a specific channel.
> > * @request_buffer: Request an IIO buffer.
> > * @free_buffer: Free an IIO buffer.
> > + * @extend_chan_spec: Extend an IIO channel.
> > + * @ext_info_set: Extended info setter.
> > + * @ext_info_get: Extended info getter.
> > **/
> > struct iio_backend_ops {
> > int (*enable)(struct iio_backend *back);
> > @@ -45,10 +71,21 @@ struct iio_backend_ops {
> > int (*chan_disable)(struct iio_backend *back, unsigned int chan);
> > int (*data_format_set)(struct iio_backend *back, unsigned int chan,
> > const struct iio_backend_data_fmt *data);
> > + int (*data_source_set)(struct iio_backend *back, unsigned int chan,
> > + enum iio_backend_data_source data);
> > + int (*set_sample_rate)(struct iio_backend *back, unsigned int chan,
> > + u64 sample_rate);
>
> Name the parameter that so we know the units. _hz?
yes. And u64 to not fall in the CCF problem for 32bits :)
- Nuno Sá
^ permalink raw reply
* Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Cristian Marussi @ 2024-03-28 15:47 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: Sudeep Holla, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Dan Carpenter, linux-arm-kernel, linux-kernel,
devicetree, linux-gpio, Peng Fan, Oleksii Moisieiev
In-Reply-To: <20240323-pinctrl-scmi-v6-3-a895243257c0@nxp.com>
On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Add basic implementation of the SCMI v3.2 pincontrol protocol.
>
Hi,
a few more comments down below...
> Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/driver.c | 2 +
> drivers/firmware/arm_scmi/pinctrl.c | 921 ++++++++++++++++++++++++++++++++++
> drivers/firmware/arm_scmi/protocols.h | 1 +
> include/linux/scmi_protocol.h | 75 +++
> 5 files changed, 1000 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index a7bc4796519c..8e3874ff1544 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-y += pinctrl.o
> scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 415e6f510057..ac2d4b19727c 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3142,6 +3142,7 @@ static int __init scmi_driver_init(void)
> scmi_voltage_register();
> scmi_system_register();
> scmi_powercap_register();
> + scmi_pinctrl_register();
>
> return platform_driver_register(&scmi_driver);
> }
> @@ -3159,6 +3160,7 @@ static void __exit scmi_driver_exit(void)
> scmi_voltage_unregister();
> scmi_system_unregister();
> scmi_powercap_unregister();
> + scmi_pinctrl_unregister();
>
> scmi_transports_exit();
>
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> new file mode 100644
> index 000000000000..87d9b89cab13
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -0,0 +1,921 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Pinctrl Protocol
> + *
> + * Copyright (C) 2024 EPAM
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include "common.h"
> +#include "protocols.h"
> +
> +/* Updated only after ALL the mandatory features for that version are merged */
> +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x0
> +
AFAICS, the only missing things are PINCTRL_SET_PERMISSIONS (optional command)
and the multiple-configs on SETTINGS_GET, but this latter is something really
that we have to ask for in the request AND we did not as of now since we dont
need it...so I would say to bump the version to 0x10000 just to avoid needless
warning as soon as a server supporting Pinctrl is met.
> +#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16))
> +#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> +#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> +
> +#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31))
> +#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0))
> +
> +#define REMAINING(x) le32_get_bits((x), GENMASK(31, 16))
> +#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0))
> +
> +#define CONFIG_FLAG_MASK GENMASK(19, 18)
> +#define SELECTOR_MASK GENMASK(17, 16)
> +#define SKIP_CONFIGS_MASK GENMASK(15, 8)
> +#define CONFIG_TYPE_MASK GENMASK(7, 0)
> +
> +enum scmi_pinctrl_protocol_cmd {
> + PINCTRL_ATTRIBUTES = 0x3,
> + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> + PINCTRL_SETTINGS_GET = 0x5,
> + PINCTRL_SETTINGS_CONFIGURE = 0x6,
> + PINCTRL_REQUEST = 0x7,
> + PINCTRL_RELEASE = 0x8,
> + PINCTRL_NAME_GET = 0x9,
> + PINCTRL_SET_PERMISSIONS = 0xa
> +};
> +
> +struct scmi_msg_settings_conf {
> + __le32 identifier;
> + __le32 function_id;
> + __le32 attributes;
> + __le32 configs[];
> +};
> +
> +struct scmi_msg_settings_get {
> + __le32 identifier;
> + __le32 attributes;
> +};
> +
> +struct scmi_resp_settings_get {
> + __le32 function_selected;
> + __le32 num_configs;
> + __le32 configs[];
> +};
> +
> +struct scmi_msg_pinctrl_protocol_attributes {
> + __le32 attributes_low;
> + __le32 attributes_high;
> +};
> +
> +struct scmi_msg_pinctrl_attributes {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_resp_pinctrl_attributes {
> + __le32 attributes;
> + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> +};
> +
> +struct scmi_msg_pinctrl_list_assoc {
> + __le32 identifier;
> + __le32 flags;
> + __le32 index;
> +};
> +
> +struct scmi_resp_pinctrl_list_assoc {
> + __le32 flags;
> + __le16 array[];
> +};
> +
> +struct scmi_msg_func_set {
> + __le32 identifier;
> + __le32 function_id;
> + __le32 flags;
> +};
> +
As said by Dan...drop this.
> +struct scmi_msg_request {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_group_info {
> + char name[SCMI_MAX_STR_SIZE];
> + bool present;
> + u32 *group_pins;
> + u32 nr_pins;
> +};
> +
> +struct scmi_function_info {
> + char name[SCMI_MAX_STR_SIZE];
> + bool present;
> + u32 *groups;
> + u32 nr_groups;
> +};
> +
> +struct scmi_pin_info {
> + char name[SCMI_MAX_STR_SIZE];
> + bool present;
> +};
> +
> +struct scmi_pinctrl_info {
> + u32 version;
> + int nr_groups;
> + int nr_functions;
> + int nr_pins;
> + struct scmi_group_info *groups;
> + struct scmi_function_info *functions;
> + struct scmi_pin_info *pins;
> +};
> +
> +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> + struct scmi_pinctrl_info *pi)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_protocol_attributes *attr;
> +
> + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr),
> + &t);
> + if (ret)
> + return ret;
> +
> + attr = t->rx.buf;
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high);
> + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
> + pi->nr_pins = GET_PINS_NR(attr->attributes_low);
I was thinking, does make sense to allow a nr_pins == 0 setup to probe
successfully ? Becasuse is legit for the platform to return zero groups or
zero functions BUT zero pins is just useless (spec does not say
anything)
Maybe you could just put a dev_warn() here on if (nr_pins == 0) and bail
out with -EINVAL...
On the other side looking at the zero groups/function case, that is
plausible and handled properly by the driver since a 0-bytes
devm_kcalloc will return ZERO_SIZE_PTR (not NULL) and all the remaining
references to pinfo->groups and pinfo->functions are guarded by a check
on selector >= nr_groups (or >= nr_functions), and by scmi_pinctrl_validate_id()
so the zero grouyps/fuctions scenarios should be safely handled.
> + }
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;
> +}
> +
> +static int scmi_pinctrl_count_get(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + switch (type) {
> + case PIN_TYPE:
> + return pi->nr_pins;
> + case GROUP_TYPE:
> + return pi->nr_groups;
> + case FUNCTION_TYPE:
> + return pi->nr_functions;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int value;
> +
> + value = scmi_pinctrl_count_get(ph, type);
> + if (value < 0)
> + return value;
> +
> + if (identifier >= value)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type,
> + u32 selector, char *name,
> + u32 *n_elems)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_attributes *tx;
> + struct scmi_resp_pinctrl_attributes *rx;
> + u32 ext_name_flag;
what about a bool
> +
> + if (!name)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> + sizeof(*rx), &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + rx = t->rx.buf;
> + tx->identifier = cpu_to_le32(selector);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + if (n_elems)
> + *n_elems = NUM_ELEMS(rx->attributes);
> +
> + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
> +
> + ext_name_flag = EXT_NAME_FLAG(rx->attributes);
> + } else
> + ext_name_flag = 0;
and you dont need this else branch to set ext_name_flag to false, since down
below you will check ext_flag ONLY if !ret, so it will have surely been
set if the do_xfer did not fail.
> +
> + ph->xops->xfer_put(ph, t);
> +
> + /*
> + * If supported overwrite short name with the extended one;
> + * on error just carry on and use already provided short name.
> + */
> + if (!ret && ext_name_flag)
> + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> + (u32 *)&type, name,
> + SCMI_MAX_STR_SIZE);
> + return ret;
> +}
> +
> +struct scmi_pinctrl_ipriv {
> + u32 selector;
> + enum scmi_pinctrl_selector_type type;
> + u32 *array;
> +};
> +
> +static void iter_pinctrl_assoc_prepare_message(void *message,
> + u32 desc_index,
> + const void *priv)
> +{
> + struct scmi_msg_pinctrl_list_assoc *msg = message;
> + const struct scmi_pinctrl_ipriv *p = priv;
> +
> + msg->identifier = cpu_to_le32(p->selector);
> + msg->flags = cpu_to_le32(p->type);
> + /* Set the number of OPPs to be skipped/already read */
OPP ? .. maybe drop this comment that was cut/pasted from somewhere else :D
> + msg->index = cpu_to_le32(desc_index);
> +}
> +
> +static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
> + const void *response, void *priv)
> +{
> + const struct scmi_resp_pinctrl_list_assoc *r = response;
> +
> + st->num_returned = RETURNED(r->flags);
> + st->num_remaining = REMAINING(r->flags);
> +
> + return 0;
> +}
> +
> +static int
> +iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st, void *priv)
> +{
> + const struct scmi_resp_pinctrl_list_assoc *r = response;
> + struct scmi_pinctrl_ipriv *p = priv;
> +
> + p->array[st->desc_index + st->loop_idx] =
> + le16_to_cpu(r->array[st->loop_idx]);
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u16 size, u32 *array)
> +{
> + int ret;
> + void *iter;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_pinctrl_assoc_prepare_message,
> + .update_state = iter_pinctrl_assoc_update_state,
> + .process_response = iter_pinctrl_assoc_process_response,
> + };
> + struct scmi_pinctrl_ipriv ipriv = {
> + .selector = selector,
> + .type = type,
> + .array = array,
> + };
> +
> + if (!array || !size || type == PIN_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + iter = ph->hops->iter_response_init(ph, &ops, size,
> + PINCTRL_LIST_ASSOCIATIONS,
> + sizeof(struct scmi_msg_pinctrl_list_assoc),
> + &ipriv);
> +
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +
> +struct scmi_settings_get_ipriv {
> + u32 selector;
> + enum scmi_pinctrl_selector_type type;
> + u32 flag;
> + enum scmi_pinctrl_conf_type *config_types;
> + u32 *config_values;
> +};
> +
> +static void
> +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> + const void *priv)
> +{
> + struct scmi_msg_settings_get *msg = message;
> + const struct scmi_settings_get_ipriv *p = priv;
> + u32 attributes;
> +
> + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
> + FIELD_PREP(SELECTOR_MASK, p->type);
> +
> + if (p->flag == 1)
A boolean like .get_all would be more clear..see down below why you dont need
a flag 0|1|2
> + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> + else if (!p->flag)
> + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);
> +
> + msg->attributes = cpu_to_le32(attributes);
> + msg->identifier = cpu_to_le32(p->selector);
> +}
> +
> +static int
> +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> + const void *response, void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> +
> + if (p->flag == 1) {
Ditto... see below the explanation
> + st->num_returned = le32_get_bits(r->num_configs, GENMASK(7, 0));
> + st->num_remaining = le32_get_bits(r->num_configs,
> + GENMASK(31, 24));
> + } else {
> + st->num_returned = 1;
> + st->num_remaining = 0;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st,
> + void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> +
> + if (!p->flag) {
> + if (p->config_types[0] !=
> + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> + return -EINVAL;
> + } else if (p->flag == 1) {
> + p->config_types[st->desc_index + st->loop_idx] =
> + le32_get_bits(r->configs[st->loop_idx * 2],
> + GENMASK(7, 0));
> + } else if (p->flag == 2) {
> + return 0;
> + }
Unneeded...see down below for explanation
> +
> + p->config_values[st->desc_index + st->loop_idx] =
> + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> +
> + return 0;
> +}
> +
> +static int
> +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + enum scmi_pinctrl_conf_type config_type,
> + u32 *config_value)
> +{
> + int ret;
> + void *iter;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_pinctrl_settings_get_prepare_message,
> + .update_state = iter_pinctrl_settings_get_update_state,
> + .process_response = iter_pinctrl_settings_get_process_response,
> + };
> + struct scmi_settings_get_ipriv ipriv = {
> + .selector = selector,
> + .type = type,
> + .flag = 0,
> + .config_types = &config_type,
> + .config_values = config_value,
> + };
> +
So this function is used to retrieve configs; as of now, just one, then
it could be extended to fetch all the configs, and for this it uses the
iterators helpers, BUT it is not and will not be used to just fetch the
selected_function with flag_2 (even though is always provided), since in
that case you wont get back a multi-part SCMI response and so there is no
need to use iterators...
IOW... no need here to handle flag_2 scenario and as a consequence I would
change the ipriv flag to be be a boolean .get_all, like it was, since it is
more readable (and so you wont need to add any comment..)
In the future could make sense to add here also a *selected_function output
param to this function since you will always get it back for free when
retrieving configs ... BUT for now is just not needed really...no users
for this case till now...
...when the time will come that we will need a function_selected_get to
be issued without retrieveing also the configs I would add a distinct
routine that crafts properly a SETTINGS_GET with flag_2 without worrying
about multi-part responses (and with no need for iterators support)
Trying to handle all in here just complicates stuff...
> + if (!config_value || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_SETTINGS_GET,
> + sizeof(struct scmi_msg_settings_get),
> + &ipriv);
> +
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +
> +static int
> +scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u32 nr_configs,
> + enum scmi_pinctrl_conf_type *config_type,
> + u32 *config_value)
> +{
> + struct scmi_xfer *t;
> + struct scmi_msg_settings_conf *tx;
> + u32 attributes;
> + int ret, i;
> + u32 configs_in_chunk, conf_num = 0;
> + u32 chunk;
> + int max_msg_size = ph->hops->get_max_msg_size(ph);
> +
> + if (!config_type || !config_value || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + configs_in_chunk = (max_msg_size - sizeof(*tx)) / (sizeof(__le32) * 2);
> + while (conf_num < nr_configs) {
> + chunk = (nr_configs - conf_num > configs_in_chunk) ?
> + configs_in_chunk : nr_configs - conf_num;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
> + sizeof(*tx) +
> + chunk * 2 * sizeof(__le32),
> + 0, &t);
> + if (ret)
> + return ret;
for consistency I would
break;
like below and you will exit always from the last return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(selector);
> + attributes = FIELD_PREP(GENMASK(1, 0), type) |
> + FIELD_PREP(GENMASK(9, 2), chunk);
> + tx->attributes = cpu_to_le32(attributes);
> +
> + for (i = 0; i < chunk; i++) {
> + tx->configs[i * 2] =
> + cpu_to_le32(config_type[conf_num + i]);
> + tx->configs[i * 2 + 1] =
> + cpu_to_le32(config_value[conf_num + i]);
> + }
> +
> + ret = ph->xops->do_xfer(ph, t);
> +
> + ph->xops->xfer_put(ph, t);
> +
> + if (ret)
> + break;
> +
> + conf_num += chunk;
> + }
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
> + u32 group,
> + enum scmi_pinctrl_selector_type type,
> + u32 function_id)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_settings_conf *tx;
> + u32 attributes;
> +
> + ret = scmi_pinctrl_validate_id(ph, group, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
> + sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(group);
> + tx->function_id = cpu_to_le32(function_id);
> + attributes = FIELD_PREP(GENMASK(1, 0), type) | BIT(10);
> + tx->attributes = cpu_to_le32(attributes);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_request *tx;
> +
> + if (type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_REQUEST, sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(identifier);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
..this function ...
> +static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph,
> + u32 pin)
> +{
> + return scmi_pinctrl_request(ph, pin, PIN_TYPE);
> +}
> +
> +static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_request *tx;
> +
> + if (type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE, sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(identifier);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
...and this are completely identical, beside the used command msg_id...please make
it a common workhorse function by adding a param for the command...
> +static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle *ph, u32 pin)
> +{
> + return scmi_pinctrl_free(ph, pin, PIN_TYPE);
> +}
> +
...and convert these _request/_free functions into a pair odf simple wrapper invoking
the common workhorse...
> +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_group_info *group)
> +{
> + int ret;
> +
> + if (!group)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> + group->name,
> + &group->nr_pins);
> + if (ret)
> + return ret;
> +
> + if (!group->nr_pins) {
> + dev_err(ph->dev, "Group %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + group->group_pins = kmalloc_array(group->nr_pins,
> + sizeof(*group->group_pins),
> + GFP_KERNEL);
> + if (!group->group_pins)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> + group->nr_pins, group->group_pins);
> + if (ret) {
> + kfree(group->group_pins);
> + return ret;
> + }
> +
> + group->present = true;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!name)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_groups)
> + return -EINVAL;
> +
> + if (!pi->groups[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_group_info(ph, selector,
> + &pi->groups[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->groups[selector].name;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle *ph,
> + u32 selector, const u32 **pins,
> + u32 *nr_pins)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!pins || !nr_pins)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_groups)
> + return -EINVAL;
> +
> + if (!pi->groups[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_group_info(ph, selector,
> + &pi->groups[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *pins = pi->groups[selector].group_pins;
> + *nr_pins = pi->groups[selector].nr_pins;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_function_info *func)
> +{
> + int ret;
> +
> + if (!func)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> + func->name,
> + &func->nr_groups);
> + if (ret)
> + return ret;
> +
> + if (!func->nr_groups) {
> + dev_err(ph->dev, "Function %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups),
> + GFP_KERNEL);
> + if (!func->groups)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> + func->nr_groups, func->groups);
> + if (ret) {
> + kfree(func->groups);
> + return ret;
> + }
> +
> + func->present = true;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!name)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_functions)
> + return -EINVAL;
> +
> + if (!pi->functions[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_function_info(ph, selector,
> + &pi->functions[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->functions[selector].name;
> + return 0;
> +}
> +
> +static int
> +scmi_pinctrl_function_groups_get(const struct scmi_protocol_handle *ph,
> + u32 selector, u32 *nr_groups,
> + const u32 **groups)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!groups || !nr_groups)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_functions)
> + return -EINVAL;
> +
> + if (!pi->functions[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_function_info(ph, selector,
> + &pi->functions[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *groups = pi->functions[selector].groups;
> + *nr_groups = pi->functions[selector].nr_groups;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_mux_set(const struct scmi_protocol_handle *ph,
> + u32 selector, u32 group)
> +{
> + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE, selector);
> +}
> +
> +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> + u32 selector, struct scmi_pin_info *pin)
> +{
> + int ret;
> +
> + if (!pin)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> + pin->name, NULL);
> + if (ret)
> + return ret;
> +
> + pin->present = true;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!name)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_pins)
> + return -EINVAL;
> +
> + if (!pi->pins[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_pin_info(ph, selector,
> + &pi->pins[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->pins[selector].name;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_name_get(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + const char **name)
> +{
> + switch (type) {
> + case PIN_TYPE:
> + return scmi_pinctrl_get_pin_name(ph, selector, name);
> + case GROUP_TYPE:
> + return scmi_pinctrl_get_group_name(ph, selector, name);
> + case FUNCTION_TYPE:
> + return scmi_pinctrl_get_function_name(ph, selector, name);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> + .count_get = scmi_pinctrl_count_get,
> + .name_get = scmi_pinctrl_name_get,
> + .group_pins_get = scmi_pinctrl_group_pins_get,
> + .function_groups_get = scmi_pinctrl_function_groups_get,
> + .mux_set = scmi_pinctrl_mux_set,
> + .settings_get = scmi_pinctrl_settings_get,
> + .settings_conf = scmi_pinctrl_settings_conf,
> + .pin_request = scmi_pinctrl_pin_request,
> + .pin_free = scmi_pinctrl_pin_free,
> +};
> +
> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + int ret;
> + u32 version;
> + struct scmi_pinctrl_info *pinfo;
> +
> + ret = ph->xops->version_get(ph, &version);
> + if (ret)
> + return ret;
> +
> + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> + if (!pinfo)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> + if (ret)
> + return ret;
..as a I was saying is nr_pins == 0 the scmi_pinctrl_attributes_get
could return -EINVAL here and bail out....not sure that a running setup
with zero pins has any values (even for testing...) BUT, as said above,
I wuld certainly add a dev_warn in scmi_pinctrl_attributes_get() when
nr_pins == 0
Thanks,
Cristian
^ permalink raw reply
* Re: [PATCH 10/10] iio: dac: support the ad9739a RF DAC
From: Jonathan Cameron @ 2024-03-28 15:51 UTC (permalink / raw)
To: Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, devicetree, Dragos Bogdan, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan
In-Reply-To: <20240328-iio-backend-axi-dac-v1-10-afc808b3fde3@analog.com>
On Thu, 28 Mar 2024 14:22:34 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> From: Nuno Sa <nuno.sa@analog.com>
>
> The AD9739A is a 14-bit, 2.5 GSPS high performance RF DACs that are capable
> of synthesizing wideband signals from dc up to 3 GHz.
DC perhaps
>
> A dual-port, source synchronous, LVDS interface simplifies the digital
> interface with existing FGPA/ASIC technology. On-chip controllers are used
> to manage external and internal clock domain variations over temperature to
> ensure reliable data transfer from the host to the DAC core.
>
> Co-developed-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Hi Nuno,
A few questions / comments inline but on the whole looking good to me.
Jonathan
> ---
> Documentation/ABI/testing/sysfs-bus-iio-ad9739a | 17 +
> MAINTAINERS | 1 +
> drivers/iio/dac/Kconfig | 16 +
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/ad9739a.c | 445 ++++++++++++++++++++++++
> 5 files changed, 480 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ad9739a b/Documentation/ABI/testing/sysfs-bus-iio-ad9739a
> new file mode 100644
> index 000000000000..8a8a5cd10386
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-ad9739a
> @@ -0,0 +1,17 @@
> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_operating_mode
> +KernelVersion: 6.9
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Dac operating mode. One of the following modes can be selected:
DAC operating mode. ...
> + * normal: This is DAC normal mode.
> + * mixed-mode: In this mode the output is effectively chopped at the
Spaces and tabs mixed...
> + DAC sample rate. This has the effect of reducing the
> + power of the fundamental signal while increasing the
> + power of the images centered around the DAC sample rate,
> + thus improving the output power of these images.
Any idea why it is called mixed mode? Name doesn't suggest to me what the Docs say
this does.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_operating_mode_available
> +KernelVersion: 6.9
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Available operating modes.
> M: Antoniu Miclaus <antoniu.miclaus@analog.com>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 7c0a8caa9a34..ee0d9798d8b4 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -131,6 +131,22 @@ config AD5624R_SPI
> Say yes here to build support for Analog Devices AD5624R, AD5644R and
> AD5664R converters (DAC). This driver uses the common SPI interface.
>
> +config AD9739A
> + tristate "Analog Devices AD9739A RF DAC spi driver"
> + depends on SPI
> + select REGMAP_SPI
> + select IIO_BACKEND
> + help
> + Say yes here to build support for Analog Devices AD9739A Digital-to
> + Analog Converter.
> +
> + The driver requires the assistance of the AXI DAC IP core to operate,
Maybe a depends on || COMPILE_TEST to increase build coverage (compared to
a hard depends on)
> + since SPI is used for configuration only, while data has to be
> + streamed into memory via DMA.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ad9739a.
> +
> diff --git a/drivers/iio/dac/ad9739a.c b/drivers/iio/dac/ad9739a.c
> new file mode 100644
> index 000000000000..46431fa345a5
> --- /dev/null
> +++ b/drivers/iio/dac/ad9739a.c
> +
> +enum {
> + AD9739A_NORMAL_MODE,
> + AD9739A_MIXED_MODE = 2,
Push these next to the relevant registers and more conventional defines.
Not seeing why the enum helps much here.
> +};
> +
> +static int ad9739a_oper_mode_get(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ad9739a_state *st = iio_priv(indio_dev);
> + u32 mode;
> + int ret;
> +
> + ret = regmap_read(st->regmap, AD9739A_REG_DEC_CNT, &mode);
> + if (ret)
> + return ret;
> +
> + mode = FIELD_GET(AD9739A_DAC_DEC, mode);
> + /* sanity check we get valid values from the HW */
> + if (mode != AD9739A_NORMAL_MODE && mode != AD9739A_MIXED_MODE)
> + return -EIO;
> + if (!mode)
> + return AD9739A_NORMAL_MODE;
> +
> + return AD9739A_MIXED_MODE - 1;
As below. I'd like to see a mapping function, or lookup table or similar
rather than handling this conversion in code.
> +}
> +
> +static int ad9739a_oper_mode_set(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, u32 mode)
> +{
> + struct ad9739a_state *st = iio_priv(indio_dev);
> +
> + if (mode == AD9739A_MIXED_MODE - 1)
> + mode = AD9739A_MIXED_MODE;
Why? Feels like a comment is needed. Or a more obvious conversion function.
> +
> + return regmap_update_bits(st->regmap, AD9739A_REG_DEC_CNT,
> + AD9739A_DAC_DEC, mode);
> +}
> +
> +static int ad9739a_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ad9739a_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = st->sample_rate;
> + *val2 = 0;
> + return IIO_VAL_INT_64;
Big numbers :)
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +
> +/*
> + * Recommended values (as per datasheet) for the dac clk common mode voltage
> + * and Mu controller. Look at table 29.
> + */
> +static const struct reg_sequence ad9739a_clk_mu_ctrl[] = {
> + /* DAC clk common mode voltage */
> + {AD9739A_REG_CROSS_CNT1, 0x0f},
{ AD9739A_REG_CROSS_CNT1, 0x0f },
etc is more readable in my opinion so is always my preference in IIO.
> + {AD9739A_REG_CROSS_CNT2, 0x0f},
> + /* Mu controller configuration */
> + {AD9739A_REG_PHS_DET, 0x30},
> + {AD9739A_REG_MU_DUTY, 0x80},
> + {AD9739A_REG_MU_CNT2, 0x44},
> + {AD9739A_REG_MU_CNT3, 0x6c},
> +};
> +
> +static int ad9739a_init(struct device *dev, const struct ad9739a_state *st)
> +{
> + unsigned int i = 0, lock, fsc;
> + u32 fsc_raw;
> + int ret;
> +
> + ret = regmap_multi_reg_write(st->regmap, ad9739a_clk_mu_ctrl,
> + ARRAY_SIZE(ad9739a_clk_mu_ctrl));
> + if (ret)
> + return ret;
> +
> + /*
> + * Try to get the MU lock. Repeat the below steps AD9739A_LOCK_N_TRIES
> + * (as specified by the datasheet) until we get the lock.
> + */
> + do {
> + ret = regmap_write(st->regmap, AD9739A_REG_MU_CNT4,
> + AD9739A_MU_CNT4_DEFAULT);
> + if (ret)
> + return ret;
> +
> + /* Enable the Mu controller search and track mode. */
MU for consistency
> + ret = regmap_set_bits(st->regmap, AD9739A_REG_MU_CNT1,
> + AD9739A_MU_EN_MASK);
> + if (ret)
> + return ret;
> +
> + /* Ensure the DLL loop is locked */
> + ret = regmap_read_poll_timeout(st->regmap, AD9739A_REG_MU_STAT1,
> + lock, lock & AD9739A_MU_LOCK_MASK,
> + 0, 1000);
if (ret < 0 && ret != -ETIMEOUT)
return ret;
i.e. deal with error codes that don't meant it timed out.
> + } while (ret && ++i < AD9739A_LOCK_N_TRIES);
> +
> + if (i == AD9739A_LOCK_N_TRIES)
> + return dev_err_probe(dev, ret, "Mu lock timeout\n");
> +
> + /* Receiver tracking and lock. Same deal as the Mu controller */
MU or Mu. Either fine but be consistent in comments. I have no idea what this is
so can't say which is better.
> + i = 0;
> + do {
> + ret = regmap_update_bits(st->regmap, AD9739A_REG_LVDS_REC_CNT4,
> + AD9739A_FINE_DEL_SKW_MASK,
> + FIELD_PREP(AD9739A_FINE_DEL_SKW_MASK, 2));
> + if (ret)
> + return ret;
> +
> + /* Disable the receiver and the loop. */
> + ret = regmap_write(st->regmap, AD9739A_REG_LVDS_REC_CNT1, 0);
> + if (ret)
> + return ret;
> +
> + /*
> + * Re-enable the loop so it falls out of lock and begins the
> + * search/track routine again.
> + */
> + ret = regmap_set_bits(st->regmap, AD9739A_REG_LVDS_REC_CNT1,
> + AD9739A_RCVR_LOOP_EN_MASK);
> + if (ret)
> + return ret;
> +
> + /* Ensure the DLL loop is locked */
> + ret = regmap_read_poll_timeout(st->regmap,
> + AD9739A_REG_LVDS_REC_STAT9, lock,
> + lock == AD9739A_RCVR_TRACK_AND_LOCK,
> + 0, 1000);
As above, consider other error codes than -ETIMEOUT;
> + } while (ret && ++i < AD9739A_LOCK_N_TRIES);
> +
> + if (i == AD9739A_LOCK_N_TRIES)
> + return dev_err_probe(dev, ret, "Receiver lock timeout\n");
> +
> + ret = device_property_read_u32(dev, "adi,full-scale-microamp", &fsc);
> + if (ret && ret == -EINVAL)
> + return 0;
> + if (ret)
> + return ret;
> + if (!in_range(fsc, AD9739A_FSC_MIN, AD9739A_FSC_RANGE))
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid full scale current(%u) [%u %u]\n",
> + fsc, AD9739A_FSC_MIN, AD9739A_FSC_MAX);
> + /*
> + * IOUTFS is given by
> + * Ioutfs = 0.0226 * FSC + 8.58
> + * and is given in mA. Hence we'll have to multiply by 10 * MILLI in
> + * order to get rid of the fractional.
> + */
> + fsc_raw = DIV_ROUND_CLOSEST(fsc * 10 - 85800, 226);
> +
> + ret = regmap_write(st->regmap, AD9739A_REG_FSC_1, fsc_raw & 0xff);
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(st->regmap, AD9739A_REG_FSC_1,
> + AD9739A_FSC_MSB, fsc_raw >> 8);
> +}
> +
> +static int ad9739a_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct ad9739a_state *st;
> + unsigned int id;
> + struct clk *clk;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> +
> + clk = devm_clk_get_enabled(dev, NULL);
> + if (IS_ERR(clk))
> + return dev_err_probe(dev, PTR_ERR(clk), "Could not get clkin\n");
> +
> + st->sample_rate = clk_get_rate(clk);
> + if (!in_range(st->sample_rate, AD9739A_MIN_DAC_CLK,
> + AD9739A_DAC_CLK_RANGE))
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid dac clk range(%lu) [%lu %lu]\n",
> + st->sample_rate, AD9739A_MIN_DAC_CLK,
> + AD9739A_MAX_DAC_CLK);
> +
> + st->regmap = devm_regmap_init_spi(spi, &ad9739a_regmap_config);
> + if (IS_ERR(st->regmap))
> + return PTR_ERR(st->regmap);
> +
> + ret = regmap_read(st->regmap, AD9739A_REG_ID, &id);
> + if (ret)
> + return ret;
> +
> + if (id != AD9739A_ID)
> + return dev_err_probe(dev, -ENODEV, "Unrecognized CHIP_ID 0x%X",
> + id);
Do we have to give up here? Could it be a compatible future part?
If so we should fallback on what firmware told us it was + perhaps a
dev_info() to say we don't recognise the ID register value.
> +
> + ret = ad9739a_reset(dev, st);
> + if (ret)
> + return ret;
> +
> + ret = ad9739a_init(dev, st);
> + if (ret)
> + return ret;
> +
> + st->back = devm_iio_backend_get(dev, NULL);
> + if (IS_ERR(st->back))
> + return PTR_ERR(st->back);
> +
> + ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_extend_chan_spec(indio_dev, st->back,
> + &ad9739a_channels[0]);
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_set_sampling_freq(st->back, 0, st->sample_rate);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_backend_enable(dev, st->back);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = "ad9739a";
> + indio_dev->info = &ad9739a_info;
> + indio_dev->channels = ad9739a_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ad9739a_channels);
> + indio_dev->setup_ops = &ad9739a_buffer_setup_ops;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
^ permalink raw reply
* Re: [PATCH 01/10] iio: buffer: add helper for setting direction
From: Jonathan Cameron @ 2024-03-28 15:54 UTC (permalink / raw)
To: Nuno Sá
Cc: Nuno Sa via B4 Relay, nuno.sa, linux-iio, devicetree,
Dragos Bogdan, Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Olivier Moysan
In-Reply-To: <d30c43f7f9d2db5b9c0e779d99f029da1a751636.camel@gmail.com>
On Thu, 28 Mar 2024 16:18:04 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Thu, 2024-03-28 at 14:36 +0000, Jonathan Cameron wrote:
> > On Thu, 28 Mar 2024 14:22:25 +0100
> > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >
> > > From: Nuno Sa <nuno.sa@analog.com>
> > >
> > > Simple helper for setting the buffer direction when it's allocated using
> > > iio_dmaengine_buffer_alloc().
> > >
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > I wonder if we should align with the approach for triggered-buffers with and _ext
> > form of the registration function that takes a direction. It seems odd to allocate
> > one then change the direction.
> >
>
> I agree it feels odd but I did not wanted to include buffer_impl.h in places that
> should not have it :)
>
> This patchseries adds the direction to devm_iio_dmaengine_buffer_setup(). Maybe what
> we need is to have a non devm variant iio_dmaengine_buffer_setup() and turn
> iio_dmaengine_buffer_alloc() static again. Maybe that would make things a bit more
> consistent. In fact looking closer into that file, I would get rid of:
>
> devm_iio_dmaengine_buffer_alloc() and __devm_iio_dmaengine_buffer_free()
>
> and have:
>
> devm_iio_dmaengine_buffer_setup() and iio_dmaengine_buffer_setup() that make use of
> iio_dmaengine_buffer_free() and iio_dmaengine_buffer_alloc().
>
> I think it would make more sense like the above. Thoughts?
Sounds reasonable to me
Jonathan
>
> - Nuno Sá
>
^ permalink raw reply
* Re: [PATCH net-next v6 15/17] net: pse-pd: Add PD692x0 PSE controller driver
From: Andrew Lunn @ 2024-03-28 15:57 UTC (permalink / raw)
To: Kory Maincent
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Luis Chamberlain, Russ Weight,
Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Oleksij Rempel, Mark Brown,
Frank Rowand, Heiner Kallweit, Russell King, Thomas Petazzoni,
netdev, linux-kernel, linux-doc, devicetree, Dent Project
In-Reply-To: <20240326-feature_poe-v6-15-c1011b6ea1cb@bootlin.com>
On Tue, Mar 26, 2024 at 03:04:52PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller.
> This driver only support i2c communication for now.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v6 16/17] dt-bindings: net: pse-pd: Add bindings for TPS23881 PSE controller
From: Andrew Lunn @ 2024-03-28 15:58 UTC (permalink / raw)
To: Kory Maincent
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet, Luis Chamberlain, Russ Weight,
Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Oleksij Rempel, Mark Brown,
Frank Rowand, Heiner Kallweit, Russell King, Thomas Petazzoni,
netdev, linux-kernel, linux-doc, devicetree, Dent Project
In-Reply-To: <20240326-feature_poe-v6-16-c1011b6ea1cb@bootlin.com>
On Tue, Mar 26, 2024 at 03:04:53PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Add the TPS23881 I2C Power Sourcing Equipment controller device tree
> bindings documentation.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 08/10] iio: backend: add new functionality
From: Jonathan Cameron @ 2024-03-28 15:59 UTC (permalink / raw)
To: Nuno Sá
Cc: Nuno Sa via B4 Relay, nuno.sa, linux-iio, devicetree,
Dragos Bogdan, Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Olivier Moysan
In-Reply-To: <cec2ac9c67aeae7c59434a86713f35461d171c04.camel@gmail.com>
On Thu, 28 Mar 2024 16:42:38 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Thu, 2024-03-28 at 15:16 +0000, Jonathan Cameron wrote:
> > On Thu, 28 Mar 2024 14:22:32 +0100
> > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >
> > > From: Nuno Sa <nuno.sa@analog.com>
> > >
> > > This adds the needed backend ops for supporting a backend inerfacing
> > > with an high speed dac. The new ops are:
> > >
> > > * data_source_set();
> > > * set_sampling_freq();
> > > * extend_chan_spec();
> > > * ext_info_set();
> > > * ext_info_get().
> > >
> > > Also to note the new helpers that are meant to be used by the backends
> > > + return 0;
> > > + /*
> > > + * !\NOTE: this will break as soon as we have multiple backends on one
> > > + * frontend and all of them extend channels. In that case, the core
> > > + * backend code has no way to get the correct backend given the
> > > + * iio device.
> > > + *
> > > + * One solution for this could be introducing a new backend
> > > + * dedicated callback in struct iio_info so we can callback into the
> > > + * frontend so it can give us the right backend given a chan_spec.
> > > + */
> >
> > Hmm. This is indeed messy. Could we associate it with the buffer as presuably
> > a front end with multiple backends is using multiple IIO buffers?
> >
>
> Hmm, the assumption of having multiple buffers seems plausible to me but considering
> the example we have in hands it would be cumbersome to get the backend. Considering
> iio_backend_ext_info_get(), how could we get the backend if it was associated to one
> of the IIO buffers? I think we would need more "intrusive" changes to make that work
> or do you have something in mind=
Nope. Just trying to get my head around the associations. I hadn't thought about
how to make that visible in the code. Probably a callabck anyway.
>
> > As you say a dance via the front end would work fine.
>
> I'm happy you're also open for a proper solution already. I mention this in the
> cover. My idea was something like (consider the iio_backend_ext_info_get()):
>
> if (!indio_dev->info->get_iio_backend())
> return -EOPNOTSUPP;
>
> back = indio_dev->info->get_iio_backend(indio_dev, chan_spec);
>
> It would be nice to have some "default/generic" implementation for cases where we
> only have one backend per frontend so that the frontend would not need to define the
> callback.
Agreed - either a default that means if the callback isn't provided we get the
single backend or if that proves fiddly at least a standard callback we can
use in all such cases.
>
> >
> >
> > > + iio_device_set_drvdata(indio_dev, back);
> > > +
> > > + /* Don't allow backends to get creative and force their own handlers */
> > > + for (ext_info = chan->ext_info; ext_info->name; ext_info++) {
> > > + if (ext_info->read != iio_backend_ext_info_get)
> > > + return -EINVAL;
> > > + if (ext_info->write != iio_backend_ext_info_set)
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_extend_chan_spec, IIO_BACKEND);
> >
> > > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> > > index a6d79381866e..09ff2f8f9fd8 100644
> > > --- a/include/linux/iio/backend.h
> > > +++ b/include/linux/iio/backend.h
> > > @@ -4,6 +4,7 @@
> > >
> > > #include <linux/types.h>
> > >
> > > +struct iio_chan_spec;
> > > struct fwnode_handle;
> > > struct iio_backend;
> > > struct device;
> > > @@ -15,6 +16,26 @@ enum iio_backend_data_type {
> > > IIO_BACKEND_DATA_TYPE_MAX
> > > };
> > >
> > > +enum iio_backend_data_source {
> > > + IIO_BACKEND_INTERNAL_CW,
> >
> > CW? Either expand out what ever that is in definition of add a comment
> > at least.
>
> Continuous wave :)
Spell that out.
>
> >
> > > + IIO_BACKEND_EXTERNAL,
> > What does external mean in this case?
>
> In this particular case comes from a DMA source (IP). I thought external to be more
> generic but if you prefer, I can do something like IIO_BACKEND_DMA?
So from another IP block? For that to be reasonably 'generic' we'd need a way
to known where it was coming from.
Now I remember advantage of reviewing on weekends - fewer replies during the reviews :)
Jonathan
^ permalink raw reply
* Re: [PATCH 17/23] dt-bindings: media: imx258: Rename to include vendor prefix
From: Luigi311 @ 2024-03-28 16:15 UTC (permalink / raw)
To: Conor Dooley, Kieran Bingham
Cc: Conor Dooley, git, linux-media, dave.stevenson, jacopo.mondi,
mchehab, robh, krzysztof.kozlowski+dt, conor+dt, shawnguo,
s.hauer, kernel, festevam, sakari.ailus, devicetree, imx,
linux-arm-kernel, linux-kernel
In-Reply-To: <20240328-prideful-unopposed-e29fcee74c29@wendy>
On 3/28/24 04:15, Conor Dooley wrote:
> On Thu, Mar 28, 2024 at 08:52:01AM +0000, Kieran Bingham wrote:
>> Quoting git@luigi311.com (2024-03-28 00:57:34)
>>> On 3/27/24 17:47, Conor Dooley wrote:
>>>> On Wed, Mar 27, 2024 at 05:17:03PM -0600, git@luigi311.com wrote:
>>>>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>>>>
>>>>> imx258.yaml doesn't include the vendor prefix of sony, so
>>>>> rename to add it.
>>>>> Update the id entry and MAINTAINERS to match.
>>>>>
>>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>>>
>>>> This is a v1 with my ack, something has gone awry here. It's also
>>>> missing your signoff. Did you pick up someone else's series?
>>>
>>> Yes, this is a continuation of Dave's work. I contacted him directly,
>>> and he mentioned that he is unable to submit a v2 any time soon and
>>> was open to someone else continuing it in his stead.
>
> Ah okay. Unfortunately I see so many binding patches pass by that I
> sometimes forget about what I already reviewed, and I did not
> remember this one at all.
No worries I'm not surprised since i see constant things submitted
to upstream and v1 was actually sent a year ago so there would be no
shot that i would remember it either.
>
>>> This is my first
>>> time submitting a patch via a mailing list, so I'm not sure if I'm
>>> missing something, but I only added my sign off for anything that
>>> actually included work from my side and not just bringing his patch
>>> forward to this patch series.
>
> Right. The rules are that you need to add it when you send someone's
> work, like chain of custody type of thing.
Ohh i see, ok ill go ahead and add my sign off to all the patches then
>
>> Your cover letter states v2, but the individual patches do not.
>>
>> Add the '-v2' (or, rather, next it will be '-v3') to git format-patch
>> when you save your series and it will add the version to each patch. You
>> can also add '-s' to that command I believe to add your SoB to each
>> patch.
>
> or a rebase will do it with --signoff:
> https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---signoff
Perfect thanks for the information you two! Ill be sure to use those
for the next revision.
>
> Cheers,
> Conor.
^ permalink raw reply
* Re: [PATCH] dt-bindings: arm: qcom: Add Samsung Galaxy Z Fold5
From: Krzysztof Kozlowski @ 2024-03-28 16:16 UTC (permalink / raw)
To: serdeliuk, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20240328-dt-bindings-arm-qcom-add-support-for-samsung-galaxy-zfold5-v1-1-cb612e3ade18@yahoo.com>
On 28/03/2024 15:31, Alexandru Marc Serdeliuc via B4 Relay wrote:
> From: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
>
> This documents Samsung Galaxy Z Fold5 (samsung,q5q)
> which is a foldable phone by Samsung based on the sm8550 SoC.
>
> Signed-off-by: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
> ---
> This documents Samsung Galaxy Z Fold5 (samsung,q5q)
> which is a foldable phone by Samsung based on the sm8550 SoC.
Please always send DTS and its binding together.
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.
https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
Best regards,
Krzysztof
^ 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