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