lkml.org 
[lkml]   [2023]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 4/5] ASoC: codecs: Add WCD939x Soundwire devices driver
On 13/12/2023 19:31, Mark Brown wrote:
> On Thu, Dec 07, 2023 at 11:28:07AM +0100, Neil Armstrong wrote:
>> Add Soundwire Slave driver for the WCD9390/WCD9395 Audio Codec.
>>
>> The WCD9390/WCD9395 Soundwire devices will be used by the
>> main WCD9390/WCD9395 Audio Codec driver to access registers
>> and configure Soundwire RX and TX ports.
>
>> +static const struct reg_default wcd939x_defaults[] = {
>
>> + { WCD939X_DIGITAL_MODE_STATUS_0, 0x00 },
>> + { WCD939X_DIGITAL_MODE_STATUS_1, 0x00 },
>
> There's a bunch of registers like this which look like they should be
> volatile and are actually volatile which makes supplying defaults rather
> strange - in general volatile registers shouldn't have defaults.

Indeed I'll clean those up

>
>> + { WCD939X_DIGITAL_EFUSE_REG_0, 0x00 },
>> + { WCD939X_DIGITAL_EFUSE_REG_1, 0xff },
>> + { WCD939X_DIGITAL_EFUSE_REG_2, 0xff },
>
> With the fuse registers even though I'd expect them to be cachable the
> whole point is usually that these are programmable per device and
> therefore I'd not expect defaults, I'd expect them to be cached on first
> use.

Ack

>
>> +static bool wcd939x_readonly_register(struct device *dev, unsigned int reg)
>> +{
>
>> + case WCD939X_DIGITAL_CHIP_ID0:
>> + case WCD939X_DIGITAL_CHIP_ID1:
>> + case WCD939X_DIGITAL_CHIP_ID2:
>> + case WCD939X_DIGITAL_CHIP_ID3:
>
>> + case WCD939X_DIGITAL_EFUSE_REG_0:
>> + case WCD939X_DIGITAL_EFUSE_REG_1:
>> + case WCD939X_DIGITAL_EFUSE_REG_2:
>
>> + /* Consider all readonly registers as volatile */
>> + .volatile_reg = wcd939x_readonly_register,
>
> There's a bunch of the readonly registers that I'd expect to be cachable
> at runtime - I *hope* the chip ID doesn't change at runtime! OTOH it
> likely doesn't matter so perhaps it's fine but the comment could use
> some improvement.


I'll improve this

>
>> +static int wcd939x_sdw_component_bind(struct device *dev, struct device *master,
>> + void *data)
>> +{
>> + /* Bind is required by component framework */
>> + return 0;
>> +}
>> +
>> +static void wcd939x_sdw_component_unbind(struct device *dev,
>> + struct device *master, void *data)
>> +{
>> + /* Unbind is required by component framework */
>> +}
>> +
>> +static const struct component_ops wcd939x_sdw_component_ops = {
>> + .bind = wcd939x_sdw_component_bind,
>> + .unbind = wcd939x_sdw_component_unbind,
>> +};
>
> So what exactly is the component framework *doing* here then? It really
> would be better to get this fixed in the component framework if this is
> a sensible usage.

So the component framework is here to synchronize probes of the main codec
and soundwire devices, because the main codec needs the soundwire devices
to access registers.
I assume this design was chosen to limit probe defer infinite loops waiting
for the soundwire devices to probe

I'll propose a change on the component framework, without any insurance it
would be accepted.

>
>> +static int __maybe_unused wcd939x_sdw_runtime_resume(struct device *dev)
>> +{
>> + struct wcd939x_sdw_priv *wcd = dev_get_drvdata(dev);
>> +
>> + if (wcd->regmap) {
>> + regcache_cache_only(wcd->regmap, false);
>> + regcache_sync(wcd->regmap);
>> + }
>> +
>> + pm_runtime_mark_last_busy(dev);
>
> The pm_runtime_mark_last_busy() in the resume function is a bit of a
> weird pattern - usually this is something that the user updates and more
> normally when releasing a runtime PM reference.

I took this from wcd938x_sd, I'll check the rationale of it in the resume function.

Thanks,
Neil

\
 
 \ /
  Last update: 2023-12-15 14:20    [W:0.054 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site