| Message ID | 1453292992-1788-2-git-send-email-LW@KARO-electronics.de |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 20, 2016 at 01:29:51PM +0100, Lothar Waßmann wrote: > This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro > electronics TX48 module. Make the IRQ optional in the driver and use a > polling routine instead if no IRQ is specified in DT. > Otherwise the driver will continuously generate interrupts and make > the system unusable. How will the driver generate interrupts if there is no interrupt physically present in the system?
On 01/20/2016 02:29 PM, Lothar Waßmann wrote: > On the AM335x SoC rev. <= 1.0 the "Input Function of the EXTINTn > Terminal is Inverted", for which the only remedy is to "Use an active > high interrupt source or use an external inverter to change the > polarity of any active low interrupt source." > > This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro > electronics TX48 module. Make the IRQ optional in the driver and use a > polling routine instead if no IRQ is specified in DT. > Otherwise the driver will continuously generate interrupts and make > the system unusable. > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > --- > drivers/regulator/ltc3589.c | 49 +++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 45 insertions(+), 4 deletions(-) > > diff --git a/drivers/regulator/ltc3589.c b/drivers/regulator/ltc3589.c [...] > } > @@ -519,6 +539,16 @@ static int ltc3589_probe(struct i2c_client *client, > return ret; > } > } > + if (client->irq <= 0) { > + dev_warn(dev, > + "No interrupt configured; poll for thermal shutdown and undervoltage events\n"); > + > + INIT_DELAYED_WORK(<c3589->poll_timer, ltc3589_poll_func); > + schedule_delayed_work(<c3589->poll_timer, > + msecs_to_jiffies(POLL_PERIOD)); > + > + return 0; > + } > > ret = devm_request_threaded_irq(dev, client->irq, NULL, ltc3589_isr, > IRQF_TRIGGER_LOW | IRQF_ONESHOT, ^^^^^^^^^^^^^^^^ I assume you have issue with IRQ because of the above hard-coded flag. if yes - Over all approach for such kind of issues - do not hard-code IRQ triggering flags in code in case of DT boot. DT core will configure it properly. [...]
Hi, > On Wed, Jan 20, 2016 at 01:29:51PM +0100, Lothar Waßmann wrote: > > > This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro > > electronics TX48 module. Make the IRQ optional in the driver and use a > > polling routine instead if no IRQ is specified in DT. > > Otherwise the driver will continuously generate interrupts and make > > the system unusable. > > How will the driver generate interrupts if there is no interrupt > physically present in the system? > It's using timer interrupts to poll the LTC3589 state. Lothar Waßmann
On Thu, Jan 21, 2016 at 08:05:24AM +0100, Lothar Waßmann wrote: > > On Wed, Jan 20, 2016 at 01:29:51PM +0100, Lothar Waßmann wrote: > > > This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro > > > electronics TX48 module. Make the IRQ optional in the driver and use a > > > polling routine instead if no IRQ is specified in DT. > > > Otherwise the driver will continuously generate interrupts and make > > > the system unusable. > > How will the driver generate interrupts if there is no interrupt > > physically present in the system? > It's using timer interrupts to poll the LTC3589 state. I know that is what your patch does, my question is why you say in your commit log that "Otherwise the driver will continuously generate interrupts and make the system unusable".
Hi, > On Thu, Jan 21, 2016 at 08:05:24AM +0100, Lothar Waßmann wrote: > > > On Wed, Jan 20, 2016 at 01:29:51PM +0100, Lothar Waßmann wrote: > > > > > This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro > > > > electronics TX48 module. Make the IRQ optional in the driver and use a > > > > polling routine instead if no IRQ is specified in DT. > > > > Otherwise the driver will continuously generate interrupts and make > > > > the system unusable. > > > > How will the driver generate interrupts if there is no interrupt > > > physically present in the system? > > > It's using timer interrupts to poll the LTC3589 state. > > I know that is what your patch does, my question is why you say in your > commit log that "Otherwise the driver will continuously generate > interrupts and make the system unusable". > Because the interrupt is level triggered and the polarity of the EXTINT pin is inverted, the interrupt will be constantly active when the IRQ pin of the LTC3589 is inactive. Lothar Waßmann
On Thu, Jan 21, 2016 at 11:26:11AM +0100, Lothar Waßmann wrote: > > On Thu, Jan 21, 2016 at 08:05:24AM +0100, Lothar Waßmann wrote: > > > > On Wed, Jan 20, 2016 at 01:29:51PM +0100, Lothar Waßmann wrote: > > > > > This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro > > > > > electronics TX48 module. Make the IRQ optional in the driver and use a > > > > > polling routine instead if no IRQ is specified in DT. > > > > > Otherwise the driver will continuously generate interrupts and make > > > > > the system unusable. > > > > How will the driver generate interrupts if there is no interrupt > > > > physically present in the system? > > > It's using timer interrupts to poll the LTC3589 state. > > I know that is what your patch does, my question is why you say in your > > commit log that "Otherwise the driver will continuously generate > > interrupts and make the system unusable". > Because the interrupt is level triggered and the polarity of the > EXTINT pin is inverted, the interrupt will be constantly active when > the IRQ pin of the LTC3589 is inactive. So, to repeat my original question, how will that generate interrupts if there is no interrupt physically present in the system?
Hi, > On Thu, Jan 21, 2016 at 11:26:11AM +0100, Lothar Waßmann wrote: > > > On Thu, Jan 21, 2016 at 08:05:24AM +0100, Lothar Waßmann wrote: > > > > > On Wed, Jan 20, 2016 at 01:29:51PM +0100, Lothar Waßmann wrote: > > > > > > > This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro > > > > > > electronics TX48 module. Make the IRQ optional in the driver and use a > > > > > > polling routine instead if no IRQ is specified in DT. > > > > > > Otherwise the driver will continuously generate interrupts and make > > > > > > the system unusable. > > > > > > How will the driver generate interrupts if there is no interrupt > > > > > physically present in the system? > > > > > It's using timer interrupts to poll the LTC3589 state. > > > > I know that is what your patch does, my question is why you say in your > > > commit log that "Otherwise the driver will continuously generate > > > interrupts and make the system unusable". > > > Because the interrupt is level triggered and the polarity of the > > EXTINT pin is inverted, the interrupt will be constantly active when > > the IRQ pin of the LTC3589 is inactive. > > So, to repeat my original question, how will that generate interrupts if > there is no interrupt physically present in the system? > It won't. That's the whole purpose of this patch. I'm afraid, I don't quite understand what you want to say... Without this patch there will be a constantly active interrupt, which will stall the system because the nNMI interrupt (on the EXTINTn pin) is level triggered. Since the polarity of the interrupt input is fixed, there is no way to use it in our HW. Lothar Waßmann
On Thu, Jan 21, 2016 at 12:33:11PM +0100, Lothar Waßmann wrote: > > On Thu, Jan 21, 2016 at 11:26:11AM +0100, Lothar Waßmann wrote: > > > > On Thu, Jan 21, 2016 at 08:05:24AM +0100, Lothar Waßmann wrote: > > > > > > On Wed, Jan 20, 2016 at 01:29:51PM +0100, Lothar Waßmann wrote: > > > > > > > This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro > > > > > > > electronics TX48 module. Make the IRQ optional in the driver and use a > > > > > > > polling routine instead if no IRQ is specified in DT. > > > > > > > Otherwise the driver will continuously generate interrupts and make > > > > > > > the system unusable. > It won't. That's the whole purpose of this patch. > I'm afraid, I don't quite understand what you want to say... Your commit message (quoted above) claims that without this patch if no interrupt is supplied then the unsupplied interrupt will somehow be left screaming and make the system unusable. This doesn't make sense, if there is no interrupt there is nothing to scream. > Without this patch there will be a constantly active interrupt, which > will stall the system because the nNMI interrupt (on the EXTINTn pin) is > level triggered. > Since the polarity of the interrupt input is fixed, there is no way to > use it in our HW. So, contrary to what you've been saying, the interrupt is actually connected (and worse, connected to a NMI) but apparently not described in DT. Why is it sensible to make the driver poll (which will affect all systems using this device, even those that don't care) and not just describe the interrupt in DT so it can be handled promptly in the normal fashion? Presumably this will run into serious problems if the interrupt actually fires at runtime since the NMI will scream, it's not clear to me how the poll will manage to run successfully in that case. This really feels like a very system specific workaround which is attempting to address things in the wrong place and coming up with something very non-obvious as a result.
Hi, > On Thu, Jan 21, 2016 at 12:33:11PM +0100, Lothar Waßmann wrote: > > > On Thu, Jan 21, 2016 at 11:26:11AM +0100, Lothar Waßmann wrote: > > > > > On Thu, Jan 21, 2016 at 08:05:24AM +0100, Lothar Waßmann wrote: > > > > > > > On Wed, Jan 20, 2016 at 01:29:51PM +0100, Lothar Waßmann wrote: > > > > > > > > > This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro > > > > > > > > electronics TX48 module. Make the IRQ optional in the driver and use a > > > > > > > > polling routine instead if no IRQ is specified in DT. > > > > > > > > Otherwise the driver will continuously generate interrupts and make > > > > > > > > the system unusable. > > > It won't. That's the whole purpose of this patch. > > I'm afraid, I don't quite understand what you want to say... > > Your commit message (quoted above) claims that without this patch if no > interrupt is supplied then the unsupplied interrupt will somehow be left > screaming and make the system unusable. This doesn't make sense, if > there is no interrupt there is nothing to scream. > "Otherwise" meant the case where the IRQ is specified in DT as is currently required to get the driver loaded at all. > > Without this patch there will be a constantly active interrupt, which > > will stall the system because the nNMI interrupt (on the EXTINTn pin) is > > level triggered. > > Since the polarity of the interrupt input is fixed, there is no way to > > use it in our HW. > > So, contrary to what you've been saying, the interrupt is actually > connected (and worse, connected to a NMI) but apparently not described > in DT. Why is it sensible to make the driver poll (which will affect > all systems using this device, even those that don't care) and not just > describe the interrupt in DT so it can be handled promptly in the normal > fashion? Presumably this will run into serious problems if the > interrupt actually fires at runtime since the NMI will scream, it's not > clear to me how the poll will manage to run successfully in that case. > Currently the driver won't even load without an IRQ specified in DT. My patch makes it possible to use the driver without requiring an IRQ! Lothar Waßmann
On Fri, Jan 22, 2016 at 06:41:45AM +0100, Lothar Waßmann wrote: > > On Thu, Jan 21, 2016 at 12:33:11PM +0100, Lothar Waßmann wrote: > > Your commit message (quoted above) claims that without this patch if no > > interrupt is supplied then the unsupplied interrupt will somehow be left > > screaming and make the system unusable. This doesn't make sense, if > > there is no interrupt there is nothing to scream. > "Otherwise" meant the case where the IRQ is specified in DT as is > currently required to get the driver loaded at all. > > So, contrary to what you've been saying, the interrupt is actually > > connected (and worse, connected to a NMI) but apparently not described > > in DT. Why is it sensible to make the driver poll (which will affect > > all systems using this device, even those that don't care) and not just > > describe the interrupt in DT so it can be handled promptly in the normal > > fashion? Presumably this will run into serious problems if the > > interrupt actually fires at runtime since the NMI will scream, it's not > > clear to me how the poll will manage to run successfully in that case. > Currently the driver won't even load without an IRQ specified in DT. > My patch makes it possible to use the driver without requiring an IRQ! You're not just making the interrupt optional, you are also implementing polling support. That's really unusual and there's no clear reason for it, your changelog seems to claim that it is needed to make the system work but that seems at best very surprising and would need a more detailed changelog. You at least need to provide an understandable changelog, though it seems it is more likely that there is a more sensible way of dealing with this.
Hi, On Fri, 22 Jan 2016 16:26:10 +0000 Mark Brown wrote: > On Fri, Jan 22, 2016 at 06:41:45AM +0100, Lothar Waßmann wrote: > > > On Thu, Jan 21, 2016 at 12:33:11PM +0100, Lothar Waßmann wrote: > > > > Your commit message (quoted above) claims that without this patch if no > > > interrupt is supplied then the unsupplied interrupt will somehow be left > > > screaming and make the system unusable. This doesn't make sense, if > > > there is no interrupt there is nothing to scream. > > > "Otherwise" meant the case where the IRQ is specified in DT as is > > currently required to get the driver loaded at all. > > > > So, contrary to what you've been saying, the interrupt is actually > > > connected (and worse, connected to a NMI) but apparently not described > > > in DT. Why is it sensible to make the driver poll (which will affect > > > all systems using this device, even those that don't care) and not just > > > describe the interrupt in DT so it can be handled promptly in the normal > > > fashion? Presumably this will run into serious problems if the > > > interrupt actually fires at runtime since the NMI will scream, it's not > > > clear to me how the poll will manage to run successfully in that case. > > > Currently the driver won't even load without an IRQ specified in DT. > > My patch makes it possible to use the driver without requiring an IRQ! > > You're not just making the interrupt optional, you are also implementing > polling support. That's really unusual and there's no clear reason for > it, your changelog seems to claim that it is needed to make the system > work but that seems at best very surprising and would need a more > detailed changelog. > > You at least need to provide an understandable changelog, though it > seems it is more likely that there is a more sensible way of dealing > with this. > Any suggestions how to handle this case in a more sensible way? Lothar Waßmann
On Mon, Jan 25, 2016 at 01:37:31PM +0100, Lothar Waßmann wrote: > On Fri, 22 Jan 2016 16:26:10 +0000 Mark Brown wrote: > > You're not just making the interrupt optional, you are also implementing > > polling support. That's really unusual and there's no clear reason for > Any suggestions how to handle this case in a more sensible way? The above, for example - make the interrupt optional.
Hi, On Mon, 25 Jan 2016 12:41:23 +0000 Mark Brown wrote: > On Mon, Jan 25, 2016 at 01:37:31PM +0100, Lothar Waßmann wrote: > > On Fri, 22 Jan 2016 16:26:10 +0000 Mark Brown wrote: > > > > You're not just making the interrupt optional, you are also implementing > > > polling support. That's really unusual and there's no clear reason for > > > Any suggestions how to handle this case in a more sensible way? > > The above, for example - make the interrupt optional. > This will make it impossible to notify the system about overtemperature (and undervoltage). I implemented polling to be able to get at least overtemperature warnings. (Undervoltage cannot be handled sensibly without interrupt anyway) Lothar Waßmann
On Mon, Jan 25, 2016 at 01:51:09PM +0100, Lothar Waßmann wrote: > On Mon, 25 Jan 2016 12:41:23 +0000 Mark Brown wrote: > > The above, for example - make the interrupt optional. > This will make it impossible to notify the system about > overtemperature (and undervoltage). > I implemented polling to be able to get at least overtemperature > warnings. > (Undervoltage cannot be handled sensibly without interrupt anyway) I'm not convinced that justifies constantly polling, if the system designers cared you'd hope they'd have wired it up to a working interrupt. People commonly don't, realistically thermal warnings are usually set near the point where the silicon will be physically damaged and typically by the time they fire the system is already experiencing catastrophic problems at a system level. The polling is at the very least a separate change, and making the interrupt work would be a much better option.
diff --git a/drivers/regulator/ltc3589.c b/drivers/regulator/ltc3589.c index 972c386..fff90cd 100644 --- a/drivers/regulator/ltc3589.c +++ b/drivers/regulator/ltc3589.c @@ -65,6 +65,8 @@ #define LTC3589_VCCR_SW3_GO BIT(4) #define LTC3589_VCCR_LDO2_GO BIT(6) +#define POLL_PERIOD 1000 + enum ltc3589_variant { LTC3589, LTC3589_1, @@ -97,6 +99,7 @@ struct ltc3589 { enum ltc3589_variant variant; struct ltc3589_regulator regulator_descs[LTC3589_NUM_REGULATORS]; struct regulator_dev *regulators[LTC3589_NUM_REGULATORS]; + struct delayed_work poll_timer; }; static const int ltc3589_ldo4[] = { @@ -407,11 +410,10 @@ static const struct regmap_config ltc3589_regmap_config = { .cache_type = REGCACHE_RBTREE, }; - -static irqreturn_t ltc3589_isr(int irq, void *dev_id) +static inline void ltc3589_handle_irq(struct ltc3589 *ltc3589) { - struct ltc3589 *ltc3589 = dev_id; - unsigned int i, irqstat, event; + u32 irqstat, event; + int i; regmap_read(ltc3589->regmap, LTC3589_IRQSTAT, &irqstat); @@ -431,6 +433,24 @@ static irqreturn_t ltc3589_isr(int irq, void *dev_id) /* Clear warning condition */ regmap_write(ltc3589->regmap, LTC3589_CLIRQ, 0); +} + +static void ltc3589_poll_func(struct work_struct *work) +{ + struct ltc3589 *ltc3589 = container_of(work, struct ltc3589, + poll_timer.work); + unsigned long timeout = msecs_to_jiffies(POLL_PERIOD); + + ltc3589_handle_irq(ltc3589); + + schedule_delayed_work(<c3589->poll_timer, timeout); +} + +static irqreturn_t ltc3589_isr(int irq, void *dev_id) +{ + struct ltc3589 *ltc3589 = dev_id; + + ltc3589_handle_irq(ltc3589); return IRQ_HANDLED; } @@ -519,6 +539,16 @@ static int ltc3589_probe(struct i2c_client *client, return ret; } } + if (client->irq <= 0) { + dev_warn(dev, + "No interrupt configured; poll for thermal shutdown and undervoltage events\n"); + + INIT_DELAYED_WORK(<c3589->poll_timer, ltc3589_poll_func); + schedule_delayed_work(<c3589->poll_timer, + msecs_to_jiffies(POLL_PERIOD)); + + return 0; + } ret = devm_request_threaded_irq(dev, client->irq, NULL, ltc3589_isr, IRQF_TRIGGER_LOW | IRQF_ONESHOT, @@ -531,6 +561,16 @@ static int ltc3589_probe(struct i2c_client *client, return 0; } +static int ltc3589_remove(struct i2c_client *client) +{ + struct ltc3589 *ltc3589 = i2c_get_clientdata(client); + + if (client->irq <= 0) + cancel_delayed_work(<c3589->poll_timer); + + return 0; +} + static struct i2c_device_id ltc3589_i2c_id[] = { { "ltc3589", LTC3589 }, { "ltc3589-1", LTC3589_1 }, @@ -544,6 +584,7 @@ static struct i2c_driver ltc3589_driver = { .name = DRIVER_NAME, }, .probe = ltc3589_probe, + .remove = ltc3589_remove, .id_table = ltc3589_i2c_id, }; module_i2c_driver(ltc3589_driver);
On the AM335x SoC rev. <= 1.0 the "Input Function of the EXTINTn Terminal is Inverted", for which the only remedy is to "Use an active high interrupt source or use an external inverter to change the polarity of any active low interrupt source." This pin is used as IRQ pin for the LTC3589 PMIC on the Ka-Ro electronics TX48 module. Make the IRQ optional in the driver and use a polling routine instead if no IRQ is specified in DT. Otherwise the driver will continuously generate interrupts and make the system unusable. Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> --- drivers/regulator/ltc3589.c | 49 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-)