| Message ID | 20171129213300.20021-1-afd@ti.com |
|---|---|
| Headers | show |
| Series | Add Headphone Detection to TLV320AIC31xx Driver | expand |
On Wed, Nov 29, 2017 at 03:32:42PM -0600, Andrew F. Davis wrote: > Fix header copyright tags, while we are here, also switch to SPDX > and fixup MODULE tags to match. > - * The TLV320AIC31xx series of audio codec is a low-power, highly integrated > - * high performance codec which provides a stereo DAC, a mono ADC, > + * The TLV320AIC31xx series of audio codecs are low-power, highly integrated > + * high performance codecs which provides a stereo DAC, a mono ADC, ...and also correct some grammar in the description of the part.
On Wed, Nov 29, 2017 at 03:32:50PM -0600, Andrew F. Davis wrote: > Platform data is not used by anyone (at least in upstream) so > drop this data and switch to using fwnode(DT/ACPI) only. The advantage being...? Not all architectures use DT or ACPI so it's not clear that this is a step forwards in itself.
On Wed, Nov 29, 2017 at 03:32:55PM -0600, Andrew F. Davis wrote: > A regulator being forcefully disabled is a catastrophic event that > should never happen to most devices, especially not sound CODECs. That's not what the disable notification handling is for. It's there so that the driver can skip having to reinitialize the device if other constraints mean the power doesn't actually get turned off when it disables the regualtors. It's nothing to do with forced disables.
On Wed, Nov 29, 2017 at 03:32:56PM -0600, Andrew F. Davis wrote: > + gpiod_set_value(aic31xx->gpio_reset, 1); > + ndelay(10); /* At least 10ns */ > + gpiod_set_value(aic31xx->gpio_reset, 0); _cansleep(), this isn't being done from interrupt context. > + } else { > + ret = regmap_write(aic31xx->regmap, AIC31XX_RESET, 1); > + if (ret < 0) > + dev_err(aic31xx->dev, "Could not reset device\n"); Print the error to help people doing debug.
On Wed, Nov 29, 2017 at 03:32:57PM -0600, Andrew F. Davis wrote: > +static irqreturn_t aic31xx_irq(int irq, void *data) > +{ > + struct aic31xx_priv *aic31xx = (struct aic31xx_priv *)data; There is no need to cast away from void, doing so can only mask bugs. > + ret = regmap_read(aic31xx->regmap, AIC31XX_INTRDACFLAG, &value); > + if (ret) { > + dev_err(dev, "Failed to read interrupt mask: %d\n", ret); > + return IRQ_NONE; > + } > + > + if (value & AIC31XX_HPLSCDETECT) > + dev_err(dev, "Short circuit on Left output is detected\n"); > + if (value & AIC31XX_HPRSCDETECT) > + dev_err(dev, "Short circuit on Right output is detected\n"); > + > + return IRQ_HANDLED; This will report the interrupt as handled even if we didn't see an interrupt we understand which will break shared interrupt lines. At the very least we should log other interrupt sources numerically. > + if (aic31xx->irq > 0) { > + regmap_update_bits(aic31xx->regmap, AIC31XX_GPIO1, > + AIC31XX_GPIO1_FUNC_MASK, > + AIC31XX_GPIO1_INT1 << > + AIC31XX_GPIO1_FUNC_SHIFT); Is the interrupt only available on GPIO1?
On Wed, Nov 29, 2017 at 03:32:59PM -0600, Andrew F. Davis wrote: > This device can detect the insertion/removal of headphones and headsets. > Enable reporting this status by enabling this interrupt and forwarding > this to upper-layers if a jack has been defined. > > This jack definition and the resulting operation from a jack detection > event must currently be defined by sound card platform code until CODEC > outputs to jack mappings can be defined generically. This only does half the job, there's no way for anything to specify a jack here.
On 12/01/2017 07:36 AM, Mark Brown wrote: > On Wed, Nov 29, 2017 at 03:32:55PM -0600, Andrew F. Davis wrote: >> A regulator being forcefully disabled is a catastrophic event that >> should never happen to most devices, especially not sound CODECs. > > That's not what the disable notification handling is for. It's there so > that the driver can skip having to reinitialize the device if other > constraints mean the power doesn't actually get turned off when it > disables the regualtors. It's nothing to do with forced disables. > Looking into the call sites, at least in this case the only time this notification will be called, outside the normal enable/disable paths (which do the same thing here: turn on regmap cache only mode and mark it dirty), will be during a force disable scenario. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2017 07:37 AM, Mark Brown wrote: > On Wed, Nov 29, 2017 at 03:32:56PM -0600, Andrew F. Davis wrote: > >> + gpiod_set_value(aic31xx->gpio_reset, 1); >> + ndelay(10); /* At least 10ns */ >> + gpiod_set_value(aic31xx->gpio_reset, 0); > > _cansleep(), this isn't being done from interrupt context. > will fix >> + } else { >> + ret = regmap_write(aic31xx->regmap, AIC31XX_RESET, 1); >> + if (ret < 0) >> + dev_err(aic31xx->dev, "Could not reset device\n"); > > Print the error to help people doing debug. > Do you mean by adding the ret code to the print? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2017 07:39 AM, Mark Brown wrote: > On Wed, Nov 29, 2017 at 03:32:57PM -0600, Andrew F. Davis wrote: > >> +static irqreturn_t aic31xx_irq(int irq, void *data) >> +{ >> + struct aic31xx_priv *aic31xx = (struct aic31xx_priv *)data; > > There is no need to cast away from void, doing so can only mask bugs. > >> + ret = regmap_read(aic31xx->regmap, AIC31XX_INTRDACFLAG, &value); >> + if (ret) { >> + dev_err(dev, "Failed to read interrupt mask: %d\n", ret); >> + return IRQ_NONE; >> + } >> + >> + if (value & AIC31XX_HPLSCDETECT) >> + dev_err(dev, "Short circuit on Left output is detected\n"); >> + if (value & AIC31XX_HPRSCDETECT) >> + dev_err(dev, "Short circuit on Right output is detected\n"); >> + >> + return IRQ_HANDLED; > > This will report the interrupt as handled even if we didn't see an > interrupt we understand which will break shared interrupt lines. At the > very least we should log other interrupt sources numerically. > Okay, I think I can make that work by checking if no bits are set in the interrupt regs and returning early if not, IRQ_NONE. >> + if (aic31xx->irq > 0) { >> + regmap_update_bits(aic31xx->regmap, AIC31XX_GPIO1, >> + AIC31XX_GPIO1_FUNC_MASK, >> + AIC31XX_GPIO1_INT1 << >> + AIC31XX_GPIO1_FUNC_SHIFT); > > Is the interrupt only available on GPIO1? > Some devices can route this to GPIO2 IIRC. I'm not sure how that would be supported, I think we would need to add interrupt names to DT so users could specify which gpio they wired their IRQ lines to. interrupt = <&host 23>; interrupt-name = "gpio2"; or similar? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 01, 2017 at 09:01:19AM -0600, Andrew F. Davis wrote: > Looking into the call sites, at least in this case the only time this > notification will be called, outside the normal enable/disable paths > (which do the same thing here: turn on regmap cache only mode and mark > it dirty), will be during a force disable scenario. If all the disable sites in the driver mark the cache dirty then they should be fixed not to.
On Fri, Dec 01, 2017 at 09:04:41AM -0600, Andrew F. Davis wrote: > On 12/01/2017 07:37 AM, Mark Brown wrote: > > On Wed, Nov 29, 2017 at 03:32:56PM -0600, Andrew F. Davis wrote: > >> + } else { > >> + ret = regmap_write(aic31xx->regmap, AIC31XX_RESET, 1); > >> + if (ret < 0) > >> + dev_err(aic31xx->dev, "Could not reset device\n"); > > Print the error to help people doing debug. > Do you mean by adding the ret code to the print? Yes.
On Fri, Dec 01, 2017 at 09:32:12AM -0600, Andrew F. Davis wrote: > On 12/01/2017 07:39 AM, Mark Brown wrote: > > Is the interrupt only available on GPIO1? > Some devices can route this to GPIO2 IIRC. > I'm not sure how that would be supported, I think we would need to add > interrupt names to DT so users could specify which gpio they wired their > IRQ lines to. > interrupt = <&host 23>; > interrupt-name = "gpio2"; > or similar? You could also use pinctrl an require the user to mux the interrupt in whatever fashion makes sense for their device.
On 11/29/2017 03:32 PM, Andrew F. Davis wrote: > Move to using newer gpiod_* GPIO handling functions. This simplifies > the code and eases dropping platform data in the next patch. Also > remember GPIO are active low, so set "1" to reset. > > Signed-off-by: Andrew F. Davis <afd@ti.com> > --- Kbuild bot seems mad a this one, looks like I need to include linux/gpio/consumer.h, will fix for v3. > sound/soc/codecs/tlv320aic31xx.c | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/sound/soc/codecs/tlv320aic31xx.c b/sound/soc/codecs/tlv320aic31xx.c > index c84febd991a0..ab03a19f6aaa 100644 > --- a/sound/soc/codecs/tlv320aic31xx.c > +++ b/sound/soc/codecs/tlv320aic31xx.c > @@ -157,6 +157,7 @@ struct aic31xx_priv { > u8 i2c_regs_status; > struct device *dev; > struct regmap *regmap; > + struct gpio_desc *gpio_reset; > struct aic31xx_pdata pdata; > struct regulator_bulk_data supplies[AIC31XX_NUM_SUPPLIES]; > struct aic31xx_disable_nb disable_nb[AIC31XX_NUM_SUPPLIES]; > @@ -1020,8 +1021,8 @@ static int aic31xx_regulator_event(struct notifier_block *nb, > * Put codec to reset and as at least one of the > * supplies was disabled. > */ > - if (gpio_is_valid(aic31xx->pdata.gpio_reset)) > - gpio_set_value(aic31xx->pdata.gpio_reset, 0); > + if (aic31xx->gpio_reset) > + gpiod_set_value(aic31xx->gpio_reset, 1); > > regcache_mark_dirty(aic31xx->regmap); > dev_dbg(aic31xx->dev, "## %s: DISABLE received\n", __func__); > @@ -1073,8 +1074,8 @@ static int aic31xx_power_on(struct snd_soc_codec *codec) > if (ret) > return ret; > > - if (gpio_is_valid(aic31xx->pdata.gpio_reset)) { > - gpio_set_value(aic31xx->pdata.gpio_reset, 1); > + if (aic31xx->gpio_reset) { > + gpiod_set_value(aic31xx->gpio_reset, 0); > udelay(100); > } > regcache_cache_only(aic31xx->regmap, false); > @@ -1334,15 +1335,11 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c, > else if (aic31xx->dev->of_node) > aic31xx_pdata_from_of(aic31xx); > > - if (aic31xx->pdata.gpio_reset) { > - ret = devm_gpio_request_one(aic31xx->dev, > - aic31xx->pdata.gpio_reset, > - GPIOF_OUT_INIT_HIGH, > - "aic31xx-reset-pin"); > - if (ret < 0) { > - dev_err(aic31xx->dev, "not able to acquire gpio\n"); > - return ret; > - } > + aic31xx->gpio_reset = devm_gpiod_get_optional(aic31xx->dev, "reset", > + GPIOD_OUT_LOW); > + if (IS_ERR(aic31xx->gpio_reset)) { > + dev_err(aic31xx->dev, "not able to acquire gpio\n"); > + return PTR_ERR(aic31xx->gpio_reset); > } > > for (i = 0; i < ARRAY_SIZE(aic31xx->supplies); i++) > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2017 07:26 AM, Mark Brown wrote: > On Wed, Nov 29, 2017 at 03:32:50PM -0600, Andrew F. Davis wrote: >> Platform data is not used by anyone (at least in upstream) so >> drop this data and switch to using fwnode(DT/ACPI) only. > > The advantage being...? Not all architectures use DT or ACPI so it's > not clear that this is a step forwards in itself. > Simplifies the code in several places, and you don't need to use DT or ACPI, it probes just fine anyway you normally add an I2C device. All we are dropping here is the platform_data way of specifying mic-bias voltage, which if you are wanting to do that in an out-of-tree board file, then I'm sure you can locally modify this driver to use your wanted voltage setting by default. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/04/2017 10:47 AM, Andrew F. Davis wrote: > On 11/29/2017 03:32 PM, Andrew F. Davis wrote: >> Move to using newer gpiod_* GPIO handling functions. This simplifies >> the code and eases dropping platform data in the next patch. Also >> remember GPIO are active low, so set "1" to reset. >> >> Signed-off-by: Andrew F. Davis <afd@ti.com> >> --- > > > Kbuild bot seems mad a this one, looks like I need to include > linux/gpio/consumer.h, will fix for v3. > Looks like you already have this in your -next branch, how do you want this fix, I can send a delta patch with the added include, a new v3 version that you can replace the patch in-tree with, or if it's easier for you manually fix in-tree? > >> sound/soc/codecs/tlv320aic31xx.c | 23 ++++++++++------------- >> 1 file changed, 10 insertions(+), 13 deletions(-) >> >> diff --git a/sound/soc/codecs/tlv320aic31xx.c b/sound/soc/codecs/tlv320aic31xx.c >> index c84febd991a0..ab03a19f6aaa 100644 >> --- a/sound/soc/codecs/tlv320aic31xx.c >> +++ b/sound/soc/codecs/tlv320aic31xx.c >> @@ -157,6 +157,7 @@ struct aic31xx_priv { >> u8 i2c_regs_status; >> struct device *dev; >> struct regmap *regmap; >> + struct gpio_desc *gpio_reset; >> struct aic31xx_pdata pdata; >> struct regulator_bulk_data supplies[AIC31XX_NUM_SUPPLIES]; >> struct aic31xx_disable_nb disable_nb[AIC31XX_NUM_SUPPLIES]; >> @@ -1020,8 +1021,8 @@ static int aic31xx_regulator_event(struct notifier_block *nb, >> * Put codec to reset and as at least one of the >> * supplies was disabled. >> */ >> - if (gpio_is_valid(aic31xx->pdata.gpio_reset)) >> - gpio_set_value(aic31xx->pdata.gpio_reset, 0); >> + if (aic31xx->gpio_reset) >> + gpiod_set_value(aic31xx->gpio_reset, 1); >> >> regcache_mark_dirty(aic31xx->regmap); >> dev_dbg(aic31xx->dev, "## %s: DISABLE received\n", __func__); >> @@ -1073,8 +1074,8 @@ static int aic31xx_power_on(struct snd_soc_codec *codec) >> if (ret) >> return ret; >> >> - if (gpio_is_valid(aic31xx->pdata.gpio_reset)) { >> - gpio_set_value(aic31xx->pdata.gpio_reset, 1); >> + if (aic31xx->gpio_reset) { >> + gpiod_set_value(aic31xx->gpio_reset, 0); >> udelay(100); >> } >> regcache_cache_only(aic31xx->regmap, false); >> @@ -1334,15 +1335,11 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c, >> else if (aic31xx->dev->of_node) >> aic31xx_pdata_from_of(aic31xx); >> >> - if (aic31xx->pdata.gpio_reset) { >> - ret = devm_gpio_request_one(aic31xx->dev, >> - aic31xx->pdata.gpio_reset, >> - GPIOF_OUT_INIT_HIGH, >> - "aic31xx-reset-pin"); >> - if (ret < 0) { >> - dev_err(aic31xx->dev, "not able to acquire gpio\n"); >> - return ret; >> - } >> + aic31xx->gpio_reset = devm_gpiod_get_optional(aic31xx->dev, "reset", >> + GPIOD_OUT_LOW); >> + if (IS_ERR(aic31xx->gpio_reset)) { >> + dev_err(aic31xx->dev, "not able to acquire gpio\n"); >> + return PTR_ERR(aic31xx->gpio_reset); >> } >> >> for (i = 0; i < ARRAY_SIZE(aic31xx->supplies); i++) >> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 05, 2017 at 03:20:19PM -0600, Andrew F. Davis wrote: > On 12/01/2017 07:26 AM, Mark Brown wrote: > > The advantage being...? Not all architectures use DT or ACPI so it's > > not clear that this is a step forwards in itself. > Simplifies the code in several places, and you don't need to use DT or > ACPI, it probes just fine anyway you normally add an I2C device. > All we are dropping here is the platform_data way of specifying mic-bias > voltage, which if you are wanting to do that in an out-of-tree board > file, then I'm sure you can locally modify this driver to use your > wanted voltage setting by default. Then if you want to upstream the driver you'll have to add the platform data support again. Like I say not all architectures have anything other than board files.
On Tue, Dec 05, 2017 at 03:23:49PM -0600, Andrew F. Davis wrote: > On 12/04/2017 10:47 AM, Andrew F. Davis wrote: > > Kbuild bot seems mad a this one, looks like I need to include > > linux/gpio/consumer.h, will fix for v3. > Looks like you already have this in your -next branch, how do you want > this fix, I can send a delta patch with the added include, a new v3 > version that you can replace the patch in-tree with, or if it's easier > for you manually fix in-tree? As the patch applied mail says: | If any updates are required or you are submitting further changes they | should be sent as incremental updates against current git, existing | patches will not be replaced.
On 12/06/2017 06:45 AM, Mark Brown wrote: > On Tue, Dec 05, 2017 at 03:20:19PM -0600, Andrew F. Davis wrote: >> On 12/01/2017 07:26 AM, Mark Brown wrote: > >>> The advantage being...? Not all architectures use DT or ACPI so it's >>> not clear that this is a step forwards in itself. > >> Simplifies the code in several places, and you don't need to use DT or >> ACPI, it probes just fine anyway you normally add an I2C device. > >> All we are dropping here is the platform_data way of specifying mic-bias >> voltage, which if you are wanting to do that in an out-of-tree board >> file, then I'm sure you can locally modify this driver to use your >> wanted voltage setting by default. > > Then if you want to upstream the driver you'll have to add the platform > data support again. Like I say not all architectures have anything > other than board files. > Then they can try, but they will rightfully get nack'd and told to stop using board files and use DT/ACPI. Most upstream architectures don't use board files anymore anyway, so I doubt this will ever happen. Besides, if they haven't upstreamed their code then it is their problem if this patch breaks them, we shouldn't hold up upstream work for out-of-tree code, especially theoretical out-of-tree code that will never exist. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2017 09:57 AM, Mark Brown wrote: > On Fri, Dec 01, 2017 at 09:32:12AM -0600, Andrew F. Davis wrote: >> On 12/01/2017 07:39 AM, Mark Brown wrote: > >>> Is the interrupt only available on GPIO1? > >> Some devices can route this to GPIO2 IIRC. > >> I'm not sure how that would be supported, I think we would need to add >> interrupt names to DT so users could specify which gpio they wired their >> IRQ lines to. > >> interrupt = <&host 23>; >> interrupt-name = "gpio2"; > >> or similar? > > You could also use pinctrl an require the user to mux the interrupt in > whatever fashion makes sense for their device. > If done at that layer then no change is needed in the driver right? We just request and use the IRQ passed to us from i2c data. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2017 07:41 AM, Mark Brown wrote: > On Wed, Nov 29, 2017 at 03:32:59PM -0600, Andrew F. Davis wrote: >> This device can detect the insertion/removal of headphones and headsets. >> Enable reporting this status by enabling this interrupt and forwarding >> this to upper-layers if a jack has been defined. >> >> This jack definition and the resulting operation from a jack detection >> event must currently be defined by sound card platform code until CODEC >> outputs to jack mappings can be defined generically. > > This only does half the job, there's no way for anything to specify a > jack here. > Other CODECs drivers expose some kind of platform/machine specific function(s) to send the jack definition to the CODEC, we seem to be missing a generic way to report jack information up to the machine layer driver. Perhaps a struct with a jack enable/disable and call-back functions could be created when registering the codec/platform component driver? Then machines can hook to this as they need? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 06, 2017 at 10:19:28AM -0600, Andrew F. Davis wrote: > On 12/06/2017 06:45 AM, Mark Brown wrote: > > Then if you want to upstream the driver you'll have to add the platform > > data support again. Like I say not all architectures have anything > > other than board files. > Then they can try, but they will rightfully get nack'd and told to stop > using board files and use DT/ACPI. Most upstream architectures don't use > board files anymore anyway, so I doubt this will ever happen. No. To repeat, not all architectures use DT or ACPI. Expecting someone to impelement DT or ACPI support for an entire architecture and try to bring the ecosystem for that architecture along in order to add machine support is obviously totally unreasonable.
On Wed, Dec 06, 2017 at 11:25:15AM -0600, Andrew F. Davis wrote: > On 12/01/2017 07:41 AM, Mark Brown wrote: > > This only does half the job, there's no way for anything to specify a > > jack here. > Other CODECs drivers expose some kind of platform/machine specific > function(s) to send the jack definition to the CODEC, we seem to be Your driver doesn't do that. > missing a generic way to report jack information up to the machine layer > driver. snd_soc_codec_set_jack()
On 12/06/2017 11:32 AM, Mark Brown wrote: > On Wed, Dec 06, 2017 at 11:25:15AM -0600, Andrew F. Davis wrote: >> On 12/01/2017 07:41 AM, Mark Brown wrote: > >>> This only does half the job, there's no way for anything to specify a >>> jack here. > >> Other CODECs drivers expose some kind of platform/machine specific >> function(s) to send the jack definition to the CODEC, we seem to be > > Your driver doesn't do that. > Right, I don't have a specific machine in mind so didn't wan to do it that way. >> missing a generic way to report jack information up to the machine layer >> driver. > > snd_soc_codec_set_jack() > Hmm, this looks close to what I was thinking (I was wanting to go the other direction and have the codec report what jacks it supports, this could allow for more than one jack), I'll see if I can make it work. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/2017 11:30 AM, Mark Brown wrote: > On Wed, Dec 06, 2017 at 10:19:28AM -0600, Andrew F. Davis wrote: >> On 12/06/2017 06:45 AM, Mark Brown wrote: > >>> Then if you want to upstream the driver you'll have to add the platform >>> data support again. Like I say not all architectures have anything >>> other than board files. > >> Then they can try, but they will rightfully get nack'd and told to stop >> using board files and use DT/ACPI. Most upstream architectures don't use >> board files anymore anyway, so I doubt this will ever happen. > > No. To repeat, not all architectures use DT or ACPI. Expecting someone > to impelement DT or ACPI support for an entire architecture and try to > bring the ecosystem for that architecture along in order to add machine > support is obviously totally unreasonable. > That would be unreasonable I agree, but it's also completely hypothetical, as again, there are no in-tree users and most platforms are DT/ACPI, so the odds of anyone needing it are next to nothing. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 06, 2017 at 11:47:14AM -0600, Andrew F. Davis wrote: > On 12/06/2017 11:32 AM, Mark Brown wrote: > >> missing a generic way to report jack information up to the machine layer > >> driver. > > snd_soc_codec_set_jack() > Hmm, this looks close to what I was thinking (I was wanting to go the > other direction and have the codec report what jacks it supports, this > could allow for more than one jack), I'll see if I can make it work. Almost always the CODEC requires some extra connections in order to enable jack detection features, these are going to be board specific so they come from the machine driver.
On Wed, Dec 06, 2017 at 11:48:43AM -0600, Andrew F. Davis wrote: > That would be unreasonable I agree, but it's also completely > hypothetical, as again, there are no in-tree users and most platforms > are DT/ACPI, so the odds of anyone needing it are next to nothing. You're removing support for something someone might want to use for no clear gain. The bar for doing that needs to be higher than just random cleanup, it needs to actively bring some benefit that justifies the cost. If something is sitting there not getting in the way and is potentially going to be helpful for something in the future then there needs to be a positive reason to take it away.
On 12/06/2017 12:15 PM, Mark Brown wrote: > On Wed, Dec 06, 2017 at 11:48:43AM -0600, Andrew F. Davis wrote: > >> That would be unreasonable I agree, but it's also completely >> hypothetical, as again, there are no in-tree users and most platforms >> are DT/ACPI, so the odds of anyone needing it are next to nothing. > > You're removing support for something someone might want to use for no > clear gain. The bar for doing that needs to be higher than just random > cleanup, it needs to actively bring some benefit that justifies the > cost. If something is sitting there not getting in the way and is > potentially going to be helpful for something in the future then there > needs to be a positive reason to take it away. > For some userspace feature sure, but this is kernel code, there is no guarantee for a sable API, in fact some would probably argue even further that there is a guarantee that stuff *will* change and this is a good thing as it kinda serves to punish for those you don't try to upstream. So the helpfulness bar should be zero for changes that break out-of-tree stuff. Even more so this patch isn't a zero gain, the cleaner, better looking, and easier to maintain code *is* the benefit in itself. Plus we gain the ability to set mic-gain voltage with ACPI, something you couldn't do before this patch. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 06, 2017 at 12:40:45PM -0600, Andrew F. Davis wrote: > For some userspace feature sure, but this is kernel code, there is no > guarantee for a sable API, in fact some would probably argue even > further that there is a guarantee that stuff *will* change and this is a > good thing as it kinda serves to punish for those you don't try to upstream. > So the helpfulness bar should be zero for changes that break out-of-tree > stuff. There is no need to actively get in people's way or put up barriers to people who do in future decide to upstream things, that doesn't help anyone. > Even more so this patch isn't a zero gain, the cleaner, better looking, > and easier to maintain code *is* the benefit in itself. Plus we gain the > ability to set mic-gain voltage with ACPI, something you couldn't do > before this patch. If this patch adds ACPI support then the patch description was clearly not great (I don't think I read the patch itself since the description just said that it was removing platform data without giving a reason, that's the main review comment here). If you want to use the device property stuff that's fine but there's no need to remove platform data to do that, it's a smaller change. I find it hard to see the platform data as a particularly big blight on the code here, looking at the driver it's just going to remove the "pdata." from a few variable accesses which isn't exactly transformational.
On 12/06/2017 01:11 PM, Mark Brown wrote: > On Wed, Dec 06, 2017 at 12:40:45PM -0600, Andrew F. Davis wrote: > >> For some userspace feature sure, but this is kernel code, there is no >> guarantee for a sable API, in fact some would probably argue even >> further that there is a guarantee that stuff *will* change and this is a >> good thing as it kinda serves to punish for those you don't try to upstream. > >> So the helpfulness bar should be zero for changes that break out-of-tree >> stuff. > > There is no need to actively get in people's way or put up barriers to > people who do in future decide to upstream things, that doesn't help > anyone. > >> Even more so this patch isn't a zero gain, the cleaner, better looking, >> and easier to maintain code *is* the benefit in itself. Plus we gain the >> ability to set mic-gain voltage with ACPI, something you couldn't do >> before this patch. > > If this patch adds ACPI support then the patch description was clearly > not great (I don't think I read the patch itself since the description > just said that it was removing platform data without giving a reason, > that's the main review comment here). > I may not be clear that the ACPI part is new, but the message does say "and switch to using fwnode(DT/ACPI)" > If you want to use the device property stuff that's fine but there's no > need to remove platform data to do that, it's a smaller change. I find > it hard to see the platform data as a particularly big blight on the > code here, looking at the driver it's just going to remove the "pdata." > from a few variable accesses which isn't exactly transformational. > If keeping platform data is that important to you then I will split the patch into fwnode addition and pdata removal, you can just not take the pdata removal if you don't want it. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2017 09:32 AM, Andrew F. Davis wrote: > On 12/01/2017 07:39 AM, Mark Brown wrote: >> On Wed, Nov 29, 2017 at 03:32:57PM -0600, Andrew F. Davis wrote: >> >>> +static irqreturn_t aic31xx_irq(int irq, void *data) >>> +{ >>> + struct aic31xx_priv *aic31xx = (struct aic31xx_priv *)data; >> >> There is no need to cast away from void, doing so can only mask bugs. >> >>> + ret = regmap_read(aic31xx->regmap, AIC31XX_INTRDACFLAG, &value); >>> + if (ret) { >>> + dev_err(dev, "Failed to read interrupt mask: %d\n", ret); >>> + return IRQ_NONE; >>> + } >>> + >>> + if (value & AIC31XX_HPLSCDETECT) >>> + dev_err(dev, "Short circuit on Left output is detected\n"); >>> + if (value & AIC31XX_HPRSCDETECT) >>> + dev_err(dev, "Short circuit on Right output is detected\n"); >>> + >>> + return IRQ_HANDLED; >> >> This will report the interrupt as handled even if we didn't see an >> interrupt we understand which will break shared interrupt lines. At the >> very least we should log other interrupt sources numerically. >> > > Okay, I think I can make that work by checking if no bits are set in the > interrupt regs and returning early if not, IRQ_NONE. > This turned out to be more difficult than I expected, plus if I do handle an interrupt it doesn't mean the other device did not right? So this wouldn't fix shared lines as far as I can tell, but I don't register it as shared so this should be fine. As for your other suggestion of "log other interrupt sources numerically", could you explain this or point to an example of what you mean? >>> + if (aic31xx->irq > 0) { >>> + regmap_update_bits(aic31xx->regmap, AIC31XX_GPIO1, >>> + AIC31XX_GPIO1_FUNC_MASK, >>> + AIC31XX_GPIO1_INT1 << >>> + AIC31XX_GPIO1_FUNC_SHIFT); >> >> Is the interrupt only available on GPIO1? >> > > Some devices can route this to GPIO2 IIRC. > > I'm not sure how that would be supported, I think we would need to add > interrupt names to DT so users could specify which gpio they wired their > IRQ lines to. > > interrupt = <&host 23>; > interrupt-name = "gpio2"; > > or similar? > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 06, 2017 at 02:22:39PM -0600, Andrew F. Davis wrote: > On 12/01/2017 09:32 AM, Andrew F. Davis wrote: > >> This will report the interrupt as handled even if we didn't see an > >> interrupt we understand which will break shared interrupt lines. At the > >> very least we should log other interrupt sources numerically. > > Okay, I think I can make that work by checking if no bits are set in the > > interrupt regs and returning early if not, IRQ_NONE. > This turned out to be more difficult than I expected, plus if I do > handle an interrupt it doesn't mean the other device did not right? So > this wouldn't fix shared lines as far as I can tell, but I don't > register it as shared so this should be fine. It'll mean that we don't offer the interrupt to anything else sharing the line. > As for your other suggestion of "log other interrupt sources > numerically", could you explain this or point to an example of what you > mean? Just print out the bits that were set.
On 12/07/2017 06:03 AM, Mark Brown wrote: > On Wed, Dec 06, 2017 at 02:22:39PM -0600, Andrew F. Davis wrote: >> On 12/01/2017 09:32 AM, Andrew F. Davis wrote: > >>>> This will report the interrupt as handled even if we didn't see an >>>> interrupt we understand which will break shared interrupt lines. At the >>>> very least we should log other interrupt sources numerically. > >>> Okay, I think I can make that work by checking if no bits are set in the >>> interrupt regs and returning early if not, IRQ_NONE. > >> This turned out to be more difficult than I expected, plus if I do >> handle an interrupt it doesn't mean the other device did not right? So >> this wouldn't fix shared lines as far as I can tell, but I don't >> register it as shared so this should be fine. > > It'll mean that we don't offer the interrupt to anything else sharing > the line. > >> As for your other suggestion of "log other interrupt sources >> numerically", could you explain this or point to an example of what you >> mean? > > Just print out the bits that were set. > I don't see anyone else doing this, what would that solve? Maybe I still don't get what you mean here. :( -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 07, 2017 at 09:37:36AM -0600, Andrew F. Davis wrote: > On 12/07/2017 06:03 AM, Mark Brown wrote: > >> As for your other suggestion of "log other interrupt sources > >> numerically", could you explain this or point to an example of what you > >> mean? > > Just print out the bits that were set. > I don't see anyone else doing this, what would that solve? Maybe I still > don't get what you mean here. :( A good proportion of devices require explicit acks for interrupts and only ack things they handled so don't have this issue, and otherwise it's common to just handle all the interrupts that the device might fire. The goal is to not silently eat interrupts if the device starts interrupting for some reason.