Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Jun 2023 21:18:39 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-i2c@vger.kernel.org, acpica-devel@lists.linuxfoundation.org,
        Len Brown <lenb@kernel.org>,
        Andi Shyti <andi.shyti@kernel.org>,
        Robert Moore <robert.moore@intel.com>,
        Michael Brunner <michael.brunner@kontron.com>
Subject: Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has
 resources
Message-ID: <ZJSQf07cO6qmNyCn@smile.fi.intel.com>
References: <20230621151652.79579-1-andriy.shevchenko@linux.intel.com>
 <CAJZ5v0jt8XCzUxQaBXLz0zXezih1Urq=dt-K9PWVY1JpN=Go6Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJZ5v0jt8XCzUxQaBXLz0zXezih1Urq=dt-K9PWVY1JpN=Go6Q@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Thu, Jun 22, 2023 at 05:53:13PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > After switching i2c-scmi driver to be a plaform one, it stopped
> 
> "platform"
> 
> > being enumerated on number of Kontron platforms, because it's
> > listed in the forbidden_id_list.
> >
> > To resolve the situation, split the list to generic one and
> > another that holds devices that has to be skipped if and only
> 
> "have"
> 
> > if they have bogus resources attached (_CRS method returns some).

Thanks for the typo fixes!

...

> > +static const struct acpi_device_id forbidden_id_with_resourses[] = {
> 
> I don't quite like this name and the driver_data field could be used
> to indicate the need to check the resources.

Okay, something like

/* Check if the device has resources provided by _CRS method */
#define ACPI_PLATFORM_CHECK_RES		BIT(0)

?

> > +       {"SMB0001",  0},        /* ACPI SMBUS virtual device */
> > +       { }
> > +};

...

> > +static int acpi_platform_resource_count(struct acpi_resource *ares, void *data)
> > +{
> > +       int *count = data;
> > +
> > +       *count = *count + 1;
> 
> Why not (*count)++?

Can be that way, I just copied'n'pasted from the existing code.

> > +       return 1;
> > +}

...

> >         INIT_LIST_HEAD(&resource_list);
> > +       ret = acpi_dev_get_resources(adev, &resource_list, acpi_platform_resource_count, &count);
> > +       if (ret < 0)
> > +               return ERR_PTR(ret);
> 
> Why not use acpi_walk_resources() directly here?

Can be done that way. Again, I just used a template (existing code in kernel)
for similar functionality.

> Also, this extra resources walk is only needed if the resources check
> is needed to decide whether or not to skip the device, so what's the
> benefit of doing it for every device that's not skipped?

Ah, indeed. Makes sense to have done it conditionally.

> > +       acpi_dev_free_resource_list(&resource_list);

...

Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko