linux-kernel - Re: [PATCH 16/19] media: i2c: ds90ub960: Enable SSCG for UB9702

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z4jYPHQx6C6rQw2U@kekkonen.localdomain>
Date: Thu, 16 Jan 2025 09:58:20 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org, Devarsh Thakkar <devarsht@...com>,
	Jai Luthra <jai.luthra@...asonboard.com>
Subject: Re: [PATCH 16/19] media: i2c: ds90ub960: Enable SSCG for UB9702

Moi,

On Wed, Jan 15, 2025 at 06:04:40PM +0200, Tomi Valkeinen wrote:
> On 15/01/2025 16:26, Sakari Ailus wrote:
> > Moi,
> > 
> > On Fri, Jan 10, 2025 at 11:14:16AM +0200, Tomi Valkeinen wrote:
> > > From: Jai Luthra <jai.luthra@...asonboard.com>
> > > 
> > > UB9702 supports spread-spectrum clock generation for the back-channel
> > > clock, which is futher used by serializers in synchronous mode to
> > > generate the forward-channel clock, which can help reduce peak EMI
> > > energy. The SSCG is common to all RX ports, so it can only be used if
> > > all the ports are in the same mode.
> > > 
> > > Add basic support for SSCG by adding a module parameter to enable the
> > > SSCG. The SSCG uses hardcoded configurationg, with 0.5% center-spread at
> > > 33kHz modulation rate. See datasheet if different configuration is
> > > required.
> > > 
> > > Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
> > > ---
> > >   drivers/media/i2c/ds90ub960.c | 102 ++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 102 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> > > index f6d6c2fe12cd..a534d077f045 100644
> > > --- a/drivers/media/i2c/ds90ub960.c
> > > +++ b/drivers/media/i2c/ds90ub960.c
> > > @@ -52,6 +52,10 @@
> > >   #include <media/v4l2-fwnode.h>
> > >   #include <media/v4l2-subdev.h>
> > > +static bool ub960_enable_sscg;
> > > +module_param_named(enable_sscg, ub960_enable_sscg, bool, 0644);
> > > +MODULE_PARM_DESC(enable_sscg, "Enable SSCG (if available)");
> > 
> > Shouldn't this come from DT instead?
> 
> SSCG is an option to use or not to use, based on what the user wants. DT
> should describe the hardware.
> 
> A module parameter is bad for this, though, as it's then used for all ub960
> devices... But I'm not sure what other options we have. We need to have it
> at probe time.
> 
> Probably the whole driver could be changed to not connect the serializers at
> probe, but instead would offer a set of userspace APIs to enable/disable
> SSCG, and to enable the links. But that brings in its own set of problems,
> as the links are used for i2c communication to the ser and sensor, not to
> mention new userspace APIs which always complicates things.

Typically determining whether you need spread spectrum signalling is highly
specific to the board. I wonder if this topic has been discussed (and
possibly somehow resolved) for other subsystems. CSI-2 appears to allow for
spread spectrum clocks but I don't think I've seen that implemeted
anywhere, scrambling may be more popular (but also that is right now
unsupported but could be supported, probably this would be best to consider
in conjunction with other CSI-2 parameters).

-- 
Terveisin,

Sakari Ailus