Hi Andrew, > -----Original Message----- > From: abrestic@xxxxxxxxxx [mailto:abrestic@xxxxxxxxxx] On Behalf Of > Andrew Bresticker > Sent: 10 March 2015 18:02 > To: James Hartley > Cc: linux-crypto@xxxxxxxxxxxxxxx > Subject: Re: [PATCH V3 1/2] crypto: Add Imagination Technologies hw hash > accelerator > > Hi James, > > >> > +static irqreturn_t img_irq_handler(int irq, void *dev_id) { > >> > + struct img_hash_dev *hdev = dev_id; > >> > + u32 reg; > >> > + > >> > + reg = img_hash_read(hdev, CR_INTSTAT); > >> > + img_hash_write(hdev, CR_INTCLEAR, reg); > >> > + > >> > + if (reg & CR_INT_NEW_RESULTS_SET) { > >> > + dev_dbg(hdev->dev, "IRQ CR_INT_NEW_RESULTS_SET\n"); > >> > + if (DRIVER_FLAGS_BUSY & hdev->flags) { > >> > + hdev->flags |= DRIVER_FLAGS_OUTPUT_READY; > >> > + if (!(DRIVER_FLAGS_CPU & hdev->flags)) > >> > + hdev->flags |= DRIVER_FLAGS_DMA_READY; > >> > + tasklet_schedule(&hdev->done_task); > >> > >> Since this done_task only gets scheduled from here, why not make this > >> a threaded IRQ handler and just do the work here instead of > >> separating it between a hard IRQ handler and a tasklet? > > > > What is the advantage of doing that? i.e is this simple case worth setting up > an extra thread? > > I believe threaded IRQ handlers are now preferred to using a hard IRQ > handler + tasklet when possible, partly because people tend to screw up > tasklet usage. Fair enough, but I'd like to leave this as an enhancement for when I have some extra bandwidth if it's not essential (same for the runtime PM). > > >> > + err = PTR_ERR(hdev->io_base); > >> > + goto hash_io_err; > >> > + } > >> > + > >> > + /* Write port (DMA or CPU) */ > >> > + hash_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > >> > + if (!hash_res) { > >> > + dev_err(dev, "no write port resource info\n"); > >> > + err = -ENODEV; > >> > + goto res_err; > >> > + } > >> > + hdev->bus_addr = hash_res->start; > >> > >> Maybe set this after devm_ioremap_resource() succeeds and avoid the > >> extra error check? > > > > Not quite following you here - which check are you saying can be removed? > > You can remove the if (hash_res) check if you set hdev->bus_addr after > devm_ioremap_resource(). I see what you mean - done. > > Thanks, > Andrew Thanks, James. ��.n��������+%������w��{.n�����{���{ay� ʇڙ���f���h������_�(�階�ݢj"��������G����?���&��
