Message ID | 1571295929-47286-25-git-send-email-biju.das@bp.renesas.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | Audio improvements/SSIU BUSIF/ | expand |
Hi! > In the future, we will support BUSIFn, but it will be more complicated > numbering. To avoid future confusable code, this patch modify > rsnd_mod_name() to return understandable name. > > To avoid using pointless memory, it uses static char and snprintf, > thus, rsnd_mod_name() user should use it immediately, and shouldn't keep > its pointer. No, don't do that. That's a dangerous interface. > +#define MOD_NAME_SIZE 16 > +char *rsnd_mod_name(struct rsnd_mod *mod) > +{ > + static char name[MOD_NAME_SIZE]; > + > + /* > + * Let's use same char to avoid pointlessness memory > + * Thus, rsnd_mod_name() should be used immediately > + * Don't keep pointer > + */ > + if ((mod)->ops->id_sub) { > + snprintf(name, MOD_NAME_SIZE, "%s[%d%d]", > + mod->ops->name, > + rsnd_mod_id(mod), > + rsnd_mod_id_sub(mod)); > + } else { > + snprintf(name, MOD_NAME_SIZE, "%s[%d]", > + mod->ops->name, > + rsnd_mod_id(mod)); > + } > + > + return name; > +} This is too hard to use correctly. > @@ -762,12 +759,10 @@ static int rsnd_dma_alloc(struct rsnd_dai_stream *io, struct rsnd_mod *mod, > if (ret < 0) > return ret; > > - dev_dbg(dev, "%s[%d] %s[%d] -> %s[%d]\n", > - rsnd_mod_name(*dma_mod), rsnd_mod_id(*dma_mod), > + dev_dbg(dev, "%s %s -> %s\n", > + rsnd_mod_name(*dma_mod), > rsnd_mod_name(mod_from ? mod_from : &mem), > - rsnd_mod_id (mod_from ? mod_from : &mem), > - rsnd_mod_name(mod_to ? mod_to : &mem), > - rsnd_mod_id (mod_to ? mod_to : &mem)); > + rsnd_mod_name(mod_to ? mod_to : &mem)); > > ret = attach(io, dma, mod_from, mod_to); > if (ret < 0) For example this usage is not correct. And yes, I took a look at mainline, and no, having 5 static buffers is not really an acceptable solution. Best regards, Pavel
Hi Pavel, Thanks for the feedback. > Subject: Re: [PATCH 4.19.y-cip 24/57] ASoC: rsnd: rsnd_mod_name() handles > both name and ID > > Hi! > > > In the future, we will support BUSIFn, but it will be more complicated > > numbering. To avoid future confusable code, this patch modify > > rsnd_mod_name() to return understandable name. > > > > To avoid using pointless memory, it uses static char and snprintf, > > thus, rsnd_mod_name() user should use it immediately, and shouldn't > > keep its pointer. > > No, don't do that. That's a dangerous interface. Can you please elaborate more on the dangerous interface ? > > +#define MOD_NAME_SIZE 16 > > +char *rsnd_mod_name(struct rsnd_mod *mod) { > > + static char name[MOD_NAME_SIZE]; > > + > > + /* > > + * Let's use same char to avoid pointlessness memory > > + * Thus, rsnd_mod_name() should be used immediately > > + * Don't keep pointer > > + */ > > + if ((mod)->ops->id_sub) { > > + snprintf(name, MOD_NAME_SIZE, "%s[%d%d]", > > + mod->ops->name, > > + rsnd_mod_id(mod), > > + rsnd_mod_id_sub(mod)); > > + } else { > > + snprintf(name, MOD_NAME_SIZE, "%s[%d]", > > + mod->ops->name, > > + rsnd_mod_id(mod)); > > + } > > + > > + return name; > > +} > > This is too hard to use correctly. Can you please suggest the correct way of doing this? > > @@ -762,12 +759,10 @@ static int rsnd_dma_alloc(struct rsnd_dai_stream > *io, struct rsnd_mod *mod, > > if (ret < 0) > > return ret; > > > > - dev_dbg(dev, "%s[%d] %s[%d] -> %s[%d]\n", > > - rsnd_mod_name(*dma_mod), rsnd_mod_id(*dma_mod), > > + dev_dbg(dev, "%s %s -> %s\n", > > + rsnd_mod_name(*dma_mod), > > rsnd_mod_name(mod_from ? mod_from : &mem), > > - rsnd_mod_id (mod_from ? mod_from : &mem), > > - rsnd_mod_name(mod_to ? mod_to : &mem), > > - rsnd_mod_id (mod_to ? mod_to : &mem)); > > + rsnd_mod_name(mod_to ? mod_to : &mem)); > > > > ret = attach(io, dma, mod_from, mod_to); > > if (ret < 0) > > For example this usage is not correct. > > And yes, I took a look at mainline, and no, having 5 static buffers is not really > an acceptable solution. > Cheers, Biju
Hi! > > > In the future, we will support BUSIFn, but it will be more complicated > > > numbering. To avoid future confusable code, this patch modify > > > rsnd_mod_name() to return understandable name. > > > > > > To avoid using pointless memory, it uses static char and snprintf, > > > thus, rsnd_mod_name() user should use it immediately, and shouldn't > > > keep its pointer. > > > > No, don't do that. That's a dangerous interface. > > Can you please elaborate more on the dangerous interface ? > > > > +#define MOD_NAME_SIZE 16 > > > +char *rsnd_mod_name(struct rsnd_mod *mod) { > > > + static char name[MOD_NAME_SIZE]; > > > + > > > + /* > > > + * Let's use same char to avoid pointlessness memory > > > + * Thus, rsnd_mod_name() should be used immediately > > > + * Don't keep pointer > > > + */ > > > + if ((mod)->ops->id_sub) { > > > + snprintf(name, MOD_NAME_SIZE, "%s[%d%d]", > > > + mod->ops->name, > > > + rsnd_mod_id(mod), > > > + rsnd_mod_id_sub(mod)); > > > + } else { > > > + snprintf(name, MOD_NAME_SIZE, "%s[%d]", > > > + mod->ops->name, > > > + rsnd_mod_id(mod)); > > > + } > > > + > > > + return name; > > > +} > > > > This is too hard to use correctly. > > Can you please suggest the correct way of doing this? void rsnd_mod_name(char *buf, struct rsnd_mod *mod) {} would be usual way of doing it. Semantics would be similar to sprintf(). Best regards, Pavel
+ Morimoto-San > Subject: Re: [PATCH 4.19.y-cip 24/57] ASoC: rsnd: rsnd_mod_name() handles > both name and ID > > Hi! > > > > > In the future, we will support BUSIFn, but it will be more > > > > complicated numbering. To avoid future confusable code, this patch > > > > modify > > > > rsnd_mod_name() to return understandable name. > > > > > > > > To avoid using pointless memory, it uses static char and snprintf, > > > > thus, rsnd_mod_name() user should use it immediately, and > > > > shouldn't keep its pointer. > > > > > > No, don't do that. That's a dangerous interface. > > > > Can you please elaborate more on the dangerous interface ? > > > > > > +#define MOD_NAME_SIZE 16 > > > > +char *rsnd_mod_name(struct rsnd_mod *mod) { > > > > + static char name[MOD_NAME_SIZE]; > > > > + > > > > + /* > > > > + * Let's use same char to avoid pointlessness memory > > > > + * Thus, rsnd_mod_name() should be used immediately > > > > + * Don't keep pointer > > > > + */ > > > > + if ((mod)->ops->id_sub) { > > > > + snprintf(name, MOD_NAME_SIZE, "%s[%d%d]", > > > > + mod->ops->name, > > > > + rsnd_mod_id(mod), > > > > + rsnd_mod_id_sub(mod)); > > > > + } else { > > > > + snprintf(name, MOD_NAME_SIZE, "%s[%d]", > > > > + mod->ops->name, > > > > + rsnd_mod_id(mod)); > > > > + } > > > > + > > > > + return name; > > > > +} > > > > > > This is too hard to use correctly. > > > > Can you please suggest the correct way of doing this? > > void rsnd_mod_name(char *buf, struct rsnd_mod *mod) {} would be usual > way of doing it. Semantics would be similar to sprintf(). Morimoto-San, Please share your opinion on this. Regards, Biju
Hi Biju > > > > > In the future, we will support BUSIFn, but it will be more > > > > > complicated numbering. To avoid future confusable code, this patch > > > > > modify > > > > > rsnd_mod_name() to return understandable name. > > > > > > > > > > To avoid using pointless memory, it uses static char and snprintf, > > > > > thus, rsnd_mod_name() user should use it immediately, and > > > > > shouldn't keep its pointer. > > > > > > > > No, don't do that. That's a dangerous interface. > > > > > > Can you please elaborate more on the dangerous interface ? > > > > > > > > +#define MOD_NAME_SIZE 16 > > > > > +char *rsnd_mod_name(struct rsnd_mod *mod) { > > > > > + static char name[MOD_NAME_SIZE]; > > > > > + > > > > > + /* > > > > > + * Let's use same char to avoid pointlessness memory > > > > > + * Thus, rsnd_mod_name() should be used immediately > > > > > + * Don't keep pointer > > > > > + */ > > > > > + if ((mod)->ops->id_sub) { > > > > > + snprintf(name, MOD_NAME_SIZE, "%s[%d%d]", > > > > > + mod->ops->name, > > > > > + rsnd_mod_id(mod), > > > > > + rsnd_mod_id_sub(mod)); > > > > > + } else { > > > > > + snprintf(name, MOD_NAME_SIZE, "%s[%d]", > > > > > + mod->ops->name, > > > > > + rsnd_mod_id(mod)); > > > > > + } > > > > > + > > > > > + return name; > > > > > +} > > > > > > > > This is too hard to use correctly. > > > > > > Can you please suggest the correct way of doing this? > > > > void rsnd_mod_name(char *buf, struct rsnd_mod *mod) {} would be usual > > way of doing it. Semantics would be similar to sprintf(). > > Morimoto-San, > Please share your opinion on this. rsnd_mod_name() is used for debug or error case, and not used in Hi frequency. At least, it doesn't used over 5 (= MOD_NAME_NUM) times in the same time. So, it is no problem *at this point*. But, indeed it is including potential issue... Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 93848f4..6023fb3 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -123,8 +123,8 @@ void rsnd_mod_make_sure(struct rsnd_mod *mod, enum rsnd_mod_type type) struct rsnd_priv *priv = rsnd_mod_to_priv(mod); struct device *dev = rsnd_priv_to_dev(priv); - dev_warn(dev, "%s[%d] is not your expected module\n", - rsnd_mod_name(mod), rsnd_mod_id(mod)); + dev_warn(dev, "%s is not your expected module\n", + rsnd_mod_name(mod)); } } @@ -137,6 +137,30 @@ struct dma_chan *rsnd_mod_dma_req(struct rsnd_dai_stream *io, return mod->ops->dma_req(io, mod); } +#define MOD_NAME_SIZE 16 +char *rsnd_mod_name(struct rsnd_mod *mod) +{ + static char name[MOD_NAME_SIZE]; + + /* + * Let's use same char to avoid pointlessness memory + * Thus, rsnd_mod_name() should be used immediately + * Don't keep pointer + */ + if ((mod)->ops->id_sub) { + snprintf(name, MOD_NAME_SIZE, "%s[%d%d]", + mod->ops->name, + rsnd_mod_id(mod), + rsnd_mod_id_sub(mod)); + } else { + snprintf(name, MOD_NAME_SIZE, "%s[%d]", + mod->ops->name, + rsnd_mod_id(mod)); + } + + return name; +} + u32 *rsnd_mod_get_status(struct rsnd_mod *mod, struct rsnd_dai_stream *io, enum rsnd_mod_type type) @@ -494,15 +518,14 @@ static int rsnd_status_update(u32 *status, __rsnd_mod_shift_##fn, \ __rsnd_mod_add_##fn, \ __rsnd_mod_call_##fn); \ - rsnd_dbg_dai_call(dev, "%s[%d]\t0x%08x %s\n", \ - rsnd_mod_name(mod), rsnd_mod_id(mod), *status, \ + rsnd_dbg_dai_call(dev, "%s\t0x%08x %s\n", \ + rsnd_mod_name(mod), *status, \ (func_call && (mod)->ops->fn) ? #fn : ""); \ if (func_call && (mod)->ops->fn) \ tmp = (mod)->ops->fn(mod, io, param); \ if (tmp && (tmp != -EPROBE_DEFER)) \ - dev_err(dev, "%s[%d] : %s error %d\n", \ - rsnd_mod_name(mod), rsnd_mod_id(mod), \ - #fn, tmp); \ + dev_err(dev, "%s : %s error %d\n", \ + rsnd_mod_name(mod), #fn, tmp); \ ret |= tmp; \ } \ ret; \ @@ -529,8 +552,8 @@ int rsnd_dai_connect(struct rsnd_mod *mod, io->mod[type] = mod; - dev_dbg(dev, "%s[%d] is connected to io (%s)\n", - rsnd_mod_name(mod), rsnd_mod_id(mod), + dev_dbg(dev, "%s is connected to io (%s)\n", + rsnd_mod_name(mod), rsnd_io_is_play(io) ? "Playback" : "Capture"); return 0; diff --git a/sound/soc/sh/rcar/dma.c b/sound/soc/sh/rcar/dma.c index e5c30ee..5daa6c9 100644 --- a/sound/soc/sh/rcar/dma.c +++ b/sound/soc/sh/rcar/dma.c @@ -174,8 +174,8 @@ static int rsnd_dmaen_start(struct rsnd_mod *mod, cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; - dev_dbg(dev, "%s[%d] %pad -> %pad\n", - rsnd_mod_name(mod), rsnd_mod_id(mod), + dev_dbg(dev, "%s %pad -> %pad\n", + rsnd_mod_name(mod), &cfg.src_addr, &cfg.dst_addr); ret = dmaengine_slave_config(dmaen->chan, &cfg); @@ -369,8 +369,7 @@ static u32 rsnd_dmapp_get_id(struct rsnd_dai_stream *io, if ((!entry) || (size <= id)) { struct device *dev = rsnd_priv_to_dev(rsnd_io_to_priv(io)); - dev_err(dev, "unknown connection (%s[%d])\n", - rsnd_mod_name(mod), rsnd_mod_id(mod)); + dev_err(dev, "unknown connection (%s)\n", rsnd_mod_name(mod)); /* use non-prohibited SRS number as error */ return 0x00; /* SSI00 */ @@ -692,12 +691,10 @@ static void rsnd_dma_of_path(struct rsnd_mod *this, *mod_to = mod[1]; } - dev_dbg(dev, "module connection (this is %s[%d])\n", - rsnd_mod_name(this), rsnd_mod_id(this)); + dev_dbg(dev, "module connection (this is %s)\n", rsnd_mod_name(this)); for (i = 0; i <= idx; i++) { - dev_dbg(dev, " %s[%d]%s\n", + dev_dbg(dev, " %s%s\n", rsnd_mod_name(mod[i] ? mod[i] : &mem), - rsnd_mod_id (mod[i] ? mod[i] : &mem), (mod[i] == *mod_from) ? " from" : (mod[i] == *mod_to) ? " to" : ""); } @@ -762,12 +759,10 @@ static int rsnd_dma_alloc(struct rsnd_dai_stream *io, struct rsnd_mod *mod, if (ret < 0) return ret; - dev_dbg(dev, "%s[%d] %s[%d] -> %s[%d]\n", - rsnd_mod_name(*dma_mod), rsnd_mod_id(*dma_mod), + dev_dbg(dev, "%s %s -> %s\n", + rsnd_mod_name(*dma_mod), rsnd_mod_name(mod_from ? mod_from : &mem), - rsnd_mod_id (mod_from ? mod_from : &mem), - rsnd_mod_name(mod_to ? mod_to : &mem), - rsnd_mod_id (mod_to ? mod_to : &mem)); + rsnd_mod_name(mod_to ? mod_to : &mem)); ret = attach(io, dma, mod_from, mod_to); if (ret < 0) diff --git a/sound/soc/sh/rcar/gen.c b/sound/soc/sh/rcar/gen.c index 1f7881cc..ca63940 100644 --- a/sound/soc/sh/rcar/gen.c +++ b/sound/soc/sh/rcar/gen.c @@ -83,8 +83,8 @@ u32 rsnd_read(struct rsnd_priv *priv, regmap_fields_read(gen->regs[reg], rsnd_mod_id(mod), &val); - dev_dbg(dev, "r %s[%d] - %-18s (%4d) : %08x\n", - rsnd_mod_name(mod), rsnd_mod_id(mod), + dev_dbg(dev, "r %s - %-18s (%4d) : %08x\n", + rsnd_mod_name(mod), rsnd_reg_name(gen, reg), reg, val); return val; @@ -102,8 +102,8 @@ void rsnd_write(struct rsnd_priv *priv, regmap_fields_force_write(gen->regs[reg], rsnd_mod_id(mod), data); - dev_dbg(dev, "w %s[%d] - %-18s (%4d) : %08x\n", - rsnd_mod_name(mod), rsnd_mod_id(mod), + dev_dbg(dev, "w %s - %-18s (%4d) : %08x\n", + rsnd_mod_name(mod), rsnd_reg_name(gen, reg), reg, data); } @@ -119,8 +119,8 @@ void rsnd_bset(struct rsnd_priv *priv, struct rsnd_mod *mod, regmap_fields_force_update_bits(gen->regs[reg], rsnd_mod_id(mod), mask, data); - dev_dbg(dev, "b %s[%d] - %-18s (%4d) : %08x/%08x\n", - rsnd_mod_name(mod), rsnd_mod_id(mod), + dev_dbg(dev, "b %s - %-18s (%4d) : %08x/%08x\n", + rsnd_mod_name(mod), rsnd_reg_name(gen, reg), reg, data, mask); } diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h index fdf007a..28bd90a 100644 --- a/sound/soc/sh/rcar/rsnd.h +++ b/sound/soc/sh/rcar/rsnd.h @@ -377,7 +377,6 @@ struct rsnd_mod { #define __rsnd_mod_call_pointer 0 #define rsnd_mod_to_priv(mod) ((mod)->priv) -#define rsnd_mod_name(mod) ((mod)->ops->name) #define rsnd_mod_power_on(mod) clk_enable((mod)->clk) #define rsnd_mod_power_off(mod) clk_disable((mod)->clk) #define rsnd_mod_get(ip) (&(ip)->mod) @@ -400,6 +399,7 @@ u32 *rsnd_mod_get_status(struct rsnd_mod *mod, int rsnd_mod_id(struct rsnd_mod *mod); int rsnd_mod_id_raw(struct rsnd_mod *mod); int rsnd_mod_id_sub(struct rsnd_mod *mod); +char *rsnd_mod_name(struct rsnd_mod *mod); struct rsnd_mod *rsnd_mod_next(int *iterator, struct rsnd_dai_stream *io, enum rsnd_mod_type *array, diff --git a/sound/soc/sh/rcar/src.c b/sound/soc/sh/rcar/src.c index 7de7afd..bdc0595 100644 --- a/sound/soc/sh/rcar/src.c +++ b/sound/soc/sh/rcar/src.c @@ -349,9 +349,8 @@ static bool rsnd_src_error_occurred(struct rsnd_mod *mod) status0 = rsnd_mod_read(mod, SCU_SYS_STATUS0); status1 = rsnd_mod_read(mod, SCU_SYS_STATUS1); if ((status0 & val0) || (status1 & val1)) { - rsnd_dbg_irq_status(dev, "%s[%d] err status : 0x%08x, 0x%08x\n", - rsnd_mod_name(mod), rsnd_mod_id(mod), - status0, status1); + rsnd_dbg_irq_status(dev, "%s err status : 0x%08x, 0x%08x\n", + rsnd_mod_name(mod), status0, status1); ret = true; } diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index bf14207..81d3864 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -181,8 +181,7 @@ static void rsnd_ssi_status_check(struct rsnd_mod *mod, udelay(5); } - dev_warn(dev, "%s[%d] status check failed\n", - rsnd_mod_name(mod), rsnd_mod_id(mod)); + dev_warn(dev, "%s status check failed\n", rsnd_mod_name(mod)); } static u32 rsnd_ssi_multi_slaves(struct rsnd_dai_stream *io) @@ -346,9 +345,7 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, ssi->rate = rate; ssi->chan = chan; - dev_dbg(dev, "%s[%d] outputs %u Hz\n", - rsnd_mod_name(mod), - rsnd_mod_id(mod), rate); + dev_dbg(dev, "%s outputs %u Hz\n", rsnd_mod_name(mod), rate); return 0; } @@ -494,8 +491,7 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod, return 0; if (!ssi->usrcnt) { - dev_err(dev, "%s[%d] usrcnt error\n", - rsnd_mod_name(mod), rsnd_mod_id(mod)); + dev_err(dev, "%s usrcnt error\n", rsnd_mod_name(mod)); return -EIO; } @@ -654,8 +650,8 @@ static void __rsnd_ssi_interrupt(struct rsnd_mod *mod, /* DMA only */ if (is_dma && (status & (UIRQ | OIRQ))) { - rsnd_dbg_irq_status(dev, "%s[%d] err status : 0x%08x\n", - rsnd_mod_name(mod), rsnd_mod_id(mod), status); + rsnd_dbg_irq_status(dev, "%s err status : 0x%08x\n", + rsnd_mod_name(mod), status); stop = true; } @@ -964,8 +960,7 @@ static int rsnd_ssi_fallback(struct rsnd_mod *mod, */ mod->ops = &rsnd_ssi_pio_ops; - dev_info(dev, "%s[%d] fallback to PIO mode\n", - rsnd_mod_name(mod), rsnd_mod_id(mod)); + dev_info(dev, "%s fallback to PIO mode\n", rsnd_mod_name(mod)); return 0; } @@ -1085,15 +1080,13 @@ static void __rsnd_ssi_parse_hdmi_connection(struct rsnd_priv *priv, /* HDMI0 */ if (strstr(remote_node->full_name, "hdmi@fead0000")) { rsnd_flags_set(ssi, RSND_SSI_HDMI0); - dev_dbg(dev, "%s[%d] connected to HDMI0\n", - rsnd_mod_name(mod), rsnd_mod_id(mod)); + dev_dbg(dev, "%s connected to HDMI0\n", rsnd_mod_name(mod)); } /* HDMI1 */ if (strstr(remote_node->full_name, "hdmi@feae0000")) { rsnd_flags_set(ssi, RSND_SSI_HDMI1); - dev_dbg(dev, "%s[%d] connected to HDMI1\n", - rsnd_mod_name(mod), rsnd_mod_id(mod)); + dev_dbg(dev, "%s connected to HDMI1\n", rsnd_mod_name(mod)); } }