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
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com