On Tuesday, December 10, 2013 at 02:37:27 PM, Shawn Guo wrote:
> On Tue, Dec 10, 2013 at 12:47:38PM +0100, Sascha Hauer wrote:
> > > @@ -34,10 +34,21 @@ enum {
> > >
> > > HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> > >
> > > };
> > >
> > > +enum ahci_imx_type {
> > > + AHCI_IMX53,
> > > + AHCI_IMX6Q,
> > > +};
> >
> > Please next time introduce a SoC specific struct to encode the
> > differences between SoCs. This way you can abstract away the differences
> > in flags and function callbacks and don't end up with functions which
> > do completely different things for different SoCs like currently in
> > imx_sata_clock_enable(). The if (type == SOC_XY) style doesn't scale in
> > many drivers anymore.
>
> Yea, I was thinking about the same thing when reviewing the patch in the
> first time, but decided to not comment on that because currently the
> added functions are doing the similar/related thing. But I agree that
> SoC specific structure should be some thing more scalable and we should
> go for in the future.
Full ACK on this one, yes. Thus far, the chips are similar so that the ID is
enough, once we get some more exotic chip, this should be changed to a
structure. I guess the IOMUX register layout might be different on the MX7 at
least for example ?
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
