[PATCH 00/10] media: microchip-isc: AWB, stream-stop and endpoint-ref fixes

[PATCH 00/10] media: microchip-isc: AWB, stream-stop and endpoint-ref fixes

[Balakrishnan Sambath]

This series has a few fixes for the Microchip ISC/XISC driver, found
while testing and from the feedback on the combined series [1].

Fixes only, sent ahead of the enhancements so they can reach stable.
The SBGGR10, WB masking and PM leak fixes are unchanged from [1] (the
SBGGR10 and PM leak ones keep their Reviewed-by).

All but the comment fix carry a Fixes tag and Cc: stable.

[1] https://lore.kernel.org/r/20260603-microchip-isc-fixes-v6-0-8c3d7474a768@microchip.com

Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
Balakrishnan Sambath (10):
      media: microchip-isc: fix awb_mutex and lock lifecycle
      media: microchip-isc: take a reference on the parsed endpoints
      media: microchip-isc: synchronize the IRQ before disabling clocks on stop
      media: microchip-isc: disable histogram and flush AWB work on stop
      media: microchip-isc: clean up histogram on the start_streaming error path
      media: microchip-isc: do not touch WB registers when not streaming
      media: microchip-isc: fix pfe_cfg0_bps comment
      media: microchip-isc: fix PM runtime leak in AWB work handler
      media: microchip-isc: fix SBGGR10 Bayer pattern
      media: microchip-isc: fix WB offset and gain register field masking

 .../media/platform/microchip/microchip-isc-base.c  | 72 +++++++++++++++-------
 drivers/media/platform/microchip/microchip-isc.h   |  7 ++-
 .../platform/microchip/microchip-sama5d2-isc.c     | 22 ++++---
 .../platform/microchip/microchip-sama7g5-isc.c     | 22 ++++---
 4 files changed, 85 insertions(+), 38 deletions(-)
---
base-commit: 05f7e89ab9731565d8a62e3b5d1ec206485eeb0b
change-id: 20260615-balki-isc-prefix-fixes-v1-c8c44224caa1

Best regards,
-- 
Balakrishnan Sambath <balakrishnan.s@microchip.com>

[PATCH 01/10] media: microchip-isc: fix awb_mutex and lock lifecycle

[Balakrishnan Sambath]

isc_async_complete() initialised awb_mutex and isc->lock only after an
early error return, and the teardown was inconsistent:

 - isc_async_unbind() destroyed awb_mutex before cancelling awb_work,
   which takes it;
 - a failed .complete() destroyed both locks, then the v4l2-async core
   unbinds the subdev and isc_async_unbind() destroyed awb_mutex again;
 - isc->lock was destroyed only on the .complete() error path, so the
   normal unbind path leaked it.

Initialise both locks before the first error return, make unbind the
single teardown site (cancel the work, then destroy both locks) and
drop the destroys from the .complete() error path.

Fixes: 314c96e5203d ("media: atmel: atmel-isc-base: use mutex to lock awb workq from streaming")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-isc-base.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index a7cdc743fda7..45a7af779323 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -1703,10 +1703,11 @@ static void isc_async_unbind(struct v4l2_async_notifier *notifier,
 {
 	struct isc_device *isc = container_of(notifier->v4l2_dev,
 					      struct isc_device, v4l2_dev);
-	mutex_destroy(&isc->awb_mutex);
 	cancel_work_sync(&isc->awb_work);
+	mutex_destroy(&isc->awb_mutex);
 	video_unregister_device(&isc->video_dev);
 	v4l2_ctrl_handler_free(&isc->ctrls.handler);
+	mutex_destroy(&isc->lock);
 }
 
 struct isc_format *isc_find_format_by_code(struct isc_device *isc,
@@ -1758,6 +1759,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
 	int ret = 0;
 
 	INIT_WORK(&isc->awb_work, isc_awb_work);
+	mutex_init(&isc->lock);
+	mutex_init(&isc->awb_mutex);
 
 	ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev);
 	if (ret < 0) {
@@ -1767,8 +1770,6 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
 
 	isc->current_subdev = container_of(notifier,
 					   struct isc_subdev_entity, notifier);
-	mutex_init(&isc->lock);
-	mutex_init(&isc->awb_mutex);
 
 	init_completion(&isc->comp);
 
@@ -1841,8 +1842,6 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
 	video_unregister_device(vdev);
 
 isc_async_complete_err:
-	mutex_destroy(&isc->awb_mutex);
-	mutex_destroy(&isc->lock);
 	return ret;
 }
 

-- 
2.34.1

[PATCH 02/10] media: microchip-isc: take a reference on the parsed endpoints

[Balakrishnan Sambath]

for_each_endpoint_of_node() drops the reference on the current node as
it advances. xisc_parse_dt() and isc_parse_dt() store the node in
subdev_entity->epn and release it later with of_node_put(), but never
took their own reference, so the stored pointer refers to an
already-released node. This underflows the refcount and can
use-after-free, reachable through the camera device tree overlay.

Take a reference with of_node_get() when storing the node, and drop it
in microchip_isc_subdev_cleanup() so the entities the bind loop never
reaches on an early exit do not leak it.

Fixes: c9aa973884a1 ("media: atmel: atmel-isc: add microchip-xisc driver")
Fixes: d6701f13bd07 ("media: atmel: Use v4l2_async_notifier_add_fwnode_remote_subdev")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-isc-base.c  |  6 ++++++
 .../media/platform/microchip/microchip-sama5d2-isc.c   | 18 ++++++++++++------
 .../media/platform/microchip/microchip-sama7g5-isc.c   | 18 ++++++++++++------
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 45a7af779323..4079c79cb668 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -1859,6 +1859,12 @@ void microchip_isc_subdev_cleanup(struct isc_device *isc)
 	list_for_each_entry(subdev_entity, &isc->subdev_entities, list) {
 		v4l2_async_nf_unregister(&subdev_entity->notifier);
 		v4l2_async_nf_cleanup(&subdev_entity->notifier);
+		/*
+		 * Release the endpoint reference taken while parsing. It is
+		 * NULL for entities the bind loop already consumed, so this
+		 * only drops the ones left over on an early exit.
+		 */
+		of_node_put(subdev_entity->epn);
 	}
 
 	INIT_LIST_HEAD(&isc->subdev_entities);
diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
index 66d3d7891991..97752eca6d6b 100644
--- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
@@ -356,28 +356,28 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 	struct device_node *epn;
 	struct isc_subdev_entity *subdev_entity;
 	unsigned int flags;
+	int ret;
 
 	INIT_LIST_HEAD(&isc->subdev_entities);
 
 	for_each_endpoint_of_node(np, epn) {
 		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
-		int ret;
 
 		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
 						 &v4l2_epn);
 		if (ret) {
-			of_node_put(epn);
 			dev_err(dev, "Could not parse the endpoint\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_put;
 		}
 
 		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
 					     GFP_KERNEL);
 		if (!subdev_entity) {
-			of_node_put(epn);
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto err_put;
 		}
-		subdev_entity->epn = epn;
+		subdev_entity->epn = of_node_get(epn);
 
 		flags = v4l2_epn.bus.parallel.flags;
 
@@ -398,6 +398,12 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 	}
 
 	return 0;
+
+err_put:
+	of_node_put(epn);
+	list_for_each_entry(subdev_entity, &isc->subdev_entities, list)
+		of_node_put(subdev_entity->epn);
+	return ret;
 }
 
 static int microchip_isc_probe(struct platform_device *pdev)
diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index b0302dfc3278..1f5debb74f18 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -340,6 +340,7 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
 	struct isc_subdev_entity *subdev_entity;
 	unsigned int flags;
 	bool mipi_mode;
+	int ret;
 
 	INIT_LIST_HEAD(&isc->subdev_entities);
 
@@ -347,23 +348,22 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
 
 	for_each_endpoint_of_node(np, epn) {
 		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
-		int ret;
 
 		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
 						 &v4l2_epn);
 		if (ret) {
-			of_node_put(epn);
 			dev_err(dev, "Could not parse the endpoint\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_put;
 		}
 
 		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
 					     GFP_KERNEL);
 		if (!subdev_entity) {
-			of_node_put(epn);
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto err_put;
 		}
-		subdev_entity->epn = epn;
+		subdev_entity->epn = of_node_get(epn);
 
 		flags = v4l2_epn.bus.parallel.flags;
 
@@ -387,6 +387,12 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
 	}
 
 	return 0;
+
+err_put:
+	of_node_put(epn);
+	list_for_each_entry(subdev_entity, &isc->subdev_entities, list)
+		of_node_put(subdev_entity->epn);
+	return ret;
 }
 
 static int microchip_xisc_probe(struct platform_device *pdev)

-- 
2.34.1

[PATCH 03/10] media: microchip-isc: synchronize the IRQ before disabling clocks on stop

[Balakrishnan Sambath]

isc_stop_streaming() masks the DMA interrupt and then drops the runtime
PM reference, which disables the ISC clocks. microchip_isc_interrupt()
may still be executing on another CPU at that point; it reads ISC_INTSR
over regmap, and touching the unclocked registers triggers an external
abort.

Store the IRQ number at probe and call synchronize_irq() after masking
the interrupt, before dropping the PM reference.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-isc-base.c    | 3 +++
 drivers/media/platform/microchip/microchip-isc.h         | 1 +
 drivers/media/platform/microchip/microchip-sama5d2-isc.c | 2 ++
 drivers/media/platform/microchip/microchip-sama7g5-isc.c | 2 ++
 4 files changed, 8 insertions(+)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 4079c79cb668..3245dd7cb980 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -425,6 +425,9 @@ static void isc_stop_streaming(struct vb2_queue *vq)
 	/* Disable DMA interrupt */
 	regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
 
+	/* let a running IRQ handler finish before the clock is disabled */
+	synchronize_irq(isc->irq);
+
 	pm_runtime_put_sync(isc->dev);
 
 	/* Disable stream on the sub device */
diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
index ad4e98a1dd8f..f5e322c2e36b 100644
--- a/drivers/media/platform/microchip/microchip-isc.h
+++ b/drivers/media/platform/microchip/microchip-isc.h
@@ -287,6 +287,7 @@ struct isc_device {
 	u32			dcfg;
 
 	struct device		*dev;
+	int			irq;
 	struct v4l2_device	v4l2_dev;
 	struct video_device	video_dev;
 
diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
index 97752eca6d6b..5d49b9d48f57 100644
--- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
@@ -438,6 +438,8 @@ static int microchip_isc_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
+	isc->irq = irq;
+
 	ret = devm_request_irq(dev, irq, microchip_isc_interrupt, 0,
 			       "microchip-sama5d2-isc", isc);
 	if (ret < 0) {
diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index 1f5debb74f18..4f9e9a5ed4d1 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -427,6 +427,8 @@ static int microchip_xisc_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
+	isc->irq = irq;
+
 	ret = devm_request_irq(dev, irq, microchip_isc_interrupt, 0,
 			       "microchip-sama7g5-xisc", isc);
 	if (ret < 0) {

-- 
2.34.1

Re: [PATCH 03/10] media: microchip-isc: synchronize the IRQ before disabling clocks on stop

[Eugen Hristev]

On 6/16/26 14:50, Balakrishnan Sambath wrote:
> isc_stop_streaming() masks the DMA interrupt and then drops the runtime
> PM reference, which disables the ISC clocks. microchip_isc_interrupt()
> may still be executing on another CPU at that point; it reads ISC_INTSR
> over regmap, and touching the unclocked registers triggers an external
> abort.
> 
> Store the IRQ number at probe and call synchronize_irq() after masking
> the interrupt, before dropping the PM reference.
> 
> Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
> Cc: stable@vger.kernel.org
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>

Reviewed-by: Eugen Hristev <ehristev@kernel.org>

[PATCH 04/10] media: microchip-isc: disable histogram and flush AWB work on stop

[Balakrishnan Sambath]

isc_stop_streaming() masked the DMA done interrupt but left the
histogram enabled, so a HISDONE that fired just before the stop could
still queue isc_awb_work() after pm_runtime_put_sync() gated the clocks.
isc_awb_work() reads the histogram registers in isc_hist_count() before
taking its own PM reference, so the access faults on the suspended
device.

Disable the histogram and flush the work before dropping the PM
reference, mirroring the start_streaming error path.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-isc-base.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 3245dd7cb980..8f255a4c4e7a 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -425,9 +425,13 @@ static void isc_stop_streaming(struct vb2_queue *vq)
 	/* Disable DMA interrupt */
 	regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
 
+	isc_set_histogram(isc, false);
+
 	/* let a running IRQ handler finish before the clock is disabled */
 	synchronize_irq(isc->irq);
 
+	cancel_work_sync(&isc->awb_work);
+
 	pm_runtime_put_sync(isc->dev);
 
 	/* Disable stream on the sub device */

-- 
2.34.1

Re: [PATCH 04/10] media: microchip-isc: disable histogram and flush AWB work on stop

[Eugen Hristev]

On 6/16/26 14:50, Balakrishnan Sambath wrote:
> isc_stop_streaming() masked the DMA done interrupt but left the
> histogram enabled, so a HISDONE that fired just before the stop could
> still queue isc_awb_work() after pm_runtime_put_sync() gated the clocks.
> isc_awb_work() reads the histogram registers in isc_hist_count() before
> taking its own PM reference, so the access faults on the suspended
> device.
> 
> Disable the histogram and flush the work before dropping the PM
> reference, mirroring the start_streaming error path.
> 

In here you reference some error path that you mirror, but you add it in
the next commit in which you say you mirror this exact commit :/
Circular mirroring of paths which initially did not exist.
I would squash this commit with the next one and show the facts as they
are : stop histogram and work queue in different stop/error scenarios,
no more mirroring.

Eugen

> Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
> Cc: stable@vger.kernel.org
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> ---
>  drivers/media/platform/microchip/microchip-isc-base.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index 3245dd7cb980..8f255a4c4e7a 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -425,9 +425,13 @@ static void isc_stop_streaming(struct vb2_queue *vq)
>  	/* Disable DMA interrupt */
>  	regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
>  
> +	isc_set_histogram(isc, false);
> +
>  	/* let a running IRQ handler finish before the clock is disabled */
>  	synchronize_irq(isc->irq);
>  
> +	cancel_work_sync(&isc->awb_work);
> +
>  	pm_runtime_put_sync(isc->dev);
>  
>  	/* Disable stream on the sub device */
> 

Re: [PATCH 04/10] media: microchip-isc: disable histogram and flush AWB work on stop

[Balakrishnan.S]

Hi Eugen,

Thanks for the feedback on this.

On 21/06/26 11:53 am, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 6/16/26 14:50, Balakrishnan Sambath wrote:
>> isc_stop_streaming() masked the DMA done interrupt but left the
>> histogram enabled, so a HISDONE that fired just before the stop could
>> still queue isc_awb_work() after pm_runtime_put_sync() gated the clocks.
>> isc_awb_work() reads the histogram registers in isc_hist_count() before
>> taking its own PM reference, so the access faults on the suspended
>> device.
>>
>> Disable the histogram and flush the work before dropping the PM
>> reference, mirroring the start_streaming error path.
>>
> 
> In here you reference some error path that you mirror, but you add it in
> the next commit in which you say you mirror this exact commit :/
> Circular mirroring of paths which initially did not exist.
> I would squash this commit with the next one and show the facts as they
> are : stop histogram and work queue in different stop/error scenarios,
> no more mirroring.

Agreed, indeed its circular.
Will squash both commits and reword so each path stands on its own.

> 
> Eugen
> 
>> Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>> ---
>>   drivers/media/platform/microchip/microchip-isc-base.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
>> index 3245dd7cb980..8f255a4c4e7a 100644
>> --- a/drivers/media/platform/microchip/microchip-isc-base.c
>> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
>> @@ -425,9 +425,13 @@ static void isc_stop_streaming(struct vb2_queue *vq)
>>        /* Disable DMA interrupt */
>>        regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
>>
>> +     isc_set_histogram(isc, false);
>> +
>>        /* let a running IRQ handler finish before the clock is disabled */
>>        synchronize_irq(isc->irq);
>>
>> +     cancel_work_sync(&isc->awb_work);
>> +
>>        pm_runtime_put_sync(isc->dev);
>>
>>        /* Disable stream on the sub device */
>>
> 

[PATCH 05/10] media: microchip-isc: clean up histogram on the start_streaming error path

[Balakrishnan Sambath]

isc_configure() enables the histogram, its ISC_INT_HISDONE interrupt and
a HISREQ before calling isc_update_profile(), which can time out and
fail. When it does, isc_start_streaming() jumps to err_configure and
drops the runtime PM reference without disabling the histogram or
cancelling awb_work. A pending HISDONE can then schedule isc_awb_work(),
which reads the histogram registers after the clocks are gone, causing
an external abort.

Disable the histogram, synchronize the IRQ and flush the work before
dropping the PM reference, mirroring isc_stop_streaming(): the
synchronize_irq() has to precede cancel_work_sync() so a handler that is
still in flight cannot re-queue awb_work after it has been cancelled.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-isc-base.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 8f255a4c4e7a..f7fbd3cd8edc 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -382,6 +382,13 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return 0;
 
 err_configure:
+	isc_set_histogram(isc, false);
+
+	/* let a running IRQ handler finish before the clock is disabled */
+	synchronize_irq(isc->irq);
+
+	cancel_work_sync(&isc->awb_work);
+
 	pm_runtime_put_sync(isc->dev);
 err_pm_get:
 	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);

-- 
2.34.1

[PATCH 06/10] media: microchip-isc: do not touch WB registers when not streaming

[Balakrishnan Sambath]

isc_s_awb_ctrl() called isc_update_awb_ctrls() unconditionally, writing
the white balance registers even when the device is runtime suspended;
on many ARM platforms accessing the unclocked registers is an external
abort. The write was also done without awb_lock, racing isc_awb_work(),
which holds it so the DMA done IRQ cannot latch a half-updated pipeline.

Write the registers only while streaming, under awb_lock, and update the
profile there. When not streaming the new gains and offsets stay cached
in the control state and are programmed by isc_configure() at the next
stream start.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-isc-base.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index f7fbd3cd8edc..2911cfc660a0 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -1509,20 +1509,21 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl)
 		if (ctrl->cluster[ISC_CTRL_GB_OFF]->is_new)
 			ctrls->offset[ISC_HIS_CFG_MODE_GB] = isc->gb_off_ctrl->val;
 
-		isc_update_awb_ctrls(isc);
-
 		mutex_lock(&isc->awb_mutex);
 		if (vb2_is_streaming(&isc->vb2_vidq)) {
-			/*
-			 * If we are streaming, we can update profile to
-			 * have the new settings in place.
-			 */
+			unsigned long flags;
+
+			/* awb_lock serialises the WB writes against the IRQ */
+			spin_lock_irqsave(&isc->awb_lock, flags);
+			isc_update_awb_ctrls(isc);
+			spin_unlock_irqrestore(&isc->awb_lock, flags);
+
 			isc_update_profile(isc);
 		} else {
 			/*
-			 * The auto cluster will activate automatically this
-			 * control. This has to be deactivated when not
-			 * streaming.
+			 * Not streaming: keep the cached values for the next
+			 * stream start and deactivate the cluster-activated
+			 * do_white_balance button.
 			 */
 			v4l2_ctrl_activate(isc->do_wb_ctrl, false);
 		}

-- 
2.34.1

[PATCH 07/10] media: microchip-isc: fix pfe_cfg0_bps comment

[Balakrishnan Sambath]

The @pfe_cfg0_bps comment claimed the field holds the "number of
hardware data lines connected to the ISC". It does not. The field
stores the pre-shifted PFE_CFG0 BPS value (e.g. ISC_PFE_CFG0_BPS_EIGHT,
which is 0x4 << 28) and is ORed straight into the PFE_CFG0 register
word in microchip-isc-base.c.

The old wording invites a reader to treat it as a small bit-depth
integer (8, 10, 12) and compare or do arithmetic on it directly, which
silently breaks since the value is shifted into bits 30:28. Document
what the field really holds and how to read the bit-depth back out with
FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...).

Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-isc.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
index f5e322c2e36b..b084459f4583 100644
--- a/drivers/media/platform/microchip/microchip-isc.h
+++ b/drivers/media/platform/microchip/microchip-isc.h
@@ -62,7 +62,11 @@ struct isc_subdev_entity {
  * @mbus_code:		V4L2 media bus format code.
  * @cfa_baycfg:		If this format is RAW BAYER, indicate the type of bayer.
 			this is either BGBG, RGRG, etc.
- * @pfe_cfg0_bps:	Number of hardware data lines connected to the ISC
+ * @pfe_cfg0_bps:	Pre-shifted ISC_PFE_CFG0 BPS field value (e.g.
+			ISC_PFE_CFG0_BPS_EIGHT), not a plain bit-depth integer.
+			OR it directly into the PFE_CFG0 register word, or use
+			FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...) to obtain the
+			3-bit BPS field value.
  * @raw:		If the format is raw bayer.
  */
 

-- 
2.34.1

Re: [PATCH 07/10] media: microchip-isc: fix pfe_cfg0_bps comment

[Eugen Hristev]

On 6/16/26 14:51, Balakrishnan Sambath wrote:
> The @pfe_cfg0_bps comment claimed the field holds the "number of
> hardware data lines connected to the ISC". It does not. The field
> stores the pre-shifted PFE_CFG0 BPS value (e.g. ISC_PFE_CFG0_BPS_EIGHT,
> which is 0x4 << 28) and is ORed straight into the PFE_CFG0 register
> word in microchip-isc-base.c.
> 
> The old wording invites a reader to treat it as a small bit-depth
> integer (8, 10, 12) and compare or do arithmetic on it directly, which
> silently breaks since the value is shifted into bits 30:28. Document
> what the field really holds and how to read the bit-depth back out with
> FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...).

In this commit message I would focus on the correct meaning of
pfe_cfg0_bps and stop inferring what a reader *might* have understood.
Let's just fix it to be right, explain the right way, and forget the
wrong way.

But it looks like we could improve this to hold an actual meaningful
data here, instead of a preshifted value, and rather shift it to the
register when the hardware needs it.

Eugen

> 
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> ---
>  drivers/media/platform/microchip/microchip-isc.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
> index f5e322c2e36b..b084459f4583 100644
> --- a/drivers/media/platform/microchip/microchip-isc.h
> +++ b/drivers/media/platform/microchip/microchip-isc.h
> @@ -62,7 +62,11 @@ struct isc_subdev_entity {
>   * @mbus_code:		V4L2 media bus format code.
>   * @cfa_baycfg:		If this format is RAW BAYER, indicate the type of bayer.
>  			this is either BGBG, RGRG, etc.
> - * @pfe_cfg0_bps:	Number of hardware data lines connected to the ISC
> + * @pfe_cfg0_bps:	Pre-shifted ISC_PFE_CFG0 BPS field value (e.g.
> +			ISC_PFE_CFG0_BPS_EIGHT), not a plain bit-depth integer.
> +			OR it directly into the PFE_CFG0 register word, or use
> +			FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...) to obtain the
> +			3-bit BPS field value.
>   * @raw:		If the format is raw bayer.
>   */
>  
> 

Re: [PATCH 07/10] media: microchip-isc: fix pfe_cfg0_bps comment

[Balakrishnan.S]

Hi Eugen,

On 21/06/26 11:57 am, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 6/16/26 14:51, Balakrishnan Sambath wrote:
>> The @pfe_cfg0_bps comment claimed the field holds the "number of
>> hardware data lines connected to the ISC". It does not. The field
>> stores the pre-shifted PFE_CFG0 BPS value (e.g. ISC_PFE_CFG0_BPS_EIGHT,
>> which is 0x4 << 28) and is ORed straight into the PFE_CFG0 register
>> word in microchip-isc-base.c.
>>
>> The old wording invites a reader to treat it as a small bit-depth
>> integer (8, 10, 12) and compare or do arithmetic on it directly, which
>> silently breaks since the value is shifted into bits 30:28. Document
>> what the field really holds and how to read the bit-depth back out with
>> FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...).
> 
> In this commit message I would focus on the correct meaning of
> pfe_cfg0_bps and stop inferring what a reader *might* have understood.
> Let's just fix it to be right, explain the right way, and forget the
> wrong way.

Sure. Will fix the comment to just say what the field holds.

> 
> But it looks like we could improve this to hold an actual meaningful
> data here, instead of a preshifted value, and rather shift it to the
> register when the hardware needs it.

Yes that seems better. Will store the actual bps value and shift it at 
the PFE_CFG0 write where its needed, will that work ?

If so will fix both in next version.

Thanks,
Balakrishnan

> 
> Eugen
> 
>>
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>> ---
>>   drivers/media/platform/microchip/microchip-isc.h | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
>> index f5e322c2e36b..b084459f4583 100644
>> --- a/drivers/media/platform/microchip/microchip-isc.h
>> +++ b/drivers/media/platform/microchip/microchip-isc.h
>> @@ -62,7 +62,11 @@ struct isc_subdev_entity {
>>    * @mbus_code:               V4L2 media bus format code.
>>    * @cfa_baycfg:              If this format is RAW BAYER, indicate the type of bayer.
>>                        this is either BGBG, RGRG, etc.
>> - * @pfe_cfg0_bps:    Number of hardware data lines connected to the ISC
>> + * @pfe_cfg0_bps:    Pre-shifted ISC_PFE_CFG0 BPS field value (e.g.
>> +                     ISC_PFE_CFG0_BPS_EIGHT), not a plain bit-depth integer.
>> +                     OR it directly into the PFE_CFG0 register word, or use
>> +                     FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...) to obtain the
>> +                     3-bit BPS field value.
>>    * @raw:             If the format is raw bayer.
>>    */
>>
>>
> 

Re: [PATCH 07/10] media: microchip-isc: fix pfe_cfg0_bps comment

[Eugen Hristev]

On 6/22/26 14:32, Balakrishnan.S@microchip.com wrote:
> Hi Eugen,
> 
> On 21/06/26 11:57 am, Eugen Hristev wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 6/16/26 14:51, Balakrishnan Sambath wrote:
>>> The @pfe_cfg0_bps comment claimed the field holds the "number of
>>> hardware data lines connected to the ISC". It does not. The field
>>> stores the pre-shifted PFE_CFG0 BPS value (e.g. ISC_PFE_CFG0_BPS_EIGHT,
>>> which is 0x4 << 28) and is ORed straight into the PFE_CFG0 register
>>> word in microchip-isc-base.c.
>>>
>>> The old wording invites a reader to treat it as a small bit-depth
>>> integer (8, 10, 12) and compare or do arithmetic on it directly, which
>>> silently breaks since the value is shifted into bits 30:28. Document
>>> what the field really holds and how to read the bit-depth back out with
>>> FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...).
>>
>> In this commit message I would focus on the correct meaning of
>> pfe_cfg0_bps and stop inferring what a reader *might* have understood.
>> Let's just fix it to be right, explain the right way, and forget the
>> wrong way.
> 
> Sure. Will fix the comment to just say what the field holds.
> 
>>
>> But it looks like we could improve this to hold an actual meaningful
>> data here, instead of a preshifted value, and rather shift it to the
>> register when the hardware needs it.
> 
> Yes that seems better. Will store the actual bps value and shift it at 
> the PFE_CFG0 write where its needed, will that work ?

Try it out, and if it's better, yes, send it in the next version. If
it's not looking good, reply with a snippet/reason why

Eugen
> 
> If so will fix both in next version.
> 
> Thanks,
> Balakrishnan
> 
>>
>> Eugen
>>
>>>
>>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>>> ---
>>>   drivers/media/platform/microchip/microchip-isc.h | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
>>> index f5e322c2e36b..b084459f4583 100644
>>> --- a/drivers/media/platform/microchip/microchip-isc.h
>>> +++ b/drivers/media/platform/microchip/microchip-isc.h
>>> @@ -62,7 +62,11 @@ struct isc_subdev_entity {
>>>    * @mbus_code:               V4L2 media bus format code.
>>>    * @cfa_baycfg:              If this format is RAW BAYER, indicate the type of bayer.
>>>                        this is either BGBG, RGRG, etc.
>>> - * @pfe_cfg0_bps:    Number of hardware data lines connected to the ISC
>>> + * @pfe_cfg0_bps:    Pre-shifted ISC_PFE_CFG0 BPS field value (e.g.
>>> +                     ISC_PFE_CFG0_BPS_EIGHT), not a plain bit-depth integer.
>>> +                     OR it directly into the PFE_CFG0 register word, or use
>>> +                     FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...) to obtain the
>>> +                     3-bit BPS field value.
>>>    * @raw:             If the format is raw bayer.
>>>    */
>>>
>>>
>>
> 

[PATCH 08/10] media: microchip-isc: fix PM runtime leak in AWB work handler

[Balakrishnan Sambath]

Early return when streaming stops skips pm_runtime_put_sync(),
leaking the reference and preventing runtime suspend.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
Reviewed-by: Eugen Hristev <ehristev@kernel.org>
---
 drivers/media/platform/microchip/microchip-isc-base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 2911cfc660a0..a09d6d1ca7e5 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -1430,7 +1430,7 @@ static void isc_awb_work(struct work_struct *w)
 	/* streaming is not active anymore */
 	if (isc->stop) {
 		mutex_unlock(&isc->awb_mutex);
-		return;
+		goto out_pm_put;
 	}
 
 	isc_update_profile(isc);
@@ -1441,6 +1441,7 @@ static void isc_awb_work(struct work_struct *w)
 	if (ctrls->awb)
 		regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ);
 
+out_pm_put:
 	pm_runtime_put_sync(isc->dev);
 }
 

-- 
2.34.1

[PATCH 09/10] media: microchip-isc: fix SBGGR10 Bayer pattern

[Balakrishnan Sambath]

SBGGR10 was mapped to ISC_BAY_CFG_RGRG instead of ISC_BAY_CFG_BGBG,
causing red/blue channel swap.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
Reviewed-by: Eugen Hristev <ehristev@kernel.org>
---
 drivers/media/platform/microchip/microchip-sama5d2-isc.c | 2 +-
 drivers/media/platform/microchip/microchip-sama7g5-isc.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
index 5d49b9d48f57..f0954a1f6376 100644
--- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
@@ -147,7 +147,7 @@ static struct isc_format sama5d2_formats_list[] = {
 		.fourcc		= V4L2_PIX_FMT_SBGGR10,
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR10_1X10,
 		.pfe_cfg0_bps	= ISC_PFG_CFG0_BPS_TEN,
-		.cfa_baycfg	= ISC_BAY_CFG_RGRG,
+		.cfa_baycfg	= ISC_BAY_CFG_BGBG,
 	},
 	{
 		.fourcc		= V4L2_PIX_FMT_SGBRG10,
diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index 4f9e9a5ed4d1..59532fdecca3 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -156,7 +156,7 @@ static struct isc_format sama7g5_formats_list[] = {
 		.fourcc		= V4L2_PIX_FMT_SBGGR10,
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR10_1X10,
 		.pfe_cfg0_bps	= ISC_PFG_CFG0_BPS_TEN,
-		.cfa_baycfg	= ISC_BAY_CFG_RGRG,
+		.cfa_baycfg	= ISC_BAY_CFG_BGBG,
 	},
 	{
 		.fourcc		= V4L2_PIX_FMT_SGBRG10,

-- 
2.34.1

[PATCH 10/10] media: microchip-isc: fix WB offset and gain register field masking

[Balakrishnan Sambath]

ISC_WB_O_* and ISC_WB_G_* pack two 13-bit fields per register. Sign
extension from negative offsets corrupts the upper field. Mask both
fields to 13 bits before packing.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 .../media/platform/microchip/microchip-isc-base.c   | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index a09d6d1ca7e5..19c371458eee 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -61,18 +61,23 @@ static inline void isc_update_awb_ctrls(struct isc_device *isc)
 
 	/* In here we set our actual hw pipeline config */
 
+	/*
+	 * Mask offset fields to 13 bits. Sign extension of negative s32
+	 * values would otherwise corrupt the adjacent field.
+	 */
 	regmap_write(isc->regmap, ISC_WB_O_RGR,
-		     ((ctrls->offset[ISC_HIS_CFG_MODE_R])) |
-		     ((ctrls->offset[ISC_HIS_CFG_MODE_GR]) << 16));
+		     ((u32)ctrls->offset[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
+		     (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
 	regmap_write(isc->regmap, ISC_WB_O_BGB,
-		     ((ctrls->offset[ISC_HIS_CFG_MODE_B])) |
-		     ((ctrls->offset[ISC_HIS_CFG_MODE_GB]) << 16));
+		     ((u32)ctrls->offset[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
+		     (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
+	/* Gains are 13-bit unsigned fields [12:0] and [28:16] */
 	regmap_write(isc->regmap, ISC_WB_G_RGR,
-		     ctrls->gain[ISC_HIS_CFG_MODE_R] |
-		     (ctrls->gain[ISC_HIS_CFG_MODE_GR] << 16));
+		     (ctrls->gain[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
+		     ((ctrls->gain[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
 	regmap_write(isc->regmap, ISC_WB_G_BGB,
-		     ctrls->gain[ISC_HIS_CFG_MODE_B] |
-		     (ctrls->gain[ISC_HIS_CFG_MODE_GB] << 16));
+		     (ctrls->gain[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
+		     ((ctrls->gain[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
 }
 
 static inline void isc_reset_awb_ctrls(struct isc_device *isc)

-- 
2.34.1