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

[Stefan Binding]

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.

> > @@ -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.

Thanks,
Stefan