Re: [PATCH 1/5] regulator: irq_helpers: Allow omitting map_event for simple IRQs

[Vaittinen, Matti]

Hi Mark,

Thanks for the review :)

On 11/18/21 15:35, Mark Brown wrote:
> On Thu, Nov 18, 2021 at 01:48:26PM +0200, Matti Vaittinen wrote:
> 
>> +static bool single_bit_set(int val, int bits_to_check)
>> +{
>> +	int bit;
>> +	const unsigned long bits = val;
>> +
>> +	bit = find_first_bit(&bits, bits_to_check);
>> +	if (bit == bits_to_check)
>> +		return false;
>> +
>> +	bit = find_next_bit(&bits, bits_to_check, bit + 1);
>> +
>> +	return (bit == bits_to_check);
>> +}
> 
> The namespacing here feels like it should be with the other _bit()
> helpers rather than private to the regulator code, I can certainly see
> other things wanting to use it.

Hmm. Right, thanks. I can put that in the same place where other bit() 
operations are - let's see what the reception is :)

> 
>> +	if (!h->desc.map_event) {
>> +		if (rdev_amount != 1 ||
>> +		    !single_bit_set(common_errs, sizeof(common_errs) * 8) ||
>> +		    per_rdev_errs)
>> +			return ERR_PTR(-EINVAL);
>> +
>> +		h->desc.map_event = map_event_simple;
>> +	}
> 
> This isn't the usual pattern, normally we would have the driver assign
> the helper operation.  We can always still do the check based on finding
> the expected map_event set up.

So, do you suggest that we export the map_event_simple() as a helper 
which drivers can provide to irq_helpers? If yes, do you think we should 
leave out the sanity check regarding the conditions (only one common 
error and only one rdev)? Or should we compare the given map function to 
the adress of the map_event_simple() and perform checks if it matches? 
It looks a bit strange to me. Or did you have some other vision?

Best Regards
     Matti Vaittinen


-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~