[...]
> > > +   ret = device_probe(bdev);
> > > +   if (ret < 0) {
> > > +           debug("Can't probe\n");
> > > +           /* TODO: undo create */
> > > +
> > > +           device_unbind(bdev);
> > > +
> > > +           return ret;
> > > +   }
> > > +
> >
> > Patches 2-6 seem to do the same thing for different subsystems.  I think
> > creating a function for that would make it easier.
>
> Well, the reason why I put those changes in separate commits is
> - first, different subsystems are owned by different maintainers, and
> - more importantly, different subsystems may have different cleanup
>   processing required.

That also stands if you create a common function doesn't it?
Create a function that fits most, add the calls in separate patches so
subsystems maintainers can have a look.  If one of the implementation
special, it can ignore the wrapper and go do it's own thing.

>   There are always extra setups after blk_create_device(), which should
>   be reverted if device_probe() fails. For instance, sil_unbind_device()
>   and fsl_unbind_device().
>   So I would like to leave subsystem owners responsible for that.

Regards
/Ilias

>
> -Takahiro Akashi
>
>
> > >     return 0;
> > >  }
> > >
> > > diff --git a/drivers/ata/fsl_sata.c b/drivers/ata/fsl_sata.c
> > > index e44db0a37458..346e9298b4c5 100644
> > > --- a/drivers/ata/fsl_sata.c
> > > +++ b/drivers/ata/fsl_sata.c
> > > @@ -982,6 +982,17 @@ static int fsl_ata_probe(struct udevice *dev)
> > >                     failed_number++;
> > >                     continue;
> > >             }
> > > +
> > > +           ret = device_probe(bdev);
> > > +           if (ret < 0) {
> > > +                   debug("Can't probe\n");
> > > +                   ret = fsl_unbind_device(blk);
> >
> > Apart from this exception I guess
> >
> > > +                   if (ret)
> > > +                           return ret;
> > > +
> > > +                   failed_number++;
> > > +                   continue;
> > > +           }
> > >     }
> > >
> > >     if (failed_number == nr_ports)
> > > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > > index 003222d47be6..09b735779ebf 100644
> > > --- a/drivers/ata/sata_mv.c
> > > +++ b/drivers/ata/sata_mv.c
> > > @@ -1099,6 +1099,15 @@ static int sata_mv_probe(struct udevice *dev)
> > >                     continue;
> > >             }
> > >
> > > +           ret = device_probe(bdev);
> > > +           if (ret < 0) {
> > > +                   debug("Can't probe\n");
> > > +                   /* TODO: undo create */
> > > +
> > > +                   device_unbind(bdev);
> > > +                   continue;
> > > +           }
> > > +
> > >             /* If we got here, the current SATA port was probed
> > >              * successfully, so set the probe status to successful.
> > >              */
> > > diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
> > > index dda712f42cb2..295f7ca72303 100644
> > > --- a/drivers/ata/sata_sil.c
> > > +++ b/drivers/ata/sata_sil.c
> > > @@ -864,6 +864,18 @@ static int sil_pci_probe(struct udevice *dev)
> > >                     failed_number++;
> > >                     continue;
> > >             }
> > > +
> > > +           ret = device_probe(bdev);
> > > +           if (ret < 0) {
> > > +                   debug("Can't probe\n");
> > > +                   ret = sil_unbind_device(blk);
> > > +                   device_unbind(bdev);
> > > +                   if (ret)
> > > +                           return ret;
> > > +
> > > +                   failed_number++;
> > > +                   continue;
> > > +           }
> > >     }
> > >
> > >     if (failed_number == sata_info.maxport)
> > > --
> > > 2.33.0
> > >

Reply via email to