| Message ID | 20200409195841.18901-4-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | ASoC/SOF/clk/gpio/dt: add Hifiberry DAC+ PRO support | expand |
On Thu, Apr 09, 2020 at 02:58:28PM -0500, Pierre-Louis Bossart wrote: > Remove direct regmap access, use gpios exposed by PCM512x codec > Keep the codec_init function, this will be used in following patches > > The gpios handling is done with an explicit lookup table. We cannot > use ACPI-based mappings since we don't have an ACPI device for the > machine driver, and the gpiochip is created during the probe of the > PCM512x driver. ... > +#include <linux/gpio/machine.h> Okay, it's a board code. ... > +static struct gpiod_lookup_table pcm512x_gpios_table = { > + /* .dev_id set during probe */ > + .table = { > + GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH), It says GPIO 4 and here is number 3. Does this 4 come from hardware documentation? > + { }, No comma for terminator entries. > + }, > +}; ... > + gpiod_add_lookup_table(&pcm512x_gpios_table); Where is the counterpart gpiod_remove_lookup_table() call? > + ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4", > + GPIOD_OUT_LOW); Can driver work without this GPIO? If so, perhaps devm_gpiod_get_optional(). > + if (IS_ERR(ctx->gpio_4)) { > + dev_err(&pdev->dev, "gpio4 not found\n"); > + ret = PTR_ERR(ctx->gpio_4); > + return ret; > + }
On Tue, Apr 14, 2020 at 08:17:52PM +0300, Andy Shevchenko wrote: > On Thu, Apr 09, 2020 at 02:58:28PM -0500, Pierre-Louis Bossart wrote: > > + GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH), > It says GPIO 4 and here is number 3. > Does this 4 come from hardware documentation? Yes, the first GPIO in the device is GPIO1.
>> +static struct gpiod_lookup_table pcm512x_gpios_table = { >> + /* .dev_id set during probe */ >> + .table = { > >> + GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH), > > It says GPIO 4 and here is number 3. > Does this 4 come from hardware documentation? Yes TI count from 1 to 6 in their documentation. The initial HifiBerry DAC+ also counts from 1 to 6. I can add a comment here. > >> + { }, > > No comma for terminator entries. ok > >> + }, >> +}; > > ... > >> + gpiod_add_lookup_table(&pcm512x_gpios_table); > > Where is the counterpart gpiod_remove_lookup_table() call? Ah that's a miss, thanks for flagging this. I remember looking but obviously needed a coffee at the time. >> + ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4", >> + GPIOD_OUT_LOW); > > Can driver work without this GPIO? If so, perhaps devm_gpiod_get_optional(). that part yes, it's only for the LED, but if this fails then probably the rest of the code will also fail. > >> + if (IS_ERR(ctx->gpio_4)) { >> + dev_err(&pdev->dev, "gpio4 not found\n"); >> + ret = PTR_ERR(ctx->gpio_4); >> + return ret; >> + } >
On Tue, Apr 14, 2020 at 12:57:35PM -0500, Pierre-Louis Bossart wrote: ... > > > + GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH), > > > > It says GPIO 4 and here is number 3. > > Does this 4 come from hardware documentation? > > Yes TI count from 1 to 6 in their documentation. The initial HifiBerry DAC+ > also counts from 1 to 6. I can add a comment here. Okay! ... > > > + ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4", > > > + GPIOD_OUT_LOW); > > > > Can driver work without this GPIO? If so, perhaps devm_gpiod_get_optional(). > > that part yes, it's only for the LED, but if this fails then probably the > rest of the code will also fail. The problem with above code that it's setting the hard dependency to a LED gpio. Is it crucial to get codec working? I bet no. In case gpiod_get_optional() fails, it will be correct to bail out, because it will mean other kind of errors not related to optionality of the GPIO (rather it's present, but something went wrong). > > > + if (IS_ERR(ctx->gpio_4)) { > > > + dev_err(&pdev->dev, "gpio4 not found\n"); > > > + ret = PTR_ERR(ctx->gpio_4); > > > + return ret; > > > + }
diff --git a/sound/soc/intel/boards/sof_pcm512x.c b/sound/soc/intel/boards/sof_pcm512x.c index fb7811899999..dcd769b352fa 100644 --- a/sound/soc/intel/boards/sof_pcm512x.c +++ b/sound/soc/intel/boards/sof_pcm512x.c @@ -10,6 +10,8 @@ #include <linux/i2c.h> #include <linux/input.h> #include <linux/module.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio/machine.h> #include <linux/platform_device.h> #include <linux/types.h> #include <sound/core.h> @@ -43,6 +45,7 @@ struct sof_hdmi_pcm { struct sof_card_private { struct list_head hdmi_pcm_list; bool idisp_codec; + struct gpio_desc *gpio_4; }; static int sof_pcm512x_quirk_cb(const struct dmi_system_id *id) @@ -84,23 +87,16 @@ static int sof_hdmi_init(struct snd_soc_pcm_runtime *rtd) static int sof_pcm512x_codec_init(struct snd_soc_pcm_runtime *rtd) { - struct snd_soc_component *codec = asoc_rtd_to_codec(rtd, 0)->component; - - snd_soc_component_update_bits(codec, PCM512x_GPIO_EN, 0x08, 0x08); - snd_soc_component_update_bits(codec, PCM512x_GPIO_OUTPUT_4, 0x0f, 0x02); - snd_soc_component_update_bits(codec, PCM512x_GPIO_CONTROL_1, - 0x08, 0x08); - return 0; } static int aif1_startup(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_soc_component *codec = asoc_rtd_to_codec(rtd, 0)->component; + struct sof_card_private *ctx = snd_soc_card_get_drvdata(rtd->card); - snd_soc_component_update_bits(codec, PCM512x_GPIO_CONTROL_1, - 0x08, 0x08); + /* Turn LED on */ + gpiod_set_value(ctx->gpio_4, 1); return 0; } @@ -108,10 +104,10 @@ static int aif1_startup(struct snd_pcm_substream *substream) static void aif1_shutdown(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_soc_component *codec = asoc_rtd_to_codec(rtd, 0)->component; + struct sof_card_private *ctx = snd_soc_card_get_drvdata(rtd->card); - snd_soc_component_update_bits(codec, PCM512x_GPIO_CONTROL_1, - 0x08, 0x00); + /* Turn LED off */ + gpiod_set_value(ctx->gpio_4, 0); } static const struct snd_soc_ops sof_pcm512x_ops = { @@ -354,6 +350,14 @@ static struct snd_soc_dai_link *sof_card_dai_links_create(struct device *dev, return NULL; } +static struct gpiod_lookup_table pcm512x_gpios_table = { + /* .dev_id set during probe */ + .table = { + GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH), + { }, + }, +}; + static int sof_audio_probe(struct platform_device *pdev) { struct snd_soc_acpi_mach *mach = pdev->dev.platform_data; @@ -413,6 +417,21 @@ static int sof_audio_probe(struct platform_device *pdev) snd_soc_card_set_drvdata(&sof_audio_card_pcm512x, ctx); + /* + * Enable GPIO4 for LED + */ + pcm512x_gpios_table.dev_id = dev_name(&pdev->dev); + gpiod_add_lookup_table(&pcm512x_gpios_table); + + ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4", + GPIOD_OUT_LOW); + + if (IS_ERR(ctx->gpio_4)) { + dev_err(&pdev->dev, "gpio4 not found\n"); + ret = PTR_ERR(ctx->gpio_4); + return ret; + } + return devm_snd_soc_register_card(&pdev->dev, &sof_audio_card_pcm512x); }
Remove direct regmap access, use gpios exposed by PCM512x codec Keep the codec_init function, this will be used in following patches The gpios handling is done with an explicit lookup table. We cannot use ACPI-based mappings since we don't have an ACPI device for the machine driver, and the gpiochip is created during the probe of the PCM512x driver. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/soc/intel/boards/sof_pcm512x.c | 45 ++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 13 deletions(-)