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
