On Thu, Mar 18, 2021 at 01:06:10PM +0000, Mark Brown wrote:
> On Wed, Mar 17, 2021 at 03:21:15PM -0500, Chris Morgan wrote:
> > Add support for the Rockchip rk817 audio codec integrated into the
> > rk817 PMIC. This is based on the sources provided by Rockchip from
> > their BSP kernel.
>
> Modulo the issue with the compatible in the binding this looks good
> apart from a couple of small nits:
>
> > +static int rk817_set_component_pll(struct snd_soc_component *component,
> > + int pll_id, int source, unsigned int freq_in,
> > + unsigned int freq_out)
> > +{
> > + snd_soc_component_write(component, RK817_CODEC_APLL_CFG1, 0x58);
>
> This should really validate freq_in and freq_out, confirming that
> they're whatever fixed values this register sequence is for (if you
> don't know what freq_out actually is it's more OK to skip that than
> freq_in I guess since the constraints on the DAI link should ensure we
> end up with a sensible value).
Unfortunately I don't know which values I should validate. While the data
sheet has these fields "documented" it doesn't have the units, so I don't know
if I'm close in the minimum/maximum range or not. I will add documentation to
the routine for each step of what I'm doing at least though. If better
documentation becomes available or a second implementation presents itself we
can update this to validate.
https://rockchip.fr/RK817%20datasheet%20V1.01.pdf
>
> > + snd_soc_component_write(component, RK817_CODEC_APLL_CFG2, 0x2d);
> > + snd_soc_component_write(component, RK817_CODEC_APLL_CFG3, 0x0c);
> > + snd_soc_component_write(component, RK817_CODEC_APLL_CFG4, 0xa5);
> > +
> > + return 0;
> > +};
>
> No ; at the end of the function definition.
Acknowledged.
