| Message ID | 20230106091543.2440-15-kiseok.jo@irondevice.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Add a driver the Iron Device SMA1303 AMP | expand |
There are a number of issues with this patch... :( The bug reports were from kbuild so you'll probably need to just resend the patch series from before as a v2 or something. It can't be [PATCH 14/14]. Where are the others in the series? If you do fix these issues as a separate patch: 1) It needs a subsystem prefix like "[PATCH] ASoC: sma1303: " or something. 2) It fixes 3 different issues so it should be 3 different patches. 3) It needs a commit description. 4) It needs a Fixes tag. > @@ -772,12 +772,13 @@ static int sma1303_add_component_controls(struct snd_soc_component *component) > sma1303_controls = devm_kzalloc(sma1303->dev, > sizeof(sma1303_snd_controls), GFP_KERNEL); > name = devm_kzalloc(sma1303->dev, > - ARRAY_SIZE(sma1303_snd_controls), GFP_KERNEL); > + ARRAY_SIZE(sma1303_snd_controls)*sizeof(char *), > + GFP_KERNEL); I am surprised checkpatch doesn't complain that spaces are required around the * operator. Please just use sizeof(sma1303_snd_controls). Otherwise you have to use devm_kcalloc() to avoid checkers warning about integer overflows. > > for (index = 0; index < ARRAY_SIZE(sma1303_snd_controls); index++) { > sma1303_controls[index] = sma1303_snd_controls[index]; > name[index] = devm_kzalloc(sma1303->dev, > - MAX_CONTROL_NAME, GFP_KERNEL); > + MAX_CONTROL_NAME*sizeof(char), GFP_KERNEL); sizeof(char) is not required. It's always zero. > size = strlen(sma1303_snd_controls[index].name) > + strlen(sma1303->dev->driver->name); > if (!name[index] || size > MAX_CONTROL_NAME) { regards, dan carpenter
On Fri, Jan 06, 2023 at 12:27:49PM +0300, Dan Carpenter wrote: > > for (index = 0; index < ARRAY_SIZE(sma1303_snd_controls); index++) { > > sma1303_controls[index] = sma1303_snd_controls[index]; > > name[index] = devm_kzalloc(sma1303->dev, > > - MAX_CONTROL_NAME, GFP_KERNEL); > > + MAX_CONTROL_NAME*sizeof(char), GFP_KERNEL); > > sizeof(char) is not required. It's always zero. s/zero/one/ obviously. regards, dan carpenter
Hi Dan, I'm sorry. There was an opinion that the pach sent last time was inconvenient to look at because the entire patch aws modified at once. I just misunderstood the patch system, so I re-edited it and sent it again. I'll continue to use this patch later. Thanks for your kindness. I'll get your description below. -----Original Message----- > Subject: Re: [PATCH 14/14] Fixed the retry_cnt bug about being zero > There are a number of issues with this patch... :( > The bug reports were from kbuild so you'll probably need to just resend the patch series from before as a v2 or something. It can't be [PATCH 14/14]. Where are the others in the series? > If you do fix these issues as a separate patch: > 1) It needs a subsystem prefix like "[PATCH] ASoC: sma1303: " or something. > 2) It fixes 3 different issues so it should be 3 different patches. > 3) It needs a commit description. > 4) It needs a Fixes tag. >> @@ -772,12 +772,13 @@ static int sma1303_add_component_controls(struct snd_soc_component *component) >> sma1303_controls = devm_kzalloc(sma1303->dev, >> sizeof(sma1303_snd_controls), GFP_KERNEL); >> name = devm_kzalloc(sma1303->dev, >> - ARRAY_SIZE(sma1303_snd_controls), GFP_KERNEL); >> + ARRAY_SIZE(sma1303_snd_controls)*sizeof(char *), >> + GFP_KERNEL); >I am surprised checkpatch doesn't complain that spaces are required around the * operator. Please just use sizeof(sma1303_snd_controls). Otherwise you have to use devm_kcalloc() to avoid checkers warning about integer overflows. I lost space between * operator. Thanks. (why didn't checkpatch check it? :( ) But I don't understand why I use 'sizeof(sma1303_snd_controls)'. I only need to know the number of 'sma1303_snd_controls'. In 'sma1303_snd_controls', it has only 3. So ARRAY_SIZE(sma1303_snd_controls) is 3, but sizeof(sma1303_snd_controls) has the value of 144. I think it's not necessary. What's the best? >> >> for (index = 0; index < ARRAY_SIZE(sma1303_snd_controls); index++) { >> sma1303_controls[index] = sma1303_snd_controls[index]; >> name[index] = devm_kzalloc(sma1303->dev, >> - MAX_CONTROL_NAME, GFP_KERNEL); >> + MAX_CONTROL_NAME*sizeof(char), GFP_KERNEL); >sizeof(char) is not required. It's always zero. >> size = strlen(sma1303_snd_controls[index].name) >> + strlen(sma1303->dev->driver->name); >> if (!name[index] || size > MAX_CONTROL_NAME) { regards, dan carpenter
> On Fri, Jan 06, 2023 at 12:27:49PM +0300, Dan Carpenter wrote: > > > for (index = 0; index < ARRAY_SIZE(sma1303_snd_controls); index++) { > > > sma1303_controls[index] = sma1303_snd_controls[index]; > > > name[index] = devm_kzalloc(sma1303->dev, > > > - MAX_CONTROL_NAME, GFP_KERNEL); > > > + MAX_CONTROL_NAME*sizeof(char), GFP_KERNEL); > > > > sizeof(char) is not required. It's always zero. > s/zero/one/ obviously. I understand. I agree the value unnecessary. I'll remove it. Thanks It was just put for the data type checking. > regards, > dan carpenter Best regards, Kiseok Jo
On Fri, Jan 06, 2023 at 09:55:43AM +0000, Ki-Seok Jo wrote: > >> @@ -772,12 +772,13 @@ static int sma1303_add_component_controls(struct snd_soc_component *component) > >> sma1303_controls = devm_kzalloc(sma1303->dev, > >> sizeof(sma1303_snd_controls), GFP_KERNEL); > >> name = devm_kzalloc(sma1303->dev, > >> - ARRAY_SIZE(sma1303_snd_controls), GFP_KERNEL); > >> + ARRAY_SIZE(sma1303_snd_controls)*sizeof(char *), > >> + GFP_KERNEL); > > >I am surprised checkpatch doesn't complain that spaces are required around the * operator. Please just use sizeof(sma1303_snd_controls). > Otherwise you have to use devm_kcalloc() to avoid checkers warning about integer overflows. > > I lost space between * operator. Thanks. (why didn't checkpatch check it? :( ) > > But I don't understand why I use 'sizeof(sma1303_snd_controls)'. > I only need to know the number of 'sma1303_snd_controls'. > In 'sma1303_snd_controls', it has only 3. > > So ARRAY_SIZE(sma1303_snd_controls) is 3, but sizeof(sma1303_snd_controls) has the value of 144. > I think it's not necessary. What's the best? > Ah. Sorry, I didn't have enough context. But could you instead use sizeof(*name) instead of (char *) (it's the standard kernel style and not just my opinion): name = devm_kcalloc(sma1303->dev, ARRAY_SIZE(sma1303_snd_controls), sizeof(*name), GFP_KERNEL); Also please declare name as char instead of unsigned char. Also there needs to be some error checking for if the allocation fails. This driver is going to need quite a bit of cleanup. :/ regards, dan carpenter
On Fri, Jan 06, 2023 at 09:55:43AM +0000, Ki-Seok Jo wrote: > > Hi Dan, > > I'm sorry. There was an opinion that the pach sent last time was inconvenient to look at because the entire patch aws modified at once. > What you should have done was just fold everything into two patches: patch 1: add the driver patch 2: add the device tree bindings Instead you did: patch 1: add the driver patch 2: add the device tree bindings patch 3: re-write all of patch 1. Re-writing everything is not allowed, but it's also not necessary. And also it is against the rules to submit broken code and fix it later. It's a new driver so just fix patch 1 and resend that as a v2 patch. Same for the stuff I mentioned in my bug report. https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/ regards, dan carpenter
>On Fri, Jan 06, 2023 at 09:55:43AM +0000, Ki-Seok Jo wrote: > > > > Hi Dan, > > > > I'm sorry. There was an opinion that the pach sent last time was inconvenient to look at because the entire patch aws modified at once. > > > What you should have done was just fold everything into two patches: > patch 1: add the driver > patch 2: add the device tree bindings > Instead you did: > patch 1: add the driver > patch 2: add the device tree bindings > patch 3: re-write all of patch 1. > Re-writing everything is not allowed, but it's also not necessary. And also it is against the rules to submit broken code and fix it later. > It's a new driver so just fix patch 1 and resend that as a v2 patch. > Same for the stuff I mentioned in my bug report. > https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/ > regards, > dan carpenter Thank you for your kindly advise. I read your report and it was very helpful. As I understand, I already sent it the wrong way. So I want to pick up the pieces. First, I already sent the new driver code a few months ago. After that, I got several feedbacks. I've edit and test it. So a lot of things changed at once. Since I changed so many things, I didn't know what to do, so I just updated it as a patch. It's my mistake... So I already sent about patch 1 and 2, if I get the feedback, should I send a lot of changes as v2 patch?(not patch 3) For each change, should I send patch log per commit? Like that: Patch 1: add the driver Patch 2: add the device tree bindings (instead patch 3) + Patch v2 1: change 1 about feedback1 + Patch v2 2: change 2 about feedback1 + ... + Patch v2 10: change 3 about feedback1 Is it right? Or should I revise it again and send it again from v2 patch 1? (It's not registered with the kernel source yet..) Patch v2 1: add the driver (applied the feedback) Patch v2 2: add the device tree bindings I'm writing this email first because I am worried that I might send it wrong again... Thank you for your kindly help. Best regards, Kiseok Jo
On Mon, Jan 09, 2023 at 07:33:19AM +0000, Ki-Seok Jo wrote: > > >On Fri, Jan 06, 2023 at 09:55:43AM +0000, Ki-Seok Jo wrote: > > > > > > Hi Dan, > > > > > > I'm sorry. There was an opinion that the pach sent last time was inconvenient to look at because the entire patch aws modified at once. > > > > > > What you should have done was just fold everything into two patches: > > patch 1: add the driver > > patch 2: add the device tree bindings > > > Instead you did: > > patch 1: add the driver > > patch 2: add the device tree bindings > > patch 3: re-write all of patch 1. > > > Re-writing everything is not allowed, but it's also not necessary. And also it is against the rules to submit broken code and fix it later. > > > It's a new driver so just fix patch 1 and resend that as a v2 patch. > > Same for the stuff I mentioned in my bug report. > > > https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/ > > > regards, > > dan carpenter > > > Thank you for your kindly advise. I read your report and it was very helpful. > As I understand, I already sent it the wrong way. So I want to pick up the pieces. > > First, I already sent the new driver code a few months ago. > After that, I got several feedbacks. > I've edit and test it. So a lot of things changed at once. > > Since I changed so many things, I didn't know what to do, so I just updated it as a patch. > It's my mistake... > > So I already sent about patch 1 and 2, if I get the feedback, should I send a lot of changes as v2 patch?(not patch 3) > For each change, should I send patch log per commit? > > Like that: > Patch 1: add the driver > Patch 2: add the device tree bindings > > (instead patch 3) > + Patch v2 1: change 1 about feedback1 > + Patch v2 2: change 2 about feedback1 > + ... > + Patch v2 10: change 3 about feedback1 > > Is it right? No. > > Or should I revise it again and send it again from v2 patch 1? > (It's not registered with the kernel source yet..) > Patch v2 1: add the driver (applied the feedback) > Patch v2 2: add the device tree bindings > Yes. Revise again and resend everything as two patches. regards, dan carpenter
> On Mon, Jan 09, 2023 at 07:33:19AM +0000, Ki-Seok Jo wrote: > > > > >On Fri, Jan 06, 2023 at 09:55:43AM +0000, Ki-Seok Jo wrote: > > > > > > > > Hi Dan, > > > > > > > > I'm sorry. There was an opinion that the pach sent last time was inconvenient to look at because the entire patch aws modified at once. > > > > > > > > > What you should have done was just fold everything into two patches: > > > patch 1: add the driver > > > patch 2: add the device tree bindings > > > > > Instead you did: > > > patch 1: add the driver > > > patch 2: add the device tree bindings patch 3: re-write all of patch > > > 1. > > > > > Re-writing everything is not allowed, but it's also not necessary. And also it is against the rules to submit broken code and fix it later. > > > > > It's a new driver so just fix patch 1 and resend that as a v2 patch. > > > Same for the stuff I mentioned in my bug report. > > > > > https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-pat > > > ch/ > > > > > regards, > > > dan carpenter > > > > > > Thank you for your kindly advise. I read your report and it was very helpful. > > As I understand, I already sent it the wrong way. So I want to pick up the pieces. > > > > First, I already sent the new driver code a few months ago. > > After that, I got several feedbacks. > > I've edit and test it. So a lot of things changed at once. > > > > Since I changed so many things, I didn't know what to do, so I just updated it as a patch. > > It's my mistake... > > > > So I already sent about patch 1 and 2, if I get the feedback, should I > > send a lot of changes as v2 patch?(not patch 3) For each change, should I send patch log per commit? > > > > Like that: > > Patch 1: add the driver > > Patch 2: add the device tree bindings > > > > (instead patch 3) > > + Patch v2 1: change 1 about feedback1 Patch v2 2: change 2 about > > + feedback1 ... > > + Patch v2 10: change 3 about feedback1 > > > > Is it right? > No. > > > > Or should I revise it again and send it again from v2 patch 1? > > (It's not registered with the kernel source yet..) Patch v2 1: add the > > driver (applied the feedback) Patch v2 2: add the device tree bindings > > > Yes. Revise again and resend everything as two patches. > regards, > dan carpenter Thank you! I'll try again. I only updates version like v2 patch as two patches(add driver & add device tree bindings) for registering a new driver. Best regards, Kiseok Jo
diff --git a/sound/soc/codecs/sma1303.c b/sound/soc/codecs/sma1303.c index 1a5d992bf3db..4f9dab5d1613 100644 --- a/sound/soc/codecs/sma1303.c +++ b/sound/soc/codecs/sma1303.c @@ -247,7 +247,7 @@ EXPORT_SYMBOL(sma1303_set_callback_func); static int sma1303_regmap_write(struct sma1303_priv *sma1303, unsigned int reg, unsigned int val) { - int ret; + int ret = 0; int cnt = sma1303->retry_cnt; while (cnt--) { @@ -266,7 +266,7 @@ static int sma1303_regmap_write(struct sma1303_priv *sma1303, static int sma1303_regmap_update_bits(struct sma1303_priv *sma1303, unsigned int reg, unsigned int mask, unsigned int val) { - int ret; + int ret = 0; int cnt = sma1303->retry_cnt; while (cnt--) { @@ -285,7 +285,7 @@ static int sma1303_regmap_update_bits(struct sma1303_priv *sma1303, static int sma1303_regmap_read(struct sma1303_priv *sma1303, unsigned int reg, unsigned int *val) { - int ret; + int ret = 0; int cnt = sma1303->retry_cnt; while (cnt--) { @@ -772,12 +772,13 @@ static int sma1303_add_component_controls(struct snd_soc_component *component) sma1303_controls = devm_kzalloc(sma1303->dev, sizeof(sma1303_snd_controls), GFP_KERNEL); name = devm_kzalloc(sma1303->dev, - ARRAY_SIZE(sma1303_snd_controls), GFP_KERNEL); + ARRAY_SIZE(sma1303_snd_controls)*sizeof(char *), + GFP_KERNEL); for (index = 0; index < ARRAY_SIZE(sma1303_snd_controls); index++) { sma1303_controls[index] = sma1303_snd_controls[index]; name[index] = devm_kzalloc(sma1303->dev, - MAX_CONTROL_NAME, GFP_KERNEL); + MAX_CONTROL_NAME*sizeof(char), GFP_KERNEL); size = strlen(sma1303_snd_controls[index].name) + strlen(sma1303->dev->driver->name); if (!name[index] || size > MAX_CONTROL_NAME) { @@ -1544,7 +1545,7 @@ static int sma1303_i2c_probe(struct i2c_client *client, struct sma1303_priv *sma1303; struct device_node *np = client->dev.of_node; int ret, i = 0; - u32 value; + u32 value = 0; unsigned int device_info, status, otp_stat; sma1303 = devm_kzalloc(&client->dev, @@ -1564,13 +1565,13 @@ static int sma1303_i2c_probe(struct i2c_client *client, if (np) { if (!of_property_read_u32(np, "i2c-retry", &value)) { - if (value > 50 || value < 0) { + if (value > 50 || value <= 0) { sma1303->retry_cnt = SMA1303_I2C_RETRY_COUNT; dev_info(&client->dev, "%s : %s\n", __func__, "i2c-retry out of range (up to 50)"); } else { sma1303->retry_cnt = value; - dev_info(&client->dev, "%s : %s = %d\n", + dev_info(&client->dev, "%s : %s = %u\n", __func__, "i2c-retry count", value); } } else { @@ -1589,7 +1590,7 @@ static int sma1303_i2c_probe(struct i2c_client *client, } if (!of_property_read_u32(np, "tdm-slot-tx", &value)) { dev_info(&client->dev, - "tdm slot tx is '%d' from DT\n", value); + "tdm slot tx is '%u' from DT\n", value); sma1303->tdm_slot_tx = value; } else { dev_info(&client->dev, @@ -1609,7 +1610,7 @@ static int sma1303_i2c_probe(struct i2c_client *client, break; default: dev_err(&client->dev, - "Invalid sys-clk-id: %d\n", value); + "Invalid sys-clk-id: %u\n", value); return -EINVAL; } sma1303->sys_clk_id = value;
Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <error27@gmail.com> --- sound/soc/codecs/sma1303.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)