> [...]
> > > > +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.
I think I kinda got the hang of it now. There are so many options like
and son have very weird names like uintN_t. The naming of char
also confuses me, it feels like I am using strings as byte arrays.
As long as I keep using uN from now on it will be fine I think.
[...]
> > > > +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.
I ended up putting everything strictly USB into this function, and everything
driver protocol related outside of it, so `check_succes()` is put outside
because there is nothing inherently USB about it.
I also dont want to put the locking in the USB function, because some
"transactions" require multiple usb messages. These are not implemented
yet but from what I remember most RGB light effects need this.
I don't know if I will ever implement this since the LED subsystem is very
primitive for this, but I want to keep the door open.
> > [...]
> > > > +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.
yeah I read that already, doesn't really talk much about what should
be logged/should not.
I ended up just making them more consistent, after noticing other
drivers used %s I figured
I could do that too, and made the messages a bit nicer too.
> [...]
I got everything compiling, I have to restart, then I will test and
send in a new patch
Kind regards,
Jaap Aarts
