Re: [PATCH V3] hwmon: add fan/pwm driver for corsair h100i platinum

["Barnabás Pőcze" <pobrn@protonmail.com>]

> [...]
> > > +struct hydro_i_pro_device {
> > > +     struct usb_device *udev;
> > > +
> > > +     const struct device_config *config;
> > > +
> > > +     unsigned char *bulk_out_buffer;
> > > +     char *bulk_in_buffer;
> >
> > Any reason why these two have different sizes? You cast both to
> > 'unsigned char*' in set_fan_target_rpm(), set_fan_pwm_curve(), etc.
>
> I am not sure what you mean by this, I should probably make the type
> consistent between them, is that what you mean by "size"?
>

Oops, yes, I meant type, not size.


> [...]
> > > +struct curve_point {
> > > +     uint8_t temp;
> > > +     uint8_t pwm;
> > > +};
> > > +
> > > +struct hwmon_fan_data {
> > > +     char fan_channel;
> > > +     long fan_target;
> > > +     unsigned char fan_pwm_target;
> >
> > This is very nitpicky, but any reason why is it not 'uint8_t' like above?
> > This applies to other variables as well, I think some variables are too big for what they store,
> > or have a sign, when they could be unsigned.
>
> I moved all the pwm values to u8, and a;ll rpm values to u16(as they
> can't be any bigger)
> I usually don't really write c code, all the different types for the
> same thing confuse me in
> which one to use.
>

Well, I personally try to use unsigned and the smallest integral type
that is acceptable in all the code I write - whenever possible.


> [...]
> > > +static int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
> > > +                           struct hwmon_fan_data *fan_data, long val)
> > > +{
> > > +     int retval;
> > > +     int wrote;
> > > +     int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> > > +     int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> > > +
> > > +     unsigned char *send_buf = hdev->bulk_out_buffer;
> > > +     unsigned char *recv_buf = hdev->bulk_in_buffer;
> > > +
> > > +     fan_data->fan_pwm_target = val;
> > > +     fan_data->fan_target = 0;
> > > +
> > > +     send_buf[0] = PWM_FAN_TARGET_CMD;
> > > +     send_buf[1] = fan_data->fan_channel;
> > > +     send_buf[3] = fan_data->fan_pwm_target;
> > > +
> > > +     retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote,
> > > +                           BULK_TIMEOUT);
> > > +     if (retval)
> > > +             return retval;
> > > +
> > > +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
> > > +                           BULK_TIMEOUT);
> > > +     if (retval)
> > > +             return retval;
> > > +
> > > +     if (!check_succes(send_buf[0], recv_buf)) {
> > > +             dev_info(&hdev->udev->dev,
> > > +                      "[*] failed setting fan pwm %d,%d,%d/%d\n",
> > > +                      recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> > > +             return -EINVAL;
> > > +     }
> > > +     return 0;
> > > +}
> > > +
> >
> > The previous four functions are structurally more or less the same,
> > I think the USB related parts could be placed into their own dedicated function.
>
> I could, but the only part that could actually be split up is the usb_bulk_msg.
> If I would remove the error code that part could also be split up.
> Are there any conventions around what to log/not to log? Maybe it is best to
> remove those log messages anyways.
>

What I had in mind was something like this:


// fill send_buf

retval = usb_transaction(...);

if (retval)
	// dev_err(...), etc.

// process recv_buf if necessary


And that function would include the two calls to usb_bulk_msg(), and the
call to check_succes(). You could even write this function in such a way
that all locking is done only in that function minimizing the possibility
of locking related issues. But really, it's up to you.


> [...]
> > > +static void astk_disconnect(struct usb_interface *interface)
> > > +{
> > > +     struct hydro_i_pro_device *hdev = usb_get_intfdata(interface);
> > > +
> > > +     dev_info(&hdev->udev->dev, "[*] DEINIT DEVICE\n");
> >
> > In my opinion the style of the log messages is not consistent,
> > sometimes you use all-caps, sometimes it's all lowercase, sometimes
> > it has punctuation.
>
> Again I couldn't find anything on logging style within the kernel, I am leaning
> towards just removing them all together. If you can find any logging style
> guide link me to it, if not I will just remove all the logs,
>

What I meant is that even in this single module, the style is not consistent.
I don't suggest you get rid of them, just that you use a consistent style.

Another thing, if you want to log a failure, then it should be
dev_err() or dev_warn() depending on the severity - in my opinion.

https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages
talks about kernel messages.


> [...]


Barnabás Pőcze