[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <ab5721fb-bb58-47f7-863f-a6c0ba3c5280@app.fastmail.com>
Date: Fri, 07 Oct 2022 16:11:59 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Manjunatha Venkatesh" <manjunatha.venkatesh@....com>,
linux-kernel@...r.kernel.org,
"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
"Will Deacon" <will@...nel.org>, "Jens Axboe" <axboe@...nel.dk>,
"Rob Herring" <robh+dt@...nel.org>
Cc: mb@...htnvm.io, ckeepax@...nsource.cirrus.com, arnd@...db.d,
"Michael S. Tsirkin" <mst@...hat.com>, javier@...igon.com,
"Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
"Jason Wang" <jasowang@...hat.com>, sunilmut@...rosoft.com,
bjorn.andersson@...aro.org, krzysztof.kozlowski+dt@...aro.org,
devicetree@...r.kernel.org, ashish.deshpande@....com,
rvmanjumce@...il.com
Subject: Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx
series chip
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.
>>> + 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.
Arnd
Powered by blists - more mailing lists