Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[caesar <caesar.wang@rock-chips.com>]

[-- Attachment #1: Type: text/plain, Size: 7415 bytes --]

Hi Doug,

在 2014年07月27日 12:59, Doug Anderson 写道:
> caesar,
>
> On Fri, Jul 25, 2014 at 3:29 AM, caesar <caesar.wang@rock-chips.com> wrote:
>> Hi Thierry,
>>
>>
>>
>>
>> 在 2014年07月21日 21:27, Thierry Reding 写道:
>>> On Mon, Jul 21, 2014 at 08:58:42PM +0800, caesar wrote:
>>>
>>>> 于 2014年07月21日 16:50, Thierry Reding 写道:
>>>>> On Sat, Jul 19, 2014 at 08:55:29PM +0800, Caesar Wang wrote:
>>> [...]
>>>>>>          struct rockchip_pwm_chip *pc;
>>>>>>          struct resource *r;
>>>>>>          int ret;
>>>>>> @@ -119,7 +182,10 @@ static int rockchip_pwm_probe(struct
>>>>>> platform_device *pdev)
>>>>>>                  return -ENOMEM;
>>>>>>          r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>> -       pc->base = devm_ioremap_resource(&pdev->dev, r);
>>>>>> +       if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
>>>>>> +               pc->base = devm_ioremap(&pdev->dev, r->start,
>>>>>> resource_size(r));
>>>>>> +       else
>>>>>> +               pc->base = devm_ioremap_resource(&pdev->dev, r);
>>>>> Sorry, this still isn't an option. You really shouldn't remap I/O
>>>>> regions that other drivers may be using. You hinted at a shared register
>>>>> space during the review of the initial version. Can you provide more
>>>>> detail about what exactly the memory map looks like of the rk3288? Is
>>>>> there some kind of technical reference manual that I could look at? Or
>>>>> do you have a device tree extract that shows what the memory map looks
>>>>> like?
>>>>>
>>>>> Thierry
>>>> Maybe,you can look at the ARM: dts: rk3288:
>>>>
>>>> https://github.com/rkchrome/kernel/blob/master/arch/arm/boot/dts/rk3288.dtsi
>>>> There is some lcdc and vop-pwm map address for rk3288.
>>>>
>>>> ,and you can look at the vop-introduce.pdf and vop-register.pdf in Annex.
>>>>
>>>> Maybe,I should put the vop-pwm in lcdc driver,but I don't hope do so it.
>>>>
>>>> Could you give a suggestion to solve it? Thanks.
>>> It looks like you could turn the lcdc device into an MFD device so that
>>> it can instantiate two devices, one for the display controller, the
>>> other for the PWM. Or perhaps it would even work with only a single
>>> child device.
>>>
>>> The device tree would become something like this:
>>>
>>>          lcdc@ff930000 {
>>>                  compatible = "rockchip,rk3288-lcdc";
>>>                  ...
>>>
>>>                  pwm@ff9301a0 {
>>>                          compatible = "rockchip,vop-pwm";
>>>                          ...
>>>                  };
>>>          };
>>>
>>> And your driver would do something like:
>>>
>>>          static const struct resource pwm_resources[] = {
>>>                  {
>>>                          .start = 0x1a0,
>>>                          .end = 0x1af,
>>>                          .flags = IORESOURCE_MEM,
>>>                  },
>>>          };
>>>
>>>          static const struct mfd_cell subdevices[] = {
>>>                  {
>>>                          .name = "pwm",
>>>                          .id = 1,
>>>                          .of_compatible = "rockchip,vop-pwm",
>>>                          .num_resources = ARRAY_SIZE(pwm_resources),
>>>                          .resources = pwm_resources,
>>>                  },
>>>          };
>>>
>>>          static int lcdc_probe(struct platform_device *pdev)
>>>          {
>>>                  struct resource *regs;
>>>                  ...
>>>
>>>                  regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>
>>>                  ...
>>>
>>>                  err = mfd_add_devices(&pdev->dev, 0, subdevices,
>>> ARRAY_SIZE(subdevices),
>>>                                        regs, NULL, NULL);
>>>                  ...
>>>          }
>>>
>>> Thierry
>> Sorry,I might a little trouble for the changes.
>>
>> The driver changes only for lcdc? If that is the case,I suddenly don't know
>> how to do it ?
>>
>> Maybe,I didn't say it clearly.
>>
>> lcdc0: lcdc@ff930000                   |     vop0pwm: pwm@ff9301a0
>> reg = <0xff930000 0x10000>     |      reg = <0xff9301a0 0x10>;
>>
>> The lcdc has to add resource's address  from  0xff930000 to 0xff93ffff.
>>
>> When the  pwm driver is loading vop0pwm. the "devm_ioremap_resource()" will
>> be used in probe();
>>
>> I think it will be occur a fail. because the resource [mem
>> 0xff9301a0-0xff9301af] has be requested by lcdc.
>> =>rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
>> 0xff9301a0-0xff9301af]
>>
>> If I do the changes in pwm driver,do you have a other suggestion for it?
>> thanks.:-)
> Sorry if this is stupid (and I haven't tried it), but does "ranges"
> help solve this problem?  AKA:
>
>           lcdc@ff930000 {
>                   compatible = "rockchip,rk3288-lcdc";
>                   reg = <0xff930000 0x10000>;
>                   #address-cells = <2>;
>                   #size-cells = <1>;
>                   ranges = <0 0xff9301a0 0x10>;
>                   ...
>
>                   pwm@0,0 {
>                           compatible = "rockchip,vop-pwm";
>                           reg = <0 0 0x10>;
>                          ...
>                   };
>           };
>
> Does that avoid the failure?  The lcdc driver would need to call
> of_platform_populate() to make the PWM show up.
>
>
Sorry  if  I  got it wrong following  your way.

example: ARM: DTS:

lcdc@ff930000 {
                  compatible = "rockchip,rk3288-lcdc";
                  reg = <0xff930000 0x10000>;
                  #address-cells = <2>;
                  #size-cells = <1>;
                  ranges = <0 0xff9301a0 0x10>;
                  ...

                  pwm@0,0 {
                          compatible = "rockchip,vop-pwm";
                          reg = <0 0 0x10>;
                         ...
                  };
          };


*The LCDC drive*r ,as follws:

static const struct of_device_id vop_pwm_dt_ids[] = {

     {.compatible = "rockchip,vop-pwm",},

     {}

};

static int lcdc_probe(struct platform_device *pdev)
         {
                 struct resource *regs;
		int ret = 0;
		...
		
		/*This step will get resource=[mem 0xff930000-0yff93ffff]*/

                 regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);

                 ...

                 *lcdc_dev->regs = devm_ioremap_resource(dev, regs);*
	
		...

		/*This step will make enable vop-pwm in PWM driver?*/

		ret = of_platform_populate(np, vop_pwm_dt_ids, NULL, &pdev->dev);
		
		...
	 }


*The PWM driver:*

static const struct of_device_id rockchip_pwm_dt_ids[] = {

     { .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1},

     { .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2},

     { .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop},

     { /* sentinel */ }

};


static int rockchip_pwm_probe(struct platform_device *pdev)
         {
                 struct resource *regs;
		int ret = 0;
		...
		
		/*This step ,if it's "rockchip,vop-pwm",the resource=[mem 0xff9301a0-0yff9301af]*/

                 regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);

                 ...
		
		/*I think will be show the log:->
		 * rockchip-pwm ff9301a0.pwm: can't request region for resource [mem 0xff9301a0-0xff9301af] ?
		*/


                 *lcdc_dev->regs = devm_ioremap_resource(dev, regs);*
	
		...

		

		ret = of_platform_populate(np, vop_pwm_dt_ids, NULL, &pdev->dev);
		
		...
	 }





[-- Attachment #2: Type: text/html, Size: 9229 bytes --]