Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <11fbc112-c410-8c67-9bcb-9450924d12ef@free.fr> <4904d37d-1cd4-b8f3-9c3c-82eb4569bca7@free.fr>
 <778d08be-b606-018a-c2bc-164fbbc33615@free.fr> <71c3a7c3-0661-c4ac-6f72-8409fa38211b@free.fr>
In-Reply-To: <71c3a7c3-0661-c4ac-6f72-8409fa38211b@free.fr>
From:   Bradford Love <brad@nextdimension.cc>
Date:   Fri, 26 Jun 2020 12:54:43 -0500
Message-ID: <CAA0YaJRs-HHO8NnoiVBujmcYXtJ_5CzrwBqkJ3vLiy1zWK+ruA@mail.gmail.com>
Subject: Re: Scanning for TV channels over DVB-T and DVB-T2
To:     Marc Gonzalez <marc.w.gonzalez@free.fr>
Cc:     linux-media <linux-media@vger.kernel.org>,
        Sean Young <sean@mess.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Jan Pieter van Woerkom <jp@jpvw.nl>,
        Antti Palosaari <crope@iki.fi>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Marc,


On Fri, Jun 19, 2020 at 3:15 PM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote:
>
> On 10/06/2020 17:22, Marc Gonzalez wrote:
>
> > FTR, on IRC, Brad pointed out this patch of his:
> > https://patchwork.kernel.org/patch/10744999/
>
> I suggest the following patch on top of Brad's patch:
>
> Author: Marc Gonzalez <marc.w.gonzalez@free.fr>
> Date:   Fri Jun 19 22:09:26 2020 +0200
>
>     si2168: wait for carrier lock before next step
>
> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
> index 31d3dc0216c2..e127e842f671 100644
> --- a/drivers/media/dvb-frontends/si2168.c
> +++ b/drivers/media/dvb-frontends/si2168.c
> @@ -152,6 +152,11 @@ static int si2168_ts_bus_ctrl(struct dvb_frontend *fe, int acquire)
>         return ret;
>  }
>
> +static bool carrier_locked(struct si2168_cmd *cmd)
> +{
> +       return cmd->args[2] & BIT(1);
> +}
> +
>  static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
>  {
>         struct i2c_client *client = fe->demodulator_priv;
> @@ -180,6 +185,9 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
>                 if (ret)
>                         goto err;
>
> +               if (!carrier_locked(&cmd))
> +                       goto parse_response;
> +


My original patch has been well tested and is currently deployed in
many thousands of assorted systems across Europe. Unless you can
guarantee that the frontend switchover race condition will *never*
happen *ever* across any system, including a large amount of
architectures and array of cpu types and speeds, I don't think it's
beneficial to remove it.

Hence, I'm very hesitant to deploy your patch and break this auto plp
detection for someone, just to save <=10ms.

Regards,

Brad


>                 if ((cmd.args[3] & 0x0f) == 7)
>                         sys = SYS_DVBT2;
>         }
> @@ -206,27 +214,10 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
>         }
>
>         ret = si2168_cmd_execute(client, &cmd);
> -       if (dvbt_auto_plp && (ret == -EREMOTEIO)) {
> -               /* In auto-PLP mode it is possible to read 0x8701 while
> -                * the frontend is in switchover transition. This causes
> -                * a status read failure, due to incorrect system. Check
> -                * the other sys if we hit this race condition.
> -                */
> -               if (sys == SYS_DVBT) {
> -                       memcpy(cmd.args, "\x50\x01", 2); /* DVB-T2 */
> -                       cmd.wlen = 2;
> -                       cmd.rlen = 14;
> -                       ret = si2168_cmd_execute(client, &cmd);
> -               } else if (sys == SYS_DVBT2) {
> -                       memcpy(cmd.args, "\xa0\x01", 2); /* DVB-T */
> -                       cmd.wlen = 2;
> -                       cmd.rlen = 13;
> -                       ret = si2168_cmd_execute(client, &cmd);
> -               }
> -       }
>         if (ret)
>                 goto err;
>
> +parse_response:
>         switch ((cmd.args[2] >> 1) & 0x03) {
>         case 0x01:
>                 *status = FE_HAS_SIGNAL | FE_HAS_CARRIER;
>