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

[Stefan Binding <sbinding@opensource.cirrus.com>]

Hi,

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