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

[Rafael J. Wysocki]

On Fri, Jan 21, 2022 at 5:55 PM Stefan Binding
<sbinding at opensource.cirrus.com> wrote:
>
> Hi,
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <rafael at kernel.org>
> > Sent: 21 January 2022 15:31
> > To: Stefan Binding <sbinding at opensource.cirrus.com>
> > Cc: Mark Brown <broonie at kernel.org>; Rafael J . Wysocki
> > <rafael at kernel.org>; Len Brown <lenb at kernel.org>; Hans de Goede
> > <hdegoede at redhat.com>; Mark Gross <markgross at kernel.org>; Jaroslav
> > Kysela <perex at perex.cz>; Takashi Iwai <tiwai at suse.com>; moderated
> > list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM... <alsa-
> > devel at alsa-project.org>; Linux Kernel Mailing List <linux-
> > kernel at vger.kernel.org>; linux-spi <linux-spi at vger.kernel.org>; ACPI Devel
> > Maling List <linux-acpi at vger.kernel.org>; Platform Driver <platform-driver-
> > x86 at vger.kernel.org>; patches at opensource.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.