linux-kernel - Re: [PATCH v5 7/9] platform/x86: serial-multi-instantiate: Add SPI support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0j+DkX+-P1XxZ=HAnUzPjdkNFkXRTjJzhSH27KfDFAGDQ@mail.gmail.com>
Date:   Fri, 21 Jan 2022 18:14:55 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Stefan Binding <sbinding@...nsource.cirrus.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Mark Brown <broonie@...nel.org>, Len Brown <lenb@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@...a-project.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH v5 7/9] platform/x86: serial-multi-instantiate: Add SPI support

On Fri, Jan 21, 2022 at 5:55 PM Stefan Binding
<sbinding@...nsource.cirrus.com> wrote:
>
> Hi,
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <rafael@...nel.org>
> > Sent: 21 January 2022 15:31
> > To: Stefan Binding <sbinding@...nsource.cirrus.com>
> > Cc: Mark Brown <broonie@...nel.org>; Rafael J . Wysocki
> > <rafael@...nel.org>; Len Brown <lenb@...nel.org>; Hans de Goede
> > <hdegoede@...hat.com>; Mark Gross <markgross@...nel.org>; Jaroslav
> > Kysela <perex@...ex.cz>; Takashi Iwai <tiwai@...e.com>; moderated
> > list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM... <alsa-
> > devel@...a-project.org>; Linux Kernel Mailing List <linux-
> > kernel@...r.kernel.org>; linux-spi <linux-spi@...r.kernel.org>; ACPI Devel
> > Maling List <linux-acpi@...r.kernel.org>; Platform Driver <platform-driver-
> > x86@...r.kernel.org>; patches@...nsource.cirrus.com
> > Subject: Re: [PATCH v5 7/9] platform/x86: serial-multi-instantiate: Add SPI
> > support
> >
>
>
> > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > index 5b65d687f046..28f5bbf0f27a 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -991,12 +991,12 @@ config TOPSTAR_LAPTOP
> > >           If you have a Topstar laptop, say Y or M here.
> > >
> > >  config SERIAL_MULTI_INSTANTIATE
> > > -       tristate "I2C multi instantiate pseudo device driver"
> > > -       depends on I2C && ACPI
> > > +       tristate "I2C and SPI multi instantiate pseudo device driver"
> > > +       depends on I2C && SPI && ACPI
> >
> > Should this be (I2C || SPI) && ACPI ?
>
> We made it dependent on both I2C and SPI because of how interconnected the
> serial-multi-instantiate driver is with both SPI and I2C. We felt attempting to make
> the driver compatible with one without the other would end up very complicated.

That's fine IMV, but it would be good to mention it in the changelog.

> > > @@ -146,7 +247,21 @@ static int smi_probe(struct platform_device *pdev)
> > >
> > >         platform_set_drvdata(pdev, smi);
> > >
> > > -       return smi_i2c_probe(pdev, adev, smi, inst_array);
> > > +       switch (node->bus_type) {
> > > +       case SMI_I2C:
> > > +               return smi_i2c_probe(pdev, adev, smi, node->instances);
> > > +       case SMI_SPI:
> > > +               return smi_spi_probe(pdev, adev, smi, node->instances);
> > > +       case SMI_AUTO_DETECT:
> > > +               if (i2c_acpi_client_count(adev) > 0)
> > > +                       return smi_i2c_probe(pdev, adev, smi, node->instances);
> > > +               else
> > > +                       return smi_spi_probe(pdev, adev, smi, node->instances);
> > > +       default:
> > > +               break;
> >
> > Why is this needed?
>
> This return code is attempting to ensure that we don’t try to guess whether we
> expect devices to be I2C or SPI - especially with regards to existing devices.
> We wanted to maintain compatibility with existing devices, which would all be
> I2C.
> For the device for which we are adding, the same HID is used by both the same
> chip for both I2C and SPI, so we also needed a way to support both.

I meant why was the "default" case needed.  Sorry for the confusion.