Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On Fri, Nov 12, 2021 at 03:19:17PM +0100, Marijn Suijten wrote:
> On 2021-11-12 13:23:36, Daniel Thompson wrote:
> > On Fri, Nov 12, 2021 at 01:45:22PM +0100, Marijn Suijten wrote:
> > > On 2021-11-12 12:12:38, Daniel Thompson wrote:
> > > > On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> > > > > The length of qcom,enabled-strings as property array is enough to
> > > > > determine the number of strings to be enabled, without needing to set
> > > > > qcom,num-strings to override the default number of strings when less
> > > > > than the default (which is also the maxium) is provided in DT.
> > > > >
> > > > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver
> > > > > for WLED3")
> > > > > Signed-off-by: Marijn Suijten
> > > > > Reviewed-by: AngeloGioacchino Del Regno
> > > > >
> > > > > ---
> > > > > drivers/video/backlight/qcom-wled.c | 2 ++
> > > > > 1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > > > b/drivers/video/backlight/qcom-wled.c
> > > > > index c5232478a343..9bfbf601762a 100644
> > > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > > @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> > > > > return -EINVAL;
> > > > > }
> > > > > }
> > > > > +
> > > > > + cfg->num_strings = string_len;
> > > >
> > > > I still don't really understand why this wants to be a separate patch.
> > >
> > > I'm viewing this as a separate issue, and this makes it easier to
> > > document the change in a loose commit.
> > >
> > > > The warning text emitted by the previous patch (whatever text we agree
> > > > on) will be nonsense until this patch is applied.
> > > >
> > > > If this patch cannot appear before the warning is introduces then there
> > > > is no correct order for patches 4 and 5 (which implies they should be
> > > > the
> > > > same patch).
> > >
> > > Agreed, this is a weird way of doing things in v2 - the error message is
> > > printed yet the length of qcom,enabled-strings is always ignored before
> > > this patch.
> > >
> > > If we were to reorder patch 5 before patch 4 that should also
> > > temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
> > > this `if` so that `qcom,num-strings` remains the definitive way to
> > > set/override length. That's doable, and makes it easier to read patch 4
> > > as that bit of code will be replaced by of_property_read_u32 on that
> > > exact line. Let me know which method you prefer.
> >
> > Personally I would just squash them together. There are no redundant
> > values in the DT that could be fixed until we can use the string_len
> > to set num_strings.
>
> Reordering this patch before patch 4 in the way described above should
> allow just that, except that no warnings will be given for ambiguity
> until patch 4 is applied after that - which is weird given that that
> patch only intends the off-by-one error. Perhaps we should keep the
> order as it is, but add the ambiguity warning in this patch instead.
That works for me. Sounds good.
Daniel.
Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On 2021-11-12 13:23:36, Daniel Thompson wrote:
> On Fri, Nov 12, 2021 at 01:45:22PM +0100, Marijn Suijten wrote:
> > On 2021-11-12 12:12:38, Daniel Thompson wrote:
> > > On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> > > > The length of qcom,enabled-strings as property array is enough to
> > > > determine the number of strings to be enabled, without needing to set
> > > > qcom,num-strings to override the default number of strings when less
> > > > than the default (which is also the maxium) is provided in DT.
> > > >
> > > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for
> > > > WLED3")
> > > > Signed-off-by: Marijn Suijten
> > > > Reviewed-by: AngeloGioacchino Del Regno
> > > >
> > > > ---
> > > > drivers/video/backlight/qcom-wled.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > > b/drivers/video/backlight/qcom-wled.c
> > > > index c5232478a343..9bfbf601762a 100644
> > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> > > > return -EINVAL;
> > > > }
> > > > }
> > > > +
> > > > + cfg->num_strings = string_len;
> > >
> > > I still don't really understand why this wants to be a separate patch.
> >
> > I'm viewing this as a separate issue, and this makes it easier to
> > document the change in a loose commit.
> >
> > > The warning text emitted by the previous patch (whatever text we agree
> > > on) will be nonsense until this patch is applied.
> > >
> > > If this patch cannot appear before the warning is introduces then there
> > > is no correct order for patches 4 and 5 (which implies they should be the
> > > same patch).
> >
> > Agreed, this is a weird way of doing things in v2 - the error message is
> > printed yet the length of qcom,enabled-strings is always ignored before
> > this patch.
> >
> > If we were to reorder patch 5 before patch 4 that should also
> > temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
> > this `if` so that `qcom,num-strings` remains the definitive way to
> > set/override length. That's doable, and makes it easier to read patch 4
> > as that bit of code will be replaced by of_property_read_u32 on that
> > exact line. Let me know which method you prefer.
>
> Personally I would just squash them together. There are no redundant
> values in the DT that could be fixed until we can use the string_len
> to set num_strings.
Reordering this patch before patch 4 in the way described above should
allow just that, except that no warnings will be given for ambiguity
until patch 4 is applied after that - which is weird given that that
patch only intends the off-by-one error. Perhaps we should keep the
order as it is, but add the ambiguity warning in this patch instead.
That means we have one patch to fix the off-by-one first, and another
that allows qcom,num-strings to provide a default for num_strings. I
guess that's better to keep separated?
- Marijn
Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On Fri, Nov 12, 2021 at 01:45:22PM +0100, Marijn Suijten wrote:
> On 2021-11-12 12:12:38, Daniel Thompson wrote:
> > On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> > > The length of qcom,enabled-strings as property array is enough to
> > > determine the number of strings to be enabled, without needing to set
> > > qcom,num-strings to override the default number of strings when less
> > > than the default (which is also the maxium) is provided in DT.
> > >
> > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for
> > > WLED3")
> > > Signed-off-by: Marijn Suijten
> > > Reviewed-by: AngeloGioacchino Del Regno
> > >
> > > ---
> > > drivers/video/backlight/qcom-wled.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > b/drivers/video/backlight/qcom-wled.c
> > > index c5232478a343..9bfbf601762a 100644
> > > --- a/drivers/video/backlight/qcom-wled.c
> > > +++ b/drivers/video/backlight/qcom-wled.c
> > > @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> > > return -EINVAL;
> > > }
> > > }
> > > +
> > > + cfg->num_strings = string_len;
> >
> > I still don't really understand why this wants to be a separate patch.
>
> I'm viewing this as a separate issue, and this makes it easier to
> document the change in a loose commit.
>
> > The warning text emitted by the previous patch (whatever text we agree
> > on) will be nonsense until this patch is applied.
> >
> > If this patch cannot appear before the warning is introduces then there
> > is no correct order for patches 4 and 5 (which implies they should be the
> > same patch).
>
> Agreed, this is a weird way of doing things in v2 - the error message is
> printed yet the length of qcom,enabled-strings is always ignored before
> this patch.
>
> If we were to reorder patch 5 before patch 4 that should also
> temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
> this `if` so that `qcom,num-strings` remains the definitive way to
> set/override length. That's doable, and makes it easier to read patch 4
> as that bit of code will be replaced by of_property_read_u32 on that
> exact line. Let me know which method you prefer.
Personally I would just squash them together. There are no redundant
values in the DT that could be fixed until we can use the string_len
to set num_strings.
However I won't object to the other approach providing the result is
bisectable.
Daniel.
Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On 2021-11-12 12:12:38, Daniel Thompson wrote:
> On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> > The length of qcom,enabled-strings as property array is enough to
> > determine the number of strings to be enabled, without needing to set
> > qcom,num-strings to override the default number of strings when less
> > than the default (which is also the maxium) is provided in DT.
> >
> > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for
> > WLED3")
> > Signed-off-by: Marijn Suijten
> > Reviewed-by: AngeloGioacchino Del Regno
> >
> > ---
> > drivers/video/backlight/qcom-wled.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/video/backlight/qcom-wled.c
> > b/drivers/video/backlight/qcom-wled.c
> > index c5232478a343..9bfbf601762a 100644
> > --- a/drivers/video/backlight/qcom-wled.c
> > +++ b/drivers/video/backlight/qcom-wled.c
> > @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> > return -EINVAL;
> > }
> > }
> > +
> > + cfg->num_strings = string_len;
>
> I still don't really understand why this wants to be a separate patch.
I'm viewing this as a separate issue, and this makes it easier to
document the change in a loose commit.
> The warning text emitted by the previous patch (whatever text we agree
> on) will be nonsense until this patch is applied.
>
> If this patch cannot appear before the warning is introduces then there
> is no correct order for patches 4 and 5 (which implies they should be the
> same patch).
Agreed, this is a weird way of doing things in v2 - the error message is
printed yet the length of qcom,enabled-strings is always ignored before
this patch.
If we were to reorder patch 5 before patch 4 that should also
temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
this `if` so that `qcom,num-strings` remains the definitive way to
set/override length. That's doable, and makes it easier to read patch 4
as that bit of code will be replaced by of_property_read_u32 on that
exact line. Let me know which method you prefer.
- Marijn
Re: [RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom,enabled-strings
On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:
> The length of qcom,enabled-strings as property array is enough to
> determine the number of strings to be enabled, without needing to set
> qcom,num-strings to override the default number of strings when less
> than the default (which is also the maxium) is provided in DT.
>
> Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
> Signed-off-by: Marijn Suijten
> Reviewed-by: AngeloGioacchino Del Regno
>
> ---
> drivers/video/backlight/qcom-wled.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/video/backlight/qcom-wled.c
> b/drivers/video/backlight/qcom-wled.c
> index c5232478a343..9bfbf601762a 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
> return -EINVAL;
> }
> }
> +
> + cfg->num_strings = string_len;
I still don't really understand why this wants to be a separate patch.
The warning text emitted by the previous patch (whatever text we agree
on) will be nonsense until this patch is applied.
If this patch cannot appear before the warning is introduces then there
is no correct order for patches 4 and 5 (which implies they should be the
same patch).
Daniel.
[RESEND PATCH v2 05/13] backlight: qcom-wled: Override default length with qcom, enabled-strings
The length of qcom,enabled-strings as property array is enough to
determine the number of strings to be enabled, without needing to set
qcom,num-strings to override the default number of strings when less
than the default (which is also the maxium) is provided in DT.
Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten
Reviewed-by: AngeloGioacchino Del Regno
---
drivers/video/backlight/qcom-wled.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/video/backlight/qcom-wled.c
b/drivers/video/backlight/qcom-wled.c
index c5232478a343..9bfbf601762a 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
return -EINVAL;
}
}
+
+ cfg->num_strings = string_len;
}
rc = of_property_read_u32(dev->of_node, "qcom,num-strings", );
--
2.33.0
