On Tue, Oct 16, 2018 at 02:41:50PM +0300, Dan Carpenter wrote:
> When we add drivers, can we use the new subsystem prefix for the driver?
> In other words:
>
> [PATCH] staging: nrf24: Add new driver for 2.4Ghz radio transceiver
>
Sure.
> This driver seems basically OK to me. I don't think you necessarily
> need to go through staging? Have you tried sending it directly to
> netdev?
>
I have not tried that, mainly as I believe it is not mature enough. I
would give it some time in staging for testing and bugfixing. If you say
that it is not needed and can be done out of staging, I will ask netdev
if it can be accepted there.
> > + //take out size of data
>
> This comment is sort of useless? "take out" is like Chinese food. It
> seems like it should be obvious what the code does. The format is
> wrong. So there are very minor things to be tidied up.
>
Agree, I will do comments cleanup.
> > + ret = kfifo_out(&device->tx_fifo, &size, sizeof(size));
> > + if (ret != sizeof(size)) {
> > + dev_dbg(&device->dev, "get size from fifo failed\n");
> > + mutex_unlock(&device->tx_fifo_mutex);
> > + continue;
> > + }
> > +
> > + //alloc space for data
> > + buf = kzalloc(size, GFP_KERNEL);
> > + if (!buf) {
> > + dev_dbg(&device->dev, "buf alloc failed\n");
> > + mutex_unlock(&device->tx_fifo_mutex);
> > + continue;
> > + }
> > +
> > + //take out size of data
> > + ret = kfifo_out(&device->tx_fifo, buf, size);
> > + if (ret != size) {
> > + dev_dbg(&device->dev, "get buf from fifo failed\n");
> > + mutex_unlock(&device->tx_fifo_mutex);
> > + goto next;
> > + }
> > +
> > + //unlock tx fifo
> > + mutex_unlock(&device->tx_fifo_mutex);
> > +
> > + //enter Standby-I mode
> > + nrf24_ce_lo(device);
> > +
> > + //set TX MODE
> > + ret = nrf24_set_mode(device->spi, NRF24_MODE_TX);
> > + if (ret < 0)
> > + goto next;
> > +
> > + //set PIPE0 address
> > + //this is needed to receive ACK
> > + ret = nrf24_set_address(device->spi,
> > + NRF24_PIPE0,
> > + (u8 *)&p->cfg.address);
> > + if (ret < 0) {
> > + dev_dbg(&device->dev, "set PIPE0 address failed (%d)\n", ret);
> > + goto next;
> > + }
> > +
> > + //set TX address
> > + ret = nrf24_set_address(device->spi,
> > + NRF24_TX,
> > + (u8 *)&p->cfg.address);
> > + if (ret < 0) {
> > + dev_dbg(&device->dev, "set TX address failed (%d)\n", ret);
> > + goto next;
> > + }
> > +
> > + //check if pipe uses static payload length
> > + spl = p->cfg.plw != 0;
> > +
> > + //check if dynamic payload length is enabled
> > + dpl = nrf24_get_dynamic_pl(device->spi);
> > +
> > + if (spl && dpl) {
> > + //disable dynamic payload if pipe
> > + //does not use dynamic payload
> > + //and dynamic paload is enabled
> > + ret = nrf24_disable_dynamic_pl(device->spi);
> > + if (ret < 0)
> > + goto next;
> > + }
> > +
> > + memset(pload, 0, PLOAD_MAX);
> > + memcpy(pload, &size, sizeof(size));
> > +
> > + //calculate payload length
> > + n = spl ? p->cfg.plw : sizeof(size);
> > +
> > + //send size
> > + nrf24_write_tx_pload(device->spi, pload, n);
> > + if (ret < 0) {
> > + dev_dbg(&device->dev, "write TX PLOAD failed (%d)\n", ret);
> > + goto next;
> > + }
> > +
> > + //enter TX MODE and start transmission
> > + nrf24_ce_hi(device);
> > +
> > + //wait for ACK
> > + wait_event_interruptible(device->tx_done_wait_queue,
> > + (device->tx_done ||
> > + kthread_should_stop()));
> > +
> > + if (kthread_should_stop())
> > + goto abort;
> > +
> > + //clear counter
> > + sent = 0;
> > +
> > + while (size > 0) {
> > + n = spl ? p->cfg.plw : min_t(ssize_t, size, PLOAD_MAX);
> > +
> > + dev_dbg(&device->dev, "tx %zd bytes\n", n);
> > +
> > + memset(pload, 0, PLOAD_MAX);
> > + memcpy(pload, buf + sent, n);
> > +
> > + //write PLOAD to nRF FIFO
> > + ret = nrf24_write_tx_pload(device->spi, pload, n);
> > +
> > + if (ret < 0) {
> > + dev_dbg(&device->dev,
> > + "write TX PLOAD failed (%d)\n",
> > + ret);
> > + goto next;
> > + }
> > +
> > + sent += n;
> > + size -= n;
> > +
> > + device->tx_done = false;
> > +
> > + //wait for ACK
> > + wait_event_interruptible(device->tx_done_wait_queue,
> > + (device->tx_done ||
> > + kthread_should_stop()));
> > +
> > + if (kthread_should_stop())
> > + goto abort;
> > + }
> > +next:
> > + //free data buffer
> > + kfree(buf);
> > +
> > + //restore dynamic payload feature
> > + if (dpl)
> > + nrf24_enable_dynamic_pl(device->spi);
> > +
> > + //if all sent enter RX MODE
> > + if (kfifo_is_empty(&device->tx_fifo)) {
> > + dev_dbg(&device->dev, "%s: NRF24_MODE_RX\n", __func__);
> > +
> > + //enter Standby-I
> > + nrf24_ce_lo(device);
> > +
> > + p = nrf24_find_pipe_id(device, NRF24_PIPE0);
> > + if (!IS_ERR(p)) {
> > + //restore PIPE0 address
> > + nrf24_set_address(device->spi,
> > + p->id,
> > + (u8 *)&p->cfg.address);
> > + }
> > + //set RX MODE
> > + nrf24_set_mode(device->spi, NRF24_MODE_RX);
> > +
> > + //enter RX MODE and start receiving
> > + nrf24_ce_hi(device);
> > + }
> > + }
> > +abort:
> > + kfree(buf);
> > +
> > + return 0;
> > +}
> > +
>
> [ snip ]
>
> > +static int nrf24_gpio_setup(struct nrf24_device *device)
> > +{
> > + int ret;
> > +
> > + device->ce = gpiod_get(&device->spi->dev, "ce", 0);
> > +
> > + if (device->ce == ERR_PTR(-ENOENT))
> > + dev_dbg(&device->dev, "%s: no entry for CE\n", __func__);
> > + else if (device->ce == ERR_PTR(-EBUSY))
> > + dev_dbg(&device->dev, "%s: CE is busy\n", __func__);
> > +
> > + if (IS_ERR(device->ce)) {
> > + ret = PTR_ERR(device->ce);
> > + dev_err(&device->dev, "%s: CE gpio setup error\n", __func__);
> > + return ret;
> > + }
> > +
> > + nrf24_ce_lo(device);
> > +
> > + //irq
> > + ret = request_irq(device->spi->irq,
> > + nrf24_isr,
> > + 0,
> > + dev_name(&device->dev),
> > + device);
> > + if (ret < 0) {
> > + free_irq(device->spi->irq, device);
>
> I don't think we need to free this because the requiest failed.
>
True, free_irq is not needed here.
> I'm not a huge fan of your label naming scheme. You're generally using
> come-from names like "alloc_failed:" but that doesn't tell you what the
> goto does so I have to open two windows and manually line up the gotos
> with the labels to see what the goto does. It's better for the name
> to say "err_put_ce".
>
I got your point and it make sens. I will review and try to rename
labels accordingly.
> > + goto err;
> > + }
> > +
> > + return 0;
> > +
> > +err:
> > + gpiod_put(device->ce);
> > + return ret;
> > +}
> > +
> > +static void nrf24_dev_release(struct device *dev)
> > +{
> > + struct nrf24_device *device = to_nrf24_device(dev);
> > +
> > + ida_simple_remove(&nrf24_ida_dev, device->id);
> > + kfree(device);
> > +}
> > +
> > +static struct device_type nrf24_dev_type = {
> > + .name = "nrf24_device",
> > + .release = nrf24_dev_release,
> > +};
> > +
> > +static struct nrf24_device *nrf24_dev_init(struct spi_device *spi)
> > +{
> > + int ret;
> > + struct nrf24_device *device;
> > + int id;
> > +
> > + id = ida_simple_get(&nrf24_ida_dev, 0, 0, GFP_KERNEL);
> > + if (id < 0)
> > + return ERR_PTR(id);
> > +
> > + //sets flags to false as well
> > + device = kzalloc(sizeof(*device), GFP_KERNEL);
> > + if (!device) {
> > + ida_simple_remove(&nrf24_ida_dev, id);
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + device->spi = spi;
> > +
> > + dev_set_name(&device->dev, "nrf%d", id);
> > + device->id = id;
> > + device->dev.parent = &spi->dev;
> > + device->dev.class = nrf24_class;
> > + device->dev.type = &nrf24_dev_type;
> > + device->dev.groups = nrf24_groups;
> > + ret = device_register(&device->dev);
> > + if (ret < 0) {
> > + put_device(&device->dev);
>
> We don't have to do ida_simple_remove()?
>
We do have to do ida_simple_remove. I missed it!
> > + return ERR_PTR(ret);
> > + }
> > +
> > + init_waitqueue_head(&device->tx_wait_queue);
> > + init_waitqueue_head(&device->tx_done_wait_queue);
> > + init_waitqueue_head(&device->rx_wait_queue);
> > +
> > + INIT_WORK(&device->isr_work, nrf24_isr_work_handler);
> > + INIT_KFIFO(device->tx_fifo);
> > + spin_lock_init(&device->lock);
> > + mutex_init(&device->tx_fifo_mutex);
> > +
> > + INIT_LIST_HEAD(&device->pipes);
> > +
> > + return device;
> > +}
>
> [ snip ]
>
> > +static ssize_t address_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct nrf24_device *device = to_nrf24_device(dev->parent);
> > + u8 addr[16];
> > + int ret;
> > + int count;
> > + int i;
> > + struct nrf24_pipe *pipe;
> > +
> > + pipe = nrf24_find_pipe_ptr(dev);
> > + if (IS_ERR(pipe))
> > + return PTR_ERR(pipe);
> > +
> > + ret = nrf24_get_address(device->spi, pipe->id, addr);
> > + if (ret < 0)
> > + return ret;
> > +
> > + count = snprintf(buf, PAGE_SIZE, "0x");
> > + for (i = --ret; i >= 0; i--)
> > + count += snprintf(buf + count, PAGE_SIZE, "%02X", addr[i]);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > + count += snprintf(buf + count, PAGE_SIZE, "\n");
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This isn't right. Use scnprintf(). The snprintf() function returns
> the nubmer of characters that would have been written if we had space.
> And it should be PAGE_SIZE - count.
>
You are right, I will use scnprintf here.
> > +
> > + return count;
> > +}
> > +
>
>
> regards,
> dan carpenter
Thanks for the review, I will send v3 in some time now.
br,
Marcin
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
