Re: [PATCH 5/7] soundwire: intel: follow documentation sequences for SHIM registers

Pierre-Louis Bossart
2020-03-20T18:18:03Z

On 3/20/20 8:51 AM, Vinod Koul wrote:
> On 11-03-20, 17:10, Pierre-Louis Bossart wrote:
>> From: Rander Wang <rander.wang@intel.com>
>>
>> Somehow the existing code is not aligned with the steps described in
>> the documentation, refactor code and make sure the register
>
> Is the documentation available public space so that we can correct

No, so you'll have to trust us blindly on this one.

>> @@ -283,11 +284,48 @@ static int intel_link_power_up(struct sdw_intel *sdw)
>> {
>> unsigned int link_id = sdw->instance;
>> void __iomem *shim = sdw->link_res->shim;
>> + u32 *shim_mask = sdw->link_res->shim_mask;
>
> this is a local pointer, so the one defined previously is not used.

No idea what you are saying, it's the same address so changes to
*shim_mask will be the same as in *sdw->link_res->shim_mask.

>
>> + struct sdw_bus *bus = &sdw->cdns.bus;
>> + struct sdw_master_prop *prop = &bus->prop;
>> int spa_mask, cpa_mask;
>> - int link_control, ret;
>> + int link_control;
>> + int ret = 0;
>> + u32 syncprd;
>> + u32 sync_reg;
>>
>> mutex_lock(sdw->link_res->shim_lock);
>>
>> + /*
>> + * The hardware relies on an internal counter,
>> + * typically 4kHz, to generate the SoundWire SSP -
>> + * which defines a 'safe' synchronization point
>> + * between commands and audio transport and allows for
>> + * multi link synchronization. The SYNCPRD value is
>> + * only dependent on the oscillator clock provided to
>> + * the IP, so adjust based on _DSD properties reported
>> + * in DSDT tables. The values reported are based on
>> + * either 24MHz (CNL/CML) or 38.4 MHz (ICL/TGL+).
>
> Sorry this looks quite bad to read, we have 80 chars, so please use
> like below:
>
> /*
> * The hardware relies on an internal counter, typically 4kHz,
> * to generate the SoundWire SSP - which defines a 'safe'
> * synchronization point between commands and audio transport
> * and allows for multi link synchronization. The SYNCPRD value
> * is only dependent on the oscillator clock provided to
> * the IP, so adjust based on _DSD properties reported in DSDT
> * tables. The values reported are based on either 24MHz
> * (CNL/CML) or 38.4 MHz (ICL/TGL+).
> */

Are we really going to have an emacs vs vi discussion here?