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

[Marek Vasut <marex@denx.de>]

On 1/4/23 11:32, Alexander Stein wrote:
> Hi Marek,

Hi,

> Am Dienstag, 3. Januar 2023, 15:31:21 CET schrieb Marek Vasut:
>> On 1/3/23 13:31, Alexander Stein wrote:
>>> The calculation DIFx is BIT(n) +1 is only true for 9FGV0241. With
>>> additional devices this is getting more complicated.
>>> Support a base bit for the DIF calculation, currently only devices
>>> with consecutive bits are supported, e.g. the 6-channel device needs
>>> additional logic.
>>>
>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>> ---
>>>
>>>    drivers/clk/clk-renesas-pcie.c | 29 ++++++++++++++++-------------
>>>    1 file changed, 16 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-renesas-pcie.c
>>> b/drivers/clk/clk-renesas-pcie.c index 0076ed8f11b0..d19b8e759eea 100644
>>> --- a/drivers/clk/clk-renesas-pcie.c
>>> +++ b/drivers/clk/clk-renesas-pcie.c
>>> @@ -18,7 +18,6 @@
>>>
>>>    #include <linux/regmap.h>
>>>    
>>>    #define RS9_REG_OE				0x0
>>>
>>> -#define RS9_REG_OE_DIF_OE(n)			BIT((n) + 1)
>>>
>>>    #define RS9_REG_SS				0x1
>>>    #define RS9_REG_SS_AMP_0V6			0x0
>>>    #define RS9_REG_SS_AMP_0V7			0x1
>>>
>>> @@ -31,9 +30,6 @@
>>>
>>>    #define RS9_REG_SS_SSC_MASK			(3 << 3)
>>>    #define RS9_REG_SS_SSC_LOCK			BIT(5)
>>>    #define RS9_REG_SR				0x2
>>>
>>> -#define RS9_REG_SR_2V0_DIF(n)			0
>>> -#define RS9_REG_SR_3V0_DIF(n)			BIT((n) + 1)
>>> -#define RS9_REG_SR_DIF_MASK(n)		BIT((n) + 1)
>>>
>>>    #define RS9_REG_REF				0x3
>>>    #define RS9_REG_REF_OE				BIT(4)
>>>    #define RS9_REG_REF_OD				BIT(5)
>>>
>>> @@ -62,6 +58,7 @@ struct rs9_chip_info {
>>>
>>>    	const enum rs9_model	model;
>>>    	unsigned int		num_clks;
>>>    	u8			did;
>>>
>>> +	u8			(*calc_dif)(int idx);
>>>
>>>    };
>>>    
>>>    struct rs9_driver_data {
>>>
>>> @@ -160,8 +157,14 @@ static const struct regmap_config rs9_regmap_config =
>>> {>
>>>    	.reg_read = rs9_regmap_i2c_read,
>>>    
>>>    };
>>>
>>> +static u8 rs9fgv0241_calc_dif(int idx)
>>> +{
>>> +	return BIT(idx) + 1;
>>
>> Can't we just do
>>
>> if (model == ...)
>>    return BIT(idx) + 1
>> else if (model == ...)
>>    return BIT(idx);
>> ...
> 
> I was tempted going this way. But I opted for a callback due to the fact that
> this driver might support 9FGV/9DBV/9DMV/9FGL/9DML/9QXL/9SQ as well(your
> comment in the header).
> Even just considering 9FVG, 9FGV0641 has an even more complex DIF offset
> calculation.
> The mapping is
> * DIF OE0 - Bit 0
> * DIF OE1 - Bit 2
> * DIF OE2 - Bit 3
> * DIF OE3 - Bit 4
> * DIF OE4 - Bit 6
> * DIF OE5 - Bit 7
> 
> So the calucation might not fit into one line, so the readability benefit is
> gone.

You can still do

if (model == ...)
  return function1();
else if (model == ...)
  return function2();

which would work without any indirection via callback.