Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1454983 > unrolled thread

Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Stefan Wahren <stefan.wahren@i2se.com> - 2016-08-02 18:40 +0200
    Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Ksenija Stanojević <ksenija.stanojevic@gmail.com> - 2016-08-02 19:10 +0200
      Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Ksenija Stanojević <ksenija.stanojevic@gmail.com> - 2016-08-03 18:40 +0200
        Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Fabio Estevam <festevam@gmail.com> - 2016-08-03 18:40 +0200
          Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Ksenija Stanojević <ksenija.stanojevic@gmail.com> - 2016-08-03 18:50 +0200
            Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Marek Vasut <marex@denx.de> - 2016-08-03 21:20 +0200
              Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Ksenija Stanojević <ksenija.stanojevic@gmail.com> - 2016-08-03 21:40 +0200
                Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Marek Vasut <marex@denx.de> - 2016-08-03 21:50 +0200
                  Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Ksenija Stanojević <ksenija.stanojevic@gmail.com> - 2016-08-03 21:50 +0200
                    Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Marek Vasut <marex@denx.de> - 2016-08-03 22:00 +0200
                      Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Ksenija Stanojević <ksenija.stanojevic@gmail.com> - 2016-08-03 22:30 +0200

#1454983 — Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD

Hi,

> Marek Vasut <marex@denx.de> hat am 14. Juli 2016 um 17:38 geschrieben:
> 
> 
> On 07/13/2016 02:49 PM, Lee Jones wrote:
> > On Fri, 01 Jul 2016, Harald Geyer wrote:
> >
> >> Hi Ksenija!
> >>
> >> Ksenija Stanojević writes:
> >>> On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >>>>> +static int mxs_lradc_add_device(struct platform_device *pdev,
> >>>>> +                             struct mxs_lradc *lradc, char *name, int
> >>>>> i)
> >>>>> +{
> >>>>> +     struct mfd_cell *cell;
> >>>>> +
> >>>>> +     cell = &lradc->cells[i];
> >>>>> +     cell->name = name;
> >>>>> +     cell->platform_data = lradc;
> >>>>> +     cell->pdata_size = sizeof(*lradc);
> >>>>> +
> >>>>> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0,
> >>>>> NULL);
> >>>>> +}
> >>>>
> >>>> Please don't roll your own API.
> >>>>
> >>>> Use 'struct mfd_cell' like everyone else does.
> >>>
> >>> It has been suggested in previous reviews to use separate function to
> >>> register mfd device, and to make mfd_cell allocate dynamically because
> >>> struc mxs-lradc is allocated dynamically.
> >>> But I can revrse changes and make mfd_cells allocate staticaly
> >>> wthout separate function.
> >>
> >> I think making mfd_cells members of struct mxs-lradc will address all
> >> review comments.
> >
> > No, please don't do that either.
> >
> It'd be nice if you explained in detail why not. Otherwise this is just 
> empty splat.

since there is no reply, here is my guess:

static const struct mfd_cell mxs_lradc_devs[] = {
	{
		.name = DRIVER_NAME_ADC,
	},
	{
		.name = DRIVER_NAME_TS,
	},
};

But i'm not sure if we need of_compatible defined here. The intension of this
patch series is to keep the DT binding.

@Lee: Could you please give us a feedback?

@Ksenija: Still motivated for next round?

Regards
Stefan

> 
> -- 
> Best regards,
> Marek Vasut

[toc] | [next] | [standalone]

#1455058

Hi,

On Tue, Aug 2, 2016 at 6:35 PM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> Hi,
>
>> Marek Vasut <marex@denx.de> hat am 14. Juli 2016 um 17:38 geschrieben:
>>
>>
>> On 07/13/2016 02:49 PM, Lee Jones wrote:
>> > On Fri, 01 Jul 2016, Harald Geyer wrote:
>> >
>> >> Hi Ksenija!
>> >>
>> >> Ksenija Stanojević writes:
>> >>> On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> >>>>> +static int mxs_lradc_add_device(struct platform_device *pdev,
>> >>>>> +                             struct mxs_lradc *lradc, char *name, int
>> >>>>> i)
>> >>>>> +{
>> >>>>> +     struct mfd_cell *cell;
>> >>>>> +
>> >>>>> +     cell = &lradc->cells[i];
>> >>>>> +     cell->name = name;
>> >>>>> +     cell->platform_data = lradc;
>> >>>>> +     cell->pdata_size = sizeof(*lradc);
>> >>>>> +
>> >>>>> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0,
>> >>>>> NULL);
>> >>>>> +}
>> >>>>
>> >>>> Please don't roll your own API.
>> >>>>
>> >>>> Use 'struct mfd_cell' like everyone else does.
>> >>>
>> >>> It has been suggested in previous reviews to use separate function to
>> >>> register mfd device, and to make mfd_cell allocate dynamically because
>> >>> struc mxs-lradc is allocated dynamically.
>> >>> But I can revrse changes and make mfd_cells allocate staticaly
>> >>> wthout separate function.
>> >>
>> >> I think making mfd_cells members of struct mxs-lradc will address all
>> >> review comments.
>> >
>> > No, please don't do that either.
>> >
>> It'd be nice if you explained in detail why not. Otherwise this is just
>> empty splat.
>
> since there is no reply, here is my guess:

Sorry for the delay, I'm currently working on it. I will post another
version soon.

> static const struct mfd_cell mxs_lradc_devs[] = {
>         {
>                 .name = DRIVER_NAME_ADC,
>         },
>         {
>                 .name = DRIVER_NAME_TS,
>         },
> };
>
> But i'm not sure if we need of_compatible defined here. The intension of this
> patch series is to keep the DT binding.

I think it needs .resources because in next version DEFINE_RES_IRQ_NAMED
will be used.

> @Lee: Could you please give us a feedback?
>
> @Ksenija: Still motivated for next round?
>
> Regards
> Stefan
>
>>
>> --
>> Best regards,
>> Marek Vasut

Regards,
Ksenija

[toc] | [prev] | [next] | [standalone]

#1455886

Hi All,

On Tue, Aug 2, 2016 at 7:08 PM, Ksenija Stanojević
<ksenija.stanojevic@gmail.com> wrote:
> Hi,
>
> On Tue, Aug 2, 2016 at 6:35 PM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> Hi,
>>
>>> Marek Vasut <marex@denx.de> hat am 14. Juli 2016 um 17:38 geschrieben:
>>>
>>>
>>> On 07/13/2016 02:49 PM, Lee Jones wrote:
>>> > On Fri, 01 Jul 2016, Harald Geyer wrote:
>>> >
>>> >> Hi Ksenija!
>>> >>
>>> >> Ksenija Stanojević writes:
>>> >>> On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
>>> >>>>> +static int mxs_lradc_add_device(struct platform_device *pdev,
>>> >>>>> +                             struct mxs_lradc *lradc, char *name, int
>>> >>>>> i)
>>> >>>>> +{
>>> >>>>> +     struct mfd_cell *cell;
>>> >>>>> +
>>> >>>>> +     cell = &lradc->cells[i];
>>> >>>>> +     cell->name = name;
>>> >>>>> +     cell->platform_data = lradc;
>>> >>>>> +     cell->pdata_size = sizeof(*lradc);
>>> >>>>> +
>>> >>>>> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0,
>>> >>>>> NULL);
>>> >>>>> +}
>>> >>>>
>>> >>>> Please don't roll your own API.
>>> >>>>
>>> >>>> Use 'struct mfd_cell' like everyone else does.
>>> >>>
>>> >>> It has been suggested in previous reviews to use separate function to
>>> >>> register mfd device, and to make mfd_cell allocate dynamically because
>>> >>> struc mxs-lradc is allocated dynamically.
>>> >>> But I can revrse changes and make mfd_cells allocate staticaly
>>> >>> wthout separate function.
>>> >>
>>> >> I think making mfd_cells members of struct mxs-lradc will address all
>>> >> review comments.
>>> >
>>> > No, please don't do that either.
>>> >
>>> It'd be nice if you explained in detail why not. Otherwise this is just
>>> empty splat.
>>
>> since there is no reply, here is my guess:
>
> Sorry for the delay, I'm currently working on it. I will post another
> version soon.
>
>> static const struct mfd_cell mxs_lradc_devs[] = {
>>         {
>>                 .name = DRIVER_NAME_ADC,
>>         },
>>         {
>>                 .name = DRIVER_NAME_TS,
>>         },
>> };
>>
>> But i'm not sure if we need of_compatible defined here. The intension of this
>> patch series is to keep the DT binding.
>
> I think it needs .resources because in next version DEFINE_RES_IRQ_NAMED
> will be used.
>
>> @Lee: Could you please give us a feedback?
>>
>> @Ksenija: Still motivated for next round?

Can someone with imx23 board send me /proc/interrupts log, I
need irq numbers...

Thanks,
Ksenija

[toc] | [prev] | [next] | [standalone]

#1455887

Hi Ksenija,

On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
<ksenija.stanojevic@gmail.com> wrote:

> Can someone with imx23 board send me /proc/interrupts log, I
> need irq numbers...

Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
Interrupt Sources)?

[toc] | [prev] | [next] | [standalone]

#1455895

Hi Fabio,

On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Ksenija,
>
> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
> <ksenija.stanojevic@gmail.com> wrote:
>
>> Can someone with imx23 board send me /proc/interrupts log, I
>> need irq numbers...
>
> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
> Interrupt Sources)?

I thought it will be more reliable if I asked someone, since numbers from MX28
manual don't match with /proc/interrupts log.

[toc] | [prev] | [next] | [standalone]

#1455980

On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
> Hi Fabio,

Hi,

> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> Hi Ksenija,
>>
>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>> <ksenija.stanojevic@gmail.com> wrote:
>>
>>> Can someone with imx23 board send me /proc/interrupts log, I
>>> need irq numbers...
>>
>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>> Interrupt Sources)?
> 
> I thought it will be more reliable if I asked someone, since numbers from MX28
> manual don't match with /proc/interrupts log.
> 
What do you mean they don't match ? Please elaborate.

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1455986

On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>> Hi Fabio,
>
> Hi,
>
>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>> Hi Ksenija,
>>>
>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>> <ksenija.stanojevic@gmail.com> wrote:
>>>
>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>> need irq numbers...
>>>
>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>> Interrupt Sources)?
>>
>> I thought it will be more reliable if I asked someone, since numbers from MX28
>> manual don't match with /proc/interrupts log.
>>
> What do you mean they don't match ? Please elaborate.

irqs don't have the same values in Table 5.1 in MX28 manual and in
/proc/interrupts
output.

For example:
touchscreen irq in MX28 manual have these values:
10 (source number) and 0x0028 (vector number)

in /proc/interrupts:
210:          0         -  10 Edge      mxs-lradc-touchscreen

[toc] | [prev] | [next] | [standalone]

#1455998

On 08/03/2016 09:34 PM, Ksenija Stanojević wrote:
> On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
>> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>>> Hi Fabio,
>>
>> Hi,
>>
>>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>>> Hi Ksenija,
>>>>
>>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>>> <ksenija.stanojevic@gmail.com> wrote:
>>>>
>>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>>> need irq numbers...
>>>>
>>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>>> Interrupt Sources)?
>>>
>>> I thought it will be more reliable if I asked someone, since numbers from MX28
>>> manual don't match with /proc/interrupts log.
>>>
>> What do you mean they don't match ? Please elaborate.
> 
> irqs don't have the same values in Table 5.1 in MX28 manual and in
> /proc/interrupts
> output.
> 
> For example:
> touchscreen irq in MX28 manual have these values:
> 10 (source number) and 0x0028 (vector number)
> 
> in /proc/interrupts:
> 210:          0         -  10 Edge      mxs-lradc-touchscreen

I see lradc_touch_irq in the MX28RM is IRQ 10 and it is also 10 in your
/proc/interrupts output . What am I missing ?

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1456001

On Wed, Aug 3, 2016 at 9:39 PM, Marek Vasut <marex@denx.de> wrote:
> On 08/03/2016 09:34 PM, Ksenija Stanojević wrote:
>> On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>>>> Hi Fabio,
>>>
>>> Hi,
>>>
>>>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>>>> Hi Ksenija,
>>>>>
>>>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>>>> <ksenija.stanojevic@gmail.com> wrote:
>>>>>
>>>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>>>> need irq numbers...
>>>>>
>>>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>>>> Interrupt Sources)?
>>>>
>>>> I thought it will be more reliable if I asked someone, since numbers from MX28
>>>> manual don't match with /proc/interrupts log.
>>>>
>>> What do you mean they don't match ? Please elaborate.
>>
>> irqs don't have the same values in Table 5.1 in MX28 manual and in
>> /proc/interrupts
>> output.
>>
>> For example:
>> touchscreen irq in MX28 manual have these values:
>> 10 (source number) and 0x0028 (vector number)
>>
>> in /proc/interrupts:
>> 210:          0         -  10 Edge      mxs-lradc-touchscreen
>
> I see lradc_touch_irq in the MX28RM is IRQ 10 and it is also 10 in your
> /proc/interrupts output . What am I missing ?

