Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

From: Lukas Prediger
Date: Thu Sep 09 2021 - 14:05:09 EST


Hey Phil,

thanks for taking the time to look at this and give feedback.

> Dear Lukas,
>
> Thank you for the patch, much appreciated and looks great. One very
> minor thing though has jumped out at me after running checkpatch though:
>
> > --- a/include/uapi/linux/cdrom.h
> > +++ b/include/uapi/linux/cdrom.h
> > @@ -147,6 +147,8 @@
> > #define CDROM_NEXT_WRITABLE 0x5394 /* get next writable block */
> > #define CDROM_LAST_WRITTEN 0x5395 /* get last block written on disc */
> >
> > +#define CDROM_TIMED_MEDIA_CHANGE 0x5396 /* get the timestamp of the last media change */
> > +
> > /*******************************************************
> > * CDROM IOCTL structures
> > *******************************************************/
> > @@ -295,6 +297,19 @@ struct cdrom_generic_command
> > };
> > };
> >
> > +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
> > +struct cdrom_timed_media_change_info
> > +{
>
> This opening brace should be on the same line as the struct definition
> as per current style rules.

I also noted that checkpatch reported this and that it is technically a style breach,
however I kept the brace in the newline to be consistent with all the other
cdrom ioctl structs defined above this in the same file for consistency.
I have no strong feelings about this, though, so we could change this.

> I also got a checkpatch warning about ENOSYS being used as an error
> value, but I can see this usage is standard in the driver and not a
> problem so no issue with that.
>
> I will review and test properly after work tomorrow (being new to the
> role I'd like to make sure I'm taking the proper time), but I have no
> doubt it will work fine. Assuming it does I will be happy to accept the
> patch with the above brace change. Thanks again.
> Regards,
> Phil

Best regards,
Lukas