Messages in this thread | | | Date | Wed, 30 Nov 2022 09:25:26 +0530 | Subject | Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip | From | Manjunatha Venkatesh <> |
| |
On 10/7/2022 7:41 PM, Arnd Bergmann wrote: > Caution: EXT Email > > On Fri, Oct 7, 2022, at 4:04 PM, Manjunatha Venkatesh wrote: >> sr1xx_dev_open(struct inode *inode, struct file *filp) >>>> +{ >>>> + struct sr1xx_dev *sr1xx_dev = >>>> + container_of(filp->private_data, struct sr1xx_dev, sr1xx_device); >>>> + >>>> + filp->private_data = sr1xx_dev; >>> This looks dangerous if the file gets opened more than once >>> and filp->private_data can have two different values. >> Do you suggest us to use mutex lock inside open api? > >>>> + >>>> + sr1xx_dev->spi = spi; >>>> + sr1xx_dev->sr1xx_device.minor = MISC_DYNAMIC_MINOR; >>>> + sr1xx_dev->sr1xx_device.name = "sr1xx"; >>>> + sr1xx_dev->sr1xx_device.fops = &sr1xx_dev_fops; >>>> + sr1xx_dev->sr1xx_device.parent = &spi->dev; >>>> + sr1xx_dev->irq_gpio = desc_to_gpio(platform_data.gpiod_irq); >>>> + sr1xx_dev->ce_gpio = desc_to_gpio(platform_data.gpiod_ce); >>>> + sr1xx_dev->spi_handshake_gpio = >>>> + desc_to_gpio(platform_data.gpiod_spi_handshake); >>> The temporary 'platform_data' structure seems useless here, >>> just fold its members into the sr1xx_dev structure itself. >>> No need to store both a gpio descriptor and a number, you >>> can simplify this to always use the descriptor. >> Just to keep separate function(sr1xx_hw_setup) for better readability >> we have added intermediate platform_data structure. > I'm fairly sure it adds nothing to readability if every reader has > to wonder about why you have a platform_data structure here when > the device was never used without devicetree. Will fix this in the next patch submission. >>>> + sr1xx_dev->tx_buffer = kzalloc(SR1XX_TXBUF_SIZE, GFP_KERNEL); >>>> + sr1xx_dev->rx_buffer = kzalloc(SR1XX_RXBUF_SIZE, GFP_KERNEL); >>>> + if (!sr1xx_dev->tx_buffer) { >>>> + ret = -ENOMEM; >>>> + goto err_exit; >>>> + } >>>> + if (!sr1xx_dev->rx_buffer) { >>>> + ret = -ENOMEM; >>>> + goto err_exit; >>>> + } >>>> + >>>> + sr1xx_dev->spi->irq = gpio_to_irq(sr1xx_dev->irq_gpio); >>>> + if (sr1xx_dev->spi->irq < 0) { >>>> + dev_err(&spi->dev, "gpio_to_irq request failed gpio = 0x%x\n", >>>> + sr1xx_dev->irq_gpio); >>>> + goto err_exit; >>>> + } >>> Instead of gpio_to_irq(), the DT binding should probably >>> list the interrupt directly using the "interrupts" property >>> pointing to the gpio controller. Since, we are configured this as generic GPIO in DT binding. So, we >> are using gpio_to_irq() to use as IRQ pin. > I meant you should change the binding first, and then adapt the > code to match. Remove all references to gpio numbers from > the code. Will work on this suggestion and update in the next patch submission. > Arnd
| |