[PATCH v2 1/2] mfd: intel_quark_i2c_gpio: Reuse BAR definitions for MFD cell indexing

[PATCH v2 1/2] mfd: intel_quark_i2c_gpio: Reuse BAR definitions for MFD cell indexing

[Andy Shevchenko]

It's convenient and less error prone to use definitions to address
different cells in an array. For this purpose we may reuse existing
BAR definitions.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: used explicit indices for MFD cells in the array (Lee)
 drivers/mfd/intel_quark_i2c_gpio.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index 7af22d1399e1..52728a963c17 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -98,15 +98,7 @@ static struct mfd_cell_acpi_match intel_quark_acpi_match_gpio = {
 };
 
 static struct mfd_cell intel_quark_mfd_cells[] = {
-	{
-		.id = MFD_GPIO_BAR,
-		.name = "gpio-dwapb",
-		.acpi_match = &intel_quark_acpi_match_gpio,
-		.num_resources = ARRAY_SIZE(intel_quark_gpio_res),
-		.resources = intel_quark_gpio_res,
-		.ignore_resource_conflicts = true,
-	},
-	{
+	[MFD_I2C_BAR] = {
 		.id = MFD_I2C_BAR,
 		.name = "i2c_designware",
 		.acpi_match = &intel_quark_acpi_match_i2c,
@@ -114,6 +106,14 @@ static struct mfd_cell intel_quark_mfd_cells[] = {
 		.resources = intel_quark_i2c_res,
 		.ignore_resource_conflicts = true,
 	},
+	[MFD_GPIO_BAR] = {
+		.id = MFD_GPIO_BAR,
+		.name = "gpio-dwapb",
+		.acpi_match = &intel_quark_acpi_match_gpio,
+		.num_resources = ARRAY_SIZE(intel_quark_gpio_res),
+		.resources = intel_quark_gpio_res,
+		.ignore_resource_conflicts = true,
+	},
 };
 
 static const struct pci_device_id intel_quark_mfd_ids[] = {
@@ -245,11 +245,11 @@ static int intel_quark_mfd_probe(struct pci_dev *pdev,
 	if (ret)
 		return ret;
 
-	ret = intel_quark_i2c_setup(pdev, &intel_quark_mfd_cells[1]);
+	ret = intel_quark_i2c_setup(pdev, &intel_quark_mfd_cells[MFD_I2C_BAR]);
 	if (ret)
 		goto err_unregister_i2c_clk;
 
-	ret = intel_quark_gpio_setup(pdev, &intel_quark_mfd_cells[0]);
+	ret = intel_quark_gpio_setup(pdev, &intel_quark_mfd_cells[MFD_GPIO_BAR]);
 	if (ret)
 		goto err_unregister_i2c_clk;
 
-- 
2.30.2

[PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Andy Shevchenko]

Allow interrupts to be MSI if supported by hardware.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 drivers/mfd/intel_quark_i2c_gpio.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index 52728a963c17..16ce9cb3aa2f 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -169,8 +169,8 @@ static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
 	res[INTEL_QUARK_IORES_MEM].end =
 		pci_resource_end(pdev, MFD_I2C_BAR);
 
-	res[INTEL_QUARK_IORES_IRQ].start = pdev->irq;
-	res[INTEL_QUARK_IORES_IRQ].end = pdev->irq;
+	res[INTEL_QUARK_IORES_IRQ].start = pci_irq_vector(pdev, 0);
+	res[INTEL_QUARK_IORES_IRQ].end = pci_irq_vector(pdev, 0);
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
@@ -217,7 +217,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
 	pdata->properties->idx		= 0;
 	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
 	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
-	pdata->properties->irq[0]	= pdev->irq;
+	pdata->properties->irq[0]	= pci_irq_vector(pdev, 0);
 
 	cell->platform_data = pdata;
 	cell->pdata_size = sizeof(*pdata);
@@ -245,22 +245,30 @@ static int intel_quark_mfd_probe(struct pci_dev *pdev,
 	if (ret)
 		return ret;
 
+	pci_set_master(pdev);
+
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		goto err_unregister_i2c_clk;
+
 	ret = intel_quark_i2c_setup(pdev, &intel_quark_mfd_cells[MFD_I2C_BAR]);
 	if (ret)
-		goto err_unregister_i2c_clk;
+		goto err_free_irq_vectors;
 
 	ret = intel_quark_gpio_setup(pdev, &intel_quark_mfd_cells[MFD_GPIO_BAR]);
 	if (ret)
-		goto err_unregister_i2c_clk;
+		goto err_free_irq_vectors;
 
 	ret = mfd_add_devices(&pdev->dev, 0, intel_quark_mfd_cells,
 			      ARRAY_SIZE(intel_quark_mfd_cells), NULL, 0,
 			      NULL);
 	if (ret)
-		goto err_unregister_i2c_clk;
+		goto err_free_irq_vectors;
 
 	return 0;
 
+err_free_irq_vectors:
+	pci_free_irq_vectors(pdev);
 err_unregister_i2c_clk:
 	intel_quark_unregister_i2c_clk(&pdev->dev);
 	return ret;
@@ -269,6 +277,7 @@ static int intel_quark_mfd_probe(struct pci_dev *pdev,
 static void intel_quark_mfd_remove(struct pci_dev *pdev)
 {
 	mfd_remove_devices(&pdev->dev);
+	pci_free_irq_vectors(pdev);
 	intel_quark_unregister_i2c_clk(&pdev->dev);
 }
 
-- 
2.30.2

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Lee Jones]

On Tue, 23 Mar 2021, Andy Shevchenko wrote:

> Allow interrupts to be MSI if supported by hardware.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  drivers/mfd/intel_quark_i2c_gpio.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
> index 52728a963c17..16ce9cb3aa2f 100644
> --- a/drivers/mfd/intel_quark_i2c_gpio.c
> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> @@ -169,8 +169,8 @@ static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
>  	res[INTEL_QUARK_IORES_MEM].end =
>  		pci_resource_end(pdev, MFD_I2C_BAR);
>  
> -	res[INTEL_QUARK_IORES_IRQ].start = pdev->irq;
> -	res[INTEL_QUARK_IORES_IRQ].end = pdev->irq;
> +	res[INTEL_QUARK_IORES_IRQ].start = pci_irq_vector(pdev, 0);
> +	res[INTEL_QUARK_IORES_IRQ].end = pci_irq_vector(pdev, 0);
>  
>  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
> @@ -217,7 +217,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
>  	pdata->properties->idx		= 0;
>  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
>  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
> -	pdata->properties->irq[0]	= pdev->irq;
> +	pdata->properties->irq[0]	= pci_irq_vector(pdev, 0);
>  
>  	cell->platform_data = pdata;
>  	cell->pdata_size = sizeof(*pdata);
> @@ -245,22 +245,30 @@ static int intel_quark_mfd_probe(struct pci_dev *pdev,
>  	if (ret)
>  		return ret;
>  
> +	pci_set_master(pdev);
> +
> +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);

Is there any way these magic number can be defined or sizeof()'ed?

[...]

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 10:29:31AM +0000, Lee Jones wrote:
> On Tue, 23 Mar 2021, Andy Shevchenko wrote:
> 
> > Allow interrupts to be MSI if supported by hardware.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > v2: new patch
> >  drivers/mfd/intel_quark_i2c_gpio.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
> > index 52728a963c17..16ce9cb3aa2f 100644
> > --- a/drivers/mfd/intel_quark_i2c_gpio.c
> > +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> > @@ -169,8 +169,8 @@ static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> >  	res[INTEL_QUARK_IORES_MEM].end =
> >  		pci_resource_end(pdev, MFD_I2C_BAR);
> >  
> > -	res[INTEL_QUARK_IORES_IRQ].start = pdev->irq;
> > -	res[INTEL_QUARK_IORES_IRQ].end = pdev->irq;
> > +	res[INTEL_QUARK_IORES_IRQ].start = pci_irq_vector(pdev, 0);
> > +	res[INTEL_QUARK_IORES_IRQ].end = pci_irq_vector(pdev, 0);
> >  
> >  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >  	if (!pdata)
> > @@ -217,7 +217,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> >  	pdata->properties->idx		= 0;
> >  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
> >  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
> > -	pdata->properties->irq[0]	= pdev->irq;
> > +	pdata->properties->irq[0]	= pci_irq_vector(pdev, 0);
> >  
> >  	cell->platform_data = pdata;
> >  	cell->pdata_size = sizeof(*pdata);
> > @@ -245,22 +245,30 @@ static int intel_quark_mfd_probe(struct pci_dev *pdev,
> >  	if (ret)
> >  		return ret;
> >  
> > +	pci_set_master(pdev);
> > +
> > +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> 
> Is there any way these magic number can be defined or sizeof()'ed?

Grep for it in the kernel, it's rarely defined.

The semantic is min-max range and having two defines (*) here for these seems
to me as an utter overkill.

Of course, if you insist I may do it.

*) since value is the same, we might have one definition, but it will be even
   more confusion to have it as a min and max at the same time.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Lee Jones]

On Wed, 24 Mar 2021, Andy Shevchenko wrote:

> On Wed, Mar 24, 2021 at 10:29:31AM +0000, Lee Jones wrote:
> > On Tue, 23 Mar 2021, Andy Shevchenko wrote:
> > 
> > > Allow interrupts to be MSI if supported by hardware.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > > v2: new patch
> > >  drivers/mfd/intel_quark_i2c_gpio.c | 21 +++++++++++++++------
> > >  1 file changed, 15 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
> > > index 52728a963c17..16ce9cb3aa2f 100644
> > > --- a/drivers/mfd/intel_quark_i2c_gpio.c
> > > +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> > > @@ -169,8 +169,8 @@ static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> > >  	res[INTEL_QUARK_IORES_MEM].end =
> > >  		pci_resource_end(pdev, MFD_I2C_BAR);
> > >  
> > > -	res[INTEL_QUARK_IORES_IRQ].start = pdev->irq;
> > > -	res[INTEL_QUARK_IORES_IRQ].end = pdev->irq;
> > > +	res[INTEL_QUARK_IORES_IRQ].start = pci_irq_vector(pdev, 0);
> > > +	res[INTEL_QUARK_IORES_IRQ].end = pci_irq_vector(pdev, 0);
> > >  
> > >  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > >  	if (!pdata)
> > > @@ -217,7 +217,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> > >  	pdata->properties->idx		= 0;
> > >  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
> > >  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
> > > -	pdata->properties->irq[0]	= pdev->irq;
> > > +	pdata->properties->irq[0]	= pci_irq_vector(pdev, 0);
> > >  
> > >  	cell->platform_data = pdata;
> > >  	cell->pdata_size = sizeof(*pdata);
> > > @@ -245,22 +245,30 @@ static int intel_quark_mfd_probe(struct pci_dev *pdev,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	pci_set_master(pdev);
> > > +
> > > +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> > 
> > Is there any way these magic number can be defined or sizeof()'ed?
> 
> Grep for it in the kernel, it's rarely defined.

I already did.  It is sometimes defined, other times not.

Also, past acceptance does not guarantee ideal/correct usage.

> The semantic is min-max range and having two defines (*) here for these seems
> to me as an utter overkill.
> 
> Of course, if you insist I may do it.
> 
> *) since value is the same, we might have one definition, but it will be even
>    more confusion to have it as a min and max at the same time.

It's just tricky to decypher for people who do not know the API, which
is most people, myself included.  For APIs like usleep_range() et al.,
obviously this makes no sense at all.

What defines a vector?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 10:47:29AM +0000, Lee Jones wrote:
> On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > On Wed, Mar 24, 2021 at 10:29:31AM +0000, Lee Jones wrote:
> > > On Tue, 23 Mar 2021, Andy Shevchenko wrote:

...

> Also, past acceptance does not guarantee ideal/correct usage.

In this case it's hardly can be misused. But I heard you.

...

> > The semantic is min-max range and having two defines (*) here for these seems
> > to me as an utter overkill.
> > 
> > Of course, if you insist I may do it.
> > 
> > *) since value is the same, we might have one definition, but it will be even
> >    more confusion to have it as a min and max at the same time.
> 
> It's just tricky to decypher for people who do not know the API, which
> is most people, myself included.  For APIs like usleep_range() et al.,
> obviously this makes no sense at all.

Seem like you are insisting. Okay, I will define them. What do you prefer one
or two definitions?

...

> What defines a vector?

The combination is solely of the driver-hardware. Driver explicitly tells that
how many vectors it may consume (taking into account the range asked) and API
returns amount given or an error.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Lee Jones]

On Wed, 24 Mar 2021, Andy Shevchenko wrote:

> On Wed, Mar 24, 2021 at 10:47:29AM +0000, Lee Jones wrote:
> > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > > On Wed, Mar 24, 2021 at 10:29:31AM +0000, Lee Jones wrote:
> > > > On Tue, 23 Mar 2021, Andy Shevchenko wrote:
> 
> ...
> 
> > Also, past acceptance does not guarantee ideal/correct usage.
> 
> In this case it's hardly can be misused. But I heard you.
> 
> ...
> 
> > > The semantic is min-max range and having two defines (*) here for these seems
> > > to me as an utter overkill.
> > > 
> > > Of course, if you insist I may do it.
> > > 
> > > *) since value is the same, we might have one definition, but it will be even
> > >    more confusion to have it as a min and max at the same time.
> > 
> > It's just tricky to decypher for people who do not know the API, which
> > is most people, myself included.  For APIs like usleep_range() et al.,
> > obviously this makes no sense at all.
> 
> Seem like you are insisting. Okay, I will define them. What do you prefer one
> or two definitions?

Actually I'm not.  I'm just trying to get my head around where the
data comes from and what the values actually mean.

> ...
> 
> > What defines a vector?
> 
> The combination is solely of the driver-hardware. Driver explicitly tells that
> how many vectors it may consume (taking into account the range asked) and API
> returns amount given or an error.

So, where does the information actually come from?

Information that comes from a datasheet is usually defined.

Information that comes from the F/W is usually read and popped into a
variable.

It's usual for values (other than things like timings) to be issued
'raw' like this.  Particularly as an argument of a bespoke API.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 11:50:33AM +0000, Lee Jones wrote:
> On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> 
> > On Wed, Mar 24, 2021 at 10:47:29AM +0000, Lee Jones wrote:
> > > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > > > On Wed, Mar 24, 2021 at 10:29:31AM +0000, Lee Jones wrote:
> > > > > On Tue, 23 Mar 2021, Andy Shevchenko wrote:
> > 
> > ...
> > 
> > > Also, past acceptance does not guarantee ideal/correct usage.
> > 
> > In this case it's hardly can be misused. But I heard you.
> > 
> > ...
> > 
> > > > The semantic is min-max range and having two defines (*) here for these seems
> > > > to me as an utter overkill.
> > > > 
> > > > Of course, if you insist I may do it.
> > > > 
> > > > *) since value is the same, we might have one definition, but it will be even
> > > >    more confusion to have it as a min and max at the same time.
> > > 
> > > It's just tricky to decypher for people who do not know the API, which
> > > is most people, myself included.  For APIs like usleep_range() et al.,
> > > obviously this makes no sense at all.
> > 
> > Seem like you are insisting. Okay, I will define them. What do you prefer one
> > or two definitions?
> 
> Actually I'm not.  I'm just trying to get my head around where the
> data comes from and what the values actually mean.
> 
> > ...
> > 
> > > What defines a vector?
> > 
> > The combination is solely of the driver-hardware. Driver explicitly tells that
> > how many vectors it may consume (taking into account the range asked) and API
> > returns amount given or an error.
> 
> So, where does the information actually come from?
> 
> Information that comes from a datasheet is usually defined.
> 
> Information that comes from the F/W is usually read and popped into a
> variable.

It's a two way road:
a) driver states that it needs only 1 vector and it's enough to it
b) hardware must provide at least 1 vector to be served by this driver.

Look again into grepped output. Most of drivers that define it as an variable
may dynamically adapt to the different amount of IRQ vectors. When it's static,
usually drivers just hard code those values.

I'm really don't see a point to define them _in this driver_.

> It's usual for values (other than things like timings) to be issued
> 'raw' like this.  Particularly as an argument of a bespoke API.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Lee Jones]

On Wed, 24 Mar 2021, Andy Shevchenko wrote:

> On Wed, Mar 24, 2021 at 11:50:33AM +0000, Lee Jones wrote:
> > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > 
> > > On Wed, Mar 24, 2021 at 10:47:29AM +0000, Lee Jones wrote:
> > > > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > > > > On Wed, Mar 24, 2021 at 10:29:31AM +0000, Lee Jones wrote:
> > > > > > On Tue, 23 Mar 2021, Andy Shevchenko wrote:
> > > 
> > > ...
> > > 
> > > > Also, past acceptance does not guarantee ideal/correct usage.
> > > 
> > > In this case it's hardly can be misused. But I heard you.
> > > 
> > > ...
> > > 
> > > > > The semantic is min-max range and having two defines (*) here for these seems
> > > > > to me as an utter overkill.
> > > > > 
> > > > > Of course, if you insist I may do it.
> > > > > 
> > > > > *) since value is the same, we might have one definition, but it will be even
> > > > >    more confusion to have it as a min and max at the same time.
> > > > 
> > > > It's just tricky to decypher for people who do not know the API, which
> > > > is most people, myself included.  For APIs like usleep_range() et al.,
> > > > obviously this makes no sense at all.
> > > 
> > > Seem like you are insisting. Okay, I will define them. What do you prefer one
> > > or two definitions?
> > 
> > Actually I'm not.  I'm just trying to get my head around where the
> > data comes from and what the values actually mean.
> > 
> > > ...
> > > 
> > > > What defines a vector?
> > > 
> > > The combination is solely of the driver-hardware. Driver explicitly tells that
> > > how many vectors it may consume (taking into account the range asked) and API
> > > returns amount given or an error.
> > 
> > So, where does the information actually come from?
> > 
> > Information that comes from a datasheet is usually defined.
> > 
> > Information that comes from the F/W is usually read and popped into a
> > variable.
> 
> It's a two way road:
> a) driver states that it needs only 1 vector and it's enough to it
> b) hardware must provide at least 1 vector to be served by this driver.
> 
> Look again into grepped output. Most of drivers that define it as an variable
> may dynamically adapt to the different amount of IRQ vectors. When it's static,
> usually drivers just hard code those values.
> 
> I'm really don't see a point to define them _in this driver_.

