sorry forgot to reply to the question about the buffer sizes.
The reason they are different sizes (in theory) is because
I get the size for each buffer from usb_endpoint_maxp.
I do not know whether or not these are the same. If you have
any experience with these functions and know they are always
the same I am willing to make them the same size.
On Sat, 15 Aug 2020 at 22:31, jaap aarts <jaap.aarts1@xxxxxxxxx> wrote:
>
> > [...]
> > > > > +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
