Re: [PATCH v3 17/22] Input: adp5585: Add Analog Devices ADP5585/89 support

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2025-05-20 at 11:33 -0700, Dmitry Torokhov wrote:
> On Tue, May 20, 2025 at 09:32:53AM +0100, Nuno Sá wrote:
> > On Mon, 2025-05-19 at 15:29 -0700, Dmitry Torokhov wrote:
> > > Hi Nuno,
> > > 
> > > On Mon, May 12, 2025 at 01:39:09PM +0100, Nuno Sá via B4 Relay wrote:
> > > > +
> > > > +	for (pin = 0; pin < n_pins; pin++) {
> > > > +		if (keypad_pins[pin] >= adp5585->info->n_pins) {
> > > > +			error = dev_err_probe(dev, -EINVAL,
> > > > +					      "Invalid keypad pin(%u)
> > > > defined\n",
> > > > +					      keypad_pins[pin]);
> > > > +			goto out_free_map;
> > > > +		}
> > > > +
> > > > +		if (test_and_set_bit(keypad_pins[pin], adp5585-
> > > > >pin_usage))
> > > > {
> > > > +			error = dev_err_probe(dev, -EBUSY,
> > > > +					      "Keypad pin(%u) already
> > > > used\n",
> > > > +					      keypad_pins[pin]);
> > > > +			goto out_free_map;
> > > 
> > > This jump looked confusing, together with devm, etc. I wonder, can you
> > > move call to devm_add_action_or_reset() before the loop? It looks like
> > > it should handle completely unpopulated pin map just fine... 
> > 
> > Seemed the logical way but I agree that what you suggest makes it more
> > simpler.
> > 
> > > 
> > > > +		}
> > > > +
> > > > +		__set_bit(keypad_pins[pin], &kpad->keypad);
> > > > +	}
> > > > +
> > > > +	error = devm_add_action_or_reset(dev, adp5585_keys_pins_free,
> > > > kpad);
> > > > +	if (error)
> > > > +		return error;
> > > > +
> > > > +	/*
> > > > +	 * Note that given that we get a mask (and the HW allows it),
> > > > we
> > > > +	 * can have holes in our keypad (eg: row0, row1 and row7
> > > > enabled).
> > > > +	 * However, for the matrix parsing functions we need to pass
> > > > the
> > > > +	 * number of rows/cols as the maximum row/col used plus 1. This
> > > > +	 * pretty much means we will also have holes in our SW keypad.
> > > > +	 */
> > > > +
> > > > +	rows = find_last_bit(&kpad->keypad, kpad->info->max_rows) + 1;
> > > > +	if (rows == kpad->info->max_rows + 1)
> > > > +		return dev_err_probe(dev, -EINVAL,
> > > > +				     "Now rows defined in the
> > > > keypad!\n");
> > > > +
> > > > +	cols = find_last_bit(&kpad->keypad, kpad->info->max_cols +
> > > > kpad-
> > > > > info->max_rows);
> > > > +	if (cols < kpad->info->max_rows)
> > > > +		return dev_err_probe(dev, -EINVAL,
> > > > +				     "No columns defined in the
> > > > keypad!\n");
> > > > +
> > > > +	cols = cols + 1 - kpad->info->max_rows;
> > > > +
> > > > +	error = matrix_keypad_build_keymap(NULL, NULL, rows, cols,
> > > > +					   kpad->keycode, kpad->input);
> > > > +	if (error)
> > > > +		return error;
> > > > +
> > > > +	kpad->row_shift = get_count_order(cols);
> > > > +
> > > > +	if (device_property_read_bool(kpad->dev, "autorepeat"))
> > > > +		__set_bit(EV_REP, kpad->input->evbit);
> > > > +
> > > > +	return adp5585_keys_check_special_events(adp5585, kpad);
> > > 
> > > 	error = adp5585_keys_check_special_events(...);
> > > 	if (error)
> > > 		return error;
> > 
> > Curious, any special reason for the above? Or is just personal preference?
> 
> More of a personal preference, however there is some logic to it ;) - 
> in a function with multiple failure/return points such form allows for
> easy addition and/or movement of the code.
> 
> Thanks.

I get your point. Honestly still prefer returning right away but no strong
feelings anyways. I'll change it.

- Nuno Sá