| Message ID | 20221114102956.914468-3-ckeepax@opensource.cirrus.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Minor SoundWire clean ups | expand |
On 11/14/22 04:29, Charles Keepax wrote: > Provide stub functions when CONFIG_SOUNDWIRE is not set for functions > that are quite likely to be used from common code on devices supporting > multiple control buses. So far this case has been covered by splitting SoundWire related code away from, say I2C, and with a clear 'depends on SOUNDWIRE'. This is the case for rt5682, max98373, etc. Is this not good enough? I am not against this patch, just wondering if allowing code for different interfaces to be part of the same file will lead to confusions with e.g. register offsets or functionality exposed with different registers. > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- > include/linux/soundwire/sdw.h | 92 +++++++++++++++++++++++++++++++---- > 1 file changed, 82 insertions(+), 10 deletions(-) > > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index 902ed46f76c80..4f80cba898f11 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -1021,15 +1021,8 @@ int sdw_stream_add_master(struct sdw_bus *bus, > struct sdw_port_config *port_config, > unsigned int num_ports, > struct sdw_stream_runtime *stream); > -int sdw_stream_add_slave(struct sdw_slave *slave, > - struct sdw_stream_config *stream_config, > - struct sdw_port_config *port_config, > - unsigned int num_ports, > - struct sdw_stream_runtime *stream); > int sdw_stream_remove_master(struct sdw_bus *bus, > struct sdw_stream_runtime *stream); > -int sdw_stream_remove_slave(struct sdw_slave *slave, > - struct sdw_stream_runtime *stream); > int sdw_startup_stream(void *sdw_substream); > int sdw_prepare_stream(struct sdw_stream_runtime *stream); > int sdw_enable_stream(struct sdw_stream_runtime *stream); > @@ -1040,8 +1033,20 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus); > int sdw_bus_clk_stop(struct sdw_bus *bus); > int sdw_bus_exit_clk_stop(struct sdw_bus *bus); > > -/* messaging and data APIs */ > +int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id); > +void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id); > + > +#if IS_ENABLED(CONFIG_SOUNDWIRE) > > +int sdw_stream_add_slave(struct sdw_slave *slave, > + struct sdw_stream_config *stream_config, > + struct sdw_port_config *port_config, > + unsigned int num_ports, > + struct sdw_stream_runtime *stream); > +int sdw_stream_remove_slave(struct sdw_slave *slave, > + struct sdw_stream_runtime *stream); > + > +/* messaging and data APIs */ > int sdw_read(struct sdw_slave *slave, u32 addr); > int sdw_write(struct sdw_slave *slave, u32 addr, u8 value); > int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value); > @@ -1053,7 +1058,74 @@ int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 * > int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val); > int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val); > > -int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id); > -void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id); > +#else > + > +static inline int sdw_stream_add_slave(struct sdw_slave *slave, > + struct sdw_stream_config *stream_config, > + struct sdw_port_config *port_config, > + unsigned int num_ports, > + struct sdw_stream_runtime *stream) > +{ > + return 0; > +} > + > +static inline int sdw_stream_remove_slave(struct sdw_slave *slave, > + struct sdw_stream_runtime *stream) > +{ > + return 0; > +} > + > +/* messaging and data APIs */ > +static inline int sdw_read(struct sdw_slave *slave, u32 addr) > +{ > + return 0; > +} > + > +static inline int sdw_write(struct sdw_slave *slave, u32 addr, u8 value) > +{ > + return 0; > +} > + > +static inline int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value) > +{ > + return 0; > +} > + > +static inline int sdw_read_no_pm(struct sdw_slave *slave, u32 addr) > +{ > + return 0; > +} > + > +static inline int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) > +{ > + return 0; > +} > + > +static inline int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) > +{ > + return 0; > +} > + > +static inline int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val) > +{ > + return 0; > +} > + > +static inline int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val) > +{ > + return 0; > +} > + > +static inline int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val) > +{ > + return 0; > +} > + > +static inline int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val) > +{ > + return 0; > +} > + > +#endif /* CONFIG_SOUNDWIRE */ > > #endif /* __SOUNDWIRE_H */
On Mon, Nov 14, 2022 at 10:13:07AM -0600, Pierre-Louis Bossart wrote: > > > On 11/14/22 04:29, Charles Keepax wrote: > > Provide stub functions when CONFIG_SOUNDWIRE is not set for functions > > that are quite likely to be used from common code on devices supporting > > multiple control buses. > > So far this case has been covered by splitting SoundWire related code > away from, say I2C, and with a clear 'depends on SOUNDWIRE'. This is the > case for rt5682, max98373, etc. > > Is this not good enough? > > I am not against this patch, just wondering if allowing code for > different interfaces to be part of the same file will lead to confusions > with e.g. register offsets or functionality exposed with different > registers. > I guess this is a bit of a grey area this one. Both work, I guess the reason I was leaning this way is that in order to avoid a circular dependency if I put all the soundwire DAI handling into the soundwire code then I have to duplicate the snd_soc_dai_driver structure into both the sdw and i2c specific code (worth noting the I2S DAIs are still usable when the part is sdw to the host). But there are also downsides to this approach in that it will likely have some small impact on driver size when soundwire is not built in. Thanks, Charles
On 15/11/2022 11:03, Charles Keepax wrote: > On Mon, Nov 14, 2022 at 10:13:07AM -0600, Pierre-Louis Bossart wrote: >> >> >> On 11/14/22 04:29, Charles Keepax wrote: >>> Provide stub functions when CONFIG_SOUNDWIRE is not set for functions >>> that are quite likely to be used from common code on devices supporting >>> multiple control buses. >> >> So far this case has been covered by splitting SoundWire related code >> away from, say I2C, and with a clear 'depends on SOUNDWIRE'. This is the >> case for rt5682, max98373, etc. >> >> Is this not good enough? >> >> I am not against this patch, just wondering if allowing code for >> different interfaces to be part of the same file will lead to confusions >> with e.g. register offsets or functionality exposed with different >> registers. >> > > I guess this is a bit of a grey area this one. Both work, I guess > the reason I was leaning this way is that in order to avoid a > circular dependency if I put all the soundwire DAI handling into > the soundwire code then I have to duplicate the snd_soc_dai_driver > structure into both the sdw and i2c specific code (worth noting > the I2S DAIs are still usable when the part is sdw to the host). But > there are also downsides to this approach in that it will likely have > some small impact on driver size when soundwire is not built in. > I think we should just add the stubs. Other subsystems use stubs to help with code that references stuff that might not be available. Splitting all the soundwire-specifics out into a separate module works for simple chips that are either I2S or soundwire. but can get messy for a complex codec. I used the separate file method for CS42L42, but for a driver I'm working on I abandoned that and put both DAIs in the core code. I didn't notice the missing stubs because my defconfig that was intended to omit soundwire apparently has something that is selecting it anyway.
On 11/15/22 05:41, Richard Fitzgerald wrote: > On 15/11/2022 11:03, Charles Keepax wrote: >> On Mon, Nov 14, 2022 at 10:13:07AM -0600, Pierre-Louis Bossart wrote: >>> >>> >>> On 11/14/22 04:29, Charles Keepax wrote: >>>> Provide stub functions when CONFIG_SOUNDWIRE is not set for functions >>>> that are quite likely to be used from common code on devices supporting >>>> multiple control buses. >>> >>> So far this case has been covered by splitting SoundWire related code >>> away from, say I2C, and with a clear 'depends on SOUNDWIRE'. This is the >>> case for rt5682, max98373, etc. >>> >>> Is this not good enough? >>> >>> I am not against this patch, just wondering if allowing code for >>> different interfaces to be part of the same file will lead to confusions >>> with e.g. register offsets or functionality exposed with different >>> registers. >>> >> >> I guess this is a bit of a grey area this one. Both work, I guess >> the reason I was leaning this way is that in order to avoid a >> circular dependency if I put all the soundwire DAI handling into >> the soundwire code then I have to duplicate the snd_soc_dai_driver >> structure into both the sdw and i2c specific code (worth noting >> the I2S DAIs are still usable when the part is sdw to the host). But >> there are also downsides to this approach in that it will likely have >> some small impact on driver size when soundwire is not built in. >> > > I think we should just add the stubs. Other subsystems use stubs to help > with code that references stuff that might not be available. > > Splitting all the soundwire-specifics out into a separate module works > for simple chips that are either I2S or soundwire. but can get messy for > a complex codec. I used the separate file method for CS42L42, but for a > driver I'm working on I abandoned that and put both DAIs in the core > code. I didn't notice the missing stubs because my defconfig that was > intended to omit soundwire apparently has something that is selecting > it anyway. It would be good if you could look into this, I don't see any 'select SOUNDWIRE'. I agree the premise of the split was that the device is used in one mode of the other, I am not sure however what the a 'complex codec' would change. It's likely that we will see a second level within a SoundWire device to deal with independent 'functions', but I don't quite see how this would matter. That said, I don't write codec drivers so I am not going to lay on the tracks over 2 stubs. We can revisit the sdw.h split as well later. Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 902ed46f76c80..4f80cba898f11 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -1021,15 +1021,8 @@ int sdw_stream_add_master(struct sdw_bus *bus, struct sdw_port_config *port_config, unsigned int num_ports, struct sdw_stream_runtime *stream); -int sdw_stream_add_slave(struct sdw_slave *slave, - struct sdw_stream_config *stream_config, - struct sdw_port_config *port_config, - unsigned int num_ports, - struct sdw_stream_runtime *stream); int sdw_stream_remove_master(struct sdw_bus *bus, struct sdw_stream_runtime *stream); -int sdw_stream_remove_slave(struct sdw_slave *slave, - struct sdw_stream_runtime *stream); int sdw_startup_stream(void *sdw_substream); int sdw_prepare_stream(struct sdw_stream_runtime *stream); int sdw_enable_stream(struct sdw_stream_runtime *stream); @@ -1040,8 +1033,20 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus); int sdw_bus_clk_stop(struct sdw_bus *bus); int sdw_bus_exit_clk_stop(struct sdw_bus *bus); -/* messaging and data APIs */ +int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id); +void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id); + +#if IS_ENABLED(CONFIG_SOUNDWIRE) +int sdw_stream_add_slave(struct sdw_slave *slave, + struct sdw_stream_config *stream_config, + struct sdw_port_config *port_config, + unsigned int num_ports, + struct sdw_stream_runtime *stream); +int sdw_stream_remove_slave(struct sdw_slave *slave, + struct sdw_stream_runtime *stream); + +/* messaging and data APIs */ int sdw_read(struct sdw_slave *slave, u32 addr); int sdw_write(struct sdw_slave *slave, u32 addr, u8 value); int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value); @@ -1053,7 +1058,74 @@ int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 * int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val); int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val); -int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id); -void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id); +#else + +static inline int sdw_stream_add_slave(struct sdw_slave *slave, + struct sdw_stream_config *stream_config, + struct sdw_port_config *port_config, + unsigned int num_ports, + struct sdw_stream_runtime *stream) +{ + return 0; +} + +static inline int sdw_stream_remove_slave(struct sdw_slave *slave, + struct sdw_stream_runtime *stream) +{ + return 0; +} + +/* messaging and data APIs */ +static inline int sdw_read(struct sdw_slave *slave, u32 addr) +{ + return 0; +} + +static inline int sdw_write(struct sdw_slave *slave, u32 addr, u8 value) +{ + return 0; +} + +static inline int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value) +{ + return 0; +} + +static inline int sdw_read_no_pm(struct sdw_slave *slave, u32 addr) +{ + return 0; +} + +static inline int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) +{ + return 0; +} + +static inline int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) +{ + return 0; +} + +static inline int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val) +{ + return 0; +} + +static inline int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val) +{ + return 0; +} + +static inline int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val) +{ + return 0; +} + +static inline int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val) +{ + return 0; +} + +#endif /* CONFIG_SOUNDWIRE */ #endif /* __SOUNDWIRE_H */
Provide stub functions when CONFIG_SOUNDWIRE is not set for functions that are quite likely to be used from common code on devices supporting multiple control buses. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- include/linux/soundwire/sdw.h | 92 +++++++++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 10 deletions(-)