RE: [PATCH v6 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

[Oder Chiou <oder_chiou@realtek.com>]

> -----Original Message-----
> From: Cezary Rojewski <cezary.rojewski@intel.com>
> Sent: Thursday, November 6, 2025 5:58 PM
> To: Oder Chiou <oder_chiou@realtek.com>
> Cc: alsa-devel@alsa-project.org; Flove(HsinFu) <flove@realtek.com>; Shuming
> [范書銘] <shumingf@realtek.com>; Jack Yu <jack.yu@realtek.com>; Derek [方
> 德義] <derek.fang@realtek.com>; broonie@kernel.org; lgirdwood@gmail.com
> Subject: Re: [PATCH v6 1/2] ASoC: rt5575: Add the codec driver for the
> ALC5575
> 
> The patch looks much nicer. Thank you for the update, Oder. Let's move
> to round 2.

Thanks very much for the suggestions.

> > +};
> > +
> > +struct rt5575_spi_burst_write {
> 
> Naming is weird - it's not a 'BURST_WRITE' command until the 'cmd' field
> says so. I'd rather have this named 'struct rt5575_spi_cmd' as its
> description and usage suggests.

The structure is only for the 'BURST_WRITE' command, and other commands are
different structure.

> > +
> > +             offset += RT5575_SPI_BUF_LEN;
> > +     }
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(rt5575_spi_burst_write);
> > +
> > +static int rt5575_spi_probe(struct spi_device *spi)
> > +{
> > +     rt5575_spi = spi;
> > +
> > +     rt5575_spi_ready = true;
> 
> This looks weird. If rt5575_spi_ready is 'true' unconditionally on
> probe(), why not just replace it with '!rt5575_spi' checks or
> IS_ERR_OR_NULL(rt5575_spi)?

If the kernel failed to allocate memory for the structure or couldn't
successfully match the device, the probe function would not be called.

> > +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> > +static void rt5575_spi_fw_loaded(const struct firmware *fw, void *context)
> 
> This has still not been addressed since my previous review. I do not
> understand the logic here. We have a 'common' file yet filled plagued by
> SPI specifics. On one hand we have rt5575_spi_burst_write() exported and
> part of SPI-specific entity, on the other we have rt5575_spi_fw_loaded()
> hidden here.
> 
> Design if off, choose one or the other. In general, prefer the common
> code to be actually _common_. Makes it easier to Realtek and non-Realtek
> to maintain the code going forward. Leave specifics within the
> specifics-files.

The function name will be changed as rt5575_fw_load_by_spi.