[v4,04/11] leds: aw200xx: calculate dts property display_rows in the driver

  From: George Stark <gnstark@salutedevices.com>

Get rid of device tree property "awinic,display-rows". The property
value actually means number of current switches and depends on how leds
are connected to the device. It should be calculated manually by max
used led number. In the same way it is computed automatically now.
Max used led is taken from led definition subnodes.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 39 +++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)
  

On Tue, 21 Nov 2023, Dmitry Rokosov wrote:

> From: George Stark <gnstark@salutedevices.com>
> 
> Get rid of device tree property "awinic,display-rows". The property
> value actually means number of current switches and depends on how leds

Nit: LEDs

> are connected to the device. It should be calculated manually by max
> used led number. In the same way it is computed automatically now.

As above - I won't mention this again.

> Max used led is taken from led definition subnodes.
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  drivers/leds/leds-aw200xx.c | 39 +++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> index 7762b3a132ac..4bce5e7381c0 100644
> --- a/drivers/leds/leds-aw200xx.c
> +++ b/drivers/leds/leds-aw200xx.c
> @@ -379,6 +379,30 @@ static void aw200xx_disable(const struct aw200xx *const chip)
>  	return gpiod_set_value_cansleep(chip->hwen, 0);
>  }
>  
> +static bool aw200xx_probe_get_display_rows(struct device *dev, struct aw200xx *chip)
> +{
> +	struct fwnode_handle *child;
> +	u32 max_source = 0;
> +
> +	device_for_each_child_node(dev, child) {
> +		u32 source;
> +		int ret;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &source);
> +		if (ret || source >= chip->cdef->channels)

Shouldn't the second clause fail instantly?

> +			continue;
> +
> +		max_source = max(max_source, source);
> +	}
> +
> +	if (!max_source)

Since max_source is an integer, please use an '== 0' comparison.

> +		return false;
> +
> +	chip->display_rows = max_source / chip->cdef->display_size_columns + 1;
> +
> +	return true;
> +}
> +
>  static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
>  {
>  	struct fwnode_handle *child;
> @@ -386,18 +410,9 @@ static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
>  	int ret;
>  	int i;
>  
> -	ret = device_property_read_u32(dev, "awinic,display-rows",
> -				       &chip->display_rows);
> -	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "Failed to read 'display-rows' property\n");
> -
> -	if (!chip->display_rows ||
> -	    chip->display_rows > chip->cdef->display_size_rows_max) {
> -		return dev_err_probe(dev, ret,
> -				     "Invalid leds display size %u\n",
> -				     chip->display_rows);
> -	}
> +	if (!aw200xx_probe_get_display_rows(dev, chip))

Function calls in side if() statements in general is rough.

Please break it out and use 'ret' as we usually do.

> +		return dev_err_probe(dev, -EINVAL,

Make this the return value from aw200xx_probe_get_display_rows() and use
'ret' instead.

> +				     "No valid led definitions found\n");
>  
>  	current_max = aw200xx_imax_from_global(chip, AW200XX_IMAX_MAX_uA);
>  	current_min = aw200xx_imax_from_global(chip, AW200XX_IMAX_MIN_uA);
> -- 
> 2.36.0
>
  

