Re: [PATCH v2 3/4] clk: rs9: Support device specific dif bit calculation

[Marek Vasut <marex@denx.de>]

On 1/10/23 14:47, Alexander Stein wrote:
> Hello Marek,
> 
> Am Dienstag, 10. Januar 2023, 14:37:19 CET schrieb Marek Vasut:
>> On 1/10/23 14:22, Alexander Stein wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> thanks for your feedback.
>>>
>>> Am Dienstag, 10. Januar 2023, 11:31:49 CET schrieb Marek Vasut:
>>>> On 1/10/23 11:00, Alexander Stein wrote:
>>>>
>>>> [...]
>>>>
>>>>>     static int rs9_get_output_config(struct rs9_driver_data *rs9, int
>>>>>     idx)
>>>>>     {
>>>>>     
>>>>>     	struct i2c_client *client = rs9->client;
>>>>>
>>>>> +	u8 dif = rs9_calc_dif(rs9, idx);
>>>>>
>>>>>     	unsigned char name[5] = "DIF0";
>>>>>     	struct device_node *np;
>>>>>     	int ret;
>>>>>     	u32 sr;
>>>>>     	
>>>>>     	/* Set defaults */
>>>>>
>>>>> -	rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
>>>>
>>>> Are you sure this line ^ should be dropped ?
>>>> Shouldn't the bitfield be cleared first and modified second?
>>>
>>> Well, I had in my mind that this function is called upon probe with
>>> clk_dif_sr being cleared anyway, so this does essentially nothing. And
>>> the DIF bit is set unconditionally, so what is the point of masking it
>>> before?
>>
>> Good point, but then, what's the point of ORRing either ? Just do a
>> plain assignment.
> 
> OR-ring is necessary as this function is called for each DIF output (see idx
> parameter), so plain assignment will clear the previously set bits.

Ah, got it.

Reviewed-by: Marek Vasut <marex@denx.de>

Thanks !