Re: [PATCH v2 1/2] staging: octeon: ethernet-spi: replace pr_err with dev_err

[Ayush Mukkanwar]

Hi Greg,

I'm preparing v4 of the octeon pr_*() conversion series, broken into
smaller patches as you requested.

While converting ethernet-spi.c, I hit a complication with the IRQ
handler. The SPI code registers a single shared IRQ across all SPI
ports:
if (number_spi_ports == 0)
    request_irq(OCTEON_IRQ_RML, handler, IRQF_SHARED, "SPI", dev_id);
number_spi_ports++;

The original code used &number_spi_ports (a static) as dev_id, so
request_irq and free_irq always matched. But to convert the handler's
pr_err() calls to netdev_err(), I need to pass a net_device * as
dev_id.

The problem is: the first SPI port registers the IRQ, but the last
port to uninit frees it with a different net_device *, causing a
dev_id mismatch.

Additionally, all the error messages in the handler are SPI4 bus-level
errors (DIP4, calendar parity, FIFO overflow, etc.), not specific to
any individual port. Would using dev_err() with the platform device be
acceptable here since the IRQ is registered once for the whole SPI
subsystem? Or would you prefer a different approach?

Thanks, Ayush

On Wed, Apr 1, 2026 at 3:39 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Mar 31, 2026 at 04:47:56PM +0530, AyushMukkanwar wrote:
> > Replace pr_err() calls with dev_err() to include device information
> > in log messages. The device pointer is passed through the interrupt
> > handler via dev_id, which is changed from &number_spi_ports to
> > dev->dev.parent in request_irq and free_irq.
> >
> > Signed-off-by: AyushMukkanwar <ayushmukkanwar@xxxxxxxxx>
> > ---
> >  drivers/staging/octeon/ethernet-spi.c | 59 ++++++++++++++-------------
> >  1 file changed, 30 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/staging/octeon/ethernet-spi.c b/drivers/staging/octeon/ethernet-spi.c
> > index 699c98c5ec13..8c02920c3cdc 100644
> > --- a/drivers/staging/octeon/ethernet-spi.c
> > +++ b/drivers/staging/octeon/ethernet-spi.c
> > @@ -17,67 +17,67 @@
> >  static int number_spi_ports;
> >  static int need_retrain[2] = { 0, 0 };
> >
> > -static void cvm_oct_spxx_int_pr(union cvmx_spxx_int_reg spx_int_reg, int index)
> > +static void cvm_oct_spxx_int_pr(union cvmx_spxx_int_reg spx_int_reg, int index, struct device *dev)
>
> This is ok, but this, but usually the pointer is the first argument.
> And shouldn't this be a netdev (see below...)
>
> > @@ -107,14 +107,15 @@ static irqreturn_t cvm_oct_spi_rml_interrupt(int cpl, void *dev_id)
> >  {
> >       irqreturn_t return_status = IRQ_NONE;
> >       union cvmx_npi_rsl_int_blocks rsl_int_blocks;
> > +     struct device *dev = dev_id;
>
> This isn't ok, the function prototype should really be a pointer, not a
> void thing.
>
> > @@ -196,7 +197,7 @@ int cvm_oct_spi_init(struct net_device *dev)
> >
> >       if (number_spi_ports == 0) {
> >               r = request_irq(OCTEON_IRQ_RML, cvm_oct_spi_rml_interrupt,
> > -                             IRQF_SHARED, "SPI", &number_spi_ports);
> > +                             IRQF_SHARED, "SPI", dev->dev.parent);
>
> Wait, no, you can't do anything with the parent!
>
> This is a netdev, keep it a netdev.  Don't pass it "up" the device
> heirchary to the device pointer, use the real netdev pointer instead.
>
> Also, it seems you didn't test build your changes, what happened?
>
> thanks,
>
> greg k-h