Re: [RFC] hwmon: (nct6775) Add NCT6799 support through ACPI layer

[Ahmad Khalifa <ahmad@khalifa.ws>]

On 19/10/2022 22:04, Denis Pauk wrote:
> Hi Ahmad,
> 
> Thank you for your patch.
> 
> I will add mention of you patch in
> https://bugzilla.kernel.org/show_bug.cgi?id=204807 also.

That's an interesting bug. It has loads of ACPI tables in there, which 
could be very useful.

The acpi patch is still a proof of concept and will show wrong values, I 
know the voltages and temperatures are mixed up or could even be pulling 
rubbish data that looks like a temperature.

I just wanted comments on where to go, thanks for the below.
There is definitely lots to fix up first.


> I have added my comments below.
>> +static void superio_acpi_select(struct nct6775_sio_data *sio_data,
>> int ld) +{
>> +	sio_data->ld = ld;
>> +}
>> +
> Could be reused superio_wmi_select here with some more general name?
> e.g rename superio_wmi_select -> superio_asus_select, or some other
> name.
>> +static int superio_acpi_enter(struct nct6775_sio_data *sio_data)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void superio_acpi_exit(struct nct6775_sio_data *sio_data)
>> +{
>> +}
>> +
> Could be reused superio_wmi_exit here with some more general name?

Yes, make them common for both.
Frankly, I replicated them mechanically so the patch is quicker to read 
without lots of +/- lines


>> +		case nct6799:
> Looks as same as for the previous one (nct6798). Have i missed some
> reg difference?

They're a replica, probably should've reused the above case.




-- 
Regards,
Ahmad