I was referring to the number of the first column 210, that's the number that
I need and cannot find in the Table of the manual.

[toc] | [prev] | [next] | [standalone]

#1456003

On 08/03/2016 09:45 PM, Ksenija Stanojević wrote:
> On Wed, Aug 3, 2016 at 9:39 PM, Marek Vasut <marex@denx.de> wrote:
>> On 08/03/2016 09:34 PM, Ksenija Stanojević wrote:
>>> On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>>>>> Hi Fabio,
>>>>
>>>> Hi,
>>>>
>>>>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>>>>> Hi Ksenija,
>>>>>>
>>>>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>>>>> <ksenija.stanojevic@gmail.com> wrote:
>>>>>>
>>>>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>>>>> need irq numbers...
>>>>>>
>>>>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>>>>> Interrupt Sources)?
>>>>>
>>>>> I thought it will be more reliable if I asked someone, since numbers from MX28
>>>>> manual don't match with /proc/interrupts log.
>>>>>
>>>> What do you mean they don't match ? Please elaborate.
>>>
>>> irqs don't have the same values in Table 5.1 in MX28 manual and in
>>> /proc/interrupts
>>> output.
>>>
>>> For example:
>>> touchscreen irq in MX28 manual have these values:
>>> 10 (source number) and 0x0028 (vector number)
>>>
>>> in /proc/interrupts:
>>> 210:          0         -  10 Edge      mxs-lradc-touchscreen
>>
>> I see lradc_touch_irq in the MX28RM is IRQ 10 and it is also 10 in your
>> /proc/interrupts output . What am I missing ?
> 
> I was referring to the number of the first column 210, that's the number that
> I need and cannot find in the Table of the manual.

The first number in /proc/interrupts is the number which Linux maps to
that particular IRQ line, but that number can be anything, it does not
(need to) match the hardware in any way. What do you need it for ?

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1456014

On Wed, Aug 3, 2016 at 9:57 PM, Marek Vasut <marex@denx.de> wrote:
> On 08/03/2016 09:45 PM, Ksenija Stanojević wrote:
>> On Wed, Aug 3, 2016 at 9:39 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 08/03/2016 09:34 PM, Ksenija Stanojević wrote:
>>>> On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
>>>>> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>>>>>> Hi Fabio,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>>>>>> Hi Ksenija,
>>>>>>>
>>>>>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>>>>>> <ksenija.stanojevic@gmail.com> wrote:
>>>>>>>
>>>>>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>>>>>> need irq numbers...
>>>>>>>
>>>>>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>>>>>> Interrupt Sources)?
>>>>>>
>>>>>> I thought it will be more reliable if I asked someone, since numbers from MX28
>>>>>> manual don't match with /proc/interrupts log.
>>>>>>
>>>>> What do you mean they don't match ? Please elaborate.
>>>>
>>>> irqs don't have the same values in Table 5.1 in MX28 manual and in
>>>> /proc/interrupts
>>>> output.
>>>>
>>>> For example:
>>>> touchscreen irq in MX28 manual have these values:
>>>> 10 (source number) and 0x0028 (vector number)
>>>>
>>>> in /proc/interrupts:
>>>> 210:          0         -  10 Edge      mxs-lradc-touchscreen
>>>
>>> I see lradc_touch_irq in the MX28RM is IRQ 10 and it is also 10 in your
>>> /proc/interrupts output . What am I missing ?
>>
>> I was referring to the number of the first column 210, that's the number that
>> I need and cannot find in the Table of the manual.
>
> The first number in /proc/interrupts is the number which Linux maps to
> that particular IRQ line, but that number can be anything, it does not
> (need to) match the hardware in any way. What do you need it for ?

Thanks, I didn't know that. Well I was using the first number to map
irq names to irq number in DEFINE_RES_IRQ_NAMED.
Otherwise I get probe failure:
probe of mxs-lradc-ts failed with error -22

It's because platform_get_irq_byname cannot find irq for the given
irq name.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web