Messages in this thread | | | Date | Mon, 13 Nov 2023 14:12:48 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v1 2/3] pinctrl: intel: Add a generic Intel pin control platform driver |
| |
On Fri, Nov 03, 2023 at 07:57:38AM +0200, Mika Westerberg wrote: > On Mon, Oct 30, 2023 at 04:10:33PM +0200, Andy Shevchenko wrote:
...
> > +config PINCTRL_INTEL_PLATFORM > > + tristate "Intel pinctrl and GPIO platform driver" > > + depends on ACPI > > + select PINCTRL_INTEL > > + help > > + This pinctrl driver provides an interface that allows configuring > > + of Intel PCH pins and using them as GPIOs. > > Add here some description that explains why this needs to be enabled, > for example for Lunar Lake. Now it is all too generic for distro folks > to understand if this is needed or not.
OK!
...
> > + * Copyright (C) 2021-2023, Intel Corporation > > That's 2023
As-is it is still valid and reflects the history.
...
> > + ngpps = device_get_child_node_count(dev); > > + if (ngpps == 0) > > if (!nggps)
0 is a plain number here (as count) and explicit comparison makes sense. But I'm okay with another form.
> > + return -ENODEV;
...
> > + ncommunities = 1, > > Why this is 1? Can't we have more communities?
As for now (version 1.0 of the specification) it's assumed that it's one community per device node in the ACPI, so I would leave this as is (we have also drivers with single community per device node, hence this is kinda pattern. Should I add a comment?
...
> > + struct device *dev = &pdev->dev; > > + struct intel_pinctrl_soc_data *data; > > > Change the ordering of the above: > > struct intel_pinctrl_soc_data *data; > struct device *dev = &pdev->dev;
Sure.
...
> > +static const struct acpi_device_id intel_platform_pinctrl_acpi_match[] = { > > + { } > > And add the _CID here in this patch as I commented in the last patch.
OK! I'll squash the next patch into this one.
> > +};
-- With Best Regards, Andy Shevchenko
| |