On Thu, Nov 23, 2023 at 04:32:52PM +0000, Lee Jones wrote:
> On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
> 
> > From: George Stark <gnstark@salutedevices.com>
> > 
> > Get rid of device tree property "awinic,display-rows". The property
> > value actually means number of current switches and depends on how leds
> 
> Nit: LEDs
> 
> > are connected to the device. It should be calculated manually by max
> > used led number. In the same way it is computed automatically now.
> 
> As above - I won't mention this again.
> 
> > Max used led is taken from led definition subnodes.
> > 
> > Signed-off-by: George Stark <gnstark@salutedevices.com>
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> >  drivers/leds/leds-aw200xx.c | 39 +++++++++++++++++++++++++------------
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> > index 7762b3a132ac..4bce5e7381c0 100644
> > --- a/drivers/leds/leds-aw200xx.c
> > +++ b/drivers/leds/leds-aw200xx.c
> > @@ -379,6 +379,30 @@ static void aw200xx_disable(const struct aw200xx *const chip)
> >  	return gpiod_set_value_cansleep(chip->hwen, 0);
> >  }
> >  
> > +static bool aw200xx_probe_get_display_rows(struct device *dev, struct aw200xx *chip)
> > +{
> > +	struct fwnode_handle *child;
> > +	u32 max_source = 0;
> > +
> > +	device_for_each_child_node(dev, child) {
> > +		u32 source;
> > +		int ret;
> > +
> > +		ret = fwnode_property_read_u32(child, "reg", &source);
> > +		if (ret || source >= chip->cdef->channels)
> 
> Shouldn't the second clause fail instantly?
> 

We already have such logic in the aw200xx_probe_fw() function, which
skips the LED node with the wrong reg value too. Furthermore, we have
strict reg constraints in the dt-bindings parts (in the current patch
series), so we assume that the DT developer will not create an LED with
the wrong reg value.

> > +			continue;
> > +
> > +		max_source = max(max_source, source);
> > +	}
> > +
> > +	if (!max_source)
> 
> Since max_source is an integer, please use an '== 0' comparison.
> 

Okay

> > +		return false;
> > +
> > +	chip->display_rows = max_source / chip->cdef->display_size_columns + 1;
> > +
> > +	return true;
> > +}
> > +
> >  static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
> >  {
> >  	struct fwnode_handle *child;
> > @@ -386,18 +410,9 @@ static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
> >  	int ret;
> >  	int i;
> >  
> > -	ret = device_property_read_u32(dev, "awinic,display-rows",
> > -				       &chip->display_rows);
> > -	if (ret)
> > -		return dev_err_probe(dev, ret,
> > -				     "Failed to read 'display-rows' property\n");
> > -
> > -	if (!chip->display_rows ||
> > -	    chip->display_rows > chip->cdef->display_size_rows_max) {
> > -		return dev_err_probe(dev, ret,
> > -				     "Invalid leds display size %u\n",
> > -				     chip->display_rows);
> > -	}
> > +	if (!aw200xx_probe_get_display_rows(dev, chip))
> 
> Function calls in side if() statements in general is rough.
> 
> Please break it out and use 'ret' as we usually do.
> 
> > +		return dev_err_probe(dev, -EINVAL,
> 
> Make this the return value from aw200xx_probe_get_display_rows() and use
> 'ret' instead.
> 

No problem, I'll prepare a new version.

> > +				     "No valid led definitions found\n");
> >  
> >  	current_max = aw200xx_imax_from_global(chip, AW200XX_IMAX_MAX_uA);
> >  	current_min = aw200xx_imax_from_global(chip, AW200XX_IMAX_MIN_uA);
> > -- 
> > 2.36.0
> > 
> 
> -- 
> Lee Jones [李琼斯]
  

On Fri, 24 Nov 2023, Dmitry Rokosov wrote:

> On Thu, Nov 23, 2023 at 04:32:52PM +0000, Lee Jones wrote:
> > On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
> > 
> > > From: George Stark <gnstark@salutedevices.com>
> > > 
> > > Get rid of device tree property "awinic,display-rows". The property
> > > value actually means number of current switches and depends on how leds
> > 
> > Nit: LEDs
> > 
> > > are connected to the device. It should be calculated manually by max
> > > used led number. In the same way it is computed automatically now.
> > 
> > As above - I won't mention this again.
> > 
> > > Max used led is taken from led definition subnodes.
> > > 
> > > Signed-off-by: George Stark <gnstark@salutedevices.com>
> > > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > > ---
> > >  drivers/leds/leds-aw200xx.c | 39 +++++++++++++++++++++++++------------
> > >  1 file changed, 27 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> > > index 7762b3a132ac..4bce5e7381c0 100644
> > > --- a/drivers/leds/leds-aw200xx.c
> > > +++ b/drivers/leds/leds-aw200xx.c
> > > @@ -379,6 +379,30 @@ static void aw200xx_disable(const struct aw200xx *const chip)
> > >  	return gpiod_set_value_cansleep(chip->hwen, 0);
> > >  }
> > >  
> > > +static bool aw200xx_probe_get_display_rows(struct device *dev, struct aw200xx *chip)
> > > +{
> > > +	struct fwnode_handle *child;
> > > +	u32 max_source = 0;
> > > +
> > > +	device_for_each_child_node(dev, child) {
> > > +		u32 source;
> > > +		int ret;
> > > +
> > > +		ret = fwnode_property_read_u32(child, "reg", &source);
> > > +		if (ret || source >= chip->cdef->channels)
> > 
> > Shouldn't the second clause fail instantly?
> > 
> 
> We already have such logic in the aw200xx_probe_fw() function, which
> skips the LED node with the wrong reg value too. Furthermore, we have
> strict reg constraints in the dt-bindings parts (in the current patch
> series), so we assume that the DT developer will not create an LED with
> the wrong reg value.

Why is it being checked again then?
  

Lee,

Thank you for the quick reply!

On Mon, Nov 27, 2023 at 08:57:55AM +0000, Lee Jones wrote:
> On Fri, 24 Nov 2023, Dmitry Rokosov wrote:
> 
> > On Thu, Nov 23, 2023 at 04:32:52PM +0000, Lee Jones wrote:
> > > On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
> > > 
> > > > From: George Stark <gnstark@salutedevices.com>
> > > > 
> > > > Get rid of device tree property "awinic,display-rows". The property
> > > > value actually means number of current switches and depends on how leds
> > > 
> > > Nit: LEDs
> > > 
> > > > are connected to the device. It should be calculated manually by max
> > > > used led number. In the same way it is computed automatically now.
> > > 
> > > As above - I won't mention this again.
> > > 
> > > > Max used led is taken from led definition subnodes.
> > > > 
> > > > Signed-off-by: George Stark <gnstark@salutedevices.com>
> > > > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > > > ---
> > > >  drivers/leds/leds-aw200xx.c | 39 +++++++++++++++++++++++++------------
> > > >  1 file changed, 27 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> > > > index 7762b3a132ac..4bce5e7381c0 100644
> > > > --- a/drivers/leds/leds-aw200xx.c
> > > > +++ b/drivers/leds/leds-aw200xx.c
> > > > @@ -379,6 +379,30 @@ static void aw200xx_disable(const struct aw200xx *const chip)
> > > >  	return gpiod_set_value_cansleep(chip->hwen, 0);
> > > >  }
> > > >  
> > > > +static bool aw200xx_probe_get_display_rows(struct device *dev, struct aw200xx *chip)
> > > > +{
> > > > +	struct fwnode_handle *child;
> > > > +	u32 max_source = 0;
> > > > +
> > > > +	device_for_each_child_node(dev, child) {
> > > > +		u32 source;
> > > > +		int ret;
> > > > +
> > > > +		ret = fwnode_property_read_u32(child, "reg", &source);
> > > > +		if (ret || source >= chip->cdef->channels)
> > > 
> > > Shouldn't the second clause fail instantly?
> > > 
> > 
> > We already have such logic in the aw200xx_probe_fw() function, which
> > skips the LED node with the wrong reg value too. Furthermore, we have
> > strict reg constraints in the dt-bindings parts (in the current patch
> > series), so we assume that the DT developer will not create an LED with
> > the wrong reg value.
> 
> Why is it being checked again then?

Hmmm, aw200xx_probe_get_display_rows() executes before the old
implementation... So we need to check it again. Do you think it should
be reworked? I've already sent a new patchset. Could you please take a
look at the other fixes?