[0/1] media: i2c: alvium: add camera sysfs attributes

Hi All,
With this patch I'm going to add some sysfs attributes to the alvium-csi2 driver.
This patch adds the following sysfs attributes:

 - cci_register_layout_version:
   Shows current cci regs layout version of the camera.

 - csi_clock_mhz:
   Shows current alvium camera csi2 freq.

 - device_capabilities:
   Show the capabilities of the current camera.

 - device_guid:
   Shows the current device guid as programmed by the vendor during production.

 - device_version:
   Shows the version of the alvium hardware.

 - family_name:
   Shows the Alvium family name, like Alvium CSI-2, GM2, FP3, ...

 - genio:
   Generic camera input/output xfer for using user programmable part of the flash.
   Reading and writing camera's user programmable flash memory.

 - lane_count:
   Shows device current CSI2 camera's lanes number.

 - manufacturer_info:
   Shows manufacturer info as programmed by the vendor during production.

 - manufacturer_name:
   Shows manufacturer name as programmed by the vendor during production.

 - mode:
   As stated by the BCRM Ref Manual camera can work in 2 modes BCM/GENCP.
   This attribute is responsible for switching the camera mode and check the current mode.

 - model_name:
   Shows model name as programmed by the vendor during production.

 - serial_number:
   Shows camera serial number as programmed by the vendor during production.

 - user_defined_name:
   Shows camera user defined name as programmed by the vendor during production.

Thanks & Regards,
Tommaso

Tommaso Merciai (1):
  media: i2c: alvium: add camera sysfs attributes

 drivers/media/i2c/alvium-csi2.c | 484 ++++++++++++++++++++++++++++++++
 drivers/media/i2c/alvium-csi2.h |  25 ++
 2 files changed, 509 insertions(+)
  

Hi Tommaso,

On Mon, Sep 09, 2024 at 12:58:29PM +0200, Tommaso Merciai wrote:
> Hi All,
> With this patch I'm going to add some sysfs attributes to the alvium-csi2 driver.

Why ? You need to convince us that this shouldn't be done with existing
V4L2 mechanisms instead, like controls, or VIDIOC_LOG_STATUS. I expect
the answer to depend on the attributes.

> This patch adds the following sysfs attributes:
> 
>  - cci_register_layout_version:
>    Shows current cci regs layout version of the camera.
> 
>  - csi_clock_mhz:
>    Shows current alvium camera csi2 freq.
> 
>  - device_capabilities:
>    Show the capabilities of the current camera.
> 
>  - device_guid:
>    Shows the current device guid as programmed by the vendor during production.
> 
>  - device_version:
>    Shows the version of the alvium hardware.
> 
>  - family_name:
>    Shows the Alvium family name, like Alvium CSI-2, GM2, FP3, ...
> 
>  - genio:
>    Generic camera input/output xfer for using user programmable part of the flash.
>    Reading and writing camera's user programmable flash memory.

Complete and absolute NACK. We don't expose direct I2C access to
registers.

>  - lane_count:
>    Shows device current CSI2 camera's lanes number.
> 
>  - manufacturer_info:
>    Shows manufacturer info as programmed by the vendor during production.
> 
>  - manufacturer_name:
>    Shows manufacturer name as programmed by the vendor during production.
> 
>  - mode:
>    As stated by the BCRM Ref Manual camera can work in 2 modes BCM/GENCP.
>    This attribute is responsible for switching the camera mode and check the current mode.

You will need to be very, very convincing to get a writable sysfs
attribute accepted.

>  - model_name:
>    Shows model name as programmed by the vendor during production.
> 
>  - serial_number:
>    Shows camera serial number as programmed by the vendor during production.
> 
>  - user_defined_name:
>    Shows camera user defined name as programmed by the vendor during production.
> 
> Thanks & Regards,
> Tommaso
> 
> Tommaso Merciai (1):
>   media: i2c: alvium: add camera sysfs attributes
> 
>  drivers/media/i2c/alvium-csi2.c | 484 ++++++++++++++++++++++++++++++++
>  drivers/media/i2c/alvium-csi2.h |  25 ++
>  2 files changed, 509 insertions(+)

All sysfs attributes are ABIs, and need to be documented in
Documentation/ABI/. I expect you'll need one patch per attribute. Don't
forget to CC linux-doc@vger.kernel.org on the patches.
  

Hi Tommaso,

On Mon, Sep 09, 2024 at 12:58:29PM +0200, Tommaso Merciai wrote:
> Hi All,
> With this patch I'm going to add some sysfs attributes to the alvium-csi2 driver.
> This patch adds the following sysfs attributes:
> 
>  - cci_register_layout_version:
>    Shows current cci regs layout version of the camera.
> 
>  - csi_clock_mhz:
>    Shows current alvium camera csi2 freq.
> 
>  - device_capabilities:
>    Show the capabilities of the current camera.
> 
>  - device_guid:
>    Shows the current device guid as programmed by the vendor during production.
> 
>  - device_version:
>    Shows the version of the alvium hardware.
> 
>  - family_name:
>    Shows the Alvium family name, like Alvium CSI-2, GM2, FP3, ...
> 
>  - genio:
>    Generic camera input/output xfer for using user programmable part of the flash.
>    Reading and writing camera's user programmable flash memory.
> 
>  - lane_count:
>    Shows device current CSI2 camera's lanes number.
> 
>  - manufacturer_info:
>    Shows manufacturer info as programmed by the vendor during production.
> 
>  - manufacturer_name:
>    Shows manufacturer name as programmed by the vendor during production.
> 
>  - mode:
>    As stated by the BCRM Ref Manual camera can work in 2 modes BCM/GENCP.
>    This attribute is responsible for switching the camera mode and check the current mode.
> 
>  - model_name:
>    Shows model name as programmed by the vendor during production.
> 
>  - serial_number:
>    Shows camera serial number as programmed by the vendor during production.
> 
>  - user_defined_name:
>    Shows camera user defined name as programmed by the vendor during production.

I think most these would be better implemented as V4L2 controls. Some
appear to be internal to the driver and not matter from actual user space
implementation point of view, such as CCI register layout version and
possibly device capabilities to some extent. What would be the reason to
expose these to the user space?

What are the BCM and GENCP modes?

If there's a need to program the device's memory, the NVM interface would
seem like a better fit for that. Presumably this would be only accessible
for the root?

The lane count control should probably be standardised, there are other
devices that could benefit from it. The LINK_FREQ control already exists,
it conveys the (CSI-2) link frequency to the user.