| Message ID | 20190722135209.30302-1-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 748fd07e2b9ca4132e3d2aae25395aedc4d1aee8 |
| Headers | show |
| Series | [v2] ASoC: madera: Read device tree configuration | expand |
On 2019-07-22 15:52, Charles Keepax wrote: > +static void madera_prop_get_inmode(struct madera_priv *priv) > +{ > + struct madera *madera = priv->madera; > + struct madera_codec_pdata *pdata = &madera->pdata.codec; > + u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS]; > + int n, i, in_idx, ch_idx; > + > + BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT); > + BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS); > + > + n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode", > + tmp, ARRAY_SIZE(tmp), > + MADERA_MAX_MUXED_CHANNELS); > + if (n < 0) > + return; > + > + in_idx = 0; > + ch_idx = 0; > + for (i = 0; i < n; ++i) { > + pdata->inmode[in_idx][ch_idx] = tmp[i]; > + > + if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) { > + ch_idx = 0; > + ++in_idx; > + } > + } > +} > + > +static void madera_prop_get_pdata(struct madera_priv *priv) > +{ > + struct madera *madera = priv->madera; > + struct madera_codec_pdata *pdata = &madera->pdata.codec; > + u32 out_mono[ARRAY_SIZE(pdata->out_mono)]; > + int i, n; > + > + madera_prop_get_inmode(priv); > + > + n = madera_get_variable_u32_array(madera->dev, "cirrus,out-mono", > + out_mono, ARRAY_SIZE(out_mono), 1); > + if (n > 0) > + for (i = 0; i < n; ++i) > + pdata->out_mono[i] = !!out_mono[i]; > + > + madera_get_variable_u32_array(madera->dev, > + "cirrus,max-channels-clocked", > + pdata->max_channels_clocked, > + ARRAY_SIZE(pdata->max_channels_clocked), > + 1); > + > + madera_get_variable_u32_array(madera->dev, "cirrus,pdm-fmt", > + pdata->pdm_fmt, > + ARRAY_SIZE(pdata->pdm_fmt), 1); > + > + madera_get_variable_u32_array(madera->dev, "cirrus,pdm-mute", > + pdata->pdm_mute, > + ARRAY_SIZE(pdata->pdm_mute), 1); > + > + madera_get_variable_u32_array(madera->dev, "cirrus,dmic-ref", > + pdata->dmic_ref, > + ARRAY_SIZE(pdata->dmic_ref), 1); Hmm, madera_get_variable_u32_array calls are not permissive within madera_prop_get_inmode yet here they are. Is this intentional? > +} > + > int madera_core_init(struct madera_priv *priv) > { > int i; > @@ -308,6 +402,9 @@ int madera_core_init(struct madera_priv *priv) > BUILD_BUG_ON(!madera_mixer_texts[MADERA_NUM_MIXER_INPUTS - 1]); > BUILD_BUG_ON(!madera_mixer_values[MADERA_NUM_MIXER_INPUTS - 1]); > > + if (!dev_get_platdata(priv->madera->dev)) > + madera_prop_get_pdata(priv); > + > mutex_init(&priv->rate_lock); > > for (i = 0; i < MADERA_MAX_HP_OUTPUT; i++) >
On Tue, Jul 23, 2019 at 12:07:32AM +0200, Cezary Rojewski wrote: > On 2019-07-22 15:52, Charles Keepax wrote: > >+static void madera_prop_get_inmode(struct madera_priv *priv) > >+{ > >+ struct madera *madera = priv->madera; > >+ struct madera_codec_pdata *pdata = &madera->pdata.codec; > >+ u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS]; > >+ int n, i, in_idx, ch_idx; > >+ > >+ BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT); > >+ BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS); > >+ > >+ n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode", > >+ tmp, ARRAY_SIZE(tmp), > >+ MADERA_MAX_MUXED_CHANNELS); > >+ if (n < 0) > >+ return; > >+ > >+ in_idx = 0; > >+ ch_idx = 0; > >+ for (i = 0; i < n; ++i) { > >+ pdata->inmode[in_idx][ch_idx] = tmp[i]; > >+ > >+ if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) { > >+ ch_idx = 0; > >+ ++in_idx; > >+ } > >+ } > >+} > >+ > >+static void madera_prop_get_pdata(struct madera_priv *priv) > >+{ > >+ struct madera *madera = priv->madera; > >+ struct madera_codec_pdata *pdata = &madera->pdata.codec; > >+ u32 out_mono[ARRAY_SIZE(pdata->out_mono)]; > >+ int i, n; > >+ > >+ madera_prop_get_inmode(priv); > > Hmm, madera_get_variable_u32_array calls are not permissive within > madera_prop_get_inmode yet here they are. Is this intentional? > Apologies but could you clarify what you mean by "not permissive"? I can't see anything that would prevent the function from being called (indeed it builds and works), and the binding documentation does specify that this field can be of variable size. Thanks, Charles
On 2019-07-23 10:17, Charles Keepax wrote: > On Tue, Jul 23, 2019 at 12:07:32AM +0200, Cezary Rojewski wrote: >> On 2019-07-22 15:52, Charles Keepax wrote: >>> +static void madera_prop_get_inmode(struct madera_priv *priv) >>> +{ >>> + struct madera *madera = priv->madera; >>> + struct madera_codec_pdata *pdata = &madera->pdata.codec; >>> + u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS]; >>> + int n, i, in_idx, ch_idx; >>> + >>> + BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT); >>> + BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS); >>> + >>> + n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode", >>> + tmp, ARRAY_SIZE(tmp), >>> + MADERA_MAX_MUXED_CHANNELS); >>> + if (n < 0) >>> + return; >>> + >>> + in_idx = 0; >>> + ch_idx = 0; >>> + for (i = 0; i < n; ++i) { >>> + pdata->inmode[in_idx][ch_idx] = tmp[i]; >>> + >>> + if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) { >>> + ch_idx = 0; >>> + ++in_idx; >>> + } >>> + } >>> +} >>> + >>> +static void madera_prop_get_pdata(struct madera_priv *priv) >>> +{ >>> + struct madera *madera = priv->madera; >>> + struct madera_codec_pdata *pdata = &madera->pdata.codec; >>> + u32 out_mono[ARRAY_SIZE(pdata->out_mono)]; >>> + int i, n; >>> + >>> + madera_prop_get_inmode(priv); >> >> Hmm, madera_get_variable_u32_array calls are not permissive within >> madera_prop_get_inmode yet here they are. Is this intentional? >> > > Apologies but could you clarify what you mean by "not > permissive"? > > I can't see anything that would prevent the function from > being called (indeed it builds and works), and the binding > documentation does specify that this field can be of variable > size. > > Thanks, > Charles No worries. By "permissive" I described the usage of _get_variable_u32_array within madera_prop_get_pdata. In madera_prop_get_inmode you do care about the return value. In madera_prop_get_pdata however, you ignore all of them. From _get_variable_u32_array declaration, it seems function may fail. Sometimes it's desired to be permissive, simply asking if that's intentional. Czarek
On Tue, Jul 23, 2019 at 04:21:41PM +0200, Cezary Rojewski wrote: > On 2019-07-23 10:17, Charles Keepax wrote: > >On Tue, Jul 23, 2019 at 12:07:32AM +0200, Cezary Rojewski wrote: > >>On 2019-07-22 15:52, Charles Keepax wrote: > >>>+static void madera_prop_get_inmode(struct madera_priv *priv) > >>>+ > >>>+ n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode", > >>>+ tmp, ARRAY_SIZE(tmp), > >>>+ MADERA_MAX_MUXED_CHANNELS); > >>>+ if (n < 0) > >>>+ return; > >> > >>Hmm, madera_get_variable_u32_array calls are not permissive within > >>madera_prop_get_inmode yet here they are. Is this intentional? > >> > > > >Apologies but could you clarify what you mean by "not > >permissive"? > > > >I can't see anything that would prevent the function from > >being called (indeed it builds and works), and the binding > >documentation does specify that this field can be of variable > >size. > > > >Thanks, > >Charles > > No worries. By "permissive" I described the usage of > _get_variable_u32_array within madera_prop_get_pdata. In > madera_prop_get_inmode you do care about the return value. In > madera_prop_get_pdata however, you ignore all of them. From > _get_variable_u32_array declaration, it seems function may fail. > Sometimes it's desired to be permissive, simply asking if that's > intentional. > Ah.. yes I follow. Yes this is intentional, all the DT fields in question are optional so the driver should proceed if even if they are corrupt or missing. madera_prop_get_inmode checks the return value because additional code is required to insert the values into the pdata structure, so that code should be skipped in the case cirrus,inmode is not present/fails. Most of the calls in madera_prop_get_pdata are self-contained not requiring further handling other than reading the data directly into the pdata structure. Uou can see the same on the read of cirrus,out-mono the additional processing is skipped in the case of an error. Thanks, Charles
On Tue, Jul 23, 2019 at 04:01:25PM +0100, Charles Keepax wrote: > Ah.. yes I follow. Yes this is intentional, all the DT fields in > question are optional so the driver should proceed if even if they > are corrupt or missing. If they're present but unparsable you should probably say something about that really. That's clearly different to optional.
On Tue, Jul 23, 2019 at 04:03:02PM +0100, Mark Brown wrote: > On Tue, Jul 23, 2019 at 04:01:25PM +0100, Charles Keepax wrote: > > > Ah.. yes I follow. Yes this is intentional, all the DT fields in > > question are optional so the driver should proceed if even if they > > are corrupt or missing. > > If they're present but unparsable you should probably say something > about that really. That's clearly different to optional. The helper logs an error message itself since essentially the only difference in the messages is the name of the property, there is just no need to process the return value since we would take the same code path regardless of failure or success. Thanks, Charles
diff --git a/sound/soc/codecs/madera.c b/sound/soc/codecs/madera.c index 1b1be19a2f991..5f1e32a5a8556 100644 --- a/sound/soc/codecs/madera.c +++ b/sound/soc/codecs/madera.c @@ -300,6 +300,100 @@ int madera_free_overheat(struct madera_priv *priv) } EXPORT_SYMBOL_GPL(madera_free_overheat); +static int madera_get_variable_u32_array(struct device *dev, + const char *propname, + u32 *dest, int n_max, + int multiple) +{ + int n, ret; + + n = device_property_count_u32(dev, propname); + if (n < 0) { + if (n == -EINVAL) + return 0; /* missing, ignore */ + + dev_warn(dev, "%s malformed (%d)\n", propname, n); + + return n; + } else if ((n % multiple) != 0) { + dev_warn(dev, "%s not a multiple of %d entries\n", + propname, multiple); + + return -EINVAL; + } + + if (n > n_max) + n = n_max; + + ret = device_property_read_u32_array(dev, propname, dest, n); + if (ret < 0) + return ret; + + return n; +} + +static void madera_prop_get_inmode(struct madera_priv *priv) +{ + struct madera *madera = priv->madera; + struct madera_codec_pdata *pdata = &madera->pdata.codec; + u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS]; + int n, i, in_idx, ch_idx; + + BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT); + BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS); + + n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode", + tmp, ARRAY_SIZE(tmp), + MADERA_MAX_MUXED_CHANNELS); + if (n < 0) + return; + + in_idx = 0; + ch_idx = 0; + for (i = 0; i < n; ++i) { + pdata->inmode[in_idx][ch_idx] = tmp[i]; + + if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) { + ch_idx = 0; + ++in_idx; + } + } +} + +static void madera_prop_get_pdata(struct madera_priv *priv) +{ + struct madera *madera = priv->madera; + struct madera_codec_pdata *pdata = &madera->pdata.codec; + u32 out_mono[ARRAY_SIZE(pdata->out_mono)]; + int i, n; + + madera_prop_get_inmode(priv); + + n = madera_get_variable_u32_array(madera->dev, "cirrus,out-mono", + out_mono, ARRAY_SIZE(out_mono), 1); + if (n > 0) + for (i = 0; i < n; ++i) + pdata->out_mono[i] = !!out_mono[i]; + + madera_get_variable_u32_array(madera->dev, + "cirrus,max-channels-clocked", + pdata->max_channels_clocked, + ARRAY_SIZE(pdata->max_channels_clocked), + 1); + + madera_get_variable_u32_array(madera->dev, "cirrus,pdm-fmt", + pdata->pdm_fmt, + ARRAY_SIZE(pdata->pdm_fmt), 1); + + madera_get_variable_u32_array(madera->dev, "cirrus,pdm-mute", + pdata->pdm_mute, + ARRAY_SIZE(pdata->pdm_mute), 1); + + madera_get_variable_u32_array(madera->dev, "cirrus,dmic-ref", + pdata->dmic_ref, + ARRAY_SIZE(pdata->dmic_ref), 1); +} + int madera_core_init(struct madera_priv *priv) { int i; @@ -308,6 +402,9 @@ int madera_core_init(struct madera_priv *priv) BUILD_BUG_ON(!madera_mixer_texts[MADERA_NUM_MIXER_INPUTS - 1]); BUILD_BUG_ON(!madera_mixer_values[MADERA_NUM_MIXER_INPUTS - 1]); + if (!dev_get_platdata(priv->madera->dev)) + madera_prop_get_pdata(priv); + mutex_init(&priv->rate_lock); for (i = 0; i < MADERA_MAX_HP_OUTPUT; i++)
Read the configuration of the Madera ASoC driver from device tree. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- Changes since v1: - Returned to using a helping in the madera driver itself, discussions around adding a generic helper indicated that all the error reporting would still need to be contained in the driver which constitutes the vast majority of the code in the helper anyway. - Updated the code to use the new device_property_count_u32 helper. Thanks, Charles sound/soc/codecs/madera.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+)