Re: [Intel-gfx] [RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal

2015-03-25 Thread Linus Walleij
On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar shobhit.ku...@intel.com wrote:

 On some Intel SoC platforms, the panel enable/disable signals are
 controlled by CRC PMIC. Add those control as a new GPIO in a lookup
 table for gpio-crystalcove chip during CRC driver load

 CC: Samuel Ortiz sa...@linux.intel.com
 Cc: Linus Walleij linus.wall...@linaro.org
 Cc: Alexandre Courbot gnu...@gmail.com
 Cc: Thierry Reding thierry.red...@gmail.com
 Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com

I have changed my mind.
Reviewed-by: Linus Walleij linus.wall...@linaro.org

Lee, please apply this.

Yours,
Linus Walleij
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal

2015-03-24 Thread Linus Walleij
On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar shobhit.ku...@intel.com wrote:

 On some Intel SoC platforms, the panel enable/disable signals are
 controlled by CRC PMIC. Add those control as a new GPIO in a lookup
 table for gpio-crystalcove chip during CRC driver load

 CC: Samuel Ortiz sa...@linux.intel.com
 Cc: Linus Walleij linus.wall...@linaro.org
 Cc: Alexandre Courbot gnu...@gmail.com
 Cc: Thierry Reding thierry.red...@gmail.com
 Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com

So as I think it is better to use a fixed voltage regulator (see code
example in the i915 patch) I think this
should register a regulator lookup instead.

Yours,
Linus Walleij
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal

2015-03-24 Thread Thierry Reding
On Thu, Mar 12, 2015 at 10:01:25PM +0530, Shobhit Kumar wrote:
 On some Intel SoC platforms, the panel enable/disable signals are
 controlled by CRC PMIC. Add those control as a new GPIO in a lookup
 table for gpio-crystalcove chip during CRC driver load
 
 CC: Samuel Ortiz sa...@linux.intel.com
 Cc: Linus Walleij linus.wall...@linaro.org
 Cc: Alexandre Courbot gnu...@gmail.com
 Cc: Thierry Reding thierry.red...@gmail.com
 Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com
 ---
  drivers/mfd/intel_soc_pmic_core.c | 14 ++
  1 file changed, 14 insertions(+)
 
 diff --git a/drivers/mfd/intel_soc_pmic_core.c 
 b/drivers/mfd/intel_soc_pmic_core.c
 index 80cef04..365d5de 100644
 --- a/drivers/mfd/intel_soc_pmic_core.c
 +++ b/drivers/mfd/intel_soc_pmic_core.c
 @@ -24,8 +24,19 @@
  #include linux/acpi.h
  #include linux/regmap.h
  #include linux/mfd/intel_soc_pmic.h
 +#include linux/gpio/machine.h
  #include intel_soc_pmic_core.h
  
 +/* Lookup table for the Panel Enable/Disable line as GPIO signals */
 +struct gpiod_lookup_table panel_gpio_table = {

Should this be static?

 + /* Intel GFX is consumer */
 + .dev_id = :00:02.0,
 + .table = {
 + /* Panel EN/DISABLE */
 + GPIO_LOOKUP(gpio_crystalcove, 94, panel, GPIO_ACTIVE_HIGH),
 + },
 +};
 +
  /*
   * On some boards the PMIC interrupt may come from a GPIO line.
   * Try to lookup the ACPI table and see if such connection exists. If not,
 @@ -85,6 +96,9 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
   if (ret)
   dev_warn(dev, Can't enable IRQ as wake source: %d\n, ret);
  
 + /* Add lookup table binding for Panel Control to the GPIO Chip */
 + gpiod_add_lookup_table(panel_gpio_table);
 +

There's no corresponding gpiod_remove_lookup_table() call anywhere. That
is understandable, given that that function doesn't exist. However, this
driver can be unloaded (or at least unbound from the device), at which
point the data effectively becomes stale. This shouldn't pose much of a
problem because the driver can't be built as a module, so the data will
stick around. However, what happens if you unload and then reload the
driver? You'll be adding a second instance of the GPIO table. Given that
the data is identical maybe that isn't even a problem. Then again, it's
probably going to mess up the global list because you're adding the same
entry twice.

So I think that's something we need to fix. The same is true for the PWM
lookup table in a later patch.

Thierry


pgpwhkhBXD8tF.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal

2015-03-12 Thread Shobhit Kumar
On some Intel SoC platforms, the panel enable/disable signals are
controlled by CRC PMIC. Add those control as a new GPIO in a lookup
table for gpio-crystalcove chip during CRC driver load

CC: Samuel Ortiz sa...@linux.intel.com
Cc: Linus Walleij linus.wall...@linaro.org
Cc: Alexandre Courbot gnu...@gmail.com
Cc: Thierry Reding thierry.red...@gmail.com
Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com
---
 drivers/mfd/intel_soc_pmic_core.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_core.c 
b/drivers/mfd/intel_soc_pmic_core.c
index 80cef04..365d5de 100644
--- a/drivers/mfd/intel_soc_pmic_core.c
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -24,8 +24,19 @@
 #include linux/acpi.h
 #include linux/regmap.h
 #include linux/mfd/intel_soc_pmic.h
+#include linux/gpio/machine.h
 #include intel_soc_pmic_core.h
 
+/* Lookup table for the Panel Enable/Disable line as GPIO signals */
+struct gpiod_lookup_table panel_gpio_table = {
+   /* Intel GFX is consumer */
+   .dev_id = :00:02.0,
+   .table = {
+   /* Panel EN/DISABLE */
+   GPIO_LOOKUP(gpio_crystalcove, 94, panel, GPIO_ACTIVE_HIGH),
+   },
+};
+
 /*
  * On some boards the PMIC interrupt may come from a GPIO line.
  * Try to lookup the ACPI table and see if such connection exists. If not,
@@ -85,6 +96,9 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
if (ret)
dev_warn(dev, Can't enable IRQ as wake source: %d\n, ret);
 
+   /* Add lookup table binding for Panel Control to the GPIO Chip */
+   gpiod_add_lookup_table(panel_gpio_table);
+
ret = mfd_add_devices(dev, -1, config-cell_dev,
  config-n_cell_devs, NULL, 0,
  regmap_irq_get_domain(pmic-irq_chip_data));
-- 
2.1.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx