| Message ID | 1371416058-22047-2-git-send-email-tomasz.figa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jun 16, 2013 at 10:54 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Instead of defining new bool field in vendor_data struct for each quirk, > it is more reasonable to use a single flags field and make each quirk > use single bits. > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> Not that I see the point, but I have no strong preference so: Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Jun 16, 2013 at 10:54:08PM +0200, Tomasz Figa wrote: > Instead of defining new bool field in vendor_data struct for each quirk, > it is more reasonable to use a single flags field and make each quirk > use single bits. Please explain why this is better over the existing system, and why it is not just churn for code modification's sake.
On Monday 17 of June 2013 19:48:56 Russell King - ARM Linux wrote: > On Sun, Jun 16, 2013 at 10:54:08PM +0200, Tomasz Figa wrote: > > Instead of defining new bool field in vendor_data struct for each > > quirk, it is more reasonable to use a single flags field and make > > each quirk use single bits. > > Please explain why this is better over the existing system, and why it > is not just churn for code modification's sake. It isn't anything important. I just thought that this is a better solution to store more than just two flags. Initially I had more of them added in further patches, but final version ended with just one, so this can be dropped. Best regards, Tomasz
On Mon, Jun 17, 2013 at 8:56 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On Monday 17 of June 2013 19:48:56 Russell King - ARM Linux wrote: >> On Sun, Jun 16, 2013 at 10:54:08PM +0200, Tomasz Figa wrote: >> > Instead of defining new bool field in vendor_data struct for each >> > quirk, it is more reasonable to use a single flags field and make >> > each quirk use single bits. >> >> Please explain why this is better over the existing system, and why it >> is not just churn for code modification's sake. > > It isn't anything important. I just thought that this is a better solution > to store more than just two flags. Initially I had more of them added in > further patches, but final version ended with just one, so this can be > dropped. I thought about it yesterday and it basically looks to me that it tries to outsmart the compiler on how to stuff the flags to save struct space on the heap and hint that they can be determined quickly by a & operation, this is like premature optimization. So please go back to the old system. Yours, Linus Walleij
On Tuesday 18 June 2013, Linus Walleij wrote: > I thought about it yesterday and it basically looks to me that > it tries to outsmart the compiler on how to stuff the flags to save > struct space on the heap and hint that they can be determined > quickly by a & operation, this is like premature optimization. > > So please go back to the old system. Note that a 'bool' is typically stored as a 32-bit field, so there would be some space saving if there are a lot of flags. I agree with the conclusion though, the previous state was more readable, which outweighs any benefits. Arnd
diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 8bad254..d443a68 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -93,18 +93,22 @@ static struct amba_driver pl08x_amba_driver; struct pl08x_driver_data; +/** Controller supports dual AHB masters. */ +#define PL08X_IS_DUALMASTER (1 << 0) +/** + * Controller has Nomadik security extension bits that need to be checked + * for permission before use and some registers are missing. + */ +#define PL08X_IS_NOMADIK (1 << 1) + /** * struct vendor_data - vendor-specific config parameters for PL08x derivatives * @channels: the number of channels available in this variant - * @dualmaster: whether this version supports dual AHB masters or not. - * @nomadik: whether the channels have Nomadik security extension bits - * that need to be checked for permission before use and some registers are - * missing + * @flags: Vendor-specific flags, see PL08X_IS_* */ struct vendor_data { u8 channels; - bool dualmaster; - bool nomadik; + u32 flags; }; /* @@ -1391,7 +1395,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy( /* Both to be incremented or the code will break */ txd->cctl |= PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR; - if (pl08x->vd->dualmaster) + if (pl08x->vd->flags & PL08X_IS_DUALMASTER) txd->cctl |= pl08x_select_bus(pl08x->mem_buses, pl08x->mem_buses); @@ -1612,7 +1616,7 @@ bool pl08x_filter_id(struct dma_chan *chan, void *chan_id) static void pl08x_ensure_on(struct pl08x_driver_data *pl08x) { /* The Nomadik variant does not have the config register */ - if (pl08x->vd->nomadik) + if (pl08x->vd->flags & PL08X_IS_NOMADIK) return; writel(PL080_CONFIG_ENABLE, pl08x->base + PL080_CONFIG); } @@ -1897,7 +1901,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id) /* By default, AHB1 only. If dualmaster, from platform */ pl08x->lli_buses = PL08X_AHB1; pl08x->mem_buses = PL08X_AHB1; - if (pl08x->vd->dualmaster) { + if (pl08x->vd->flags & PL08X_IS_DUALMASTER) { pl08x->lli_buses = pl08x->pd->lli_buses; pl08x->mem_buses = pl08x->pd->mem_buses; } @@ -1954,7 +1958,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id) * down for the secure world only. Lock up these channels * by perpetually serving a dummy virtual channel. */ - if (vd->nomadik) { + if (vd->flags & PL08X_IS_NOMADIK) { u32 val; val = readl(ch->base + PL080_CH_CONFIG); @@ -2039,18 +2043,17 @@ out_no_pl08x: /* PL080 has 8 channels and the PL080 have just 2 */ static struct vendor_data vendor_pl080 = { .channels = 8, - .dualmaster = true, + .flags = PL08X_IS_DUALMASTER, }; static struct vendor_data vendor_nomadik = { .channels = 8, - .dualmaster = true, - .nomadik = true, + .flags = PL08X_IS_DUALMASTER | PL08X_IS_NOMADIK, }; static struct vendor_data vendor_pl081 = { .channels = 2, - .dualmaster = false, + .flags = 0, }; static struct amba_id pl08x_ids[] = {
Instead of defining new bool field in vendor_data struct for each quirk, it is more reasonable to use a single flags field and make each quirk use single bits. Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> --- drivers/dma/amba-pl08x.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)