>Hi,
>
>On Wed, Jun 05, 2019 at 04:36:28PM +0000, Rojewski, Cezary wrote:
>> >+static s8 sun4i_i2s_get_sr(const struct sun4i_i2s *i2s, int width)
>> >+{
>> >+ if (width < 16 || width > 24)
>> >+ return -EINVAL;
>> >+
>> >+ if (width % 4)
>> >+ return -EINVAL;
>> >+
>> >+ return (width - 16) / 4;
>> >+}
>> >+
>> >+static s8 sun4i_i2s_get_wss(const struct sun4i_i2s *i2s, int width)
>> >+{
>> >+ if (width < 16 || width > 32)
>> >+ return -EINVAL;
>> >+
>> >+ if (width % 4)
>> >+ return -EINVAL;
>> >+
>> >+ return (width - 16) / 4;
>> >+}
>> >+
>> >+static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width)
>> >+{
>> >+ if (width % 4)
>> >+ return -EINVAL;
>> >+
>>
>> In the two above you start with boundary check before mod yet in
>> this one the order is reversed. Keeping the same order should prove
>> more cohesive.
>
>Indeed, I'll fix this.
>
>>
>> >+ if (width < 8 || width > 32)
>> >+ return -EINVAL;
>> >+
>> >+ return (width - 8) / 4 + 1;
>> >+}
>> >+
>>
>> Other, probably less welcome suggestion is introduction of unified
>> function which ones listed here would simply invoke. All of these
>> "computations" differ in fact only in: min and max boundary. The +1
>> for _sr_wss is negligible, you can append it on return.
>
>It's not just about the min and max boundaries. It's also the offset
>at which to start with (16 vs 8), and the offset to apply to the
>result (0 vs 1).
>
>That's 4 parameters out of 5 that are different. For something that
>trivial, I don't think it's worth it to put it in common.
>
>Maxime
This is what was going through my mind:
static inline s8 my_unified(int width, u8 min, u8 max)
{
if (width < min || width > max)
return -EINVAL;
if (width % 4)
return -EINVAL;
return (width - min) / 4;
}
static s8 sun4i_i2s_get_sr(const struct sun4i_i2s *i2s, int width)
{
return my_unified(width, 16, 24);
}
static s8 sun4i_i2s_get_wss(const struct sun4i_i2s *i2s, int width)
{
return my_unified(width, 16, 32);
}
static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width)
{
return my_unified(width, 8, 32) + 1;
}
However, if indeed 'start' offset is variable and may differ from min boundary, then my approach would fail.
Otherwise, treat it as suggestion, personally I find it easier to update only the unified function (development phase), especially if you're planning for adding more of these (the min/ max variants) in the future.
One more thing, the i2s ptr is unused - consider flagging it or simply removing from declaration?
Czarek
