回复: [bug report] ASoC: tas2781: Add tas2781 driver

[<13916275206@139.com>]

> -----邮件原件-----
> 发件人: Dan Carpenter <dan.carpenter@linaro.org>
> 发送时间: 2023年6月22日 23:07
> 收件人: 13916275206@139.com
> 抄送: alsa-devel@alsa-project.org
> 主题: [bug report] ASoC: tas2781: Add tas2781 driver
> 
> Hello Shenghao Ding,
> 
> The patch ef3bcde75d06: "ASoC: tas2781: Add tas2781 driver" from Jun 18,
> 2023, leads to the following Smatch static checker warning:
> 
> 	sound/soc/codecs/tas2781-i2c.c:651 tasdevice_parse_dt()
> 	warn: assigning signed to unsigned: 'tas_priv->ndev = ndev'
> 's32min-s32max'
> 
> sound/soc/codecs/tas2781-i2c.c
>     602 static void tasdevice_parse_dt(struct tasdevice_priv *tas_priv)
>     603 {
>     604         struct i2c_client *client = (struct i2c_client
> *)tas_priv->client;
>     605         unsigned int dev_addrs[TASDEVICE_MAX_CHANNELS];
>     606         int rc, i, ndev = 0;
>     607
>     608         if (tas_priv->isacpi) {
>     609                 ndev =
> device_property_read_u32_array(&client->dev,
>     610                         "ti,audio-slots", NULL, 0);
> 
> When we pass NULL to device_property_read_u32_array() then it returns then
> number of items.
> 
>     611                 if (ndev <= 0) {
>     612                         ndev = 1;
>     613                         dev_addrs[0] = client->addr;
>     614                 } else {
>     615                         ndev = (ndev <
> ARRAY_SIZE(dev_addrs))
>     616                                 ? ndev :
> ARRAY_SIZE(dev_addrs);
>     617                         ndev =
> device_property_read_u32_array(&client->dev,
>                                 ^^^^^^^
> Smatch is concerned that this value can be negative.  But actually this
sets it
> to zero, doesn't it?  Is that intentional?  It feels like we should leave
ndev as
> the number of items.  Or if we want ndev to be zero do "ndev = 0;" on the
> next line.
> 
>     618                                 "ti,audio-slots", dev_addrs,
> ndev);
Thanks for your report, correct and add as following:
			if (ndev <= 0) {
				ndev = 1;
				dev_addrs[0] = client->addr;
			}
One more thing, how to smatch the code in kernel. Where can I find the
guideline?
>     619                 }
>     620
>     621                 tas_priv->irq_info.irq_gpio =
>     622
> acpi_dev_gpio_irq_get(ACPI_COMPANION(&client->dev), 0);
>     623         } else {
>     624                 struct device_node *np =
> tas_priv->dev->of_node;
>     625 #ifdef CONFIG_OF
>     626                 const __be32 *reg, *reg_end;
>     627                 int len, sw, aw;
>     628
>     629                 aw = of_n_addr_cells(np);
>     630                 sw = of_n_size_cells(np);
>     631                 if (sw == 0) {
>     632                         reg = (const __be32
> *)of_get_property(np,
>     633                                 "reg", &len);
>     634                         reg_end = reg + len/sizeof(*reg);
>     635                         ndev = 0;
>     636                         do {
>     637                                 dev_addrs[ndev] =
> of_read_number(reg, aw);
>     638                                 reg += aw;
>     639                                 ndev++;
>     640                         } while (reg < reg_end);
>     641                 } else {
>     642                         ndev = 1;
>     643                         dev_addrs[0] = client->addr;
>     644                 }
>     645 #else
>     646                 ndev = 1;
>     647                 dev_addrs[0] = client->addr;
>     648 #endif
>     649                 tas_priv->irq_info.irq_gpio = of_irq_get(np, 0);
>     650         }
> --> 651         tas_priv->ndev = ndev;
>                 ^^^^^^^^^^^^^^^^^^^^^
> Warning
> 
Do I need the casting? ndev is sure to less than or equal to 8 and more than
1, is won't be overflow in unsigned char.
>     652         for (i = 0; i < ndev; i++)
>     653                 tas_priv->tasdevice[i].dev_addr = dev_addrs[i];
>     654
>     655         tas_priv->reset = devm_gpiod_get_optional(&client->dev,
>     656                         "reset-gpios", GPIOD_OUT_HIGH);
> 
> regards,
> dan carpenter