On Tue, Aug 27, 2019 at 03:08:16PM +0200, Geert Uytterhoeven wrote:
> Hi Shimoda-san,
>
> On Tue, Aug 27, 2019 at 1:12 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > Since we will have changed memory mapping of the DMAC in the future,
> > this patch uses of_data values instead of a macro to calculate
> > each channel's base offset.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> > --- a/drivers/dma/sh/rcar-dmac.c
> > +++ b/drivers/dma/sh/rcar-dmac.c
> > @@ -208,12 +208,20 @@ struct rcar_dmac {
> >
> > #define to_rcar_dmac(d) container_of(d, struct rcar_dmac, engine)
> >
> > +/*
> > + * struct rcar_dmac_of_data - This driver's OF data
> > + * @chan_offset_base: DMAC channels base offset
> > + * @chan_offset_coefficient: DMAC channels offset coefficient
>
> Perhaps "stride" instead of "coefficient"? Or "step"?
>
> > @@ -1803,10 +1813,15 @@ static int rcar_dmac_probe(struct platform_device *pdev)
> > unsigned int channels_offset = 0;
> > struct dma_device *engine;
> > struct rcar_dmac *dmac;
> > + const struct rcar_dmac_of_data *data;
> > struct resource *mem;
> > unsigned int i;
> > int ret;
> >
> > + data = of_device_get_match_data(&pdev->dev);
> > + if (!data)
> > + return -EINVAL;
>
> This cannot fail, as the driver is DT only, and all entries in the match table
> have a data pointer.
It seems to me that not including this check would make the code both more
fragile and less intuitive for a marginal gain in simplicity.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
