* Re: [PATCH 1/4] dt-bindings: display: simple: Document support for Innolux G121XCE-L01
From: Conor Dooley @ 2024-03-28 17:48 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, Conor Dooley, Daniel Vetter, David Airlie,
Jessica Zhang, Krzysztof Kozlowski, Maarten Lankhorst,
Maxime Ripard, Neil Armstrong, Rob Herring, Sam Ravnborg,
Thierry Reding, Thomas Zimmermann, devicetree
In-Reply-To: <20240328102746.17868-1-marex@denx.de>
[-- Attachment #1: Type: text/plain, Size: 2609 bytes --]
On Thu, Mar 28, 2024 at 11:27:35AM +0100, Marek Vasut wrote:
> Document support for Innolux CheMei 12" G121XCE-L01 XGA LVDS display.
>
> G121XCE-L01 is a Color Active Matrix Liquid Crystal Display composed of
> a TFT LCD panel, a driver circuit, and LED backlight system. The screen
> format is intended to support the 4:3, 1024(H) x 768(V) screen and either
> 262k/16.7M colors (RGB 6-bits or 8-bits) with LED backlight driver circuit.
> All input signals are LVDS interface compatible.
>
> Documentation [1] and [2] indicate that G121X1-L03 and G121XCE-L01 are
> effectively identical panels, use the former as RGB 6-bits variant and
> document the later as RGB 8-bits variant.
>
> [1] https://www.distec.de/fileadmin/pdf/produkte/TFT-Displays/Innolux/G121X1-L03_Datasheet.pdf
> [2] https://www.distec.de/fileadmin/pdf/produkte/TFT-Displays/Innolux/G121XCE-L01_Datasheet.pdf
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> ---
> .../devicetree/bindings/display/panel/panel-simple.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> index e0f6aa9a025c4..931d98836e121 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> @@ -190,6 +190,8 @@ properties:
> - innolux,g121i1-l01
> # Innolux Corporation 12.1" G121X1-L03 XGA (1024x768) TFT LCD panel
> - innolux,g121x1-l03
> + # Innolux Corporation 12.1" G121XCE-L01 XGA (1024x768) TFT LCD panel
> + - innolux,g121xce-l01
> # Innolux Corporation 11.6" WXGA (1366x768) TFT LCD panel
> - innolux,n116bca-ea1
> # Innolux Corporation 11.6" WXGA (1366x768) TFT LCD panel
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi
From: Marc Gonzalez @ 2024-03-28 17:39 UTC (permalink / raw)
To: Kalle Valo, Jeff Johnson, ath10k
Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio,
Jami Kettunen, Jeffrey Hugo
In-Reply-To: <fd26ce4a-a9f3-4ada-8d46-ed36fb2456ca@freebox.fr>
The ath10k driver waits for an "MSA_READY" indicator
to complete initialization. If the indicator is not
received, then the device remains unusable.
cf. ath10k_qmi_driver_event_work()
Several msm8998-based devices are affected by this issue.
Oddly, it seems safe to NOT wait for the indicator, and
proceed immediately when QMI_EVENT_SERVER_ARRIVE.
Jeff Johnson wrote:
The feedback I received was "it might be ok to change all ath10k qmi
to skip waiting for msa_ready", and it was pointed out that ath11k
(and ath12k) do not wait for it.
However with so many deployed devices, "might be ok" isn't a strong
argument for changing the default behavior.
cf. also
https://wiki.postmarketos.org/wiki/Qualcomm_Snapdragon_835_(MSM8998)#WLAN
Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
arch/arm64/boot/dts/qcom/msm8998.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 026b6b97785b5..681a80f4dc9db 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -3185,6 +3185,7 @@ wifi: wifi@18800000 {
iommus = <&anoc2_smmu 0x1900>,
<&anoc2_smmu 0x1901>;
qcom,snoc-host-cap-8bit-quirk;
+ qcom,no-msa-ready-indicator;
};
};
};
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/3] wifi: ath10k: fake missing MSA_READY indicator
From: Marc Gonzalez @ 2024-03-28 17:38 UTC (permalink / raw)
To: Kalle Valo, Jeff Johnson, ath10k
Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio,
Jami Kettunen, Jeffrey Hugo
In-Reply-To: <fd26ce4a-a9f3-4ada-8d46-ed36fb2456ca@freebox.fr>
The ath10k driver waits for an "MSA_READY" indicator
to complete initialization. If the indicator is not
received, then the device remains unusable.
Several msm8998-based devices are affected by this issue.
Oddly, it seems safe to NOT wait for the indicator, and
proceed immediately when QMI_EVENT_SERVER_ARRIVE.
fw_version 0x100204b2
fw_build_timestamp 2019-09-04 03:01
fw_build_id QC_IMAGE_VERSION_STRING=WLAN.HL.1.0-01202-QCAHLSWMTPLZ-1.221523.2
Jeff Johnson wrote:
The feedback I received was "it might be ok to change all ath10k qmi
to skip waiting for msa_ready", and it was pointed out that ath11k
(and ath12k) do not wait for it.
However with so many deployed devices, "might be ok" isn't a strong
argument for changing the default behavior.
Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr>
Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
drivers/net/wireless/ath/ath10k/qmi.c | 7 +++++++
drivers/net/wireless/ath/ath10k/qmi.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c
index 38e939f572a9e..50e28fa37e430 100644
--- a/drivers/net/wireless/ath/ath10k/qmi.c
+++ b/drivers/net/wireless/ath/ath10k/qmi.c
@@ -1040,6 +1040,10 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work)
switch (event->type) {
case ATH10K_QMI_EVENT_SERVER_ARRIVE:
ath10k_qmi_event_server_arrive(qmi);
+ if (qmi->fake_msa_ready_indicator) {
+ ath10k_dbg(ar, ATH10K_DBG_QMI, "qmi HACK fake msa_ready indicator");
+ ath10k_qmi_event_msa_ready(qmi);
+ }
break;
case ATH10K_QMI_EVENT_SERVER_EXIT:
ath10k_qmi_event_server_exit(qmi);
@@ -1077,6 +1081,9 @@ int ath10k_qmi_init(struct ath10k *ar, u32 msa_size)
if (of_property_read_bool(dev->of_node, "qcom,msa-fixed-perm"))
qmi->msa_fixed_perm = true;
+ if (of_property_read_bool(dev->of_node, "qcom,no-msa-ready-indicator"))
+ qmi->fake_msa_ready_indicator = true;
+
ret = qmi_handle_init(&qmi->qmi_hdl,
WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
&ath10k_qmi_ops, qmi_msg_handler);
diff --git a/drivers/net/wireless/ath/ath10k/qmi.h b/drivers/net/wireless/ath/ath10k/qmi.h
index 89464239fe96a..c68526aad8946 100644
--- a/drivers/net/wireless/ath/ath10k/qmi.h
+++ b/drivers/net/wireless/ath/ath10k/qmi.h
@@ -107,6 +107,7 @@ struct ath10k_qmi {
char fw_build_timestamp[MAX_TIMESTAMP_LEN + 1];
struct ath10k_qmi_cal_data cal_data[MAX_NUM_CAL_V01];
bool msa_fixed_perm;
+ bool fake_msa_ready_indicator;
enum ath10k_qmi_state state;
};
--
2.34.1
^ permalink raw reply related
* [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
From: Marc Gonzalez @ 2024-03-28 17:36 UTC (permalink / raw)
To: Kalle Valo, Jeff Johnson, ath10k
Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio,
Jami Kettunen, Jeffrey Hugo
In-Reply-To: <fd26ce4a-a9f3-4ada-8d46-ed36fb2456ca@freebox.fr>
The ath10k driver waits for an "MSA_READY" indicator
to complete initialization. If the indicator is not
received, then the device remains unusable.
cf. ath10k_qmi_driver_event_work()
Several msm8998-based devices are affected by this issue.
Oddly, it seems safe to NOT wait for the indicator, and
proceed immediately when QMI_EVENT_SERVER_ARRIVE.
Jeff Johnson wrote:
The feedback I received was "it might be ok to change all ath10k qmi
to skip waiting for msa_ready", and it was pointed out that ath11k
(and ath12k) do not wait for it.
However with so many deployed devices, "might be ok" isn't a strong
argument for changing the default behavior.
Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr>
Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml
index 9b3ef4bc37325..1680604cd1b7e 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml
@@ -122,6 +122,13 @@ properties:
Whether to skip executing an SCM call that reassigns the memory
region ownership.
+ qcom,no-msa-ready-indicator:
+ type: boolean
+ description:
+ The driver waits for an MSA_READY indicator to complete init.
+ If the indicator is not received, interface remains unusable.
+ Pretending we received the indicator works around the issue.
+
qcom,smem-states:
$ref: /schemas/types.yaml#/definitions/phandle-array
description: State bits used by the AP to signal the WLAN Q6.
--
2.34.1
^ permalink raw reply related
* [PATCH v2 0/3] Work around missing MSA_READY indicator for msm8998 devices
From: Marc Gonzalez @ 2024-03-28 17:24 UTC (permalink / raw)
To: Kalle Valo, Jeff Johnson, ath10k
Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio,
Jami Kettunen, Jeffrey Hugo
Work around missing MSA_READY indicator in ath10k driver
(apply work-around for all msm8998 devices)
Marc Gonzalez (3):
dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
wifi: ath10k: fake missing MSA_READY indicator
arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi
Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml | 8 ++++++++
arch/arm64/boot/dts/qcom/msm8998.dtsi | 1 +
drivers/net/wireless/ath/ath10k/qmi.c | 7 +++++++
drivers/net/wireless/ath/ath10k/qmi.h | 1 +
4 files changed, 17 insertions(+)
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
From: Marc Gonzalez @ 2024-03-28 17:09 UTC (permalink / raw)
To: Kalle Valo, Jeff Johnson, Dmitry Baryshkov
Cc: ath10k, wireless, DT, Pierre-Hugues Husson, Arnaud Vrac,
Jami Kettunen, Jeffrey Hugo, Bjorn Andersson, Konrad Dybcio
In-Reply-To: <4b7bf7cc-ede6-4c2a-983c-da4c8c09c5d1@quicinc.com>
On 26/03/2024 21:21, Jeff Johnson wrote:
> On 3/26/2024 10:51 AM, Dmitry Baryshkov wrote:
>
>> On Tue, 26 Mar 2024 at 19:45, Marc Gonzalez wrote:
>>>
>>> [ It has been pointed out to me that the previous message was unclear. ]
>>> [ Below is my 2nd attempt at a clearer message. ]
>>>
>>> Problem: firmware-5.bin has not been parsed yet when we have to handle
>>> the ATH10K_QMI_EVENT_SERVER_ARRIVE case, so we can't rely on feature bits
>>> to work around the lack of MSA_READY indicator.
>>
>> Then, I'd say, we have to resort to the DT property, unless Kalle or
>> Jeff have other proposals.
>
> Another option is to follow the downstream driver model and only expect this
> based upon static configuration within the driver.
>
> Downstream driver has:
> if (priv->device_id == ADRASTEA_DEVICE_ID) {
> ret = wlfw_msa_mem_info_send_sync_msg(priv);
> ret = wlfw_msa_ready_send_sync_msg(priv);
> }
>
> https://git.codelinaro.org/clo/la/platform/vendor/qcom-opensource/wlan/platform/-/blob/wlan-platform.lnx.1.0.r4-rel/icnss2/main.c?ref_type=heads#L968
>
> The downstream MSA logic (including some other code that populates MSA-related
> fields in the QMI messages) is only invoked for ADRASTEA_DEVICE_ID.
>
> We could introduce a new hw_params parameter to have the same semantics.
>
> But I'm OK with the DT option as well.
>
> Kalle?
I'll send a v2 series with the DT option + setting the quirk flag for
all msm8998 devices. I'll include msm8998 board maintainers for feedback.
Regards
^ permalink raw reply
* Re: [PATCH v4 0/6] iio: temperature: ltc2983: small improvements
From: Nuno Sá @ 2024-03-28 17:07 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sa via B4 Relay
Cc: nuno.sa, linux-kernel, linux-iio, devicetree, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana, Krzysztof Kozlowski
In-Reply-To: <20240328165650.1d8d4216@jic23-huawei>
On Thu, 2024-03-28 at 16:56 +0000, Jonathan Cameron wrote:
> On Thu, 28 Mar 2024 17:22:00 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> > The v4 introduces an new dev_errp_probe() helper to deal with cases
> > where we want to return error pointers. The refactor in the IIO ltc2983
> > is an heavy user of the pattern and was the main motivation for this.
> >
> > Also added two new patches so we have three users of the new
> > dev_errp_probe() helper.
>
> Probably better to do this as 2 series. The other ltc2983 changes in one series
> and one with a cover letter title that will get noticed by
> those who care about dev_printk helpers.
>
That makes sense, yes.
- Nuno Sá
> From a quick look the content of the patches is fine.
>
> Jonathan
>
> >
> > ---
> > Changes in v4:
> > - Link to v3:
> > https://lore.kernel.org/r/20240301-ltc2983-misc-improv-v3-0-c09516ac0efc@analog.com
> > - Patch 1
> > * New patch
> > - Patch 2
> > * Use dev_errp_probe() instead of local variant
> > - Patch 5
> > * New patch
> > - Patch 6
> > * New patch
> >
> > ---
> > Nuno Sa (6):
> > printk: add new dev_errp_probe() helper
> > iio: temperature: ltc2983: convert to dev_err_probe()
> > dt-bindings: iio: temperature: ltc2983: document power supply
> > iio: temperature: ltc2983: support vdd regulator
> > iio: backend: make use dev_errp_probe()
> > iio: common: scmi_iio: convert to dev_err_probe()
> >
> > .../bindings/iio/temperature/adi,ltc2983.yaml | 4 +
> > drivers/iio/common/scmi_sensors/scmi_iio.c | 45 ++--
> > drivers/iio/industrialio-backend.c | 8 +-
> > drivers/iio/temperature/ltc2983.c | 260 ++++++++++-----------
> > include/linux/dev_printk.h | 5 +
> > 5 files changed, 151 insertions(+), 171 deletions(-)
> > ---
> > base-commit: 27eea4778db8268cd6dc80a5b853c599bd3099f1
> > change-id: 20240227-ltc2983-misc-improv-d9c4a3819b1f
> > --
> >
> > Thanks!
> > - Nuno Sá
> >
> >
>
^ permalink raw reply
* Re: [PATCH 18/23] dt-bindings: media: imx258: Add alternate compatible strings
From: Krzysztof Kozlowski @ 2024-03-28 17:06 UTC (permalink / raw)
To: Luigi311, git, linux-media
Cc: 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: <30d886be-cac8-400a-9b2f-dd2ce64b34d8@luigi311.com>
On 28/03/2024 18:05, Luigi311 wrote:
>
> Looks like it no longer complains when i run
> make dt_binding_check DT_SCHEMA_FILES=media/i2c/sony,imx258
>
> with the following
>
> properties:
> compatible:
> enum:
> - sony,imx258
> - sony,imx258-pdaf
>
> If that looks good I can go ahead and include that in the v3
>
Looks good, thanks.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 18/23] dt-bindings: media: imx258: Add alternate compatible strings
From: Luigi311 @ 2024-03-28 17:05 UTC (permalink / raw)
To: Krzysztof Kozlowski, git, linux-media
Cc: 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: <586bdcc9-793d-4cbe-9544-9012a665288e@linaro.org>
On 3/28/24 01:47, Krzysztof Kozlowski wrote:
> On 28/03/2024 00:17, git@luigi311.com wrote:
>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>> There are a number of variants of the imx258 modules that can not
>> be differentiated at runtime, so add compatible strings for them.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Signed-off-by: Luigi311 <git@luigi311.com>
>> ---
>> .../devicetree/bindings/media/i2c/sony,imx258.yaml | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
>> index bee61a443b23..c7856de15ba3 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
>> @@ -14,10 +14,14 @@ description: |-
>> type stacked image sensor with a square pixel array of size 4208 x 3120. It
>> is programmable through I2C interface. Image data is sent through MIPI
>> CSI-2.
>> + There are a number of variants of the sensor which cannot be detected at
>> + runtime, so multiple compatible strings are required to differentiate these.
>>
>> properties:
>> compatible:
>> - const: sony,imx258
>> + - enum:
>> + - sony,imx258
>
> Two people working on patch but no one tested it before sending. Do not
> send untested code.
>
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
Hello, looks like I messed this up during my v2 (sorry missed the v in
my format patch) when I took this off Dave's hands. This is all new to
me so thank you for the command used to check, I was only compiling
the kernel and testing that so I didn't realize this needed separate
testing.
Looks like it no longer complains when i run
make dt_binding_check DT_SCHEMA_FILES=media/i2c/sony,imx258
with the following
properties:
compatible:
enum:
- sony,imx258
- sony,imx258-pdaf
If that looks good I can go ahead and include that in the v3
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line
From: Alban Browaeys @ 2024-03-28 17:00 UTC (permalink / raw)
To: Conor Dooley, dev
Cc: Vinod Koul, Kishon Vijay Abraham I, Heiko Stuebner, Chris Ruehl,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Christopher Obbard, Doug Anderson, Brian Norris, Jensen Huang,
linux-phy, linux-arm-kernel, linux-rockchip, linux-kernel,
devicetree
In-Reply-To: <20240326-tactical-onlooker-3df8d2352dc2@spud>
Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> Relay wrote:
> > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > - if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > strobe-pulldown"))
> > - rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_ENABLE;
> > + if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > strobe-pulldown"))
> > + rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_DISABLE;
>
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was
> required
> to enable it. With this change the absence of a property means that
> it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break
> and is
> clearly not backwards compatible, that's a NAK unless it is
> demonstrable
> that noone actually wants to disable it at all.
But the patch that introduced the new default to disable the pulldown
explicitely introduced a regression for at least 4 boards.
It took time to sort out that the default to disable pulldown was the
culprit but still.
Will we carry this new behavor that breaks the default design for
rk3399 because since the regression was introduced new board definition
might have expceted this new behavior.
Could the best option be to revert to énot set a default enable/disable
pulldown" (as before the commit that introduced the regression) and
allow one to force the pulldown via the enable/disable pulldown
property?
I mean the commit that introduced a default value for the pulldown did
not seem to be about fixing anything. But it broke a lot. ANd it was
really really hard to find the description of this commit to understand
that one had to enable pulldown to restore hs400.
In more than 3 years, only one board maintainer noticed that this
property was required to get back HS400 and thanks to a user telling
me that this board was working I found from this board that this
property was "missing" from most board definitions (while it was not
required before).
I am all for not breaking ABI. But what about not reverting a patch
that already broke ABI because this patch introduced a new ABI that we
don't want to break?
I mean shouldn't a new commit with a new ABI that regressed the kernel
be reverted?
Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
pulldown for strobe line in dts" does not necessarily mean changing the
default to the opposite value but could also be reverting to not
setting a default.
Though I don't know if there are pros to setting a default.
> If this patch fixes a problem on a board that you have, I would
> suggest
> that you add the property to enable it, as the binding tells you to.
>
> Thanks,
> Conor.
Regards,
Alban
^ permalink raw reply
* Re: [PATCH v4 0/6] iio: temperature: ltc2983: small improvements
From: Jonathan Cameron @ 2024-03-28 16:56 UTC (permalink / raw)
To: Nuno Sa via B4 Relay
Cc: nuno.sa, linux-kernel, linux-iio, devicetree, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana, Krzysztof Kozlowski
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
On Thu, 28 Mar 2024 17:22:00 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> The v4 introduces an new dev_errp_probe() helper to deal with cases
> where we want to return error pointers. The refactor in the IIO ltc2983
> is an heavy user of the pattern and was the main motivation for this.
>
> Also added two new patches so we have three users of the new
> dev_errp_probe() helper.
Probably better to do this as 2 series. The other ltc2983 changes in one series
and one with a cover letter title that will get noticed by
those who care about dev_printk helpers.
From a quick look the content of the patches is fine.
Jonathan
>
> ---
> Changes in v4:
> - Link to v3: https://lore.kernel.org/r/20240301-ltc2983-misc-improv-v3-0-c09516ac0efc@analog.com
> - Patch 1
> * New patch
> - Patch 2
> * Use dev_errp_probe() instead of local variant
> - Patch 5
> * New patch
> - Patch 6
> * New patch
>
> ---
> Nuno Sa (6):
> printk: add new dev_errp_probe() helper
> iio: temperature: ltc2983: convert to dev_err_probe()
> dt-bindings: iio: temperature: ltc2983: document power supply
> iio: temperature: ltc2983: support vdd regulator
> iio: backend: make use dev_errp_probe()
> iio: common: scmi_iio: convert to dev_err_probe()
>
> .../bindings/iio/temperature/adi,ltc2983.yaml | 4 +
> drivers/iio/common/scmi_sensors/scmi_iio.c | 45 ++--
> drivers/iio/industrialio-backend.c | 8 +-
> drivers/iio/temperature/ltc2983.c | 260 ++++++++++-----------
> include/linux/dev_printk.h | 5 +
> 5 files changed, 151 insertions(+), 171 deletions(-)
> ---
> base-commit: 27eea4778db8268cd6dc80a5b853c599bd3099f1
> change-id: 20240227-ltc2983-misc-improv-d9c4a3819b1f
> --
>
> Thanks!
> - Nuno Sá
>
>
^ permalink raw reply
* Re: [PATCH 08/10] iio: backend: add new functionality
From: Nuno Sá @ 2024-03-28 16:54 UTC (permalink / raw)
To: Jonathan Cameron
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: <20240328155929.20848a6a@jic23-huawei>
On Thu, 2024-03-28 at 15:59 +0000, Jonathan Cameron wrote:
> 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.
>
I'll have to think a bit about it. We may need some association/link between iio_dev
and iio_backend in order to "if the callback isn't provided we get the single
backend". The easiest that comes to my mind without much thinking would be to use
iio_device_set_drvdata()/iio_device_get_drvdata() in case the frontend does not
provide a callback. This would already force callers to assign the indio_dev->info
pointer before this call. Not that nice but acceptable if properly documented I
guess.
Anyways, I'll see if I can think of something better...
> >
> > >
> > >
> > > > + 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.
>
Yeps, in this case comes from the IIO DMA buffer which in HW means a DMA IP core...
> Now I remember advantage of reviewing on weekends - fewer replies during the
> reviews :)
>
:)
- Nuno Sá
^ permalink raw reply
* Re: [PATCH 10/10] iio: dac: support the ad9739a RF DAC
From: Jonathan Cameron @ 2024-03-28 16:52 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: <62e07212e1eebf8bbc6a7f9ee27f670a6d79c57e.camel@gmail.com>
> > > +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.
> >
>
> To match what we want to write in the register... With just two values it's too
> simple that opt not to have any helper function or table. Would you be fine with a
> comment?
yes
>
> > > +
> > > + 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 :)
>
> My setup is using 2.5Ghz which is big enough to overflow INT but would work on UINT.
I like big numbers so it's fine doing this. Just unusual to force
val2 to 0 so it made me look closer and appreciate just how big these were getting ;)
> > > + 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.
> >
>
> I typically prefer to really give up in these cases but no strong opinion... Can turn
> this into a dev_warn()...
DT maintainers generally advise carrying on and trusting the firmware.
I used to agree with you that paranoia was good but I can see there point
and we do have cases where this happened in real parts.
Jonathan
>
> - Nuno Sá
>
^ permalink raw reply
* Re: [PATCH v6 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Cristian Marussi @ 2024-03-28 16:48 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-0-a895243257c0@nxp.com>
On Sat, Mar 23, 2024 at 08:15:13PM +0800, Peng Fan (OSS) wrote:
> This patchset is a rework from Oleksii's RFC v5 patchset
> https://lore.kernel.org/all/cover.1698353854.git.oleksii_moisieiev@epam.com/
>
Hi,
beside a few more remarks on the series, I tested this again on my setup
with the latest v3.2 changes and it works fine.
Thanks,
Cristian
^ permalink raw reply
* Re: [PATCH v6 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver
From: Cristian Marussi @ 2024-03-28 16:46 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-4-a895243257c0@nxp.com>
On Sat, Mar 23, 2024 at 08:15:17PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCMI platform firmware, which does the changes in HW.
>
Hi,
> 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>
> ---
> MAINTAINERS | 1 +
> drivers/firmware/arm_scmi/pinctrl.c | 1 +
> drivers/pinctrl/Kconfig | 11 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi.c | 564 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 578 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b511a55101c..d8270ac6651a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21457,6 +21457,7 @@ F: drivers/cpufreq/sc[mp]i-cpufreq.c
> F: drivers/firmware/arm_scmi/
> F: drivers/firmware/arm_scpi.c
> F: drivers/hwmon/scmi-hwmon.c
> +F: drivers/pinctrl/pinctrl-scmi.c
> F: drivers/pmdomain/arm/
> F: drivers/powercap/arm_scmi_powercap.c
> F: drivers/regulator/scmi-regulator.c
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> index 87d9b89cab13..0ecefe855432 100644
> --- a/drivers/firmware/arm_scmi/pinctrl.c
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -465,6 +465,7 @@ scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
>
> tx = t->tx.buf;
> tx->identifier = cpu_to_le32(selector);
> + tx->function_id = cpu_to_le32(0xFFFFFFFF);
As already said....does not belong to this patch
> attributes = FIELD_PREP(GENMASK(1, 0), type) |
> FIELD_PREP(GENMASK(9, 2), chunk);
> tx->attributes = cpu_to_le32(attributes);
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index d45657aa986a..4e6f65cf0e76 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -450,6 +450,17 @@ config PINCTRL_ROCKCHIP
> help
> This support pinctrl and GPIO driver for Rockchip SoCs.
>
> +config PINCTRL_SCMI
> + tristate "Pinctrl driver using SCMI protocol interface"
> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> + select PINMUX
> + select GENERIC_PINCONF
> + help
> + This driver provides support for pinctrl which is controlled
> + by firmware that implements the SCMI interface.
> + It uses SCMI Message Protocol to interact with the
> + firmware providing all the pinctrl controls.
> +
[snip]
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param config_type;
> + enum scmi_pinctrl_conf_type type;
> + u32 config_value;
> +
> + if (!config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> + ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> +
> + ret = pinctrl_ops->settings_get(pmx->ph, group, GROUP_TYPE, type,
> + &config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
> +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
> + .is_generic = true,
> + .pin_config_get = pinctrl_scmi_pinconf_get,
> + .pin_config_set = pinctrl_scmi_pinconf_set,
> + .pin_config_group_set = pinctrl_scmi_pinconf_group_set,
> + .pin_config_group_get = pinctrl_scmi_pinconf_group_get,
> + .pin_config_config_dbg_show = pinconf_generic_dump_config,
> +};
> +
> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> + struct pinctrl_desc *desc)
> +{
> + struct pinctrl_pin_desc *pins;
> + unsigned int npins;
> + int ret, i;
> +
> + npins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
> + pins = devm_kmalloc_array(pmx->dev, npins, sizeof(*pins), GFP_KERNEL);
> + if (!pins)
> + return -ENOMEM;
> +
This is fine only if npins != 0, because on zero npins
devm_kmalloc_array will return ZERO_SIZE_PTR which is NOT-NULL so I
would add a check for !npins and bail out..or use ZERO_OR_NULL_PTR()
to check the return value...if from the previous pinctrl patch you
had decided to bail out at the protocol layer when (nr_pins == 0) and so
you will never get here, please add a comment above that npins cannot be
zero...
> + for (i = 0; i < npins; i++) {
> + pins[i].number = i;
> + ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE, &pins[i].name);
> + if (ret)
> + return dev_err_probe(pmx->dev, ret,
> + "Can't get name for pin %d", i);
> + }
> +
> + desc->npins = npins;
> + desc->pins = pins;
> + dev_dbg(pmx->dev, "got pins %d", npins);
> +
> + return 0;
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static int scmi_pinctrl_probe(struct scmi_device *sdev)
> +{
> + int ret;
> + struct device *dev = &sdev->dev;
> + struct scmi_pinctrl *pmx;
> + const struct scmi_handle *handle;
> + struct scmi_protocol_handle *ph;
> +
> + if (!sdev || !sdev->handle)
if (!sdev->handle) is enough...
> + return -EINVAL;
> +
> + handle = sdev->handle;
> +
> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
> + &ph);
> + if (IS_ERR(pinctrl_ops))
> + return PTR_ERR(pinctrl_ops);
> +
> + pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL);
> + if (!pmx)
> + return -ENOMEM;
> +
> + pmx->ph = ph;
> +
> + pmx->dev = dev;
> + pmx->pctl_desc.name = DRV_NAME;
> + pmx->pctl_desc.owner = THIS_MODULE;
> + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
> + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
> + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
> +
> + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc);
> + if (ret)
> + return ret;
> +
> + ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx,
> + &pmx->pctldev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
> +
> + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
> + pmx->functions = devm_kcalloc(dev, pmx->nr_functions,
> + sizeof(*pmx->functions),
> + GFP_KERNEL);
> + if (!pmx->functions)
> + return -ENOMEM;
> +
> + return pinctrl_enable(pmx->pctldev);
> +}
> +
> +static struct scmi_driver scmi_pinctrl_driver = {
> + .name = DRV_NAME,
> + .probe = scmi_pinctrl_probe,
> + .id_table = scmi_id_table,
> +};
> +module_scmi_driver(scmi_pinctrl_driver);
> +
> +MODULE_AUTHOR("Oleksii Moisieiev <oleksii_moisieiev@epam.com>");
> +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>");
> +MODULE_DESCRIPTION("ARM SCMI pin controller driver");
> +MODULE_LICENSE("GPL");
>
Thanks,
Cristian
^ permalink raw reply
* Re: [PATCH 09/10] iio: dac: add support for AXI DAC IP core
From: Nuno Sá @ 2024-03-28 16:43 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: <20240328153542.7ddb897c@jic23-huawei>
On Thu, 2024-03-28 at 15:35 +0000, Jonathan Cameron wrote:
> 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.
>
Ok, will see what of the alternatives looks better.
>
>
> > + 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.
>
ack..
> > + 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?
Has no meaning as it's not used with DDS enabled...
- Nuno Sá
>
^ permalink raw reply
* Re: [PATCH 10/10] iio: dac: support the ad9739a RF DAC
From: Nuno Sá @ 2024-03-28 16:37 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: <20240328155126.2575d754@jic23-huawei>
On Thu, 2024-03-28 at 15:51 +0000, Jonathan Cameron wrote:
> 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.
Nope, just respecting the datasheet names for the modes. But I may give it another
read. Likely there was a reason for that naming :)
>
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_operating_mode_ava
> > ilable
> > +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)
>
Can do that...
> > + 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.
Alright..
>
> > +};
> > +
> > +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.
>
To match what we want to write in the register... With just two values it's too
simple that opt not to have any helper function or table. Would you be fine with a
comment?
> > +
> > + 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 :)
My setup is using 2.5Ghz which is big enough to overflow INT but would work on UINT.
>
> > + 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
ack
>
> > + 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.
>
Oh yes, that makes sense.
> > + } 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.
>
I typically prefer to really give up in these cases but no strong opinion... Can turn
this into a dev_warn()...
- Nuno Sá
^ permalink raw reply
* Re: [PATCH 2/2] mailbox: arm_mhuv3: Add driver
From: kernel test robot @ 2024-03-28 16:37 UTC (permalink / raw)
To: Cristian Marussi, linux-kernel, linux-arm-kernel, devicetree
Cc: oe-kbuild-all, sudeep.holla, cristian.marussi, jassisinghbrar,
robh+dt, krzysztof.kozlowski+dt, conor+dt
In-Reply-To: <20240325092808.117510-3-cristian.marussi@arm.com>
Hi Cristian,
kernel test robot noticed the following build warnings:
[auto build test WARNING on soc/for-next]
[also build test WARNING on robh/for-next linus/master v6.9-rc1 next-20240328]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cristian-Marussi/dt-bindings-mailbox-arm-mhuv3-Add-bindings/20240326-020048
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link: https://lore.kernel.org/r/20240325092808.117510-3-cristian.marussi%40arm.com
patch subject: [PATCH 2/2] mailbox: arm_mhuv3: Add driver
config: powerpc-randconfig-r131-20240328 (https://download.01.org/0day-ci/archive/20240329/202403290015.tCLXudqC-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 23de3862dce582ce91c1aa914467d982cb1a73b4)
reproduce: (https://download.01.org/0day-ci/archive/20240329/202403290015.tCLXudqC-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403290015.tCLXudqC-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/mailbox/arm_mhuv3.c:565:46: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected struct pdbcw_page *dbcw @@ got struct pdbcw_page [noderef] __iomem * @@
drivers/mailbox/arm_mhuv3.c:565:46: sparse: expected struct pdbcw_page *dbcw
drivers/mailbox/arm_mhuv3.c:565:46: sparse: got struct pdbcw_page [noderef] __iomem *
>> drivers/mailbox/arm_mhuv3.c:568:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile [noderef] __iomem *addr @@ got struct pdbcw_int * @@
drivers/mailbox/arm_mhuv3.c:568:25: sparse: expected void const volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:568:25: sparse: got struct pdbcw_int *
>> drivers/mailbox/arm_mhuv3.c:568:25: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void volatile [noderef] __iomem *addr @@ got struct pdbcw_int * @@
drivers/mailbox/arm_mhuv3.c:568:25: sparse: expected void volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:568:25: sparse: got struct pdbcw_int *
drivers/mailbox/arm_mhuv3.c:569:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile [noderef] __iomem *addr @@ got struct pdbcw_int * @@
drivers/mailbox/arm_mhuv3.c:569:25: sparse: expected void const volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:569:25: sparse: got struct pdbcw_int *
drivers/mailbox/arm_mhuv3.c:569:25: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void volatile [noderef] __iomem *addr @@ got struct pdbcw_int * @@
drivers/mailbox/arm_mhuv3.c:569:25: sparse: expected void volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:569:25: sparse: got struct pdbcw_int *
>> drivers/mailbox/arm_mhuv3.c:570:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile [noderef] __iomem *addr @@ got struct xbcw_ctrl * @@
drivers/mailbox/arm_mhuv3.c:570:25: sparse: expected void const volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:570:25: sparse: got struct xbcw_ctrl *
>> drivers/mailbox/arm_mhuv3.c:570:25: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void volatile [noderef] __iomem *addr @@ got struct xbcw_ctrl * @@
drivers/mailbox/arm_mhuv3.c:570:25: sparse: expected void volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:570:25: sparse: got struct xbcw_ctrl *
>> drivers/mailbox/arm_mhuv3.c:573:46: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected struct mdbcw_page *dbcw @@ got struct mdbcw_page [noderef] __iomem * @@
drivers/mailbox/arm_mhuv3.c:573:46: sparse: expected struct mdbcw_page *dbcw
drivers/mailbox/arm_mhuv3.c:573:46: sparse: got struct mdbcw_page [noderef] __iomem *
>> drivers/mailbox/arm_mhuv3.c:576:25: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void volatile [noderef] __iomem *addr @@ got unsigned int * @@
drivers/mailbox/arm_mhuv3.c:576:25: sparse: expected void volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:576:25: sparse: got unsigned int *
drivers/mailbox/arm_mhuv3.c:577:25: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void volatile [noderef] __iomem *addr @@ got unsigned int * @@
drivers/mailbox/arm_mhuv3.c:577:25: sparse: expected void volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:577:25: sparse: got unsigned int *
drivers/mailbox/arm_mhuv3.c:578:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile [noderef] __iomem *addr @@ got struct xbcw_ctrl * @@
drivers/mailbox/arm_mhuv3.c:578:25: sparse: expected void const volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:578:25: sparse: got struct xbcw_ctrl *
drivers/mailbox/arm_mhuv3.c:578:25: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void volatile [noderef] __iomem *addr @@ got struct xbcw_ctrl * @@
drivers/mailbox/arm_mhuv3.c:578:25: sparse: expected void volatile [noderef] __iomem *addr
drivers/mailbox/arm_mhuv3.c:578:25: sparse: got struct xbcw_ctrl *
>> drivers/mailbox/arm_mhuv3.c:360:9: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:370:9: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:373:9: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:639:30: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:659:33: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:688:14: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:700:17: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:719:14: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:732:14: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:789:22: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:795:22: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:796:22: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:802:31: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:806:17: sparse: sparse: dereference of noderef expression
drivers/mailbox/arm_mhuv3.c:1038:17: sparse: sparse: dereference of noderef expression
vim +565 drivers/mailbox/arm_mhuv3.c
558
559 static void mhuv3_dbe_combined_irq_setup(struct mhuv3 *mhu)
560 {
561 int i;
562 struct mhuv3_extension *e = mhu->ext[DBE_EXT];
563
564 if (mhu->frame == PBX_FRAME) {
> 565 struct pdbcw_page *dbcw = mhu->pbx->dbcw;
566
567 for (i = 0; i < e->max_chans; i++) {
> 568 writel_relaxed_bitfield(0x1, &dbcw[i].int_clr, tfr_ack);
569 writel_relaxed_bitfield(0x0, &dbcw[i].int_en, tfr_ack);
> 570 writel_relaxed_bitfield(0x1, &dbcw[i].ctrl, comb_en);
571 }
572 } else {
> 573 struct mdbcw_page *dbcw = mhu->mbx->dbcw;
574
575 for (i = 0; i < e->max_chans; i++) {
> 576 writel_relaxed(0xFFFFFFFF, &dbcw[i].clr);
577 writel_relaxed(0xFFFFFFFF, &dbcw[i].msk_set);
578 writel_relaxed_bitfield(0x1, &dbcw[i].ctrl, comb_en);
579 }
580 }
581 }
582
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH net-next v6 17/17] net: pse-pd: Add TI TPS23881 PSE controller driver
From: Andrew Lunn @ 2024-03-28 16:24 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-17-c1011b6ea1cb@bootlin.com>
On Tue, Mar 26, 2024 at 03:04:54PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Add a new driver for the TI TPS23881 I2C Power Sourcing Equipment
> controller.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
>
> ---
> Change in v3:
> - New patch.
>
> Change in v6:
> - Fix firmware management, release_firmware was missing.
> ---
> drivers/net/pse-pd/Kconfig | 9 +
> drivers/net/pse-pd/Makefile | 1 +
> drivers/net/pse-pd/tps23881.c | 818 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 828 insertions(+)
>
> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
> index e3a6ba669f20..80cf373a5a0e 100644
> --- a/drivers/net/pse-pd/Kconfig
> +++ b/drivers/net/pse-pd/Kconfig
> @@ -31,4 +31,13 @@ config PSE_PD692X0
> To compile this driver as a module, choose M here: the
> module will be called pd692x0.
>
> +config PSE_TPS23881
> + tristate "TPS23881 PSE controller"
> + depends on I2C
> + help
> + This module provides support for TPS23881 regulator based Ethernet
> + Power Sourcing Equipment.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called tps23881.
> endif
> diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
> index 9c12c4a65730..9d2898b36737 100644
> --- a/drivers/net/pse-pd/Makefile
> +++ b/drivers/net/pse-pd/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
>
> obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
> obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
> +obj-$(CONFIG_PSE_TPS23881) += tps23881.o
> diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> new file mode 100644
> index 000000000000..c338d9eae363
> --- /dev/null
> +++ b/drivers/net/pse-pd/tps23881.c
> @@ -0,0 +1,818 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for the TI TPS23881 PoE PSE Controller driver (I2C bus)
> + *
> + * Copyright (c) 2023 Bootlin, Kory Maincent <kory.maincent@bootlin.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pse-pd/pse.h>
> +
> +#define TPS23881_MAX_CHANS 8
> +
> +#define TPS23881_REG_PW_STATUS 0x10
> +#define TPS23881_REG_OP_MODE 0x12
> +#define TPS23881_REG_DIS_EN 0x13
> +#define TPS23881_REG_DET_CLA_EN 0x14
> +#define TPS23881_REG_GEN_MASK 0x17
> +#define TPS23881_REG_NBITACC BIT(5)
> +#define TPS23881_REG_PW_EN 0x19
> +#define TPS23881_REG_PORT_MAP 0x26
> +#define TPS23881_REG_PORT_POWER 0x29
> +#define TPS23881_REG_POEPLUS 0x40
> +#define TPS23881_REG_TPON BIT(0)
> +#define TPS23881_REG_FWREV 0x41
> +#define TPS23881_REG_DEVID 0x43
> +#define TPS23881_REG_SRAM_CTRL 0x60
> +#define TPS23881_REG_SRAM_DATA 0x61
> +
> +struct tps23881_port_desc {
> + u8 chan[2];
> + bool is_4p;
> +};
> +
> +struct tps23881_priv {
> + struct i2c_client *client;
> + struct pse_controller_dev pcdev;
> + struct device_node *np;
> + struct tps23881_port_desc port[TPS23881_MAX_CHANS];
> +};
> +
> +static struct tps23881_priv *to_tps23881_priv(struct pse_controller_dev *pcdev)
> +{
> + return container_of(pcdev, struct tps23881_priv, pcdev);
> +}
> +
> +static int tps23881_pi_enable(struct pse_controller_dev *pcdev, int id)
> +{
> + struct tps23881_priv *priv = to_tps23881_priv(pcdev);
> + struct i2c_client *client = priv->client;
> + u8 chan;
> + u16 val;
> + int ret;
> +
> + if (id >= TPS23881_MAX_CHANS)
> + return -ERANGE;
> +
> + ret = i2c_smbus_read_word_data(client, TPS23881_REG_PW_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + chan = priv->port[id].chan[0];
> + if (chan < 4)
> + val = (u16)(ret | BIT(chan));
> + else
> + val = (u16)(ret | BIT(chan + 4));
> +
> + if (priv->port[id].is_4p) {
> + chan = priv->port[id].chan[1];
> + if (chan < 4)
> + val |= BIT(chan);
> + else
> + val |= BIT(chan + 4);
> + }
> +
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_PW_EN, val);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int tps23881_pi_disable(struct pse_controller_dev *pcdev, int id)
> +{
> + struct tps23881_priv *priv = to_tps23881_priv(pcdev);
> + struct i2c_client *client = priv->client;
> + u8 chan;
> + u16 val;
> + int ret;
> +
> + if (id >= TPS23881_MAX_CHANS)
> + return -ERANGE;
> +
> + ret = i2c_smbus_read_word_data(client, TPS23881_REG_PW_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + chan = priv->port[id].chan[0];
> + if (chan < 4)
> + val = (u16)(ret | BIT(chan + 4));
> + else
> + val = (u16)(ret | BIT(chan + 8));
> +
> + if (priv->port[id].is_4p) {
> + chan = priv->port[id].chan[1];
> + if (chan < 4)
> + val |= BIT(chan + 4);
> + else
> + val |= BIT(chan + 8);
> + }
> +
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_PW_EN, val);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int tps23881_pi_is_enabled(struct pse_controller_dev *pcdev, int id)
> +{
> + struct tps23881_priv *priv = to_tps23881_priv(pcdev);
> + struct i2c_client *client = priv->client;
> + bool enabled;
> + u8 chan;
> + int ret;
> +
> + ret = i2c_smbus_read_word_data(client, TPS23881_REG_PW_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + chan = priv->port[id].chan[0];
> + if (chan < 4)
> + enabled = ret & BIT(chan);
> + else
> + enabled = ret & BIT(chan + 4);
> +
> + if (priv->port[id].is_4p) {
> + chan = priv->port[id].chan[1];
> + if (chan < 4)
> + enabled &= !!(ret & BIT(chan));
> + else
> + enabled &= !!(ret & BIT(chan + 4));
> + }
> +
> + /* Return enabled status only if both channel are on this state */
> + return enabled;
> +}
> +
> +static int tps23881_ethtool_get_status(struct pse_controller_dev *pcdev,
> + unsigned long id,
> + struct netlink_ext_ack *extack,
> + struct pse_control_status *status)
> +{
> + struct tps23881_priv *priv = to_tps23881_priv(pcdev);
> + struct i2c_client *client = priv->client;
> + bool enabled, delivering;
> + u8 chan;
> + int ret;
> +
> + ret = i2c_smbus_read_word_data(client, TPS23881_REG_PW_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + chan = priv->port[id].chan[0];
> + if (chan < 4) {
> + enabled = ret & BIT(chan);
> + delivering = ret & BIT(chan + 4);
> + } else {
> + enabled = ret & BIT(chan + 4);
> + delivering = ret & BIT(chan + 8);
> + }
> +
> + if (priv->port[id].is_4p) {
> + chan = priv->port[id].chan[1];
> + if (chan < 4) {
> + enabled &= !!(ret & BIT(chan));
> + delivering &= !!(ret & BIT(chan + 4));
> + } else {
> + enabled &= !!(ret & BIT(chan + 4));
> + delivering &= !!(ret & BIT(chan + 8));
> + }
> + }
> +
> + /* Return delivering status only if both channel are on this state */
> + if (delivering)
> + status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_DELIVERING;
> + else
> + status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_DISABLED;
> +
> + /* Return enabled status only if both channel are on this state */
> + if (enabled)
> + status->c33_admin_state = ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED;
> + else
> + status->c33_admin_state = ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED;
> +
> + return 0;
> +}
> +
> +/* Parse managers subnode into a array of device node */
> +static int
> +tps23881_get_of_channels(struct tps23881_priv *priv,
> + struct device_node *chan_node[TPS23881_MAX_CHANS])
> +{
> + struct device_node *channels_node, *node;
> + int i, ret;
> +
> + if (!priv->np)
> + return -EINVAL;
> +
> + channels_node = of_find_node_by_name(priv->np, "channels");
> + if (!channels_node)
> + return -EINVAL;
> +
> + for_each_child_of_node(channels_node, node) {
> + u32 chan_id;
> +
> + if (!of_node_name_eq(node, "channel"))
> + continue;
> +
> + ret = of_property_read_u32(node, "reg", &chan_id);
> + if (ret) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (chan_id >= TPS23881_MAX_CHANS || chan_node[chan_id]) {
> + dev_err(&priv->client->dev,
> + "wrong number of port (%d)\n", chan_id);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + of_node_get(node);
> + chan_node[chan_id] = node;
> + }
> +
> + of_node_put(channels_node);
> + return 0;
> +
> +out:
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + of_node_put(chan_node[i]);
> + chan_node[i] = NULL;
> + }
> +
> + of_node_put(node);
> + of_node_put(channels_node);
> + return ret;
> +}
> +
> +struct tps23881_port_matrix {
> + u8 pi_id;
> + u8 lgcl_chan[2];
> + u8 hw_chan[2];
> + bool is_4p;
> + bool exist;
> +};
> +
> +static int
> +tps23881_match_channel(const struct pse_pi_pairset *pairset,
> + struct device_node *chan_node[TPS23881_MAX_CHANS])
> +{
> + int i;
> +
> + /* Look on every channels */
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + if (pairset->np == chan_node[i])
> + return i;
> + }
> +
> + return -ENODEV;
> +}
> +
> +static bool
> +tps23881_is_chan_free(struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS],
> + int chan)
> +{
> + int i;
> +
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + if (port_matrix[i].exist &&
> + (port_matrix[i].hw_chan[0] == chan ||
> + port_matrix[i].hw_chan[1] == chan))
> + return false;
> + }
> +
> + return true;
> +}
> +
> +/* Fill port matrix with the matching channels */
> +static int
> +tps23881_match_port_matrix(struct pse_pi *pi, int pi_id,
> + struct device_node *chan_node[TPS23881_MAX_CHANS],
> + struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS])
> +{
> + int ret;
> +
> + if (!pi->pairset[0].np)
> + return 0;
> +
> + ret = tps23881_match_channel(&pi->pairset[0], chan_node);
> + if (ret < 0)
> + return ret;
> +
> + if (!tps23881_is_chan_free(port_matrix, ret)) {
> + pr_err("tps23881: channel %d already used\n", ret);
> + return -ENODEV;
> + }
> +
> + port_matrix[pi_id].hw_chan[0] = ret;
> + port_matrix[pi_id].exist = true;
> +
> + if (!pi->pairset[1].np)
> + return 0;
> +
> + ret = tps23881_match_channel(&pi->pairset[1], chan_node);
> + if (ret < 0)
> + return ret;
> +
> + if (!tps23881_is_chan_free(port_matrix, ret)) {
> + pr_err("tps23881: channel %d already used\n", ret);
> + return -ENODEV;
> + }
> +
> + if (port_matrix[pi_id].hw_chan[0] / 4 != ret / 4) {
> + pr_err("tps23881: 4-pair PSE can only be set within the same 4 ports group");
> + return -ENODEV;
> + }
> +
> + port_matrix[pi_id].hw_chan[1] = ret;
> + port_matrix[pi_id].is_4p = true;
> +
> + return 0;
> +}
> +
> +static int
> +tps23881_get_unused_chan(struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS],
> + int port_cnt)
> +{
> + bool used;
> + int i, j;
> +
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + used = false;
> +
> + for (j = 0; j < port_cnt; j++) {
> + if (port_matrix[j].hw_chan[0] == i) {
> + used = true;
> + break;
> + }
> +
> + if (port_matrix[j].is_4p &&
> + port_matrix[j].hw_chan[1] == i) {
> + used = true;
> + break;
> + }
> + }
> +
> + if (!used)
> + return i;
> + }
> +
> + return -1;
> +}
> +
> +/* Sort the port matrix to following particular hardware ports matrix
> + * specification of the tps23881. The device has two 4-ports groups and
> + * each 4-pair powered device has to be configured to use two consecutive
> + * logical channel in each 4 ports group (1 and 2 or 3 and 4). Also the
> + * hardware matrix has to be fully configured even with unused chan to be
> + * valid.
> + */
> +static int
> +tps23881_sort_port_matrix(struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS])
> +{
> + struct tps23881_port_matrix tmp_port_matrix[TPS23881_MAX_CHANS] = {0};
> + int i, ret, port_cnt = 0, cnt_4ch_grp1 = 0, cnt_4ch_grp2 = 4;
> +
> + /* Configure 4p port matrix */
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + int *cnt;
> +
> + if (!port_matrix[i].exist || !port_matrix[i].is_4p)
> + continue;
> +
> + if (port_matrix[i].hw_chan[0] < 4)
> + cnt = &cnt_4ch_grp1;
> + else
> + cnt = &cnt_4ch_grp2;
> +
> + tmp_port_matrix[port_cnt].exist = true;
> + tmp_port_matrix[port_cnt].is_4p = true;
> + tmp_port_matrix[port_cnt].pi_id = i;
> + tmp_port_matrix[port_cnt].hw_chan[0] = port_matrix[i].hw_chan[0];
> + tmp_port_matrix[port_cnt].hw_chan[1] = port_matrix[i].hw_chan[1];
> +
> + /* 4-pair ports have to be configured with consecutive
> + * logical channels 0 and 1, 2 and 3.
> + */
> + tmp_port_matrix[port_cnt].lgcl_chan[0] = (*cnt)++;
> + tmp_port_matrix[port_cnt].lgcl_chan[1] = (*cnt)++;
> +
> + port_cnt++;
> + }
> +
> + /* Configure 2p port matrix */
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + int *cnt;
> +
> + if (!port_matrix[i].exist || port_matrix[i].is_4p)
> + continue;
> +
> + if (port_matrix[i].hw_chan[0] < 4)
> + cnt = &cnt_4ch_grp1;
> + else
> + cnt = &cnt_4ch_grp2;
> +
> + tmp_port_matrix[port_cnt].exist = true;
> + tmp_port_matrix[port_cnt].pi_id = i;
> + tmp_port_matrix[port_cnt].lgcl_chan[0] = (*cnt)++;
> + tmp_port_matrix[port_cnt].hw_chan[0] = port_matrix[i].hw_chan[0];
> +
> + port_cnt++;
> + }
> +
> + /* Complete the rest of the first 4 port group matrix even if
> + * channels are unused
> + */
> + while (cnt_4ch_grp1 < 4) {
> + ret = tps23881_get_unused_chan(tmp_port_matrix, port_cnt);
> + if (ret < 0) {
> + pr_err("tps23881: port matrix issue, no chan available\n");
> + return -ENODEV;
> + }
> +
> + if (port_cnt >= TPS23881_MAX_CHANS) {
> + pr_err("tps23881: wrong number of channels\n");
> + return -ENODEV;
> + }
> + tmp_port_matrix[port_cnt].lgcl_chan[0] = cnt_4ch_grp1;
> + tmp_port_matrix[port_cnt].hw_chan[0] = ret;
> + cnt_4ch_grp1++;
> + port_cnt++;
> + }
> +
> + /* Complete the rest of the second 4 port group matrix even if
> + * channels are unused
> + */
> + while (cnt_4ch_grp2 < 8) {
> + ret = tps23881_get_unused_chan(tmp_port_matrix, port_cnt);
> + if (ret < 0) {
> + pr_err("tps23881: port matrix issue, no chan available\n");
> + return -ENODEV;
> + }
> +
> + if (port_cnt >= TPS23881_MAX_CHANS) {
> + pr_err("tps23881: wrong number of channels\n");
> + return -ENODEV;
> + }
> + tmp_port_matrix[port_cnt].lgcl_chan[0] = cnt_4ch_grp2;
> + tmp_port_matrix[port_cnt].hw_chan[0] = ret;
> + cnt_4ch_grp2++;
> + port_cnt++;
> + }
> +
> + memcpy(port_matrix, tmp_port_matrix, sizeof(tmp_port_matrix));
> +
> + return port_cnt;
> +}
> +
> +/* Write port matrix to the hardware port matrix and the software port
> + * matrix.
> + */
> +static int
> +tps23881_write_port_matrix(struct tps23881_priv *priv,
> + struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS],
> + int port_cnt)
> +{
> + struct i2c_client *client = priv->client;
> + u8 pi_id, lgcl_chan, hw_chan;
> + u16 val = 0;
> + int i, ret;
> +
> + for (i = 0; i < port_cnt; i++) {
> + pi_id = port_matrix[i].pi_id;
> + lgcl_chan = port_matrix[i].lgcl_chan[0];
> + hw_chan = port_matrix[i].hw_chan[0] % 4;
> +
> + /* Set software port matrix for existing ports */
> + if (port_matrix[i].exist)
> + priv->port[pi_id].chan[0] = lgcl_chan;
> +
> + /* Set hardware port matrix for all ports */
> + val |= hw_chan << (lgcl_chan * 2);
> +
> + if (!port_matrix[i].is_4p)
> + continue;
> +
> + lgcl_chan = port_matrix[i].lgcl_chan[1];
> + hw_chan = port_matrix[i].hw_chan[1] % 4;
> +
> + /* Set software port matrix for existing ports */
> + if (port_matrix[i].exist) {
> + priv->port[pi_id].is_4p = true;
> + priv->port[pi_id].chan[1] = lgcl_chan;
> + }
> +
> + /* Set hardware port matrix for all ports */
> + val |= hw_chan << (lgcl_chan * 2);
> + }
> +
> + /* Write hardware ports matrix */
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_PORT_MAP, val);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int
> +tps23881_set_ports_conf(struct tps23881_priv *priv,
> + struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS])
> +{
> + struct i2c_client *client = priv->client;
> + int i, ret;
> + u16 val;
> +
> + /* Set operating mode */
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_OP_MODE, 0xaaaa);
> + if (ret)
> + return ret;
> +
> + /* Disable DC disconnect */
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_DIS_EN, 0x0);
> + if (ret)
> + return ret;
> +
> + /* Set port power allocation */
> + val = 0;
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + if (!port_matrix[i].exist)
> + continue;
> +
> + if (port_matrix[i].is_4p)
> + val |= 0xf << ((port_matrix[i].lgcl_chan[0] / 2) * 4);
> + else
> + val |= 0x3 << ((port_matrix[i].lgcl_chan[0] / 2) * 4);
> + }
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_PORT_POWER, val);
> + if (ret)
> + return ret;
> +
> + /* Enable detection and classification */
> + val = 0;
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + if (!port_matrix[i].exist)
> + continue;
> +
> + val |= BIT(port_matrix[i].lgcl_chan[0]) |
> + BIT(port_matrix[i].lgcl_chan[0] + 4);
> + if (port_matrix[i].is_4p)
> + val |= BIT(port_matrix[i].lgcl_chan[1]) |
> + BIT(port_matrix[i].lgcl_chan[1] + 4);
> + }
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_DET_CLA_EN, 0xffff);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int
> +tps23881_set_ports_matrix(struct tps23881_priv *priv,
> + struct device_node *chan_node[TPS23881_MAX_CHANS])
> +{
> + struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS] = {0};
> + int i, ret;
> +
> + /* Update with values for every PSE PIs */
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + ret = tps23881_match_port_matrix(&priv->pcdev.pi[i], i,
> + chan_node, port_matrix);
> + if (ret)
> + return ret;
> + }
> +
> + ret = tps23881_sort_port_matrix(port_matrix);
> + if (ret < 0)
> + return ret;
> +
> + ret = tps23881_write_port_matrix(priv, port_matrix, ret);
> + if (ret)
> + return ret;
> +
> + ret = tps23881_set_ports_conf(priv, port_matrix);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int tps23881_setup_pi_matrix(struct pse_controller_dev *pcdev)
> +{
> + struct device_node *chan_node[TPS23881_MAX_CHANS] = {NULL};
> + struct tps23881_priv *priv = to_tps23881_priv(pcdev);
> + int ret, i;
> +
> + ret = tps23881_get_of_channels(priv, chan_node);
> + if (ret < 0) {
> + dev_warn(&priv->client->dev,
> + "Unable to parse port-matrix, default matrix will be used\n");
> + return 0;
> + }
> +
> + ret = tps23881_set_ports_matrix(priv, chan_node);
> +
> + for (i = 0; i < TPS23881_MAX_CHANS; i++)
> + of_node_put(chan_node[i]);
> +
> + return ret;
> +}
> +
> +static const struct pse_controller_ops tps23881_ops = {
> + .setup_pi_matrix = tps23881_setup_pi_matrix,
> + .pi_enable = tps23881_pi_enable,
> + .pi_disable = tps23881_pi_disable,
> + .pi_is_enabled = tps23881_pi_is_enabled,
> + .ethtool_get_status = tps23881_ethtool_get_status,
> +};
> +
> +static const char fw_parity_name[] = "ti/tps23881/tps23881-parity-14.bin";
> +static const char fw_sram_name[] = "ti/tps23881/tps23881-sram-14.bin";
> +
> +struct tps23881_fw_conf {
> + u8 reg;
> + u8 val;
> +};
> +
> +static const struct tps23881_fw_conf tps23881_parity_flash_conf[] = {
> + {.reg = 0x60, .val = 0x01},
> + {.reg = 0x62, .val = 0x00},
> + {.reg = 0x63, .val = 0x80},
> + {.reg = 0x60, .val = 0xC4},
> + {.reg = 0x1D, .val = 0xBC},
> + {.reg = 0xD7, .val = 0x02},
> + {.reg = 0x91, .val = 0x00},
> + {.reg = 0x90, .val = 0x00},
> + {.reg = 0xD7, .val = 0x00},
> + {.reg = 0x1D, .val = 0x00},
> + { /* sentinel */ }
> +};
> +
> +static const struct tps23881_fw_conf tps23881_sram_flash_conf[] = {
> + {.reg = 0x60, .val = 0xC5},
> + {.reg = 0x62, .val = 0x00},
> + {.reg = 0x63, .val = 0x80},
> + {.reg = 0x60, .val = 0xC0},
> + {.reg = 0x1D, .val = 0xBC},
> + {.reg = 0xD7, .val = 0x02},
> + {.reg = 0x91, .val = 0x00},
> + {.reg = 0x90, .val = 0x00},
> + {.reg = 0xD7, .val = 0x00},
> + {.reg = 0x1D, .val = 0x00},
> + { /* sentinel */ }
> +};
> +
> +static int tps23881_flash_fw_part(struct i2c_client *client,
> + const char *fw_name,
> + const struct tps23881_fw_conf *fw_conf)
Does the device actually have flash? Or is this just downloading to
SRAM?
> +{
> + const struct firmware *fw = NULL;
> + int i, ret;
> +
> + ret = request_firmware(&fw, fw_name, &client->dev);
> + if (ret)
> + return ret;
> +
> + dev_info(&client->dev, "Flashing %s\n", fw_name);
If this is a one-time thing whenever there is a new firmware version
dropped into /lib/firmware, this would be O.K. However, if this
happens every boot, i would use dev_dbg().
Andrew
^ permalink raw reply
* [PATCH v4 6/6] iio: common: scmi_iio: convert to dev_err_probe()
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Make use of dev_err_probe() and dev_errp_probe() to simplify error paths
during probe.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/common/scmi_sensors/scmi_iio.c | 45 +++++++++++++-----------------
1 file changed, 19 insertions(+), 26 deletions(-)
diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 0c2caf3570db..30d58af02b4c 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -626,12 +626,10 @@ scmi_alloc_iiodev(struct scmi_device *sdev,
SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE,
&sensor->sensor_info->id,
&sensor->sensor_update_nb);
- if (ret) {
- dev_err(&iiodev->dev,
- "Error in registering sensor update notifier for sensor %s err %d",
- sensor->sensor_info->name, ret);
- return ERR_PTR(ret);
- }
+ if (ret)
+ return dev_errp_probe(&iiodev->dev, ret,
+ "Error in registering sensor update notifier for sensor %s err %d",
+ sensor->sensor_info->name, ret);
scmi_iio_set_timestamp_channel(&iio_channels[i], i);
iiodev->channels = iio_channels;
@@ -653,10 +651,9 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
return -ENODEV;
sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph);
- if (IS_ERR(sensor_ops)) {
- dev_err(dev, "SCMI device has no sensor interface\n");
- return PTR_ERR(sensor_ops);
- }
+ if (IS_ERR(sensor_ops))
+ return dev_err_probe(dev, PTR_ERR(sensor_ops),
+ "SCMI device has no sensor interface\n");
nr_sensors = sensor_ops->count_get(ph);
if (!nr_sensors) {
@@ -667,8 +664,8 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
for (i = 0; i < nr_sensors; i++) {
sensor_info = sensor_ops->info_get(ph, i);
if (!sensor_info) {
- dev_err(dev, "SCMI sensor %d has missing info\n", i);
- return -EINVAL;
+ return dev_err_probe(dev, -EINVAL,
+ "SCMI sensor %d has missing info\n", i);
}
/* This driver only supports 3-axis accel and gyro, skipping other sensors */
@@ -683,29 +680,25 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
scmi_iio_dev = scmi_alloc_iiodev(sdev, sensor_ops, ph,
sensor_info);
if (IS_ERR(scmi_iio_dev)) {
- dev_err(dev,
- "failed to allocate IIO device for sensor %s: %ld\n",
- sensor_info->name, PTR_ERR(scmi_iio_dev));
- return PTR_ERR(scmi_iio_dev);
+ return dev_err_probe(dev, PTR_ERR(scmi_iio_dev),
+ "failed to allocate IIO device for sensor %s: %ld\n",
+ sensor_info->name, PTR_ERR(scmi_iio_dev));
}
err = devm_iio_kfifo_buffer_setup(&scmi_iio_dev->dev,
scmi_iio_dev,
&scmi_iio_buffer_ops);
if (err < 0) {
- dev_err(dev,
- "IIO buffer setup error at sensor %s: %d\n",
- sensor_info->name, err);
- return err;
+ return dev_err_probe(dev, err,
+ "IIO buffer setup error at sensor %s: %d\n",
+ sensor_info->name, err);
}
err = devm_iio_device_register(dev, scmi_iio_dev);
- if (err) {
- dev_err(dev,
- "IIO device registration failed at sensor %s: %d\n",
- sensor_info->name, err);
- return err;
- }
+ if (err)
+ return dev_err_probe(dev, err,
+ "IIO device registration failed at sensor %s: %d\n",
+ sensor_info->name, err);
}
return err;
}
--
2.44.0
^ permalink raw reply related
* [PATCH v4 4/6] iio: temperature: ltc2983: support vdd regulator
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Add support for the power supply regulator.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/temperature/ltc2983.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index b4a8ca36458a..ff7f0829b575 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <linux/property.h>
#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <linux/spi/spi.h>
#include <asm/byteorder.h>
@@ -1574,6 +1575,10 @@ static int ltc2983_probe(struct spi_device *spi)
if (ret)
return ret;
+ ret = devm_regulator_get_enable(&spi->dev, "vdd");
+ if (ret)
+ return ret;
+
gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
--
2.44.0
^ permalink raw reply related
* [PATCH v4 5/6] iio: backend: make use dev_errp_probe()
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Using dev_errp_probe() to simplify the code.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/industrialio-backend.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index 2fea2bbbe47f..e0b08283d667 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -296,11 +296,9 @@ struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
}
fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
- if (IS_ERR(fwnode)) {
- dev_err_probe(dev, PTR_ERR(fwnode),
- "Cannot get Firmware reference\n");
- return ERR_CAST(fwnode);
- }
+ if (IS_ERR(fwnode))
+ return dev_errp_probe(dev, PTR_ERR(fwnode),
+ "Cannot get Firmware reference\n");
guard(mutex)(&iio_back_lock);
list_for_each_entry(back, &iio_back_list, entry) {
--
2.44.0
^ permalink raw reply related
* [PATCH v4 3/6] dt-bindings: iio: temperature: ltc2983: document power supply
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana, Krzysztof Kozlowski
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Add a property for the VDD power supply regulator.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
index dbb85135fd66..312febeeb3bb 100644
--- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
@@ -57,6 +57,8 @@ properties:
interrupts:
maxItems: 1
+ vdd-supply: true
+
adi,mux-delay-config-us:
description: |
Extra delay prior to each conversion, in addition to the internal 1ms
@@ -460,6 +462,7 @@ required:
- compatible
- reg
- interrupts
+ - vdd-supply
additionalProperties: false
@@ -489,6 +492,7 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
+ vdd-supply = <&supply>;
interrupts = <20 IRQ_TYPE_EDGE_RISING>;
interrupt-parent = <&gpio>;
--
2.44.0
^ permalink raw reply related
* [PATCH v4 2/6] iio: temperature: ltc2983: convert to dev_err_probe()
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
Use dev_err_probe() in the probe() path. While at it, made some simple
improvements:
* Declare a struct device *dev helper. This also makes the style more
consistent (some places the helper was used and not in other places);
* Explicitly included the err.h and errno.h headers;
* Removed an useless else if();
* Removed some unnecessary line breaks.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/temperature/ltc2983.c | 255 +++++++++++++++++---------------------
1 file changed, 115 insertions(+), 140 deletions(-)
diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 3c4524d57b8e..b4a8ca36458a 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -8,6 +8,8 @@
#include <linux/bitfield.h>
#include <linux/completion.h>
#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
#include <linux/kernel.h>
#include <linux/iio/iio.h>
#include <linux/interrupt.h>
@@ -657,10 +659,11 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
const struct ltc2983_sensor *sensor)
{
struct ltc2983_thermocouple *thermo;
+ struct device *dev = &st->spi->dev;
u32 oc_current;
int ret;
- thermo = devm_kzalloc(&st->spi->dev, sizeof(*thermo), GFP_KERNEL);
+ thermo = devm_kzalloc(dev, sizeof(*thermo), GFP_KERNEL);
if (!thermo)
return ERR_PTR(-ENOMEM);
@@ -687,21 +690,19 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
LTC2983_THERMOCOUPLE_OC_CURR(3);
break;
default:
- dev_err(&st->spi->dev,
- "Invalid open circuit current:%u", oc_current);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid open circuit current:%u",
+ oc_current);
}
thermo->sensor_config |= LTC2983_THERMOCOUPLE_OC_CHECK(1);
}
/* validate channel index */
if (!(thermo->sensor_config & LTC2983_THERMOCOUPLE_DIFF_MASK) &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for differential thermocouple",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for differential thermocouple",
+ sensor->chan);
struct fwnode_handle *ref __free(fwnode_handle) =
fwnode_find_reference(child, "adi,cold-junction-handle", 0);
@@ -709,14 +710,13 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
ref = NULL;
} else {
ret = fwnode_property_read_u32(ref, "reg", &thermo->cold_junction_chan);
- if (ret) {
+ if (ret)
/*
* This would be catched later but we can just return
* the error right away.
*/
- dev_err(&st->spi->dev, "Property reg must be given\n");
- return ERR_PTR(ret);
- }
+ return dev_errp_probe(dev, ret,
+ "Property reg must be given\n");
}
/* check custom sensor */
@@ -752,16 +752,14 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
struct fwnode_handle *ref __free(fwnode_handle) =
fwnode_find_reference(child, "adi,rsense-handle", 0);
- if (IS_ERR(ref)) {
- dev_err(dev, "Property adi,rsense-handle missing or invalid");
- return ERR_CAST(ref);
- }
+ if (IS_ERR(ref))
+ return dev_errp_probe(dev, PTR_ERR(ref),
+ "Property adi,rsense-handle missing or invalid");
ret = fwnode_property_read_u32(ref, "reg", &rtd->r_sense_chan);
- if (ret) {
- dev_err(dev, "Property reg must be given\n");
- return ERR_PTR(ret);
- }
+ if (ret)
+ return dev_errp_probe(dev, ret,
+ "Property reg must be given\n");
ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
if (!ret) {
@@ -780,19 +778,19 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
rtd->sensor_config = LTC2983_RTD_N_WIRES(3);
break;
default:
- dev_err(dev, "Invalid number of wires:%u\n", n_wires);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid number of wires:%u\n",
+ n_wires);
}
}
if (fwnode_property_read_bool(child, "adi,rsense-share")) {
/* Current rotation is only available with rsense sharing */
if (fwnode_property_read_bool(child, "adi,current-rotate")) {
- if (n_wires == 2 || n_wires == 3) {
- dev_err(dev,
- "Rotation not allowed for 2/3 Wire RTDs");
- return ERR_PTR(-EINVAL);
- }
+ if (n_wires == 2 || n_wires == 3)
+ return dev_errp_probe(dev, -EINVAL,
+ "Rotation not allowed for 2/3 Wire RTDs");
+
rtd->sensor_config |= LTC2983_RTD_C_ROTATE(1);
} else {
rtd->sensor_config |= LTC2983_RTD_R_SHARE(1);
@@ -815,29 +813,22 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
if (((rtd->sensor_config & LTC2983_RTD_KELVIN_R_SENSE_MASK)
== LTC2983_RTD_KELVIN_R_SENSE_MASK) &&
- (rtd->r_sense_chan <= min)) {
+ (rtd->r_sense_chan <= min))
/* kelvin rsense*/
- dev_err(dev,
- "Invalid rsense chann:%d to use in kelvin rsense",
- rtd->r_sense_chan);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid rsense chann:%d to use in kelvin rsense",
+ rtd->r_sense_chan);
- return ERR_PTR(-EINVAL);
- }
-
- if (sensor->chan < min || sensor->chan > max) {
- dev_err(dev, "Invalid chann:%d for the rtd config",
- sensor->chan);
-
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->chan < min || sensor->chan > max)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for the rtd config",
+ sensor->chan);
} else {
/* same as differential case */
- if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for RTD", sensor->chan);
-
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for RTD",
+ sensor->chan);
}
/* check custom sensor */
@@ -885,10 +876,9 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
rtd->excitation_current = 0x08;
break;
default:
- dev_err(&st->spi->dev,
- "Invalid value for excitation current(%u)",
- excitation_current);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid value for excitation current(%u)",
+ excitation_current);
}
}
@@ -912,16 +902,14 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
struct fwnode_handle *ref __free(fwnode_handle) =
fwnode_find_reference(child, "adi,rsense-handle", 0);
- if (IS_ERR(ref)) {
- dev_err(dev, "Property adi,rsense-handle missing or invalid");
- return ERR_CAST(ref);
- }
+ if (IS_ERR(ref))
+ return dev_errp_probe(dev, PTR_ERR(ref),
+ "Property adi,rsense-handle missing or invalid");
ret = fwnode_property_read_u32(ref, "reg", &thermistor->r_sense_chan);
- if (ret) {
- dev_err(dev, "rsense channel must be configured...\n");
- return ERR_PTR(ret);
- }
+ if (ret)
+ return dev_errp_probe(dev, ret,
+ "rsense channel must be configured...\n");
if (fwnode_property_read_bool(child, "adi,single-ended")) {
thermistor->sensor_config = LTC2983_THERMISTOR_SGL(1);
@@ -936,12 +924,10 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
}
/* validate channel index */
if (!(thermistor->sensor_config & LTC2983_THERMISTOR_DIFF_MASK) &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for differential thermistor",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for differential thermistor",
+ sensor->chan);
/* check custom sensor */
if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART) {
@@ -980,12 +966,10 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
switch (excitation_current) {
case 0:
/* auto range */
- if (sensor->type >=
- LTC2983_SENSOR_THERMISTOR_STEINHART) {
- dev_err(&st->spi->dev,
- "Auto Range not allowed for custom sensors\n");
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART)
+ return dev_errp_probe(dev, -EINVAL,
+ "Auto Range not allowed for custom sensors\n");
+
thermistor->excitation_current = 0x0c;
break;
case 250:
@@ -1022,10 +1006,9 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
thermistor->excitation_current = 0x0b;
break;
default:
- dev_err(&st->spi->dev,
- "Invalid value for excitation current(%u)",
- excitation_current);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid value for excitation current(%u)",
+ excitation_current);
}
}
@@ -1036,11 +1019,12 @@ static struct ltc2983_sensor *
ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *st,
const struct ltc2983_sensor *sensor)
{
+ struct device *dev = &st->spi->dev;
struct ltc2983_diode *diode;
u32 temp = 0, excitation_current = 0;
int ret;
- diode = devm_kzalloc(&st->spi->dev, sizeof(*diode), GFP_KERNEL);
+ diode = devm_kzalloc(dev, sizeof(*diode), GFP_KERNEL);
if (!diode)
return ERR_PTR(-ENOMEM);
@@ -1055,12 +1039,11 @@ ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *
/* validate channel index */
if (!(diode->sensor_config & LTC2983_DIODE_DIFF_MASK) &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev,
- "Invalid chann:%d for differential thermistor",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for differential thermistor",
+ sensor->chan);
+
/* set common parameters */
diode->sensor.fault_handler = ltc2983_common_fault_handler;
diode->sensor.assign_chan = ltc2983_diode_assign_chan;
@@ -1082,10 +1065,9 @@ ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *
diode->excitation_current = 0x03;
break;
default:
- dev_err(&st->spi->dev,
- "Invalid value for excitation current(%u)",
- excitation_current);
- return ERR_PTR(-EINVAL);
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid value for excitation current(%u)",
+ excitation_current);
}
}
@@ -1101,26 +1083,26 @@ static struct ltc2983_sensor *ltc2983_r_sense_new(struct fwnode_handle *child,
struct ltc2983_data *st,
const struct ltc2983_sensor *sensor)
{
+ struct device *dev = &st->spi->dev;
struct ltc2983_rsense *rsense;
int ret;
u32 temp;
- rsense = devm_kzalloc(&st->spi->dev, sizeof(*rsense), GFP_KERNEL);
+ rsense = devm_kzalloc(dev, sizeof(*rsense), GFP_KERNEL);
if (!rsense)
return ERR_PTR(-ENOMEM);
/* validate channel index */
- if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev, "Invalid chann:%d for r_sense",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chann:%d for r_sense",
+ sensor->chan);
ret = fwnode_property_read_u32(child, "adi,rsense-val-milli-ohms", &temp);
- if (ret) {
- dev_err(&st->spi->dev, "Property adi,rsense-val-milli-ohms missing\n");
- return ERR_PTR(-EINVAL);
- }
+ if (ret)
+ return dev_errp_probe(dev, -EINVAL,
+ "Property adi,rsense-val-milli-ohms missing\n");
+
/*
* Times 1000 because we have milli-ohms and __convert_to_raw
* expects scales of 1000000 which are used for all other
@@ -1139,21 +1121,21 @@ static struct ltc2983_sensor *ltc2983_adc_new(struct fwnode_handle *child,
struct ltc2983_data *st,
const struct ltc2983_sensor *sensor)
{
+ struct device *dev = &st->spi->dev;
struct ltc2983_adc *adc;
- adc = devm_kzalloc(&st->spi->dev, sizeof(*adc), GFP_KERNEL);
+ adc = devm_kzalloc(dev, sizeof(*adc), GFP_KERNEL);
if (!adc)
return ERR_PTR(-ENOMEM);
if (fwnode_property_read_bool(child, "adi,single-ended"))
adc->single_ended = true;
- if (!adc->single_ended &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev, "Invalid chan:%d for differential adc\n",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ if (!adc->single_ended && sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chan:%d for differential adc\n",
+ sensor->chan);
+
/* set common parameters */
adc->sensor.assign_chan = ltc2983_adc_assign_chan;
adc->sensor.fault_handler = ltc2983_common_fault_handler;
@@ -1165,21 +1147,20 @@ static struct ltc2983_sensor *ltc2983_temp_new(struct fwnode_handle *child,
struct ltc2983_data *st,
const struct ltc2983_sensor *sensor)
{
+ struct device *dev = &st->spi->dev;
struct ltc2983_temp *temp;
- temp = devm_kzalloc(&st->spi->dev, sizeof(*temp), GFP_KERNEL);
+ temp = devm_kzalloc(dev, sizeof(*temp), GFP_KERNEL);
if (!temp)
return ERR_PTR(-ENOMEM);
if (fwnode_property_read_bool(child, "adi,single-ended"))
temp->single_ended = true;
- if (!temp->single_ended &&
- sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
- dev_err(&st->spi->dev, "Invalid chan:%d for differential temp\n",
- sensor->chan);
- return ERR_PTR(-EINVAL);
- }
+ if (!temp->single_ended && sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+ return dev_errp_probe(dev, -EINVAL,
+ "Invalid chan:%d for differential temp\n",
+ sensor->chan);
temp->custom = __ltc2983_custom_sensor_new(st, child, "adi,custom-temp",
false, 4096, true);
@@ -1329,10 +1310,9 @@ static int ltc2983_parse_fw(struct ltc2983_data *st)
device_property_read_u32(dev, "adi,filter-notch-freq", &st->filter_notch_freq);
st->num_channels = device_get_child_node_count(dev);
- if (!st->num_channels) {
- dev_err(&st->spi->dev, "At least one channel must be given!");
- return -EINVAL;
- }
+ if (!st->num_channels)
+ return dev_err_probe(dev, -EINVAL,
+ "At least one channel must be given!");
st->sensors = devm_kcalloc(dev, st->num_channels, sizeof(*st->sensors),
GFP_KERNEL);
@@ -1419,6 +1399,7 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
unsigned int wait_time, unsigned int status_reg,
unsigned long status_fail_mask)
{
+ struct device *dev = &st->spi->dev;
unsigned long time;
unsigned int val;
int ret;
@@ -1437,19 +1418,16 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
time = wait_for_completion_timeout(&st->completion,
msecs_to_jiffies(wait_time));
- if (!time) {
- dev_err(&st->spi->dev, "EEPROM command timed out\n");
- return -ETIMEDOUT;
- }
+ if (!time)
+ return dev_err_probe(dev, -ETIMEDOUT, "EEPROM command timed out\n");
ret = regmap_read(st->regmap, status_reg, &val);
if (ret)
return ret;
- if (val & status_fail_mask) {
- dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val);
- return -EINVAL;
- }
+ if (val & status_fail_mask)
+ return dev_err_probe(dev, -EINVAL,
+ "EEPROM command failed: 0x%02X\n", val);
return 0;
}
@@ -1457,16 +1435,15 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
{
u32 iio_chan_t = 0, iio_chan_v = 0, chan, iio_idx = 0, status;
+ struct device *dev = &st->spi->dev;
int ret;
/* make sure the device is up: start bit (7) is 0 and done bit (6) is 1 */
ret = regmap_read_poll_timeout(st->regmap, LTC2983_STATUS_REG, status,
LTC2983_STATUS_UP(status) == 1, 25000,
25000 * 10);
- if (ret) {
- dev_err(&st->spi->dev, "Device startup timed out\n");
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "Device startup timed out\n");
ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG,
LTC2983_NOTCH_FREQ_MASK,
@@ -1566,12 +1543,13 @@ static const struct iio_info ltc2983_iio_info = {
static int ltc2983_probe(struct spi_device *spi)
{
+ struct device *dev = &spi->dev;
struct ltc2983_data *st;
struct iio_dev *indio_dev;
struct gpio_desc *gpio;
int ret;
- indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
if (!indio_dev)
return -ENOMEM;
@@ -1582,10 +1560,9 @@ static int ltc2983_probe(struct spi_device *spi)
return -ENODEV;
st->regmap = devm_regmap_init_spi(spi, <c2983_regmap_config);
- if (IS_ERR(st->regmap)) {
- dev_err(&spi->dev, "Failed to initialize regmap\n");
- return PTR_ERR(st->regmap);
- }
+ if (IS_ERR(st->regmap))
+ return dev_err_probe(dev, PTR_ERR(st->regmap),
+ "Failed to initialize regmap\n");
mutex_init(&st->lock);
init_completion(&st->completion);
@@ -1597,7 +1574,7 @@ static int ltc2983_probe(struct spi_device *spi)
if (ret)
return ret;
- gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH);
+ gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
@@ -1607,7 +1584,7 @@ static int ltc2983_probe(struct spi_device *spi)
gpiod_set_value_cansleep(gpio, 0);
}
- st->iio_chan = devm_kzalloc(&spi->dev,
+ st->iio_chan = devm_kzalloc(dev,
st->iio_channels * sizeof(*st->iio_chan),
GFP_KERNEL);
if (!st->iio_chan)
@@ -1617,12 +1594,10 @@ static int ltc2983_probe(struct spi_device *spi)
if (ret)
return ret;
- ret = devm_request_irq(&spi->dev, spi->irq, ltc2983_irq_handler,
+ ret = devm_request_irq(dev, spi->irq, ltc2983_irq_handler,
IRQF_TRIGGER_RISING, st->info->name, st);
- if (ret) {
- dev_err(&spi->dev, "failed to request an irq, %d", ret);
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to request an irq\n");
if (st->info->has_eeprom) {
ret = ltc2983_eeprom_cmd(st, LTC2983_EEPROM_WRITE_CMD,
@@ -1639,7 +1614,7 @@ static int ltc2983_probe(struct spi_device *spi)
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = <c2983_iio_info;
- return devm_iio_device_register(&spi->dev, indio_dev);
+ return devm_iio_device_register(dev, indio_dev);
}
static int ltc2983_resume(struct device *dev)
--
2.44.0
^ permalink raw reply related
* [PATCH v4 1/6] printk: add new dev_errp_probe() helper
From: Nuno Sa via B4 Relay @ 2024-03-28 16:22 UTC (permalink / raw)
To: linux-kernel, linux-iio, devicetree
Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Olivier Moysan, Jyoti Bhayana
In-Reply-To: <20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com>
From: Nuno Sa <nuno.sa@analog.com>
This is similar to dev_err_probe() but for cases where an ERR_PTR() is
to be returned simplifying patterns like:
dev_err_probe(dev, ret, ...);
return ERR_PTR(ret)
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
include/linux/dev_printk.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index 6bfe70decc9f..64484d092a77 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -276,4 +276,9 @@ do { \
__printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */
+#define dev_errp_probe(dev, ___err, fmt, ...) ({ \
+ ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__)); \
+})
+
#endif /* _DEVICE_PRINTK_H_ */
--
2.44.0
^ permalink raw reply related
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