Re: [PATCH v6 07/10] hda: cs35l41: Add support for CS35L41 in HDA systems

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Lucas tanure <tanureal@opensource.cirrus.com>
Cc: "Rafael J . Wysocki" <rafael@kernel.org>,
	Len Brown <lenb@kernel.org>, Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <markgross@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Jaroslav Kysela <perex@perex.cz>, Mark Brown <broonie@kernel.org>,
	Takashi Iwai <tiwai@suse.com>,
	ALSA Development Mailing List <alsa-devel@alsa-project.org>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	patches@opensource.cirrus.com,
	Platform Driver <platform-driver-x86@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 07/10] hda: cs35l41: Add support for CS35L41 in HDA systems
Date: Thu, 13 Jan 2022 20:13:36 +0200	[thread overview]
Message-ID: <CAHp75Vc=x0AanRhkGHvbgs0M9VLdZfEFKJUxRTEMFvT5YwDtzA@mail.gmail.com> (raw)
In-Reply-To: <66351fd6-f434-775f-d8d5-2a6baf098269@opensource.cirrus.com>

On Thu, Jan 13, 2022 at 6:53 PM Lucas tanure
<tanureal@opensource.cirrus.com> wrote:
> On 1/6/22 12:29, Andy Shevchenko wrote:
> > On Fri, Dec 17, 2021 at 5:45 PM Lucas Tanure
> > <tanureal@opensource.cirrus.com> wrote:

...

> >> +        * Device CLSA0100 doesn't have _DSD so a gpiod_get by the label reset won't work.
> >
> > So, you need to add mapping tables and switch to regular APIs, tell
> > me, why it won't work.
> I will submit a patch series to fix most of the issues you pointed out.

Thanks!

> The part about how the driver access the ACPI table is going to be
> improved later if possible.
> The laptop has already shipped and doesn't have a _DSD node, so the
> driver needs to read the reset GPIO from a hard coded index inside a
> node that contains more than one cs35l41.

We have a lot of cases like this, hint: `git grep -n -w
devm_acpi_dev_add_driver_gpios`.

...

> >> +int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int irq,
> >> +                     struct regmap *regmap)
> >
> >> +       if (IS_ERR(regmap))
> >> +               return PTR_ERR(regmap);
> >
> > Why?
> It is up to the I2C/SPI module to create the regmap and provide to the
> main module. If that fails the main module can't continue.

So, this is band-aiding the issue, which is in the caller. Caller
shouldn't call this function without the regmap being ready-to-use.

...

> >> +       {"CLSA0100", 0 },
> >> +       {"CSC3551", 0 },
> >
> > I believe these IDs are officially allocated by the Cirrus Logic, right?
> CLSA010* is not a valid id for Cirrus Logic, but the Bios is already in
> production, so we must support it.
> CSC3551 is a valid id for Cirrus Logic.

Thank you for elaborating. Can we be sure that there won't be any
abuse of ACPI specification in the future (meaning all IDs will follow
the ACPI/PNP registries and be allocated by certain vendor)?

-- 
With Best Regards,
Andy Shevchenko