That's fine.  I just felt like I had to ask.

Would you consider a comment that lets people unfamiliar with the API
what the values mean?

Something to the tune of:

  "This driver requests 1 (and only 1) IRQ vector"

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 01:07:23PM +0000, Lee Jones wrote:
> On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> 
> > On Wed, Mar 24, 2021 at 11:50:33AM +0000, Lee Jones wrote:
> > > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > > 
> > > > On Wed, Mar 24, 2021 at 10:47:29AM +0000, Lee Jones wrote:
> > > > > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > > > > > On Wed, Mar 24, 2021 at 10:29:31AM +0000, Lee Jones wrote:
> > > > > > > On Tue, 23 Mar 2021, Andy Shevchenko wrote:
> > > > 
> > > > ...
> > > > 
> > > > > Also, past acceptance does not guarantee ideal/correct usage.
> > > > 
> > > > In this case it's hardly can be misused. But I heard you.
> > > > 
> > > > ...
> > > > 
> > > > > > The semantic is min-max range and having two defines (*) here for these seems
> > > > > > to me as an utter overkill.
> > > > > > 
> > > > > > Of course, if you insist I may do it.
> > > > > > 
> > > > > > *) since value is the same, we might have one definition, but it will be even
> > > > > >    more confusion to have it as a min and max at the same time.
> > > > > 
> > > > > It's just tricky to decypher for people who do not know the API, which
> > > > > is most people, myself included.  For APIs like usleep_range() et al.,
> > > > > obviously this makes no sense at all.
> > > > 
> > > > Seem like you are insisting. Okay, I will define them. What do you prefer one
> > > > or two definitions?
> > > 
> > > Actually I'm not.  I'm just trying to get my head around where the
> > > data comes from and what the values actually mean.
> > > 
> > > > ...
> > > > 
> > > > > What defines a vector?
> > > > 
> > > > The combination is solely of the driver-hardware. Driver explicitly tells that
> > > > how many vectors it may consume (taking into account the range asked) and API
> > > > returns amount given or an error.
> > > 
> > > So, where does the information actually come from?
> > > 
> > > Information that comes from a datasheet is usually defined.
> > > 
> > > Information that comes from the F/W is usually read and popped into a
> > > variable.
> > 
> > It's a two way road:
> > a) driver states that it needs only 1 vector and it's enough to it
> > b) hardware must provide at least 1 vector to be served by this driver.
> > 
> > Look again into grepped output. Most of drivers that define it as an variable
> > may dynamically adapt to the different amount of IRQ vectors. When it's static,
> > usually drivers just hard code those values.
> > 
> > I'm really don't see a point to define them _in this driver_.
> 
> That's fine.  I just felt like I had to ask.
> 
> Would you consider a comment that lets people unfamiliar with the API
> what the values mean?
> 
> Something to the tune of:
> 
>   "This driver requests 1 (and only 1) IRQ vector"


