Re: [PATCH 1/4] hwmon: (oxp-sensors) Separate logic from device-specific data

[Tobias Jakobi]

On 12/26/24 22:04, Guenter Roeck wrote:
> On Thu, Dec 26, 2024 at 06:00:16PM +0100, tjakobi@xxxxxxxxxxxxxxxxxxxxx wrote:
> > From: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
> >
> > We currently have large switch-statements in all functions that
> > write to EC registers, even though the bulk of the supported
> > devices functions more or less the same.
> >
> > Factor the device-specific data out into a struct oxp_config. This
> > only leaves logic in the corresponding functions and should make
> > adding future devices much easier and less error-prone.
> >
> > Also introduce struct oxp_data which is going to be used in a
> > later commit to cache device state.
> >
> > Signed-off-by: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/hwmon/oxp-sensors.c | 517 +++++++++++++++---------------------
> >   1 file changed, 215 insertions(+), 302 deletions(-)
> >
> >   
> ...
> > -static int oxp_pwm_disable(void)
> > +static int oxp_pwm_disable(const struct oxp_config *config)
> >   {
> > -	switch (board) {
> > -	case orange_pi_neo:
> > -		return write_to_ec(ORANGEPI_SENSOR_PWM_ENABLE_REG, PWM_MODE_AUTO);
> > -	case aok_zoe_a1:
> > -	case aya_neo_2:
> > -	case aya_neo_air:
> > -	case aya_neo_air_1s:
> > -	case aya_neo_air_plus_mendo:
> > -	case aya_neo_air_pro:
> > -	case aya_neo_flip:
> > -	case aya_neo_geek:
> > -	case aya_neo_kun:
> > -	case oxp_2:
> > -	case oxp_fly:
> > -	case oxp_mini_amd:
> > -	case oxp_mini_amd_a07:
> > -	case oxp_mini_amd_pro:
> > -	case oxp_x1:
> > -		return write_to_ec(OXP_SENSOR_PWM_ENABLE_REG, PWM_MODE_AUTO);
> > -	default:
> > -		return -EINVAL;
> > -	}
> > +	if (test_bit(OXP_FEATURE_PWM, &config->features))
> > +		return write_to_ec(config->sensor_pwm_enable_reg, PWM_MODE_AUTO);
> > +
>
> This and all the other feature checks are completely wrong.
> Those checks whould happen once in the is_visible functions,
> and there should not be any such runtime checks. If a feature
> is not available, the associated attributes should not be created
> in the first place.
Hmm, so if the feature checks are wrong _now_, then the board check was 
also wrong to begin with. This is my first time working on hwmon code. 
If we can just drop the checks here (and elsewhere) and just have them 
in the is_visible function -- I'm all for it. Less code is better code.


> If such checks happen in the current code, that should be fixed
> in the is_visible functions. Any existing runtime feature checks
> should be removed.
OK, so to reiterate: We don't want any feature checks during runtime. 
Only during probe time. And during probe we just create the attributes 
that make sense for the device. What we can't decide on the level of 
attributes, we do in the is_visible functions. Is this correct?

With best wishes,
Tobias


> Guenter