Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231018182943.18700-1-ddrokosov@salutedevices.com> <20231018182943.18700-5-ddrokosov@salutedevices.com>
In-Reply-To: <20231018182943.18700-5-ddrokosov@salutedevices.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 19 Oct 2023 12:01:48 +0300
Message-ID: <CAHp75VeTVU-1mSzAX4j7s9bbDGxDR_FwgFL7dnC4_6OhsN8kpw@mail.gmail.com>
Subject: Re: [PATCH v2 04/11] leds: aw200xx: calculate dts property
 display_rows in driver
To:     Dmitry Rokosov <ddrokosov@salutedevices.com>
Cc:     lee@kernel.org, pavel@ucw.cz, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        kernel@sberdevices.ru, rockosov@gmail.com,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-leds@vger.kernel.org,
        George Stark <gnstark@salutedevices.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 18, 2023 at 9:30=E2=80=AFPM Dmitry Rokosov
<ddrokosov@salutedevices.com> wrote:
>
> From: George Stark <gnstark@salutedevices.com>
>
> Get rid of device tree property "awinic,display-rows" and calculate it
> in driver using led definition nodes. display-row actually means number
> of current switches and depends on how leds are connected to the device.

Still the commit message does not answer the question why it's safe
for the users that have this property enabled in their DTBs (note B
letter).

...

> +       device_for_each_child_node(dev, child) {
> +               u32 source;
> +               int ret;
> +
> +               ret =3D fwnode_property_read_u32(child, "reg", &source);
> +               if (ret || source >=3D chip->cdef->channels)

Perhaps a warning?

    dev_warn(dev, "Unable to read from %pfw or apply a source channel
number\n", child);

> +                       continue;
> +
> +               max_source =3D max(max_source, source);
> +       }

...

> +       chip->display_rows =3D max_source / chip->cdef->display_size_colu=
mns + 1;
> +       if (!chip->display_rows) {
> +               dev_err(dev, "No valid led definitions found\n");
> +               return -EINVAL;

So, this part is in ->probe() flow only, correct? If so,
  return dev_err_probe(...);

> +       }

...

> +       if (aw200xx_probe_get_display_rows(dev, chip))
> +               return -EINVAL;

Why is the error code shadowed?

--=20
With Best Regards,
Andy Shevchenko