Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1228425 > unrolled thread
| Started by | Lee Jones <lee.jones@linaro.org> |
|---|---|
| First post | 2015-09-19 12:20 +0200 |
| Last post | 2015-09-21 19:50 +0200 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity Lee Jones <lee.jones@linaro.org> - 2015-09-19 12:20 +0200
Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity Aaron Sierra <asierra@xes-inc.com> - 2015-09-21 19:50 +0200
| From | Lee Jones <lee.jones@linaro.org> |
|---|---|
| Date | 2015-09-19 12:20 +0200 |
| Subject | Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity |
| Message-ID | <qanpo-6SK-37@gated-at.bofh.it> |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Aaron Sierra <asierra@xes-inc.com> |
|---|---|
| Date | 2015-09-21 19:50 +0200 |
| Message-ID | <qbdnY-5do-7@gated-at.bofh.it> |
| In reply to | #1228425 |
----- 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web