Rather

    "This driver requests only 1 (and it's enough) IRQ vector"

or something like this.

Should I send a patch with the comment included? If so, please suggest if it's
good from English grammar/style perspective.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Lee Jones]

On Wed, 24 Mar 2021, Andy Shevchenko wrote:

> On Wed, Mar 24, 2021 at 01:07:23PM +0000, Lee Jones wrote:
> > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > 
> > > On Wed, Mar 24, 2021 at 11:50:33AM +0000, Lee Jones wrote:
> > > > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > > > 
> > > > > On Wed, Mar 24, 2021 at 10:47:29AM +0000, Lee Jones wrote:
> > > > > > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > > > > > > On Wed, Mar 24, 2021 at 10:29:31AM +0000, Lee Jones wrote:
> > > > > > > > On Tue, 23 Mar 2021, Andy Shevchenko wrote:
> > > > > 
> > > > > ...
> > > > > 
> > > > > > Also, past acceptance does not guarantee ideal/correct usage.
> > > > > 
> > > > > In this case it's hardly can be misused. But I heard you.
> > > > > 
> > > > > ...
> > > > > 
> > > > > > > The semantic is min-max range and having two defines (*) here for these seems
> > > > > > > to me as an utter overkill.
> > > > > > > 
> > > > > > > Of course, if you insist I may do it.
> > > > > > > 
> > > > > > > *) since value is the same, we might have one definition, but it will be even
> > > > > > >    more confusion to have it as a min and max at the same time.
> > > > > > 
> > > > > > It's just tricky to decypher for people who do not know the API, which
> > > > > > is most people, myself included.  For APIs like usleep_range() et al.,
> > > > > > obviously this makes no sense at all.
> > > > > 
> > > > > Seem like you are insisting. Okay, I will define them. What do you prefer one
> > > > > or two definitions?
> > > > 
> > > > Actually I'm not.  I'm just trying to get my head around where the
> > > > data comes from and what the values actually mean.
> > > > 
> > > > > ...
> > > > > 
> > > > > > What defines a vector?
> > > > > 
> > > > > The combination is solely of the driver-hardware. Driver explicitly tells that
> > > > > how many vectors it may consume (taking into account the range asked) and API
> > > > > returns amount given or an error.
> > > > 
> > > > So, where does the information actually come from?
> > > > 
> > > > Information that comes from a datasheet is usually defined.
> > > > 
> > > > Information that comes from the F/W is usually read and popped into a
> > > > variable.
> > > 
> > > It's a two way road:
> > > a) driver states that it needs only 1 vector and it's enough to it
> > > b) hardware must provide at least 1 vector to be served by this driver.
> > > 
> > > Look again into grepped output. Most of drivers that define it as an variable
> > > may dynamically adapt to the different amount of IRQ vectors. When it's static,
> > > usually drivers just hard code those values.
> > > 
> > > I'm really don't see a point to define them _in this driver_.
> > 
> > That's fine.  I just felt like I had to ask.
> > 
> > Would you consider a comment that lets people unfamiliar with the API
> > what the values mean?
> > 
> > Something to the tune of:
> > 
> >   "This driver requests 1 (and only 1) IRQ vector"
> 
> 
> Rather
> 
>     "This driver requests only 1 (and it's enough) IRQ vector"

 "This driver only requires 1 IRQ vector"

