| Message ID | 20180609140224.32606-4-jmkrzyszt@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jun 09, 2018 at 04:02:18PM +0200, Janusz Krzysztofik wrote: > Modify the driver so it no longer requests and manipulates the > "keybrd_pwr" GPIO pin but a "vcc" regulator supply instead. > > For this to work with Amstrad Delta, define a regulator over the > "keybrd_pwr" GPIO pin with the "vcc" supply for ams-delta-serio device > and register it from the board file. Both assign an absulute GPIO > number to the soon depreciated .gpio member of the regulator config > structure, and also build and register a GPIO lookup table so it is > ready for use by the regulator driver as soon as its upcoming update > is applied. > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > --- > arch/arm/mach-omap1/board-ams-delta.c | 63 +++++++++++++++++++++++++++++++++-- > drivers/input/serio/ams_delta_serio.c | 27 ++++++++++----- > 2 files changed, 79 insertions(+), 11 deletions(-) > > diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c > index 2119d2d3ba84..706eb2f9301d 100644 > --- a/arch/arm/mach-omap1/board-ams-delta.c > +++ b/arch/arm/mach-omap1/board-ams-delta.c > @@ -509,6 +509,46 @@ static struct platform_device ams_delta_serio_device = { > .id = PLATFORM_DEVID_NONE, > }; > > +static struct regulator_consumer_supply keybrd_pwr_consumers[] = { > + /* > + * Initialize supply .dev_name with NULL. It will be replaced > + * with serio dev_name() as soon as the serio device is registered. > + */ > + REGULATOR_SUPPLY("vcc", NULL), > +}; > + > +static struct regulator_init_data keybrd_pwr_initdata = { > + .constraints = { > + .valid_ops_mask = REGULATOR_CHANGE_STATUS, > + }, > + .num_consumer_supplies = ARRAY_SIZE(keybrd_pwr_consumers), > + .consumer_supplies = keybrd_pwr_consumers, > +}; > + > +static struct fixed_voltage_config keybrd_pwr_config = { > + .supply_name = "keybrd_pwr", > + .microvolts = 5000000, > + .gpio = AMS_DELTA_GPIO_PIN_KEYBRD_PWR, > + .enable_high = 1, > + .init_data = &keybrd_pwr_initdata, > +}; > + > +static struct platform_device keybrd_pwr_device = { > + .name = "reg-fixed-voltage", > + .id = PLATFORM_DEVID_AUTO, > + .dev = { > + .platform_data = &keybrd_pwr_config, > + }, > +}; > + > +static struct gpiod_lookup_table keybrd_pwr_gpio_table = { > + .table = { > + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_KEYBRD_PWR, NULL, > + GPIO_ACTIVE_HIGH), > + { }, > + }, > +}; > + > static struct platform_device *ams_delta_devices[] __initdata = { > &latch1_gpio_device, > &latch2_gpio_device, > @@ -526,6 +566,7 @@ static struct platform_device *late_devices[] __initdata = { > > static struct gpiod_lookup_table *ams_delta_gpio_tables[] __initdata = { > &ams_delta_audio_gpio_table, > + &keybrd_pwr_gpio_table, > }; > > static struct gpiod_lookup_table *late_gpio_tables[] __initdata = { > @@ -566,12 +607,30 @@ static void __init ams_delta_init(void) > platform_add_devices(ams_delta_devices, ARRAY_SIZE(ams_delta_devices)); > > /* > - * As soon as devices have been registered, assign their dev_names > - * to respective GPIO lookup tables before they are added. > + * As soon as regulator consumers have been registered, assign their > + * dev_names to consumer supply entries of respective regulators. > + */ > + keybrd_pwr_consumers[0].dev_name = > + dev_name(&ams_delta_serio_device.dev); > + > + /* > + * Once consumer supply entries are populated with dev_names, > + * register regulator devices. At this stage only the keyboard > + * power regulator has its consumer supply table fully populated. > + */ > + platform_device_register(&keybrd_pwr_device); > + > + /* > + * As soon as GPIO consumers have been registered, assign > + * their dev_names to respective GPIO lookup tables. > */ > ams_delta_audio_gpio_table.dev_id = > dev_name(&ams_delta_audio_device.dev); > + keybrd_pwr_gpio_table.dev_id = dev_name(&keybrd_pwr_device.dev); > > + /* > + * Once GPIO lookup tables are populated with dev_names, register them. > + */ > gpiod_add_lookup_tables(ams_delta_gpio_tables, > ARRAY_SIZE(ams_delta_gpio_tables)); > > diff --git a/drivers/input/serio/ams_delta_serio.c b/drivers/input/serio/ams_delta_serio.c > index 551a4fa73fe4..d48beab1d00d 100644 > --- a/drivers/input/serio/ams_delta_serio.c > +++ b/drivers/input/serio/ams_delta_serio.c > @@ -23,6 +23,7 @@ > #include <linux/gpio.h> > #include <linux/irq.h> > #include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > #include <linux/serio.h> > #include <linux/slab.h> > #include <linux/module.h> > @@ -39,6 +40,7 @@ MODULE_LICENSE("GPL"); > > struct ams_delta_serio { > struct serio *serio; > + struct regulator *vcc; > }; > > static int check_data(struct serio *serio, int data) > @@ -94,16 +96,18 @@ static irqreturn_t ams_delta_serio_interrupt(int irq, void *dev_id) > > static int ams_delta_serio_open(struct serio *serio) > { > - /* enable keyboard */ > - gpio_set_value(AMS_DELTA_GPIO_PIN_KEYBRD_PWR, 1); > + struct ams_delta_serio *priv = serio->port_data; > > - return 0; > + /* enable keyboard */ > + return regulator_enable(priv->vcc); > } > > static void ams_delta_serio_close(struct serio *serio) > { > + struct ams_delta_serio *priv = serio->port_data; > + > /* disable keyboard */ > - gpio_set_value(AMS_DELTA_GPIO_PIN_KEYBRD_PWR, 0); > + regulator_disable(priv->vcc); > } > > static const struct gpio ams_delta_gpios[] __initconst_or_module = { > @@ -117,11 +121,6 @@ static const struct gpio ams_delta_gpios[] __initconst_or_module = { > .flags = GPIOF_DIR_IN, > .label = "serio-clock", > }, > - { > - .gpio = AMS_DELTA_GPIO_PIN_KEYBRD_PWR, > - .flags = GPIOF_OUT_INIT_LOW, > - .label = "serio-power", > - }, > { > .gpio = AMS_DELTA_GPIO_PIN_KEYBRD_DATAOUT, > .flags = GPIOF_OUT_INIT_LOW, > @@ -146,6 +145,16 @@ static int ams_delta_serio_init(struct platform_device *pdev) > goto serio; > } > > + priv->vcc = devm_regulator_get(&pdev->dev, "vcc"); > + if (IS_ERR(priv->vcc)) { > + err = PTR_ERR(priv->vcc); > + dev_err(&pdev->dev, "regulator request failed (%d)\n", err); > + /* Fail softly if the regulator is not available yet */ > + if (err == -ENODEV) > + err = -EPROBE_DEFER; Hmm, if regulator is not ready yet, devm_regulator_get() should be returning -EPROBE_DEFER already, we should not have to convert -ENODEV to -EPROBE_DEFER... Is it because we have_full_constraints() returns false? You might need to add call to regulator_has_full_constraints() to your board file. > + goto gpio; > + } > + > err = request_irq(gpio_to_irq(AMS_DELTA_GPIO_PIN_KEYBRD_CLK), > ams_delta_serio_interrupt, IRQ_TYPE_EDGE_RISING, > DRIVER_NAME, priv); > -- > 2.16.1 > Thanks.
On Wednesday, June 13, 2018 12:17:24 AM CEST Dmitry Torokhov wrote: > On Sat, Jun 09, 2018 at 04:02:18PM +0200, Janusz Krzysztofik wrote: > > ... > > + priv->vcc = devm_regulator_get(&pdev->dev, "vcc"); > > + if (IS_ERR(priv->vcc)) { > > + err = PTR_ERR(priv->vcc); > > + dev_err(&pdev->dev, "regulator request failed (%d)\n", err); > > + /* Fail softly if the regulator is not available yet */ > > + if (err == -ENODEV) > > + err = -EPROBE_DEFER; > > Hmm, if regulator is not ready yet, devm_regulator_get() should be > returning -EPROBE_DEFER already, we should not have to convert -ENODEV > to -EPROBE_DEFER... Regulator is not ready because its initialization at subsys_initcall is deferred by not ready GPIO pin, that in turn is caused by gpio-mmio driver, unlike many other GPIO drivers, registered as late as at device_initcall. I agree devm_regulator_get() could return -EPROBE_DEFER in this case, but I can see it does that only when of_get_regulator() indicates the regulator should exist. In non-dt case there is apparently no way to justify if it should unless its consumer supply table was already in place. For that, registration of that table would have to be independent of successful registration of the regulator itself while it's not. Maybe it should, but that's a separate topic for a separate discussion, I think. > Is it because we have_full_constraints() returns false? You might need > to add call to regulator_has_full_constraints() to your board file. If have_full_constraints() returned true before the regulator or its consumer supply table is ready, devm_regulator_get() would happily return a dummy regulator and our keyboard would never get its power. I'm afraid we have to live with that return code conversion as long as the only user of this driver is not migrated to dt. Thanks, Janusz
On Wed, Jun 13, 2018 at 03:01:05AM +0200, Janusz Krzysztofik wrote: > On Wednesday, June 13, 2018 12:17:24 AM CEST Dmitry Torokhov wrote: > > On Sat, Jun 09, 2018 at 04:02:18PM +0200, Janusz Krzysztofik wrote: > > > ... > > > + priv->vcc = devm_regulator_get(&pdev->dev, "vcc"); > > > + if (IS_ERR(priv->vcc)) { > > > + err = PTR_ERR(priv->vcc); > > > + dev_err(&pdev->dev, "regulator request failed (%d)\n", err); > > > + /* Fail softly if the regulator is not available yet */ > > > + if (err == -ENODEV) > > > + err = -EPROBE_DEFER; > > > > Hmm, if regulator is not ready yet, devm_regulator_get() should be > > returning -EPROBE_DEFER already, we should not have to convert -ENODEV > > to -EPROBE_DEFER... > > Regulator is not ready because its initialization at subsys_initcall is > deferred by not ready GPIO pin, that in turn is caused by gpio-mmio driver, > unlike many other GPIO drivers, registered as late as at device_initcall. > > I agree devm_regulator_get() could return -EPROBE_DEFER in this case, but I > can see it does that only when of_get_regulator() indicates the regulator > should exist. In non-dt case there is apparently no way to justify if it > should unless its consumer supply table was already in place. For that, > registration of that table would have to be independent of successful > registration of the regulator itself while it's not. Maybe it should, but > that's a separate topic for a separate discussion, I think. > > > Is it because we have_full_constraints() returns false? You might need > > to add call to regulator_has_full_constraints() to your board file. > > If have_full_constraints() returned true before the regulator or its consumer > supply table is ready, devm_regulator_get() would happily return a dummy > regulator and our keyboard would never get its power. > > I'm afraid we have to live with that return code conversion as long as the > only user of this driver is not migrated to dt. OK, fair enough. Can you please add a comment to that effect? Thanks.
diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c index 2119d2d3ba84..706eb2f9301d 100644 --- a/arch/arm/mach-omap1/board-ams-delta.c +++ b/arch/arm/mach-omap1/board-ams-delta.c @@ -509,6 +509,46 @@ static struct platform_device ams_delta_serio_device = { .id = PLATFORM_DEVID_NONE, }; +static struct regulator_consumer_supply keybrd_pwr_consumers[] = { + /* + * Initialize supply .dev_name with NULL. It will be replaced + * with serio dev_name() as soon as the serio device is registered. + */ + REGULATOR_SUPPLY("vcc", NULL), +}; + +static struct regulator_init_data keybrd_pwr_initdata = { + .constraints = { + .valid_ops_mask = REGULATOR_CHANGE_STATUS, + }, + .num_consumer_supplies = ARRAY_SIZE(keybrd_pwr_consumers), + .consumer_supplies = keybrd_pwr_consumers, +}; + +static struct fixed_voltage_config keybrd_pwr_config = { + .supply_name = "keybrd_pwr", + .microvolts = 5000000, + .gpio = AMS_DELTA_GPIO_PIN_KEYBRD_PWR, + .enable_high = 1, + .init_data = &keybrd_pwr_initdata, +}; + +static struct platform_device keybrd_pwr_device = { + .name = "reg-fixed-voltage", + .id = PLATFORM_DEVID_AUTO, + .dev = { + .platform_data = &keybrd_pwr_config, + }, +}; + +static struct gpiod_lookup_table keybrd_pwr_gpio_table = { + .table = { + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_KEYBRD_PWR, NULL, + GPIO_ACTIVE_HIGH), + { }, + }, +}; + static struct platform_device *ams_delta_devices[] __initdata = { &latch1_gpio_device, &latch2_gpio_device, @@ -526,6 +566,7 @@ static struct platform_device *late_devices[] __initdata = { static struct gpiod_lookup_table *ams_delta_gpio_tables[] __initdata = { &ams_delta_audio_gpio_table, + &keybrd_pwr_gpio_table, }; static struct gpiod_lookup_table *late_gpio_tables[] __initdata = { @@ -566,12 +607,30 @@ static void __init ams_delta_init(void) platform_add_devices(ams_delta_devices, ARRAY_SIZE(ams_delta_devices)); /* - * As soon as devices have been registered, assign their dev_names - * to respective GPIO lookup tables before they are added. + * As soon as regulator consumers have been registered, assign their + * dev_names to consumer supply entries of respective regulators. + */ + keybrd_pwr_consumers[0].dev_name = + dev_name(&ams_delta_serio_device.dev); + + /* + * Once consumer supply entries are populated with dev_names, + * register regulator devices. At this stage only the keyboard + * power regulator has its consumer supply table fully populated. + */ + platform_device_register(&keybrd_pwr_device); + + /* + * As soon as GPIO consumers have been registered, assign + * their dev_names to respective GPIO lookup tables. */ ams_delta_audio_gpio_table.dev_id = dev_name(&ams_delta_audio_device.dev); + keybrd_pwr_gpio_table.dev_id = dev_name(&keybrd_pwr_device.dev); + /* + * Once GPIO lookup tables are populated with dev_names, register them. + */ gpiod_add_lookup_tables(ams_delta_gpio_tables, ARRAY_SIZE(ams_delta_gpio_tables)); diff --git a/drivers/input/serio/ams_delta_serio.c b/drivers/input/serio/ams_delta_serio.c index 551a4fa73fe4..d48beab1d00d 100644 --- a/drivers/input/serio/ams_delta_serio.c +++ b/drivers/input/serio/ams_delta_serio.c @@ -23,6 +23,7 @@ #include <linux/gpio.h> #include <linux/irq.h> #include <linux/platform_device.h> +#include <linux/regulator/consumer.h> #include <linux/serio.h> #include <linux/slab.h> #include <linux/module.h> @@ -39,6 +40,7 @@ MODULE_LICENSE("GPL"); struct ams_delta_serio { struct serio *serio; + struct regulator *vcc; }; static int check_data(struct serio *serio, int data) @@ -94,16 +96,18 @@ static irqreturn_t ams_delta_serio_interrupt(int irq, void *dev_id) static int ams_delta_serio_open(struct serio *serio) { - /* enable keyboard */ - gpio_set_value(AMS_DELTA_GPIO_PIN_KEYBRD_PWR, 1); + struct ams_delta_serio *priv = serio->port_data; - return 0; + /* enable keyboard */ + return regulator_enable(priv->vcc); } static void ams_delta_serio_close(struct serio *serio) { + struct ams_delta_serio *priv = serio->port_data; + /* disable keyboard */ - gpio_set_value(AMS_DELTA_GPIO_PIN_KEYBRD_PWR, 0); + regulator_disable(priv->vcc); } static const struct gpio ams_delta_gpios[] __initconst_or_module = { @@ -117,11 +121,6 @@ static const struct gpio ams_delta_gpios[] __initconst_or_module = { .flags = GPIOF_DIR_IN, .label = "serio-clock", }, - { - .gpio = AMS_DELTA_GPIO_PIN_KEYBRD_PWR, - .flags = GPIOF_OUT_INIT_LOW, - .label = "serio-power", - }, { .gpio = AMS_DELTA_GPIO_PIN_KEYBRD_DATAOUT, .flags = GPIOF_OUT_INIT_LOW, @@ -146,6 +145,16 @@ static int ams_delta_serio_init(struct platform_device *pdev) goto serio; } + priv->vcc = devm_regulator_get(&pdev->dev, "vcc"); + if (IS_ERR(priv->vcc)) { + err = PTR_ERR(priv->vcc); + dev_err(&pdev->dev, "regulator request failed (%d)\n", err); + /* Fail softly if the regulator is not available yet */ + if (err == -ENODEV) + err = -EPROBE_DEFER; + goto gpio; + } + err = request_irq(gpio_to_irq(AMS_DELTA_GPIO_PIN_KEYBRD_CLK), ams_delta_serio_interrupt, IRQ_TYPE_EDGE_RISING, DRIVER_NAME, priv);
Modify the driver so it no longer requests and manipulates the "keybrd_pwr" GPIO pin but a "vcc" regulator supply instead. For this to work with Amstrad Delta, define a regulator over the "keybrd_pwr" GPIO pin with the "vcc" supply for ams-delta-serio device and register it from the board file. Both assign an absulute GPIO number to the soon depreciated .gpio member of the regulator config structure, and also build and register a GPIO lookup table so it is ready for use by the regulator driver as soon as its upcoming update is applied. Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> --- arch/arm/mach-omap1/board-ams-delta.c | 63 +++++++++++++++++++++++++++++++++-- drivers/input/serio/ams_delta_serio.c | 27 ++++++++++----- 2 files changed, 79 insertions(+), 11 deletions(-)