Thanks Guennadi for looking at this code, it's hard to review and figure things out... I replied to each, even trivial ones, to have a trace of all the issues.
+static void sdw_compute_slave_ports(struct sdw_master_runtime *m_rt,
struct sdw_transport_data *t_data)+{
- struct sdw_slave_runtime *s_rt = NULL;
Superfluous initialisation.
ok
- struct sdw_port_runtime *p_rt;
- int port_bo, sample_int;
- unsigned int rate, bps, ch = 0;
ditto for ch
ok
- port_bo = t_data->block_offset;
- list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
rate = m_rt->stream->params.rate;bps = m_rt->stream->params.bps;sample_int = (m_rt->bus->params.curr_dr_freq / rate);list_for_each_entry(p_rt, &s_rt->port_list, port_node) {ch = sdw_ch_mask_to_ch(p_rt->ch_mask);sdw_fill_xport_params(&p_rt->transport_params,p_rt->num, true,SDW_BLK_GRP_CNT_1,sample_int, port_bo, port_bo >> 8,t_data->hstart,t_data->hstop,I think the above two lines could fit in one
likely yes.
(SDW_BLK_GRP_CNT_1 * ch), 0x0);superfluous parentheses
yep
+static void sdw_compute_master_ports(struct sdw_master_runtime *m_rt,
struct sdw_group_params *params,int port_bo, int hstop)+{
- struct sdw_transport_data t_data = {0};
- struct sdw_port_runtime *p_rt;
- struct sdw_bus *bus = m_rt->bus;
- int sample_int, hstart = 0;
superfluous initialisation
yes
- unsigned int rate, bps, ch, no_ch;
- rate = m_rt->stream->params.rate;
- bps = m_rt->stream->params.bps;
- ch = m_rt->ch_count;
- sample_int = (bus->params.curr_dr_freq / rate);
superfluous parentheses
yes
- if (rate != params->rate)
return;- t_data.hstop = hstop;
- hstart = hstop - params->hwidth + 1;
- t_data.hstart = hstart;
- list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
no_ch = sdw_ch_mask_to_ch(p_rt->ch_mask);sdw_fill_xport_params(&p_rt->transport_params, p_rt->num,true, SDW_BLK_GRP_CNT_1, sample_int,port_bo, port_bo >> 8, hstart, hstop,(SDW_BLK_GRP_CNT_1 * no_ch), 0x0);superfluous parentheses
yes
sdw_fill_port_params(&p_rt->port_params,p_rt->num, bps,SDW_PORT_FLOW_MODE_ISOCH,SDW_PORT_DATA_MODE_NORMAL);/* Check for first entry */if (!(p_rt == list_first_entry(&m_rt->port_list,struct sdw_port_runtime,port_node))) {you wanted to write "if (p_rt != ...)"
bad code indeed, thanks for spotting this. I need to double-check this one, now I don't trust the == it could also be that it was meant to be a NULL check on the first entry.
port_bo += bps * ch;continue;}t_data.hstart = hstart;t_data.hstop = hstop;You already set these two above
need to check this as well.
t_data.block_offset = port_bo;t_data.sub_block_offset = 0;port_bo += bps * ch;- }
- sdw_compute_slave_ports(m_rt, &t_data);
+}
+static void _sdw_compute_port_params(struct sdw_bus *bus,
struct sdw_group_params *params, int count)+{
- struct sdw_master_runtime *m_rt = NULL;
superfluous initialisation
yes
- int hstop = bus->params.col - 1;
- int block_offset, port_bo, i;
- /* Run loop for all groups to compute transport parameters */
- for (i = 0; i < count; i++) {
port_bo = 1;block_offset = 1;list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {sdw_compute_master_ports(m_rt, ¶ms[i],port_bo, hstop);block_offset += m_rt->ch_count *m_rt->stream->params.bps;port_bo = block_offset;}hstop = hstop - params[i].hwidth;hstop -= ...
yes
- }
+}
+static int sdw_compute_group_params(struct sdw_bus *bus,
struct sdw_group_params *params,int *rates, int count)+{
- struct sdw_master_runtime *m_rt = NULL;
ditto
yes
- int sel_col = bus->params.col;
- unsigned int rate, bps, ch;
- int i, column_needed = 0;
- /* Calculate bandwidth per group */
- for (i = 0; i < count; i++) {
params[i].rate = rates[i];params[i].full_bw = bus->params.curr_dr_freq / params[i].rate;- }
- list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
rate = m_rt->stream->params.rate;bps = m_rt->stream->params.bps;ch = m_rt->ch_count;for (i = 0; i < count; i++) {if (rate == params[i].rate)params[i].payload_bw += bps * ch;I don't know about the algorithm, rates can repeat, right? So you cannot break out of the loop here once you found one match?
This code looks wrong. Need to get an intravenous caffeine injection.
}- }
- for (i = 0; i < count; i++) {
params[i].hwidth = (sel_col *params[i].payload_bw + params[i].full_bw - 1) /params[i].full_bw;column_needed += params[i].hwidth;- }
- if (column_needed > sel_col - 1)
return -EINVAL;- return 0;
+}
+static int sdw_add_element_group_count(struct sdw_group *group,
unsigned int rate)+{
- int num = group->count;
- int i;
- for (i = 0; i <= num; i++) {
if (rate == group->rates[i])Are you sure this is correct? You actually check count + 1 rates - from 0 to count inclusively. I think this isn't what you wanted to do, so my proposal below only checks count rates.
I'll have to double check. There's already the warning on krealloc that looks suspicious as well.
break;if (i != num)continue;if (group->count >= group->max_size) {group->max_size += 1;group->rates = krealloc(group->rates,(sizeof(int) * group->max_size),GFP_KERNEL);if (!group->rates)return -ENOMEM;}group->rates[group->count++] = rate;- }
How about this:
for (i = 0; i < num; i++) if (rate == group->rates[i]) return 0;
if (group->count >= group->max_size) { group->max_size += 1; group->rates = krealloc(group->rates, (sizeof(int) * group->max_size), GFP_KERNEL); if (!group->rates) return -ENOMEM; }
group->rates[group->count++] = rate;
return 0;
will check offline and see.
- return 0;
+}
+static int sdw_get_group_count(struct sdw_bus *bus,
struct sdw_group *group)+{
- struct sdw_master_runtime *m_rt;
- unsigned int rate;
- int ret = 0;
- group->count = 0;
- group->max_size = SDW_STRM_RATE_GROUPING;
- group->rates = kcalloc(group->max_size, sizeof(int), GFP_KERNEL);
- if (!group->rates)
return -ENOMEM;- list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
rate = m_rt->stream->params.rate;if (m_rt == list_first_entry(&bus->m_rt_list,struct sdw_master_runtime,bus_node)) {group->rates[group->count++] = rate;} else {ret = sdw_add_element_group_count(group, rate);if (ret < 0)Actually it looks like you should free rates here? I see that not doing this makes the caller function below easier, but I'd say this is more error- prone... Up to you but I'd go the "safe" way - if it fails, it frees itself, if it succeeds - it's freed elsewhere.
good catch, will look into this.
return ret;}- }
- return ret;
I think this will always return 0 here, so you don't need the "ret" variable in the function scope, you only need it in the "else" scope above.
will check. In general I avoid per-scope declarations.
+}
+/**
- sdw_compute_port_params: Compute transport and port parameters
- @bus: SDW Bus instance
- */
+static int sdw_compute_port_params(struct sdw_bus *bus) +{
- struct sdw_group_params *params = NULL;
- struct sdw_group group;
- int ret;
- ret = sdw_get_group_count(bus, &group);
- if (ret < 0)
goto out;- if (group.count == 0)
goto out;- params = kcalloc(group.count, sizeof(*params), GFP_KERNEL);
- if (!params) {
ret = -ENOMEM;goto out;- }
- /* Compute transport parameters for grouped streams */
- ret = sdw_compute_group_params(bus, params,
&group.rates[0], group.count);- if (ret < 0)
goto out;- _sdw_compute_port_params(bus, params, group.count);
+out:
- kfree(params);
- kfree(group.rates);
Depending whether or not you change the code above, this might change too.
will check
+static int sdw_compute_bus_params(struct sdw_bus *bus) +{
- unsigned int max_dr_freq, curr_dr_freq = 0;
- struct sdw_master_prop *mstr_prop = NULL;
superfluous initialisation
yes
- int i, clk_values, ret;
- bool is_gear = false;
- u32 *clk_buf;
- mstr_prop = &bus->prop;
- if (!mstr_prop)
this is impossible, it's an address of bus->prop...
Gah, yes.
return -EINVAL;- if (mstr_prop->num_clk_gears) {
clk_values = mstr_prop->num_clk_gears;clk_buf = mstr_prop->clk_gears;is_gear = true;- } else if (mstr_prop->num_clk_freq) {
clk_values = mstr_prop->num_clk_freq;clk_buf = mstr_prop->clk_freq;- } else {
clk_values = 1;clk_buf = NULL;- }
- max_dr_freq = mstr_prop->max_clk_freq * SDW_DOUBLE_RATE_FACTOR;
- for (i = 0; i < clk_values; i++) {
if (!clk_buf)curr_dr_freq = max_dr_freq;elsecurr_dr_freq = (is_gear) ?superfluous parentheses
(max_dr_freq >> clk_buf[i]) :ditto
yes and yes
clk_buf[i] * SDW_DOUBLE_RATE_FACTOR;if (curr_dr_freq <= bus->params.bandwidth)continue;break;I think this is raw code, you'd actually want to write this as
if (curr_dr_freq > bus->params.bandwidth) break;
I saw this before my Summer break and forgot about it. yes it needs to be fixed.
bus->debugfs = sdw_bus_debugfs_init(bus);
- if (!bus->compute_params)
bus->compute_params = &sdw_compute_params;I think it is more usual to not use "&" with functions, but it's valid too
yes, will fix
+/* Retrieve and return channel count from channel mask */ +static inline int sdw_ch_mask_to_ch(int ch_mask) +{
- int c = 0;
superfluous initialisation
yes
- for (c = 0; ch_mask; ch_mask >>= 1)
c += ch_mask & 1;isn't there a built-in or something to count set bits... You could use ffs() to at least not loop 31 times for 0x80000000
will check