Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1721645 > unrolled thread

Re: [PATCH] nokia n900: update dts with camera support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] nokia n900: update dts with camera support Pavel Machek <pavel@ucw.cz> - 2017-08-28 16:10 +0200
    Re: [PATCH] nokia n900: update dts with camera support Sakari Ailus <sakari.ailus@iki.fi> - 2017-08-28 17:10 +0200
      Re: [PATCH] nokia n900: update dts with camera support Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> - 2017-08-28 20:30 +0200

#1721645 — Re: [PATCH] nokia n900: update dts with camera support

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > > Well, strobe property above already uses = <0>/<1> format, as do
> > > others.
> >
> > > Problem with "false = property does not exist" is that you don't know
> > > if it is "someone forgot to define it" or "someone made a typo" or
> > > "dts is too old to know about this property" or "the property indeed
> > > should be false"...
> >
> > As this is an established practice, I think we should follow it for bool
> > properties.
> 
> Yes it's common practice, there is also device_property_read_bool()
> to get the value.
> 
> > We could change the existing ones, too, and leave some extra checks in
> > place to handle old dtbs.
> 
> The following should be downward compatible:
> 
> var = read_bool();
> if (var && !read_int())
>     var = false;
> 
> Btw. DT people should be CC'd for DT binding additions/changes.

Ok, so for now we do crc=1 by default. Now we'd like to have crc
support configurable in the dts. But if we just introduce "crc;"
option, it will break old dts users. We could introduce "no-crc;" and
that would work in this particular case, but will break when we want
different defaults at different devices.

Anyway, introducing "no-crc;" seems pretty ugly to me. I'd rather do
"crc=<0>;", in a similar way we handle other options now.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1721719

Hi Pavel,

On Mon, Aug 28, 2017 at 03:59:59PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > Well, strobe property above already uses = <0>/<1> format, as do
> > > > others.
> > >
> > > > Problem with "false = property does not exist" is that you don't know
> > > > if it is "someone forgot to define it" or "someone made a typo" or
> > > > "dts is too old to know about this property" or "the property indeed
> > > > should be false"...
> > >
> > > As this is an established practice, I think we should follow it for bool
> > > properties.
> > 
> > Yes it's common practice, there is also device_property_read_bool()
> > to get the value.
> > 
> > > We could change the existing ones, too, and leave some extra checks in
> > > place to handle old dtbs.
> > 
> > The following should be downward compatible:
> > 
> > var = read_bool();
> > if (var && !read_int())
> >     var = false;
> > 
> > Btw. DT people should be CC'd for DT binding additions/changes.
> 
> Ok, so for now we do crc=1 by default. Now we'd like to have crc
> support configurable in the dts. But if we just introduce "crc;"
> option, it will break old dts users. We could introduce "no-crc;" and
> that would work in this particular case, but will break when we want
> different defaults at different devices.
> 
> Anyway, introducing "no-crc;" seems pretty ugly to me. I'd rather do
> "crc=<0>;", in a similar way we handle other options now.

The same issue actually exists for CSI-2: the CRC is typically enabled and
I think you can at least disable the check (in OMAP3 ISP). Do we have a
need to disable it at the moment for any purpose? I think I've seen this
was disabled somewhere but it may just as well be a thoughtlessly written
configuration.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

[toc] | [prev] | [next] | [standalone]

#1721890

Hi

On 28.08.2017 18:07, Sakari Ailus wrote:
> Hi Pavel,
> 
> On Mon, Aug 28, 2017 at 03:59:59PM +0200, Pavel Machek wrote:
>> Hi!
>>
>>>>> Well, strobe property above already uses = <0>/<1> format, as do
>>>>> others.
>>>>
>>>>> Problem with "false = property does not exist" is that you don't know
>>>>> if it is "someone forgot to define it" or "someone made a typo" or
>>>>> "dts is too old to know about this property" or "the property indeed
>>>>> should be false"...
>>>>
>>>> As this is an established practice, I think we should follow it for bool
>>>> properties.
>>>
>>> Yes it's common practice, there is also device_property_read_bool()
>>> to get the value.
>>>
>>>> We could change the existing ones, too, and leave some extra checks in
>>>> place to handle old dtbs.
>>>
>>> The following should be downward compatible:
>>>
>>> var = read_bool();
>>> if (var && !read_int())
>>>      var = false;
>>>
>>> Btw. DT people should be CC'd for DT binding additions/changes.
>>
>> Ok, so for now we do crc=1 by default. Now we'd like to have crc
>> support configurable in the dts. But if we just introduce "crc;"
>> option, it will break old dts users. We could introduce "no-crc;" and
>> that would work in this particular case, but will break when we want
>> different defaults at different devices.
>>
>> Anyway, introducing "no-crc;" seems pretty ugly to me. I'd rather do
>> "crc=<0>;", in a similar way we handle other options now.
> 
> The same issue actually exists for CSI-2: the CRC is typically enabled and
> I think you can at least disable the check (in OMAP3 ISP). Do we have a
> need to disable it at the moment for any purpose? I think I've seen this
> was disabled somewhere but it may just as well be a thoughtlessly written
> configuration.
> 

Front camera has crc disabled in Nokia kernel. I am not sure the sensor 
supports crc at all.

Regards,
Ivo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web