| Message ID | 20171129213300.20021-10-afd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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.
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 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.
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 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.
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.
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.
diff --git a/sound/soc/codecs/tlv320aic31xx.c b/sound/soc/codecs/tlv320aic31xx.c index ab03a19f6aaa..0e00421d363b 100644 --- a/sound/soc/codecs/tlv320aic31xx.c +++ b/sound/soc/codecs/tlv320aic31xx.c @@ -157,8 +157,9 @@ struct aic31xx_priv { u8 i2c_regs_status; struct device *dev; struct regmap *regmap; + enum aic31xx_type codec_type; struct gpio_desc *gpio_reset; - struct aic31xx_pdata pdata; + int micbias_vg; struct regulator_bulk_data supplies[AIC31XX_NUM_SUPPLIES]; struct aic31xx_disable_nb disable_nb[AIC31XX_NUM_SUPPLIES]; unsigned int sysclk; @@ -450,7 +451,7 @@ static int mic_bias_event(struct snd_soc_dapm_widget *w, /* change mic bias voltage to user defined */ snd_soc_update_bits(codec, AIC31XX_MICBIAS, AIC31XX_MICBIAS_MASK, - aic31xx->pdata.micbias_vg << + aic31xx->micbias_vg << AIC31XX_MICBIAS_SHIFT); dev_dbg(codec->dev, "%s: turned on\n", __func__); break; @@ -673,14 +674,14 @@ static int aic31xx_add_controls(struct snd_soc_codec *codec) int ret = 0; struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec); - if (!(aic31xx->pdata.codec_type & DAC31XX_BIT)) + if (!(aic31xx->codec_type & DAC31XX_BIT)) ret = snd_soc_add_codec_controls( codec, aic31xx_snd_controls, ARRAY_SIZE(aic31xx_snd_controls)); if (ret) return ret; - if (aic31xx->pdata.codec_type & AIC31XX_STEREO_CLASS_D_BIT) + if (aic31xx->codec_type & AIC31XX_STEREO_CLASS_D_BIT) ret = snd_soc_add_codec_controls( codec, aic311x_snd_controls, ARRAY_SIZE(aic311x_snd_controls)); @@ -698,7 +699,7 @@ static int aic31xx_add_widgets(struct snd_soc_codec *codec) struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec); int ret = 0; - if (aic31xx->pdata.codec_type & DAC31XX_BIT) { + if (aic31xx->codec_type & DAC31XX_BIT) { ret = snd_soc_dapm_new_controls( dapm, dac31xx_dapm_widgets, ARRAY_SIZE(dac31xx_dapm_widgets)); @@ -722,7 +723,7 @@ static int aic31xx_add_widgets(struct snd_soc_codec *codec) return ret; } - if (aic31xx->pdata.codec_type & AIC31XX_STEREO_CLASS_D_BIT) { + if (aic31xx->codec_type & AIC31XX_STEREO_CLASS_D_BIT) { ret = snd_soc_dapm_new_controls( dapm, aic311x_dapm_widgets, ARRAY_SIZE(aic311x_dapm_widgets)); @@ -1257,42 +1258,6 @@ static const struct of_device_id tlv320aic31xx_of_match[] = { {}, }; MODULE_DEVICE_TABLE(of, tlv320aic31xx_of_match); - -static void aic31xx_pdata_from_of(struct aic31xx_priv *aic31xx) -{ - struct device_node *np = aic31xx->dev->of_node; - unsigned int value = MICBIAS_2_0V; - int ret; - - of_property_read_u32(np, "ai31xx-micbias-vg", &value); - switch (value) { - case MICBIAS_2_0V: - case MICBIAS_2_5V: - case MICBIAS_AVDDV: - aic31xx->pdata.micbias_vg = value; - break; - default: - dev_err(aic31xx->dev, - "Bad ai31xx-micbias-vg value %d DT\n", - value); - aic31xx->pdata.micbias_vg = MICBIAS_2_0V; - } - - ret = of_get_named_gpio(np, "reset-gpios", 0); - if (ret > 0) { - aic31xx->pdata.gpio_reset = ret; - } else { - ret = of_get_named_gpio(np, "gpio-reset", 0); - if (ret > 0) { - dev_warn(aic31xx->dev, "Using deprecated property \"gpio-reset\", please update your DT"); - aic31xx->pdata.gpio_reset = ret; - } - } -} -#else /* CONFIG_OF */ -static void aic31xx_pdata_from_of(struct aic31xx_priv *aic31xx) -{ -} #endif /* CONFIG_OF */ #ifdef CONFIG_ACPI @@ -1307,6 +1272,7 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct aic31xx_priv *aic31xx; + unsigned int micbias_value = MICBIAS_2_0V; int i, ret; dev_dbg(&i2c->dev, "## %s: %s codec_type = %d\n", __func__, @@ -1325,15 +1291,23 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c, } aic31xx->dev = &i2c->dev; - aic31xx->pdata.codec_type = id->driver_data; + aic31xx->codec_type = id->driver_data; dev_set_drvdata(aic31xx->dev, aic31xx); - if (dev_get_platdata(aic31xx->dev)) - memcpy(&aic31xx->pdata, dev_get_platdata(aic31xx->dev), - sizeof(aic31xx->pdata)); - else if (aic31xx->dev->of_node) - aic31xx_pdata_from_of(aic31xx); + fwnode_property_read_u32(aic31xx->dev->fwnode, "ai31xx-micbias-vg", + &micbias_value); + switch (micbias_value) { + case MICBIAS_2_0V: + case MICBIAS_2_5V: + case MICBIAS_AVDDV: + aic31xx->micbias_vg = micbias_value; + break; + default: + dev_err(aic31xx->dev, "Bad ai31xx-micbias-vg value %d in DT\n", + micbias_value); + aic31xx->micbias_vg = MICBIAS_2_0V; + } aic31xx->gpio_reset = devm_gpiod_get_optional(aic31xx->dev, "reset", GPIOD_OUT_LOW); @@ -1353,7 +1327,7 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c, return ret; } - if (aic31xx->pdata.codec_type & DAC31XX_BIT) + if (aic31xx->codec_type & DAC31XX_BIT) return snd_soc_register_codec(&i2c->dev, &soc_codec_driver_aic31xx, dac31xx_dai_driver, diff --git a/sound/soc/codecs/tlv320aic31xx.h b/sound/soc/codecs/tlv320aic31xx.h index 15ac7cba86fe..ab94e6a0c742 100644 --- a/sound/soc/codecs/tlv320aic31xx.h +++ b/sound/soc/codecs/tlv320aic31xx.h @@ -29,12 +29,6 @@ enum aic31xx_type { DAC3101 = DAC31XX_BIT | AIC31XX_STEREO_CLASS_D_BIT, }; -struct aic31xx_pdata { - enum aic31xx_type codec_type; - unsigned int gpio_reset; - int micbias_vg; -}; - #define AIC31XX_REG(page, reg) ((page * 128) + reg) #define AIC31XX_PAGECTL AIC31XX_REG(0, 0) /* Page Control Register */
Platform data is not used by anyone (at least in upstream) so drop this data and switch to using fwnode(DT/ACPI) only. Signed-off-by: Andrew F. Davis <afd@ti.com> --- sound/soc/codecs/tlv320aic31xx.c | 72 +++++++++++++--------------------------- sound/soc/codecs/tlv320aic31xx.h | 6 ---- 2 files changed, 23 insertions(+), 55 deletions(-)