Re: [PATCH] rename s1810c* to presonus_studio to get s1824c working

[Takashi Iwai <tiwai@xxxxxxx>]

On Thu, 27 Feb 2025 10:53:40 +0100,
Amin Dandache wrote:
> 
> Hi,
> 
> 
> Yes, the previous file was named after the exact model 1810c.
> I would not like to put the 1824c into the file as it suggest its just
> one model.
> It feels like monkey patching for me.
> 
> Also it's the same series: Presonus Studio.

Heh, and who knows that the name will keep in the next model? ;)

Don't get me wrong: if there will be more handful new models with
completely different names in future, we should think of renaming,
indeed.  Or, if the code varies significantly depending on the model,
or if we need some fundamental changes, we can take a drastic change,
sure.

But, a case like yours would need only a small bit of actual code
change, while the whole renaming makes really hard to see what the
actual change is.

So, don't mix up two things.  If a rename is mandatory, do only
rename but nothing else.  Then apply another fix (a new device ID
support) afterwards -- or vice versa.


thanks,

Takashi

> And yes it's just a new device ID, but there are some features missing
> like SPDIF which i could not test yet, also if every function is
> adopted by the 1810c.
> 
> I just tested the basic functionality and it works perfect for analog
> signals.
> 
> In the future there could be some differences in the handling for the
> models, so i tried to prepare that and rename it to a global name that
> supports more devices inside and more cases for the devices.
> like the mixer_scarlett.c which supports all focusrite scarlett
> devices in one file.
> 
> I will refactor the proper subject and try put the signed-off-by tag,
> as this is my first commit i have to learn a little bit new.
> 
> 
> Best Regards,
> Amin
> 
> On 27.02.25 10:29, Takashi Iwai wrote:
> > On Thu, 27 Feb 2025 07:43:08 +0100,
> > Amin Dandache wrote:
> >> ---
> >>   sound/usb/Makefile                            |   2 +-
> >>   sound/usb/format.c                            |  12 +-
> >>   ...mixer_s1810c.c => mixer_presonus_studio.c} | 247 +++++++++---------
> >>   sound/usb/mixer_presonus_studio.h             |   8 +
> >>   sound/usb/mixer_quirks.c                      |  12 +-
> >>   sound/usb/mixer_s1810c.h                      |   7 -
> >>   sound/usb/quirks.c                            |   9 +-
> >>   7 files changed, 161 insertions(+), 136 deletions(-)
> >>   rename sound/usb/{mixer_s1810c.c => mixer_presonus_studio.c} (59%)
> >>   create mode 100644 sound/usb/mixer_presonus_studio.h
> >>   delete mode 100644 sound/usb/mixer_s1810c.h
> > First off, could you give the reason why you must rename the whole
> > stuff?  Is it just to add the support of a new device ID 194f:010d?
> > If so, we don't have to rename everything, but just keep using the
> > same function as is.  You can put simply a comment that the function
> > supports both devices, instead.
> > 
> > And, the patch should be formatted properly. You need a proper subject
> > prefix (e.g. "ALSA: usb-audio: ....."), then put more detailed patch
> > description texts.
> > 
> > Last but not least, most importantly, we need your Signed-off-by tag.
> > It's a legal requirement.
> > 
> > Could you address the above and resubmit?
> > 
> > 
> > thanks,
> > 
> > Takashi