Received-SPF: pass (google.com: domain of linux-kernel+bounces-157204-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240424122932.79120-1-brgl@bgdev.pl> <0f8ba1b5-490e-4961-80e2-7942f66730ec@linaro.org>
 <CACMJSeuBCkNyaD60qGVpAq91DqD_OA=tCVEY0t+JNK2vcWBc+Q@mail.gmail.com> <d82c2e39-d2b8-4de0-a11a-6ab2420f8f95@quicinc.com>
In-Reply-To: <d82c2e39-d2b8-4de0-a11a-6ab2420f8f95@quicinc.com>
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Wed, 24 Apr 2024 17:30:10 +0200
Message-ID: <CAMRc=Md6yJ+ZMd7PD6ifQW4me1JLg3m6ZiVAMvbo9QaOH_jmaQ@mail.gmail.com>
Subject: Re: [PATCH v2] Bluetooth: qca: set power_ctrl_enabled on NULL
 returned by gpiod_get_optional()
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>, quic_zijuhu <quic_zijuhu@quicinc.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Marcel Holtmann <marcel@holtmann.org>, 
	linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Wren Turkal <wt@penguintechs.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 24, 2024 at 5:24=E2=80=AFPM quic_zijuhu <quic_zijuhu@quicinc.co=
m> wrote:
>
> On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
> > On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>
> >>
> >>>               qcadev->susclk =3D devm_clk_get_optional(&serdev->dev, =
NULL);
> >>>               if (IS_ERR(qcadev->susclk)) {
> >>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_dev=
ice *serdev)
> >>>               qcadev->bt_en =3D devm_gpiod_get_optional(&serdev->dev,=
 "enable",
> >>>                                              GPIOD_OUT_LOW);
> >>>               if (IS_ERR(qcadev->bt_en)) {
> >>> -                     dev_warn(&serdev->dev, "failed to acquire enabl=
e gpio\n");
> >>> -                     power_ctrl_enabled =3D false;
> >>> +                     dev_err(&serdev->dev, "failed to acquire enable=
 gpio\n");
> >>> +                     return PTR_ERR(qcadev->bt_en);
> please think about for QCA2066. if it is returned from here.  BT will
> not working at all.  if you don't return here. i will be working fine
> for every BT functionality.
> NAK again by me.
>

Luiz,

This in turn is an example of Zijun making a claim that looks like a
legitimate review but is simply untrue. He's done it several times.
I'm afraid that it may affect your judgment due to the confidence the
claims are made with. As Krzysztof said multiple times: the
device-tree bindings for QCA2066 are very clear: the enable-gpios
property is *required* and so returning an error on failure here is
correct. Even changing gpiod_get_optional() to just gpiod_get() would
be in line with what the contract in the binding document says. Even
if we relaxed the bindings, returning here stil *IS CORRECT* as if the
enable-gpios is not defined, GPIOLIB will return NULL and we will NOT
return.

Bartosz

> >>>               }
> >>>
> >>> +             if (!qcadev->bt_en)
> >>> +                     power_ctrl_enabled =3D false;
> >>
> >> This looks duplicated - you already have such check earlier.
> >>
> >
> > It's under a different switch case!
> >
> > Bartosz
>