* Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity
2015-09-03 19:14 ` [PATCH] mfd: lpc_ich: Separate device cells for clarity Aaron Sierra
@ 2015-09-17 16:17 ` Andy Shevchenko
2015-09-19 10:13 ` Lee Jones
2015-09-19 10:16 ` Lee Jones
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2015-09-17 16:17 UTC (permalink / raw)
To: Aaron Sierra, Lee Jones
Cc: linux-kernel, Peter Tyser, Guenter Roeck, Jean Delvare,
Samuel Ortiz, Mika Westerberg, Matt Fleming
On Thu, 2015-09-03 at 14:14 -0500, Aaron Sierra wrote:
> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.
>
> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
>
Looks good for me.
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
> drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++-----------------------
> -
> 1 file changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8de3439..7a01d430 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> },
> };
>
> -enum lpc_cells {
> - LPC_WDT = 0,
> - LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> + .name = "iTCO_wdt",
> + .num_resources = ARRAY_SIZE(wdt_ich_res),
> + .resources = wdt_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> -static struct mfd_cell lpc_ich_cells[] = {
> - [LPC_WDT] = {
> - .name = "iTCO_wdt",
> - .num_resources = ARRAY_SIZE(wdt_ich_res),
> - .resources = wdt_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> - [LPC_GPIO] = {
> - .name = "gpio_ich",
> - .num_resources = ARRAY_SIZE(gpio_ich_res),
> - .resources = gpio_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> +static struct mfd_cell lpc_ich_gpio_cell = {
> + .name = "gpio_ich",
> + .num_resources = ARRAY_SIZE(gpio_ich_res),
> + .resources = gpio_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> /* chipset related info */
> @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> base_addr = base_addr_cfg & 0x0000ff80;
> if (!base_addr) {
> dev_notice(&dev->dev, "I/O space for ACPI
> uninitialized\n");
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> goto gpe0_done;
> }
>
> @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> * the platform_device subsystem doesn't see this
> resource
> * or it will register an invalid region.
> */
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> acpi_conflict = true;
> } else {
> lpc_ich_enable_acpi_space(dev);
> @@ -933,14 +927,14 @@ gpe0_done:
> lpc_chipset_info[priv->chipset].use_gpio = ret;
> lpc_ich_enable_gpio_space(dev);
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> + lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_GPIO], 1, NULL, 0,
> NULL);
> + &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>
> gpio_done:
> if (acpi_conflict)
> pr_warn("Resource conflict(s) found affecting %s\n",
> - lpc_ich_cells[LPC_GPIO].name);
> + lpc_ich_gpio_cell.name);
> return ret;
> }
>
> @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> */
> if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> /* Don't register iomem for TCO ver 1 */
> - lpc_ich_cells[LPC_WDT].num_resources--;
> + lpc_ich_wdt_cell.num_resources--;
> } else if (lpc_chipset_info[priv->chipset].iTCO_version ==
> 2) {
> pci_read_config_dword(dev, RCBABASE,
> &base_addr_cfg);
> base_addr = base_addr_cfg & 0xffffc000;
> @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev
> *dev)
> res->end = base_addr + ACPIBASE_PMC_END;
> }
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> + lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_WDT], 1, NULL, 0,
> NULL);
> + &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>
> wdt_done:
> return ret;
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity
2015-09-17 16:17 ` Andy Shevchenko
@ 2015-09-19 10:13 ` Lee Jones
2015-09-22 11:36 ` Andy Shevchenko
0 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2015-09-19 10:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Aaron Sierra, linux-kernel, Peter Tyser, Guenter Roeck,
Jean Delvare, Samuel Ortiz, Mika Westerberg, Matt Fleming
On Thu, 17 Sep 2015, Andy Shevchenko wrote:
> On Thu, 2015-09-03 at 14:14 -0500, Aaron Sierra wrote:
> > The lpc_ich_cells array gives the wrong impression about the
> > relationship between the watchdog and GPIO devices. They are
> > completely distinct devices, so this patch separates the
> > array into distinct mfd_cell structs per device.
> >
> > A side effect of removing the array, is that the lpc_cells enum
> > is no longer needed.
> >
>
> Looks good for me.
Is that an Ack? If so, can you formally provide one please?
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> > drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++-----------------------
> > -
> > 1 file changed, 18 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > index 8de3439..7a01d430 100644
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> > },
> > };
> >
> > -enum lpc_cells {
> > - LPC_WDT = 0,
> > - LPC_GPIO,
> > +static struct mfd_cell lpc_ich_wdt_cell = {
> > + .name = "iTCO_wdt",
> > + .num_resources = ARRAY_SIZE(wdt_ich_res),
> > + .resources = wdt_ich_res,
> > + .ignore_resource_conflicts = true,
> > };
> >
> > -static struct mfd_cell lpc_ich_cells[] = {
> > - [LPC_WDT] = {
> > - .name = "iTCO_wdt",
> > - .num_resources = ARRAY_SIZE(wdt_ich_res),
> > - .resources = wdt_ich_res,
> > - .ignore_resource_conflicts = true,
> > - },
> > - [LPC_GPIO] = {
> > - .name = "gpio_ich",
> > - .num_resources = ARRAY_SIZE(gpio_ich_res),
> > - .resources = gpio_ich_res,
> > - .ignore_resource_conflicts = true,
> > - },
> > +static struct mfd_cell lpc_ich_gpio_cell = {
> > + .name = "gpio_ich",
> > + .num_resources = ARRAY_SIZE(gpio_ich_res),
> > + .resources = gpio_ich_res,
> > + .ignore_resource_conflicts = true,
> > };
> >
> > /* chipset related info */
> > @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> > base_addr = base_addr_cfg & 0x0000ff80;
> > if (!base_addr) {
> > dev_notice(&dev->dev, "I/O space for ACPI
> > uninitialized\n");
> > - lpc_ich_cells[LPC_GPIO].num_resources--;
> > + lpc_ich_gpio_cell.num_resources--;
> > goto gpe0_done;
> > }
> >
> > @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> > * the platform_device subsystem doesn't see this
> > resource
> > * or it will register an invalid region.
> > */
> > - lpc_ich_cells[LPC_GPIO].num_resources--;
> > + lpc_ich_gpio_cell.num_resources--;
> > acpi_conflict = true;
> > } else {
> > lpc_ich_enable_acpi_space(dev);
> > @@ -933,14 +927,14 @@ gpe0_done:
> > lpc_chipset_info[priv->chipset].use_gpio = ret;
> > lpc_ich_enable_gpio_space(dev);
> >
> > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > + lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > - &lpc_ich_cells[LPC_GPIO], 1, NULL, 0,
> > NULL);
> > + &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
> >
> > gpio_done:
> > if (acpi_conflict)
> > pr_warn("Resource conflict(s) found affecting %s\n",
> > - lpc_ich_cells[LPC_GPIO].name);
> > + lpc_ich_gpio_cell.name);
> > return ret;
> > }
> >
> > @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> > */
> > if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> > /* Don't register iomem for TCO ver 1 */
> > - lpc_ich_cells[LPC_WDT].num_resources--;
> > + lpc_ich_wdt_cell.num_resources--;
> > } else if (lpc_chipset_info[priv->chipset].iTCO_version ==
> > 2) {
> > pci_read_config_dword(dev, RCBABASE,
> > &base_addr_cfg);
> > base_addr = base_addr_cfg & 0xffffc000;
> > @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev
> > *dev)
> > res->end = base_addr + ACPIBASE_PMC_END;
> > }
> >
> > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > + lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > - &lpc_ich_cells[LPC_WDT], 1, NULL, 0,
> > NULL);
> > + &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
> >
> > wdt_done:
> > return ret;
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity
2015-09-19 10:13 ` Lee Jones
@ 2015-09-22 11:36 ` Andy Shevchenko
0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2015-09-22 11:36 UTC (permalink / raw)
To: Lee Jones
Cc: Aaron Sierra, linux-kernel, Peter Tyser, Guenter Roeck,
Jean Delvare, Samuel Ortiz, Mika Westerberg, Matt Fleming
On Sat, 2015-09-19 at 11:13 +0100, Lee Jones wrote:
> On Thu, 17 Sep 2015, Andy Shevchenko wrote:
>
> > On Thu, 2015-09-03 at 14:14 -0500, Aaron Sierra wrote:
> > > The lpc_ich_cells array gives the wrong impression about the
> > > relationship between the watchdog and GPIO devices. They are
> > > completely distinct devices, so this patch separates the
> > > array into distinct mfd_cell structs per device.
> > >
> > > A side effect of removing the array, is that the lpc_cells enum
> > > is no longer needed.
> > >
> >
> > Looks good for me.
>
> Is that an Ack? If so, can you formally provide one please?
Sorry, might be late. Was too busy by some hw quirks yesterday.
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> > > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > > ---
> > > drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++-------------------
> > > ----
> > > -
> > > 1 file changed, 18 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > > index 8de3439..7a01d430 100644
> > > --- a/drivers/mfd/lpc_ich.c
> > > +++ b/drivers/mfd/lpc_ich.c
> > > @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> > > },
> > > };
> > >
> > > -enum lpc_cells {
> > > - LPC_WDT = 0,
> > > - LPC_GPIO,
> > > +static struct mfd_cell lpc_ich_wdt_cell = {
> > > + .name = "iTCO_wdt",
> > > + .num_resources = ARRAY_SIZE(wdt_ich_res),
> > > + .resources = wdt_ich_res,
> > > + .ignore_resource_conflicts = true,
> > > };
> > >
> > > -static struct mfd_cell lpc_ich_cells[] = {
> > > - [LPC_WDT] = {
> > > - .name = "iTCO_wdt",
> > > - .num_resources = ARRAY_SIZE(wdt_ich_res),
> > > - .resources = wdt_ich_res,
> > > - .ignore_resource_conflicts = true,
> > > - },
> > > - [LPC_GPIO] = {
> > > - .name = "gpio_ich",
> > > - .num_resources = ARRAY_SIZE(gpio_ich_res),
> > > - .resources = gpio_ich_res,
> > > - .ignore_resource_conflicts = true,
> > > - },
> > > +static struct mfd_cell lpc_ich_gpio_cell = {
> > > + .name = "gpio_ich",
> > > + .num_resources = ARRAY_SIZE(gpio_ich_res),
> > > + .resources = gpio_ich_res,
> > > + .ignore_resource_conflicts = true,
> > > };
> > >
> > > /* chipset related info */
> > > @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev
> > > *dev)
> > > base_addr = base_addr_cfg & 0x0000ff80;
> > > if (!base_addr) {
> > > dev_notice(&dev->dev, "I/O space for ACPI
> > > uninitialized\n");
> > > - lpc_ich_cells[LPC_GPIO].num_resources--;
> > > + lpc_ich_gpio_cell.num_resources--;
> > > goto gpe0_done;
> > > }
> > >
> > > @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev
> > > *dev)
> > > * the platform_device subsystem doesn't see
> > > this
> > > resource
> > > * or it will register an invalid region.
> > > */
> > > - lpc_ich_cells[LPC_GPIO].num_resources--;
> > > + lpc_ich_gpio_cell.num_resources--;
> > > acpi_conflict = true;
> > > } else {
> > > lpc_ich_enable_acpi_space(dev);
> > > @@ -933,14 +927,14 @@ gpe0_done:
> > > lpc_chipset_info[priv->chipset].use_gpio = ret;
> > > lpc_ich_enable_gpio_space(dev);
> > >
> > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > > + lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > - &lpc_ich_cells[LPC_GPIO], 1, NULL,
> > > 0,
> > > NULL);
> > > + &lpc_ich_gpio_cell, 1, NULL, 0,
> > > NULL);
> > >
> > > gpio_done:
> > > if (acpi_conflict)
> > > pr_warn("Resource conflict(s) found affecting
> > > %s\n",
> > > - lpc_ich_cells[LPC_GPIO].name);
> > > + lpc_ich_gpio_cell.name);
> > > return ret;
> > > }
> > >
> > > @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev
> > > *dev)
> > > */
> > > if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> > > /* Don't register iomem for TCO ver 1 */
> > > - lpc_ich_cells[LPC_WDT].num_resources--;
> > > + lpc_ich_wdt_cell.num_resources--;
> > > } else if (lpc_chipset_info[priv->chipset].iTCO_version
> > > ==
> > > 2) {
> > > pci_read_config_dword(dev, RCBABASE,
> > > &base_addr_cfg);
> > > base_addr = base_addr_cfg & 0xffffc000;
> > > @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev
> > > *dev)
> > > res->end = base_addr + ACPIBASE_PMC_END;
> > > }
> > >
> > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > > + lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > - &lpc_ich_cells[LPC_WDT], 1, NULL,
> > > 0,
> > > NULL);
> > > + &lpc_ich_wdt_cell, 1, NULL, 0,
> > > NULL);
> > >
> > > wdt_done:
> > > return ret;
> >
>
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity
2015-09-03 19:14 ` [PATCH] mfd: lpc_ich: Separate device cells for clarity Aaron Sierra
2015-09-17 16:17 ` Andy Shevchenko
@ 2015-09-19 10:16 ` Lee Jones
2015-09-21 17:37 ` Aaron Sierra
2015-09-21 23:13 ` Lee Jones
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2015-09-19 10:16 UTC (permalink / raw)
To: Aaron Sierra
Cc: linux-kernel, Peter Tyser, Guenter Roeck, Jean Delvare,
Samuel Ortiz, Andy Shevchenko, Mika Westerberg, Matt Fleming
On Thu, 03 Sep 2015, Aaron Sierra wrote:
> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.
What does this mean? In what way are they distinct?
> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
> drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
> 1 file changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8de3439..7a01d430 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> },
> };
>
> -enum lpc_cells {
> - LPC_WDT = 0,
> - LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> + .name = "iTCO_wdt",
> + .num_resources = ARRAY_SIZE(wdt_ich_res),
> + .resources = wdt_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> -static struct mfd_cell lpc_ich_cells[] = {
> - [LPC_WDT] = {
> - .name = "iTCO_wdt",
> - .num_resources = ARRAY_SIZE(wdt_ich_res),
> - .resources = wdt_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> - [LPC_GPIO] = {
> - .name = "gpio_ich",
> - .num_resources = ARRAY_SIZE(gpio_ich_res),
> - .resources = gpio_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> +static struct mfd_cell lpc_ich_gpio_cell = {
> + .name = "gpio_ich",
> + .num_resources = ARRAY_SIZE(gpio_ich_res),
> + .resources = gpio_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> /* chipset related info */
> @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> base_addr = base_addr_cfg & 0x0000ff80;
> if (!base_addr) {
> dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> goto gpe0_done;
> }
>
> @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> * the platform_device subsystem doesn't see this resource
> * or it will register an invalid region.
> */
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> acpi_conflict = true;
> } else {
> lpc_ich_enable_acpi_space(dev);
> @@ -933,14 +927,14 @@ gpe0_done:
> lpc_chipset_info[priv->chipset].use_gpio = ret;
> lpc_ich_enable_gpio_space(dev);
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> + lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> + &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>
> gpio_done:
> if (acpi_conflict)
> pr_warn("Resource conflict(s) found affecting %s\n",
> - lpc_ich_cells[LPC_GPIO].name);
> + lpc_ich_gpio_cell.name);
> return ret;
> }
>
> @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> */
> if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> /* Don't register iomem for TCO ver 1 */
> - lpc_ich_cells[LPC_WDT].num_resources--;
> + lpc_ich_wdt_cell.num_resources--;
> } else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
> pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
> base_addr = base_addr_cfg & 0xffffc000;
> @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> res->end = base_addr + ACPIBASE_PMC_END;
> }
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> + lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> + &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>
> wdt_done:
> return ret;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity
2015-09-19 10:16 ` Lee Jones
@ 2015-09-21 17:37 ` Aaron Sierra
0 siblings, 0 replies; 11+ messages in thread
From: Aaron Sierra @ 2015-09-21 17:37 UTC (permalink / raw)
To: Lee Jones
Cc: linux-kernel, Peter Tyser, Guenter Roeck, Jean Delvare,
Samuel Ortiz, Andy Shevchenko, Mika Westerberg, Matt Fleming
----- Original Message -----
> From: "Lee Jones" <lee.jones@linaro.org>
> Sent: Saturday, September 19, 2015 5:16:06 AM
>
> On Thu, 03 Sep 2015, Aaron Sierra wrote:
>
> > The lpc_ich_cells array gives the wrong impression about the
> > relationship between the watchdog and GPIO devices. They are
> > completely distinct devices, so this patch separates the
> > array into distinct mfd_cell structs per device.
>
> What does this mean? In what way are they distinct?
Lee,
This issue was discussed in late July regarding this patch Matt Fleming submitted:
[PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform data
You asked why there were multiple calls to mfd_add_devices() in the driver.
The reason that I provided was that the WDT cells can be registered even
if the GPIO cells cannot. And vice versa.
Having their cells stored in the same array added to the illusion that a
failure preparing/registering the cells of one should cause all cells to
be unregistered.
-Aaron
> > A side effect of removing the array, is that the lpc_cells enum
> > is no longer needed.
> >
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> > drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
> > 1 file changed, 18 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > index 8de3439..7a01d430 100644
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> > },
> > };
> >
> > -enum lpc_cells {
> > - LPC_WDT = 0,
> > - LPC_GPIO,
> > +static struct mfd_cell lpc_ich_wdt_cell = {
> > + .name = "iTCO_wdt",
> > + .num_resources = ARRAY_SIZE(wdt_ich_res),
> > + .resources = wdt_ich_res,
> > + .ignore_resource_conflicts = true,
> > };
> >
> > -static struct mfd_cell lpc_ich_cells[] = {
> > - [LPC_WDT] = {
> > - .name = "iTCO_wdt",
> > - .num_resources = ARRAY_SIZE(wdt_ich_res),
> > - .resources = wdt_ich_res,
> > - .ignore_resource_conflicts = true,
> > - },
> > - [LPC_GPIO] = {
> > - .name = "gpio_ich",
> > - .num_resources = ARRAY_SIZE(gpio_ich_res),
> > - .resources = gpio_ich_res,
> > - .ignore_resource_conflicts = true,
> > - },
> > +static struct mfd_cell lpc_ich_gpio_cell = {
> > + .name = "gpio_ich",
> > + .num_resources = ARRAY_SIZE(gpio_ich_res),
> > + .resources = gpio_ich_res,
> > + .ignore_resource_conflicts = true,
> > };
> >
> > /* chipset related info */
> > @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> > base_addr = base_addr_cfg & 0x0000ff80;
> > if (!base_addr) {
> > dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> > - lpc_ich_cells[LPC_GPIO].num_resources--;
> > + lpc_ich_gpio_cell.num_resources--;
> > goto gpe0_done;
> > }
> >
> > @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> > * the platform_device subsystem doesn't see this resource
> > * or it will register an invalid region.
> > */
> > - lpc_ich_cells[LPC_GPIO].num_resources--;
> > + lpc_ich_gpio_cell.num_resources--;
> > acpi_conflict = true;
> > } else {
> > lpc_ich_enable_acpi_space(dev);
> > @@ -933,14 +927,14 @@ gpe0_done:
> > lpc_chipset_info[priv->chipset].use_gpio = ret;
> > lpc_ich_enable_gpio_space(dev);
> >
> > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > + lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > - &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> > + &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
> >
> > gpio_done:
> > if (acpi_conflict)
> > pr_warn("Resource conflict(s) found affecting %s\n",
> > - lpc_ich_cells[LPC_GPIO].name);
> > + lpc_ich_gpio_cell.name);
> > return ret;
> > }
> >
> > @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> > */
> > if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> > /* Don't register iomem for TCO ver 1 */
> > - lpc_ich_cells[LPC_WDT].num_resources--;
> > + lpc_ich_wdt_cell.num_resources--;
> > } else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
> > pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
> > base_addr = base_addr_cfg & 0xffffc000;
> > @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> > res->end = base_addr + ACPIBASE_PMC_END;
> > }
> >
> > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > + lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > - &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> > + &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
> >
> > wdt_done:
> > return ret;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity
2015-09-03 19:14 ` [PATCH] mfd: lpc_ich: Separate device cells for clarity Aaron Sierra
2015-09-17 16:17 ` Andy Shevchenko
2015-09-19 10:16 ` Lee Jones
@ 2015-09-21 23:13 ` Lee Jones
2015-09-22 22:41 ` Lee Jones
2015-09-23 0:04 ` [PATCH v2] " Aaron Sierra
4 siblings, 0 replies; 11+ messages in thread
From: Lee Jones @ 2015-09-21 23:13 UTC (permalink / raw)
To: Aaron Sierra
Cc: linux-kernel, Peter Tyser, Guenter Roeck, Jean Delvare,
Samuel Ortiz, Andy Shevchenko, Mika Westerberg, Matt Fleming
On Thu, 03 Sep 2015, Aaron Sierra wrote:
> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.
>
> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
> drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
> 1 file changed, 18 insertions(+), 24 deletions(-)
Applied, thanks.
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8de3439..7a01d430 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> },
> };
>
> -enum lpc_cells {
> - LPC_WDT = 0,
> - LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> + .name = "iTCO_wdt",
> + .num_resources = ARRAY_SIZE(wdt_ich_res),
> + .resources = wdt_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> -static struct mfd_cell lpc_ich_cells[] = {
> - [LPC_WDT] = {
> - .name = "iTCO_wdt",
> - .num_resources = ARRAY_SIZE(wdt_ich_res),
> - .resources = wdt_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> - [LPC_GPIO] = {
> - .name = "gpio_ich",
> - .num_resources = ARRAY_SIZE(gpio_ich_res),
> - .resources = gpio_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> +static struct mfd_cell lpc_ich_gpio_cell = {
> + .name = "gpio_ich",
> + .num_resources = ARRAY_SIZE(gpio_ich_res),
> + .resources = gpio_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> /* chipset related info */
> @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> base_addr = base_addr_cfg & 0x0000ff80;
> if (!base_addr) {
> dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> goto gpe0_done;
> }
>
> @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> * the platform_device subsystem doesn't see this resource
> * or it will register an invalid region.
> */
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> acpi_conflict = true;
> } else {
> lpc_ich_enable_acpi_space(dev);
> @@ -933,14 +927,14 @@ gpe0_done:
> lpc_chipset_info[priv->chipset].use_gpio = ret;
> lpc_ich_enable_gpio_space(dev);
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> + lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> + &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>
> gpio_done:
> if (acpi_conflict)
> pr_warn("Resource conflict(s) found affecting %s\n",
> - lpc_ich_cells[LPC_GPIO].name);
> + lpc_ich_gpio_cell.name);
> return ret;
> }
>
> @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> */
> if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> /* Don't register iomem for TCO ver 1 */
> - lpc_ich_cells[LPC_WDT].num_resources--;
> + lpc_ich_wdt_cell.num_resources--;
> } else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
> pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
> base_addr = base_addr_cfg & 0xffffc000;
> @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> res->end = base_addr + ACPIBASE_PMC_END;
> }
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> + lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> + &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>
> wdt_done:
> return ret;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity
2015-09-03 19:14 ` [PATCH] mfd: lpc_ich: Separate device cells for clarity Aaron Sierra
` (2 preceding siblings ...)
2015-09-21 23:13 ` Lee Jones
@ 2015-09-22 22:41 ` Lee Jones
2015-09-23 0:04 ` [PATCH v2] " Aaron Sierra
4 siblings, 0 replies; 11+ messages in thread
From: Lee Jones @ 2015-09-22 22:41 UTC (permalink / raw)
To: Aaron Sierra
Cc: linux-kernel, Peter Tyser, Guenter Roeck, Jean Delvare,
Samuel Ortiz, Andy Shevchenko, Mika Westerberg, Matt Fleming
On Thu, 03 Sep 2015, Aaron Sierra wrote:
> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.
>
> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
> drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
> 1 file changed, 18 insertions(+), 24 deletions(-)
You need to rebase this. It doesn't apply.
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8de3439..7a01d430 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> },
> };
>
> -enum lpc_cells {
> - LPC_WDT = 0,
> - LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> + .name = "iTCO_wdt",
> + .num_resources = ARRAY_SIZE(wdt_ich_res),
> + .resources = wdt_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> -static struct mfd_cell lpc_ich_cells[] = {
> - [LPC_WDT] = {
> - .name = "iTCO_wdt",
> - .num_resources = ARRAY_SIZE(wdt_ich_res),
> - .resources = wdt_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> - [LPC_GPIO] = {
> - .name = "gpio_ich",
> - .num_resources = ARRAY_SIZE(gpio_ich_res),
> - .resources = gpio_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> +static struct mfd_cell lpc_ich_gpio_cell = {
> + .name = "gpio_ich",
> + .num_resources = ARRAY_SIZE(gpio_ich_res),
> + .resources = gpio_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> /* chipset related info */
> @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> base_addr = base_addr_cfg & 0x0000ff80;
> if (!base_addr) {
> dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> goto gpe0_done;
> }
>
> @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> * the platform_device subsystem doesn't see this resource
> * or it will register an invalid region.
> */
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> acpi_conflict = true;
> } else {
> lpc_ich_enable_acpi_space(dev);
> @@ -933,14 +927,14 @@ gpe0_done:
> lpc_chipset_info[priv->chipset].use_gpio = ret;
> lpc_ich_enable_gpio_space(dev);
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> + lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> + &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>
> gpio_done:
> if (acpi_conflict)
> pr_warn("Resource conflict(s) found affecting %s\n",
> - lpc_ich_cells[LPC_GPIO].name);
> + lpc_ich_gpio_cell.name);
> return ret;
> }
>
> @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> */
> if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> /* Don't register iomem for TCO ver 1 */
> - lpc_ich_cells[LPC_WDT].num_resources--;
> + lpc_ich_wdt_cell.num_resources--;
> } else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
> pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
> base_addr = base_addr_cfg & 0xffffc000;
> @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> res->end = base_addr + ACPIBASE_PMC_END;
> }
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> + lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> + &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>
> wdt_done:
> return ret;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2] mfd: lpc_ich: Separate device cells for clarity
2015-09-03 19:14 ` [PATCH] mfd: lpc_ich: Separate device cells for clarity Aaron Sierra
` (3 preceding siblings ...)
2015-09-22 22:41 ` Lee Jones
@ 2015-09-23 0:04 ` Aaron Sierra
2015-09-23 0:31 ` Lee Jones
4 siblings, 1 reply; 11+ messages in thread
From: Aaron Sierra @ 2015-09-23 0:04 UTC (permalink / raw)
To: Lee Jones
Cc: linux-kernel, Peter Tyser, Guenter Roeck, Jean Delvare,
Samuel Ortiz, Andy Shevchenko, Mika Westerberg, Matt Fleming
The lpc_ich_cells array gives the wrong impression about the
relationship between the watchdog and GPIO devices. They are
completely distinct devices, so this patch separates the
array into distinct mfd_cell structs per device.
A side effect of removing the array, is that the lpc_cells enum
is no longer needed.
Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
v2 - rebase onto 4.3-rc2
drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index c5a9a08..b514f3c 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -132,24 +132,18 @@ static struct resource gpio_ich_res[] = {
},
};
-enum lpc_cells {
- LPC_WDT = 0,
- LPC_GPIO,
+static struct mfd_cell lpc_ich_wdt_cell = {
+ .name = "iTCO_wdt",
+ .num_resources = ARRAY_SIZE(wdt_ich_res),
+ .resources = wdt_ich_res,
+ .ignore_resource_conflicts = true,
};
-static struct mfd_cell lpc_ich_cells[] = {
- [LPC_WDT] = {
- .name = "iTCO_wdt",
- .num_resources = ARRAY_SIZE(wdt_ich_res),
- .resources = wdt_ich_res,
- .ignore_resource_conflicts = true,
- },
- [LPC_GPIO] = {
- .name = "gpio_ich",
- .num_resources = ARRAY_SIZE(gpio_ich_res),
- .resources = gpio_ich_res,
- .ignore_resource_conflicts = true,
- },
+static struct mfd_cell lpc_ich_gpio_cell = {
+ .name = "gpio_ich",
+ .num_resources = ARRAY_SIZE(gpio_ich_res),
+ .resources = gpio_ich_res,
+ .ignore_resource_conflicts = true,
};
/* chipset related info */
@@ -841,7 +835,7 @@ static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
struct itco_wdt_platform_data *pdata;
struct lpc_ich_priv *priv = pci_get_drvdata(dev);
struct lpc_ich_info *info;
- struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
+ struct mfd_cell *cell = &lpc_ich_wdt_cell;
pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
@@ -860,7 +854,7 @@ static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
{
struct lpc_ich_priv *priv = pci_get_drvdata(dev);
- struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
+ struct mfd_cell *cell = &lpc_ich_gpio_cell;
cell->platform_data = &lpc_chipset_info[priv->chipset];
cell->pdata_size = sizeof(struct lpc_ich_info);
@@ -904,7 +898,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
base_addr = base_addr_cfg & 0x0000ff80;
if (!base_addr) {
dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
- lpc_ich_cells[LPC_GPIO].num_resources--;
+ lpc_ich_gpio_cell.num_resources--;
goto gpe0_done;
}
@@ -918,7 +912,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
* the platform_device subsystem doesn't see this resource
* or it will register an invalid region.
*/
- lpc_ich_cells[LPC_GPIO].num_resources--;
+ lpc_ich_gpio_cell.num_resources--;
acpi_conflict = true;
} else {
lpc_ich_enable_acpi_space(dev);
@@ -958,12 +952,12 @@ gpe0_done:
lpc_ich_finalize_gpio_cell(dev);
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
- &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
+ &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
gpio_done:
if (acpi_conflict)
pr_warn("Resource conflict(s) found affecting %s\n",
- lpc_ich_cells[LPC_GPIO].name);
+ lpc_ich_gpio_cell.name);
return ret;
}
@@ -1007,7 +1001,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
*/
if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
/* Don't register iomem for TCO ver 1 */
- lpc_ich_cells[LPC_WDT].num_resources--;
+ lpc_ich_wdt_cell.num_resources--;
} else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
base_addr = base_addr_cfg & 0xffffc000;
@@ -1035,7 +1029,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
goto wdt_done;
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
- &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
+ &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
wdt_done:
return ret;
--
1.9.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2] mfd: lpc_ich: Separate device cells for clarity
2015-09-23 0:04 ` [PATCH v2] " Aaron Sierra
@ 2015-09-23 0:31 ` Lee Jones
2015-09-23 14:13 ` Aaron Sierra
0 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2015-09-23 0:31 UTC (permalink / raw)
To: Aaron Sierra
Cc: linux-kernel, Peter Tyser, Guenter Roeck, Jean Delvare,
Samuel Ortiz, Andy Shevchenko, Mika Westerberg, Matt Fleming
On Tue, 22 Sep 2015, Aaron Sierra wrote:
> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.
>
> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
> v2 - rebase onto 4.3-rc2
>
> drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
> 1 file changed, 18 insertions(+), 24 deletions(-)
Looks like you didn't apply Andy's Ack!
Applied with Andy's Ack.
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index c5a9a08..b514f3c 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -132,24 +132,18 @@ static struct resource gpio_ich_res[] = {
> },
> };
>
> -enum lpc_cells {
> - LPC_WDT = 0,
> - LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> + .name = "iTCO_wdt",
> + .num_resources = ARRAY_SIZE(wdt_ich_res),
> + .resources = wdt_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> -static struct mfd_cell lpc_ich_cells[] = {
> - [LPC_WDT] = {
> - .name = "iTCO_wdt",
> - .num_resources = ARRAY_SIZE(wdt_ich_res),
> - .resources = wdt_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> - [LPC_GPIO] = {
> - .name = "gpio_ich",
> - .num_resources = ARRAY_SIZE(gpio_ich_res),
> - .resources = gpio_ich_res,
> - .ignore_resource_conflicts = true,
> - },
> +static struct mfd_cell lpc_ich_gpio_cell = {
> + .name = "gpio_ich",
> + .num_resources = ARRAY_SIZE(gpio_ich_res),
> + .resources = gpio_ich_res,
> + .ignore_resource_conflicts = true,
> };
>
> /* chipset related info */
> @@ -841,7 +835,7 @@ static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
> struct itco_wdt_platform_data *pdata;
> struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> struct lpc_ich_info *info;
> - struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> + struct mfd_cell *cell = &lpc_ich_wdt_cell;
>
> pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
> if (!pdata)
> @@ -860,7 +854,7 @@ static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
> static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
> {
> struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> - struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
> + struct mfd_cell *cell = &lpc_ich_gpio_cell;
>
> cell->platform_data = &lpc_chipset_info[priv->chipset];
> cell->pdata_size = sizeof(struct lpc_ich_info);
> @@ -904,7 +898,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> base_addr = base_addr_cfg & 0x0000ff80;
> if (!base_addr) {
> dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> goto gpe0_done;
> }
>
> @@ -918,7 +912,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> * the platform_device subsystem doesn't see this resource
> * or it will register an invalid region.
> */
> - lpc_ich_cells[LPC_GPIO].num_resources--;
> + lpc_ich_gpio_cell.num_resources--;
> acpi_conflict = true;
> } else {
> lpc_ich_enable_acpi_space(dev);
> @@ -958,12 +952,12 @@ gpe0_done:
>
> lpc_ich_finalize_gpio_cell(dev);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> + &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>
> gpio_done:
> if (acpi_conflict)
> pr_warn("Resource conflict(s) found affecting %s\n",
> - lpc_ich_cells[LPC_GPIO].name);
> + lpc_ich_gpio_cell.name);
> return ret;
> }
>
> @@ -1007,7 +1001,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> */
> if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> /* Don't register iomem for TCO ver 1 */
> - lpc_ich_cells[LPC_WDT].num_resources--;
> + lpc_ich_wdt_cell.num_resources--;
> } else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
> pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
> base_addr = base_addr_cfg & 0xffffc000;
> @@ -1035,7 +1029,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> goto wdt_done;
>
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> - &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> + &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>
> wdt_done:
> return ret;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] mfd: lpc_ich: Separate device cells for clarity
2015-09-23 0:31 ` Lee Jones
@ 2015-09-23 14:13 ` Aaron Sierra
0 siblings, 0 replies; 11+ messages in thread
From: Aaron Sierra @ 2015-09-23 14:13 UTC (permalink / raw)
To: Lee Jones, Andy Shevchenko
Cc: linux-kernel, Peter Tyser, Guenter Roeck, Jean Delvare,
Samuel Ortiz, Mika Westerberg, Matt Fleming
----- Original Message -----
> From: "Lee Jones" <lee.jones@linaro.org>
> Sent: Tuesday, September 22, 2015 7:31:19 PM
>
> On Tue, 22 Sep 2015, Aaron Sierra wrote:
>
> > The lpc_ich_cells array gives the wrong impression about the
> > relationship between the watchdog and GPIO devices. They are
> > completely distinct devices, so this patch separates the
> > array into distinct mfd_cell structs per device.
> >
> > A side effect of removing the array, is that the lpc_cells enum
> > is no longer needed.
> >
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> > v2 - rebase onto 4.3-rc2
> >
> > drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
> > 1 file changed, 18 insertions(+), 24 deletions(-)
>
> Looks like you didn't apply Andy's Ack!
Sorry, Andy! I erred on the side of caution. I wasn't sure if
the Ack applied to the concept or the iteration of the patch.
> Applied with Andy's Ack.
Thanks
-Aaron
^ permalink raw reply [flat|nested] 11+ messages in thread