Re: [RFC PATCH v1 1/1] platform/x86: panasonic-laptop: add platform_profile support

[Alex Yeo]

Thank you very much for taking the time to do a review. I have sent a v2 
to address the comments.

v2: 
https://lore.kernel.org/platform-driver-x86/20261006085021.853827-1-alexyeo362@xxxxxxxxx

> >   
> > +static struct pcc_quirk quirk_cf_sr4 = {
> > +	.use_platform_profiles = true,
> > +	.platform_profiles = {
> > +		[PLATFORM_PROFILE_QUIET] =  {
>
> There's extra space in all these.

This has been fixed.

> > +static int pcc_fan_mode_get(struct pcc_acpi *pcc, enum pcc_fan_mode *fan_mode)
> > +{
> > +	unsigned long long state;
> > +	acpi_status status;
> > +
> > +	status = acpi_evaluate_integer(pcc->ec_handle, "CEFM", NULL,
> > +				       &state);
>
> Fits to one line.

This has been applied.

> > +static int pcc_fan_mode_set(struct pcc_acpi *pcc, enum pcc_fan_mode fan_mode)
> > +{
> > +	acpi_status status;
> > +
> > +	switch (fan_mode) {
> > +	case PCC_FAN_MODE_ACTIVE:
> > +		status = acpi_execute_simple_method(pcc->ec_handle,
> > +						    "SEFM",
> > +						    PCC_ACPI_FAN_ACTIVE_MODE);
> > +		break;
> > +	case PCC_FAN_MODE_PASSIVE:
> > +		status = acpi_execute_simple_method(pcc->ec_handle,
> > +						    "SEFM",
> > +						    PCC_ACPI_FAN_PASSIVE_MODE);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ACPI_FAILURE(status)) {
> > +		pr_err("failed to set fan mode via SEFM\n");
> > +		return -EIO;
> > +	}
>
> IMO, spreading stuff around like this just makes it harder to follow code
> flow. And the variation is just to pass a different argument to
> acpi_execute_simple_method() which would suggest a helper would be useful.
>
> Once everything is done within the case, you can directly return from
> those cases for simplicity.

This has been fixed by adding a helper function + refactor code to be 
like the code mentioned below

> > +	if (ACPI_FAILURE(status)) {
> > +		pr_err("failed to set power mode via SEPL\n");
> > +		return -EIO;
> > +	}
>
> Same problem here as above.

Code changes applied to this section too.

> > +static int pcc_platform_profile_get(struct device *dev, enum platform_profile_option *profile)
> > +{
> > +	struct pcc_acpi *pcc = dev_get_drvdata(dev);
> > +	enum pcc_fan_mode fan_mode;
> > +	enum pcc_tdp_mode tdp_mode;
> > +	int status;
> > +
> > +	status = pcc_fan_mode_get(pcc, &fan_mode);
> > +	if (status)
> > +		return status;
> > +
> > +	status = pcc_tdp_mode_get(pcc, &tdp_mode);
> > +	if (status)
> > +		return status;
>
> Please leave "status" for acpi_status and pick another name for the
> generic return variable (I personally prefer "ret" because it doesn't
> carry error connotation "err" does, but the latter seems to be already
> in use by this driver, among other variable names).

This has been applied to the whole patch.

> > +	for (enum platform_profile_option pp_opt = 0;
>
> Move declaration to the beginning of the function.

This was done.

> > +	     pp_opt < PLATFORM_PROFILE_LAST;
>
> Please use ARRAY_SIZE() + make sure you add the include for it.

This was done.

> > +	     pp_opt++) {
> > +		enum pcc_fan_mode profile_fan_mode =
> > +			pcc->quirks->platform_profiles[pp_opt].fan_mode;
> > +		enum pcc_tdp_mode profile_tdp_mode =
> > +			pcc->quirks->platform_profiles[pp_opt].tdp_mode;
>
> Please make a local variable out of pcc->quirks->platform_profiles[pp_opt]
> instead (with a reasonably short name).

This has been done with additional refactoring to make this into a 
pointer (as pointed out below).

> > +
> > +		if (!(profile_fan_mode && profile_tdp_mode))
>
> This code doesn't make sense for variables that are declared as enums
> (do not handle enums as truth values).

This has been addressed by making explicit comparisons as opposed to 
treating enums as truth values.

> > +static int pcc_platform_profile_set_profile(struct pcc_acpi *pcc,
> > +					    enum pcc_fan_mode fan_mode,
> > +					    enum pcc_tdp_mode tdp_mode)
> > +{
> > +	int status;
>
> Change name.

Changed

> > +
> > +	switch (tdp_mode) {
> > +	case PCC_TDP_MODE_UNLOCKED:
> > +		status = pcc_fan_mode_set(pcc, fan_mode);
> > +		if (status)
> > +			return status;
> > +
> > +		return pcc_tdp_mode_set(pcc, tdp_mode);
> > +	case PCC_TDP_MODE_LOCKED:
> > +		status = pcc_tdp_mode_set(pcc, tdp_mode);
> > +		if (status)
> > +			return status;
> > +
> > +		return pcc_fan_mode_set(pcc, fan_mode);
>
> This is structurally much easier to follow than pcc_fan_mode_set() above.

This has been applied to the code above.

> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int pcc_platform_profile_set(struct device *dev, enum platform_profile_option profile)
> > +{
> > +	struct pcc_acpi *pcc = dev_get_drvdata(dev);
> > +	struct pcc_platform_profile pcc_profile;
>
> Why isn't this a pointer?

This pattern and ones like it have been converted to be a pointer.

> > +	pcc_profile = pcc->quirks->platform_profiles[profile];
>
> I'd put the assignment to the declaration line (it'll be only 91 chars
> long and is quite boilerplately so fits well into the variable
> declarations block, IMO)

This has been done.

> > +	if (pcc_profile.fan_mode && pcc_profile.tdp_mode)
>
> Again, those are enums but you treat them as truth values which makes
> things harder to understand.

This part was removed as this check was originally put in place to check 
for malformed quirks (fan and TDP both need to be set). This is 
addressed via a WARN_ON below.

> > +		return pcc_platform_profile_set_profile(pcc,
> > +							pcc_profile.fan_mode,
> > +							pcc_profile.tdp_mode);
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int pcc_platform_profile_probe(void *drvdata, unsigned long *choices)
> > +{
> > +	struct pcc_acpi *pcc = drvdata;
> > +
> > +	for (enum platform_profile_option pp_opt = 0;
> > +	     pp_opt < PLATFORM_PROFILE_LAST;
> > +	     pp_opt++) {
>
> Declare the enum in the function variables and put this to single line.

This was done.

> > +		enum pcc_fan_mode fan_mode =
> > +			pcc->quirks->platform_profiles[pp_opt].fan_mode;
> > +		enum pcc_tdp_mode tdp_mode =
> > +			pcc->quirks->platform_profiles[pp_opt].tdp_mode;
> > +
> > +		if (fan_mode && tdp_mode) {
>
> Same comments as with the other code.

This has been converted to a pointer + do not treat enums as truth values

> > +			set_bit(pp_opt, choices);
> > +		} else if (fan_mode || tdp_mode) {
> > +			pr_err("error probing platform profiles: malformed quirk\n");
>
> This looks a clear developer error so WARN_ON() would be more appropriate
> than pr_err().

This has been done.

> > +
> >   	return 0;
> >   
> >   out_platform:
>
> Also, this looked entirely independent of the existing code (?) so it
> looks as if it should be make a separate platform_driver instead of trying
> to klugde it into the existing probe. If there aren't cross references
> besides the sharing of the private data structure, I'd just introduce it
> as a separate struct platform_driver with a proper ID table and own probe,
> etc.

After considering this, I agree and a separate platform_driver makes a 
lot more sense for something like this.