> or something like this.
> 
> Should I send a patch with the comment included? If so, please suggest if it's
> good from English grammar/style perspective.
> 

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: enable MSI interrupt

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 03:10:35PM +0000, Lee Jones wrote:
> On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > On Wed, Mar 24, 2021 at 01:07:23PM +0000, Lee Jones wrote:
> > > On Wed, 24 Mar 2021, Andy Shevchenko wrote:
> > > > On Wed, Mar 24, 2021 at 11:50:33AM +0000, Lee Jones wrote:

...

> > > > It's a two way road:
> > > > a) driver states that it needs only 1 vector and it's enough to it
> > > > b) hardware must provide at least 1 vector to be served by this driver.
> > > > 
> > > > Look again into grepped output. Most of drivers that define it as an variable
> > > > may dynamically adapt to the different amount of IRQ vectors. When it's static,
> > > > usually drivers just hard code those values.
> > > > 
> > > > I'm really don't see a point to define them _in this driver_.
> > > 
> > > That's fine.  I just felt like I had to ask.
> > > 
> > > Would you consider a comment that lets people unfamiliar with the API
> > > what the values mean?
> > > 
> > > Something to the tune of:
> > > 
> > >   "This driver requests 1 (and only 1) IRQ vector"
> > 
> > 
> > Rather
> > 
> >     "This driver requests only 1 (and it's enough) IRQ vector"
> 
>  "This driver only requires 1 IRQ vector"

Thanks! v3 has been sent.

> > or something like this.
> > 
> > Should I send a patch with the comment included? If so, please suggest if it's
> > good from English grammar/style perspective.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] mfd: intel_quark_i2c_gpio: Reuse BAR definitions for MFD cell indexing

[Lee Jones]

On Tue, 23 Mar 2021, Andy Shevchenko wrote:

> It's convenient and less error prone to use definitions to address
> different cells in an array. For this purpose we may reuse existing
> BAR definitions.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: used explicit indices for MFD cells in the array (Lee)
>  drivers/mfd/intel_quark_i2c_gpio.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog