Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
Hi, On Tue, Jun 30, 2020 at 7:54 PM Vinod Koul wrote: > > On 30-06-20, 15:17, Amit Tomer wrote: > > Hi Vinod, > > > > On Mon, Jun 29, 2020 at 3:22 PM Vinod Koul wrote: > > > > > If you use of_device_get_match_data() you will not fall into this :) > > > > But again, of_device_get_match_data() returns void *, and we need > > "uintptr_t" in order to type cast it properly (at-least without > > warning). > > Not really, you can cast from void * to you own structure.. btw why do > you need uintptr_t? uintptr_t allows us to cast to an integer type that matches with enum in terms of size, and clang is happy about it (no more such warnings). > The problem is a pointer to enum conversion :) I think the right way > would be to do would be below > > soc_type = (enum foo)of_device_get_match_data(dev); > > or > soc_type = (unsigned long) of_device_get_match_data(dev); > > which I think should be fine in gcc, but possibly give you above warning Yeah, GCC is always fine with it. > in clang.. but i thought that was fixed in clang > https://reviews.llvm.org/D75758 Thanks for pointing this out. To be honest, I thought clang had brought something important which is missed by GCC (via emitting this warning) that needed to be fixed in Kernel code. But looking at this commit[1], feeling that CLANG people just wanted to be compatible with GCC, and in that situation why should one believe the clang ? [1]: https://github.com/ClangBuiltLinux/llvm-project/commit/4fd4438882cc7f78e56e147d52d9a1f63b58ba81 Thanks -Amit
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
On 30-06-20, 15:17, Amit Tomer wrote: > Hi Vinod, > > On Mon, Jun 29, 2020 at 3:22 PM Vinod Koul wrote: > > > If you use of_device_get_match_data() you will not fall into this :) > > But again, of_device_get_match_data() returns void *, and we need > "uintptr_t" in order to type cast it properly (at-least without > warning). Not really, you can cast from void * to you own structure.. btw why do you need uintptr_t? > > Also, while looking around found the similar warning for other file > where it uses " of_device_get_match_data()" > drivers/pci/controller/pcie-iproc-platform.c:56:15: warning: cast to > smaller integer type 'enum iproc_pcie_type' from 'const void *' > [-Wvoid-pointer-to-enum-cast] > pcie->type = (enum iproc_pcie_type) of_device_get_match_data(dev); The problem is a pointer to enum conversion :) I think the right way would be to do would be below soc_type = (enum foo)of_device_get_match_data(dev); or soc_type = (unsigned long) of_device_get_match_data(dev); which I think should be fine in gcc, but possibly give you above warning in clang.. but i thought that was fixed in clang https://reviews.llvm.org/D75758 Thanks -- ~Vinod
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
Hi Vinod, On Mon, Jun 29, 2020 at 3:22 PM Vinod Koul wrote: > If you use of_device_get_match_data() you will not fall into this :) But again, of_device_get_match_data() returns void *, and we need "uintptr_t" in order to type cast it properly (at-least without warning). Also, while looking around found the similar warning for other file where it uses " of_device_get_match_data()" drivers/pci/controller/pcie-iproc-platform.c:56:15: warning: cast to smaller integer type 'enum iproc_pcie_type' from 'const void *' [-Wvoid-pointer-to-enum-cast] pcie->type = (enum iproc_pcie_type) of_device_get_match_data(dev); Thanks -Amit
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
On 29-06-20, 13:49, Amit Tomer wrote:
> Hi Vinod,
>
> Thanks for having a look and providing the comments.
>
> > Is the .compatible documented, Documentation patch should come before
> > the driver use patch in a series
>
> Yes, this new compatible string is documented in patch (05/10).
> I would make it as a patch (1/10).
>
> > > static int owl_dma_probe(struct platform_device *pdev)
> > > {
> > > struct device_node *np = pdev->dev.of_node;
> > > struct owl_dma *od;
> > > int ret, i, nr_channels, nr_requests;
> > > + const struct of_device_id *of_id =
> > > + of_match_device(owl_dma_match, &pdev->dev);
> >
> > You care about driver_data rather than of_id, so using
> > of_device_get_match_data() would be better..
>
> Okay. would take care of it in next version.
>
> > > od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
> > > if (!od)
> > > @@ -1083,6 +1116,8 @@ static int owl_dma_probe(struct platform_device
> > > *pdev)
> > > dev_info(&pdev->dev, "dma-channels %d, dma-requests %d\n",
> > >nr_channels, nr_requests);
> > >
> > > + od->devid = (enum owl_dma_id)(uintptr_t)of_id->data;
> >
> > Funny casts, I dont think you need uintptr_t!
>
> But without this cast, clang compiler emits following warning:
>
> warning: cast to smaller integer type 'enum owl_dma_id' from 'const void *'
> [-Wvoid-pointer-to-enum-cast]
If you use of_device_get_match_data() you will not fall into this :)
--
~Vinod
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
On 29-06-20, 12:19, André Przywara wrote: > On 29/06/2020 10:54, Vinod Koul wrote: > >> What newer SoCs? I don't think we should try to guess the future here. > > > > In a patch for adding new SoC, quite ironical I would say! > > S700 is not a new SoC, it's just this driver didn't support it yet. What > I meant is that I don't even know about the existence of upcoming SoCs > (Google seems clueless), not to speak of documentation to assess which > DMA controller they use. > > >> We can always introduce further abstractions later, once we actually > >> *know* what we are looking at. > > > > Rather if we know we are adding abstractions, why not add in a way that > > makes it scale better rather than rework again > > I appreciate the effort, but this really tapping around in the dark, > since we don't know which direction any new DMA controller is taking. I > might not even be similar. > > >> Besides, I don't understand what you are after. The max frame length is > >> 1MB in both cases, it's just a matter of where to put FCNT_VAL, either > >> in FLEN or in CTRLB. And having an extra flag for that in driver data > >> sounds a bit over the top at the moment. > > > > Maybe, maybe not. I would rather make it support N SoC when adding > > support for second one rather than keep adding everytime a new SoC is > > added... > > Well, what do you suggest, specifically? At the moment we have two > *slightly* different DMA controllers, so we differentiate between the > two based on the model. Do you want to introduce an extra flag like > FRAME_CNT_IN_CTRLB? That seems to be a bit over the top here, since we > don't know if a future DMA controller is still compatible, or introduces > completely new differences. Fair enough, okay let us go with compatible for now -- ~Vinod
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
On 24-06-20, 10:35, André Przywara wrote:
> On 24/06/2020 07:15, Vinod Koul wrote:
> > On 09-06-20, 15:47, Amit Singh Tomar wrote:
> >
> >> @@ -372,6 +383,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan
> >> *vchan,
> >> struct dma_slave_config *sconfig,
> >> bool is_cyclic)
> >> {
> >> + struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
> >>u32 mode, ctrlb;
> >>
> >>mode = OWL_DMA_MODE_PW(0);
> >> @@ -427,14 +439,26 @@ static inline int owl_dma_cfg_lli(struct
> >> owl_dma_vchan *vchan,
> >>lli->hw[OWL_DMADESC_DADDR] = dst;
> >>lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
> >>lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
> >> - /*
> >> - * Word starts from offset 0xC is shared between frame length
> >> - * (max frame length is 1MB) and frame count, where first 20
> >> - * bits are for frame length and rest of 12 bits are for frame
> >> - * count.
> >> - */
> >> - lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
> >> - lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
> >> +
> >> + if (od->devid == S700_DMA) {
> >> + /* Max frame length is 1MB */
> >> + lli->hw[OWL_DMADESC_FLEN] = len;
> >> + /*
> >> + * On S700, word starts from offset 0x1C is shared between
> >> + * frame count and ctrlb, where first 12 bits are for frame
> >> + * count and rest of 20 bits are for ctrlb.
> >> + */
> >> + lli->hw[OWL_DMADESC_CTRLB] = FCNT_VAL | ctrlb;
> >> + } else {
> >> + /*
> >> + * On S900, word starts from offset 0xC is shared between
> >> + * frame length (max frame length is 1MB) and frame count,
> >> + * where first 20 bits are for frame length and rest of
> >> + * 12 bits are for frame count.
> >> + */
> >> + lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
> >> + lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
> >
> > Unfortunately this wont scale, we will keep adding new conditions for
> > newer SoC's! So rather than this why not encode max frame length in
> > driver_data rather than S900_DMA/S700_DMA.. In future one can add values
> > for newer SoC and not code above logic again.
>
> What newer SoCs? I don't think we should try to guess the future here.
In a patch for adding new SoC, quite ironical I would say!
> We can always introduce further abstractions later, once we actually
> *know* what we are looking at.
Rather if we know we are adding abstractions, why not add in a way that
makes it scale better rather than rework again
> Besides, I don't understand what you are after. The max frame length is
> 1MB in both cases, it's just a matter of where to put FCNT_VAL, either
> in FLEN or in CTRLB. And having an extra flag for that in driver data
> sounds a bit over the top at the moment.
Maybe, maybe not. I would rather make it support N SoC when adding
support for second one rather than keep adding everytime a new SoC is
added...
--
~Vinod
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
Hi Vinod,
Thanks for having a look and providing the comments.
> Is the .compatible documented, Documentation patch should come before
> the driver use patch in a series
Yes, this new compatible string is documented in patch (05/10).
I would make it as a patch (1/10).
> > static int owl_dma_probe(struct platform_device *pdev)
> > {
> > struct device_node *np = pdev->dev.of_node;
> > struct owl_dma *od;
> > int ret, i, nr_channels, nr_requests;
> > + const struct of_device_id *of_id =
> > + of_match_device(owl_dma_match, &pdev->dev);
>
> You care about driver_data rather than of_id, so using
> of_device_get_match_data() would be better..
Okay. would take care of it in next version.
> > od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
> > if (!od)
> > @@ -1083,6 +1116,8 @@ static int owl_dma_probe(struct platform_device *pdev)
> > dev_info(&pdev->dev, "dma-channels %d, dma-requests %d\n",
> >nr_channels, nr_requests);
> >
> > + od->devid = (enum owl_dma_id)(uintptr_t)of_id->data;
>
> Funny casts, I dont think you need uintptr_t!
But without this cast, clang compiler emits following warning:
warning: cast to smaller integer type 'enum owl_dma_id' from 'const void *'
[-Wvoid-pointer-to-enum-cast]
Thanks
-Amit
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
Hi,
On Wed, Jun 24, 2020 at 3:06 PM André Przywara wrote:
>
> On 24/06/2020 07:15, Vinod Koul wrote:
>
> Hi,
>
> > On 09-06-20, 15:47, Amit Singh Tomar wrote:
> >
> >> @@ -372,6 +383,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan
> >> *vchan,
> >>struct dma_slave_config *sconfig,
> >>bool is_cyclic)
> >> {
> >> +struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
> >> u32 mode, ctrlb;
> >>
> >> mode = OWL_DMA_MODE_PW(0);
> >> @@ -427,14 +439,26 @@ static inline int owl_dma_cfg_lli(struct
> >> owl_dma_vchan *vchan,
> >> lli->hw[OWL_DMADESC_DADDR] = dst;
> >> lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
> >> lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
> >> -/*
> >> - * Word starts from offset 0xC is shared between frame length
> >> - * (max frame length is 1MB) and frame count, where first 20
> >> - * bits are for frame length and rest of 12 bits are for frame
> >> - * count.
> >> - */
> >> -lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
> >> -lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
> >> +
> >> +if (od->devid == S700_DMA) {
> >> +/* Max frame length is 1MB */
> >> +lli->hw[OWL_DMADESC_FLEN] = len;
> >> +/*
> >> + * On S700, word starts from offset 0x1C is shared between
> >> + * frame count and ctrlb, where first 12 bits are for frame
> >> + * count and rest of 20 bits are for ctrlb.
> >> + */
> >> +lli->hw[OWL_DMADESC_CTRLB] = FCNT_VAL | ctrlb;
> >> +} else {
> >> +/*
> >> + * On S900, word starts from offset 0xC is shared between
> >> + * frame length (max frame length is 1MB) and frame count,
> >> + * where first 20 bits are for frame length and rest of
> >> + * 12 bits are for frame count.
> >> + */
> >> +lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
> >> +lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
> >
> > Unfortunately this wont scale, we will keep adding new conditions for
> > newer SoC's! So rather than this why not encode max frame length in
> > driver_data rather than S900_DMA/S700_DMA.. In future one can add values
> > for newer SoC and not code above logic again.
>
> What newer SoCs? I don't think we should try to guess the future here.
> We can always introduce further abstractions later, once we actually
> *know* what we are looking at.
>
Apart from it , we have *one* more SoC from Actions .i.e. S500 where
the DMA controller is
identical to S900, and requires *no* additional code to work properly.
So, I think we are safe to have the changes proposed in this patch.
Thanks
-Amit
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
On 29/06/2020 10:54, Vinod Koul wrote:
Hi Vinod,
> On 24-06-20, 10:35, Andr� Przywara wrote:
>> On 24/06/2020 07:15, Vinod Koul wrote:
>>> On 09-06-20, 15:47, Amit Singh Tomar wrote:
>>>
@@ -372,6 +383,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan
*vchan,
struct dma_slave_config *sconfig,
bool is_cyclic)
{
+ struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
u32 mode, ctrlb;
mode = OWL_DMA_MODE_PW(0);
@@ -427,14 +439,26 @@ static inline int owl_dma_cfg_lli(struct
owl_dma_vchan *vchan,
lli->hw[OWL_DMADESC_DADDR] = dst;
lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
- /*
- * Word starts from offset 0xC is shared between frame length
- * (max frame length is 1MB) and frame count, where first 20
- * bits are for frame length and rest of 12 bits are for frame
- * count.
- */
- lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
- lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
+
+ if (od->devid == S700_DMA) {
+ /* Max frame length is 1MB */
+ lli->hw[OWL_DMADESC_FLEN] = len;
+ /*
+ * On S700, word starts from offset 0x1C is shared between
+ * frame count and ctrlb, where first 12 bits are for frame
+ * count and rest of 20 bits are for ctrlb.
+ */
+ lli->hw[OWL_DMADESC_CTRLB] = FCNT_VAL | ctrlb;
+ } else {
+ /*
+ * On S900, word starts from offset 0xC is shared between
+ * frame length (max frame length is 1MB) and frame count,
+ * where first 20 bits are for frame length and rest of
+ * 12 bits are for frame count.
+ */
+ lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
+ lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
>>>
>>> Unfortunately this wont scale, we will keep adding new conditions for
>>> newer SoC's! So rather than this why not encode max frame length in
>>> driver_data rather than S900_DMA/S700_DMA.. In future one can add values
>>> for newer SoC and not code above logic again.
>>
>> What newer SoCs? I don't think we should try to guess the future here.
>
> In a patch for adding new SoC, quite ironical I would say!
S700 is not a new SoC, it's just this driver didn't support it yet. What
I meant is that I don't even know about the existence of upcoming SoCs
(Google seems clueless), not to speak of documentation to assess which
DMA controller they use.
>> We can always introduce further abstractions later, once we actually
>> *know* what we are looking at.
>
> Rather if we know we are adding abstractions, why not add in a way that
> makes it scale better rather than rework again
I appreciate the effort, but this really tapping around in the dark,
since we don't know which direction any new DMA controller is taking. I
might not even be similar.
>> Besides, I don't understand what you are after. The max frame length is
>> 1MB in both cases, it's just a matter of where to put FCNT_VAL, either
>> in FLEN or in CTRLB. And having an extra flag for that in driver data
>> sounds a bit over the top at the moment.
>
> Maybe, maybe not. I would rather make it support N SoC when adding
> support for second one rather than keep adding everytime a new SoC is
> added...
Well, what do you suggest, specifically? At the moment we have two
*slightly* different DMA controllers, so we differentiate between the
two based on the model. Do you want to introduce an extra flag like
FRAME_CNT_IN_CTRLB? That seems to be a bit over the top here, since we
don't know if a future DMA controller is still compatible, or introduces
completely new differences.
Cheers,
Andre
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
On 24/06/2020 07:15, Vinod Koul wrote:
Hi,
> On 09-06-20, 15:47, Amit Singh Tomar wrote:
>
>> @@ -372,6 +383,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan
>> *vchan,
>>struct dma_slave_config *sconfig,
>>bool is_cyclic)
>> {
>> +struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
>> u32 mode, ctrlb;
>>
>> mode = OWL_DMA_MODE_PW(0);
>> @@ -427,14 +439,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan
>> *vchan,
>> lli->hw[OWL_DMADESC_DADDR] = dst;
>> lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
>> lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
>> -/*
>> - * Word starts from offset 0xC is shared between frame length
>> - * (max frame length is 1MB) and frame count, where first 20
>> - * bits are for frame length and rest of 12 bits are for frame
>> - * count.
>> - */
>> -lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
>> -lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
>> +
>> +if (od->devid == S700_DMA) {
>> +/* Max frame length is 1MB */
>> +lli->hw[OWL_DMADESC_FLEN] = len;
>> +/*
>> + * On S700, word starts from offset 0x1C is shared between
>> + * frame count and ctrlb, where first 12 bits are for frame
>> + * count and rest of 20 bits are for ctrlb.
>> + */
>> +lli->hw[OWL_DMADESC_CTRLB] = FCNT_VAL | ctrlb;
>> +} else {
>> +/*
>> + * On S900, word starts from offset 0xC is shared between
>> + * frame length (max frame length is 1MB) and frame count,
>> + * where first 20 bits are for frame length and rest of
>> + * 12 bits are for frame count.
>> + */
>> +lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
>> +lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
>
> Unfortunately this wont scale, we will keep adding new conditions for
> newer SoC's! So rather than this why not encode max frame length in
> driver_data rather than S900_DMA/S700_DMA.. In future one can add values
> for newer SoC and not code above logic again.
What newer SoCs? I don't think we should try to guess the future here.
We can always introduce further abstractions later, once we actually
*know* what we are looking at.
Besides, I don't understand what you are after. The max frame length is
1MB in both cases, it's just a matter of where to put FCNT_VAL, either
in FLEN or in CTRLB. And having an extra flag for that in driver data
sounds a bit over the top at the moment.
Cheers,
Andre.
>
>> +static const struct of_device_id owl_dma_match[] = {
>> +{ .compatible = "actions,s900-dma", .data = (void *)S900_DMA,},
>> +{ .compatible = "actions,s700-dma", .data = (void *)S700_DMA,},
>
> Is the .compatible documented, Documentation patch should come before
> the driver use patch in a series
>
>> static int owl_dma_probe(struct platform_device *pdev)
>> {
>> struct device_node *np = pdev->dev.of_node;
>> struct owl_dma *od;
>> int ret, i, nr_channels, nr_requests;
>> +const struct of_device_id *of_id =
>> +of_match_device(owl_dma_match, &pdev->dev);
>
> You care about driver_data rather than of_id, so using
> of_device_get_match_data() would be better..
>
>> od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
>> if (!od)
>> @@ -1083,6 +1116,8 @@ static int owl_dma_probe(struct platform_device *pdev)
>> dev_info(&pdev->dev, "dma-channels %d, dma-requests %d\n",
>> nr_channels, nr_requests);
>>
>> +od->devid = (enum owl_dma_id)(uintptr_t)of_id->data;
>
> Funny casts, I dont think you need uintptr_t!
>
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
Hi Amit,
On 09-06-20, 15:47, Amit Singh Tomar wrote:
> @@ -372,6 +383,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan
> *vchan,
> struct dma_slave_config *sconfig,
> bool is_cyclic)
> {
> + struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
> u32 mode, ctrlb;
>
> mode = OWL_DMA_MODE_PW(0);
> @@ -427,14 +439,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan
> *vchan,
> lli->hw[OWL_DMADESC_DADDR] = dst;
> lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
> lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
> - /*
> - * Word starts from offset 0xC is shared between frame length
> - * (max frame length is 1MB) and frame count, where first 20
> - * bits are for frame length and rest of 12 bits are for frame
> - * count.
> - */
> - lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
> - lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
> +
> + if (od->devid == S700_DMA) {
> + /* Max frame length is 1MB */
> + lli->hw[OWL_DMADESC_FLEN] = len;
> + /*
> + * On S700, word starts from offset 0x1C is shared between
> + * frame count and ctrlb, where first 12 bits are for frame
> + * count and rest of 20 bits are for ctrlb.
> + */
> + lli->hw[OWL_DMADESC_CTRLB] = FCNT_VAL | ctrlb;
> + } else {
> + /*
> + * On S900, word starts from offset 0xC is shared between
> + * frame length (max frame length is 1MB) and frame count,
> + * where first 20 bits are for frame length and rest of
> + * 12 bits are for frame count.
> + */
> + lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
> + lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
Unfortunately this wont scale, we will keep adding new conditions for
newer SoC's! So rather than this why not encode max frame length in
driver_data rather than S900_DMA/S700_DMA.. In future one can add values
for newer SoC and not code above logic again.
> +static const struct of_device_id owl_dma_match[] = {
> + { .compatible = "actions,s900-dma", .data = (void *)S900_DMA,},
> + { .compatible = "actions,s700-dma", .data = (void *)S700_DMA,},
Is the .compatible documented, Documentation patch should come before
the driver use patch in a series
> static int owl_dma_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct owl_dma *od;
> int ret, i, nr_channels, nr_requests;
> + const struct of_device_id *of_id =
> + of_match_device(owl_dma_match, &pdev->dev);
You care about driver_data rather than of_id, so using
of_device_get_match_data() would be better..
> od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
> if (!od)
> @@ -1083,6 +1116,8 @@ static int owl_dma_probe(struct platform_device *pdev)
> dev_info(&pdev->dev, "dma-channels %d, dma-requests %d\n",
>nr_channels, nr_requests);
>
> + od->devid = (enum owl_dma_id)(uintptr_t)of_id->data;
Funny casts, I dont think you need uintptr_t!
--
~Vinod
