Re: [PATCH v8 char-misc-next 1/5] misc: microchip: pci1xxxx: Fix error handling path in probe function

Re: [PATCH v8 char-misc-next 1/5] misc: microchip: pci1xxxx: Fix error handling path in probe function

[Greg KH]

On Tue, Mar 28, 2023 at 08:10:04PM +0530, Vaibhaav Ram T.L wrote:
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> 
> Removed unnecessary header files.

That's not a "fix", it is a cleanup.

> Fix error handling path in probe function.
> Add pci_free_irq_vectors and auxiliary_device_delete in 
> error handling path.

All of these should be individual patches, right?

And you have trailing whitespace here :(

> 
> Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus driver for the PIO function in the multi-function endpoint of pci1xxxx device.")

Is this really a fix of that commit?  What was wrong there, just the
error handling?

> Reported by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>

No blank line there please.

> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> ---
>  drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 104 +++++++++---------
>  1 file changed, 55 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> index 32af2b14ff34..64302fdfbefc 100644
> --- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> @@ -1,16 +1,15 @@
>  // SPDX-License-Identifier: GPL-2.0
> -// Copyright (C) 2022 Microchip Technology Inc.
> +// Copyright (C) 2022-2023 Microchip Technology Inc.
>  
> -#include <linux/mfd/core.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> -#include <linux/spinlock.h>
> -#include <linux/gpio/driver.h>
> -#include <linux/interrupt.h>
> -#include <linux/io.h>
>  #include <linux/idr.h>
> +#include <linux/io.h>
>  #include "mchp_pci1xxxx_gp.h"
>  
> +#define PCI_DRIVER_NAME			"PCI1xxxxGP"

This is not a "fix" but rather a new change.

All of the changes in here are not related, break this up into "one
logical change per patch" please.

thanks,

greg k-h

Re: [PATCH v8 char-misc-next 1/5] misc: microchip: pci1xxxx: Fix error handling path in probe function

[VaibhaavRam.TL]

On Wed, 2023-03-29 at 11:59 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Mar 28, 2023 at 08:10:04PM +0530, Vaibhaav Ram T.L wrote:
> > From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> > 
> > Removed unnecessary header files.
> 
> That's not a "fix", it is a cleanup.
> 
> > Fix error handling path in probe function.
> > Add pci_free_irq_vectors and auxiliary_device_delete in
> > error handling path.
> 
> All of these should be individual patches, right?
Ok. I will split this patch into multiple patches.
> 
> And you have trailing whitespace here :(
Ok. I will correct this.
> 
> > 
> > Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus
> > driver for the PIO function in the multi-function endpoint of
> > pci1xxxx device.")
> 
> Is this really a fix of that commit?  What was wrong there, just the
> error handling?
Yes, There are two errors in the error handling path.
> 
> > Reported by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > 
> > Co-developed-by: Tharun Kumar P
> > <tharunkumar.pasumarthi@microchip.com>
> 
> No blank line there please.
Ok.
> 
> > Signed-off-by: Tharun Kumar P
> > <tharunkumar.pasumarthi@microchip.com>
> > Signed-off-by: Kumaravel Thiagarajan
> > <kumaravel.thiagarajan@microchip.com>
> > Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> > ---
> >  drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 104 +++++++++-----
> > ----
> >  1 file changed, 55 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> > b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> > index 32af2b14ff34..64302fdfbefc 100644
> > --- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> > +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> > @@ -1,16 +1,15 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > -// Copyright (C) 2022 Microchip Technology Inc.
> > +// Copyright (C) 2022-2023 Microchip Technology Inc.
> > 
> > -#include <linux/mfd/core.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> > -#include <linux/spinlock.h>
> > -#include <linux/gpio/driver.h>
> > -#include <linux/interrupt.h>
> > -#include <linux/io.h>
> >  #include <linux/idr.h>
> > +#include <linux/io.h>
> >  #include "mchp_pci1xxxx_gp.h"
> > 
> > +#define PCI_DRIVER_NAME                      "PCI1xxxxGP"
> 
> This is not a "fix" but rather a new change.
Ok. Will correct this.
> 
> All of the changes in here are not related, break this up into "one
> logical change per patch" please.
Ok Greg. I understand this now.
> 
> Thank You.


Regards,
Vaibhaav Ram

Re: [PATCH v8 char-misc-next 2/5] misc: microchip: pci1xxxx: Add OTP Functionality to read and write into OTP bin sysfs

[Greg KH]

On Tue, Mar 28, 2023 at 08:10:05PM +0530, Vaibhaav Ram T.L wrote:
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> 
> Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> industrial, and automotive applications. This switch integrates OTP 
> and EEPROM to enable customization of the part in the field. 
> This patch adds the OTP functionality to support the same.

Why not just use the in-kernel eeprom api instead of creating your own
custom user/kernel api?  Why is this so special to deserve that?

> 
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> ---
>  drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 198 ++++++++++++++++++
>  drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h |   7 +-
>  2 files changed, 204 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> index 64302fdfbefc..bf175e22090e 100644
> --- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> @@ -3,13 +3,210 @@
>  
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/iopoll.h>
>  #include <linux/pci.h>
>  #include <linux/idr.h>
>  #include <linux/io.h>
>  #include "mchp_pci1xxxx_gp.h"
>  
> +#define OTP_NAME			"pci1xxxx_otp"
>  #define PCI_DRIVER_NAME			"PCI1xxxxGP"
>  
> +#define MMAP_CFG_OFFSET(x)		(CONFIG_REG_ADDR_BASE + x)
> +#define MMAP_OTP_OFFSET(x)		(OTP_REG_ADDR_BASE + x)
> +
> +#define OTP_SIZE_BYTES			8192
> +
> +#define CONFIG_REG_ADDR_BASE		0
> +#define OTP_REG_ADDR_BASE		0x1000
> +
> +#define OTP_ADDR_HIGH_OFFSET		0x04
> +#define OTP_ADDR_LOW_OFFSET		0x08
> +#define OTP_PRGM_DATA_OFFSET		0x10
> +#define OTP_PRGM_MODE_OFFSET		0x14
> +#define OTP_RD_DATA_OFFSET		0x18
> +#define OTP_FUNC_CMD_OFFSET		0x20
> +#define OTP_CMD_GO_OFFSET		0x28
> +#define OTP_PASS_FAIL_OFFSET		0x2C
> +#define OTP_STATUS_OFFSET		0x30
> +
> +#define OTP_FUNC_RD_BIT			BIT(0)
> +#define OTP_FUNC_PGM_BIT		BIT(1)
> +#define OTP_CMD_GO_BIT			BIT(0)
> +#define OTP_STATUS_BUSY_BIT		BIT(0)
> +#define OTP_PGM_MODE_BYTE_BIT		BIT(0)
> +#define OTP_FAIL_BIT			BIT(0)
> +
> +#define STATUS_READ_DELAY_US		1
> +#define STATUS_READ_TIMEOUT_US		20000
> +
> +#define CFG_SYS_LOCK_OFFSET		0xA0
> +#define CFG_SYS_LOCK_PF3		BIT(5)
> +
> +#define BYTE_LOW			(GENMASK(7, 0))
> +#define BYTE_HIGH			(GENMASK(12, 8))
> +
> +static int set_sys_lock(struct pci1xxxx_otp_eeprom_device *priv)
> +{
> +	void __iomem *sys_lock = priv->reg_base +
> +				 MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> +	u8 data;
> +
> +	writel(CFG_SYS_LOCK_PF3, sys_lock);
> +	data = readl(sys_lock);
> +	if (data != CFG_SYS_LOCK_PF3)
> +		return -EPERM;
> +
> +	return 0;
> +}
> +
> +static void release_sys_lock(struct pci1xxxx_otp_eeprom_device *priv)
> +{
> +	void __iomem *sys_lock = priv->reg_base +
> +				 MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> +	writel(0, sys_lock);
> +}
> +
> +static void otp_device_set_address(struct pci1xxxx_otp_eeprom_device *priv,
> +				   u16 address)
> +{
> +	u16 lo, hi;
> +
> +	lo = address & BYTE_LOW;
> +	hi = (address & BYTE_HIGH) >> 8;
> +	writew(lo, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_LOW_OFFSET));
> +	writew(hi, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_HIGH_OFFSET));
> +}
> +
> +static ssize_t pci1xxxx_otp_read(struct file *filp, struct kobject *kobj,
> +				 struct bin_attribute *bin_attr,
> +				 char *buf, loff_t off, size_t count)
> +{
> +	struct pci1xxxx_otp_eeprom_device *priv;
> +	struct device *dev;
> +	void __iomem *rb;
> +	u32 regval;
> +	u32 byte;
> +	int ret;
> +	u8 data;
> +
> +	dev = container_of(kobj, struct device, kobj);
> +	priv = dev_get_drvdata(dev);
> +	if (priv != NULL)
> +		rb = priv->reg_base;
> +	else
> +		return -ENODEV;
> +
> +	ret = set_sys_lock(priv);
> +	if (ret)
> +		return ret;
> +
> +	for (byte = 0; byte < count; byte++) {
> +		otp_device_set_address(priv, (u16)(off + byte));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		writel(data | OTP_FUNC_RD_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +		writel(data | OTP_CMD_GO_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> +		ret = read_poll_timeout(readl, regval,
> +					!(regval & OTP_STATUS_BUSY_BIT),
> +					STATUS_READ_DELAY_US,
> +					STATUS_READ_TIMEOUT_US, true,
> +					rb + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> +
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> +		if (ret < 0 || data & OTP_FAIL_BIT)
> +			break;
> +
> +		buf[byte] = readl(rb + MMAP_OTP_OFFSET(OTP_RD_DATA_OFFSET));
> +	}
> +	release_sys_lock(priv);
> +
> +	return byte;
> +}
> +
> +static ssize_t pci1xxxx_otp_write(struct file *filp, struct kobject *kobj,
> +				  struct bin_attribute *bin_attr,
> +				  char *value, loff_t off, size_t count)
> +{
> +	struct pci1xxxx_otp_eeprom_device *priv;
> +	struct device *dev;
> +	void __iomem *rb;
> +	u32 regval;
> +	u32 byte;
> +	int ret;
> +	u8 data;
> +
> +	dev = container_of(kobj, struct device, kobj);
> +	priv = dev_get_drvdata(dev);
> +	if (priv != NULL)
> +		rb = priv->reg_base;
> +	else
> +		return -ENODEV;
> +
> +	ret = set_sys_lock(priv);
> +	if (ret)
> +		return ret;
> +
> +	for (byte = 0; byte < count; byte++) {
> +		otp_device_set_address(priv, (u16)(off + byte));
> +
> +		/*
> +		 * Set OTP_PGM_MODE_BYTE command bit in OTP_PRGM_MODE register
> +		 * to enable Byte programming
> +		 */
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> +		writel(data | OTP_PGM_MODE_BYTE_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> +		writel(*(value + byte), rb + MMAP_OTP_OFFSET(OTP_PRGM_DATA_OFFSET));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		writel(data | OTP_FUNC_PGM_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +		writel(data | OTP_CMD_GO_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> +		ret = read_poll_timeout(readl, regval,
> +					!(regval & OTP_STATUS_BUSY_BIT),
> +					STATUS_READ_DELAY_US,
> +					STATUS_READ_TIMEOUT_US, true,
> +					rb + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> +
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> +		if (ret < 0 || data & OTP_FAIL_BIT)
> +			break;
> +	}
> +	release_sys_lock(priv);
> +
> +	return byte;
> +}
> +
> +static struct bin_attribute pci1xxxx_otp_attr = {
> +	.attr = {
> +		.name = OTP_NAME,
> +		.mode = 0600,
> +	},
> +	.size = OTP_SIZE_BYTES,
> +	.read = pci1xxxx_otp_read,
> +	.write = pci1xxxx_otp_write,
> +};
> +
> +static struct bin_attribute *pci1xxxx_bin_attributes[] = {
> +	&pci1xxxx_otp_attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group pci1xxxx_bin_attributes_group = {
> +	.bin_attrs = pci1xxxx_bin_attributes,
> +};
> +
> +static const struct attribute_group *pci1xxxx_bin_attributes_groups[] = {
> +	&pci1xxxx_bin_attributes_group,
> +	NULL
> +};
> +
>  struct aux_bus_device {
>  	struct auxiliary_device_wrapper *aux_device_wrapper[2];
>  };
> @@ -55,6 +252,7 @@ static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
>  	otp_eeprom_wrapper->aux_dev.name = aux_dev_otp_e2p_name;
>  	otp_eeprom_wrapper->aux_dev.dev.parent = &pdev->dev;
>  	otp_eeprom_wrapper->aux_dev.dev.release = gp_auxiliary_device_release;
> +	otp_eeprom_wrapper->aux_dev.dev.groups = pci1xxxx_bin_attributes_groups;
>  	otp_eeprom_wrapper->aux_dev.id = retval;
>  
>  	otp_eeprom_wrapper->gp_aux_data.region_start = pci_resource_start(pdev, 0);
> diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h
> index 37eec73b20d7..c6238a817dc6 100644
> --- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> -/* Copyright (C) 2022 Microchip Technology Inc. */
> +/* Copyright (C) 2022-2023 Microchip Technology Inc. */
>  
>  #ifndef _GPIO_PCI1XXXX_H
>  #define _GPIO_PCI1XXXX_H
> @@ -25,4 +25,9 @@ struct auxiliary_device_wrapper {
>  	struct gp_aux_data_type gp_aux_data;
>  };
>  
> +struct pci1xxxx_otp_eeprom_device {
> +	struct auxiliary_device *pdev;
> +	void __iomem *reg_base;
> +	bool is_eeprom_present;

This field is never used, why have it?

> +};

Why does this need to be in the .h file and not in the .c file?

thanks,

greg k-h

Re: [PATCH v8 char-misc-next 2/5] misc: microchip: pci1xxxx: Add OTP Functionality to read and write into OTP bin sysfs

[VaibhaavRam.TL]

On Wed, 2023-03-29 at 12:01 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Mar 28, 2023 at 08:10:05PM +0530, Vaibhaav Ram T.L wrote:
> > From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> > 
> > Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> > industrial, and automotive applications. This switch integrates OTP
> > and EEPROM to enable customization of the part in the field.
> > This patch adds the OTP functionality to support the same.
> 
> Why not just use the in-kernel eeprom api instead of creating your
> own
> custom user/kernel api?  Why is this so special to deserve that?
Unlike other in-Kernel EEPROM APIs, this OTP is not accessible through
any of the i2c/spi buses available to the kernel.

It is only accessible through the register interface available in the
OTP controller of the PCI1XXXX device.

The architecture of the device was discussed @
https://lore.kernel.org/all/Y+9HOdHGqmPP%2FUde@kroah.com/
> > +struct pci1xxxx_otp_eeprom_device {
> > +     struct auxiliary_device *pdev;
> > +     void __iomem *reg_base;
> > +     bool is_eeprom_present;
> 
> This field is never used, why have it?
This should have appeared in EEPROM patch. Will change it.
> 
> > +};
> 
> Why does this need to be in the .h file and not in the .c file?
This structure is shared by both mchp_pci1xxxx_gp.c and
mchp_pci1xxxx_otpe2p.c files.
> 
> thanks,
> 
> greg k-h

Re: [PATCH v8 char-misc-next 3/5] misc: microchip: pci1xxxx: Add EEPROM Functionality to read and write into EEPROM bin sysfs

[Greg KH]

On Tue, Mar 28, 2023 at 08:10:06PM +0530, Vaibhaav Ram T.L wrote:
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> 
> Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> industrial, and automotive applications. This switch integrates OTP
> and EEPROM to enable customization of the part in the field. 
> This patch adds EEPROM functionality to support the same.

Again, why not use the in-kernel eeprom api instead?

thanks,

greg k-h

Re: [PATCH v8 char-misc-next 3/5] misc: microchip: pci1xxxx: Add EEPROM Functionality to read and write into EEPROM bin sysfs

[VaibhaavRam.TL]

On Wed, 2023-03-29 at 12:01 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Mar 28, 2023 at 08:10:06PM +0530, Vaibhaav Ram T.L wrote:
> > From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> > 
> > Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> > industrial, and automotive applications. This switch integrates OTP
> > and EEPROM to enable customization of the part in the field.
> > This patch adds EEPROM functionality to support the same.
> 
> Again, why not use the in-kernel eeprom api instead?
Unlike other in-Kernel EEPROM APIs, this EEPROM is not accessible
through any of the i2c/spi buses available to the kernel.

It is only accessible through the register interface available in the
EEPROM controller of the PCI1XXXX device.

The architecture of the device was explained @
https://lore.kernel.org/all/Y+9HOdHGqmPP%2FUde@kroah.com/
> 
> thanks,
> 
> greg k-h

Re: [PATCH v8 char-misc-next 3/5] misc: microchip: pci1xxxx: Add EEPROM Functionality to read and write into EEPROM bin sysfs

[Greg KH]

On Thu, Mar 30, 2023 at 05:28:43AM +0000, VaibhaavRam.TL@microchip.com wrote:
> On Wed, 2023-03-29 at 12:01 +0200, Greg KH wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Tue, Mar 28, 2023 at 08:10:06PM +0530, Vaibhaav Ram T.L wrote:
> > > From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> > > 
> > > Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> > > industrial, and automotive applications. This switch integrates OTP
> > > and EEPROM to enable customization of the part in the field.
> > > This patch adds EEPROM functionality to support the same.
> > 
> > Again, why not use the in-kernel eeprom api instead?
> Unlike other in-Kernel EEPROM APIs, this EEPROM is not accessible
> through any of the i2c/spi buses available to the kernel.
> 
> It is only accessible through the register interface available in the
> EEPROM controller of the PCI1XXXX device.
> 
> The architecture of the device was explained @
> https://lore.kernel.org/all/Y+9HOdHGqmPP%2FUde@kroah.com/

That shows the architecture, but I left it as "try using the EEPROM api
and let us know how it goes" and you never did that.

If you are going to create your own user/kernel api for something that
the kernel already has a user/kernel api for, you need to document it
in the changelog text really really really well why you can't use the
existing api, and why this new custom one really is the only way to
solve this issue, to explain all of this.

thanks,

greg k-h

Re: [PATCH v8 char-misc-next 3/5] misc: microchip: pci1xxxx: Add EEPROM Functionality to read and write into EEPROM bin sysfs

[Michael Walle]

[First, please CC people who did comments on previous versions.]

> > > Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> > > industrial, and automotive applications. This switch integrates OTP
> > > and EEPROM to enable customization of the part in the field.
> > > This patch adds EEPROM functionality to support the same.
> > 
> > Again, why not use the in-kernel eeprom api instead?
> Unlike other in-Kernel EEPROM APIs, this EEPROM is not accessible
> through any of the i2c/spi buses available to the kernel.

I fail to see how this matters. NVMEM has a generic read/write callback.
There is no dependency on I2C or SPI. Again, you should look into nvmem.
And it should be perfectly fine to use nvmem without nvmem cells at all.

With CONFIG_NVMEM_SYSFS you should get a "nvmem" binary file in sysfs.
Wit config->compat set (although I don't know if that is recommended) you
should get an "eeprom" binary file in sysfs.

> It is only accessible through the register interface available in the
> EEPROM controller of the PCI1XXXX device.

-michael

RE: [PATCH v8 char-misc-next 3/5] misc: microchip: pci1xxxx: Add EEPROM Functionality to read and write into EEPROM bin sysfs

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Thursday, March 30, 2023 3:22 PM
> To: VaibhaavRam TL - I69105 <VaibhaavRam.TL@microchip.com>
> 
> [First, please CC people who did comments on previous versions.]
Sure. We will do this henceforth.
> 
> > > > Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> > > > industrial, and automotive applications. This switch integrates
> > > > OTP and EEPROM to enable customization of the part in the field.
> > > > This patch adds EEPROM functionality to support the same.
> > >
> > > Again, why not use the in-kernel eeprom api instead?
> > Unlike other in-Kernel EEPROM APIs, this EEPROM is not accessible
> > through any of the i2c/spi buses available to the kernel.
> 
> I fail to see how this matters. NVMEM has a generic read/write callback.
> There is no dependency on I2C or SPI. Again, you should look into nvmem.
> And it should be perfectly fine to use nvmem without nvmem cells at all.
> 
> With CONFIG_NVMEM_SYSFS you should get a "nvmem" binary file in sysfs.
> Wit config->compat set (although I don't know if that is recommended) you
> should get an "eeprom" binary file in sysfs.
> 
> > It is only accessible through the register interface available in the
> > EEPROM controller of the PCI1XXXX device.
Michael & Greg,
By in-kernel EEPROM APIs and NVMEM, are you both referring to the same?
Can you please confirm?
We can explore this if that is fine to use this without nvmem cells.
I think we misunderstood as we were referring to drivers/misc/eeprom for examples.

Thank You.

Regards,
Kumar

Re: [PATCH v8 char-misc-next 4/5] misc: microchip: pci1xxxx: Load auxiliary driver for OTP/EEPROM auxiliary device

[Greg KH]

On Tue, Mar 28, 2023 at 08:10:07PM +0530, Vaibhaav Ram T.L wrote:
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> 
> This patch contains driver to perform ioremap for the OTP/EEPROM 
> auxiliary device.

What does this mean?  What will it do?  Why is this needed?

thanks,

greg k-h

Re: [PATCH v8 char-misc-next 4/5] misc: microchip: pci1xxxx: Load auxiliary driver for OTP/EEPROM auxiliary device

[VaibhaavRam.TL]

On Wed, 2023-03-29 at 12:02 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Mar 28, 2023 at 08:10:07PM +0530, Vaibhaav Ram T.L wrote:
> > From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> > 
> > This patch contains driver to perform ioremap for the OTP/EEPROM
> > auxiliary device.
> 
> What does this mean?  What will it do?  Why is this needed?
In PCI1XXXX, same PCIe function is used for gpio handling as well as
the OTP/EEPROM programming.

Hence, we are loading auxiliary bus driver for the PCIe function and
spawning two separate auxiliary devices for each purpose.

Auxiliary device for OTP/EEPROM includes OTP and EEPROM as separate bin
attributes in its device's attribute_group and maps them into sysfs.

But these bin attributes can be accessed only when the memory resource
of the auxiliary device is mapped into the virtual space of the kernel
which is done by this auxiliary device driver.
> 
> thanks,
> 
> greg k-h

Re: [PATCH v8 char-misc-next 5/5] misc: microchip: pci1xxxx: Add documentation for sysfs bin attributes

[Greg KH]

On Tue, Mar 28, 2023 at 08:10:08PM +0530, Vaibhaav Ram T.L wrote:
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> 
> This patch contains Documentation for Microchip PCI1XXXX OTP/EEPROM sysfs
> bin attributes.
> 
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> ---
>  Documentation/ABI/stable/sysfs-devices-mchp-pci1xxxx | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>  create mode 100644 Documentation/ABI/stable/sysfs-devices-mchp-pci1xxxx
> 
> diff --git a/Documentation/ABI/stable/sysfs-devices-mchp-pci1xxxx b/Documentation/ABI/stable/sysfs-devices-mchp-pci1xxxx
> new file mode 100644
> index 000000000000..0913c7b01990
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-devices-mchp-pci1xxxx
> @@ -0,0 +1,11 @@
> +What:		/sys/devices/pciXXXX:XX/XXXX:XX:XX.X/../mchp_pci1xxxx_gp.gp_otp_e2p.n/pci1xxxx_eeprom
> +Date:		March 27, 2023
> +KernelVersion:	6.3
> +Contact:	Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> +Description:	This bin file is used for writing into and reading from Microchip PCI1XXXX EEPROM

"bin"?  Userspace doesn't know what that means.

But again, why can't you use the eeprom api instead?

thanks,

greg k-h

Re: [PATCH v8 char-misc-next 5/5] misc: microchip: pci1xxxx: Add documentation for sysfs bin attributes

[VaibhaavRam.TL]

On Wed, 2023-03-29 at 12:03 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Mar 28, 2023 at 08:10:08PM +0530, Vaibhaav Ram T.L wrote:
> > From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> > 
> > This patch contains Documentation for Microchip PCI1XXXX OTP/EEPROM
> > sysfs
> > bin attributes.
> > 
> > Co-developed-by: Tharun Kumar P
> > <tharunkumar.pasumarthi@microchip.com>
> > Signed-off-by: Tharun Kumar P
> > <tharunkumar.pasumarthi@microchip.com>
> > Signed-off-by: Kumaravel Thiagarajan
> > <kumaravel.thiagarajan@microchip.com>
> > Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> > ---
> >  Documentation/ABI/stable/sysfs-devices-mchp-pci1xxxx | 11
> > +++++++++++
> >  1 file changed, 11 insertions(+)
> >  create mode 100644 Documentation/ABI/stable/sysfs-devices-mchp-
> > pci1xxxx
> > 
> > diff --git a/Documentation/ABI/stable/sysfs-devices-mchp-pci1xxxx
> > b/Documentation/ABI/stable/sysfs-devices-mchp-pci1xxxx
> > new file mode 100644
> > index 000000000000..0913c7b01990
> > --- /dev/null
> > +++ b/Documentation/ABI/stable/sysfs-devices-mchp-pci1xxxx
> > @@ -0,0 +1,11 @@
> > +What:               
> > /sys/devices/pciXXXX:XX/XXXX:XX:XX.X/../mchp_pci1xxxx_gp.gp_otp_e2p
> > .n/pci1xxxx_eeprom
> > +Date:                March 27, 2023
> > +KernelVersion:       6.3
> > +Contact:     Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> > +Description: This bin file is used for writing into and reading
> > from Microchip PCI1XXXX EEPROM
> 
> "bin"?  Userspace doesn't know what that means.
Can I replace with "files"?
> 
> But again, why can't you use the eeprom api instead?
This is explained in other replies - this eeprom/otp is not accessible
through any i2c/SPI bus available to kernel.


Thank You.

Regards,
Vaibhaav Ram

Re: [PATCH v8 char-misc-next 0/5] Fix error handling in probe

[Greg KH]

On Tue, Mar 28, 2023 at 08:10:03PM +0530, Vaibhaav Ram T.L wrote:
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> 
> Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> industrial, and automotive applications. This switch integrates OTP
> and EEPROM to enable customization of the part in the field. First 
> patch provides fix for error handling in the probe function of 
> mchp_pci1xxxx_gp driver. Remaining patches add the OTP/EEPROM driver 
> for the switch.
> 
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>


Why are there signed-off-by for the 00/XX patch?

And the subject doesn't make sense to me.

thanks,

greg k-h

Re: [PATCH v8 char-misc-next 0/5] Fix error handling in probe

[VaibhaavRam.TL]

On Wed, 2023-03-29 at 12:03 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Mar 28, 2023 at 08:10:03PM +0530, Vaibhaav Ram T.L wrote:
> > From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> > 
> > Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> > industrial, and automotive applications. This switch integrates OTP
> > and EEPROM to enable customization of the part in the field. First
> > patch provides fix for error handling in the probe function of
> > mchp_pci1xxxx_gp driver. Remaining patches add the OTP/EEPROM
> > driver
> > for the switch.
> > 
> > Co-developed-by: Tharun Kumar P
> > <tharunkumar.pasumarthi@microchip.com>
> > Signed-off-by: Tharun Kumar P
> > <tharunkumar.pasumarthi@microchip.com>
> > Signed-off-by: Kumaravel Thiagarajan
> > <kumaravel.thiagarajan@microchip.com>
> > Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> 
> 
> Why are there signed-off-by for the 00/XX patch?
Ok I will remove in next version of patch
> 
> And the subject doesn't make sense to me.
Sorry. I will fix this.


Thank You.

Regards,
Vaibhaav Ram