| Message ID | 20240201151747.7524-6-ansuelsmth@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | net: phy: Introduce PHY Package concept | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success |
On 01/02/2024 16:17, Christian Marangi wrote: > From: Robert Marko <robert.marko@sartura.hr> > > Add DT bindings defined for Qualcomm QCA807x PHY series related to > calibration and DAC settings. Nothing from this file is used and your commit msg does not provide rationale "why", thus it does not look like something for bindings. Otherwise please point me which patch with *driver* uses these bindings. > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++ Use filename matching compatible, so vendor,device. No wildcards, unless your compatible also has them. > 1 file changed, 30 insertions(+) > create mode 100644 include/dt-bindings/net/qcom-qca807x.h > Best regards, Krzysztof
On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote: > On 01/02/2024 16:17, Christian Marangi wrote: > > From: Robert Marko <robert.marko@sartura.hr> > > > > Add DT bindings defined for Qualcomm QCA807x PHY series related to > > calibration and DAC settings. > > Nothing from this file is used and your commit msg does not provide > rationale "why", thus it does not look like something for bindings. > Otherwise please point me which patch with *driver* uses these bindings. > Hi, since I have to squash this, I will include the reason in the schema patch. Anyway these are raw values used to configure the qcom,control-dac property. In the driver it's used by qca807x_config_init. We read what is set in DT and we configure the reg accordingly. If this is wrong should we use a more schema friendly approach with declaring an enum of string and document that there? > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++ > > Use filename matching compatible, so vendor,device. No wildcards, unless > your compatible also has them. > > > 1 file changed, 30 insertions(+) > > create mode 100644 include/dt-bindings/net/qcom-qca807x.h > > > > > > Best regards, > Krzysztof >
On Fri, Feb 02, 2024 at 04:19:15PM +0100, Christian Marangi wrote: > On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote: > > On 01/02/2024 16:17, Christian Marangi wrote: > > > From: Robert Marko <robert.marko@sartura.hr> > > > > > > Add DT bindings defined for Qualcomm QCA807x PHY series related to > > > calibration and DAC settings. > > > > Nothing from this file is used and your commit msg does not provide > > rationale "why", thus it does not look like something for bindings. > > Otherwise please point me which patch with *driver* uses these bindings. > > > > Hi, since I have to squash this, I will include the reason in the schema > patch. > > Anyway these are raw values used to configure the qcom,control-dac > property. Maybe I am missing something, but a quick scan of the patchset and a grep of the tree doesn't show this property being documented anywhere. > In the driver it's used by qca807x_config_init. We read what is set in > DT and we configure the reg accordingly. > > If this is wrong should we use a more schema friendly approach with > declaring an enum of string and document that there? Without any idea of what that property is used for it is hard to say, but personally I much prefer enums of strings for what looks like a property that holds register values. That you felt it necessary to add defines for the values makes it more like that that is the case. Cheers, Conor. > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > --- > > > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++ > > > > Use filename matching compatible, so vendor,device. No wildcards, unless > > your compatible also has them. > > > > > 1 file changed, 30 insertions(+) > > > create mode 100644 include/dt-bindings/net/qcom-qca807x.h > > > > > > > > > > > Best regards, > > Krzysztof > > > > -- > Ansuel
On Fri, Feb 02, 2024 at 04:58:32PM +0000, Conor Dooley wrote: > On Fri, Feb 02, 2024 at 04:19:15PM +0100, Christian Marangi wrote: > > On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote: > > > On 01/02/2024 16:17, Christian Marangi wrote: > > > > From: Robert Marko <robert.marko@sartura.hr> > > > > > > > > Add DT bindings defined for Qualcomm QCA807x PHY series related to > > > > calibration and DAC settings. > > > > > > Nothing from this file is used and your commit msg does not provide > > > rationale "why", thus it does not look like something for bindings. > > > Otherwise please point me which patch with *driver* uses these bindings. > > > > > > > Hi, since I have to squash this, I will include the reason in the schema > > patch. > > > > Anyway these are raw values used to configure the qcom,control-dac > > property. > > Maybe I am missing something, but a quick scan of the patchset and a > grep of the tree doesn't show this property being documented anywhere. > > > In the driver it's used by qca807x_config_init. We read what is set in > > DT and we configure the reg accordingly. > > > > If this is wrong should we use a more schema friendly approach with > > declaring an enum of string and document that there? > > Without any idea of what that property is used for it is hard to say, > but personally I much prefer enums of strings for what looks like a > property that holds register values. > > That you felt it necessary to add defines for the values makes it more > like that that is the case. > Ok, no problem. The big idea here was really to skip having to have a big function that parse strings but I get that it's better handle with string and have them documented directly in the yaml. > > > > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > > --- > > > > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++ > > > > > > Use filename matching compatible, so vendor,device. No wildcards, unless > > > your compatible also has them. > > > > > > > 1 file changed, 30 insertions(+) > > > > create mode 100644 include/dt-bindings/net/qcom-qca807x.h > > > > > > > > > > > > > > > > Best regards, > > > Krzysztof > > > > > > > -- > > Ansuel
diff --git a/include/dt-bindings/net/qcom-qca807x.h b/include/dt-bindings/net/qcom-qca807x.h new file mode 100644 index 000000000000..e7d4d09b7dd4 --- /dev/null +++ b/include/dt-bindings/net/qcom-qca807x.h @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */ +/* + * Device Tree constants for the Qualcomm QCA807X PHYs + */ + +#ifndef _DT_BINDINGS_QCOM_QCA807X_H +#define _DT_BINDINGS_QCOM_QCA807X_H + +/* Full amplitude, full bias current */ +#define QCA807X_CONTROL_DAC_FULL_VOLT_BIAS 0 +/* Amplitude follow DSP (amplitude is adjusted based on cable length), half bias current */ +#define QCA807X_CONTROL_DAC_DSP_VOLT_HALF_BIAS 1 +/* Full amplitude, bias current follow DSP (bias current is adjusted based on cable length) */ +#define QCA807X_CONTROL_DAC_FULL_VOLT_DSP_BIAS 2 +/* Both amplitude and bias current follow DSP */ +#define QCA807X_CONTROL_DAC_DSP_VOLT_BIAS 3 +/* Full amplitude, half bias current */ +#define QCA807X_CONTROL_DAC_FULL_VOLT_HALF_BIAS 4 +/* Amplitude follow DSP setting; 1/4 bias current when cable<10m, + * otherwise half bias current + */ +#define QCA807X_CONTROL_DAC_DSP_VOLT_QUARTER_BIAS 5 +/* Full amplitude; same bias current setting with “010” and “011”, + * but half more bias is reduced when cable <10m + */ +#define QCA807X_CONTROL_DAC_FULL_VOLT_HALF_BIAS_SHORT 6 +/* Amplitude follow DSP; same bias current setting with “110”, default value */ +#define QCA807X_CONTROL_DAC_DSP_VOLT_HALF_BIAS_SHORT 7 + +#endif