On Sat, Jul 26, 2025 at 06:06:05AM +0000, Hardevsinh Palaniya wrote:

quoted

Quoting Hardevsinh Palaniya (2025-07-25 06:55:23)
<snip>

quoted

quoted

quoted

+static int ov2735_page_access(struct ov2735 *ov2735,
+�������������������������� u32 reg, void *val, int *err, bool is_read)
+{
+���� u8 page = (reg >> CCI_REG_PRIVATE_SHIFT) & 0xff;

' & 0xff' part is redundant.

quoted

+���� u32 addr = reg & ~CCI_REG_PRIVATE_MASK;
+���� int ret = 0;

How is this assignment being used?

quoted

+���� if (err && *err)
+������������ return *err;
+
+���� mutex_lock(&ov2735->page_lock);
+
+���� /* Perform page access before read/write */
+���� if (ov2735->current_page != page) {
+������������ ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, page, err);
+������������ if (ret)
+�������������������� goto err_mutex_unlock;
+������������ ov2735->current_page = page;
+���� }
+
+���� if (is_read)
+������������ ret = cci_read(ov2735->cci, addr, (u64 *)val, err);
+���� else
+������������ ret = cci_write(ov2735->cci, addr, *(u64 *)val, err);

Do you really need this castings?

Do you really think this casting is unnecessary?

Yes? Well quite probably - I haven't checked myself yet but ..

quoted

Please check the definitions of cci_read/write

without this, we can't even build the driver.

How about ... changing the function prototype of ov2735_page_access ?

Of course, changing the function prototype would work.

My intention is to keep a single ov2735_page_access() function that can
handle both read and write operations. The cci_read() function expects 
a u64 *, whereas cci_write() expects a u64 value. To support both cases
within one function, I’ve used a void *val and cast it appropriately 
depending on the operation.

If we were to remove the casting, we would need to split this into two
separate functions, one for read and one for write, even though the only 
difference between them would be a single line. I’d prefer to avoid that
redundancy and keep the code compact. 

Let me know if you see a better way to handle this without duplicating
the logic. 

Move the cci_read() and cci_write() calls from the bottom of
ov2735_page_access() to ov2735_read() and ov2735_write() respectively.

-- 
Regards,

Laurent Pinchart